Skip to content

fix(replication): bound total bootstrap stall with a force-drain deadline - #197

Open
dirvine wants to merge 3 commits into
mainfrom
fix/bootstrap-drain-wedge
Open

fix(replication): bound total bootstrap stall with a force-drain deadline#197
dirvine wants to merge 3 commits into
mainfrom
fix/bootstrap-drain-wedge

Conversation

@dirvine

@dirvine dirvine commented Aug 6, 2026

Copy link
Copy Markdown
Member

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

  • T0 — docs / tooling / CI / pure UX-output. Repo CI only.
  • T1 — client-only, no network-facing behavior change. CI + prod compat smoke.
  • T2 — node/client logic with behavioral surface, no protocol/format/economics change. Dev testnet + ADR.
  • T3 — protocol / storage format / payments / routing. T2 evidence + adversarial testing.

Compatibility

  • Wire: none; no message or protocol changes.
  • Storage: none; no persisted format or migration changes.
  • API: additive internal ReplicationConfig field; no public client API or CLI change.

Semver impact

  • breaking
  • feature
  • fix

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 ReplicationConfig while retaining the existing per-source capacity-rejection expiry path.

…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 grumbach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 before start_bootstrap_sync() (:2181), ticks every 250ms (:1464), and calls expire_and_recheck_bootstrap_drain as 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), so check_bootstrap_drained takes 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. ReplicationConfig derives only Debug, Clone, has no serde and no env or CLI plumbing, and node.rs:123 takes ReplicationConfig::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 BootstrapState literal with a backdated Instant and call check_bootstrap_drained directly (bootstrap.rs:657-721). Nothing covers the 250ms poll noticing the deadline, is_bootstrapping flipping, the new break, or the config-to-state wiring at mod.rs:1897-1900. Blocker 1 is invisible to the whole suite. The poc_bootstrap_stall run 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.rs and the structured target: / event= logging on the audit requester are the existing idiom. A force_drained: bool on BootstrapState also gives blocker 1 its fix for free.

Follow-ups, happy for these to be separate.

  • types.rs:819 hardcodes Duration::from_secs(30 * 60) rather than the config constant, and mod.rs:1897-1900 is construct-then-patch. A with_deadline(...) constructor removes both. types.rs:796 also calls an Instant a 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_AGE of 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 drained guards in bootstrap.rs:194, 268, 284 sit on wrappers that production no longer calls. The live path is publish_bootstrap_admission_outcomes (mod.rs:7464-7505), which mutates pending_keys and capacity_rejected_sources directly 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants