fix(dig-node): sweep modules-cache after every read-path §21 sync attempt - #170
Merged
Merged
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
…e bound is loop-independent The read-path §21 whole-store sync (dispatch `sync_module`) landed a whole capsule via `write_atomic` without running the tier-aware size-cap sweep, unlike the tier-0 precache loop and `cache.fetchAndCache`. With tier-0 disabled and no `cache.fetchAndCache` traffic, remotely-triggered read-path backfill could grow `<cache>/modules` unbounded (#2041 defense-in-depth). Add `Node::sync_module_and_bound` (sync then `evict_modules_if_needed` on success) and call it at the read-path land site. The call site holds no `cache_lock`, so the async variant is correct; the `sync_module_from` choke-point is left unswept (it is also reached by `cache_fetch_and_cache` under the lock). Correct the overstated `capsule_store` comment and the SPEC modules-cache bound. Closes #2041 Co-Authored-By: Claude <noreply@anthropic.com>
…empt, not only on success A read-path §21 sync whose served root differs from the requested one (the upstream head advanced) returns false from `sync_module` yet still lands a whole chain-anchored capsule under the served root via `write_atomic`. Gating the sweep on the true return let repeated head-advance lands grow `<cache>/modules` unbounded — the residual the reviewer flagged. Make `sync_module_and_bound` sweep after every sync ATTEMPT (the scan is bounded + idempotent, so the `false`/`Err` no-land path is a cheap no-op), add a served≠requested regression test, and tighten the SPEC wording. Refs #2041 Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes dig_ecosystem#2041. Defense-in-depth follow-up flagged by the #1934 PR-3 loop-security re-audit.
Why
The tier-aware
<cache>/modulessize-cap eviction fires after the tier-0 land + aftercache_fetch_and_cache, but the read-path §21 whole-store sync did NOT sweep:sync_module_from(lib.rs:1507) lands viawrite_atomic(:1594) and only bumps telemetry. So with tier-0 disabled AND zerocache.fetchAndCachetraffic, a remotely-triggered read-path §21 backfill could grow<cache>/modulesunbounded. Thecapsule_store.rscomment claiming the §21 backfill kept whole-capsule storage bounded overstated.What changed
Node::sync_module_and_bound(lib.rs:1476) — callssync_module, then unconditionally runsself.evict_modules_if_needed().awaitafter the sync attempt, and returnsmay_serve_locally. Wired at the read-path §21 land sitedispatch.rs:768.write_atomicwhilesync_modulereturnsfalse. Gating the sweep on thetruereturn would leave that land unswept (remotely drivable at will with the pin off + an identity configured), reopening the residual. Sweeping after every attempt closes it; the extra bounded, idempotent dir-scan on the no-land path is cheap.dispatch.rs:768holds NOcache_lock(verified), soevict_modules_if_needed(takes the lock fresh) is correct;evict_modules_lockedwould be wrong.sync_module_fromis deliberately left unswept — it's also reached bycache_fetch_and_cachewhile holdingcache_lock, so sweeping there would double-sweep / risk lock inversion.maybe_backfill_capsulealready covered — it →spawn_capsule_backfill→gap_fill_generation→cache_fetch_and_cache(capsule_store.rs:218), which sweeps at:262under the held lock. No change needed (confirmed by trace).capsule_store.rscomment + added SPEC §3.4 (the tier-aware whole-capsule bound + loop-independence across all on-demand land paths, including the served≠requested sync attempt).How verified (TDD)
read_path_sync_module_land_bounds_modules_cache_without_tier0_loop— served == requested: drives the full real sync against a live mock upstream (chain-anchored capsule) with a pre-existing oversized tier-0 module, tiny cap, NO tier-0 loop / nofetchAndCache; asserts the demanded module survives and the oversized one is evicted.read_path_sync_land_under_served_root_bounds_cache_even_when_served_ne_requested— the served≠requested edge: mock §21-clone servesserved=0xAA…forrequested=0xBB…, so a chain-anchored capsule lands under the served root whilesync_modulereturnsfalse; asserts the served-root capsule landed AND the oversized tier-0 module is still evicted. Non-vacuous: re-gating the sweep on the bool makes this test FAIL while the served==requested test still passes.cargo test -p dig-node-core --lib read_path_sync→ 2 passed, 0 failed. fmt clean, build clean.Follow-up (out of scope, filed)
The reshare-warm land path (
module_reshare.rspromote_into_cache) also lands a whole capsule into<cache>/moduleswith no size-cap sweep — pre-existing, self-driven (the reshare flywheel, not a direct remote read), non-gating. Tracked as dig_ecosystem#2053.Version
root
[workspace.package].version0.93.1 → 0.93.2 (patch,fix:);Cargo.lockregenerated.Generated by Claude Code