Give the test suite one temp root instead of fourteen ambient guesses - #406
Conversation
Rule commands run under base_child_env(), which passes PATH and nothing
else, so a test the build launches sees no TMPDIR and
std::filesystem::temp_directory_path() falls back to bare /tmp. Where /tmp
is read-only the scratch write fails, the REQUIRE fires, and Catch2 — built
-fno-exceptions — aborts the process. That abort is what the issue recorded
as a second, unrelated SIGABRT, and the shard that reported it varied
between runs, which is what made it look like a 32-way race. It is neither:
one failure, one cause, reproducible in a single sequential process under
env -u TMPDIR, with the failed REQUIRE and the SIGABRT in one output.
Every test that needs scratch now asks pup::test::temp_root(), which
resolves one writable base per process — the ambient temp directory when it
works, then /var/tmp, then /dev/shm — and hands out unique paths under it.
Each candidate is probed by actually creating a directory rather than
assumed. The hardcoded "/tmp/claude" fallback in the e2e fixture is deleted
rather than generalized: it was an earlier repair of this same failure
applied in the one file where it was noticed, which fixed that instance and
left the class alive to be rediscovered here.
The class was wider than the grep that found it. Two sites reached for an
ambient path in spellings that temp_directory_path() does not match — a
getenv("TMPDIR") with a bare "/tmp" literal fed to mkdtemp, and "/tmp" used
as a working directory — and the first of them was the only test still
failing after every known site had been converted.
Scratch has to stay outside any putup project tree, not merely be writable.
A working-directory fallback passed every per-test run and then failed the
sharded runner, where cwd is test/runner — itself a putup project — with
"Attempting to create files the build does not own". putup was right to
refuse it; the constraint is now recorded where the resolution happens.
Two sites are deliberately left alone: a realpath comparison that needs a
real system path to exist rather than to be writable, and the e2e fixture
data.
The helper is header-only so test/unit/Tupfile keeps its explicit source
list and the build graph is unchanged; no bootstrap regeneration is needed.
Known and recorded rather than fixed: /dev/shm can be mounted noexec, and
e2e fixtures compile and execute binaries in their scratch tree, so a host
where both earlier candidates are unwritable and /dev/shm is noexec would
fail confusingly. Dropping /dev/shm would abort the suite on the machine
this issue is about, where it is the candidate that works, and an honest
exec probe means fork+exec at startup in every test process because
access(X_OK) does not reflect a noexec mount.
Verified: make test exit 0 (170219 assertions, 835 cases, 32 shards); the
full suite green in one process under env -u TMPDIR; five formerly failing
tests quoted red before the change and green after; format, format-check,
tidy, iwyu and spec-check all exit 0. Pair-reviewed by a non-Opus partner:
no blockers.
Fixes #397
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014wx1bWwRf23eFT82y9D641
test/runner is a nested putup project and `make test` configures it, so test/runner/tup.config appears after any test run — on main as much as on a branch. It was neither tracked nor ignored, which makes every git status report a modification nobody made and trains the reader to skim the untracked list. Anchored to the one path so it cannot silently ignore a future tracked tup.config elsewhere in the tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014wx1bWwRf23eFT82y9D641
PR metricsPerformance (gcc example, Linux)
Deterministic signals: instructions (cachegrind-simulated instruction reads — exact across runs, no PMU needed), page faults, peak RSS, and the cachegrind D1/LL miss rates. CPU time is user+sys from time(1). Internal statistics (gcc example, up-to-date dry run)
Counters from Binary size (Linux)
Code churn (whole codebase, last 30d)
Of the lines written across the codebase in the last 30 days, how many are already gone — work that was written and then discarded or rewritten inside the same window. This is the state of the tree including this PR, not a measure of the PR itself. Only code we write is counted: tests, examples, vendored and generated files, CI plumbing and prose are excluded. 5268 lines were deleted in the window in total, most of them older than it. Where the churn is
Test coverage (lines)
102 files · 16614/18854 lines covered Deltas vs main@26ddb6105. Updated for 8027ca8 |
A persisted entry type was cast to NodeType and a persisted edge type to LinkType without checking either against its enum. NodeType names 0-10, so a recorded byte of 11-255 became a well-defined value no enumerator names; it then fell through prior_paths' deliberately default-less switch into none of sources, generated, or unowned, and fed the overwrite guard as a path classified as nothing. The switch's totality comment was true of the switch and false of the function. The edge side was the same shape with a worse tail: LinkType names 1-5 and has no zero enumerator, so both 0 and anything above 5 produced LinkRole::Unknown, which joins no mask and therefore routes nothing — a recorded dependency silently ceasing to route is the failure class the incremental campaign exists to prevent. An unnameable type byte is damage, not a value from a future version. Tolerating it has no disposition a reader can implement: dropping the entry is the silent-nothing state itself, and substituting a classification is the substitution this area already forbids. Forward compatibility does not apply either, because the readable window's floor admits only versions whose type vocabulary is a prefix of this one's. Enforce at the cast site so no entry carrying an unnameable type is ever constructed: FileEntry::from_raw and EdgeEntry::from_raw now return Result and report IndexDamaged, which the announcement side already covers. Observers are untouched, no default: arm is added anywhere, and the totality comment stands unchanged because the fix makes it true. names_node_type switches exhaustively with no default, so adding a NodeType enumerator fails -Wswitch rather than silently widening what the reader accepts. names_link_type delegates to link_role instead of restating the valid range, keeping one source of truth for which link types exist. The recovery read (read_prior_paths, #291) reports the record as lost on a damaged byte rather than proceeding with a partial table — no prior knowledge is strictly safer than knowledge that omits entries. Verified: RED quoted for both legs before any production code. build, format, tidy, iwyu, spec-check all exit 0; [index] tag 0 with 7356 assertions. The full suite is green when carried onto the #397 base; on main it fails only in test_builder.cpp:87, which makes scratch under a read-only /tmp on this machine and is exactly what PR #406 fixes. This change does not touch that file. Fixes #399
Test-infrastructure only — no
src/change, no Tupfile change, so the build graph is untouched and no bootstrap regeneration is needed.The issue was misdiagnosed, and the correction is the fix
It is filed as a 32-way race on fixed-name temp paths. It is neither a race nor a name collision.
putupruns rule commands hermetically:base_child_env()(src/platform/process-posix.cpp:52) builds a child environment containing onlyPATH, and the scheduler uses it (src/exec/scheduler.cpp:775). So no rule command seesTMPDIR,std::filesystem::temp_directory_path()falls back to bare/tmp, and on a machine where/tmpis read-only the scratch write fails. Deterministic, in one sequential process:The suite is built
-fno-exceptions, so the SIGABRT is Catch2 aborting the failedREQUIRE— the issue's "two unrelated failures" are one event, and both appear in that single output. Which shard reported it varied by scheduling, which is what made it look like contention. Running the fast rule and all 32 shards concurrently by hand passes, three rounds for three rounds.CI stays green because its
/tmpis writable, so the same environment stripping is harmless there.What changed
Every test that needs scratch now calls
pup::test::temp_root()(new header-onlytest/unit/temp_root.hpp), which resolves one writable base per process — ambient temp, then/var/tmp, then/dev/shm, each probed by actually creating a directory — and hands out unique paths beneath it. 14 sites across 12 files.The hardcoded
"/tmp/claude"fallback in the e2e fixture is deleted, not generalized. It was an earlier repair of this same failure, applied in the one file where someone noticed it; it fixed that instance and left the class alive, which is why this issue exists.Two sites are deliberately left alone and said so rather than skipped silently: a
realpathcomparison that needs a real system path to exist rather than be writable, and the e2e fixture data.Two things worth reading in review
The grep that finds this class does not find all of it. Two sites spelled it differently —
getenv("TMPDIR")with a bare"/tmp"literal fed tomkdtemp, and"/tmp"used as a working directory. The first was the only test still failing after every known site had been converted, and cost a fullmake testcycle to find.Writable is not the constraint; writable and outside any putup project tree is. The first version of the fix fell back to the current working directory. Every per-test run went green, then
make testfailed in shards 17 and 23 withAttempting to create files the build does not own— shards run with cwd =test/runner, which is itself a putup project, so putup correctly refused scratch created there. That constraint is now recorded where the resolution happens.Verification
make testenv -u TMPDIRmake format/make format-checkmake tidymake iwyumake spec-checkFive formerly failing tests were quoted red before the change and green after. Exit statuses were captured on their own line, never through a pipe. Pair-reviewed by a non-Opus partner against a hash-verified snapshot: no blockers.
Known, recorded rather than fixed
/dev/shmcan be mountednoexec, and the e2e shell fixtures compile and then execute binaries in their scratch tree. On a host where ambient temp and/var/tmpare both unwritable and/dev/shmisnoexec, that fails confusingly rather than cleanly.It is left as built deliberately: on the machine this issue is about, ambient temp and
/var/tmpare both read-only and/dev/shmis the candidate that works — the full suite, including every fixture that compiles and runs a binary, passes from there — so dropping it would un-fix the issue on the machine that has it. An honest exec probe is not cheap either: Linuxaccess(X_OK)does not reflect anoexecmount, so probing truthfully means fork+exec at startup in all 33 test processes.Fixes #397
🤖 Generated with Claude Code
https://claude.ai/code/session_014wx1bWwRf23eFT82y9D641