fix(node): dead transport ctor, temp-dir leak, version-gate reporting, executed sweep examples - #368
Conversation
211e885 to
ca164a0
Compare
Rebase onto PR #369 dry-run — done, clean, and it found one thing that MUST be handledPR #369 ( Result: no textual conflict, and both sides survive
They are complementary rather than competing: #363 documents what each shape DOES, executably; #363's examples are unaffected by the crate-qualification change. They assert only Verified on the rebased tree — What MUST be handled: the rebase drops BOTH version bumps#369 bumps to the same two versions this branch did — root Worth noting for its own sake: this is exactly the failure #364 was filed about, reproduced Not taken into this PR: the
|
Co-Authored-By: Claude <noreply@anthropic.com>
…ctor `ChainTransport::with_sources` had no callers anywhere in the workspace, was `pub` + `#[must_use]`, and built `peer_reads: None` — a transport whose `peak_height` takes `chia-query`'s router path, which asks `api.coinset.org` FIRST. It read like the obvious way to build a transport over the node's own fabric while quietly being the one shape NC-12 exists to prevent: the node's headline chain fact decided by a single third party. Deleted rather than repaired: a constructor kept "in case someone needs it" is how it comes back, and the only production construction (`sage/service.rs:173`) already chains `with_peer_reads`. Makes the deletion durable with a fail-closed source guard: the constructors that may build `peer_reads: None` are a CLOSED enumeration of two (`new`, and the `#[cfg(test)]` `with_client`), and a third one fails the suite by name. Proved load-bearing by reintroducing `with_sources` — the guard reports it. Closes #360 Co-Authored-By: Claude <noreply@anthropic.com>
`content_serve.rs` built its temp path by hand -- `env::temp_dir().join(format!
("dig-node-serve-test-{pid}-{n}"))` -- and nothing ever removed it. Each node
seeds a real compiled `.dig` module and warms a cache, so a run cost ~57 MB per
node. 1,123 trees reached 62.5 GB and took the dev machine to 81 MB free on a
1.9 TB disk, producing a machine-wide ENOSPC that stopped an unrelated lane
mid-build. It is self-concealing: it grows fastest when the suite runs most, so
it reads like a build-cache problem (the first diagnosis blamed worktree
`target/` dirs, which were not the cause).
- A `NodeCache` RAII guard OWNS the tree (`tempfile::TempDir`), so removal
happens in `Drop` -- including on an unwind. Ownership is the point, not the
deletion: a cleanup line at the end of a test is skipped by every failing
assertion, i.e. exactly the runs a developer repeats.
- Both leaking sites in the file are covered (`dig-node-serve-test-*` and
`dig-node-origin-test-*`); the latter now shares the guard.
- The node's detached axum task still holds `wallet.sqlite` open when the test
body returns, and Windows will not unlink an open file, so `TempDir::drop`
cannot remove that last ~1 MB. `Drop` cannot cancel the task (it cannot
`.await`), so the residue is BOUNDED rather than pretended away: a once-per-
process sweep removes trees idle for 15 minutes, which no live run can be.
Measured, same machine, full suite: per-run residue 970 MB -> 17 MB, and the
62.5 GB accumulation is gone. Two tests hold it: one asserts the tree is gone
after a normal drop AND after a `catch_unwind` panic; one asserts the sweep
removes an abandoned tree while sparing a live one and a stranger's directory.
Each was proved load-bearing by reverting only its own fix.
Closes #361
Co-Authored-By: Claude <noreply@anthropic.com>
The version-increment gate held both numbers and reported only pass/fail. A stale bump is invisible everywhere else -- `git status`, the diff, fmt, clippy and the test suite are all green when `main` has taken your version number, and a rebase can drop the bump commit entirely as "already upstream". It went stale three times on one branch (PR #354) across a five-round gate, and each time the lane had to rediscover at merge what the comparison even was. Writes a table to `$GITHUB_STEP_SUMMARY` on EVERY run, success or failure: the base ref actually compared (short SHA of `main`, plus the head SHA), a row per manifest with base, head and verdict, the must-match row where both exist, and the overall verdict spelled out. Being able to READ the comparison while the gate is green is the point -- that is what turns a merge-time surprise into something a long-lived review round can notice. What the gate ENFORCES is unchanged: same predicate, same exit code, same error annotations. `${GITHUB_STEP_SUMMARY:-/dev/null}` keeps the script runnable off Actions, which is how the three cases below were exercised. Bounded to dig-node deliberately (#364, CLAUDE.md 1.3c rule zero). 25 other repos carry a byte-identical copy and 6 carry a diverged one; propagating is a named follow-up with that finite list, not "every repo". Verified by running the extracted script unpiped against scratch repos: passing (RC=0), the stale-bump failure with `main` holding the same version (RC=1), and the both-manifests-agree case (RC=0) -- each producing its summary. Closes #364 Co-Authored-By: Claude <noreply@anthropic.com>
…serting them A doc example in `sources.rs` was exactly backwards -- it claimed the attribute-inline shape failed loudly, when it measures `sites=[2]` and drops the construction -- and it survived FOUR adversarial gate rounds. Everything else in that file was measured; only the examples were prose, and that is where the false statement lived. Reviewers read a doc example as documentation rather than as a claim to test, so the remedy is to make the claim executable. The worked examples are now `json` blocks carrying their fixture AND their expected `sites` / `ended_inside_a_test_item`, and a test extracts every one of them from this file's own source and runs the real classifier over it. Doing this as a rustdoc doctest is not possible and would have been worse than prose: rustdoc does not run doctests on `#[cfg(test)]` items, and this whole module is one, so a ```rust example would compile in nobody's build while reading exactly like a passing test. Fail-closed twice over: an unterminated block is an error, and the example COUNT is pinned, so deleting an inconvenient example -- the cheapest way to green -- fails rather than passing quietly. Found while doing it: the trailing-comment example asserted `sites=[5]` and `sites=[]` for "the same fixture" WITHOUT ever writing that fixture down, so its numbers were unreproducible by construction. The fixture is now explicit and the measured values are `[6]` and `[]`. A claim whose input is missing cannot be checked by anyone, which is worse than a claim that is merely wrong. No classifier heuristic is added. Five rounds established that the durable artifact is the fail-closed flag plus an honestly open enumeration, and `sweep` is untouched -- only the file's statements ABOUT it are now enforced. Each of the four examples was proved load-bearing: altering any expected value, and flipping any documented flag, turns the suite red naming that example. Closes #363 Co-Authored-By: Claude <noreply@anthropic.com>
`ChainTransport::with_sources` was `pub`, so deleting it is a breaking change to this crate's surface even though it had no callers. SemVer minor on 0.x. Co-Authored-By: Claude <noreply@anthropic.com>
53261ee to
753118a
Compare
|
Gate in progress at head Rebase re-verified — both bumps survived. Compilation: #360 guard — verified by mutation, all three directions:
Continuing with the #363 doc-example mutations, the four #361 reverts, and the required-context assertion. |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
PASS
Head reviewed: 753118a4a0ebf3b2e6758522668bb3eac12e43da (resolved from the remote myself, post-rebase). Reviewed in an isolated worktree; no shared checkout was mutated.
No gating findings. No inline threads opened, so nothing here bars the merge. Child of MVP epic dig_ecosystem#2790.
Rebase re-verified - both bumps survived
merge-base(HEAD, origin/main) = 7ea14fee = origin/main tip: a clean fast-forward, no silent drop.
| manifest | main | head |
|---|---|---|
workspace Cargo.toml |
0.154.0 | 0.155.0 |
crates/dig-wallet/Cargo.toml |
0.35.0 | 0.36.0 |
cargo test --no-run --all-targets -p dig-wallet -p dig-node-service compiles clean post-rebase, so no cfg(test) symbol collision is hiding behind a green cargo build.
Gates re-earned at this head
cargo fmt --all -- --check RC=0. cargo clippy --all-targets --workspace -- -D warnings clean. dig-wallet 657 + 22 + 8 passed, 0 failed. dig-node-service --test content_serve 15 passed. All with --all-targets, parsing the test result: line.
Required contexts asserted BY NAME via check-merge-preconditions.sh --repo DIG-Network/dig-node --pr 368, unpiped, gated on exit code - all five present and SUCCESS (Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage), unresolvedReviewThreads=0, mergeStateStatus=CLEAN. It exits 1 solely on draft=true, the correct pre-merge posture; undrafting is yours.
#360 - verified by mutation, all three directions
with_sources is genuinely dead: the only surviving occurrence of the identifier anywhere in the tree is its own doc comment at crates/dig-wallet/src/sage/chain.rs:712. Removal is safe workspace-wide, and dig-wallet 0.35.0 -> 0.36.0 is the correct 0.x semver-incompatible signal for a removed pub fn.
The guard at crates/dig-wallet/src/sage/chain.rs:703-759 held in all three directions I tried to break it:
- Third oracle-first constructor added -> RED, naming it:
not on the closed list ["new", "with_client"]: ["with_registry"]. It fails by name, as the ticket asks. - Prose immunity - I added a doc paragraph to the same file discussing
peer_reads: Nonein prose, including describing the struct-literal form in words. Still green. The struct-literal-field needle (trailing comma) does not count sentences, so the wrong-RED the first version produced does not recur. This was the specific thing worth checking and it is correct. - Vacuity - rendering the needle unmatchable (
peer_reads: None,->peer_reads: Default::default(),) -> RED with "the scan matched nothing, so it proves nothing ... this guard has gone vacuous". A source-scanning guard that fails closed on its own obsolescence is the rare good version of this pattern; the usual one goes quietly green.
The two-fragment runtime assembly does work - the literal never appears contiguously in the file, so the test cannot match itself and report a site that does not exist.
#361 - four reverts, each alone, each firing its OWN assertion
Confirmed the proof is not duration-bounded: both tests assert on the path after the guard is gone, never on Drop having been reached.
| revert | assertion that fired |
|---|---|
dir: ManuallyDrop<TempDir> (no RAII removal at all) |
"a guard dropped normally left its tree at ..." |
leak only under std::thread::panicking() |
"a panic unwound past the guard ... this is the ENOSPC defect" |
age check dropped from sweep_trees_in |
"the sweep deleted a tree young enough to belong to a running test" |
| prefix check dropped | "the sweep removed a directory that is not one of ours at all" |
The self-report is accurate on the subtle point: the plain-leak revert fires only the normal assertion (it is first), which is exactly why the separate thread::panicking() revert was needed to prove the panic half. Both halves are independently proven rather than one standing in for the other.
The live-tree control exists and is load-bearing - dropping the age check turns the test RED on that assertion specifically, so the sweep is provably not "delete everything". On blast radius: sweep_trees_in does a depth-1 read_dir of std::env::temp_dir() only, filtered by the dig-node-serve-test- prefix and by age, treating unreadable metadata as RECENT. It cannot reach C:\tmp\worktrees or any repo checkout, so it cannot delete a sibling lane worktree. The residue judgement is sound and honestly documented: Drop cannot .await to cancel the detached axum task holding wallet.sqlite, so bounding the residue beats pretending it away. The stated "finishes in ~2.5 minutes" is accurate - I measured 122.65s, well inside the 15-minute threshold.
#363 - the rustdoc argument is CORRECT, and measured
I checked the claim rather than accepting it: cargo test --doc -p dig-wallet collects running 0 tests. A fenced rust example inside #[cfg(test)] mod sole_owner_tests would compile in no build at all and read exactly like a passing test - the cfg-gated-and-unfalsifiable trap. The extracted-JSON-executed-against-sweep shape is the right call, and the reasoning holds.
"sweep byte-for-byte unchanged" is confirmed mechanically: every changed line in sources.rs outside the added test fn is a /// line. No production classifier code moved.
Five mutations, all RED:
- documented
sites: [6]->[5](the old, unreproducible number) -> RED - documented
sites: [2, 3]->[2](the original backwards claim) -> RED ended_inside_a_test_itemflipped -> RED- one whole example block deleted -> RED on the pinned count ("an example has been lost and its claim is unchecked again")
- a json fence unterminated -> RED
The corrected numbers are the measured ones. [6]/[] is what the classifier actually returns; the old [5]/[] fails. The second false claim found here is real and its correction is right - the multi-line production item is genuinely necessary, since a one-line fn later() { ... } would end at column 0 and clear the latch, so the drop would not occur and the example would demonstrate nothing.
Also confirmed the new doc blocks containing ChiaQuery::new(c); introduce no new sweep sites - they sit inside the column-0 #[cfg(test)] module, and the 7 sole_owner tests stay green.
#364 - verified unpiped off Actions, four cases
I extracted the run: body and executed it directly, gating on the real exit code:
| case | RC | summary |
|---|---|---|
| base 0.154.0 -> head 0.155.0 | 0 | table + PASS |
| stale bump (main took 0.155.0) | 1 | "does not increment" + the rebase-drop diagnostic |
GITHUB_STEP_SUMMARY unset (off-Actions) |
0 | /dev/null fallback holds under set -euo pipefail |
| both manifests present + agreeing | 0 | three rows incl. "agree" |
Enforcement is genuinely unchanged - only the summary writes were added, and the greater inversion is a faithful if/else rewrite of the original negation. Agreed that a job summary is not readable through any gh API, so these local unpiped runs are the durable evidence, not a CI artifact. The failure text names the exact rebase-drop mode that bit three branches today, which is the point of the ticket.
dig-constants check (both directions) - clear
- Does anything here BELONG in dig-constants? No. The four constants added are
TREE_PREFIX(a temp-dir name for one test binary),STALE_AFTER(a sweep threshold),WORKED_EXAMPLES(a doc-example count) andALLOWED(two fn names in one file). Each is harness-local to a single file; none is a shared/cross-repo value, asset id, address, puzzle hash, port, genesis value or version pin, and no second repo could need to match any of them. - Should anything here be USING dig-constants? No. The diff introduces zero new numeric or hex literals, so nothing duplicates a published constant.
#366 / PR#369 collision - no weakening, and the resolution is mechanical
This PR cannot weaken the widened two-crate sweep scope of #366: the scope-defining call sites (the rust_files(Path::new(env!("CARGO_MANIFEST_DIR")).join("src") ...) calls) are untouched context here - the three hunks are the doc block at pre-image 502-534 and the new test inserted after pre-image 729. The only overlap with #369 is the prose/doc block itself, which carries no scope.
For whoever resolves it: on the #369 rebase, take the executed-fixture shape from this PR for the doc block (as the #369 gate recommended), then confirm the two rust_files(...) roots still name both crates. That is the one assertion that would catch a mechanical "take mine" silently reverting the NC-12 gap closure.
Non-gating notes (recorded, not blocking, nothing to resolve)
drive_s_getpreviously wrotedig-node-origin-test-*trees and now writesdig-node-serve-test-*, so pre-existing residue under the old prefix will never be swept. One-time, bounded, dev machines only - not worth a change.- The touch-a-crate dep-freshness rule is not exercised here. Reasonable in this instance: the diff is test/doc/CI-only plus a dead-fn removal,
Cargo.lockmoves by exactly the two version lines, and dep freshness for this crate belongs to the active chia-0.36 cascade rather than to an MVP-epic child under end-to-end-first. Flagged for visibility, not asking for a change in this PR.
Verdict
PASS. Every claim in the PR body that I could test, I tested by mutation rather than by reading, and each held - including the two previously-wrong ones that were self-reported. Both self-corrections are genuine and the corrected numbers are the measured ones.
Merging, undrafting and the pointer bump are yours. Worth remembering that the dig-node midnight cron cuts a real stable release from main unattended, so this reaches users the night it lands; nothing I found argues against that.
DRAFT — DO NOT MERGE — gate round in progress.
Four dig-node tickets on one branch, one root version bump (0.154.0 -> 0.155.0), one gate round.
Serves MVP epics DIG-Network/dig_ecosystem#2790 and DIG-Network/dig_ecosystem#1502.
Closes #360
Closes #361
Closes #363
Closes #364
#367 was SPLIT OUT of this PR and is not here. It is complete and pushed on
loop/367-keystore-hardware(953eafb); see the note at the bottom.Blast radius
gitnexusMCP tools were not available in this lane, so the radius was measured with ripgrep overthe workspace plus direct reads, and is stated per symbol below (CLAUDE.md 2.0 bound 2 — falling
back is permitted, and saying so is required).
ChainTransport::with_sourcesgrep -rn with_sources --include=*.rs)ChainTransport::new/with_peer_readssage/service.rs:173), 4 testcontent_serve.rstest harnessensure-version-increment.ymlsweep/ends_a_column_0_itemsole_owner_testsonly,#[cfg(test)]No HIGH/CRITICAL risk. One breaking change, and it is deliberate: a
pub fnwith no callers isremoved, so
dig-walletgoes 0.35.0 -> 0.36.0.Sibling-lane coordination: PR #369 owns #365/#366 and the anchored-root resolver plus the sweep
SCOPE in
sources.rs. This PR touchessources.rsonly inside the#[cfg(test)] mod sole_owner_testsdoc comments and adds one test fn; it changes no classifier behaviour and nosweep scope.
#360 — delete the dead oracle-first
ChainTransportconstructorwith_sourceswaspub,#[must_use], had no callers anywhere in the workspace, and builtpeer_reads: None— a transport whosepeak_heighttakeschia-query's router path, which asksapi.coinset.orgFIRST. It read like the obvious way to build a transport over the node's ownfabric while being exactly the single-source shape NC-12 exists to prevent.
Deleted rather than repaired, per the ticket. The durable artifact is a fail-closed closed
enumeration: the constructors permitted to build
peer_reads: Nonearenewand the#[cfg(test)]with_client, and a third one fails the suite by name. The needle is assembledat run time from two fragments so the guard cannot match itself, and it matches the struct-literal
field form rather than the prose form, so the doc comments discussing this shape are not counted as
sites.
Load-bearing: reintroducing
with_sourcesgives... are not on the closed list ["new", "with_client"]: ["with_sources"].#361 — the ~57 MB-per-run temp-tree leak
content_serve.rsbuilt its temp path by hand and nothing removed it. Each node seeds a realcompiled
.digmodule and warms a cache, so a run cost ~57 MB per node; 1,123 trees reached62.5 GB and took the machine to 81 MB free on a 1.9 TB disk, producing a machine-wide ENOSPC that
stopped an unrelated lane.
A
NodeCacheRAII guard now owns the tree (tempfile::TempDir), so removal happens inDrop,including on an unwind. Ownership is the point rather than the deletion: a cleanup line at the end
of a test is skipped by every failing assertion — i.e. exactly the runs a developer repeats. Both
leaking sites in the file are covered (
dig-node-serve-test-*anddig-node-origin-test-*).The honest residue. The detached axum task still holds
wallet.sqliteopen when the test bodyreturns, and Windows will not unlink an open file, so
TempDir::dropcannot remove that last ~1 MB.Dropcannot cancel the task (it cannot.await), so the residue is bounded rather thanpretended away: a once-per-process sweep removes trees idle for 15 minutes, which no live run can be
(the suite takes ~2.5 min).
How the fix was proved (measured on this machine, full suite, twice each way):
Two tests hold it, and a leak test bounded by its own duration would prove nothing, so neither
is one — both assert on the PATH after the guard is gone:
the_temp_tree_is_removed_on_drop_and_on_panic— asserts the tree is absent after a normal dropAND after a real
catch_unwindunwind. Asserting thatDropwas reached would pass for aDropthat calledremove_dir_alland swallowed its error, which is the exact silent failure ofthe type under test.
the_sweep_removes_an_abandoned_tree_and_spares_a_live_one— three actors: an abandoned tree, alive tree, and a stranger's directory. The live one is not decoration; a sweep that removed
everything would satisfy "the stale one is gone" while deleting a concurrent lane's working
directory.
Load-bearing, each fix reverted alone (committed first, restored from a file copy — never
git checkout):TempDir::into_path()inDrop(consumes without deleting)if std::thread::panicking()— i.e. a deferred cleanup lineThe second revert is the important one: the first fires on the normal assertion, so it does not
prove the panic half. Modelling a deferred cleanup separately is what proves it.
#364 — the version gate reports the comparison it made
The gate held both numbers and reported only pass/fail. A stale bump is invisible to
git status,the diff, fmt, clippy and the tests; on PR #354 it went stale three times across a five-round gate,
the last when a rebase dropped the bump as "already upstream".
It now writes a table to
$GITHUB_STEP_SUMMARYon every run: the base ref actually compared(short SHA of
main, plus head), a row per manifest with base/head/verdict, the must-match rowwhere both exist, and the verdict spelled out. What the gate enforces is unchanged — same
predicate, same exit code, same
::error::annotations.${GITHUB_STEP_SUMMARY:-/dev/null}keeps the script runnable off Actions, which is how it wasverified: the run block was extracted from the YAML and executed unpiped against scratch git
repos — passing (RC=0), the stale-bump failure with
mainholding the same version (RC=1), and theboth-manifests-agree case (RC=0), each producing its summary.
Bounded to dig-node deliberately (CLAUDE.md 1.3c rule zero). Measured: 25 other repos carry a
byte-identical copy and 6 carry a diverged one (dig-app, dig-browser, dig-updater,
chip35_dl_coin, dig-framework-adapters, hub.dig.net). Propagation is a named follow-up with that
finite list, not "every repo".
#363 — the sweep's worked examples are executed, not asserted
A doc example in
sources.rswas exactly backwards and survived four adversarial gate rounds:everything else in the file was measured, only the examples were prose, and that is where the false
statement lived.
The worked examples are now
jsonblocks carrying their fixture and their expectedsites/ended_inside_a_test_item, and a test extracts every one from this file's own source and runs thereal classifier over it.
Why not a rustdoc doctest.
rustdocdoes not run doctests on#[cfg(test)]items and this wholemodule is one, so a fenced
rustexample would compile in nobody's build while reading exactly likea passing test — the cfg-gated-and-unfalsifiable trap. It would have left the file in the state the
ticket was filed about while looking fixed.
Fail-closed twice: an unterminated block is an error, and the example count is pinned, so
deleting an inconvenient example — the cheapest route to green — fails rather than passing quietly.
No classifier heuristic is added.
sweepandends_a_column_0_itemare byte-for-byte unchanged;only the file's statements about them are now enforced.
Second defect found while doing it. The trailing-comment example asserted
sites=[5]andsites=[]for "the same fixture" without ever writing that fixture down, so its numbers wereunreproducible by construction. The reconstructed fixture measures
[6]and[], and both fixturesare now explicit. A claim whose input is missing cannot be checked by anyone — worse than a claim
that is merely wrong.
Load-bearing: all five mutations turn the suite red, each naming the offending example — altering
any of the four
sitesvalues, and flipping a documentedended_inside_a_test_item.#367 split out —
loop/367-keystore-hardware(953eafb), complete and pushedThe work is done, green, and measured, and is held back only because
dig-keystore0.13.0 isnot published yet (the index shows 0.12.0 as latest; PR#23 adds the macOS and Linux providers plus
the atomic mint and is still in gate). Landing 0.12 now and 0.13 in days would be two dependency
cascades across
dig-node-coreanddig-walletwhere one will do.What is on that branch:
platform_provider()— a hardcodedNoneunder a doc comment assertingthat dig-keystore ships no platform binding, untrue since
ed9601a3— is deleted, and productionuses a new
MachineKeyStore::open_platform_boundbuilt ondig_keystore_hardware::bind_strongest,so the ladder owns selection and re-runs the wrap/unwrap self-test. Measured on this Windows TPM
host:
Hardware(WindowsTpm20). The regression test asserts the production constructor can neverreport
DegradeReason::NotRequested— the fingerprint of the oldNone— which is host-independentwhile still being exactly the regression, with
open(dir, None)kept as a control so the needlecannot go vacuous.
Two things the split lane must change before it ships, both from the coordinator and both
recorded on #367: target 0.13.0, and request
HardwarePolicy::Preferredwith the refusalHANDLED rather than
Optional— 0.13 lets macOS/Linux produceIndeterminate, whichPreferredtreats as an error, so an unhandled refusal would stop those hosts serving the peer network.
Gates run (unpiped, exit codes read directly)
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspace --no-run(re-run after the rebase)cargo test --workspace --no-fail-fastVersion re-checked against freshly-fetched
origin/main(4efa57f, 0.154.0) after the rebase:root 0.155.0, and
dig-wallet0.35.0 -> 0.36.0 for the removedpubconstructor.