Repository navigation
fix(server): surface novelty backpressure as 503 + Retry-After with err:db/NoveltyAtMax - #1719
Conversation
When novelty reaches reindex_max_bytes the backpressure check is a hard error, but it fell through the transact catch-alls to the client as HTTP 400 InvalidTransaction (or a status-only 400 passthrough on the consensus path). A client that retries 5xx and treats 4xx as permanent classified the write as malformed and dropped it — a silent-data-loss shape for sustained ingest while the indexer catches up. Both novelty variants now map to 503 + Retry-After with the already defined but previously unused err:db/NoveltyAtMax code: - TransactError::NoveltyAtMax (novelty already at the ceiling, checked at stage time) and TransactError::NoveltyWouldExceed (commit-time predictive sizing) map to HTTP 503, @type err:db/NoveltyAtMax, Retry-After: 1. Both are the same "indexer must drain" condition; mapping only one leaves the footgun in place for whichever check fires first. - ApiError::status_code() classifies both as 503 (previously the 400 Transact(_) fallthrough). - The consensus boundary flattens ApiError to SubmissionError, and an Execution status alone cannot identify the condition (Raft paths emit other retryable 503s), so a dedicated SubmissionError::NoveltyBackpressure variant carries the identity through. It is unsettled — not cached as a terminal idempotency failure — so a retry with the same key re-executes once novelty drains instead of replaying the refusal for the cache TTL. - Every 503 the server emits now carries Retry-After: 1; all of them are retryable capacity pressure and RFC 9110 says a 503 should tell the client when to come back. - Genuinely invalid transactions keep 400 InvalidTransaction. The docs claimed reaching reindex_max_bytes "blocks commits until reindexed" — it never blocked; it errors. The operations table, the server_defaults doc comment and generated config template, and the ServerConfig field doc now describe the real erroring behavior and the 503 semantics. Regression tests pin status + @type + Retry-After end-to-end through the HTTP transact surface for both variants, plus unit tests for every mapping shape and the Retry-After rule. Fixes #1708
… re-execute Internal review of the novelty-backpressure mapping found the unsettled classification inverted (with a failing probe at the previous HEAD): `record_outcome` skips unsettled errors while the claim guard's commit unconditionally disarms the drop-time InFlight cleanup, so a novelty refusal pinned the idempotency slot as InFlight for the full cache TTL (1 hour by default). A keyed client that obeyed Retry-After got 409 AlreadyInFlight on every retry until then, and status polls reported a definitively dead submission as still in flight — the dropped-write footgun behind one extra round-trip, and worse than the pre-fix state for keyed clients. Settled is both truthful and what makes retry-after-drain work: the refusal is decided before commit construction (no maybe-still-commits ambiguity — the unsettled rule exists for Raft leader transitions, and Raft never produces the variant), and a recorded Failed entry is re-attemptable by design — try_claim_slot replaces it on a matching body hash and re-executes. Pinned three ways: an is_settled unit assertion, a scripted-committer keyed-retry test (refusal, then the same key re-executes and commits), and an HTTP-level keyed-retry test (503 then 503 again on retry, never 409). All three fail with the arm flipped back to false. Also folds in the review's oversized-delta recommendation: a transaction whose delta alone meets or exceeds reindex_max_bytes can never be admitted by draining, so answering 503 + Retry-After would wedge a pipeline on that record at 1s intervals forever. That sub-case (delta >= max, mirroring the commit check current + delta >= max) now maps to HTTP 413 + err:db/NoveltyDeltaTooLarge with no Retry-After — threaded through the consensus boundary as its own typed SubmissionError/ServerError pair, because an Execution status of 413 is ambiguous with the HTTP body-size limit. Drainable cases (at-max, and would-exceed with delta < max) keep 503 + err:db/NoveltyAtMax + Retry-After.
bplatz
left a comment
bplatz
left a comment
There was a problem hiding this comment.
Approving. Verified the bug and the fix rather than reading the writeup.
The bug is real, and worse than #1708 describes. Reverting all six source files with the tests kept, the actual pre-fix response is:
{"error":"Transaction error: Novelty at maximum size, reindexing required",
"status":400, "@type":"err:system/InternalError"}Not err:db/InvalidTransaction — err:system/InternalError. The issue analysed the direct ApiError::Transact path; the consensus-flattened path every HTTP route actually uses degrades further. That confirms the PR's claim that an edge-only mapping change would have been inert.
The delta >= max boundary checks out against commit.rs:643 (current_bytes + delta_bytes >= max_bytes), including the >= rather than >.
The idempotency fix is genuinely pinned. Flipping the is_settled novelty arm to false:
[consensus] keyed_retry_after_novelty_refusal_reexecutes ... FAILED
keyed retry must re-execute and succeed: AlreadyInFlight
[http] novelty_backpressure_keyed_retry_reexecutes_not_409 ... FAILED
{"status":409,"@type":"err:db/CommitConflict"}
Both levels reproduce the 409 verbatim. Also verified all three 503 sources (NoveltyDeferred, deferral-only MaterializePartial, novelty backpressure) are genuinely retryable, so the blanket Retry-After isn't over-broad. fluree-db-server and fluree-db-consensus green, CI green.
Two inline notes below, plus one convention item: no Follow-up: #N for the Raft residual. It's disclosed honestly in the prose, but a worker-side novelty failure still surfaces as a generic 422 there — the issue's footgun, unfixed, on the clustered path. Per CLAUDE.md that's what the marker is for.
| /// `Retry-After`): this is a permanent refusal of this transaction at this | ||
| /// configuration, surfaced as HTTP 413 with no `Retry-After` — split the | ||
| /// transaction into smaller pieces or raise `reindex_max_bytes`. | ||
| pub const NOVELTY_DELTA_TOO_LARGE: &str = "err:db/NoveltyDeltaTooLarge"; |
There was a problem hiding this comment.
New code with no user-facing documentation. docs/api/errors.md is untouched and currently documents different @types for both statuses this PR uses:
| status | errors.md says | this PR emits |
|---|---|---|
| 503 | err:db/ServiceUnavailable |
err:db/NoveltyAtMax |
| 413 | err:db/PayloadTooLarge |
err:db/NoveltyDeltaTooLarge |
err:db/ServiceUnavailable isn't defined in fluree-vocab at all, so that example is a code the server can't emit. The 413 collision is the sharper half — same status, two @types, only one documented.
Since the point of the PR is that clients branch on @type, a developer following the docs writes a handler matching neither new case.
There was a problem hiding this comment.
This was the sharpest catch of the review — and when we pulled the thread it went further than your table. err:db/PayloadTooLarge turns out to be fictional too: the body-limit 413 was falling through the same Http catch-all to err:system/InternalError, and docs/api/headers.md carried a third spelling, err:http/PayloadTooLarge. So before this PR, neither 413 nor 503 ever emitted an @type the docs mentioned.
One judgment call to flag for your veto: rather than document the absurdity ("the body-limit 413 carries err:system/InternalError"), we made the long-documented code real — added PAYLOAD_TOO_LARGE to fluree-vocab and a 413 => errors::PAYLOAD_TOO_LARGE arm in error_type()'s Http match, same pattern as the existing 401/403/409/422 arms, pinned by body_limit_413_carries_payload_too_large_not_the_novelty_code. That's ~10 lines beyond the strict scope here, but it's what makes the two 413s genuinely distinguishable by @type, which is the whole point of the split. Easy to revert to docs-only if you'd rather.
errors.md, headers.md, and common-errors.md now match emitted reality for every status this PR touches, and a repo sweep shows zero remaining references to either fictional code. (The broader fiction in errors.md — err:db/BadRequest, err:db/Timeout, err:db/RateLimited etc. all diverge from vocab — is real but pre-existing and untouched; flagging rather than folding since it's outside every status this PR changes.) Commits e7466dd96 + f177abf9a.
| // Static 1s: the indexer's drain time is not observable at the | ||
| // error site, and 1s is the shortest interval that stops a tight | ||
| // retry loop without penalizing recovery. | ||
| if status == StatusCode::SERVICE_UNAVAILABLE { |
There was a problem hiding this comment.
Worth reconsidering the value. Many HTTP clients honour Retry-After in preference to their own exponential backoff, and novelty drain on a large ledger is seconds to minutes — so this pins every blocked client at 1 rps of rejected requests during exactly the window the server is already capacity-stressed. That can be worse than sending no header, which leaves clients on their own backoff.
"Drain time isn't observable at the error site" argues for a conservative constant rather than the smallest non-zero one. A larger default with jitter would be gentler.
Separately: this keys purely on status, so any future 503 inherits the header automatically. The "every 503 here is retryable" invariant is now load-bearing and enforced by a comment.
There was a problem hiding this comment.
You're right, and the honor-in-preference-to-backoff point is the part I hadn't weighed — it means the smallest non-zero value is the most aggressive possible choice during exactly the stressed window, which is backwards. Now a uniform random integer in [3, 8] per response (rand is already a direct dep; consts + rationale comment in error.rs), with the unit and integration tests asserting presence + range rather than an exact value.
On the invariant being comment-enforced: also fixed, three layers deep — a RETRYABLE_503_TYPES allowlist with a debug_assert at the attachment point, a corpus test over every 503-producing shape, and a no-wildcard match over SubmissionError so a future variant refuses to compile until someone classifies it.
Chasing that invariant surfaced something your enumeration (and ours) missed: there are four 503 sources, not three. SubmissionError::Overloaded (the committer's in-flight cap) reaches clients as 503 + the err:system/InternalError catch-all @type. It is retryable capacity per its own contract, so the blanket Retry-After stays sound — it's now consciously pinned in the allowlist and documented honestly, with a note that it deserves a typed code someday rather than the internal catch-all.
Review follow-ups on the novelty-backpressure mapping: - Retry-After on 503s is now a uniform random integer in [3, 8] per response instead of a static 1. Many HTTP clients honor Retry-After in preference to their own exponential backoff, so the smallest non-zero value pinned every blocked client at 1 rps of rejected requests during exactly the window the server is capacity-stressed; the jitter de-choruses clients refused in the same instant. Unit and integration assertions check presence + range rather than an exact value. - The 'every 503 is retryable capacity' invariant is now load-bearing in code, not a comment: RETRYABLE_503_TYPES is an explicit allowlist of the @types a 503 may carry, enforced by a debug_assert at the Retry-After attachment point, a corpus test enumerating every 503-producing path (including the SubmissionError::Overloaded passthrough, whose @type is still the Http catch-all), and an exhaustive no-wildcard match over SubmissionError variants that refuses to compile until a future variant is consciously classified. - The HTTP body-size-limit 413 (read_limited_body) now carries the documented err:db/PayloadTooLarge code instead of falling through the Http-passthrough arm to err:system/InternalError, so the two 413s (body cap vs err:db/NoveltyDeltaTooLarge oversized novelty delta) are distinguishable by @type - which is the point of the split.
docs/api/errors.md documented 503 as err:db/ServiceUnavailable and 413 as a lone err:db/PayloadTooLarge; err:db/ServiceUnavailable is not defined in fluree-vocab and is never emitted, so a client following the docs wrote a handler matching nothing. - 503: the real codes (err:db/NoveltyAtMax, err:db/NoveltyDeferred, and the err:system/InternalError committer-overload passthrough), the every-503-is-retryable-capacity contract, and the jittered Retry-After. - 413: both real codes with branch-on-@type guidance - the HTTP body cap (err:db/PayloadTooLarge) and an oversized novelty delta (err:db/NoveltyDeltaTooLarge, never admissible by drain, no retry). - docs/api/headers.md: err:http/PayloadTooLarge (a third spelling, also never emitted) corrected to err:db/PayloadTooLarge with the real message; docs/troubleshooting/common-errors.md: fictional code/details response shape replaced with the real body.
|
Thank you for verification-grade review — reverting six files to reproduce the pre-fix response is above and beyond, and it paid off: Claude re-verified your repro against Both inline items are addressed (details on each thread), the docs reconciliation went deeper than expected — see the addendum in the PR body, including one small scope judgment (making |
2 participants
Closes #1708.
The issue's sketch was three lines; the fix grew a layer deeper for a reason worth stating up front: every transact surface (JSON-LD, Turtle, SPARQL UPDATE) routes through the consensus committer, which flattens
ApiErrorintoSubmissionError::Execution { status, message }— the variant's identity dies there, and status alone can't recover it (the Raft paths emit other, differently-caused 503s). So a mapping-only change at the server edge would have been inert on the paths that matter. The fix threads identity through explicitly, and the final mapping is:@typeRetry-AfterNoveltyAtMax(stage-time: novelty already at ceiling)err:db/NoveltyAtMax3–8(jittered)NoveltyWouldExceed, drainable (delta < max)err:db/NoveltyAtMax3–8(jittered)NoveltyWouldExceed, oversized (delta >= max— can never succeed by drain, since the commit check iscurrent + delta >= maxeven against empty novelty)err:db/NoveltyDeltaTooLarge(new vocab code)err:db/InvalidTransactionBoth novelty variants had to come along:
WouldExceedis the commit-time predictive check and in a plain HTTP workload it fires first — mapping onlyAtMaxwould leave the exact footgun in place (in-repo precedent:classify_noveltyinr2rml_materialize.rsdocuments a production incident where handling one variant made a backpressure fix inert). The oversized case is typed end-to-end (SubmissionError::NoveltyDeltaTooLarge→ServerError::NoveltyDeltaTooLarge) rather than a status-map passthrough, deliberately:read_limited_bodyalready emits 413 for the HTTP body-size limit, and a status-keyed arm would have mislabeled those.Retry-Afterattaches to every 503 this server emits and only to 503s — they are all retryable capacity conditions, RFC 9110 wants the pairing, and the 413 never carries it (pinned by unit assertion). The value is a conservative jittered constant, uniform in [3, 8] seconds per response (review feedback: many clients honorRetry-Afterin preference to their own exponential backoff, so the smallest non-zero value is the most aggressive possible choice during exactly the stressed window; jitter de-choruses synchronized retries). The every-503-is-retryable invariant is no longer comment-enforced: an allowlist +debug_assertat the attachment point, a corpus test over all six 503-producing shapes, and a no-wildcardSubmissionErrormatch mean a future 503-producing variant fails to compile or fails a test until consciously classified.Idempotency semantics — the part internal review caught. The first cut of this branch marked the novelty refusals unsettled, intending "retry re-executes after drain." That did the opposite:
record_outcomeskips unsettled errors while the in-flight guard unconditionally disarms its cleanup, so a keyed refusal pinned the slot as InFlight for the 1-hour TTL and a keyed client obeying our ownRetry-Aftergot 409 AlreadyInFlight on every retry — worse than pre-PR for exactly the keyed ingest clients this issue is about. Internal review proved it with a failing probe. The refusals are now settled — a settled Failed entry is re-attemptable viatry_claim_slot's body-hash-matching replace, which is what actually yields retry-after-drain semantics — and the property is pinned three ways: a consensus-level keyed-retry test (scripted committer, executor invoked exactly twice), a real-HTTP keyed-retry integration test whose mutation failure reproduces the 409 symptom verbatim, and the settlement-classification unit pin. No double-apply risk: both novelty raise sites precede commit construction, and Raft never produces the variants (unsettled stays reserved for its maybe-still-commits transitions).Callers of the old 400 shape, checked: CLI uses only generic status handling; peer forwarding relays status + headers so 503/Retry-After passes through;
classify_noveltymatches variants, not codes — and its split-the-chunk answer toWouldExceedis exactly right for the oversized case. One residual stated honestly: on the Raft path a worker-side novelty failure still surfaces as a generic 422 via poison records (the replicated record carries only a reason enum — threading novelty identity through Raft is real scope beyond this fix; single-node/local, the issue's scenario, is fully covered).Docs: the false "blocks commits until reindexed" claim existed in four places, not the two the issue found —
docs/operations/configuration.md, theserver_defaults.rsdoc comment and its generated config-file template, and theServerConfigfield doc — all now describe the real erroring behavior and the new semantics.Tests (integration in
fluree-db-server/tests/integration.rs, running incargo test -p fluree-db-serverand CI): stage-timeAtMaxprovoked the way production hits it (commit under a permissive threshold, reopen the same storage path with a 1-byte threshold so replay-on-load fills novelty) → 503 + code + header; the oversized case (1-byte ceiling, first insert) → 413 +NoveltyDeltaTooLarge+ no header; the keyed-retry test above; unit pins for all three error shapes, the drainable/oversized routing at the flattening point, and header presence-on-503/absence-elsewhere. A genuinely-drainableWouldExceedover real HTTP isn't cheaply constructible (needs byte-exact flake sizing), so that leg is covered by the AtMax integration test plus the unit-level split pins — the 413 test's doc comment says exactly this.Gates (Claude ran these; pasting the tally): workspace
cargo check --all-targetsgreen with--exclude fluree-search-httpd --exclude fluree-search-service(local C++ toolchain issue in the usearch build script, unrelated — CI is authoritative for those two); clippy-D warningsgreen across fluree-vocab/api/consensus/server/transact;cargo test -p fluree-db-serverfull package green (incl. the new tests);-p fluree-db-consensus56/56;-p fluree-db-api --lib775/775;-p fluree-vocab20/20; fmt clean after the last edit.Post-review addendum (commits
e7466dd96,f177abf9a): the docs reconciliation went deeper than the review's two rows —err:db/PayloadTooLargewas also fictional (the body-limit 413 actually fell through the same catch-all toerr:system/InternalError, anddocs/api/headers.mdcarried a third spelling,err:http/PayloadTooLarge). Rather than document that absurdity, this now makes the long-documented code real:PAYLOAD_TOO_LARGEadded tofluree-vocabwith a413 => PAYLOAD_TOO_LARGEarm for the body-limit path, so the two 413s are genuinely distinguishable by@type— which is the point of the split. Also surfaced: there are four 503 sources, not three —SubmissionError::Overloadedreaches clients as 503 + the internal catch-all@type; it is retryable capacity so the blanketRetry-Afterstays sound, and it's now consciously pinned in the allowlist and documented honestly (a typed code for it is noted as a residual).docs/api/errors.md/headers.md/common-errors.mdnow match emitted reality for every status this PR touches.Follow-up: #1741 (Raft path — worker-side novelty failure still surfaces as a generic 422 via poison records; wire-format change, deliberately not smuggled in here).