Repository navigation
docs(adr-2793): decide that head refusals drain the request body - #2978
Conversation
Amends ADR-2793 decision 4 for #2926 and sets its status to Accepted. A head refusal on an HTTP/1 ingest route answers before the body is read. hyper then closes a socket with unread bytes in its receive queue, the kernel resets it, and the reset can discard the 429: about 13% of refusals reached a pooled client as a broken pipe once the body passed a socket buffer (#2926). The amendment decides that head refusals read and discard the body before answering, bounded at 1 MiB, 1 s and 64 concurrent drains, with no decode, no budget charge and no permit held. A refusal past any bound keeps today's behaviour plus Connection: close and is counted by reason. gRPC and OTAP are excluded with their reasons. The code lands under #2926. Refs: #2793 Refs: #2926 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
@claude-fleet review |
|
Review task queued. Task: ada695e3-f5aa-49fd-b7fd-8b57c2c399ab |
![claude-fleet[bot]](https://avatars.githubusercontent.com/in/4876296?s=60&v=4){kind=small}
There was a problem hiding this comment.
Docs-only amendment to ADR-2793. I read the whole amendment, the Decision section it narrows, the admission middleware it describes, and the two other docs that state the property it supersedes.
Request changes: four should_fix findings, all in the amendment text. The core decision (drain the body before a head refusal, bounded) is sound and the measurement table backs it; what is missing is the 401 path's acceptance case, the exposure that path creates, a resolvable statement about configurability, and the two non-ADR docs the amendment contradicts.
Verified:
- "The admission middleware answers the in-flight ceiling's 429 before the body extractor runs" and the 401 is the next head decision —
services/ravel-server/src/ingest_admission.rs:251-263; the body wrapper is installed only after both (:265-268). - The body-read timeout is 30 s and answers 503 with
Retry-After—ingest_admission.rs:118,:199-209,:275-278. - "the routes' 16 MiB body limit" —
ingest_decode.rs:24, applied asDefaultBodyLimit::maxoutside the admission middleware atotlp_http.rs:132. - "the three OTLP HTTP routes and Remote Write" —
otlp_http.rs:121-124,remote_write.rs:20-37. - gRPC head refusals are a separate layer, so the exclusion at line 430 matches the code —
ingest_admission.rs:285-296,:471. - The marker pair at lines 371-372 is well formed and the Decision section carries the pointer at line 242.
- Table arithmetic: 262/2000 = 13.1%, consistent with "about 13%" for that row.
Checks run:
scripts/guards/check-amendment-integrity.sh→ exit 0, clean (339 amendments over 186 files).scripts/guards/check-adr-band-sources.sh→ exit 0, clean (the new table is not a band table; it carries a Source column regardless).grep -rn "never buffers\|before the body is read" docs/→ two live occurrences outsidedocs/adrs/, see the finding on line 450.
No revert check: the PR changes no code, so there is no test to prove.
Smaller things:
- The PR description says "About 13% of refusals reached a pooled client as a broken pipe ... (169 of 2,000 ...)". 169/2,000 is 8.5%; the 13% figure comes from the 64,351 B row, not that one. The ADR table itself is correct.
Not checked:
- hyper's HTTP/1 read-side behaviour on an unread body (line 377-379) and the kernel reset it triggers: inferred from the cited measurements, not reproduced here.
- The measurement table's underlying runs (issue #2926 body, comment 6156727395): no network access to the issue tracker, so the figures are taken as cited.
- Issue #2805's plan to move the pending-ack refusal to the request head (line 395).
- The HTTP/2 claim that a refused stream always delivers response headers (line 430-432); the ADR itself marks it as inferred.
Task ada695e3-f5aa-49fd-b7fd-8b57c2c399ab on rp1, model claude-opus-5, effort medium.
| it starts. The concurrency bound is what limits a drain, because the | ||
| admission permits do not: under the overload that produces refusals, and | ||
| for unauthenticated callers on the 401 path, the drains would otherwise | ||
| have no bound on count. |
There was a problem hiding this comment.
should_fix: The amendment puts the 401 on the drain path without stating the exposure that creates, and the exposure can re-create the bug the amendment exists to remove.
Failure: an unauthenticated caller needs no credential to reach the 401 at the request head, so it can occupy all 64 process-wide drain slots for up to 1 s each with up to 1 MiB of body each. While they are held, every authenticated in-flight 429 takes the busy path at line 412, which is exactly today's behaviour, so the refusals the amendment is for are lost as transport errors again. The slots are a single process-wide pool shared between an authenticated path and an unauthenticated one, with no partition stated.
Evidence: the 401 is answered from the request head before the body wrapper is installed, services/ravel-server/src/ingest_admission.rs:258-268; docs/ingest.md:76-83 states the property this removes ("a shed request costs this process the bytes of one request head, an unauthenticated caller cannot make it buffer a body"), which ADR-0051 and issue #1705 put there deliberately; line 405-408 here notes the 401 path is why a bound is needed, but not what the bound still permits.
Fix: state the decision either as a reserved share of the 64 slots for the authenticated path, or as not draining the 401 at all, and say which, with the residual exposure named.
| measured. The acceptance tests below include a gRPC case to pin it. | ||
| - OTAP: its refusals answer inside an open stream. | ||
|
|
||
| **Acceptance.** A pooled-client test sends a body larger than the loopback |
There was a problem hiding this comment.
should_fix: The acceptance list covers only the in-flight 429; the 401 drain has no case.
Failure: the test described runs "while the ceiling is held", so an implementation that drains on the 429 and leaves the 401 exactly as it is today passes every listed case, including the three wrong-implementation rule-outs and the busy unit test, while one of the three refusals the decision names at line 394 still arrives as a broken pipe. The 401 path is also the one with the exposure question above, so it is the case most worth pinning.
Evidence: decision scope at lines 392-396 names 429, 401 and the future pending-ack 429; acceptance at lines 435-452 names no 401 case.
Fix: add a case that sends a body above the socket buffer and below the cap with a bad credential and asserts a readable 401 with zero transport errors.
| times that. Draining 16 MiB per refusal, under the overload that makes the | ||
| refusals, holds a connection task for the transfer time of the largest | ||
| legal body. The time bound is what limits a slow sender, and the | ||
| concurrency bound is what limits the total. A deployment that needs a larger cap can |
There was a problem hiding this comment.
should_fix: The cap's configurability is stated two ways in consecutive sentences.
Failure: "A deployment that needs a larger cap can raise it with its own evidence" and "This amendment does not add a flag for it" cannot both describe the same artifact. An implementer of #2926 cannot tell whether HEAD_REFUSAL_DRAIN_MAX_BYTES is a const like INGEST_BODY_READ_TIMEOUT (ingest_admission.rs:118, documented in docs/ingest.md:94 as "a fixed constant ..., not a flag") or a server flag, and a reviewer of that PR cannot tell which shape satisfies the ADR.
Fix: say which it is. If the intent is "a constant today, a flag only with a later ADR", write that; drop "can raise it".
| A unit test fills the drain slots and asserts that the next refusal takes | ||
| the `busy` path. | ||
|
|
||
| A gRPC case with a large message asserts `RESOURCE_EXHAUSTED`. Tests that |
There was a problem hiding this comment.
should_fix: The follow-through list names the tests #2926 must change but not the two documents it falsifies, and no guard covers them.
Failure: once #2926 lands, docs/ingest.md:80-83 ("a shed request costs this process the bytes of one request head, an unauthenticated caller cannot make it buffer a body") and docs/reference/ravel-server-flags.md:94 ("The permit is taken from the request head, before the body is read or decoded ..., so a shed request never buffers a body") both state the superseded property as current behaviour. docs/ingest.md is the normative doc for ravel-ingest per the doc map, and check-amendment-integrity.sh reads only docs/adrs, so the amendment-supersedes phrase at line 372 catches neither: I ran it and it is clean with both occurrences standing.
Fix: list both file:line in the follow-through sentence at lines 450-452, beside the test changes.
|
|
||
| **Excluded.** | ||
|
|
||
| - The body-read timeout's 503: that body did not arrive within 30 s, so |
There was a problem hiding this comment.
nit: The 503 exclusion's reason holds at the instant the deadline fires, not at the instant the response is written.
Failure: DeadlineBody::poll_frame starts the timeout branch only when the inner body returns Pending, so "nothing to drain" is true for that poll. A trickling sender can put more bytes in the receive queue between that poll and the 503 being written, and content-length still has bytes outstanding, so the connection closes with unread bytes: the same reset class the amendment is about. The exclusion is probably still right (the sender is already past its deadline, and 503 is retried), but the stated reason is not what makes it right.
Evidence: services/ravel-server/src/ingest_admission.rs:166-180 and :275-278.
| - `HEAD_REFUSAL_DRAIN_MAX_BYTES`: 1 MiB of body as sent, before any | ||
| inflate. | ||
| - `HEAD_REFUSAL_DRAIN_TIMEOUT`: 1 s. | ||
| - `HEAD_REFUSAL_DRAIN_MAX_CONCURRENT`: 64 drains at once, process-wide. |
There was a problem hiding this comment.
nit: The concurrency bound is the only one of the three with no derivation.
Failure: lines 418-424 derive 1 MiB from the socket buffer and explain what the 1 s bound limits, but 64 appears with no relation to the ceiling that produces the refusals (--max-inflight-ingest-requests, default 1024) or to the refusal rate at which drains start falling to busy. A later reader cannot tell whether 64 is a measured number or a placeholder, which is the same question line 423 leaves open for the byte cap.
Evidence: lines 398-408 and 418-424; docs/reference/ravel-server-flags.md:94 for the ceiling's default.
| for unauthenticated callers on the 401 path, the drains would otherwise | ||
| have no bound on count. | ||
|
|
||
| A refusal that cannot be drained is answered as it is today, with |
There was a problem hiding this comment.
nit: What Connection: close buys on the undrained path is left for the reader to infer, and the acceptance section says it buys nothing on its own.
Failure: lines 410-413 read as a mitigation ("answered as it is today, with Connection: close added"), while line 441 pins that "Connection: close alone: transport errors stay". An operator reading only the decision will not know that an over_cap, timeout or busy refusal can still reach the client as a broken pipe, which is the whole reason the counter at line 414 is worth alerting on.
Fix: say in the decision paragraph that an undrained refusal may still be lost in transport, and that the counter is how a deployment sees it.
| concurrency bound is what limits the total. A deployment that needs a larger cap can | ||
| raise it with its own evidence. This amendment does not add a flag for it. | ||
|
|
||
| **Excluded.** |
There was a problem hiding this comment.
nit: The Excluded list omits the 413 from the route body limit, the other refusal answered on the head of these same routes.
Failure: DefaultBodyLimit is layered outside the admission middleware and rejects from content-length before any body byte is read, so it has the identical shape. The decision text at line 392 scopes to "the admission layer", which excludes it correctly, but an implementer of #2926 who sees 503, gRPC and OTAP enumerated will ask about 413 and find no answer. It is also over the cap by construction (>16 MiB against a 1 MiB cap), so it is a one-line exclusion.
Evidence: services/ravel-server/src/otlp_http.rs:126-132 (route_layer for admission, .layer(DefaultBodyLimit::max(...)) outside it); services/ravel-server/tests/admission_e2e.rs:223-249.
| - `HEAD_REFUSAL_DRAIN_MAX_CONCURRENT`: 64 drains at once, process-wide. | ||
|
|
||
| A drained body is never inflated, decoded or charged to any budget. The | ||
| drain holds no admission permit: the in-flight permit is released before |
There was a problem hiding this comment.
nit: "The drain holds no admission permit: the in-flight permit is released before it starts" is a decision property with no test behind it.
Failure: on the 401 path the permit is live when the refusal is built, and today it is dropped only after next.run returns (ingest_admission.rs:251, :271-272). An implementation that drains inside that scope still holds a slot for up to 1 s per refusal and nothing in the acceptance list fails. This ADR pins the comparable ordering property elsewhere (line 231, "A test pins that order on OTAP").
Fix: add the ordering to the acceptance list, as a fourth wrong implementation to rule out.
Review of the drain amendment. Draining the 401 and an unauthenticated shed would let a caller with no credential make the server read up to 1 MiB per request and hold every drain slot, undoing ADR-0051's rule that an unauthenticated caller cannot make the server buffer a body. The drain now runs only for a shed request whose credential resolves; the 401 and an unauthenticated shed are answered without one and counted under a new unauthenticated reason. The amendment now states that the three bounds are constants changed only by a later amendment, that Connection: close does not prevent the reset, why the concurrency bound is 64, why the 503 and the 413 are excluded, and which doc and help sentences the code change must update. Refs: #2793 Refs: #2926 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
Round 1 findings, addressed in 3ed0bf4:
@claude-fleet review |
|
Review task queued. Task: 767b025f-d9f8-48a7-a2a6-eb8e98a7d17d |
There was a problem hiding this comment.
Re-review of the fix round (cd82082 → 3ed0bf4). Verdict: request changes — two should_fix, both narrow and both in text the amendment itself added.
The round addressed seven of the eight earlier findings. Draining only an authenticated caller (lines 412-425) is a better answer than the reserved-share split I proposed: it keeps ADR-0051's property intact instead of bounding its loss, and it states the diagnosability cost it buys that with. The constants-not-a-flag wording (396-402), the 64 derivation (448-451), the Connection: close disclaimer (437-439), the 413 exclusion (460-461) and the 503's corrected reason (459) all land. The permit-ordering nit is withdrawn: with the 401 off the drain path, admit_ingest_request returns at ingest_admission.rs:256 before any permit exists, so "a shed request was never given one" (406-407) is true for everything in scope today.
What remains is the follow-through list, which is still shorter than the facts, and the new counter's reason label, which two implementations can satisfy differently.
Verified:
- A shed request holds no permit when the drain would run —
services/ravel-server/src/ingest_admission.rs:254-257. - The credential check is "right after the permit" —
ingest_admission.rs:259-265, anddocs/ingest.md:75-78says the same. docs/reference/ravel-server-flags.mdis generated from the clap help, so namingconfig.rsplus "the generated flag reference rebuilt from it" covers both —docs/reference/ravel-server-flags.md:1-17, help text atservices/ravel-server/src/config.rs:1723-1724.- The 413 is layered outside admission:
.route_layer(admit_ingest_request)then.layer(DefaultBodyLimit::max(...)), so the limit wraps the middleware —services/ravel-server/src/otlp_http.rs:128-132,services/ravel-server/src/remote_write.rs:252-256; both caps are 16 MiB (ingest_decode.rs:24,remote_write.rs:37). - The acceptance case at 482 maps onto an existing test rather than a new shape: the over-ceiling request in
ingest_refuses_overflow_before_reading_the_bodyis already unauthenticated —services/ravel-server/tests/ingest_admission_before_body.rs:375. - Arithmetic in the bounds section: 1 MiB is 16 socket buffers at 64 KB, 64 × 1 MiB is 64 MiB, 1024/64 is 16.
PoisonBodyis gRPC-only, so "stays as it is" is right —ingest_admission.rs:657-661,743.
Checks run:
scripts/guards/check-amendment-integrity.sh→ exit 0, clean over 339 amendments. Theamendment-applies/amendment-supersedesmarkers at lines 371-372 resolve against the single## Decisionheading and the narrowed phrase at line 242.scripts/guards/check-adr-band-sources.sh→ exit 0, clean. The measurement table at 381-386 is not a band table, and each row carries its source.grepfor the superseded phrasings across*.md/*.rs→ the hits aredocs/ingest.md:81,docs/reference/ravel-server-flags.md:94,services/ravel-server/src/config.rs:1723-1724andservices/ravel-server/src/ingest_concurrency.rs:113-116; the first three are named in the follow-through list, the fourth is not.
Not checked:
- The measurement table at lines 381-386: the figures come from issue #2926's body and comment 6156727395, neither reachable from here. The percentages quoted in the PR description are consistent with the counts (262/2000 = 13.1%, 169/2000 = 8.5%).
- hyper's HTTP/1 connection internals: that it polls an unread body once and closes the read side, producing the kernel reset described at lines 376-380. Taken from the measurements.
- ADR-0051 and issue #1705 themselves; the ADR-0051 property is checked only through docs/ingest.md:80-83 and the admission code.
- Whether a build of the published image selects both ravel-server and ravel-ingest-router, which is what decides the axum http2 feature in finding 4.
Task 767b025f-d9f8-48a7-a2a6-eb8e98a7d17d on ci-16gb-fsn1-1, model claude-opus-5, effort medium.
| - a 401 is sent while the client has sent only the request head. | ||
| - A gRPC case with a large message asserts `RESOURCE_EXHAUSTED`. | ||
|
|
||
| **Follow-through in the code change.** Some text in the tree states the old |
There was a problem hiding this comment.
should_fix: The follow-through list is still short of the sentences the amendment falsifies, and it names none of them by line, unlike this ADR's own Consequences section.
Failure: after #2926 lands, services/ravel-server/src/ingest_concurrency.rs:113-116 still says try_admit is called "before the body is read and before tenant resolution ... so a shed request does none of that work". For an authenticated shed both clauses are false: the drain reads the body and the drain decision resolves the tenant. The same applies to services/ravel-server/tests/ingest_admission_before_body.rs:331-333, whose doc claims "no credential check is reached" on the shed path; that test's assertions still pass (its over-ceiling request is unauthenticated, line 375), so the stale claim goes in green and nothing fails. No guard covers either: check-amendment-integrity.sh reads only docs/adrs, and I ran it clean with both sentences standing.
Evidence: ingest_concurrency.rs:113-116; ingest_admission_before_body.rs:331-333 and :375; the precedent for the format is this ADR's Consequences bullet at lines 331-352, which names four comments with file:line.
Fix: add ingest_concurrency.rs:113-116 to the list, and make the test bullet at 494-495 name the credential-check claim as well as the body-read one.
| the connection. It does not prevent the reset, so an undrained refusal can | ||
| still reach the client as a transport error. A deployment sees how often | ||
| this happens through `ravel_ingest_head_refusal_undrained_total`, with a | ||
| `reason` label of `over_cap`, `timeout`, `busy` or `unauthenticated`. |
There was a problem hiding this comment.
should_fix: ravel_ingest_head_refusal_undrained_total's reason carries one label per event, but the four conditions at 431-435 are not mutually exclusive and no precedence is given, and the plain 401 is neither included nor excluded.
Failure: a shed request with no credential declaring a 15 MiB body satisfies both unauthenticated and over_cap, and one taken while the slots are full satisfies busy too. Acceptance line 482 implies unauthenticated wins, but only for a body whose size it does not state, so two implementations both satisfy this ADR and produce different series for the same traffic. Separately, line 421 brings the credential check's own 401 into the decision and line 435 lists "the credential does not resolve" in the fallback, which reads as counting a plain 401 (ceiling never reached) on a counter named for head refusals; a deployment's routine misconfigured-scraper 401s would then swamp the signal the counter exists for at 439-440. The existing e2e case is exactly this shape: a 15 MiB Content-Length with no bearer, tests/ingest_admission_before_body.rs:51 and :375.
Fix: state the precedence (credential first, then cap, then slots, then deadline, or whichever you want) and say explicitly whether a 401 at a healthy ceiling increments the counter.
| authenticated refusal onto the fallback below. So: | ||
|
|
||
| - A shed request whose credential does not resolve gets the 429 with no | ||
| drain. The check costs nothing extra: it reads only the request head. |
There was a problem hiding this comment.
nit: "The check costs nothing extra: it reads only the request head" is a claim about cost, not about side effects, and the check it reuses has one.
Failure: the shed path returns at ingest_admission.rs:256 today, before the credential check, and the module comment at :236-239 makes that deliberate ("shed (429) rather than rejected (401) even when its credentials are also bad"). An implementer who resolves the credential on the shed path by reusing the existing branch also calls state.on_unauthorized(), which is record_request_rejected() on Remote Write (remote_write.rs:242-244, counter at :145-147), so every shed with a bad credential starts counting as a rejection. Nothing in the acceptance list fails.
Evidence: ingest_admission.rs:236-239, :259-265; remote_write.rs:145-147, :242-244.
Fix: one clause saying the shed path's credential resolution is side-effect free and does not touch the surface's rejected-request counter.
| - Draining 16 MiB per refusal would hold a task for the transfer time of | ||
| the largest legal body. | ||
|
|
||
| **Excluded.** |
There was a problem hiding this comment.
nit: The decision is scoped to "an HTTP/1 ingest request" (line 392-393) but the Excluded list does not say what an HTTP/2 request on those same routes does.
Failure: axum::serve serves both HTTP/1 and HTTP/2 through hyper-util's auto builder, so an h2c prior-knowledge client can reach the OTLP HTTP and Remote Write routes whenever axum resolves with http2. An implementer who does not branch on request.version() drains those refusals too, where the reset the amendment is about cannot happen, and those drains occupy slots the HTTP/1 path depends on, pushing HTTP/1 refusals onto the busy fallback. One who does branch has to invent whether that counts on the counter and under what reason.
Evidence: axum-0.8.9/src/serve/mod.rs:17,31 (auto::Builder, "supports both HTTP/1 as well as HTTP/2"); the http2 feature is off in axum's defaults but enabled by services/ravel-ingest-router/Cargo.toml:33, which cargo unifies onto ravel-server in any build selecting both; the listeners are plain axum::serve at services/ravel-server/src/lib.rs:3783 and :4219.
Fix: one Excluded bullet saying an HTTP/2 request is answered without a drain and is not counted.
Review round 2 of the drain amendment. The four undrained conditions overlap, so the amendment orders them (credential, cap, slots, deadline) and the first that applies names the reason. It states that the counter counts sheds only, so a plain 401 is not counted; that the credential check on the shed path calls no unauthorized hook and moves no rejection counter; and that an HTTP/2 request is not drained or counted. The follow-through list gains the try_admit doc comment and the over-ceiling test's doc, both of which a drain makes false. Refs: #2793 Refs: #2926 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
Round 2 findings, addressed in b3041af:
@claude-fleet review |
|
Review task queued. Task: 671bdc62-abfe-4b69-9bc8-bd7e4525696c |
There was a problem hiding this comment.
Docs-only amendment to ADR-2793. Round 1's findings are largely addressed: the drain is now narrowed to an authenticated caller with the ADR-0051 exposure named, the three bounds are stated as constants with "changing any of the three takes a later amendment" replacing the contradictory "can raise it", the fallback reasons have an explicit precedence, Connection: close is said not to prevent the reset, the 401/413/HTTP-2 cases are enumerated, and the follow-through now names config.rs + the generated flag reference, docs/ingest.md, ingest_concurrency.rs and the ingest_admission_before_body.rs doc.
What is left is one internal contradiction the amendment introduces, a vacuous acceptance rule-out, and the merge-state question the PR description itself raises.
Verified:
- The reset mechanism as described: the shed returns at
services/ravel-server/src/ingest_admission.rs:256, before the body wrapper and beforenext.runat :273. The credential check follows the permit at :259-265 and callson_unauthorized()only on the 401 branch, so "no side effect on the shed path" (ADR line 419-422) is implementable as written. - "The check reads only the request head":
TenantResolver::resolve(&HeaderMap)is sync and header-only,crates/ravel-tenant-resolve/src/lib.rs:81-82. - "the routes' 16 MiB body limit", layered outside admission:
otlp_http.rs:126-132(route_layerfor admission,.layer(DefaultBodyLimit::max(MAX_REQUEST_BODY_BYTES))outside),ingest_decode.rs:24= 16 MiB;remote_write.rs:251-256withMAX_COMPRESSED_REQUEST_BODY_BYTES= 16 MiB at :37. Both routes are 16 MiB, so "past the drain cap by construction" holds for both. - The 503 exclusion's new reason:
INGEST_BODY_READ_TIMEOUTis 30 s (ingest_admission.rs:118) andbody_timeout_responseis 503 withRetry-Afterchosen because the exporters retry it (:193-197). The replacement reason no longer rests on thepoll_frameinstant, and the bullet now concedes the trickling sender can still be reset. - Arithmetic of the bounds: 64 × 1 MiB = 64 MiB; 64 is 1/16 of the documented default ceiling 1,024 (
docs/reference/ravel-server-flags.md:94); 16 × 64 KB = 1 MiB. - The follow-through's "generated flag reference rebuilt from it" is real and enforced:
services/ravel-server/tests/cli_reference.rs:1-33,106is a drift check overdocs/reference/ravel-server-flags.md, so that file cannot be left stale onceconfig.rs:1717-1724changes. - The test the follow-through names does keep passing unchanged:
tests/ingest_admission_before_body.rs:331-333claims "no credential check is reached" and its over-ceiling request is unauthenticated (post_head_only(..., None), :375).
Checks run:
scripts/guards/check-amendment-integrity.sh→ clean, 339 amendments over 186 files (so theamendment-applies/amendment-supersedesmarkers at lines 371-372 are satisfied; the Decision pointer is at line 242).scripts/guards/check-adr-band-sources.sh→ clean, 8 band tables, 62 rows.grep -rn "never buffers a body" docs/ services/ crates/→ three live occurrences: this ADR line 242 (now qualified),docs/reference/ravel-server-flags.md:94,services/ravel-server/src/config.rs:1724. The latter two are covered by the follow-through list.- No build or test run: the diff is one markdown file.
Smaller things:
- Line 458-461 derives 64 from "the default in-flight ceiling of 1,024", but the ceiling is a flag and the drain bound a constant, so the one-sixteenth relation holds only at the default. A deployment running
--max-inflight-ingest-requests 64gets a drain pool the size of its whole ceiling. Not worth a bound that reads a flag, but the sentence could say "at the default". - The permit-ordering property at line 405-408 still has no acceptance case, which is fine today: the shed path returns at
ingest_admission.rs:256before anyIngestPermitHandleexists, so "a shed request was never given one" is structural. Only the #2805 pending-ack path will need the test, and that is that issue's.
Not checked:
- hyper's HTTP/1 internals behind "polls the unread body once and closes the read side" (line 377-378) and whether hyper strips a Connection header from an HTTP/2 response: no hyper source in this workspace.
- The measurements in the table at lines 386-390 (issue #2926 body and comment 6156727395): no issue access from here, and no loopback reproduction was run.
- docs/ingest.md's own line numbering for the admission section referenced at ADR line 336, which is pre-existing text this diff does not touch.
Findings outside the diff:
- docs/adrs/2793-strict-flush-on-arrival-and-permit-release.md:348 (should_fix): The amendment contradicts this ADR's own Consequences section about the same sentence in the same file, and neither marker catches it.
Failure: Consequences lines 347-350 instruct the #2793 code change that the--max-inflight-ingest-requestshelp atconfig.rs:1550-1565"keeps its head-shed sentence and gains one saying the permit is released at enqueue". That edit has already landed:services/ravel-server/src/config.rs:1717-1724now carries both the released-at-enqueue sentence and "so a shed request never buffers a body". The amendment's follow-through at line 503-504 then requires that same help text to change. An implementer of #2926 reading the document top to bottom finds one instruction to keep the sentence and one to change it, with no indication which is current. Theamendment-supersedesmarker at line 372 does not reach it, because Consequences says "its head-shed sentence" rather than the phrase the marker names: I ran check-amendment-integrity.sh and it is clean with this bullet standing.
Evidence: ADR lines 347-350 against lines 503-504; the live text at services/ravel-server/src/config.rs:1717-1724; check-amendment-integrity.sh clean (339 amendments, 186 files).
Fix: add an inline pointer to the amendment on the Consequences bullet at line 348, in the form the Decision already uses at line 242, or add "its head-shed sentence" to theamendment-supersedesphrase list so the guard holds it.
Task 671bdc62-abfe-4b69-9bc8-bd7e4525696c on ravel-clickbench-ubuntu-2, model claude-opus-5, effort medium.
| arrive as a readable 429 with `Retry-After`, with zero client transport | ||
| errors. The test must rule out three wrong implementations: | ||
| - `Connection: close` alone: transport errors stay. | ||
| - A drain that decodes: the inner handler must never run. |
There was a problem hiding this comment.
should_fix: The "a drain that decodes" rule-out cannot fail, so the acceptance list does not rule that implementation out.
Failure: the assertion offered is "the inner handler must never run". On the shed path the middleware returns at services/ravel-server/src/ingest_admission.rs:256, before next.run(request) at :273, so no drain implementation placed in this layer can make the inner handler run, whatever it does with the bytes. An implementation that gzip-inflates each drained frame, or charges it to the ADR-0069 byte budget, passes this case unchanged, and lines 398-399 and 405 ("1 MiB of body as sent, before any inflate", "never inflated, decoded or charged to any budget") are the properties the case exists to pin. The follow-through at line 516-518 already names the right assertions for the other test ("pin never decoded and never charged").
Fix: state the rule-out as counters that move only on a decode, the way the existing over-ceiling test does: ravel_ingest_buffered_items_total and the ingest byte-budget charge stay at zero across the drained refusals (tests/ingest_admission_before_body.rs:336-339 for the existing form).
| # ADR-2793: Strict flush-on-arrival ack policy and admission permit release | ||
|
|
||
| Status: Proposed | ||
| Status: Accepted, amended 2026-10-14 (amendments at the end) |
There was a problem hiding this comment.
should_fix: The status line declares the amended ADR Accepted, while the PR description says the current revision is not approved yet.
Failure: the description states "After review round 1 the decision was narrowed ... That revision is awaiting the maintainer's re-approval and must not merge before it." The approval on record is for the first draft, whose decision drained every head refusal; the narrowed decision at lines 412-427 is materially different (the 401 and the unauthenticated shed are no longer drained, and a new diagnosability cost is accepted). If this merges as is, #2926 implements against an ADR whose status asserts an approval its current text never received, and nothing in the document records which revision was approved.
Fix: either hold the merge until the re-approval lands, or make the status name the approved revision (for example "Accepted 2026-10-14; the 2026-10-14 amendment as revised after review round 1 is pending re-approval").
| That client fails either way, and neither response tells it to retry. | ||
|
|
||
| **The fallback.** A refusal that is not drained is answered as it is today: | ||
| the same status, `Retry-After` and body text, with `Connection: close` |
There was a problem hiding this comment.
nit: The fallback's Connection: close is scoped to "a refusal that is not drained", which textually includes the excluded HTTP/2 refusals, where the header is not legal.
Failure: the HTTP/2 bullet at lines 476-479 says such a refusal "is answered without a drain", which makes it a refusal that is not drained, and line 429-431 then adds Connection: close to it. Connection is a connection-specific header field that RFC 9113 section 8.2.2 forbids on HTTP/2; an implementer following the fallback literally emits it on the h2 path, and the surrounding text gives no reason to branch there.
Fix: scope the fallback paragraph to HTTP/1, or say in the HTTP/2 bullet that the response carries no Connection header.
| the same status, `Retry-After` and body text, with `Connection: close` | ||
| added. That happens when: | ||
|
|
||
| - the credential does not resolve; |
There was a problem hiding this comment.
nit: "the credential does not resolve" does not distinguish the shed with a bad credential from the plain 401, so what the 401 carries is left open.
Failure: the counter paragraph at lines 449-451 disambiguates counting ("A 401 at a ceiling with room is not a shed and is not counted"), but nothing disambiguates the header. Read literally, this bullet puts Connection: close on every 401, including the routine misconfigured-scraper 401 at a healthy ceiling, which costs that client a fresh connection per request. Two implementations of #2926 can both satisfy the ADR and differ on it.
Fix: say whether the plain 401 gets Connection: close, the same way line 450 says whether it is counted.
| - A drain with no cap: a request whose `Content-Length` is above the cap | ||
| must get `Connection: close` and move the `over_cap` series by exactly | ||
| one. | ||
| - A unit test fills the drain slots and asserts that the next refusal |
There was a problem hiding this comment.
nit: Three of the four reason values have an acceptance case; timeout has none.
Failure: over_cap is pinned at lines 489-491 ("move the over_cap series by exactly one"), busy at 492-493 and unauthenticated at 494-496, but HEAD_REFUSAL_DRAIN_TIMEOUT (line 400) has no case. An implementation that never arms the deadline, or that lets a stalled sender hold a drain slot past 1 s, passes every listed case; the slot leak it produces is what pushes authenticated refusals onto the busy path, which is the failure line 416-417 names.
Fix: add a case with a chunked body that stalls below the cap and assert a Connection: close response within about the deadline and one increment of the timeout series.
Review round 3 of the drain amendment. The rule-out for a drain that decodes now asserts the decode and charge counters, since the inner handler cannot run on the shed path whatever the drain does. A stalled chunked body gets its own timeout case. The fallback is scoped to HTTP/1, an HTTP/2 refusal carries no Connection header, and a plain 401 is answered exactly as today. Decision 4's Consequences note on the head-shed sentence points at the amendment, and the status line says the revised amendments are pending the maintainer's approval. Refs: #2793 Refs: #2926 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
|
Round 3 findings, addressed in 8d43896. This was the third round, so there is no further review request; the PR goes to the maintainer.
|
1 participant
Amends ADR-2793 decision 4 for #2926 and sets the ADR's status to Accepted. Docs only; the code lands under #2926.
A refusal the admission layer answers at the head of an HTTP/1 ingest request goes out before the body is read. hyper then closes a socket with unread bytes in its receive queue, the kernel resets it, and the reset can discard the 429. About 13% of refusals of a 64,351-byte body reached a pooled client as a broken pipe (#2926); a 6,000-series body lost 169 of 2,000 (8.5%) on an x86_64 executor (comment 6156727395).
The amendment decides:
Connection: close, counted byravel_ingest_head_refusal_undrained_total{reason=over_cap|timeout|busy|unauthenticated}.The maintainer approved the first draft on 2026-10-14, and approved the text as revised over three review rounds the same day.
Refs: #2793
Refs: #2926