fix(consensus): park queued submissions on entry liveness, not a fixed clock - #1792
fix(consensus): park queued submissions on entry liveness, not a fixed clock#1792bplatz wants to merge 2 commits into
Conversation
EmbeddedRaftConfig::with_submit_wait(timeout, max_retries) overrides the QueuedTransactor's 8 s × 3 default. That default is sized for leader transitions on a cluster; a single-node host whose per-branch queue runs tens of seconds deep (bulk publishes, sequential upsert chunks) reported a still-queued submission as stranded and then committed it anyway.
…d clock The queued transactor gave a submission a fixed per-attempt wait (8 s) and a fixed number of attempts (3), then reported it stranded with a 504. That budget was sized for leader transitions, but it was spent on healthy slow commits too: a single node running sequential bulk chunks whose stage plus publish exceeded 24 s had every submission reported stranded while the worker committed it anyway. A wait timeout is now a probe. When it fires the transactor checks whether the entry is still in the replicated per-branch queue and the cluster has a leader; if so the submission is alive and it parks again without spending an attempt. Only a probe that finds the entry gone (or the node leaderless — what a partitioned former leader sees) spends an attempt on the existing re-propose path. The former-leader case needs nothing more: ApplyHead replicates to every node, so a waiter bound on a former leader resolves when the new leader's worker finishes the entry. A ceiling on total parked time (default 10 minutes, `with_max_wait` / `EmbeddedRaftConfig::with_submit_max_wait`) backstops a worker that never finishes; hitting it reports the outcome as unknown rather than failed, since the commit may still land. Two embedded-node tests pin the behaviour with a 1 ms probe and a single attempt: a 2,000-node insert completes with a receipt, and a 1 ms ceiling turns the same insert into a 504 that names the outcome unknown while the commit lands regardless. Both fail under the previous fixed budget with the production message.
aaj3f
left a comment
There was a problem hiding this comment.
@bplatz the problem makes sense and seems well-identified. No contention with the solution either. Claude review below:
Praise — turning the waiter timeout from a verdict into a probe of the replicated queue and the leader, and keeping the retry path for exactly the condition it was built for. I verified it rather than read it: cargo test -p fluree-db-consensus --all-features is 332 lib + 14 integration green with all six new tests present by name, fmt and clippy are clean, and forcing every probe timeout back to SpendAttempt makes the 2,000-node embedded-node test fail with the production message verbatim (submission stranded by leader transition; retry with an idempotency key). The ceiling reporting the outcome as unknown rather than failed is the honest shape, the waiter.rs scope doc now states the ApplyHead-replicates-everywhere fact that makes dropping the drain sound, and the host knobs are additive.
Two small fold-ins and one note, none blocking: a gone-entry probe can race a terminal apply that landed between the timeout and the probe (ticket.wait leaves the outcome in the receiver on TimedOut), which for an anonymous submission returns stranded while the ticket holds the receipt — one non-blocking re-check before spending the attempt closes it; the ceiling message tells anonymous callers to poll with a key they don't have; and a live entry can now hold a request for ten minutes, which the standalone server's HTTP timeouts and any load balancer should know about.
Nothing ran in CI for this head (stacked two deep), so the gates above are local; the four files here don't overlap #1789 or #1791, so this could be retargeted straight to main and get CI on its own rather than waiting on the stats stack.
Adherence to repo commitments:
- Patterns/abstractions: ✔ extends
QueuedTransactorandEmbeddedRaftConfigin place; reusesSharedStateand the existing re-propose path; no parallel construct. - Performance (speed first, memory second): ✔ no product hot path; one read lock and an O(queue) scan per probe interval.
- Testing: ✔ verdict matrix and
entry_queuedunit tests plus two embedded-node tests with a 1 ms probe that pin both outcomes; mutation-verified;⚠️ not run in CI on this head (stacked). - Conventions: ✔ two focused multi-line commits with the mechanism; docs updated in the waiter module; clippy/fmt clean under
--all-features.
Verified locally at branch HEAD d183a0d4d: cargo test -p fluree-db-consensus --all-features → 332 + 2 + 3 + 3 + 6 passed; mutation (probe_verdict → always SpendAttempt) → embedded test FAILED as expected, restored clean; cargo fmt --all -- --check and cargo clippy -p fluree-db-consensus --all-features --all-targets -- -D warnings clean.
Approving so you can land it, particularly once underlying PRs are merged and this is retargeted to main so CI sees it, and consider the two small fold-ins first.
| match ticket.wait(self.wait_timeout).await { | ||
| Ok(outcome) => return Ok(SubmissionOutcome::Waiter(outcome)), | ||
| Err(WaitError::Displaced) => break, | ||
| Err(WaitError::TimedOut) => { |
There was a problem hiding this comment.
Optional (fold in now). This is more of a question than a suggestion. A gone-entry probe can race a terminal apply that landed between the timeout and the probe: ticket.wait returns TimedOut without consuming the oneshot (waiter.rs:146-158), so an ApplyHead that lands a moment later leaves the outcome sitting in the receiver while entry_alive finds the entry already popped → SpendAttempt.
For an idempotency-keyed submission the re-propose hits the cache and returns the receipt, so no harm. For an anonymous one it returns stranded_error while the ticket is holding the receipt. One non-blocking re-check of the ticket (ticket.wait(Duration::ZERO)) before spending the attempt closes the window. Minor, but if you agree it's right I'd rather see it here than in the backlog.
| entry_queued(&state, ref_key, queue_id) | ||
| } | ||
|
|
||
| fn ceiling_error(&self) -> SubmissionError { |
There was a problem hiding this comment.
Optional. The ceiling message tells every caller to "poll with the idempotency key", but an anonymous submission has none. stranded_error already distinguishes retry_eligible; the ceiling message should say the same two things: keyed → poll with the key; anonymous → the outcome is unknown and may have committed, check the ledger head.
| /// probe that finds the entry still queued keeps waiting without | ||
| /// spending an attempt, so these bound leader-transition recovery, | ||
| /// not commit latency — see `QueuedTransactor`. | ||
| pub submit_wait: Option<(Duration, usize)>, |
There was a problem hiding this comment.
Note (operational, docs). A live entry can now hold the request for up to ten minutes where it used to give up at ~24 s — which is the point — but the standalone server's HTTP layer and any load balancer in front have their own idle timeouts (60 s is typical), so a client can be severed while the server task keeps waiting, and the parked tasks are bounded only by concurrent callers. Worth a line in the standalone docs next to with_submit_max_wait, and a look at whether the server's request timeout should be at least the probe interval times a few.
| /// never applied the enqueue) and a leaderless view — which is also | ||
| /// what a partitioned former leader sees — both read as not alive, | ||
| /// so the retry path gets to surface the real condition. | ||
| async fn entry_alive(&self, ref_key: &RefKey, queue_id: Option<u64>) -> bool { |
There was a problem hiding this comment.
Praise. entry_alive reads the state the cluster actually has — the replicated per-branch queue and the current leader — instead of a clock, an unbound ticket and a leaderless view both read as not alive so the existing retry path still surfaces the real condition, and entry_queued scanning by queue_id on its own branch is O(queue) once per probe interval. Reverting the verdict to the fixed budget reproduces the production message in the new embedded-node test verbatim, so the pin has teeth.
| @@ -30,10 +30,15 @@ | |||
| //! | |||
There was a problem hiding this comment.
Praise. Rewriting the scope doc to say ApplyHead replicates to every node — so a waiter bound on a former leader resolves when the new leader's worker finishes the entry — is the fact that makes dropping the leadership-loss drain sound. Good to have it stated where the next reader will look.
Problem
QueuedTransactorgave every submission a fixed per-attempt wait (8 s) and a fixed attempt count (3), then reported it stranded with a 504. The budget was sized for leader transitions, but it was spent on healthy slow commits too. On a single node running sequential bulk chunks, any chunk whose stage plus publish exceeded ~24 s was reported stranded while the worker committed it anyway — the caller saw a 504 and the write landed in the background.EmbeddedRaftConfigalso gave a host no way to change the numbers.Change
SharedState.queues) and the cluster has a leader. If so the submission is alive and it parks again without spending an attempt. Only a probe that finds the entry gone — or the node leaderless, which is what a partitioned former leader sees — spends an attempt on the existing re-propose path.ApplyHeadreplicates to every node, so a waiter bound on a former leader resolves when the new leader's worker finishes the entry. The waiter module docs now say so.with_max_waiton the transactor,with_submit_max_waitonEmbeddedRaftConfig). Hitting it reports the outcome as unknown, not failed, since the commit may still land.EmbeddedRaftConfig::with_submit_wait(timeout, max_retries)sets the probe interval and attempt count (first commit, carried from the local patch that worked around this);with_submit_max_waitsets the ceiling.Tests
probe_verdictmatrix (live → keep waiting, gone → spend attempt regardless of elapsed time, live past the ceiling → ceiling) andentry_queuedacross branches and after a pop.tests/it_embedded_node.rs, single-voter node with a 1 ms probe and one attempt: a 2,000-node insert completes with a receipt; the same insert under a 1 ms ceiling returns a 504 naming the outcome unknown while the replicated head still advances. Both fail under the previous fixed budget with the production messagesubmission stranded by leader transition.fluree-db-consensus --features raftsuite green; clippy and fmt clean.Notes
fix/staged-view-dict-gap(fix(transact): give staged views dictionaries that cover their own flakes #1791) for a demo build; to be retargeted to main afterwards.