fix(peer): derive dig.getAvailability from the servable module, not a snapshot

[MichaelTaylor3d]

The reproduced mechanism (#1592)

The holder answers dig.getAvailability = false for a capsule it can serve.

Two paths answered the same question from two different sources of truth:

	source
availability_presence (root/resource granularity)	the cache_list_cached() DIRECTORY-WALK snapshot
serve_local_blocking → dig.fetchRange	<cache>/modules/<store>/<root>.module, read directly

availability_batch takes that snapshot ONCE and then answers each item against the slice, awaiting a spawn_blocking module decode per item — so the window between "snapshot taken" and "answer produced" is real. Any capsule landing inside it (a hosted pin, a §21 sync, an on-demand cache.fetchAndCache, a chain-watch gap-fill, the read-side backfill) is already servable but absent from the snapshot → the node answers not available for content it would serve. The reverse drift also held: a snapshot predating an EVICTION claimed availability the node could no longer serve.

Why that is read-killing rather than cosmetic: dig-download's locate_and_confirm DROPS every provider whose answer is not available BEFORE issuing any fetchRange. A single stale no removes a genuine holder and the read 404s. PR#100 worked around it reader-side for CONNECTED-pool holders only (connection = confirmation), so a DHT-discovered stranger still ran the real confirm — the discover→read leg of the MVP flywheel (#1425).

Note on the ticket's stated hypothesis: cache_list_cached() is a LIVE walk per batch (there is no retained/cached snapshot to go stale between requests), so the lag is strictly the intra-batch snapshot→answer window plus the walk's own duration, not a long-lived cached inventory. The divergence itself is proven directly by the RED tests below.

Failing-test evidence (RED, pre-fix)

availability_answer_reports_a_capsule_that_landed_after_the_inventory_snapshot ... FAILED
assertion failed: a servable capsule must be reported available even if the snapshot predates it
left: Bool(false) right: true
availability_answer_reports_not_available_when_the_snapshot_lags_an_eviction ... FAILED
assertion failed: an evicted capsule must not be reported available
left: Bool(true) right: false

The first test asserts the precondition first (serve_local_cached(...).is_some()), so it fails for exactly the right reason: the resource IS servable and availability says no.

The fix — answer from the servable source (cannot drift by construction)

peer::availability_presence takes a capsule_servable: bool and, at root/resource granularity, IS that flag. The caller computes it as module_exists(&self.cache_dir, store, root) — a single existence check of the very file the serve path reads. It is not "refresh the snapshot more often" (that only shrinks the window): the answer and the action now read the same source, so no window exists. The inventory walk remains ONLY for the STORE-granularity roots enumeration, which a single-path check cannot answer.

Semantics preserved: capsule-granularity availability (a resource in a held capsule → available; retrieval_key still not required to match a per-resource record), and a capsule genuinely not held still answers false — the answer is not weakened to an unconditional available in either direction.

Cost bound (peer-reachable path, §7.4)

• Per item: one path stat instead of a whole-cache directory walk. Strictly cheaper than before.
• Per batch: the walk runs at most once, and only when some item asks at store granularity — a batch of root/resource items (what a downloading peer actually sends) does zero directory walks. The MAX_AVAILABILITY_ITEMS cap is unchanged.
• Peer-supplied store_id/root now feed a path, so they are validated canonical 64-hex before the join (the same path-traversal guard cache.removeCached applies); a crafted key answers not-available without touching the filesystem. Covered by a traversal test.

Tests (RED → GREEN)

New, in dig-node-core:

• availability_answer_reports_a_capsule_that_landed_after_the_inventory_snapshot — the regression (RED above).
• availability_answer_reports_not_available_when_the_snapshot_lags_an_eviction — the other direction (RED above).
• availability_batch_reports_a_capsule_landed_at_runtime_and_stops_after_eviction — the public peer-facing path: landed → available, never-held → not available, evicted → not available.
• availability_batch_rejects_a_non_canonical_key_without_touching_the_filesystem — traversal-shaped + non-hex keys, with a real file planted outside the modules tree.
• availability_root_granularity_answers_from_the_servable_flag_not_the_snapshot — replaces the old snapshot-based unit test.

Verification:

• cargo test -p dig-node-core --lib → 347 passed, 0 failed (existing peer/availability tests green, including connected_pool_holder_is_fetched_even_when_it_answers_availability_false from fix(read): bypass getAvailability confirm for connected-pool holders (#836) #100).
• cargo test -p dig-node-core --tests → 347 + 8 + 2 + 2 passed (incl. the peer_network mTLS integration test that drives a real framed dig.getAvailability batch).
• cargo clippy --workspace --all-targets --locked -- -D warnings → clean.
• cargo fmt --all --check → clean.

Blast radius checked

gitnexus is disabled in the loop (§2.0 temp override), so this was done by grep + direct read. availability_presence callers: Node::availability_answer (lib.rs) + 3 unit tests — all updated. availability_batch callers: dig_rpc::dispatch (dig.getAvailability), NodeResponder::handle_availability (the peer mTLS stream), and tests — signature unchanged. module_exists is unchanged (still the chain-watch gap-fill held-check); sync_eligible now delegates to the extracted is_canonical_capsule_key with identical behaviour. Nothing in the fetchRange serve handler is touched (PR#87's lane).

Coherence

• SPEC.md §19.3: the invariant — the availability answer MUST agree with what the node can serve, derived from the servable source (or a cache invalidated on every inventory-changing write), never from a snapshot that can lag a write; plus the both-directions consequences and the cost bound.
• DEVELOPMENT_LOG.md: the snapshot-lag class (a peer-facing ANSWER and the ACTION it gates reading different sources will eventually lie).
• SYSTEM.md (superproject) untouched — no cross-repo wire change; the wire shape is byte-identical.

Version

0.58.9 → 0.58.10 (workspace/binary), dig-node-core0.18.4 → 0.18.5. Patch: behaviour-restoring, no API/wire change. Cargo.lock re-locked in the same commit.

Refs #1592, #1425. (Closes nothing — the super-repo ticket is closed by the orchestrator.)

[MichaelTaylor3d]

VERDICT: CHANGES-REQUIRED (1 gating finding; the core fix is correct)

(Recorded as a COMMENT review: the review identity is the PR author, so GitHub rejects both --approve and --request-changes with 422. The verdict above is the binding one; the gating thread stays OPEN until fixed.)

Independent correctness review, fresh context. The CORE fix is right and I confirmed each claim in the PR body:

• One source of truth (confirmed).module_exists -> module_path(dir, store, root) (crates/dig-node-core/src/lib.rs:760,769) is byte-for-byte the same path serve_local_blocking reads (lib.rs:1015). Answer and action now read the same file; the snapshot->answer window is closed by construction, not shrunk.
• Both directions honest (confirmed). Root/resource is exactly capsule_servable (peer.rs:404-408); the landed-after-snapshot and lags-an-eviction tests are genuinely discriminating (the PR body's RED output shows both failing pre-fix, one in each direction). Not weakened to unconditional available. Capsule-granularity semantics + the ignored _retrieval_key preserved.
• Store granularity still enumerates from the walk — correct, a single-path stat cannot answer roots.
• Guard is real.is_canonical_capsule_key is applied to BOTH store and root (lib.rs:1766), and .filter(...) runs BEFORE .map(module_exists), so a rejected key answers not-available with zero fs access. sync_eligible delegates with byte-identical behaviour (compared against origin/main:lib.rs:929-933); cache_remove_cached (seams/capsule/capsule_store.rs:155) is untouched, so no caller weakened.
• Cost bound (confirmed). One stat per item; the walk runs at most once per batch and only when some item omits root, so a downloading peer's root/resource batch does ZERO walks. MAX_AVAILABILITY_ITEMS cap unchanged. No per-item walk remains.
• Version/coherence. 0.58.10 + core 0.18.5, Cargo.toml and Cargo.lock agree, version-increment gate green. SPEC §19.3 states the invariant, DEVELOPMENT_LOG records the class. Reads cleanly (§2.5) — doc-comments carry the WHY, no cruft.
• Single-writer respected: the diff does NOT touch the fetchRange serve handler (PR#87/#1577's lane).
• dig-constants check (both questions asked): (1) nothing in this diff is a shared/cross-repo value defined locally — no new constant, asset id, address, puzzle hash, port, or version pin is introduced; is_canonical_capsule_key is a local predicate, not a shared value. (2) nothing here hardcodes a literal dig-constants publishes or should; the <cache>/modules/<store>/<root>.module layout is pre-existing and unchanged by this PR. No drift introduced, nothing to relocate.
• Gates: all 13 checks pass (Clippy, Rustfmt, Test + coverage, CodeQL x3, commitlint, version increment, 3 installer builds). Zero pre-existing review threads.

The gating finding

The traversal test for the NEW path-traversal guard passes with the guard removed — it does not defend the guard it was written for. Detail inline. One-line fix.

No other blocker: fix that one test and this is a PASS.

[MichaelTaylor3d]

VERDICT: PASS (re-review round 2, head 2746df13)

(Recorded as a COMMENT review — the review identity is the PR author, so GitHub 422s --approve. This body is the binding PASS. Branch protection requires 0 approvals; merge is gated on all checks green + zero unresolved threads.)

The delta vs my last review (afa3e7e..2746df1) is exactly the two fixes and nothing else: crates/dig-node-core/src/lib.rs only, +54/-9, confined to availability_batch's needs_inventory predicate and the test module. No production behaviour outside that predicate changed, and no version bump was needed (0.58.10 / core 0.18.5 already correct for the merged unit).

(a) The traversal test now genuinely discriminates — independently proven, not taken on faith

I did NOT rely on the reported RED. I reproduced the path arithmetic directly against the test's own on-disk layout (cache_dir == td.path(), a real <cache>/modules/<store>/ created by seed_served_capsule, the planted <cache>/secret.module):

EXISTS: modules/../secret.module <-- store_id="..", root="secret" (item 0)
EXISTS: modules/./../secret.module <-- store_id=".", root="../secret" (item 1)

Both new keys resolve to the planted file. So with is_canonical_capsule_key removed, module_exists returns true for each, and availability_presence's root-granularity arm is unconditionally capsule_servable (peer.rs:404-408 — no other predicate can rescue it), so the answer becomes available: true and BOTH assertions fail. The test now fails without the guard, by construction. Contrast the previous key, which resolved to <cache>/../secret.module (absent) and passed either way. The corrected comment (lib.rs:4819-4824) now accurately describes what the keys do. The "zz".repeat(32) non-hex case is retained and still meaningful.

(b) The needs_inventory change is correct and its test is non-vacuous

lib.rs:1821-1823 now reads root exactly as availability_answer does (and_then(Value::as_str)), so the batch's walk gate and the answer's granularity switch can no longer disagree — the same "two reads of one field" class the PR fixes elsewhere.

availability_batch_null_root_still_takes_the_inventory_snapshot (lib.rs:~4856) discriminates: revert the predicate to .is_none() and {"root": null} yields Some(Value::Null) → gate false → no walk → empty snapshot → availability_answer still routes to STORE granularity (its as_str() collapses null) → available:false, roots:[], failing the asserted true and the roots == [root] assertion. It also covers the numeric-root shape. Correct direction, and it asserts the roots CONTENT, not just the boolean.

No new cost amplification: a root:null item now costs what an omitted-root item always cost — one walk, at most once per batch, MAX_AVAILABILITY_ITEMS unchanged. The planted <cache>/secret.module sits outside <cache>/modules/, so it never enters the inventory walk.

(c) Nothing else regressed

Re-verified at the new head: one-source-of-truth intact (module_exists→module_path, lib.rs:760/769, the same path serve_local_blocking reads at lib.rs:1015); both-direction honesty intact (landed→available, evicted→false, never-held→false); the guard still applied to BOTH store and root with .filter before .map so a rejected key touches no filesystem (lib.rs:1766); store granularity still enumerates from the walk; sync_eligible still byte-identical to origin/main; cost bound intact (one stat per root/resource item, zero walks for a downloading peer's batch). The fetchRange serve handler is still untouched — single-writer with PR#87/#1577 respected. SPEC §19.3 + DEVELOPMENT_LOG unchanged and still accurate.

(3) The cache_remove_cached duplicate is_hex64

Agreed — correctly kept OUT of this PR's radius. That path is eviction/unlink code with its own canonicalize + starts_with containment check; expanding into it here would widen the blast radius for zero behavioural gain. Separate cleanup, non-blocking, my thread stays resolved.

Gate state

At 2746df13: Clippy, Rustfmt, commitlint, version-increment, CodeQL + Analyze (actions / js-ts / rust), build .deb all pass; Test + coverage, build .msi, build .pkg still pending at review time. All 3 of my threads resolved, 0 unresolved on the PR. Reported local run 348 lib tests / 0 failed (up 1 = the new null-root test), clippy -D warnings clean, fmt clean.

PASS. Merge once Test + coverage and the two remaining installer builds go green (§2.4a — a green PASS is necessary, not sufficient).