fix(test): own test scratch dirs with tempfile::TempDir so they clean on drop - #476
Conversation
Main released 0.208.0 while this lane worked, so its bump became a no-op against the new main -- the version gate reads the file, not the log.
|
Gate — IN PROGRESS, not the verdict. Head read: Item 1 (the count) — CONFIRMED, independently. Not sampled; full enumeration, same regex on both trees:
65 − 12 = 53 converted. The PR body's table matches the code. The body enumerates all 12 Continuing with: the 12 justifications one by one, the |
|
Gate — IN PROGRESS, not the verdict. Head Item 2 (the 12 left) — 11 of 12 justifications CONFIRMED by reading the sites. One is not. Confirmed correct to leave:
GATING — the one exception: The site is a genuine leak of exactly this class (
So there is no single-writer boundary to respect, and the file is free. The consequence is what makes this gating rather than a nit: the PR says Either fix is fine and both are small; do not fix it by deleting the admission:
Still to come: the |
|
Gate — IN PROGRESS, not the verdict. Head Item 3 (the The fix at Method — two passes, because the shape has two halves:
Worth knowing about the class in general: it fails at runtime with One non-gating note, same shape but currently benign: |
|
Gate — IN PROGRESS, not the verdict. Head Item 5 (the residual) — the PR body states it HONESTLY. Confirmed by reading, not by trusting. The body has a section headed "Residual — this reduces the class, it does not close it", and it The single caveat is the one already filed above: the body's honesty about Two blockers that are NOT about the code, flagged rather than fixed, per my brief:
Remaining: my own re-run of the |
MichaelTaylor3d
left a comment
There was a problem hiding this comment.
CHANGES-REQUIRED — head read: f0224b7e44cd22f77f2cf35b499f6ba5849e84ef.
One gating correctness finding, posted inline below, plus two mechanical merge blockers already
detailed in my interim comments. The conversion work itself is careful and verifies: the 65/53/12
count is exact, 11 of the 12 justifications hold on reading, the seeded_log fix is right and I
found no manifesting sibling, the revert-proof discriminates (I re-ran it), and the residual is
stated honestly in the body.
VERDICT: CHANGES-REQUIREDHead read: Ranked findings1. GATING — 2. BLOCKS MERGE, mechanical — the version does not increment. 3. BLOCKS MERGE, mechanical — 4. NON-GATING — 5. NON-GATING — the What I verified, and how
What I could not reach
Findings 2 and 3 clear on a rebase + re-bump. Finding 1 is the one that needs a decision. |
DRAFT — DO NOT MERGE. Orchestrator gates.
Closes #397
Closes #370
What this does
Converts the hand-rolled
std::env::temp_dir().join(..)test scratch directories to ownedtempfile::TempDirguards, so removal happens inDrop— including on an unwind, which is thecase that produced the leak. Every one of these sites ended with a manual
remove_dir_all, whichis exactly the line a failing assertion skips: the runs a developer repeats were the runs that
leaked.
Counts — measured from the code, not from directory listings
std::env::temp_dir()call sites onmainTempDirguardOne further guard-lifetime defect was found by running the suite and is fixed here:
spend_audit_cli.rs::seeded_logbound the guard locally while returning the log that pointedinto it, which turned 10 tests red. See #370 for the shape.
maincarried 65 sites and one pre-existingtempfile::Builder(the #361 fix incontent_serve.rs); this branch carries 11 and 55.The tickets' own figures came from directory counts on one machine and did not match the code:
#370 estimated "roughly 50" sites across 8 named prefixes, and three of the prefixes reported as
leaking here —
dig-webview-shared*,digstore-remote-test-decoy/*,dig-node-warmloc-engine*in its literal form — do not exist in this repository at all. The first two belong to other
repos' harnesses; the third is the
-enginetag ofdig-node-warmloc-{tag}, which is converted.The 11 left, individually
Genuinely leaks, NOT fixed here — none.
src/spend_audit.rs:1282was previously listed here, deferred on the ground that the file wasowned by a live lane. That premise was false at the head it was written against — #475, the
last PR to touch the file, merged
2026-09-01T00:35:35Z, 5h34m earlier, and none of the seven openPRs touches it. Since
Closes #370would have retired the only ticket naming the site, it is nowconverted:
tmp_log()returns(SpendLog, tempfile::TempDir)and all 14 call sites bind theguard for the life of the test. Run, not merely compiled:
cargo test -p dig-node-service --lib spend_audit-> 47 passed; 0 failed. Binding the guard locally inside the helper wouldreproduce exactly the
seeded_logdefect fixed above, which type-checks at every depth.Also converted while the pattern was fresh, though it was benign:
dig-node-core/src/capsule_warm_locator_tests.rs:123—node_with_pool_peernow returns(Arc<NodeContent>, tempfile::TempDir)(3 callers).cargo test -p dig-node-core --lib capsule_warm_locator-> 3 passed; 0 failed.Production code, not a test harness — two sites:
dig-node-core/src/lib.rs:537—private_fallback_dir(), the degraded un-shared mode.dig-node-service/src/server.rs:496— the fail-closed ephemeral control-token dir. One pernode process on a real host, by design.
Never creates a directory — three sites:
src/seed_export_cli.rs:139— names a file that must be absent (json_is_refused_before_the_file_is_read).src/spend_audit.rs:1575— an absent ledger path (a_node_that_never_spent_reads_as_an_empty_ledger).src/state.rs:1139— a pureis_machine_state_dirpath predicate.Not code / already fixed — two sites:
tests/content_serve.rs:40— a doc comment describing the fix(test): serve integration test leaks a ~57MB temp dir per run — 1,123 dirs / 62.5 GB took the machine to ENOSPC #361 fix.tests/content_serve.rs:87— fix(test): serve integration test leaks a ~57MB temp dir per run — 1,123 dirs / 62.5 GB took the machine to ENOSPC #361's once-per-process sweep, which reads the temp root on purpose.Deliberate, and a guard would break the test — four sites:
dig-runtime/src/lib.rs:635, 930, 959, 990. These start the process-global runtime, whichkeeps reading the cache for the rest of the process; a guard dropped at the end of the test
would pull the tree out from under it. They use fixed names, so the residue is bounded at
four directories reused every run rather than one per run. The reason is now stated at each site.
Where the guard could not simply be a local binding
Three shapes needed more than a type change, and each is documented at the site:
peer.rs::fresh_pool_handle{,_on},tests/pool_connect.rs::start_pool,openrpc_drift_guard.rs::ephemeral_node,spend_audit_cli.rs::tmp_logandcollateral_census.rs::seeded_storereturn(T, TempDir)— the started pool or store reads its files for its whole lifetime, so the treemust outlive the builder.
EnvHoldintests/server.rs— the value every caller already keepsalive for exactly the span the spawned server runs for.
state.rs(ensure_dir_restricted/harden_state_dir) andupdater.rs::unique_path. Their property under test is what happens when the directory doesnot yet exist; a guard over the directory itself would have created it and quietly retired
the test.
Test
the_scratch_tree_is_removed_on_drop_and_on_an_unwindinprofile_sync.rs. It captures one pathit owns and asserts on that path, rather than counting entries under the system temp root — a
disk-state assertion would be measuring every other test running concurrently on this box, and a
flaky one is worse than none.
The panic half is the load-bearing half: a helper that called
TempDir::keep, or handed backan unowned
PathBuf, satisfies nothing there, while a merely tidier manual cleanup passes thesuccess case and fails it.
Residual — this reduces the class, it does not close it
Dropdoes not run on a panicking abort or on a killed process. Several of the directoriesmeasured on this machine came from lanes killed mid-run, and no
Drop-based fix can catch those.Separately, Windows will not unlink a file another handle still holds, so a tree whose
wallet.sqlitea detached serve task still has open can survive its guard;content_serve.rs'sonce-per-process sweep over trees idle longer than any live run is the existing answer to that
half, and it is unchanged here.
So the harness no longer leaks one directory per run on the ordinary and the failing path, which
is what the tickets asked for. It does not make the temp root self-cleaning.
Blast radius
Test-only, apart from the two production sites named above, which are not touched. The changed
symbols are test helpers plus their call sites:
fresh_pool_handle{,_on}(10 callers, incl.pex.rs),start_pool(3),transport_with_one_peer(3),tempdirinprofile_sync.rs(27),unique_path(4 + 5),tmp_log(3 + 2),seeded_store(10 + its integration twin),tmp_config(4),
scratch/dir/state_dirin the wallet crate, andEnvHold's shape intests/server.rs.No public API changes; no production behaviour changes.
Version
0.212.0, with
Cargo.lockrefreshed in the same commit.Measured evidence — two consecutive
cargo test -p dig-wallet --librunsCounting only the prefixes this crate owns (
dig-tip*,dig-wallet-sup*,dig-wallet-svc*,dig-watchlist*,dig-seed-export*) under%LOCALAPPDATA%\Temp:The delta is a stable bound, not growth. The same three survive each run, and they are all the
same thing:
Every survivor contains an open
wallet.sqlite. This is the residue stated in the PR and in #370'sown reference: Windows will not unlink a file another handle still holds, so
remove_dir_allfails and
TempDir::dropswallows the error. It is bounded by the number of fixtures that leave adatabase open, not by the number of tests.
For scale: those helpers are called ~40 times across the run. Roughly 40 scratch directories
created, 3 survive — and all 3 from the one helper (
service.rs::scratch) whose fixture opens awallet DB. Before this change every one of the ~40 persisted, because nothing removed them.
The one failure in run 1 is NOT this change
sync_supervisor::tests::stall_evidence_survives_the_end_of_a_sessionhit its own 120-secondwait: "timed out after 120s waiting for: the stall to be named despite the sessions turning over".
It passed alone (
1 passed, run in isolation), passed in run 2, and passed in the full workspacerun. The box was compiling concurrently at the time. Recorded rather than dropped, because a
timing-sensitive 120s wait on a loaded machine is a real flake risk in CI and is worth its own
ticket if it recurs — it is not a directory-lifetime failure.
Full workspace
cargo test --workspace -j 2on the same tree: 2,807 passed, 0 failed, zero compile errors andzero code warnings.
The unwind half of the regression test is now independently mutation-proved
Previously only the success half was:
disable_cleanup(true)trips the FIRST assertion, so theunwind assertion never speaks. One mutation cannot prove both, because
TempDircleanup is asingle
Dropon both paths. So a second, separate mutation was run — both blocks ofthe_scratch_tree_is_removed_on_drop_and_on_an_unwindrewritten to the pre-fix idiom (a rawcreate_dir_allplus a manualremove_dir_allat the end of the block, which is the nearest wrongimplementation: a merely tidier manual cleanup):
The success half REACHED its manual removal line and passed; the run failed at the unwind
assertion alone. Both mutations are recorded in the test's doc comment.
Version
0.216.0.origin/mainmerged in at0.213.0; 0.209-0.215 are claimed by sibling PRs.Cargo.lockrefreshed in the same commit (cargo update -w --offline).