Skip to content

fix(warm): surface the pull's real cause and bound the descriptor ask with a ladder - #319

Merged
MichaelTaylor3d merged 4 commits into
mainfrom
loop/3128-capsule-warm
Aug 23, 2026
Merged

fix(warm): surface the pull's real cause and bound the descriptor ask with a ladder#319
MichaelTaylor3d merged 4 commits into
mainfrom
loop/3128-capsule-warm

Conversation

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor

Closes#318

Fixes the capsule-warm pull failure blocking the acceptance bar of https://github.com/DIG-Network/dig_ecosystem/issues/3128. Two defects, both in the GENERIC warm path.

The v0.141.0 relay fix is untouched and is not implicated.module_relay.rs:95 fires after the second-pass RelayEscalation, pool_locator names the holder from the connected pool, and the holder serves. A direct, non-relay B->C capsule fetch failed identically, which is what this PR addresses.

(a) The warm discarded its own error

module_reshare.rs used let Ok(bytes) = pulled else { ... reason = "pull failed" }. A let...else cannot bind the Err, so every failed warm in the field reported one constant string and four live diagnostic rounds produced no error text at all.

The cause is now carried in the return value - WarmFailure::PullFailed { reason } - and logged from there.

Why the value and not just the log: the first version of the regression test read a captured log and passed in isolation while failing in the full suite. tracing caches per-callsite interest, so a scoped subscriber never sees a callsite an earlier global subscriber already registered as disabled. A cause that exists only in a log is also precisely the shape of the original defect.

The error it now surfaces, from the regression fixture:

transport error from provider dddd...dddd: holder-refused-the-descriptor-sentinel

dig-download composes that text from this node's own vocabulary plus SENTINELLED peer ids, so surfacing it does not let a peer author what this node records (#1603).

(c) The descriptor ask had no bound this node had chosen

A dig.getModuleInfo is a full read plus SHA-256 of every chunk. Measured: 129 chunks, 135 MB, ~4.0 s, five times, on a host whose cat of the same file took 0.01 s with 0 block inputs - compute, not disk. The holder answered exactly once, 1 ms after the requestor had given up.

NatModuleTransport::get_module_info now asks under an explicit ladder of per-attempt deadlines - 5 s, 15 s, 45 s - re-dialling on each rung.

Why a ladder rather than a bigger number

The asker cannot know the capsule size before the descriptor arrives.dig_dht::ProviderRecord carries addresses and an expiry and no size at all, so a size-derived deadline has nothing to derive from on the first ask. Any single constant is therefore either too short for a large capsule or a blanket licence for a slow peer to hold a slot on a small one. At the measured ~34 MB/s a 1 GB capsule costs ~30 s, so the constant that covers it is one an unanswerable holder can also spend.

A ladder needs no size. Total wait grows only for a holder that keeps being slow, is bounded at the sum of the rungs (65 s), and a fast holder still fails fast on rung 1.

The later rungs are nearly free, and this is the load-bearing observation: a cold describe runs under spawn_blocking on the holder (Node::describe_held_module), and a blocking task is not cancelled when the requestor's stream drops. The abandoned ask still completes and populates that node's descriptor memo, so the re-ask is answered in milliseconds. Before this change the memo warmed one millisecond too late and nothing ever asked again.

A refusal - an ask that ANSWERS "no" - spends exactly one rung. Re-asking it cannot change the answer, and doing so would make every genuine miss cost the whole ladder before the next holder is tried.

(b) is NOT fixed here, and is genuinely blocked

MAX_DESCRIPTOR_ATTEMPTS never applies to a transport failure: in dig-download 0.19.0 src/module.rs:356, let (source, info) = self.fetch_module_info(...).await?; returns beforeattempts += 1 and before the demote-warn. Corroborated by dig_download emitting zero lines all run while 6 WARNs from other crates reached the log.

That is a dig-download defect, filed as DIG-Network/dig-download#37. dig-node cannot take a dig-download 0.20 carrying it - it needs dig-dht ^0.12 / dig-nat ^0.20 / dig-peer ^0.12 / dig-tls ^0.4, and dig-node-core is behind on all four. That cascade is https://github.com/DIG-Network/dig_ecosystem/issues/3152 and was not started. Nothing was vendored, patched, or worked around at the pin. The ladder above is dig-node retrying at the one layer it controls; it does not make the dig-download defect benign for any other consumer.

Blast radius

gitnexus was not available per-worktree here, so this was measured by call-graph grep across crates/ plus a direct read of each call site - a permitted fallback, stated rather than implied.

SymbolCallersRisk
CapsuleWarmer::warm_with_configwarm, warm_capped only, both in this filelow
WarmFailure::PullFailedzero references outside module_reshare.rs; 4 sites, all in-filelow - the variant gains a field, so any external match would fail to COMPILE rather than silently misbehave
NatModuleTransport::get_module_infothe ModuleTransport impl only; reached by dig-download's fetch_module_info through the traitmedium - the behaviour change is a longer worst-case wait per holder, bounded at 65 s
descriptor_over / connectunchanged; now invoked from inside the retry closurelow

No HIGH/CRITICAL. The one behaviour a reviewer should weigh: an unanswerable holder now costs up to 65 s instead of failing immediately, and an_unanswerable_holder_costs_exactly_the_ladder pins that bound from the upper side.

Tests

New, in module_transport.rs, with the bound pinned from BOTH sides:

  • the_capsule_that_failed_in_the_field_is_obtained_under_the_new_bound
  • the_same_capsule_is_lost_under_the_field_deadline - the same 4.0 s fixture under the 2 s bound the field behaved as if it had
  • a_capsule_an_order_of_magnitude_larger_needs_the_later_rungs - asserts the ask COUNT is 3, so it cannot pass on rung 1
  • one_rung_sized_for_the_measured_capsule_loses_the_larger_one
  • a_refusal_does_not_climb_the_ladder
  • an_unanswerable_holder_costs_exactly_the_ladder

Fixture sizes are taken FROM the measurement rather than picked: 4.0 s is the measured 135 MB cost, and 30 s is the same ~34 MB/s applied to 1 GB.

New, in module_reshare.rs:

  • a_failed_warm_carries_the_underlying_cause_not_a_constant

Mutation proofs, by name

RevertedTest that reddened
the match back to the discarding let...elsea_failed_warm_carries_the_underlying_cause_not_a_constant
DESCRIPTOR_ASK_DEADLINES collapsed to one 5 s runga_capsule_an_order_of_magnitude_larger_needs_the_later_rungs (12 passed, 1 failed)

A false green was found and fixed by mutation during this work: the first version of the (a) test asserted logs.contains(REFUSAL) over the whole capture, and it still passed with the let...else restored, because dig-download logs the same reason on its own descriptor-demotion warn. It asserted a property the code genuinely has, on a fixture that could not exhibit the property under test. The assertion moved to the return value.

Known verification gap, stated rather than papered over

The ladder tests exercise ask_within_deadlines directly. They do not prove that get_module_info's call site still uses it - a free function extracted for testability hides its call site, and a real get_module_info test needs a dialable peer. The call site is one expression and is reviewed by eye.

Version

0.141.0 -> 0.142.0 (minor): new observable behaviour on the warm path and a new field on a public enum variant, with no removed API.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Local full-suite note (not a CI failure): dig-wallet's sage::sync_supervisor::tests::stall_evidence_survives_the_end_of_a_session hit its own 120 s wait-for guard during a --workspace --no-fail-fast run that shared the machine with a clippy build, and passes in 0.03 s when run alone. Load-sensitive, in a crate this PR does not touch. Logged here rather than ticketed.

@MichaelTaylor3d

Copy link
Copy Markdown
ContributorAuthor

Follow-up on the review: the call-site gap is now closed

The verification gap I flagged (the ladder tests exercised ask_within_deadlines directly and did not prove the call site uses it) is fixed rather than merely documented — it is the same class as the defect this PR fixes, and download.rs:1397-1401 already records this repo paying for it once.

New test: the_production_get_module_info_climbs_the_whole_ladder. It drives the production entry — ModuleTransport::get_module_info, the method dig-download actually calls — and asserts the ELAPSED VIRTUAL TIME equals the sum of DESCRIPTOR_ASK_DEADLINES. A bypassed ladder yields one deadline; a missing one yields ~zero. Neither can be produced by the helper being correct in isolation.

The hang is placed in a seam the test owns (a find_providers that never returns), not in the network. A real dial to a black-holed address is a race: a routable-but-closed port answers ECONNREFUSED in milliseconds, which is a REFUSAL and correctly spends one rung, so the test's meaning would depend on which error a CI host's network stack happened to produce.

Mutation proof — discriminating, as asked

Reverted the call site alone to a single fixed deadline (&DESCRIPTOR_ASK_DEADLINES[..1]), leaving ask_within_deadlines untouched:

the_production_get_module_info_climbs_the_whole_ladder ... FAILED
assertion `left == right` failed: `get_module_info` must spend every rung of the ladder,
not one fixed deadline
left: 5s
test result: FAILED. 13 passed; 1 failed

Only the new test reddened. All six ask_within_deadlines tests stayed green, which is the isolation the review asked for — had they also reddened, the new test would not have been isolating the call site.


Two items for the live run, recorded rather than fixed

1. The 2.00 s spacing is still UNEXPLAINED — the next three-machine run must confirm the ladder is reached

The field measured three connections at 2.00 s spacing. I could not find that bound: it is not in dig-node, not in dig-nat 0.18, not in dig-peer 0.11, and no descriptor timeout exists anywhere on that path. dig_nat::NatConfig::per_method_timeout defaults to 5 s and the module transport is built with default_rpc_timeout() (also 5 s); neither is 2 s.

This ladder replaces an unchosen implicit bound with a chosen explicit one, which is strictly better regardless. But an implicit bound shorter than rung 1 would defeat the ladder silently: the ask would be killed at ~2 s on every rung, all three rungs would elapse against a bound that is not the one this code sets, and the symptom would be unchanged while the tests stay green.

What the run must confirm: that a failing descriptor ask now takes ~65 s in total (rungs of 5 s, 15 s, 45 s) rather than ~6 s (three rungs each truncated at 2 s). The per-rung DEBUG line module pull: descriptor ask exceeded its deadline; re-asking on the next rung carries rung and deadline_secs; compare the interval between consecutive lines against deadline_secs. If they disagree, an implicit bound is still in force and must be found before this fix can be called complete.

Secondary check while there: the holder should log dig.getModuleInfo answeredonce, and the requestor should then succeed on a later rung from the warmed memo — success on rung 2 or 3 is the expected shape, not a failure.

2. Scoped log-capture tests are order-dependent (noted, not swept)

tracing caches per-callsite interest, so a scoped set_default subscriber never observes a callsite an earlier GLOBAL subscriber already registered as disabled. A test asserting on a scoped capture of an info! can therefore pass in isolation and fail in the full suite — which is exactly what happened to the first version of this PR's regression test, and is why the cause now lives in the return value.

Other such tests exist in this repo (peer.rs's capture_logs battery). They are not touched here and no sweep is done in this PR; recording it so the sweep has a home.

@MichaelTaylor3dMichaelTaylor3d left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Correctness gate: PASS

Head reviewed: f7501b69d41ba7c58e1f97bd65e905f414805114 (resolved from the remote, matches the dispatch).
Tier: correctness. No custody, no crypto, no wire change — the proxy flag and the framing are untouched.
Read-only review from a detached worktree at C:\tmp\worktrees\gate319; the shared checkout was not mutated.

The three questions I was asked to answer

1. Does the call-site test genuinely isolate the call site from the helper? YES — reproduced.

I applied the claimed mutation myself: line 510 only, &DESCRIPTOR_ASK_DEADLINES -> &DESCRIPTOR_ASK_DEADLINES[..1],
with ask_within_deadlines untouched. Result:

the_production_get_module_info_climbs_the_whole_ladder ... FAILED
left: 5s right: 65s
test result: FAILED. 13 passed; 1 failed

Exactly one test reddened and it is the new one. All six ask_within_deadlines tests stayed green, as did the
other transport tests. That is real isolation, not a shared-fixture coincidence: the assertion is on elapsed
virtual time throughModuleTransport::get_module_info, which a correct-but-unused helper cannot satisfy.
The download.rs:1397-1401 lesson is genuinely closed for this call site.

Unmutated baseline: cargo test -p dig-node-core --lib seams::dig_peer:: -> 386 passed, 0 failed.

The fixture choice is sound. Putting the hang in HangingLocator::find_providers rather than in a real dial
is correct, and for the stated reason: connect reaches the locator through dial_targets ->
discovered_candidates, so the hang lands inside production's own dial preparation rather than beside it. A
routable-but-closed port really would return ECONNREFUSED in milliseconds, which this call site maps to
Ok(None) — a refusal, one rung — so a network-based fixture would assert 5s or 65s depending on the CI host's
error mapping. That is a test of the host, not of the ladder. The owned seam is the right instrument.

2. Does a refusal spend exactly one rung? YES, in the helper and on production's path.

module_transport.rs:427 returns on Ok(None) without consuming another rung, and the production closure
funnels every non-answer to None (:511connect(...).ok()?, :515descriptor_over). So a holder that
answers "not held", refuses the connection, or is simply unreachable costs one rung, not three.

This matters more than the ladder itself for the cost question. The union locator makes every connected peer
a candidate provider, and a connected peer answers getModuleInfo with a miss — a refusal — so it spends one
rung. Only a holder that accepts a connection and then stalls costs the full 65 s. The amplification worry
is therefore much smaller than providers x 65s.

On the 65 s worst case and what bounds it.dig-download 0.19.0module.rs:596 iterates every
not-yet-demoted provider, recording each recoverable transport error and continuing, so one pull's worst case is
stalling_holders x 65s. The ceiling above it is WarmRegistry::max_concurrent (default 4), which SKIPS
rather than queues — so at most 4 warms, hence at most 4 of these asks in flight from this node at a time. A warm
is a background task; nothing user-facing waits on it. Acceptable.

3. Is the blocking-task-warms-the-memo claim true? YES — verified end to end.

  • lib.rs:2741describe_held_module dispatches describe_module inside tokio::task::spawn_blocking.
  • module_serve.rs:104describe_module is a plain sync fn that writes the memo at module_serve.rs:143
    inside the closure, not after the await.
  • A spawn_blocking closure that has started cannot be cancelled; dropping the JoinHandle detaches it.

So an ask abandoned at rung 1 still runs to completion on the holder and populates the memo, and the rung-2
re-ask hits module_serve.rs:119's memo lookup and returns in milliseconds. The claim holds, and it is what
makes 65 s a ceiling rather than a routine cost. (The memo is (len, mtime)-fingerprinted and LRU-capped at 512,
so the warm entry is not stale-servable and not unbounded.)

The rest

  • Error surfacing. The value-carrying version is what landed: WarmFailure::PullFailed { reason: String }
    at module_reshare.rs:191, let...else -> match at :601, cause taken from error.to_string(). Not a
    log-only fix, so the tracing per-callsite-interest trap the lane hit cannot recur.
  • Peer ids still sentinelled (#1603). Verified through the actual formatter, not the comment:
    DownloadError::Transport's #[error(...)] is hex64_or_sentinel(provider, "peer-id") +
    sanitize_untrusted_text(reason, MAX_ERROR_REASON_CHARS), Debug delegates to that same Display, and the
    terminal NotFound text embeds HolderReasons, which sentinels and sanitizes per entry at
    module.rs:1023/1028. error.to_string() therefore cannot carry peer-authored text into the outcome.
  • Nothing regressed.HolderClaim::Suppress and the .relay sidecar (module_reshare.rs:252/337/350/467)
    are outside the diff. Resolving real test names rather than the string relay_capsule (which, as warned,
    matches no test--lib module_relay printed 0 passed and exited 0): the three that guard this are
    a_relayed_capsule_is_cached_without_announcing_while_a_local_one_announces,
    a_relayed_capsule_stays_out_of_a_later_unrelated_announce_cycle, and
    a_relayed_capsule_becomes_announceable_once_this_node_warms_it_for_itself — all green in the 386.
  • SPEC.md gained a normative paragraph covering both halves (bound-and-re-ask, complete-a-begun-describe,
    do-not-re-ask-a-refusal). It agrees with the code; no superseded phrasing survives elsewhere in §21.
  • Gates.check-merge-preconditions.sh --allow-draft -> RESULT: preconditions MET [EXIT 0], all five
    required contexts present and SUCCESS by name, unresolvedReviewThreads=0, mergeStateStatus=CLEAN.
  • Version.origin/main still 6ef2ed3 at 0.141.0, PR at 0.142.0, main is an ancestor of the head.
  • Recorded-not-fixed, confirmed recorded, not gated: the unexplained 2.00 s spacing (with the ~65 s vs ~6 s
    acceptance check and the per-rung DEBUG line); dig-download's MAX_DESCRIPTOR_ATTEMPTS gap as
    dig-download#37 behind #3152 — I confirmed the pin is a clean dig-download = "0.19" from crates.io with
    nothing vendored or [patch]ed; the load-sensitive dig-wallet stall-supervisor test.

Two non-gating observations posted inline and resolved by me. Do not undraft or merge on my account — that is
the orchestrator's call.

Comment threadcrates/dig-node-core/src/seams/dig_peer/module_transport.rs
Comment threadcrates/dig-node-core/src/seams/dig_peer/module_transport.rs
@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 23, 2026 14:53
@MichaelTaylor3d
MichaelTaylor3d merged commit 9869f95 into mainAug 23, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the loop/3128-capsule-warm branch August 23, 2026 14:53
Sign up for freeto 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.

capsule warm discards its own pull error and abandons a legitimately-slow descriptor

1 participant

@MichaelTaylor3d