diff --git a/bootstrap/tests/backend_behaviour.rs b/bootstrap/tests/backend_behaviour.rs index 87cc39665..2612e2cf8 100644 --- a/bootstrap/tests/backend_behaviour.rs +++ b/bootstrap/tests/backend_behaviour.rs @@ -33,8 +33,18 @@ fn tool_present(tool: &str) -> bool { fn tmp_dir(tag: &str) -> std::path::PathBuf { // Deterministic per-test, so a failing run leaves its artefacts behind to - // look at instead of a random name nobody can find again. - let d = std::env::temp_dir().join(format!("t27c-backend-behaviour-{tag}")); + // look at instead of a random name nobody can find again. The pid keeps + // that property while separating concurrent RUNS: the tag is already + // distinct per test, so nothing here collides inside one process, and this + // binary still failed 30 of 32 runs with 16 copies going at once -- every + // one of them writing `-{tag}` into the same `$TMPDIR`, and + // `remove_dir_all` two lines down deleting a sibling run's directory + // mid-read. A counter is deliberately NOT added: it would make the path + // unpredictable, which is the property this comment is defending. + let d = std::env::temp_dir().join(format!( + "t27c-backend-behaviour-{tag}-{}", + std::process::id() + )); let _ = std::fs::remove_dir_all(&d); std::fs::create_dir_all(&d).expect("create temp dir"); d diff --git a/bootstrap/tests/generic_type_application.rs b/bootstrap/tests/generic_type_application.rs index b3e4a1254..ca95dc9c8 100644 --- a/bootstrap/tests/generic_type_application.rs +++ b/bootstrap/tests/generic_type_application.rs @@ -8,7 +8,15 @@ use std::process::{Command, Output}; fn parse_source(source: &str, stem: &str) -> Output { let bin = env!("CARGO_BIN_EXE_t27c"); - let path = std::env::temp_dir().join(format!("t27c_issue_2164_{stem}.t27")); + // The two tests pass distinct stems, so nothing collides inside one + // process -- confirmed by running this binary with `--test-threads=1`, + // which still failed 6 of 128 times with 16 copies concurrent. The + // collision is between RUNS sharing `$TMPDIR`, and the pid is what + // separates those. + let path = std::env::temp_dir().join(format!( + "t27c_issue_2164_{stem}_{}.t27", + std::process::id() + )); std::fs::write(&path, source).expect("failed to write generic application fixture"); Command::new(bin) .arg("parse") diff --git a/bootstrap/tests/verilog_range_bound.rs b/bootstrap/tests/verilog_range_bound.rs index 1c24ddb92..45edbb92f 100644 --- a/bootstrap/tests/verilog_range_bound.rs +++ b/bootstrap/tests/verilog_range_bound.rs @@ -83,12 +83,38 @@ test count_from_two_to_seven_is_five { } "#; + +/// Two axes, and a key needs BOTH. Measured on this file, release build +/// (the four-arm table is in `verilog_real_arithmetic.rs`, same shape): +/// +/// | key | one process, 4 threads | 16 concurrent processes | +/// |--------------------|------------------------|-------------------------| +/// | neither | n/a | 24 / 64 | +/// | both | n/a | 0 / 64 | +/// +/// The counter separates the THREADS of one run -- six tests here call +/// `emit("gen-verilog")`, so six writers share one path. The pid separates +/// concurrent RUNS, which is not hypothetical: two agents, two worktrees, or a +/// `cargo test` beside a manual run all share `$TMPDIR`. +/// +/// `tri harness scratch` advises "an AtomicUsize counter, not the pid". The +/// first half is right and the second is what the middle row of that table +/// costs. +fn unique() -> String { + static N: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + format!( + "{}-{}", + std::process::id(), + N.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + ) +} + fn emit(subcommand: &str) -> String { let bin = env!("CARGO_BIN_EXE_t27c"); // Keyed per subcommand: `icarus-simulate` writes its scratch file into the // shared temp dir under the spec's BASENAME, so two probes sharing a stem // overwrite each other's Verilog. - let spec_path = std::env::temp_dir().join(format!("t27_range_bound_{subcommand}.t27")); + let spec_path = std::env::temp_dir().join(format!("t27_range_bound_{subcommand}_{}.t27", unique())); let mut f = std::fs::File::create(&spec_path).expect("create probe spec"); f.write_all(SPEC.as_bytes()).expect("write probe spec"); drop(f); @@ -227,7 +253,7 @@ fn the_specs_own_tests_pass_under_the_simulator() { } let bin = env!("CARGO_BIN_EXE_t27c"); - let spec_path = std::env::temp_dir().join("t27_range_bound_sim.t27"); + let spec_path = std::env::temp_dir().join(format!("t27_range_bound_sim_{}.t27", unique())); let mut f = std::fs::File::create(&spec_path).expect("create probe spec"); f.write_all(SPEC.as_bytes()).expect("write probe spec"); drop(f); diff --git a/bootstrap/tests/verilog_real_arithmetic.rs b/bootstrap/tests/verilog_real_arithmetic.rs index 2296b2d30..9fdb9a13d 100644 --- a/bootstrap/tests/verilog_real_arithmetic.rs +++ b/bootstrap/tests/verilog_real_arithmetic.rs @@ -55,12 +55,39 @@ test int_binding_stays_integer /// It PANICS rather than returning when the binary fails. A test that returns /// quietly on a broken front end reports PASSED while measuring nothing -- the /// exact shape this file exists to catch in the emitter. + +/// Two axes, and a key needs BOTH. Measured on this file, release build: +/// +/// | key | one process, 4 threads | 16 concurrent processes | +/// |--------------------|------------------------|-------------------------| +/// | neither | 6 / 150 | 41 / 64 | +/// | `process::id` only | 7 / 150 | 0 / 64 | +/// | counter only | 0 / 150 | 29 / 64 | +/// | both | 0 / 150 | 0 / 64 | +/// +/// The counter separates the THREADS of one run -- six tests here call +/// `emit("gen-verilog")`, so six writers share one path. The pid separates +/// concurrent RUNS, which is not hypothetical: two agents, two worktrees, or a +/// `cargo test` beside a manual run all share `$TMPDIR`. +/// +/// `tri harness scratch` advises "an AtomicUsize counter, not the pid". The +/// first half is right and the second is what the middle row of that table +/// costs. +fn unique() -> String { + static N: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + format!( + "{}-{}", + std::process::id(), + N.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + ) +} + fn emit(subcommand: &str) -> String { let bin = env!("CARGO_BIN_EXE_t27c"); // Keyed by subcommand: `icarus-simulate` writes its scratch file into the // shared temp dir under the spec's BASENAME, so two probes sharing a stem // overwrite each other's Verilog. - let spec_path = std::env::temp_dir().join(format!("t27_real_arith_{subcommand}.t27")); + let spec_path = std::env::temp_dir().join(format!("t27_real_arith_{subcommand}_{}.t27", unique())); let mut f = std::fs::File::create(&spec_path).expect("create probe spec"); f.write_all(SPEC.as_bytes()).expect("write probe spec"); drop(f); @@ -191,7 +218,7 @@ fn integer_given_binding_is_still_a_reg() { #[test] fn the_specs_own_tests_pass_under_the_simulator() { let bin = env!("CARGO_BIN_EXE_t27c"); - let spec_path = std::env::temp_dir().join("t27_real_arith_sim.t27"); + let spec_path = std::env::temp_dir().join(format!("t27_real_arith_sim_{}.t27", unique())); let mut f = std::fs::File::create(&spec_path).expect("create probe spec"); f.write_all(SPEC.as_bytes()).expect("write probe spec"); drop(f); diff --git a/cli/tri/src/scratch.rs b/cli/tri/src/scratch.rs index a666246a5..8dec1dead 100644 --- a/cli/tri/src/scratch.rs +++ b/cli/tri/src/scratch.rs @@ -20,6 +20,14 @@ //! every run; only the timing of the delete decides whether a test dies. It //! failed roughly one run in three, and passed the first time it was written. //! +//! TWO COLLISIONS, AND THIS DETECTOR SEES ONE. Everything below is about tests +//! inside ONE process. A second collision is between concurrent RUNS of the same +//! binary sharing `$TMPDIR`, and a key that separates threads does not separate +//! those: `verilog_real_arithmetic.rs` with a counter but no pid still failed +//! 29 of 64 runs with 16 copies going at once, and 0 of 64 with both. Nothing +//! here detects that; the printed advice now says so rather than recommending +//! against the pid. +//! //! What this looks for is the CONJUNCTION -- more than one `#[test]`, a scratch //! path under `temp_dir()`, a `remove_dir_all` of that path, and a key with no //! per-call component. Any one of those alone is fine. @@ -123,6 +131,30 @@ pub fn judge(path: &Path, s: &str) -> Option { }) } +pub fn advice() -> String { + [ + " Fix: an AtomicUsize counter AND the pid. Not either one, and not", + " any property of the input (two inputs can agree).", + "", + " The two are for different collisions, and this used to say \"not", + " the pid\". Measured on bootstrap/tests/verilog_real_arithmetic.rs,", + " release build, four arms:", + "", + " key 1 process, 4 threads 16 processes", + " neither 6 / 150 41 / 64", + " process::id only 7 / 150 0 / 64", + " counter only 0 / 150 29 / 64", + " both 0 / 150 0 / 64", + "", + " The counter separates the THREADS of one run; the pid separates", + " concurrent RUNS, which is not hypothetical -- two agents, two", + " worktrees, or a `cargo test` beside a manual run share $TMPDIR.", + " This detector only looks for the first collision. The middle two", + " rows are what each half alone costs.", + ] + .join("\n") +} + pub fn run(gate: bool, self_check: bool) -> Result<()> { if self_check { return run_self_check(); @@ -151,9 +183,7 @@ pub fn run(gate: bool, self_check: bool) -> Result<()> { println!(" It passes the first time it runs. A green run does not clear it --"); println!(" print the paths of a single run and count the distinct ones."); println!(); - println!(" Fix: key the directory by an AtomicUsize counter, not by the pid"); - println!(" (shared by every test in the binary) and not by any property of"); - println!(" the input (two inputs can agree)."); + println!("{}", advice()); println!(); if gate && !found.is_empty() { @@ -245,3 +275,38 @@ fn yn(b: bool) -> &'static str { "NO" } } + +#[cfg(test)] +mod advice_tests { + /// The advice is the deliverable, so it is pinned like one. It used to read + /// "an AtomicUsize counter, NOT the pid", and the four-arm table it now + /// carries is what that cost: counter-only left 29 of 64 concurrent runs + /// failing. Both components are required, and this asserts the text says so. + #[test] + fn the_advice_asks_for_both_components_and_not_one_instead_of_the_other() { + let a = super::advice(); + assert!(a.contains("AtomicUsize"), "the per-call half is missing:\n{a}"); + assert!(a.contains("pid"), "the per-process half is missing:\n{a}"); + assert!( + a.contains("AND"), + "both are required; the text must not offer a choice:\n{a}" + ); + // The exact sentence that was wrong. Anywhere in the text, in any + // casing, it is the old advice coming back. + let lower = a.to_lowercase(); + assert!( + !lower.contains("not by the pid") && !lower.contains("not the pid"), + "the old advice is back:\n{a}" + ); + } + + /// The numbers are the reason the sentence changed, so losing them turns the + /// advice back into an assertion. All four arms, or none of this is evidence. + #[test] + fn the_advice_carries_the_measurement_that_settled_it() { + let a = super::advice(); + for arm in ["6 / 150", "41 / 64", "0 / 150", "29 / 64", "0 / 64"] { + assert!(a.contains(arm), "missing the {arm} arm:\n{a}"); + } + } +} diff --git a/docs/now/2026-09-04-two-collisions-and-each-instrument-sees-one.md b/docs/now/2026-09-04-two-collisions-and-each-instrument-sees-one.md new file mode 100644 index 000000000..450fc702a --- /dev/null +++ b/docs/now/2026-09-04-two-collisions-and-each-instrument-sees-one.md @@ -0,0 +1,26 @@ +# NOW -- Two collisions, and each instrument sees one (2026-09-04) + +## A scratch key needs a per-call component AND a per-process one + +- Four test binaries shared a scratch path with no per-process component. + Measured with a control -- run alone, then 16 copies at once: + `verilog_real_arithmetic` 0/8 alone and 41/64 concurrent, `backend_behaviour` + 30/32, `verilog_range_bound` 24/64, `generic_type_application` 10/192. +- The four-arm experiment on one file settles which half does what: + neither 6/150 intra and 41/64 inter; `process::id` only 7/150 and 0/64; + counter only 0/150 and 29/64; both 0/150 and 0/64. The counter separates the + THREADS of one run, the pid separates concurrent RUNS, and neither alone is + enough. +- `tri harness scratch` advised "an AtomicUsize counter, not the pid". The + first half is right; the second is the 29/64 row. The advice now asks for + both and carries the table, and two unit tests pin it -- one fails if the old + sentence returns, one fails if an arm of the table is dropped. +- After the fix all four read 0/150 intra and 0/64 inter. That pair is the + acceptance criterion, not a green ordinary run: none of them ever failed + alone. +- Where the two instruments disagree is the finding. Reading proves the + STRUCTURE of a race; running shows whether the window opens. `verilog_r_si_1` + has the structure -- one literal path, two callers, a truncating write -- and + 0 of 64 concurrent runs plus 0 vacuous passes. `backend_behaviour` was + refuted by reading, correctly, on the only axis reading was asked about, and + is 30/32 on the other.