fix(replication): bound total bootstrap stall with a force-drain deadline - #197
fix(replication): bound total bootstrap stall with a force-drain deadline#197dirvine wants to merge 3 commits into
Conversation
…line Adds an unconditional drain ceiling as defence-in-depth behind the per-source capacity-rejection expiry already on main. If bootstrap has not drained within bootstrap_drain_deadline (default 30m) of when drain tracking began, check_bootstrap_drained forces drain with a warn log, regardless of pending peer requests, capacity-rejected sources, or pending keys. This bounds total bootstrap stall even where per-source expiry alone would not resolve the wedge: multiple coordinated over-cap sources, or a pending_keys path that never empties after a disk-full restart. By the deadline, stale pending_verify entries have already been evicted by PENDING_VERIFY_MAX_AGE, so genuine residual work is minimal. While is_bootstrapping is true, audits are paused (Invariant 19) and pruning is gated on drain, so an unbounded stall disables the node's reputation and prune paths for its lifetime. - BootstrapState: +bootstrap_started_at, +bootstrap_drain_deadline - ReplicationConfig: +bootstrap_drain_deadline (default BOOTSTRAP_DRAIN_DEADLINE = 30m) - check_bootstrap_drained: force-drain ceiling placed before all other checks - Tests: force_drain_after_overall_deadline, force_drain_does_not_fire_within_deadline
… forced drain, prevent post-drain re-wedge - Stamp bootstrap_started_at when replication bootstrap actually begins, not at ReplicationEngine construction, so P2P startup and cache dials no longer consume the 30-minute drain budget. - Represent force-drain as a distinct degraded completion rather than ordinary drained readiness. - Add drained no-op guards to bootstrap accounting helpers so post-drain batches cannot resurrect pending_peer_requests / pending_keys debt. - Lift the verification batch barrier once drained so a resurrected request counter cannot wedge verification after the deadline. - Await the now-async ReplicationEngine::start in the feature-gated shutdown PoC so it actually exercises the engine setup it intends to test.
grumbach
left a comment
There was a problem hiding this comment.
Reviewed at head 6b0a313. The objective is right and the deadline does get polled, so this design can deliver what the ticket asks. Commit 1 is close to landable. The batch-loop break, wrapper guards and barrier lift added in commit 2 are outside the scope written into the ticket, and that is where the two worst problems are.
Three blockers.
1. src/replication/mod.rs:3749-3752: the new if drained { break } cancels bootstrap neighbour sync completely.
The break tests plain drained, and on current main a fresh node is already drained before the loop reaches its first batch:
start_verification_worker()(mod.rs:2180) is spawned beforestart_bootstrap_sync()(:2181), ticks every 250ms (:1464), and callsexpire_and_recheck_bootstrap_drainas the first statement of the cycle (:7648-7655).- A fresh node has zero pending requests, no rejected sources, no pending keys and empty queues (
:1890, nothing restores from disk), socheck_bootstrap_drainedtakes the ordinary drain branch (bootstrap.rs:169-175) at roughly 250ms. - Bootstrap sync is still parked in
wait_for_bootstrap_complete(bootstrap.rs:46-89, up to 60s). By the time it reaches the batch loop the break fires on iteration 1 and no sync request is ever sent.
The node then discovers nothing at join. Its only remaining path is the steady-state loop, which parks 10 to 20 minutes before its first tick and covers 4 of 20 neighbours per tick, so 50 to 100 minutes for full coverage. That is an under-replication window on every joining node, and CI is green only because it is a race: on a fast devnet the DHT event can land inside 250ms.
The premature ordinary drain itself is pre-existing (49093a0, first shipped in v0.17.0-rc.1), not introduced here. This PR turns it into a bootstrap sync outage.
Fix: drop the break. Nothing needs it. check_bootstrap_drained already short-circuits on drained at bootstrap.rs:117, and both verification barriers now test !state.drained, so there is no re-wedge left to prevent. If you want to keep it, add the forced-versus-normal distinction the commit message claims ("distinct degraded completion"). BootstrapState still carries only drained: bool, so nothing distinguishes the two today.
2. src/replication/config.rs:637-639 against :1043-1056: the 30 minute default sits below the defence it is supposed to back.
With shipped defaults, capacity_rejected_max_age() is 125 minutes: 20min x 5 batches + 15s x 20 peers = 105min, floored at the 1h cooldown, plus one 20min interval.
A capacity rejection recorded during bootstrap can only clear on that source's next admission cycle, which is in the steady-state round robin. So any node that takes a single capacity rejection during bootstrap is guaranteed to exit through the force-drain warning at exactly 30 minutes, and expire_capacity_rejected becomes unreachable in the bootstrap window.
That matters for two reasons. It inverts what the ADR says it is doing, where per-source expiry is the first-line defence and this is the backstop, and it contradicts the ticket's own line about preserving existing per-source expiry. It also destroys the warning's signal value, and that warning is the ADR's stated fleet-review trigger. If busy nodes hit it routinely, nobody will notice the real wedge when it recurs.
The PENDING_VERIFY_MAX_AGE alignment argument does not hold either. An entry admitted at 29:59 is one second old when the total deadline fires at 30:00, so residual work is not necessarily stale or minimal.
Fix: derive the default from config so it sits strictly above the targeted defence, for example capacity_rejected_max_age().saturating_add(neighbor_sync_interval_max). Keep the config field so tests can shorten it. If 30 minutes is deliberate policy, then say so in the ADR and drop both the first-line-defence framing and the alignment argument.
3. src/replication/mod.rs:3653-3674 and :7933: the ceiling is only as live as the verification worker.
expire_and_recheck_bootstrap_drain is the only periodic caller of check_bootstrap_drained; every other caller is event driven. It runs inside run_verification_cycle, which later awaits paid_list.insert(key).await with no timeout. The failure mode this PR targets is a disk-full or stalled-storage node, which is exactly what can park that write. A worker panic does the same, since the handle is not supervised until shutdown.
So the "bounded independently of the exact cleanup defect" claim holds only while the pipeline being unwedged is healthy enough to keep ticking. Fix: arm the deadline from something independent, either a small task doing sleep_until(bootstrap_started_at + deadline) or an extra select! branch that fires the ceiling without entering the cycle body.
Smaller, still want these before merge.
- The stated rollback does not exist.
ReplicationConfigderives onlyDebug, Clone, has no serde and no env or CLI plumbing, andnode.rs:123takesReplicationConfig::default(). Tuning the deadline is a rebuild and a fleet redeploy, same cost as the revert. Either add an env override or say plainly that revert is the only rollback. - Neither new test exercises the live path. Both build a
BootstrapStateliteral with a backdatedInstantand callcheck_bootstrap_draineddirectly (bootstrap.rs:657-721). Nothing covers the 250ms poll noticing the deadline,is_bootstrappingflipping, the new break, or the config-to-state wiring atmod.rs:1897-1900. Blocker 1 is invisible to the whole suite. Thepoc_bootstrap_stallrun cited under test evidence is on an untouched file with no force-drain test in it. - No metric and no durable completion reason. One unstructured warn that then erases the counts it reported. The ADR asks the fleet to monitor force-drain frequency and outstanding counts, which is not implementable once logs rotate.
audit_metrics.rsand the structuredtarget:/event=logging on the audit requester are the existing idiom. Aforce_drained: boolonBootstrapStatealso gives blocker 1 its fix for free.
Follow-ups, happy for these to be separate.
types.rs:819hardcodesDuration::from_secs(30 * 60)rather than the config constant, andmod.rs:1897-1900is construct-then-patch. Awith_deadline(...)constructor removes both.types.rs:796also calls anInstanta wall-clock moment.validate()does not bound the new field. A zero deadline force-drains on the first tick, and with the break in place skips every neighbour. Low reachability today, but a floor is one line.- After a forced drain the node advertises
bootstrapping: false, so peers record repair proofs (mod.rs:7409) and can later audit keys still in its fetch queue at weight 5.REPAIR_HINT_MIN_AGEof 1h plus a required completed sync cycle softens this a lot, so I would document rather than block, but the ADR's trade-offs section does not mention trust attribution at all and should. Same for the fact that a forced drain permanently burns the node's bootstrap grace window with peers (mod.rs:7402-7407). - The new
drainedguards inbootstrap.rs:194, 268, 284sit on wrappers that production no longer calls. The live path ispublish_bootstrap_admission_outcomes(mod.rs:7464-7505), which mutatespending_keysandcapacity_rejected_sourcesdirectly and unguarded. Harmless in practice because the drain check short-circuits, but the commit message's re-wedge claim is not what the code does. - The bound is per process. Nothing is persisted, so a node crash-looping under 30 minutes never reaches the ceiling. Worth one sentence in the ADR scoping the guarantee to an uninterrupted process lifetime.
Checked and clean: lock ordering is consistently queues before bootstrap state at every new and existing site; clearing pending_keys loses no data since it is drain-tracking only and the queues survive; every production caller pairs the drain check with complete_bootstrap; making start async is safe, &mut self serialises it and no lock is held across the call; no wire or storage-format change, so mixed-version rollout and binary rollback are mechanically fine.
One thing on the premise, worth settling before the acceptance run. The production evidence in the parent issue is dated 2026-08-04, but the periodic drain self-heal only shipped in v0.17.0-rc.1 on that same day, so the wedged node was almost certainly on v0.16.0 or earlier without it. Also, two of the three nodes sampled there were budget_deferred, not bootstrap_deferred, and the warn storm is a remote peer's bootstrap claim, which this does not touch. This is a fair backstop for one of the three observed blockers, but I would not book it as the fix for the parent issue.
For the dev-testnet acceptance run, the grep that settles blocker 1 on a joiner is the relative order of these three lines:
Bootstrap drained: all peer requests completed and work queues empty(bootstrap.rs:171)Bootstrap sync: syncing with N close neighbors(mod.rs:3742)Bootstrap sync: drain already completed, stopping remaining batches(mod.rs:3750)
If the first comes before the second, bootstrap sync never ran. Then, on a joiner against a loaded network, record the natural drain-time distribution so there is evidence the ceiling sits above the honest p99 rather than below it. Without that number blocker 2 says it is probably below.
Linear issue
V2-882 — Bound replication bootstrap stall with an overall force-drain deadline
Parent production issue: V2-864 — Pruning defers 100% of candidates in production
Risk tier
Compatibility
ReplicationConfigfield; no public client API or CLI change.Semver impact
Test evidence
Local exact-head evidence:
cargo check --lib— passed.cargo test --lib replication::bootstrap— 16 passed, 0 failed.cargo test --lib replication::— 567 passed, 0 failed.cargo test --lib replication::config— 35 passed, 0 failed.cargo test --test poc_bootstrap_stall --features test-utils— 3 passed, 0 failed.cargo fmt --all -- --check— passed.cargo clippy --lib -- -D warnings— passed.python3 scripts/adr-governance.py check— passed (10 ADRs).A dev-testnet acceptance run remains required for Tier 2 before merge. In particular, verify normal nodes drain before the ceiling and that a deliberately wedged node emits the force-drain warning, transitions out of bootstrap, resumes audit activity, and no longer leaves pruning indefinitely
bootstrap_deferred.New dependency
none
ADR
ADR-0010: Bound replication bootstrap drain with an overall deadline
Status remains Proposed pending human review; this PR does not mark it Accepted.
Mitigation / rollback
Revert the single implementation commit, or raise/disable the internal deadline through
ReplicationConfigwhile retaining the existing per-source capacity-rejection expiry path.