Repository navigation
docs(adr-2793): decide to take the pending-ack permit at the head - #2979
Conversation
|
@claude-fleet review |
|
Review task queued. Task: 30d15adf-a604-494a-8f39-26d1e8bfb90d |
![claude-fleet[bot]](https://avatars.githubusercontent.com/in/4876296?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: two should_fix findings on the amendment's acceptance table and the head mode resolution. The decision itself is sound, the measurement is internally consistent, and the doc guards pass.
Verified:
- Shared budget claim holds: one
Arc<PendingAckBudget>is built atservices/ravel-server/src/lib.rs:2373and cloned into the three routers at:2425,:2509,:2544, so "the bound, the gauge and the shed counter keep one source" is true of the budget the layer would take from. - Gauge and shed counter are properties of the budget itself (
crates/ravel-ingest/src/pending_ack.rs:60-88), so moving acquisition to the head automatically moves the gauge to "admission to ack" as the amendment states; the permit is RAII (pending_ack.rs:102-107), which is what makes "Submittedholds the permit throughacks()and the deadline, as today" unchanged. resolve_write_modeis header-only and so is resolvable at the head (services/ravel-server/src/ingest_mode.rs:14-27); the gRPC side reads the same values throughmetadata_to_headers(otlp_grpc.rs:72).admit_grpc_requestdoes take the in-flight permit itself on the direct-call path (ingest_admission.rs:290-300).- Defaults 8,192 and 1,024 (
config.rs:1741-1765), and0on the in-flight flag really is unlimited (config.rs:6512-6522). - Body-read timeout 30 s, matching the drain amendment's own exclusion at line 429.
- Measurement table is self-consistent with the throughput sentence: 6.2/466 = 13.3 ms CPU per post-decode refusal, 3.8/12,859 = 0.30 ms at the head, difference 13.0 ms against the stated 12.97 ms; 1.25 ms at 1,000 series clears the pre-registered 1 ms rule.
- The amendment markers are satisfied: decision 4 carries the inline pointer at line 244-245.
Checks run:
scripts/guards/check-amendment-integrity.sh→ clean, 340 amendments over 186 filesscripts/guards/check-adr-band-sources.sh→ clean, 8 band tables, 62 rows, 1 exempt
Smaller things:
- The amendment names no doc consequences, unlike decision 4, which enumerated the sentences that go stale with it.
docs/ingest.md:21-23("A strictsubmittakes a pending-ack permit ... before it charges the byte budget or touches a shard") and the metric descriptions atdocs/ingest.md:1985-1989("strict-modesubmits currently holding a pending-ack permit", "strictsubmits refused") both become false for admission-layer writes when #2805 lands, and the repo rule is that they change in the same commit.
Not checked:
- Issue #2902 comment 6156727395 and issue #2805 comment 6119242471: no network access, so the measurement figures and the pre-registration outcome were checked only for internal consistency against the throughput sentence in the amendment.
- The #2926 drain amendment on the base branch is taken as given; only its interaction with this amendment (the 30 s exclusion and the drain-on-head-refusal claim) was read.
Task 30d15adf-a604-494a-8f39-26d1e8bfb90d on rp1, model claude-opus-5, effort medium.
|
|
||
| | Setup | Expected | Rules out | | ||
| |---|---|---| | ||
| | Budget 1. One strict ack is held by a gated data PUT. A second strict request has a body that cannot be decoded. | The second request gets the pending-ack 429 text, not a 400. | The refusal is still taken in `submit`, so the 400 comes first. A check-only test at the head followed by acquisition in `submit`, so the decode runs. | |
There was a problem hiding this comment.
should_fix: Acceptance row 1 does not rule out the second wrong implementation it names. With budget 1 already held, a head that only CHECKS availability and acquires in submit sees zero available at the head, refuses there with the pending-ack 429, and never decodes, so the test passes on exactly the implementation it is supposed to reject.
Failure: an implementation that checks at the head and acquires in submit ships green; two strict requests that both pass the check while the budget is free then both decode, and one is refused after a full decode, which is the cost this amendment exists to remove.
Evidence: line 539 of the amendment; the budget's acquire is a single try_acquire that both refuses and takes in one step (crates/ravel-ingest/src/pending_ack.rs:60-77), so a check-only head is a distinct, writable implementation.
Fix: add a row where the budget is NOT exhausted at the head — budget 1, no ack held, one strict request parked between the head and submit (gated decode or gated PUT), a second strict request must be refused at the head. Under check-only the second is admitted and decoded.
| surfaces, the admission layer takes the pending-ack permit at the request | ||
| head. It does this immediately after the in-flight permit, and only when | ||
| the write mode resolved from the head is strict. That mode comes from | ||
| `resolve_write_mode`, which reads the header or the gRPC metadata; Remote |
There was a problem hiding this comment.
should_fix: "That mode comes from resolve_write_mode ... Remote Write is always strict" leaves Remote Write's head mode underspecified against the middleware both surfaces share. admit_ingest_request is one generic middleware instantiated for GatewayState and RemoteWriteState (services/ravel-server/src/otlp_http.rs:128-131, services/ravel-server/src/remote_write.rs:252-255); wiring resolve_write_mode into it uniformly reads the x-ravel-ingest-mode header on the Remote Write route too, where the handler ignores that header entirely and always writes with state.strict_flush_policy.write_mode() (remote_write.rs:506-513, module doc at remote_write.rs:4-8).
Failure: a Remote Write request carrying x-ravel-ingest-mode: buffered resolves as buffered at the head, takes no pending-ack permit, and is then submitted as a strict write; since a pre-admitted write never takes a second permit in the router, the strict ack wait is unbounded and uncounted, and any client can opt every Remote Write out of the bound with one header. No acceptance row covers it (row 3 pins buffered on HTTP OTLP and gRPC only).
Fix: state that the head mode is a property of the surface, not of the header — the admission state supplies it, and the Remote Write instantiation returns strict unconditionally — and add an acceptance row: budget 1 held, a Remote Write request with x-ravel-ingest-mode: buffered must be refused with the pending-ack 429.
| rule pre-registered on #2805 was to implement at 1 ms or more on the large | ||
| body. The outcome is recorded in comment 6119242471. | ||
|
|
||
| **Decision.** On the OTLP HTTP routes, on Remote Write, and on the gRPC |
There was a problem hiding this comment.
nit: "On the OTLP HTTP routes, on Remote Write, and on the gRPC surfaces, the admission layer takes the pending-ack permit at the request head" sweeps in OTAP, which line 512 then excludes. OTAP is served behind the same gRPC admission layer, which claims its path and authenticates the stream head without taking a permit (services/ravel-server/src/ingest_admission.rs:426, lib.rs:4020-4034).
Failure: an implementer reading the normative sentence takes one pending-ack permit per OTAP stream head, held for the stream's whole lifetime, with the mode resolved once from stream metadata that says nothing about the per-batch writes.
Evidence: lines 479-480 against lines 508-520; ingest_admission.rs:426 and the test at ingest_admission.rs:874.
Fix: write "on the OTLP gRPC surfaces (not OTAP; see below)", the way the in-flight permit rule is already qualified.
|
|
||
| **What the bound now counts.** A pre-admitted permit is held from the head | ||
| through the body read and the decode, then through the ack wait. The read | ||
| can take up to the body-read timeout (30 s). How many requests hold one |
There was a problem hiding this comment.
nit: "How many requests hold one before submit is bounded by the in-flight ceiling" is stated unconditionally, but --max-inflight-ingest-requests 0 is a documented, supported value meaning unlimited (services/ravel-server/src/config.rs:6512-6522, flag doc at config.rs:1739-1741).
Failure: with the in-flight ceiling at 0 and the pending-ack bound finite, nothing bounds the number of requests holding a pending-ack permit while reading a body for up to 30 s, so strict writes can be shed while zero acks are actually pending. The next sentence accepts that configuration as "tighter, not unsafe", which is a different claim from the bound asserted here.
Fix: qualify as "bounded by the in-flight ceiling when that ceiling is set".
| | Budget 1. One strict ack is held by a gated data PUT. A second strict request has a body that cannot be decoded. | The second request gets the pending-ack 429 text, not a 400. | The refusal is still taken in `submit`, so the 400 comes first. A check-only test at the head followed by acquisition in `submit`, so the decode runs. | | ||
| | Budget 2, PUT held. Two concurrent strict writes. | Both are admitted, the gauge reads 2, and a third is refused. | The permit is acquired at the head and again in `submit`, so the second is refused. The permit is released at `submit`, so the third is admitted. | | ||
| | Budget 1, one ack held. A `buffered` request on HTTP and on gRPC. | 200 / OK on both. | Every mode takes a permit. The mode is read from the wrong header source on one transport. | | ||
| | Budget 1, PUT held. The direct-call gRPC path with no layer. | The second request is refused. | The no-layer path takes no permit. The bare permit is dropped along with tonic's extensions. | |
There was a problem hiding this comment.
nit: Acceptance row 4's expected outcome does not separate its first wrong implementation. An implementation that misses the direct-call path at the head still acquires in submit under the rule at line 508-510, so with budget 1 already held by the gated PUT the second request is refused either way, just after decode.
Failure: the row passes on a tree where the no-layer gRPC path never refuses at the head, which is the property the row claims to pin. (It does separate the extensions-drop implementation, since that one frees the first permit and admits the second.)
Evidence: line 542 against lines 507-510.
Fix: assert the refusal happens before decode, for instance by asserting the decode counters stay at zero, the way the drain amendment's acceptance does at line 443.
| is taken inside `submit`, after decode and before the byte charge, so a | ||
| is taken inside `submit`, after decode and before the byte charge (moved | ||
| to the request head by the head pending-ack amendment below), so a | ||
| refusal there has paid a full decode and holds no charge. It is a bound |
There was a problem hiding this comment.
nit: The parenthetical qualifies only the clause the amendment-supersedes marker names. The rest of the same sentence, "so a refusal there has paid a full decode and holds no charge", still reads as current behaviour for the admission-layer surfaces, where after #2805 a pending-ack refusal pays no decode at all.
Failure: a reader of decision 4 who stops at the end of the sentence takes the post-decode cost as the live property; the integrity guard passes because it only checks the phrase the marker names.
Evidence: lines 243-246; check-amendment-integrity.sh exits 0 on the current text.
Fix: extend the parenthetical to the end of the clause, or move it after "holds no charge".
4e0bb28 to
9cf9a3a
Compare
|
Round 1 findings, addressed in 9cf9a3a (rebased onto the revised drain amendment):
@claude-fleet review |
|
Review task queued. Task: 08e6aa18-f3b9-42b0-a57c-1770e5c9de94 |
There was a problem hiding this comment.
Reviewed the amendment against the code it describes. The six findings from the previous round are all addressed: acceptance row 1 now parks a request between the head and submit so a check-only head fails it, Remote Write's head mode is pinned to the surface rather than the header and has its own row, the normative sentence excludes OTAP, the in-flight bound is qualified with "While the in-flight ceiling is set", row 6 asserts the refusal precedes decode, and the decision-4 parenthetical names the move.
Four new things, all in the amendment's own text rather than its decision. The decision itself holds up against the tree.
Verified:
- Credential check sits right after the in-flight permit in both axum and gRPC admission, so "right after the in-flight permit and the credential check" is placeable —
ingest_admission.rs:254-265,ingest_admission.rs:430-451. - Remote Write is strict unconditionally and reads no mode header —
remote_write.rs:4-10,remote_write.rs:513. resolve_write_modeis the single function both OTLP HTTP and gRPC handlers use —ingest_mode.rs:14-27,otlp_http.rs:231,327,416,otlp_grpc.rs:72.admit_grpc_requesttakes the in-flight permit itself when no layer ran —ingest_admission.rs:290-301.- One
Arc<PendingAckBudget>backs the bound, the gauge and the shed counter, and its acquire is a singletry_acquire—pending_ack.rs:43-88. PendingAckPermitreleases on ack, deadline or drop ofSubmitted—pending_ack.rs:89-106.- OTAP refuses on the in-flight ceiling and the byte rate before
decode_gated, and each tears the stream down, whilePendingAckLimitReachednacks with the stream open —otap_grpc.rs:161-220,otap_grpc.rs:257-295. - Defaults 8,192 and 1,024, with
0meaning unlimited on the in-flight flag —config.rs:1741,config.rs:1759-1762,config.rs:6513-6525. - Body-read timeout is 30 s —
ingest_admission.rs:118. - All four follow-through targets exist as described:
docs/ingest.md:21,docs/ingest.md:1985-1988, the gauge HELP atmetrics.rs:3056-3061, the flag help atconfig.rs:1744-1762.
Checks run:
scripts/guards/check-amendment-integrity.sh→ clean, 340 amendments over 186 files. Both markers resolve: theDecisionsection carries the "head pending-ack amendment" pointer at line 245, and the retired phrase appears nowhere else.scripts/guards/check-adr-band-sources.sh→ clean, 8 band tables, 62 rows. The new CPU table is not a band table by the guard's header rule, but every row carries a source anyway.- No revert check: docs only.
Smaller things:
- Both amendment tables cite
comment 6156727395for different measurements (the drain amendment's x86_64 transport-error row at line 391, and every row of the new CPU table). Plausible if one comment reported both runs, but I could not open it. - The mermaid node
router submit with held permitis on the path a buffered request takes too, where no permit was taken.
Not checked:
- The measured figures themselves: issue #2902 comment 6156727395 (CPU per post-decode refusal, the 466/12859 requests per second and the core counts) and the #2805 pre-registration outcome in comment 6119242471 are not reachable from this workspace.
- Whether comment 6156727395 genuinely sources both the drain amendment's transport-error row at line 391 and every row of the new CPU table.
- Whether
GrpcIngestAdmissionLayercan reach aStrictFlushPolicyto resolve the mode at the gRPC head: the layer is a concrete struct carrying only a controller, resolver and service set (ingest_admission.rs:354-359), so the amendment's "the admission state each surface instantiates the shared layer with" describes the axum trait, not this one. I did not trace whether wiring the policy in is mechanical. - The generated flag reference docs/reference/ravel-server-flags.md was not opened.
Findings outside the diff:
- docs/adrs/2793-strict-flush-on-arrival-and-permit-release.md:235 (nit): Decision 4's plainest statement of the retired property is still unqualified, two sentences before the one this amendment qualified.
Failure: a reader of decision 4 reaches "submittakes a pending-ack permit for a strict write before sending" at line 235 and takes it as current for every surface, which it is not after #2805 for anything behind the admission layer. The integrity guard passes because theamendment-supersedesmarker names only the line 243-244 phrase.
Evidence: lines 234-237 against lines 243-246 and the amendment's bullet at line 544 ("A pre-admitted write never takes a second permit in the router").
Fix: carry the same pointer here, e.g. "submittakes a pending-ack permit for a strict write before sending (see the head pending-ack amendment below: a write pre-admitted at the head arrives holding one)".
Task 08e6aa18-f3b9-42b0-a57c-1770e5c9de94 on pmoust-mbp, model claude-opus-5, effort medium.
| decoder state for every refused batch. A stream's batches share one | ||
| connection and one decoder, so wasting the decode is the cheaper of the two | ||
| costs. The OTAP stream head takes no pending-ack permit: its mode is read | ||
| once per stream and says nothing about each batch's write. |
There was a problem hiding this comment.
should_fix: The reason given for the OTAP stream head taking no permit is false: the mode read once at the stream head IS the mode of every batch's write.
Failure: an implementer who accepts this reason concludes the stream-head mode is unusable for per-batch decisions, and a later change that does make the stream head take a permit (or that adds per-batch mode metadata) gets judged against a premise the code contradicts. The real reason is a count mismatch, not a mode mismatch: one stream serves many batches, so one permit per stream would be held for the whole stream lifetime while bounding an unbounded number of writes.
Evidence: otap_grpc.rs:98 resolves the mode from the stream-open metadata into ctx.mode; otap_grpc.rs:107 stores it on StreamCtx; otap_grpc.rs:228-230 passes that same ctx.mode to submit_batch for every batch, and otap_grpc.rs:485 is where it reaches submit_values. otap_grpc.rs:86-89 states the per-connection design outright.
Fix: replace the clause with the holding reason, e.g. "its mode covers every batch on the stream, so one permit at the head would be held for the stream's whole lifetime while bounding an unbounded number of writes".
| | Budget 1, one ack held. A `buffered` request on HTTP and on gRPC. | 200 and OK. | Taking a permit for every mode. Reading the mode from the wrong source on one transport. | | ||
| | Budget 1, one ack held. A Remote Write request carrying `x-ravel-ingest-mode: buffered`. | The pending-ack 429. | Reading the header on Remote Write, which would let any client opt out of the bound. | | ||
| | Budget 1, PUT held. A direct-call gRPC request with no layer and an undecodable body. | `RESOURCE_EXHAUSTED`, with the decode never reached. | A no-layer path that takes no permit at the head: it refuses only after decode. A bare permit dropped with tonic's extensions: the second request is admitted. | | ||
| | Budget 1, one ack held. A refusal on HTTP/1 with a body below the drain cap. | A readable 429, and the in-flight gauge back to its prior value before the drain completes. | A drain that runs while the in-flight permit is still held. | |
There was a problem hiding this comment.
should_fix: Acceptance row 7 asserts against an in-flight gauge that does not exist, so the row cannot be written as stated.
Failure: the implementer either adds a new metric the amendment's follow-through never mentions, or quietly drops the assertion and ships the row as a plain "readable 429" check, which no longer rules out the drain-while-holding-the-permit implementation it names.
Evidence: IngestConcurrencyController exposes only shed_total() (ingest_concurrency.rs:120-142); /metrics carries ravel_ingest_concurrency_shed_total and no in-flight request gauge (metrics.rs:3035-3041; the full ravel_ingest_* set has ravel_ingest_in_flight_flushes, which counts flushes, not admission permits).
Fix: assert the observable property instead, for instance that a second request admitted concurrently with the drain succeeds (the slot was returned), or name the gauge as something the change must add and list it under follow-through.
| | Budget 2, PUT held. Two concurrent strict writes. | Both admitted, gauge 2, and a third refused. | Acquiring at the head and again in `submit`: the second is refused. Releasing at `submit`: the third is admitted. | | ||
| | Budget 1, one ack held. A `buffered` request on HTTP and on gRPC. | 200 and OK. | Taking a permit for every mode. Reading the mode from the wrong source on one transport. | | ||
| | Budget 1, one ack held. A Remote Write request carrying `x-ravel-ingest-mode: buffered`. | The pending-ack 429. | Reading the header on Remote Write, which would let any client opt out of the bound. | | ||
| | Budget 1, PUT held. A direct-call gRPC request with no layer and an undecodable body. | `RESOURCE_EXHAUSTED`, with the decode never reached. | A no-layer path that takes no permit at the head: it refuses only after decode. A bare permit dropped with tonic's extensions: the second request is admitted. | |
There was a problem hiding this comment.
should_fix: Acceptance row 6 pins "the decode never reached" on a path that has no decode to reach, so the row is not constructible.
Failure: the direct-call path is a caller invoking the service method in-process with an already-typed tonic::Request<ExportMetricsServiceRequest>; there is no wire body and no "undecodable body" to supply. In the other reading, a tonic server with the layer not installed, tonic's codec decodes the message before the handler is entered at all, so an undecodable body fails in the codec and never produces RESOURCE_EXHAUSTED. Under either reading the row cannot be written, and the wrong implementation it names (a no-layer path that takes no permit at the head) goes unseparated again, which is what the previous round asked this row to fix.
Evidence: otlp_grpc.rs:50-64 takes an already-decoded Request<ExportMetricsServiceRequest> and calls admit_grpc_request before into_inner(); ingest_admission.rs:283-285 defines the direct-call path as "a unit test that invokes a service method without the listener's layer stack".
Fix: assert the work after the head is never reached by something observable on this path, e.g. that normalization and the submit never ran (router/ingest counters unmoved, byte-rate charge not taken), rather than by a decode that does not happen here.
| pending-ack permit before it charges the byte budget; | ||
| - the `docs/ingest.md` metric descriptions of the pending-ack gauge and | ||
| shed counter, which describe `submit`; | ||
| - the gauge's HELP text; |
There was a problem hiding this comment.
should_fix: The follow-through list misses two in-tree doc comments that state the superseded property, both in ravel-ingest rather than ravel-server.
Failure: the code change lands with the list satisfied and both comments still asserting that submit is where a strict write takes its pending-ack permit, which after this amendment is false for every admission-layer write. The next reader of the budget's own module doc gets the retired model from the most authoritative-looking place.
Evidence: crates/ravel-ingest/src/pending_ack.rs:12-14 ("A strict submit takes a permit before charging the byte budget and before touching any shard, so a refused submit holds no byte charge") and crates/ravel-ingest/src/log_router.rs:517 ("A strict submit takes a pending-ack permit before the byte ...").
Fix: add both to the list, qualified the way the docs/ingest.md entries are (true for callers outside the admission layer, pre-admitted writes carry a permit in).
|
Round 2 findings, addressed in f78eee7:
@claude-fleet review |
|
Review task queued. Task: a5ba3f74-40d0-4c2b-b83a-9ca0ca994652 |
There was a problem hiding this comment.
Reviewed the two commits on this branch against 3ed0bf4 (the drain amendment's tip), and checked each claim against the code it describes.
All ten findings from the previous round are addressed in the current text: row 1 now exercises an unexhausted budget, Remote Write strictness is stated as a surface property with its own acceptance row, the normative sentence excludes OTAP explicitly, the in-flight bound is qualified with "while the in-flight ceiling is set", the direct-call row asserts "normalization and submit never reached", the undecodable-body row moved onto a path that has a decode, the OTAP reason is now the count mismatch rather than the mode, the drain row asserts the observable slot return instead of a nonexistent gauge, and the follow-through list gained the two ravel-ingest doc comments.
One should_fix remains, on the follow-through list again, for a different set of sites.
Verified:
- Remote Write never reads
x-ravel-ingest-mode: the handler writes withstate.strict_flush_policy.write_mode()and the module doc says so — remote_write.rs:4-10, 513;resolve_write_modehas no Remote Write caller. resolve_write_modeis one function over aHeaderMap, used by the OTLP HTTP handlers and the three OTLP gRPC handlers — ingest_mode.rs:14-27, otlp_http.rs:231/327/416, otlp_grpc.rs:72.- The gRPC admission layer carries controller, resolver and registered services, and no strict flush policy, so it does need to gain one — ingest_admission.rs:355-396.
- The direct-call path takes the in-flight permit in
admit_grpc_request— ingest_admission.rs:290-301, otlp_grpc.rs:56-64. - OTAP's in-flight and byte-rate refusals happen before
ctx.decoder.decodeand tear the stream down, while the pending-ack refusal is in the arm set that keeps it open — otap_grpc.rs:144-169, 201-208, 255-300. - OTAP's stream mode does cover every batch: resolved once at otap_grpc.rs:86-98, carried on
StreamCtx, passed to everysubmit_batch. - The 30 s body-read timeout and its "head admitted before the body arrives" ordering — ingest_admission.rs:104-118, 249-279, and the stalled-body test at 1045-1130.
- Defaults 8,192 and 1,024 — docs/reference/ravel-server-flags.md:94, 98.
- One
submitper admission-layer request on every surface, so one head permit suffices: ingest.rs:339, logs_ingest.rs:469, traces_ingest.rs:303, remote_write.rs:510.submit_columnar, the second strict acquire site inlog_router.rs:876, has onlyravel-cli loadand test callers, which the amendment excludes. - The measurement table is internally consistent with the prose: 6.2/466 = 13.3 ms against 3.8/12,859 = 0.30 ms, a 13.0 ms difference matching the 12.97 ms band row.
- The existing pending-ack e2e tests still hold after the change (same status, body, counter and gauge value at the scrape point) — log_ingest_e2e.rs:723-839.
Checks run:
scripts/guards/check-amendment-integrity.sh→ exit 0, 340 amendments over 186 files.scripts/guards/check-adr-band-sources.sh→ exit 0, 8 band tables, 62 rows, so the new measurement table's sources parse.check-doc-figures.sh,check-claim-single-source.sh,check-superseded-figures.sh→ all exit 0.
Smaller things:
- Line 537 says "the state answers" for gRPC, but line 534 has just established that the gRPC side has no admission state and gets the policy on the layer. Worth saying "the layer" there.
- Line 237 wraps at 90 columns where the paragraph it is in wraps at 76-80.
docs/reference/ravel-server-flags.md:94(the--max-inflight-ingest-requestshelp) says--max-pending-strict-acks"bounds those waits", which becomes an understatement once the bound also covers the read and decode window. That help is already on the drain amendment's follow-through list, so it will be edited anyway.
Not checked:
- The #2902 measurement itself and issue comments 6156727395 and 6119242471: no network access, so the band table's figures and sources are taken as given (they are internally consistent with the 466/12,859 requests per second and 6.2/3.8 core prose).
- The #2805 pre-registered 1 ms rule and the maintainer approval state described in the pull request body.
Task a5ba3f74-40d0-4c2b-b83a-9ca0ca994652 on rp1, model claude-opus-5, effort medium.
| **Follow-through in the code change.** Some text in the tree stops being true | ||
| for admission-layer writes and must change in the same commit as the code: | ||
|
|
||
| - the `docs/ingest.md` sentence saying that a strict `submit` takes a |
There was a problem hiding this comment.
should_fix: The follow-through list misses the two passages that state the retired property most plainly, and one of its two docs/ingest.md entries is ambiguous between two sentences that both match it.
Failure: the code change lands with the list satisfied while docs/ingest.md's admission section still tells an operator that the pending-ack shed happens after decode and does not replace the head shed, which is exactly the property this amendment retires for every admission-layer surface.
Evidence: docs/ingest.md:110-112 ("A strict write takes its slot after decode, as its points are submitted") and docs/ingest.md:116 ("comes after decode, so it does not replace the head shed above as the memory bound") are a third passage, matched by neither list entry: entry 1 describes the byte-charge sentence and entry 2 the metric descriptions at docs/ingest.md:1985-1988. docs/guides/operations/configuration.md:741-744 ("bounds how many strict writes may wait for their ack at once") is a fourth, in the operator guide. Separately, entry 1's wording fits both docs/ingest.md:21 and docs/ingest.md:1662-1664, and the latter also carries "it is a refusal after decode, not a head shed like the in-flight request ceiling above", a claim entry 1 does not describe at all.
Fix: list the admission section and the "Pending strict-ack budget" section separately by their claims, and add the configuration guide.
| | Budget 2, PUT held. Two concurrent strict writes. | Both admitted, gauge 2, and a third refused. | Acquiring at the head and again in `submit`: the second is refused. Releasing at `submit`: the third is admitted. | | ||
| | Budget 1, one ack held. A `buffered` request on HTTP and on gRPC. | 200 and OK. | Taking a permit for every mode. Reading the mode from the wrong source on one transport. | | ||
| | Budget 1, one ack held. A Remote Write request carrying `x-ravel-ingest-mode: buffered`. | The pending-ack 429. | Reading the header on Remote Write, which would let any client opt out of the bound. | | ||
| | Budget 1, PUT held. A direct-call gRPC request, with no layer, invoking the service method in-process. | `RESOURCE_EXHAUSTED`, with normalization and `submit` never reached: the router and ingest counters do not move. | A no-layer path that takes no permit at the head: it refuses only after decode. A bare permit dropped with tonic's extensions: the second request is admitted. | |
There was a problem hiding this comment.
nit: Row 6's second wrong implementation is not separated unless the setup says which surface holds the budget.
Failure: the row's request under test is a direct call, where admit_grpc_request takes the permit in the handler and nothing is inserted into the request extensions, so the extensions-drop bug is never exercised by it. If the "PUT held" permit is held by an HTTP request or another direct call, the extensions-drop implementation refuses the second request exactly like the correct one, and the row passes on it. Only a holder that came through the gRPC layer makes the implementation observable.
Evidence: ingest_admission.rs:290-301 (the direct-call branch takes the permit itself); decision 4 at lines 211-213 of this file states the same for the in-flight permit.
Fix: say the held permit belongs to a strict gRPC request admitted through the layer.
| - The layer takes the permit from the same `Arc<PendingAckBudget>` the | ||
| routers hold, so the bound, the gauge and the shed counter keep one | ||
| source. | ||
| - The permit travels to the handler the way the in-flight release handle |
There was a problem hiding this comment.
nit: "The permit travels to the handler the way the in-flight release handle does" names a transport the permit type does not support, and drops the property that makes the in-flight version safe.
Failure: an implementer following the sentence literally puts a PendingAckPermit in the request extensions. It does not implement Clone, which extensions require, so the compile fails; the nearest thing that does compile is a shared handle with no clone retained by the layer, and that is the "bare permit dropped with tonic's extensions" implementation row 6 names, since into_inner drops the extensions mid-handler and the permit releases before the write is even enqueued.
Evidence: PendingAckPermit is a Drop-only RAII guard with no Clone (crates/ravel-ingest/src/pending_ack.rs:91-108); ingest_admission.rs:88-102 states why the in-flight permit travels as a clonable release handle and why the layer holds a clone of its own.
Fix: state the two properties rather than the analogy: the permit needs its own clonable release handle, the layer keeps a clone for the request's lifetime, and the handler reads it before request.into_inner().
| with, and the gRPC admission layer through the strict flush policy, which | ||
| it does not carry today and gains in this change: | ||
|
|
||
| - On OTLP HTTP and gRPC the state answers with `resolve_write_mode` over |
There was a problem hiding this comment.
nit: "the same function the handler uses, so the head and the handler cannot disagree" holds on gRPC only if the head reads the metadata the way the handler does, which the text does not say.
Failure: the gRPC layer holds raw http::HeaderMap, while the handler calls resolve_write_mode over metadata_to_headers(request.metadata()), which drops binary and non-ASCII entries and any name that fails HeaderName parsing. Calling the same function over two different header maps is not the same input, and the ADR's stated guarantee rests on the input, not the function.
Evidence: otlp_grpc.rs:31-48 and otlp_grpc.rs:70-72; grpc_head_tenant exists precisely to apply that round trip at the head so head and handler agree (ingest_admission.rs:319-331).
Fix: say the head resolves the mode from the metadata the way grpc_head_tenant resolves the tenant, through MetadataMap::from_headers and metadata_to_headers.
| unlimited). `submit` takes a pending-ack permit for a strict write before | ||
| sending, and `Submitted` holds it until the acks resolve or the deadline | ||
| sending (see the head pending-ack amendment below: a write admitted at the | ||
| head arrives holding one), and `Submitted` holds it until the acks resolve or the deadline |
There was a problem hiding this comment.
nit: The reflowed sentence runs to 90 columns where the file wraps at 80.
Failure: a later edit to this paragraph re-wraps it and produces a diff larger than the change, obscuring what was edited.
Evidence: line 237 is 90 characters; every other prose line in the paragraph is under 80, and the only other over-length lines in the file are headings, markers, table rows and mermaid.
f78eee7 to
bbaeab5
Compare
|
Round 3 findings, addressed in bbaeab5, rebased onto the drain amendment's 8d43896. This was the third round, so there is no further review request; the PR goes to the maintainer.
|
Amends ADR-2793 decision 4 for #2805. a head refusal on an 8 vCPU x86_64 executor: 1.25 ms at 1,000 series and 12.97 ms at 10,000, past the 1 ms rule pre-registered on #2805. The amendment decides that the admission layer takes the pending-ack permit at the request head for strict writes on the OTLP HTTP routes, Remote Write and gRPC, from the routers' own budget, and hands it to the router, which holds it to the ack without taking a second one. OTAP keeps the post-decode refusal, since a pre-decode refusal there ends the stream. It names four acceptance tests and the wrong implementations each rules out. The code lands under #2805, after the #2926 drain. Refs: #2793 Refs: #2805 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
Review round 2 of the head pending-ack amendment. The OTAP stream head takes no permit because one stream serves unbounded writes, not because its mode differs per batch. The direct-call gRPC row asserts that normalization and submit never run, since that path has no wire body to decode. The drain-ordering row asserts that a concurrent request is admitted, since no in-flight gauge exists. The gRPC layer gains the strict flush policy it needs to decide strictness at the head, and the follow-through list names the ravel-ingest doc comments on the strict submit. Decision 4's plainest statement of the submit-time permit now carries the pointer too. Refs: #2793 Refs: #2805 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
Review round 3 of the head pending-ack amendment. The permit travels in its own clonable release handle, since the permit guard has no Clone and cannot sit in request extensions; the layer keeps a clone and the handler reads its own before into_inner. The gRPC head resolves the mode from the metadata round trip the handler sees, as it already does for the tenant. The direct-call row names a layer-admitted holder so the extensions-drop implementation is observable. The follow-through list names each docs/ingest.md passage by its claim and adds the operator configuration guide. Refs: #2793 Refs: #2805 Changelog: none Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
bbaeab5 to
65c548b
Compare
1 participant
Amends ADR-2793 decision 4 for #2805. Docs only; stacked on the #2926 drain amendment (base branch
docs/adr-2793-head-drain). The code lands under #2805 after the #2926 code.#2902 measured the extra CPU per post-decode pending-ack refusal over a head refusal on an 8 vCPU x86_64 executor: 1.25 ms at 1,000 series and 12.97 ms at 10,000 (comment 6156727395). That is past the 1 ms rule pre-registered on #2805 (outcome in comment 6119242471).
The amendment decides:
PendingAckBudget, so the bound, the gauge and the shed counter keep one source.ravel-cli load, OTAP, tests) keep acquiring insidesubmit.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: #2805