Skip to content

feat(cache): retract evicted capsules through dig_sex::holdings - #280

Merged
MichaelTaylor3d merged 9 commits into
mainfrom
loop/267-holdings-wiring
Aug 20, 2026
Merged

feat(cache): retract evicted capsules through dig_sex::holdings#280
MichaelTaylor3d merged 9 commits into
mainfrom
loop/267-holdings-wiring

Conversation

@MichaelTaylor3d

@MichaelTaylor3dMichaelTaylor3d commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Closes#267

Parent epic: https://github.com/DIG-Network/dig_ecosystem/issues/3138

What was wrong

Node::evict_modules_locked deleted capsules and returned (). The victim list it had already computed was discarded on the floor, so nothing downstream ever learned a capsule had left. The node kept naming itself a provider for content it had deleted, and every reader that acted on that record paid a dial for a guaranteed miss. The advertiser sees nothing wrong; only the dialler pays.

There was a second, quieter half. announce_and_bound_after_land ran refresh_dht_inventory() and then evict_modules_locked() — advertisement first, deletion second. So even on the one path that did re-advertise, the reconcile ran against a world in which the victim was still present. The capsule was announced, then silently removed, and stayed advertised until some unrelated inventory change happened to reconcile it. On a quiet node that is never.

What changed

dig_sex::holdings decides the delta; the node performs the I/O. No new advertisement mechanism was built.

  • crates/dig-node-core/src/lib.rs:2340evict_modules_locked now returns Vec<dig_sex::CapsuleIdentity>: the capsules it actually deleted, not the ones the policy nominated. A nominated victim whose remove_file failed is still held and must still be advertised, so retracting it would make the node invisible for content it can serve.
  • crates/dig-node-core/src/lib.rs:2228evict_modules_if_needed feeds that list to dig_sex::holdings::after_eviction and re-advertises. This one site covers every sweep-only caller — the read-path sync (sync_module_and_bound), the tier-0 precache loop (tier0_live.rs:392), and the reshare warm — without touching any of them.
  • crates/dig-node-core/src/lib.rs:2255 — new Node::advertise_holdings_change(&HoldingsDelta): gates on HoldingsDelta::is_empty() and otherwise calls the existing refresh_dht_inventory. The gate is load-bearing, not cosmetic: a reconcile is a Kademlia round trip per changed id, and the read-path sweep runs after every capsule land, the overwhelming majority of which sacrifice nothing.
  • crates/dig-node-core/src/seams/capsule/capsule_store.rs:399announce_and_bound_after_land now sweeps first, then advertises one after_admission(admitted, &evicted) delta covering both the arrival and its cost. after_admission always announces the admitted capsule, so a land that evicts nothing advertises exactly as before; the retraction is additive, never a replacement.
  • crates/dig-node-core/src/seams/capsule/capsule_store.rs:279, :389 — the two land call sites pass what they admitted.
  • crates/dig-node-core/src/capsule_key.rs:166CapsuleKey::identity(). Infallible by construction (parse already admitted only canonical 64-hex), which is why the conversion lives on the type rather than at each call site where it would need a fallback for a case that cannot occur.
  • SPEC.md §19.3 — the normative retract-on-inventory-loss clause, beside the existing announce-on-inventory-gain rule, including the ordering requirement.

The existing advertisement path, reused

Node::refresh_dht_inventory (seams/dig_peer/peer_network.rs:85) → the refresher installed at peer.rs:2722reconcile_and_flood (peer.rs:955) → holdings::reconcile_and_announce (seams/dig_peer/holdings.rs:800) → DhtHandle::reconcile_inventorysync_inventory (seams/dig_peer/dht.rs:540), which calls announce_provider on gain and the activeretract_own_provider on loss, and floods the matching signed opcode-222 Add/Remove deltas from that same reconcile.

Routing through that rather than announcing the crate's delta directly is deliberate: one advertisement path means a retraction can never disagree with the provider record it retracts.

Blast radius

Tooling: ripgrep plus direct reads, not gitnexus. No gitnexus MCP tools were available in this session, and the repo checkout is the primary shared submodule checkout rather than an isolated worktree, so a per-worktree analyze was not run.

Symbols edited and their callers:

SymbolCallers foundHandled
evict_modules_locked2 (evict_modules_if_needed, announce_and_bound_after_land)both updated
evict_modules_if_needed4 (sync_module_and_bound, tier0_live.rs:392, module_reshare doc ref, 3 tests)signature unchanged
announce_and_bound_after_land2 (both in capsule_store.rs)both updated
CapsuleKeyadditive method onlyno existing caller affected

Risk: MEDIUM, not high. The change is additive at every seam except one — the land-path ordering swap, which is a genuine behaviour change and is the point of the fix. Nothing custody-, crypto-, or wire-format-touching. No public API of a published crate changes (dig-node-core items here are pub(crate)).

Evidence

Full library suite on the fix: 854 passed, 0 failed.

Three new tests:

  • an_eviction_advertises_after_the_victim_is_gone (lib.rs) — the sweep-only path.
  • a_sweep_that_evicts_nothing_advertises_nothing (lib.rs) — the control for the emptiness gate.
  • a_land_that_evicts_advertises_after_the_sweep (seams/capsule/push_capsule.rs) — the land path, driving a real push_capsule through land_capsule_bytesannounce_and_bound_after_land.

On fixture design. A call counter would have been the obvious spy and would have been blind to half the defect. The bug has two shapes: no advertisement at all, and one placed before the delete. A counter reports one round under both orderings while the retraction is still never computed — so a placement fix needs an observation that placement can move. The spy therefore snapshots the on-disk capsule set inside the round, and each test asserts the victim is absent from it while the survivor is present. An early round still lists the victim and fails.

This mattered concretely on the land path: install_announce_counter, already used by the two tests either side of the new one, passes identically under both orderings.

The a_sweep_that_evicts_nothing_advertises_nothing control exists because without it, "refresh unconditionally on every sweep" — strictly wrong, a Kademlia round trip per read-path land — would pass the first test. It shares one fixture with it and differs in exactly one variable, the cache cap.

Revert-proof 1 — the sweep path

lib.rs backed up by file copy (never git checkout; sibling lanes are live in this repo), the wiring line in evict_modules_if_needed replaced with let _ = evicted; — the exact pre-fix behaviour of discarding the victim list:

test tests::a_sweep_that_evicts_nothing_advertises_nothing ... ok
test tests::an_eviction_advertises_after_the_victim_is_gone ... FAILED
assertion `left == right` failed: an eviction MUST drive exactly one advertisement round; got []
test result: FAILED. 16 passed; 1 failed

Fails on got [] — no advertisement round at all, the defect exactly.

Revert-proof 2 — the land path

Added after the gate measured that reverting onlyannounce_and_bound_after_land left the suite fully green, i.e. the site the ticket was actually about was unfalsifiable. capsule_store.rs backed up by file copy, the tail restored to refresh_dht_inventory()evict_modules_locked() with the delta ignored, and the full lib suite re-run:

thread 'seams::capsule::push_capsule::tests::a_land_that_evicts_advertises_after_the_sweep'
panicked at crates\dig-node-core\src\seams\capsule\push_capsule.rs:780:9:
the advertisement must run AFTER the sweep, so the evicted filler is no longer in the set it
advertises; saw ["22785d2b…/7b4e93fc…", "bababa…/cdcdcd…"]
test result: FAILED. 853 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

One failure, and it is the new test. The message names the tier-0 filler (bababa…/cdcdcd…) still present in the set the node was about to advertise — which is the ordering defect, observed rather than inferred. 853 passed matches the pre-existing green count, so nothing else moved.

Both files restored from their backup copies; working tree verified clean against HEAD afterwards. cargo clippy --workspace --all-targets -- -D warnings clean; cargo fmt --all --check clean.

on_disk_capsules and install_inventory_snapshot_spy moved into test_support so both suites share one implementation rather than duplicating the spy.

Where the ticket was wrong against the code

Two corrections, one of them load-bearing.

  1. "dig-node imports not one symbol from dig_sex" is not true, and has not been since v0.130.0 shipped hours ago.lib.rs already uses dig_sex::TieredPolicy, dig_sex::CapsuleIdentity, dig_sex::CacheTier and dig_sex::SelectionSeed. What was unused is specifically the holdings module. The eviction decision was already folded onto the crate; only its advertising corollary was missing.

  2. holdings::reconcile is deliberately not wired, because the reconcile it describes is already implemented in-tree. The ticket asks for it as the drift-repair path. DhtHandle::reconcile_inventorysync_inventory (dht.rs:540) already diffs the remembered advertised set against the current held set and performs both halves of the repair from that one delta: announce_provider on gain, the active retract_own_provider on loss, and the signed opcode-222 Add/Remove flood derived from the same delta (holdings.rs:806, peer.rs:955). Wiring holdings::reconcile beside it would be a second, weaker implementation of an existing one — weaker because it computes only the delta and is not connected to the I/O or the flood.

    An earlier version of this section argued the call would be "vacuous by construction", and that argument was wrong — recorded here because it points a future reader in the opposite direction. It described only the bring-up instant, where advertised is seeded from held (peer.rs:2925-2932). But DhtHandle remembers its announced set for the whole process lifetime, so from the first inventory change onward the two genuinely diverge and the delta is routinely non-empty. The reason to decline is duplication, not emptiness.

    Separately, and for the same reason: dht.rs:inventory_diffisholdings::reconcile, re-implemented over ContentId instead of CapsuleIdentity. Unifying them needs a type change in one or the other, so it is left alone and recorded as a follow-up rather than done here.

    The one thing genuinely not repairable today is drift in records held at remote peers after an unclean shutdown: this node cannot retract those without a persisted record of what it previously advertised, and they age out via the provider TTL. That needs a persisted advertised set — a separate change, out of scope for the thinnest path here.

Version

0.130.00.131.0, minor: new observable behaviour (a node now retracts what it evicts), no breaking API change. Cargo.lock updated for dig-node-service.

MichaelTaylor3dand others added 5 commits August 20, 2026 08:16
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>

@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.

CHANGES-REQUIRED — head d3fdbb46e46a9602d6a85ea1aef49313ae8dbfa1

One gating finding. The fix itself is correct at both sites; the gap is that the site where the
live defect actually lived has no test, and I proved that by reverting it.

What I verified positively

  • Ordering is correct at every site that both evicts and advertises. There are exactly two:
    lib.rs:2231-2236 (sweep) and capsule_store.rs:404-411 (land). Both now delete first. The
    response-cache LRU (lib.rs:2143) is not an advertised inventory and is correctly untouched.
    After this PR refresh_dht_inventory() has exactly one call site (lib.rs:2260), so every
    advertisement now routes through the delta gate — that consolidation is worth more than the fix.
  • Revert-proof 1 reproduces exactly as claimed. Replacing the wiring with let _ = evicted;
    fails an_eviction_advertises_after_the_victim_is_gone with got [], while
    a_sweep_that_evicts_nothing_advertises_nothing stays green.
  • The spy's shape is sufficient, and I did not take that on argument. I reordered
    evict_modules_if_needed to advertise-then-evict (the second defect shape a call counter is blind
    to) and the test failed with its own intended message, listing the victim still on disk. Both
    tests failed under that probe. The decision not to use a counter was the right one and it is
    empirically load-bearing, not decorative.
  • CapsuleKey::identity() cannot panic.parse (capsule_key.rs:149) is the sole constructor
    and admits only 64 ASCII hex digits; hex64 checks length then decodes. The expect is
    discharged by the type.
  • evict_modules_locked's new return value is consumed at both callers. No silent drop.
  • SPEC §19.3 states the ordering normatively, not descriptively — "the sweep MUST run BEFORE the
    advertisement", with the reason (a quiet node never reconciles). That is a contract a
    reimplementer can be held to.
  • 853 tests pass; the claimed local evidence holds.

The gating finding

The reported defect was at capsule_store.rs:399-401 — the land path. That is the one site
with zero test coverage, so the fix there is unfalsifiable. Detail on the inline thread.

Non-gating (resolved by me, no action needed)

The refusal to wire dig_sex::holdings::reconcile reaches the right conclusion — declining to
write a call that provides no coverage is correct behaviour and is recorded as such. The stated
reason is inaccurate, and the accurate one is stronger. See the inline note.

Comment threadcrates/dig-node-core/src/seams/capsule/capsule_store.rs
Comment threadcrates/dig-node-core/src/lib.rs
MichaelTaylor3dand others added 4 commits August 20, 2026 10:07
…che cap
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Not part of the holdings work. CI uses dtolnay/rust-toolchain@stable, which floats,
and 1.98.0 shipped 2026-08-18 carrying clippy::chunks_exact_to_as_chunks. So this
branch inherited a red gate rather than causing one; dig-app broke the same way at the
same moment on unrelated files.
for chunk in point.chunks_exact_mut(8) -> point.as_chunks_mut::<8>().0
Behaviour-identical: as_chunks_mut yields the same four 8-byte windows, typed as
[u8; 8] rather than a slice, so the big-endian packing and therefore seeded replay of
every sampled keyspace point are unchanged.
Chose the fix over an #[allow]. Suppressing a new lint on first contact is how a
codebase accumulates them.
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 20, 2026 19:31
@MichaelTaylor3d
MichaelTaylor3d merged commit 1db4d6e into mainAug 20, 2026
15 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the loop/267-holdings-wiring branch August 20, 2026 19:31
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.

Wire dig-sex holdings (SPEC 7) into the DHT announce/retract path

1 participant

@MichaelTaylor3d