Skip to content

Name every test scratch directory the one collision-proof way - #185

Merged
MJohnson459 merged 1 commit into
mainfrom
scratch-dir-one-way
Aug 20, 2026
Merged

Name every test scratch directory the one collision-proof way#185
MJohnson459 merged 1 commit into
mainfrom
scratch-dir-one-way

Conversation

@MJohnson459

Copy link
Copy Markdown
Contributor

Gives the workspace one collision-proof way to name a test scratch directory,
and moves every site that had its own onto it.

The problem

Twenty test scratch roots named themselves from the process id plus a
SystemTime nanosecond stamp. That reads as though it could not repeat, and on
one thread it does not, because each SystemTime::now() is ordered after the
last. Across threads there is no such ordering and the clock does not advance a
nanosecond at a time, so two threads reading inside one step read the same
number and land on the same directory — the same SQLite database, the same git
repository. Task 396 found this in the dispatch fixture, where a run produced an
intermittent UNIQUE constraint failed: projects.name.

Surveying the rest turned up more exposure than 455 was filed with. The task
expected the risk to sit with tag-based helpers like store::scratch(tag),
which are safe only while every caller remembers to pass a distinct tag. In fact
four shared helpers have several callers and no distinguishing tag at all, so
they were collision-capable already rather than after some future copy-paste:
worktree::repo (eight callers), cli::ctx_with_toml (six),
app::app_with_agents (four) and app::pr_ready_app (three).

Change

Adopt tempfile as a dev-dependency and use it at all twenty sites, across
cli.rs, ui.rs, app.rs, worktree.rs, dispatch.rs, config_edit.rs,
store.rs and tests/propose_ignores_ambient_task_id.rs:

tempfile::Builder::new().prefix("voro-<area>-").tempdir().unwrap().keep()

tempfile creates the directory itself, exclusively, and retries under a
different name on a clash, so distinctness is a property of the call rather than
an argument about how finely the clock ticks. Because it creates the directory,
twelve now-redundant create_dir_all calls come out with it. The net is 220
lines deleted for 148 added.

store::scratch_db is folded in beyond the original inventory. Its own atomic
counter did keep concurrent callers apart, but with no stamp it left the file
open to reuse by a later run under a recycled pid, and leaving it would have
meant the workspace still had two mechanisms — which is the one thing this task
exists to end.

CLAUDE.md now documents the idiom under its testing rules, so the convention
is where a contributor looks rather than buried in one test module.

Decisions taken

tempfile over a hand-rolled helper (operator's call, asked before starting).
It is correct by construction rather than by argument, and it needs no
cross-crate test plumbing: a hand-rolled helper would have meant either a
feature-gated hidden module inside voro-core, a published crate, purely to
serve its sibling, or two copies of the same ten lines. It is dev-only, so it
reaches neither the shipped binary nor a consumer build, and it adds exactly one
leaf crate — fastrand — since every other transitive dependency was already in
Cargo.lock. This is the "justify anything beyond the boring dependencies"
note CLAUDE.md asks for.

Cleanup deliberately not included (also the operator's call). TempDir
deletes on drop, so the guard would have to outlive each test and every helper
returning a PathBuf would have to return it — dispatch::fixture alone has
fifty-eight call sites to reshape. keep() holds today's behaviour exactly:
the directories stay behind, so a failing test's scratch state survives for
inspection. Filed as task 462, which quantifies what deferring it costs.

What was removed

The dispatch fixture's atomic counter and the test that measured its clock
collisions, both added by 396, go with the mechanism they guarded. That test
named four thousand roots to demonstrate a clock-only name repeats; under
tempfile every name is created on disk, so re-running it would leave four
thousand kept directories behind per run to prove a property that now holds by
construction. The eight-thread test over the real fixture stays, since it still
guards our own helper.

Verification

  • cargo clippy --workspace --all-targets -- -D warnings clean.
  • cargo test --workspace — 878 tests — green over five consecutive runs.
  • Confirmed the directories are still left behind after a run, and that no
    temp_dir().join(format!(...)) name built from a clock stamp remains.

Note for review

Branched off fix-dispatch-fixture-root (task 396, PR #176) rather than main,
because this change consolidates the fixture that PR introduced. It needs 396 to
land first, or to be rebased if 396 is dropped.

Follow-ups filed

  • 462 — let the scratch directories clean themselves up; the deferred half
    of this task, with the disk cost measured (29,092 voro-* directories on this
    workstation, in a /tmp grown to 7.0G).
  • 463 — a second, smaller family of temp paths named from the pid and a
    per-test tag with no stamp, in reconcile.rs, cli.rs, dispatch.rs and
    agent.rs. They cannot collide within a run but can reuse a previous run's
    leftovers under a recycled pid; reconcile::sessions_fixture already carries
    a remove_dir_all workaround that shows the problem is real.

Twenty-one test scratch roots across the workspace built their name from
the process id and a SystemTime nanosecond stamp. That reads as though
it could not repeat, and on one thread it does not, because each
SystemTime::now() is ordered after the last. Across threads there is no
such ordering and the clock does not advance a nanosecond at a time, so
two threads reading inside one step read the same number and land on the
same directory — the same database, the same git repo.

Four of those sites are shared helpers with several callers and no tag
to tell one call from another, so they could collide today rather than
after some future copy-paste: worktree::repo (eight callers),
cli::ctx_with_toml (six), app::app_with_agents (four) and
app::pr_ready_app (three).

Move them all onto tempfile, which creates the directory exclusively and
retries under a different name on a clash, so distinctness is a property
of the call rather than an argument about clock granularity. Fold in
store::scratch_db too: its own counter kept concurrent callers apart but
left the file exposed to reuse by a later run under a recycled pid, and
leaving it in place would mean the workspace still had two mechanisms.

keep() holds today's behaviour — the directories stay behind, as they
always have, so a failing test's scratch state survives for inspection.
Cleanup is deliberately not part of this change: TempDir deletes on
drop, so the guard would have to outlive each test and every helper
returning a PathBuf would have to return it, reshaping call sites the
dispatch fixture alone has fifty-eight of.

The dispatch fixture's counter and the test that measured its clock
collisions go with it. That test named four thousand roots to show a
clock-only name repeats; under tempfile every name is created, so
re-running it would leave four thousand kept directories behind per run
to prove a property that now holds by construction.

CLAUDE.md gains the convention under its testing rules, so it sits where
a contributor looks rather than inside one test module. The
twenty-first site arrived in #178 while this change was being written,
which is the copy-paste the convention exists to stop.

tempfile is dev-only, so it reaches neither the shipped binary nor a
consumer build, and it adds one leaf crate: every other transitive
dependency was already in Cargo.lock.

Verified: cargo clippy --workspace --all-targets -D warnings clean, and
cargo test --workspace (895 tests) green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@MJohnson459
MJohnson459 merged commit e68e85c into main Aug 20, 2026
7 checks passed
@MJohnson459
MJohnson459 deleted the scratch-dir-one-way branch August 20, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant