tests: own the temp vault directories, and bound them at the source

[iceteaSA]

A fleet census found 17,678 leaked directories in /tmp (1.6 GB, >13k inodes) minted by this repo's tests over roughly two weeks, growing faster than the OS reaper removes them. They are test vaults — a store.db plus a .lease — created with std::env::temp_dir() and never removed.

I reaped the existing ones separately. This closes the source.

The change

A TestTempDir guard in credentials-core::test_support: owns the directory, removes it on Drop, and derefs to Path so root.join("store.db") compiles unchanged at the call sites. create_dir rather than create_dir_all is deliberate — these names embed a pid, pids are reused, and inheriting a stale vault silently is worse than failing loudly on the collision.

Converted the four prefixes carrying ~99% of the leak by count:

ck-cred-health     13253    main.rs
ck-admin-surface    3053    admin_surface.rs
ck-cred-store       1099    store.rs
ck-cred-cli          463    tests/common/mod.rs

Five sites are deliberately unconverted and named in the diff — three CLI discovery tests, an exclusivity test, and the vault_read_probe example. None of them appear in the census.

The acceptance test

/tmp directories matching the four prefixes
  before full gate run   856
  after  full gate run   856

Unchanged across a complete scripts/gate.sh, which is the property that matters: the bound is structural now rather than depending on the reaper outpacing us.

What I got wrong first, since it shaped the diff

My brief to the implementer asserted that the crash-cut suites (kill9_mid_refresh, rotate_crash_cut, login_crash_cut) need an explicit keep(), because they SIGKILL a helper and then inspect the directory and a Drop would delete the evidence. That was wrong. The guard binds to a variable that lives to the end of the test function, and every inspection happens inside that scope — Drop never fires early.

Worse, the keep() I mandated returns a plain PathBuf, so the guard became a temporary dropped at end-of-statement and disarmed. The directory then survived only via an explicit remove_dir_all at the test's tail, which does not run on panic. The correction left the crash-cut tests — the ones most likely to fail — as the only sites still leaking on the failure path.

Verified the fix directly rather than reasoning about it. Injecting a panic immediately after construction:

2 tests panicked
/tmp/ck-cred-login-cut-*   before 0   after 0

So the guard removes on the panic path. keep() stays on the type with no current caller, documented as existing for a path that must outlive its guard.

One coverage fact worth knowing before anyone tidies a test away. I broke the disarm (clone instead of take, so Drop still fires) and all three crash-cut suites stayed green — only the focused keep_disarms_removal unit test caught it. That test is the sole coverage of the disarm; there is a comment saying so at the test.

Gate green, e2e 9/9, all five crash-cut arms green, release-artifact scan clean, workspace floor measured at 580. Lock byte-identical to master.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Test-created vault directories in /tmp were leaking (17,678 over two weeks). A new TestTempDir guard in credentials-core::test_support now owns them and removes them on Drop, including on panic paths.

• The guard derefs to Path, so root.join("store.db") call sites compile unchanged.
• create_dir fails loudly on a PID-reuse collision with a stale vault instead of silently inheriting it.
• Converted the four prefixes carrying ~99% of the leak: ck-cred-health, ck-admin-surface, ck-cred-store, ck-cred-cli.
• Crash-cut suites hold guards through evidence inspection, so the directories are removed even when tests panic.
• keep() stays on the type for evidence that must outlive a guard but has no callers.
• Five sites are deliberately unconverted: three CLI discovery tests, an exclusivity test, and the vault_read_probe example.

^{Written for commit ab0dd9a. Summary will update on new commits.}

[cubic-dev-ai]

9 issues found across 21 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/credentials-module/tests/cli_opencode.rs">

<violation number="1" location="crates/credentials-module/tests/cli_opencode.rs:643">
P3: Now that `root` is a `TestTempDir` whose `Drop` removes the directory, the manual `impl Drop for MigrationRig` that calls `remove_dir_all(&self.root)` is redundant and runs a second, always-failing removal after the guard already deleted the tree. Remove the explicit `Drop` impl and let the guard own cleanup.</violation>
</file>

<file name="crates/credentials-module/src/admin_surface.rs">

<violation number="1" location="crates/credentials-module/src/admin_surface.rs:370">
P2: The `_root` guard drops before `store` closes its SQLite connection (fields drop in declaration order). remove_dir_all then fails silently on platforms that cannot delete an open file (e.g. Windows), leaking the directory this PR is meant to clean up. Declare `_root` as the last field so the store drops and closes the connection before the directory is removed.</violation>
</file>

<file name="crates/credentials-core/src/test_support.rs">

<violation number="1" location="crates/credentials-core/src/test_support.rs:69">
P2: These unit tests use fixed temp names, so an interrupted `keep_disarms_removal` or concurrent test run makes `create_dir` return `AlreadyExists` and panics before the test. Include a process or sequence suffix in both names so the tests can be rerun safely.</violation>
</file>

<file name="crates/credentials-module/tests/cli_admin.rs">

<violation number="1" location="crates/credentials-module/tests/cli_admin.rs:338">
P3: Now that `root` is a `TestTempDir`, its own `Drop` already removes the directory, so the hand-written `impl Drop for GrantCliVault` deletes the directory a second time. Remove the manual `Drop` block so the guard's ownership is the single cleanup path; keeping both implies the guard is not self-cleaning and misleads future readers.</violation>
</file>

<file name="crates/credentials-core/Cargo.toml">

<violation number="1" location="crates/credentials-core/Cargo.toml:48">
P2: `migration-tools` is a production one-shot feature (ck_key_move/ck_key_verify write master-key material), not an integration test, yet it now pulls in `test-support`, contradicting the stated goal that test fixtures stay out of normal builds. Neither migration bin uses `test_support`/`TestTempDir` and neither has unit tests, so the dependency is unnecessary and widens the test-code surface in a master-key-handling tool. Drop the `test-support` dependency and keep the feature empty.</violation>
</file>

<file name="crates/credentials-module/tests/common/mod.rs">

<violation number="1" location="crates/credentials-module/tests/common/mod.rs:87">
P3: The `unique_temp_dir` doc comment is now wrong: it says callers create the directory themselves and that this path cannot refuse a collision the way `tmp_root` does. Since this line now returns `TestTempDir::new(...)`, which calls `create_dir` and panics on `AlreadyExists`, the function both creates the directory and refuses collisions. Update the comment to state that the guard now owns and creates the directory and refuses collisions.</violation>
</file>

<file name="scripts/gate.sh">

<violation number="1" location="scripts/gate.sh:248">
P3: The gate now passes `--features credentials-core/test-support` on the workspace test and seam-clippy arms, but CI's matching arms do not, and this file's own header makes gate/CI flag parity a hard invariant. The flag is also redundant here: test-support is already enabled transitively (credentials-module's dev-dependency sets it, and every seam feature declares it), so CI compiles the same code without the flag. Either drop the redundant flag to restore parity, or add it to CI deliberately.</violation>
</file>

<file name="crates/credentials-module/src/main.rs">

<violation number="1" location="crates/credentials-module/src/main.rs:3299">
P2: On platforms that reject removing an open SQLite file, these guards run before their stores close, and `TestTempDir::drop` ignores the failed removal. Declare each guard after the store-owning fields, or explicitly drop the stores before the guard, so cleanup runs after all SQLite handles are closed.</violation>
</file>

<file name="crates/credentials-core/src/store.rs">

<violation number="1" location="crates/credentials-core/src/store.rs:3925">
P2: These two tests migrated to TestTempDir but kept the manual `let _ = std::fs::remove_dir_all(root);` at the end, which now undercuts the guard's purpose. Because `remove_dir_all(root)` moves (consumes) the TestTempDir, the guard's Drop never runs after the store drops, and at that point the store's rusqlite connection is still open, so on Windows the removal fails (open file handle) and the error is silently swallowed — the directory leaks, defeating this PR's goal. Rely on the guard instead: drop the manual `remove_dir_all` and let the store drop first (reverse declaration order closes the connection) before the guard removes the directory, which works on all platforms.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: The _root guard drops before store closes its SQLite connection (fields drop in declaration order). remove_dir_all then fails silently on platforms that cannot delete an open file (e.g. Windows), leaking the directory this PR is meant to clean up. Declare _root as the last field so the store drops and closes the connection before the directory is removed.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/admin_surface.rs, line 370:

<comment>The `_root` guard drops before `store` closes its SQLite connection (fields drop in declaration order). remove_dir_all then fails silently on platforms that cannot delete an open file (e.g. Windows), leaking the directory this PR is meant to clean up. Declare `_root` as the last field so the store drops and closes the connection before the directory is removed.</comment>

<file context>
@@ -361,11 +361,13 @@ mod tests {
     /// A test rig: the AdminSurface plus everything a caller-side signer needs
     /// (the same MAC key derivation the CLI would perform from the keychain key).
     struct Rig {
+        _root: TestTempDir,
         admin: AdminSurface,
         store: Arc<EncryptedStore>,
</file context>

[cubic-dev-ai]

P2: These unit tests use fixed temp names, so an interrupted keep_disarms_removal or concurrent test run makes create_dir return AlreadyExists and panics before the test. Include a process or sequence suffix in both names so the tests can be rerun safely.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/test_support.rs, line 69:

<comment>These unit tests use fixed temp names, so an interrupted `keep_disarms_removal` or concurrent test run makes `create_dir` return `AlreadyExists` and panics before the test. Include a process or sequence suffix in both names so the tests can be rerun safely.</comment>

<file context>
@@ -0,0 +1,82 @@
+        // The crash-cut suites still pass if keep() is broken because they inspect before
+        // their guards drop. This focused test is the only coverage that detects a clone
+        // here instead of take(), which would leave Drop armed and erase kept evidence.
+        let path = TestTempDir::new("ck-cred-test-support-keep").keep();
+        assert!(path.exists(), "keep must leave crash-cut evidence behind");
+        std::fs::remove_dir_all(path).expect("remove kept test directory");
</file context>

[cubic-dev-ai]

P2: migration-tools is a production one-shot feature (ck_key_move/ck_key_verify write master-key material), not an integration test, yet it now pulls in test-support, contradicting the stated goal that test fixtures stay out of normal builds. Neither migration bin uses test_support/TestTempDir and neither has unit tests, so the dependency is unnecessary and widens the test-code surface in a master-key-handling tool. Drop the test-support dependency and keep the feature empty.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/Cargo.toml, line 48:

<comment>`migration-tools` is a production one-shot feature (ck_key_move/ck_key_verify write master-key material), not an integration test, yet it now pulls in `test-support`, contradicting the stated goal that test fixtures stay out of normal builds. Neither migration bin uses `test_support`/`TestTempDir` and neither has unit tests, so the dependency is unnecessary and widens the test-code surface in a master-key-handling tool. Drop the `test-support` dependency and keep the feature empty.</comment>

<file context>
@@ -42,7 +45,7 @@ rotate-test-seam = []
 # putting it on PATH is how a wrong-argument run becomes reachable years after anyone
 # remembers what it does.
-migration-tools = []
+migration-tools = ["test-support"]
 
 # The two rename-ceremony one-shots. Gated together because they are used at the same
</file context>

Suggested change

	migration-tools = ["test-support"]
	migration-tools = []

[cubic-dev-ai]

P2: On platforms that reject removing an open SQLite file, these guards run before their stores close, and TestTempDir::drop ignores the failed removal. Declare each guard after the store-owning fields, or explicitly drop the stores before the guard, so cleanup runs after all SQLite handles are closed.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/main.rs, line 3299:

<comment>On platforms that reject removing an open SQLite file, these guards run before their stores close, and `TestTempDir::drop` ignores the failed removal. Declare each guard after the store-owning fields, or explicitly drop the stores before the guard, so cleanup runs after all SQLite handles are closed.</comment>

<file context>
@@ -3284,7 +3296,7 @@ mod tests {
     #[tokio::test]
     async fn get_through_a_resolving_handle_returns_its_bound_credential_id() {
-        let (surface, store, _db) = tmp_surface_with_store(93);
+        let (surface, store, _db, _root) = tmp_surface_with_store(93);
         let credential_id = "apikey:get-binding-proof";
         store
</file context>

[cubic-dev-ai]

P2: These two tests migrated to TestTempDir but kept the manual let _ = std::fs::remove_dir_all(root); at the end, which now undercuts the guard's purpose. Because remove_dir_all(root) moves (consumes) the TestTempDir, the guard's Drop never runs after the store drops, and at that point the store's rusqlite connection is still open, so on Windows the removal fails (open file handle) and the error is silently swallowed — the directory leaks, defeating this PR's goal. Rely on the guard instead: drop the manual remove_dir_all and let the store drop first (reverse declaration order closes the connection) before the guard removes the directory, which works on all platforms.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/store.rs, line 3925:

<comment>These two tests migrated to TestTempDir but kept the manual `let _ = std::fs::remove_dir_all(root);` at the end, which now undercuts the guard's purpose. Because `remove_dir_all(root)` moves (consumes) the TestTempDir, the guard's Drop never runs after the store drops, and at that point the store's rusqlite connection is still open, so on Windows the removal fails (open file handle) and the error is silently swallowed — the directory leaks, defeating this PR's goal. Rely on the guard instead: drop the manual `remove_dir_all` and let the store drop first (reverse declaration order closes the connection) before the guard removes the directory, which works on all platforms.</comment>

<file context>
@@ -3918,17 +3919,17 @@ mod tests {
         use std::sync::atomic::{AtomicU64, Ordering};
         static SEQ: AtomicU64 = AtomicU64::new(0);
-        let root = std::env::temp_dir().join(format!(
+        let root = TestTempDir::new(format!(
             "ck-restore-{}-{}-{}",
             label,
</file context>

[cubic-dev-ai]

P3: Now that root is a TestTempDir whose Drop removes the directory, the manual impl Drop for MigrationRig that calls remove_dir_all(&self.root) is redundant and runs a second, always-failing removal after the guard already deleted the tree. Remove the explicit Drop impl and let the guard own cleanup.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/tests/cli_opencode.rs, line 643:

<comment>Now that `root` is a `TestTempDir` whose `Drop` removes the directory, the manual `impl Drop for MigrationRig` that calls `remove_dir_all(&self.root)` is redundant and runs a second, always-failing removal after the guard already deleted the tree. Remove the explicit `Drop` impl and let the guard own cleanup.</comment>

<file context>
@@ -640,7 +640,7 @@ fn spawn_route_daemon(
 
 struct MigrationRig {
-    root: PathBuf,
+    root: credentials_core::test_support::TestTempDir,
     vault: PathBuf,
     key: PathBuf,
</file context>

[cubic-dev-ai]

P3: Now that root is a TestTempDir, its own Drop already removes the directory, so the hand-written impl Drop for GrantCliVault deletes the directory a second time. Remove the manual Drop block so the guard's ownership is the single cleanup path; keeping both implies the guard is not self-cleaning and misleads future readers.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/tests/cli_admin.rs, line 338:

<comment>Now that `root` is a `TestTempDir`, its own `Drop` already removes the directory, so the hand-written `impl Drop for GrantCliVault` deletes the directory a second time. Remove the manual `Drop` block so the guard's ownership is the single cleanup path; keeping both implies the guard is not self-cleaning and misleads future readers.</comment>

<file context>
@@ -335,7 +335,7 @@ fn a_global_flag_before_the_verb_reaches_the_same_vault_as_one_after_it() {
 
 struct GrantCliVault {
-    root: PathBuf,
+    root: credentials_core::test_support::TestTempDir,
     data_dir: PathBuf,
     key_path: PathBuf,
</file context>

[cubic-dev-ai]

P3: The unique_temp_dir doc comment is now wrong: it says callers create the directory themselves and that this path cannot refuse a collision the way tmp_root does. Since this line now returns TestTempDir::new(...), which calls create_dir and panics on AlreadyExists, the function both creates the directory and refuses collisions. Update the comment to state that the guard now owns and creates the directory and refuses collisions.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/tests/common/mod.rs, line 87:

<comment>The `unique_temp_dir` doc comment is now wrong: it says callers create the directory themselves and that this path cannot refuse a collision the way `tmp_root` does. Since this line now returns `TestTempDir::new(...)`, which calls `create_dir` and panics on `AlreadyExists`, the function both creates the directory and refuses collisions. Update the comment to state that the guard now owns and creates the directory and refuses collisions.</comment>

<file context>
@@ -86,13 +78,13 @@ pub fn tmp_root(tag: &str) -> PathBuf {
         .map(|d| d.subsec_nanos())
         .unwrap_or(0);
-    std::env::temp_dir().join(format!("{label}-{}-{n}-{nanos:09}", process::id()))
+    TestTempDir::new(format!("{label}-{}-{n}-{nanos:09}", process::id()))
 }
 
</file context>

[cubic-dev-ai]

P3: The gate now passes --features credentials-core/test-support on the workspace test and seam-clippy arms, but CI's matching arms do not, and this file's own header makes gate/CI flag parity a hard invariant. The flag is also redundant here: test-support is already enabled transitively (credentials-module's dev-dependency sets it, and every seam feature declares it), so CI compiles the same code without the flag. Either drop the redundant flag to restore parity, or add it to CI deliberately.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/gate.sh, line 248:

<comment>The gate now passes `--features credentials-core/test-support` on the workspace test and seam-clippy arms, but CI's matching arms do not, and this file's own header makes gate/CI flag parity a hard invariant. The flag is also redundant here: test-support is already enabled transitively (credentials-module's dev-dependency sets it, and every seam feature declares it), so CI compiles the same code without the flag. Either drop the redundant flag to restore parity, or add it to CI deliberately.</comment>

<file context>
@@ -235,17 +235,17 @@ stream and pass the arm without ever seeing it skip."
-run_expect 578 "workspace unit + integration" \
-  cargo test --locked --workspace
+run_expect 580 "workspace unit + integration" \
+  cargo test --locked --workspace --features credentials-core/test-support
 
 # Two independent defences, because each catches what the other misses:
</file context>

Suggested change

	cargo test --locked --workspace --features credentials-core/test-support
	cargo test --locked --workspace