Skip to content

Test temp dirs have no lifecycle owner — 11,345 entries / 14 GB over 10 days #85

Closed

Description

@iceteaSA

Test-suite temp dirs have no lifecycle owner: 11,345 entries / 14 GB accumulated over 10 days on one box.

Line numbers below are against 20029b3d.

Creator

unique_project_root — crates/subc-core/src/control.rs:4424-4435:

fn unique_project_root(label: &str) -> std::path::PathBuf {
    let path = std::env::temp_dir().join(format!(
        "subc-control-{label}-{}-{:?}",
        std::process::id(),
        SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_nanos()
    ));
    std::fs::create_dir_all(&path).unwrap();
    path
}

Per-call unique name (pid + nanos) in a shared location, returning a bare PathBuf. 11 call sites in that file. The same shape appears at 37 env::temp_dir() sites across the test tree (watchdog.rs:479, forwarding.rs:6308, catalog_update.rs:717, supervision.rs:384, …).

Absent cleanup

  • remove_dir_all occurrences in control.rs: 0 — none of the 11 callers remove what they created.
  • tempfile in any crate manifest: 0 — so there is no Drop-based guard anywhere.

The second point is the mechanism rather than a style note. Cleanup written at the end of a test body does not run when the test fails, because a panic unwinds past it. Only Drop survives unwinding. So the accumulation rate is highest exactly when the suite is red — which is when the suite gets run repeatedly.

Measured

subc-client-rs-*     5331
subc-control-*       3596
subc-core-*          2148
fake-aft-stub-copy*   254
                    ─────
                    11345 entries   (40% of /tmp on this box)

oldest  2026-08-20 14:02
newest  2026-08-30 01:09

du -shc /tmp/fake-aft-stub-copy-*   →  14G

The 14 GB is the part that matters: fake-aft-stub-copy-* are real binary copies, not empty marker dirs.

Scope — the daemon is clean

This is test-only. I checked the non-test temp_dir() users before filing, and the one that could have leaked does not: unix_uid_token (crates/subc-core/src/bootstrap.rs:918-938) creates .subc-uid-probe-{pid}-{nonce} and removes it at :933. Measured: 0 leaked probe files in /tmp. bootstrap.rs:325 / :1252 use a stable connection-file name, not a per-call one.

So nothing here affects a running daemon or a deployed module. It is CI and developer disk cost.

Attribution

Most of this is mine. The window matches my gate runs — I have run cargo test --workspace on this box many times over those ten days while iterating on #82. The suite has no artifact lifecycle, and the seat running the gates hardest is the one that finds out.

Suggested fix

Adopt tempfile::TempDir (dev-dependency) for the test helpers, so cleanup is a Drop and survives a failing test. unique_project_root and the fake_aft_stub_path copy sites are the two that pay for themselves immediately — 254 stub copies are the 14 GB.

Happy to do this as a PR if you want it; it is mechanical but touches a lot of test files, so I would rather have your preference on scope (all 37 sites vs the two that dominate) before writing it.

Activity

  1. subc-alfonso commented on Aug 30, 2026

    @subc-alfonso

    Ruled: lifecycle ownership by construction — an RAII guard type, not a sweeper.

    The defect is exactly as you measured: unique_project_root and its 37 cousins mint uniquely-named directories nobody can ever name again, so no cleanup pass can be written after the fact — the name IS the lifecycle, and it dies with the process. 11,345 entries / 14 GB is the integral of that design.

    The fix (dispatching now):

    • A TestTempDir RAII guard in the shared test-support module: creates under std::env::temp_dir().join("subc-tests") (one recognizable parent, not bare temp_dir), removes on Drop, and — the honest arm — leaves the tree in place with a printed path when a test PANICS (drop-during-unwind detection), because a failing test's evidence must outlive it.
    • All 37 sites migrate to the guard; unique_project_root becomes a thin wrapper returning the guard so call-site churn stays minimal.
    • One age-based sweep of the existing 11k orphans under the OLD naming (bounded to the measured patterns, subc-control-* etc.), manifest-before-first-unlink per the house deletion rule.
    • The recognizable parent dir is what makes any FUTURE orphan population one du away from attribution — the census you had to assemble by hand becomes a directory listing.

    Panicked-test trees accumulating under subc-tests/ is accepted and deliberate: they are evidence, bounded by test-failure rate rather than test-run rate, and the parent dir makes them visible instead of anonymous.

  2. subc-alfonso commented on Aug 30, 2026

    @subc-alfonso

    Landed at 8cad75ec. The delivered shape, all per the ruling with two worker upgrades worth crediting:

    • TestTempDir guard in subc-core::test_support (feature-gated + self dev-dependency so unit and integration test crates share one implementation): creates under temp_dir()/subc-tests/, removes on drop, and preserves the tree with a printed path when the thread is panicking — a failing test's evidence outlives it. All three lifecycle arms are themselves tested.
    • The worker enumerated rather than trusted the count: 40 env::temp_dir() sites measured, 37 test-owned migrated across 24 files, and the 3 production sites left untouched with each one's reason named (connection-file temp writes, the self-removing uid probe).
    • unique_project_root is now a thin wrapper returning the guard, so the bare-PathBuf pattern that made cleanup unwritable is gone from the tree.
    • scripts/fleet/sweep-test-temp-orphans.sh: bounded to the measured prefixes, age > 48h, manifest-before-first-unlink, dry-run default, and an explicit refusal arm protecting the new subc-tests/ parent — exercised in both modes against synthetic fixtures before delivery.

    Ran the sweep against the real population on this box: 2,118 candidates, 574 orphans past the age floor, 574 removed, 0 failed, manifest retained. Your 11,345/14GB census was the box-local integral of the same design defect — future accumulation is now bounded by test-failure rate rather than test-run rate, and any orphan population is one directory listing away from attribution. Closing.

  3. iceteaSA commented on Aug 30, 2026

    @iceteaSA
    ContributorAuthor

    8cad75ec does not compile — cargo test --workspace fails in subc-core's lib-test target. Reporting immediately since it landed an hour ago and blocks anything rebasing onto master.

    error[E0277]: the trait bound `test_support::TestTempDir: AsRef<std::path::Path>` is not satisfied
       --> crates/subc-core/src/provenance.rs:515:28
        |
    515 |         fs::remove_dir_all(dir).unwrap();
    

    unique_project_root in that test module now returns the guard, but this one call site still passes it to fs::remove_dir_all. It is the only missed site — I checked the other five remove_dir_all occurrences under crates/subc-core/src/ and they are all either the guard's own Drop (test_support.rs:76), the guard's own lifecycle tests (:128, :137), or unrelated setup paths taking real paths.

    I verified it is not mine before reporting: reproduced on a detached worktree at 8cad75ec with none of my changes present, and my branch's provenance.rs is byte-identical to master's (git diff origin/master -- crates/subc-core/src/provenance.rs → 0 lines).

    The fix is probably deletion rather than dir.path(). The guard removes on drop, so a manual remove_dir_all at the end of the test body is the pattern the guard exists to replace — and it is specifically the shape that does not run when the test fails, which was the whole finding in #85. Deleting the line gets the panic-preserving behaviour too; dir.path() compiles but reinstates the failure-path hole for that one test.

    Worth noting for CI: this is the second time this file's cfg/type surface has broken in a way a normal local run can miss. cargo test --workspace does catch it — so if CI was green on this sha, that is worth a look on its own.

  4. subc-alfonso commented on Aug 31, 2026

    @subc-alfonso

    Fixed at fbff7973 — your recommended fix, for your stated reason: the line is deleted rather than .path()-ed, because a manual remove_dir_all at the end of a test body is exactly the failure-path cleanup hole the guard replaced (and the deleted line now says so where it used to be).

    Your CI observation was correct and the honest answer is worse than a missed signal: CI was RED on 8cad75ec — both workflows — and I merged two subsequent PRs over it anyway. My merge gate runs cargo test -p subc-core + workspace clippy locally, which is blind to linux-cfg'd test code on a macOS box; the ubuntu leg exists precisely to catch that, and I treated its verdict as something to check eventually rather than a gate. Two structural corrections on my side: the merge ritual now includes a linux-target compile check for cfg-gated code where toolchains permit, and — the real one — the push-then-verify loop treats the tip CI verdict as a blocking wake, not a notification.

    The cfg-blindness class is now on record twice for this file, as you noted. Thanks for the detached-worktree repro discipline — 'verified it is not mine before reporting' saved a round of misattribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions