Skip to content

fix(dig-node): guard remaining inline background loops against panic-death (#2068) - #175

Merged
MichaelTaylor3d merged 2 commits into
mainfrom
harden/2068-inline-loop-panic-guard
Aug 3, 2026
Merged

fix(dig-node): guard remaining inline background loops against panic-death (#2068)#175
MichaelTaylor3d merged 2 commits into
mainfrom
harden/2068-inline-loop-panic-guard

Conversation

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor

Closes dig_ecosystem#2068. Follow-up to #2067 — extends the same crate::shared::catch_iteration(loop_name, fut) combinator (no new mechanism) to the deferred long-lived background loops, conservatively scoped to the ones that extract cleanly with low blast radius.

Guarded (2) — both in seams/dig_peer/holdings.rs

  1. run_first_peer_announcer — the per-pool-event re-state is wrapped in catch_iteration("first_peer_announce", restate_holdings(...)). To make the iteration testable, restate_holdings now takes transport: &dyn AnnounceTransport (the seam announce_all_holdings already consumes) instead of &GossipHandle — a private fn, 2 call sites both in this file. events.recv().await stays at the loop top, so a persistently-panicking re-state cannot hot-spin.
    • AssertUnwindSafe soundness — SOUND: the guarded iteration carries only &dyn seam refs; no std::sync::MutexGuard crosses the catch boundary (the broadcaster seq is an AtomicU64, not a guard).
  2. run_holdings_ingest — the per-frame apply body (canonicalize + accept + log) was extracted into apply_inbound_announcement(...) and wrapped in catch_iteration("holdings_ingest", ...). inbound.recv().await stays at the loop top (natural throttle, no hot-spin); the Lagged/Closed control-flow arms stay OUTSIDE the guard.
    • AssertUnwindSafe soundness — SOUND: no lock held across the boundary; the ingress state is a tokio (async, non-poisoning) Mutex taken + released inside accept, never held by the loop across iterations.

Neither change masks real errors — the guard catches only panics; each loop's Result/Rejected handling is unchanged. A caught panic abandons that iteration's work (fail-closed) and the loop continues — it never bypasses the accept/signature gates (those run inside apply_inbound_announcement, so a caught panic drops the frame, it does not land it unverified).

Deferred (with reasons — per the conservative scope, → follow-up)

  • pex.rs spawn_pool_feeder and download.rs spawn_gc — iterate over concrete handles/filesystem with no injectable panic seam; a non-vacuous test needs an invasive seam.
  • peer.rs inline bring-up loops (selector-registry feed, DHT-routing feed, pool-stats, relay-acceptor) — extracting testable helpers + seams in central, high-fan-in peer.rs is exactly the invasive restructuring #2068 says to defer. No peer.rs edits made (kept the PR small).

These need injectable seams to get genuine per-loop panic tests — a further hardening follow-up sized above this scope.

Note: run_first_peer_announcer's initial (pre-loop) re-state is intentionally left UNGUARDED — one-shot at bring-up, where task-death is the correct fail-safe; only the long-lived loop iteration is guarded.

How verified (TDD)

  • holdings::a_panicking_re_state_is_caught_and_does_not_propagate — drives the real restate_holdings via catch_iteration against a PanickingInventory + a real signed broadcaster; asserts None.
  • holdings::a_panicking_frame_application_is_caught_and_does_not_propagate — builds a genuinely-signed announcement (real P-256 identity; passes every accept gate) and drives apply_inbound_announcement against a PanickingSink; asserts None. Non-vacuous — the panic is raised by the real apply path reaching the sink, so removing catch_unwind aborts the test.
  • cargo test -p dig-node-core --lib664 passed, 0 failed (662 baseline + 2). fmt + clippy -D warnings clean; both crates build.

Blast radius

No peer.rs edits. Only restate_holdings changed signature — private, 2 in-file call sites. The two public loop fns kept their signatures (callers in peer.rs/integration tests unaffected). 3 files, +200/−30.

Version

root [workspace.package].version 0.93.6 → 0.93.7 (patch, fix:); Cargo.lock regenerated.


Generated by Claude Code

claude added 2 commits August 3, 2026 18:37
…death (#2068)

Extends the #2067 catch_iteration panic-guard to two of the long-lived holdings
background loops #2067 deferred, so a transient per-iteration panic no longer
silently kills a subsystem for the process's lifetime.

- run_first_peer_announcer: the per-event re-state is now wrapped in
  catch_iteration("first_peer_announce", ...). restate_holdings was refactored to
  take the network as a &dyn AnnounceTransport (the seam announce_all_holdings
  already uses) so the guarded iteration is drivable against a mock transport with
  no live pool.
- run_holdings_ingest: the per-frame apply body was extracted into
  apply_inbound_announcement and wrapped in catch_iteration("holdings_ingest", ...).
  recv().await stays at the loop top, so a persistently-panicking source cannot
  hot-spin.

Per-site AssertUnwindSafe soundness verified: neither guarded iteration holds a
std::sync MutexGuard across the catch boundary (the ingress state is a tokio Mutex
taken + released inside accept; the broadcaster seq is an atomic). The guard catches
only panics — each loop's normal Result/error handling is unchanged.

TDD: two non-vacuous panic-injection tests (a_panicking_re_state_is_caught_and_does_not_propagate,
a_panicking_frame_application_is_caught_and_does_not_propagate) drive the real
guarded futures against a panicking inventory / sink and assert containment as None.

Deferred (reported on the ticket): pex spawn_pool_feeder churn, download spawn_gc,
and the peer.rs inline bring-up loops — no cheap injectable panic seam / would need
invasive central-wiring extraction, out of this conservative scope.

root [workspace.package].version 0.93.6 -> 0.93.7 (patch, fix).

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d merged commit c341d15 into main Aug 3, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the harden/2068-inline-loop-panic-guard branch August 3, 2026 19:36
MichaelTaylor3d added a commit that referenced this pull request Aug 3, 2026
…ot Ok(None)

The production `MeltChain` derived a melt from `AnchoredRootResolver::anchored_root()
== Ok(None)`, which was wrong in both directions and made a remote-triggered,
irreversible delete unsafe:

- `Ok(None)` is the node's fail-closed sentinel for "no confirmed generation".
  `CoinsetResolver` produces it for "launcher coin is unspent (store not minted
  yet)" — a store whose lineage has not STARTED. Deleting on it wrongfully erases
  live data and broadcasts a false melt network-wide, correlated across holders.
- A genuine melt never produces it. A melted tip is spent without a datastore
  child, so the lineage walk returns Err("singleton spend did not yield a store"),
  which fail-closed to Unknown — the gate could not fire on a real melt.

Replace it with `confirm_melt_via_chain(&dyn ChainReads, store_id)`, composed from
two POSITIVE chain facts, neither of which re-walks the lineage (#747):

1. The launcher coin whose `coin_id == store_id` exists and is SPENT. `coin_id ==
   store_id` is a 256-bit hash preimage that cannot be ground, so identity is
   pinned to the one unforgeable on-chain anchor, never to a look-alike singleton
   that merely curries `launcher_id == store_id` (#1473). An UNSPENT launcher is
   Live — "not minted yet" is the opposite of melted.
2. The launcher's hint index is NON-EMPTY and every generation under it is SPENT.
   One `coin_records_by_hint(store_id, include_spent = true)` read gives both. The
   non-empty half is load-bearing: an empty index is indistinguishable from an
   un-indexed store, so it resolves to Unknown rather than authorizing a delete.

Every error, absence, or ambiguity resolves away from deletion, and fact 2 is
deliberately conservative — candidates are not launcher-anchored before they count
as live, so an adversary who can plant hints can only suppress a deletion, never
cause one.

Tests: 8 new cases drive the real `ChainReads` trait with crafted coin records —
the link the spy-driven policy tests could not reach. Unused trait methods on the
mock are `unimplemented!()` so the gate cannot silently grow a chain dependency,
and the mock asserts `include_spent = true`. All six inverting mutations of the
gate were confirmed to fail their test.

Also: both new background loops wrap their per-iteration body in
`shared::catch_iteration` (#173/#174/#175) with `recv()`/`tick()` outside the
guard, so a persistently-panicking iteration paces instead of hot-spinning;
`TombstoneSet` recovers from lock poisoning rather than panicking forever after
one contained panic; SPEC.md 14.5 corrected — it documented the `Ok(None)` rule.

root [workspace.package].version -> 0.94.0 (minor, new capability).

Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d added a commit that referenced this pull request Aug 3, 2026
…ot Ok(None)

The production `MeltChain` derived a melt from `AnchoredRootResolver::anchored_root()
== Ok(None)`, which was wrong in both directions and made a remote-triggered,
irreversible delete unsafe:

- `Ok(None)` is the node's fail-closed sentinel for "no confirmed generation".
  `CoinsetResolver` produces it for "launcher coin is unspent (store not minted
  yet)" — a store whose lineage has not STARTED. Deleting on it wrongfully erases
  live data and broadcasts a false melt network-wide, correlated across holders.
- A genuine melt never produces it. A melted tip is spent without a datastore
  child, so the lineage walk returns Err("singleton spend did not yield a store"),
  which fail-closed to Unknown — the gate could not fire on a real melt.

Replace it with `confirm_melt_via_chain(&dyn ChainReads, store_id)`, composed from
two POSITIVE chain facts, neither of which re-walks the lineage (#747):

1. The launcher coin whose `coin_id == store_id` exists and is SPENT. `coin_id ==
   store_id` is a 256-bit hash preimage that cannot be ground, so identity is
   pinned to the one unforgeable on-chain anchor, never to a look-alike singleton
   that merely curries `launcher_id == store_id` (#1473). An UNSPENT launcher is
   Live — "not minted yet" is the opposite of melted.
2. The launcher's hint index is NON-EMPTY and every generation under it is SPENT.
   One `coin_records_by_hint(store_id, include_spent = true)` read gives both. The
   non-empty half is load-bearing: an empty index is indistinguishable from an
   un-indexed store, so it resolves to Unknown rather than authorizing a delete.

Every error, absence, or ambiguity resolves away from deletion, and fact 2 is
deliberately conservative — candidates are not launcher-anchored before they count
as live, so an adversary who can plant hints can only suppress a deletion, never
cause one.

Tests: 8 new cases drive the real `ChainReads` trait with crafted coin records —
the link the spy-driven policy tests could not reach. Unused trait methods on the
mock are `unimplemented!()` so the gate cannot silently grow a chain dependency,
and the mock asserts `include_spent = true`. All six inverting mutations of the
gate were confirmed to fail their test.

Also: both new background loops wrap their per-iteration body in
`shared::catch_iteration` (#173/#174/#175) with `recv()`/`tick()` outside the
guard, so a persistently-panicking iteration paces instead of hot-spinning;
`TombstoneSet` recovers from lock poisoning rather than panicking forever after
one contained panic; SPEC.md 14.5 corrected — it documented the `Ok(None)` rule.

root [workspace.package].version -> 0.94.0 (minor, new capability).

Co-Authored-By: Claude <noreply@anthropic.com>
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.

2 participants