From bd532dde6b21bad6f51a504ea93813e119d4c830 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 5 Aug 2026 13:41:05 +0200 Subject: [PATCH] =?UTF-8?q?fix(test):=20retire=20R2=20and=20R4=20=E2=80=94?= =?UTF-8?q?=20two=20readiness=20predicates=20weaker=20than=20their=20asser?= =?UTF-8?q?tions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 2 of `docs/macos-ci-signal-integrity-framing.md` revision 3 (acceptance 6–9). Two test races, both the same shape: the thing waited for was weaker than the thing asserted, so the wait could return inside the window the assertion needs closed. R4 — `wait_for_file` returned as soon as `fs::read` succeeded, which succeeds on a ZERO-BYTE FILE. The probe publishes with `open(path,'wb').write(b'1')` and `open()` creates the file before `write()` fills it, so the helper handed `[]` to a caller asserting `== b"1"`. It now takes the expected bytes and waits while the file holds a STRICT PREFIX of them — the states a write in flight can be observed in — returning anything else immediately so the caller's `assert_eq!` stays the discriminating assertion rather than becoming a timeout inside a helper that does not know what was expected. All four callers pass their expectation. `wait_for_published_file`, one function away in the same suite, gated the real-TUI smoke's `assert_eq!(…, b"1")` on the identical predicate and is fixed with it: leaving it would have let R4 recur under a different selector, which the registry would then have had to judge a new incident. R2 — the USR1 fixture waited on `ProcessEventKind::Started`, emitted at SPAWN, not when `/bin/sh` has parsed `trap '' USR1`. SIGUSR1's default disposition is terminate, so a signal inside that window kills the child. The child now publishes a marker AFTER the trap and the test waits for that marker's CONTENT (the same zero-byte trap applies to a shell's `>` redirection). `exec` replaces the forked `sleep`, so the group holds exactly one process and the ignored disposition survives by POSIX rather than by the shell's fork-suppression optimization — an unstated dependency the old fixture had, since these signals are group-directed and a forked `sleep` is an untrapped group member. Four witnesses, each verified by REVERTING the fix and observing the failure rather than by reasoning about it: - `wait_for_file_does_not_return_a_zero_byte_readiness_file` fails `left: []`, `right: [49]` — R4's two required fragments, verbatim; - `wait_for_file_does_not_return_a_partial_write` fails on the torn read a length check alone would admit; - `wait_for_file_returns_divergent_content_rather_than_timing_out` fails against an over-strict helper that waits for an exact match; - `usr1_readiness_waits_for_the_trap_not_for_the_spawn` fails `left: Some("SIGUSR1")`, `right: Some("SIGTERM")` with the readiness wait removed. Its fixture sleeps before `trap` so the pre-trap window is deliberate rather than load-dependent, and it proves survival by the child's EXIT DISPOSITION rather than by an absence observed within a window. R1 is NOT touched — referred to the async-runtime lane (Q#MCI3), because widening its budget would make it pass and measure nothing more. R3 is NOT touched and remains UNRESOLVED, owned by the process-signal / reap-ledger lanes. `docs/ci-red-signatures.md` moves R2 and R4 to a "Retired rows" section with their dispositions and adds the rule the file needed and lacked: a red matching a retired row is a RECURRENCE that puts the retirement in question, never a known flake. `docs/active-work.md` carries this lane from its first commit rather than after review asks for it. Repetition sets, not single runs: the two `--lib` process tests 15/15, the whole `vterm_stage2_acceptance` suite 15/15 at default parallelism. Co-Authored-By: Claude Opus 5 (1M context) --- docs/active-work.md | 131 +++++++++++++++++--------- docs/agent-handoff.md | 4 +- docs/ci-red-signatures.md | 106 +++++++++++++++++---- src/process.rs | 117 ++++++++++++++++++++++- tests/vterm_stage2_acceptance.rs | 154 ++++++++++++++++++++++++++++--- 5 files changed, 432 insertions(+), 80 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 1ba0422..b759f46 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -17,6 +17,13 @@ branch and no owner since #166. It is implemented on (#215, in review), which this file required a lane for and did not have until review caught it — the #171 defect recurring. Its block is below. +**Updated later the same day.** #215 **merged** (`main` @ `12f2970`) and +that lane is **rewritten, not removed** — rule 4 removes a lane when its +ARC is done, and Stage 2 is the arc. Stage 2 (**hardening**) is now in +flight on `ci-signal-hardening`, with its own lane block, its own +checkpoint table, and a lane written **before** the PR was opened rather +than after review asked for it. + **Updated 2026-08-04.** Four PRs landed since: the CI CRDT coverage lane #209, Distribution Stage 1 #211 (released as **v1.1.0**), the post-release accuracy pass #212, and **bottom-panel Stage 3 #213 — @@ -93,8 +100,9 @@ lesson, §1 for the two framings). are identical on every machine. Remote names are otherwise machine-local: `origin` may name this canonical URL, a release mirror, or something else, and therefore has no authority by name alone. -- Canonical base at this snapshot: **`githubsucks/main` @ `f186253`** - — bottom-panel Stage 3 **#213**, which completes Arc 7, atop the +- Canonical base at this snapshot: **`githubsucks/main` @ `12f2970`** — + the macOS CI signal-integrity registry **#215**, atop `f186253`: + bottom-panel Stage 3 **#213**, which completes Arc 7, atop the post-release accuracy pass **#212**, Distribution Stage 1 **#211** (released as **v1.1.0**, the first release with prebuilt binaries), and the CI CRDT coverage lane **#209**. Beneath those, `cfc1710`: @@ -106,7 +114,7 @@ lesson, §1 for the two framings). `Hello` still advertises v20** — two different facts, and #184 landed only the first. **The recovery floor advances with the base**, so the check below - now requires `f186253` or newer; a tree at `cfc1710` no longer + now requires `12f2970` or newer; a tree at `f186253` no longer passes. That is deliberate — a check accepting an older commit than the declared base passes on a tree the rest of this file does not describe. @@ -145,7 +153,7 @@ git worktree list git status --short --branch ``` -The `git log` command must expose `f186253` — the base named above — or a +The `git log` command must expose `12f2970` — the base named above — or a newer intentional main. Keep this threshold and the canonical-base line in step: a recovery check that accepts an older commit than the base it declares canonical will pass on a tree the rest of this file does not @@ -161,31 +169,92 @@ form. All four steps ran clean. **The two-argument form still does not work** for a remote-only branch (`fatal: invalid reference`), which is why every lane below spells out the `-b` form. -## macOS CI signal integrity — STAGE 1 IN REVIEW, PR #215 +## macOS CI signal integrity — STAGE 1 MERGED (#215); STAGE 2 IN REVIEW **This file requires a lane for every open PR** (see the #171/#174 note above: an open PR is exactly the volatile work this file records, and -#171 drifted 153 commits while invisible here). #215 had none until this -entry — the same defect, caught in review. +#171 drifted 153 commits while invisible here). #215 had none until +review caught it; **Stage 2's lane is written with its first commit**, +which is where the same defect stops recurring. -- **Branch `macos-ci-signal-integrity`**, base `githubsucks/main` @ - `bfb97c6`. Framing `docs/macos-ci-signal-integrity-framing.md` - **revision 3**. -- **PR: ** — Stage 1, - docs only. **Checkpoints, newest last** — a lane that records only the - *reviewed* head goes stale the moment a review round is pushed, which - is how #171 became invisible: +Framing `docs/macos-ci-signal-integrity-framing.md` **revision 3**, +already approved. Stage 1 is acceptance 1–5; Stage 2 is acceptance 6–9. + +### Stage 2 — hardening, IN REVIEW + +- **Branch `ci-signal-hardening`**, worktree `../pmacs-ci-signals`, base + `githubsucks/main` @ `12f2970` (the #215 merge). Opened from an + **isolated worktree** because the shared checkout was on another + lane's branch with clean-but-foreign state; never switch it. +- **PR: opened from this branch; the number and its first CI checkpoint + land in the follow-up commit** — this block is written *with* the + work, before the PR exists, so the row below is filled in rather than + invented. **Checkpoints, newest last**, because a lane that records + only one head goes stale on the next push: + + | head | CI run | result | + |---|---|---| + | *(opening head)* | *(pending — filled in once the run exists)* | — | + + **The branch tip is authoritative over any row here.** Verify with + `git rev-parse githubsucks/ci-signal-hardening` rather than trusting + the newest line. +- **What it ships — exactly two fixes, both at the readiness + predicate**, plus four witnesses: + - **R4.** `wait_for_file` takes the **expected bytes** and waits while + the file holds a *strict prefix* of them, so a zero-byte or torn + read is no longer a readiness signal. Four callers updated; **the + identical mechanism in `wait_for_published_file`**, one function + away in the same suite, is fixed with it — leaving it would have let + R4 recur under a different selector. + - **R2.** The USR1 fixture publishes a marker **after** `trap '' USR1` + and the test waits for that marker's **content**; `exec` replaces + the forked `sleep`, so the group holds one process and the ignored + disposition survives by POSIX rather than by a shell's + fork-suppression optimization. +- **The witnesses were verified by REVERTING each fix**, not by + reasoning. With the old predicate restored, + `wait_for_file_does_not_return_a_zero_byte_readiness_file` fails + `left: []`, `right: [49]` — **R4's two required fragments, verbatim** + — and with the readiness wait removed, + `usr1_readiness_waits_for_the_trap_not_for_the_spawn` fails + `left: Some("SIGUSR1")`, `right: Some("SIGTERM")`. +- **R1 is NOT touched** — referred to the async-runtime lane (Q#MCI3); + widening its budget would make it pass and measure nothing more. + **R3 is NOT touched** and stays **unresolved**; it belongs to the + process-signal / reap-ledger lanes and nothing here may make it look + otherwise. +- **Repetition sets, not single runs:** the two `--lib` process tests + **15/15**, the whole `vterm_stage2_acceptance` suite (9 tests, default + parallelism) **15/15**. +- **Verification:** fmt, diff-check, clippy with and without `crdt`, + `--lib` 1897, `--lib --features crdt` 2082, vterm Stage 2 9, m4 149, + required GPU 221, and the **full serialized sweep in BOTH feature + configurations against a `main` baseline** — the blindness the handoff + §5 warns about, which is why the baseline is taken first. Each `--lib` + figure is exactly one above #215's (1896 / 2081): the R2 witness. + +Recovery from a clean checkout: + +```sh +git fetch githubsucks +git worktree add ../pmacs-ci-signals \ + -b ci-signal-hardening \ + githubsucks/ci-signal-hardening +``` + +### Stage 1 — MERGED as #215 + +- **Branch `macos-ci-signal-integrity`**, merged to `main` @ `12f2970` + after two review rounds. Docs only. Final CI checkpoint: | head | CI run | result | |---|---|---| | `d33bf4d` | [30950108477](https://github.com/levineuwirth/pmacs/actions/runs/30950108477) | 14/14 green — the reviewed head | | `f76897c` | [30990816890](https://github.com/levineuwirth/pmacs/actions/runs/30990816890) | review round 1 | - | *(tip)* | — | review round 2: framing wording, the missing quote, this table | + | `2e0617f` | — | review round 2: framing wording, the missing quote, the checkpoint table | - **The branch tip is authoritative over any row here.** Verify with - `git rev-parse githubsucks/macos-ci-signal-integrity` rather than - trusting the newest line. -- **What Stage 1 ships:** `docs/ci-red-signatures.md`, the single +- **What Stage 1 shipped:** `docs/ci-red-signatures.md`, the single authority for judging a red CI run. Rows key on **signature** — selector + job/flavor + every required fragment, normalized — so **a test-name match confers nothing**. The rerun rule is replaced: a green @@ -202,30 +271,6 @@ entry — the same defect, caught in review. not rows — unmatchable by construction, so a red in either is a new incident. The registry is therefore *stricter* than the list it replaces: nothing is pre-excused. -- **Verification:** fmt, diff-check, clippy with and without `crdt`, - `--lib` 1896, `--lib --features crdt` 2081, pmacs-protocol 19, m4 149, - required GPU 221. All three named tests pass locally on Linux — - consistent with R1–R4 being macOS occurrences, and **not evidence - about any of them**. - -### Stage 2 — hardening, NOT started - -Waits on #215's merge. Owns R2's trap-readiness fix and R4's -`wait_for_file` predicate, each with a **repetition set** rather than a -single green run, plus a discriminating witness that fails without the -fix. **R1 is referred to the async-runtime lane** (Q#MCI3) rather than -patched: widening its budget would make it pass and measure nothing -more. **R3 stays unresolved** and belongs to the process-signal / -reap-ledger lanes. - -Recovery from a clean checkout: - -```sh -git fetch githubsucks -git worktree add ../pmacs-ci-signals \ - -b macos-ci-signal-integrity \ - githubsucks/macos-ci-signal-integrity -``` ## CI CRDT coverage — MERGED (#209); kept for its three follow-ons diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index b165193..d84f814 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -1,7 +1,9 @@ # Agent handoff — cross-machine continuity **Last updated: 2026-08-05.** The live CI-triage rule in §5 now points -at `docs/ci-red-signatures.md` (PR #215, in review), which keys on +at `docs/ci-red-signatures.md` (**PR #215, merged as `main` @ +`12f2970`**; its Stage 2 hardening is in flight — see +`docs/active-work.md`), which keys on signature rather than test name; the hazards list this file used to carry is retired, and its two unevidenced entries are audit notes there. Previously **2026-08-04, as bottom-panel Stage 3 #213 — the adopter diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 3c702f9..529ba02 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -79,10 +79,19 @@ Main-branch greens are **occurrence evidence** and accumulate toward a rate. They retire nothing by themselves. Retired rows stay in this file with their disposition, so a recurrence is recognisable. +**A red matching a RETIRED row is a recurrence, and it puts the +retirement in question — it is not a known flake.** The claim a +retirement makes is that the mechanism is gone; the same signature +afterwards falsifies that claim, which is a stronger finding than a live +row, not a weaker one. Reopen the row rather than rerunning. + --- ## Live rows +**Two of the four evidenced rows are live.** R2 and R4 were retired on +2026-08-05 and are below, under "Retired rows", with their dispositions. + ### R1 — supersede cancellation budget | field | value | @@ -102,22 +111,6 @@ test's own `tick()` + `sleep(1ms)` pump, so the interval is dominated by when *the test* was scheduled. **Widening the budget would make it pass and measure nothing more.** -### R2 — USR1 delivered before the trap is installed - -| field | value | -|---|---| -| **selector** | `--lib process::tests::a_successful_signal_disposition_depends_on_whether_it_is_fatal` | -| **job / flavor** | macOS / lua54 | -| **required fragments** | `leader=exited(signal SIGUSR1)` — **one exact fragment, not two loose ones**. Split into `leader=exited(` and `SIGUSR1` it would match a child that exited by some *other* disposition while `SIGUSR1` appeared elsewhere in the output | -| **causal status** | **test race** | -| **evidence** | [#213 run 30927084982 attempt 1](https://github.com/levineuwirth/pmacs/actions/runs/30927084982/attempts/1) | -| **retirement** | the fixture proves the trap is installed, with a witness that fails without it | - -Readiness is `ProcessEventKind::Started`, emitted at **spawn** — not when -`/bin/sh` has parsed `trap '' USR1`. SIGUSR1's default disposition is -terminate, so a signal inside that window kills the child. The fixture's -own comment states the requirement it does not enforce. - ### R3 — live-leader EPERM with an unobservable group | field | value | @@ -138,7 +131,58 @@ field able to disagree — could not be read at all. **Do not treat a red matching this row as environmental.** A green rerun changes nothing about it. -### R4 — readiness predicate satisfied by an empty file +--- + +## Retired rows + +**These stay here on purpose.** A retirement is a claim that a mechanism +is gone; keeping the signature is what makes a recurrence recognisable +as a falsification of that claim rather than as a fresh mystery. Both +were retired **causally** — the mechanism removed, plus a discriminating +witness that fails without the fix — never by a count of green runs. + +### R2 — USR1 delivered before the trap is installed — RETIRED 2026-08-05 + +| field | value | +|---|---| +| **selector** | `--lib process::tests::a_successful_signal_disposition_depends_on_whether_it_is_fatal` | +| **job / flavor** | macOS / lua54 | +| **required fragments** | `leader=exited(signal SIGUSR1)` — **one exact fragment, not two loose ones**. Split into `leader=exited(` and `SIGUSR1` it would match a child that exited by some *other* disposition while `SIGUSR1` appeared elsewhere in the output | +| **causal status** | **test race** | +| **evidence** | [#213 run 30927084982 attempt 1](https://github.com/levineuwirth/pmacs/actions/runs/30927084982/attempts/1) | +| **retirement condition** | the fixture proves the trap is installed, with a witness that fails without it | +| **disposition** | **met.** The child publishes a readiness marker *after* `trap '' USR1`, the test waits for that marker's **content**, and `process::tests::usr1_readiness_waits_for_the_trap_not_for_the_spawn` is the witness | + +Readiness was `ProcessEventKind::Started`, emitted at **spawn** — not when +`/bin/sh` has parsed `trap '' USR1`. SIGUSR1's default disposition is +terminate, so a signal inside that window kills the child. The fixture's +own comment stated the requirement it did not enforce. + +Three things the fix and the witness settled that the row did not say: + +- **Which call failed.** The fragment is rendered only on a *failed* + `kill`, and the USR1 that killed the child cannot itself have failed — + it is what did the killing. The failing call is therefore the SIGTERM + that follows, whose diagnostic reports the leader's earlier death. + **Why a group-directed TERM found no group is not established here**, + and the fix does not depend on the answer. **This says nothing about + R3**, whose leader was observed **live**. +- **The witness proves the predicate, not the platform.** The old fixture + passes on Linux; the window is real everywhere but only macOS ever + reported it. So the witness widens the pre-trap window *deliberately* + (the fixture sleeps before `trap`) instead of hoping a loaded runner + supplies one, and it proves survival by the child's **exit + disposition** — a child that took the USR1 reports + `Signaled { signal: "SIGUSR1" }` — rather than by an absence observed + within a window. +- **The fixture had a second, unnamed dependency.** These signals are + group-directed, so a forked `sleep` would be an *untrapped* member of + the same group. That it survived at all depended on the shell + suppressing the fork for the last command of a `-c` script — a bash + and dash optimization, not a guarantee. The fixture now says `exec`, + and an ignored disposition survives `exec` by POSIX. + +### R4 — readiness predicate satisfied by an empty file — RETIRED 2026-08-05 | field | value | |---|---| @@ -147,14 +191,28 @@ changes nothing about it. | **required fragments** | `left: []` **and** `right: [49]` | | **causal status** | **test race** | | **evidence** | [#214 run 30932558752 attempt 1](https://github.com/levineuwirth/pmacs/actions/runs/30932558752/attempts/1) | -| **retirement** | `wait_for_file` requires the expected content, with a witness that fails against a zero-byte file | +| **retirement condition** | `wait_for_file` requires the expected content, with a witness that fails against a zero-byte file | +| **disposition** | **met.** The helper takes the expected bytes and waits while the file holds a **strict prefix** of them; `wait_for_file_does_not_return_a_zero_byte_readiness_file` is the witness, and with the old predicate restored it fails with `left: []`, `right: [49]` — **this row's two fragments, verbatim** | -`wait_for_file` returns as soon as `fs::read` succeeds — which succeeds +`wait_for_file` returned as soon as `fs::read` succeeded — which succeeds on a **zero-byte file**. The probe writes readiness with `open(path,'wb').write(b'1')`, and `open()` creates the file before -`write()` fills it. The predicate is "readable"; the assertion is +`write()` fills it. The predicate was "readable"; the assertion is "contains `1`" (`49` is ASCII `'1'`). +Two notes for anyone who reads a future red here: + +- **The same mechanism lived in a second helper.** `wait_for_published_file`, + one function away in the same suite, gated the real-TUI smoke's + `assert_eq!(…, b"1")` on the identical "readable" predicate. It was + fixed with the same predicate; leaving it would have let this row + recur under a different selector, which the registry would have had to + judge a new incident. +- **The helper returns divergent content instead of waiting for a + match**, so a child that publishes the *wrong* thing is reported as a + diff by the caller that owns the expectation, rather than as a timeout + in a helper that does not. That behaviour has its own witness. + --- ## Audit notes — historical claims with no linked occurrence @@ -219,6 +277,14 @@ replaces: nothing is pre-excused. Four incidents, three tests, **four signatures**. Count signatures: the process test contributed two, and only one of them is a test bug. +| date | row | event | +|---|---|---| +| 2026-08-05 | R2, R4 | **retired** — mechanism removed, discriminating witness added; see "Retired rows" | + +**The retirements are not occurrences and do not close the log.** R1 and +R3 stay live, and each retired row keeps its signature so a later red +matching one reopens it. + **All four *evidenced* rows (R1–R4) are macOS.** That is a property of these occurrences, not of the file: **A1's job is `GPU Render (headless)`, which runs on Ubuntu**, and **A2's job was never diff --git a/src/process.rs b/src/process.rs index 1d8af2f..c4a9277 100644 --- a/src/process.rs +++ b/src/process.rs @@ -2796,6 +2796,61 @@ mod tests { .expect("Started carries a pid") } + /// Wait until `path` holds exactly `expected`. + /// + /// **The predicate is the content, never the file's existence.** A + /// shell publishes readiness with `printf … > path`, and the `>` + /// redirection creates the file *before* the command that fills it — + /// so "it exists", and even "it is readable", is satisfied by a + /// zero-byte file. That is `docs/ci-red-signatures.md` R4's mechanism + /// arriving in a second place; a readiness gate written the weak way + /// re-opens the very window R2 is about. + fn wait_for_published(path: &std::path::Path, expected: &[u8], timeout: Duration) { + let deadline = Instant::now() + timeout; + loop { + if std::fs::read(path).is_ok_and(|bytes| bytes == expected) { + return; + } + assert!( + Instant::now() < deadline, + "child never published {expected:?} to {} within {timeout:?} \ + (last read: {:?})", + path.display(), + std::fs::read(path).ok() + ); + std::thread::sleep(Duration::from_millis(10)); + } + } + + /// `/bin/sh -c` argument for a child that ignores SIGUSR1, says so, + /// and then becomes `sleep` — the R2 fixture. + /// + /// The order is the whole point: + /// + /// - **`trap '' USR1` first.** SIGUSR1's default disposition is + /// terminate, and `ProcessEventKind::Started` is emitted when the + /// process is *spawned*, not when `/bin/sh` has parsed anything. + /// Waiting on `Started` therefore returns inside a window where the + /// signal kills the child. + /// - **The marker second**, so the test has something to wait on that + /// the shell can only publish once the trap exists. + /// - **`exec` last**, so the process group holds exactly one process. + /// An *ignored* disposition survives `exec`, while a forked `sleep` + /// would be an untrapped member of the same group — and these + /// signals are group-directed, so it would die and take the shell's + /// `wait` (and the group) with it. + /// + /// `pre_trap` is the delay the R2 witness prepends to widen the + /// pre-trap window deliberately; the fixture itself passes `""`. + fn trapped_usr1_command(ready: &std::path::Path, pre_trap: &str) -> String { + let path = ready.to_str().expect("UTF-8 temp path"); + assert!( + !path.contains('\''), + "the single-quoting below assumes no quote in the temp path: {path}" + ); + format!("{pre_trap}trap '' USR1; printf trapped > '{path}'; exec sleep 30") + } + /// Drive the production diagnostic until it observes the leader as /// exited, bounded by `timeout`. /// @@ -3223,13 +3278,20 @@ mod tests { #[test] fn a_successful_signal_disposition_depends_on_whether_it_is_fatal() { let mut sup = ProcessSupervisor::new(); + let temp = tempfile::TempDir::new().expect("tempdir"); + let ready = temp.path().join("usr1-trapped"); let mut spec = ProcessSpec::new("diag-disposition-live", "/bin/sh"); // Ignore USR1 so the successful non-fatal signal cannot end the - // child and confuse the state assertion with a real exit. - spec.args = vec!["-c".into(), "trap '' USR1; sleep 30".into()]; + // child and confuse the state assertion with a real exit — and + // then WAIT for the child to say it has done so. `Started` is + // emitted at spawn and proves nothing about what `/bin/sh` has + // parsed (`docs/ci-red-signatures.md` R2); see + // `trapped_usr1_command`. + spec.args = vec!["-c".into(), trapped_usr1_command(&ready, "")]; spec.group = true; let id = sup.spawn(spec).expect("spawn"); let pid = spawn_started_pid(&mut sup, id); + wait_for_published(&ready, b"trapped", Duration::from_secs(10)); sup.signal(id, Signal::SIGUSR1).expect("USR1 delivers"); assert!( @@ -3261,6 +3323,57 @@ mod tests { nix::sys::signal::kill(Pid::from_raw(-i32::try_from(pid).unwrap()), Signal::SIGKILL); } + /// R2's discriminating witness — readiness must gate on the **trap**, + /// not on the spawn. + /// + /// `spawn_started_pid` returns when `ProcessEventKind::Started` + /// arrives, which is emitted at spawn; the shell has not parsed + /// `trap '' USR1` yet, and SIGUSR1's default disposition is + /// terminate. This fixture makes that window a deliberate second wide + /// instead of leaving it to a loaded runner, so the weak predicate is + /// *guaranteed* to return inside it — which is what makes this test + /// discriminating rather than lucky. + /// + /// Survival is proved by the child's **exit disposition**, not by an + /// absence observed within a window: a child that took the USR1 + /// reports `Signaled { signal: "SIGUSR1" }`, and one that ignored it + /// reports the SIGTERM sent afterwards. Remove the + /// `wait_for_published` line and this test reports `SIGUSR1`. + #[test] + fn usr1_readiness_waits_for_the_trap_not_for_the_spawn() { + let mut sup = ProcessSupervisor::new(); + let temp = tempfile::TempDir::new().expect("tempdir"); + let ready = temp.path().join("usr1-trapped"); + let mut spec = ProcessSpec::new("diag-trap-readiness", "/bin/sh"); + spec.args = vec!["-c".into(), trapped_usr1_command(&ready, "sleep 1; ")]; + spec.group = true; + let id = sup.spawn(spec).expect("spawn"); + let pid = spawn_started_pid(&mut sup, id); + + // The weak predicate is already satisfied, a second before the + // trap exists. The strong one cannot be. + wait_for_published(&ready, b"trapped", Duration::from_secs(10)); + + sup.signal(id, Signal::SIGUSR1) + .expect("USR1 delivers to a child that has trapped it"); + sup.terminate(id).expect("TERM delivers"); + + let events = drain_until(&mut sup, id, Duration::from_secs(5), has_exited); + let signal = events.iter().find_map(|e| match &e.kind { + ProcessEventKind::Signaled { signal } => Some(signal.clone()), + _ => None, + }); + assert_eq!( + signal.as_deref(), + Some("SIGTERM"), + "a child that trapped USR1 before the signal must die of the \ + TERM instead: {events:?}" + ); + + let _ = + nix::sys::signal::kill(Pid::from_raw(-i32::try_from(pid).unwrap()), Signal::SIGKILL); + } + /// Q#DC4 / acceptance 4 — the measured group is a real observation, /// not a restatement of the input. /// diff --git a/tests/vterm_stage2_acceptance.rs b/tests/vterm_stage2_acceptance.rs index b537d7d..8621d17 100644 --- a/tests/vterm_stage2_acceptance.rs +++ b/tests/vterm_stage2_acceptance.rs @@ -544,7 +544,10 @@ fn terminal_escape_gates_local_bindings_and_double_escape_sends_interrupt() { )) .eval::() .expect("open raw input probe"); - assert_eq!(wait_for_file(&ready_path, Duration::from_secs(5)), b"1"); + assert_eq!( + wait_for_file(&ready_path, b"1", Duration::from_secs(5)), + b"1" + ); let frontend_id = FrontendId::LOCAL; state.dispatch_key( @@ -564,9 +567,10 @@ fn terminal_escape_gates_local_bindings_and_double_escape_sends_interrupt() { KeyEvent::new(KeyCode::Char('w'), KeyModifiers::ALT), ); + let expected_input = b"\x1bv\x03\x1bw"; assert_eq!( - wait_for_file(&input_path, Duration::from_secs(5)), - b"\x1bv\x03\x1bw", + wait_for_file(&input_path, expected_input, Duration::from_secs(5)), + expected_input, "unescaped local bindings reach the child; C-c C-c sends one literal interrupt" ); } @@ -718,42 +722,151 @@ fn wait_for_output( /// The plain helper panics with only the missing path, which is the least /// useful thing to know: a readiness file that never appears is exactly when /// "how far did startup get" decides where to look next. +/// +/// Carries [`wait_for_file`]'s `expected` predicate for the same reason: +/// this helper gates on a file the probe fills with `open(path,'wb')` +/// too, so "readable" would let a zero-byte read through here as well. fn wait_for_published_file( pty: &mut PmacsPty, path: &Path, + expected: &[u8], timeout: Duration, startup: &[(&str, &Path)], ) -> Vec { let deadline = Instant::now() + timeout; loop { - if let Ok(bytes) = fs::read(path) { + if let Some(bytes) = settled_file(path, expected) { return bytes; } assert!( Instant::now() < deadline, - "child never published {} within {timeout:?}\n startup: {}", + "child never published {expected:?} to {} within {timeout:?}\n \ + last read: {:?}\n startup: {}", path.display(), + fs::read(path).ok(), describe_startup(pty, startup) ); thread::sleep(Duration::from_millis(20)); } } -fn wait_for_file(path: &Path, timeout: Duration) -> Vec { +/// Wait until `path` holds bytes the caller can assert against, and +/// return them. +/// +/// **The predicate is the content, not readability.** Every probe in this +/// suite publishes with `open(path,'wb').write(...)`, and `open()` creates +/// the file *before* `write()` fills it — so "`fs::read` succeeded" is +/// satisfied by a **zero-byte file** and this helper used to return `[]` +/// into a caller asserting `== b"1"`. That is `docs/ci-red-signatures.md` +/// R4, whose failure text is `left: []`, `right: [49]`. +/// +/// The wait therefore continues while the file holds a **strict prefix** +/// of `expected` — the states a write in flight can be observed in — and +/// returns as soon as the bytes are anything else. Returning on +/// divergence rather than insisting on a match is deliberate: the +/// caller's `assert_eq!` stays the discriminating assertion, so wrong +/// content is reported as a diff by the test that owns the expectation, +/// not as a timeout in a helper that does not. +fn wait_for_file(path: &Path, expected: &[u8], timeout: Duration) -> Vec { let deadline = Instant::now() + timeout; loop { - if let Ok(bytes) = fs::read(path) { + if let Some(bytes) = settled_file(path, expected) { return bytes; } assert!( Instant::now() < deadline, - "file was not published: {}", - path.display() + "file never settled: {} (expected {expected:?}, last read {:?})", + path.display(), + fs::read(path).ok() ); thread::sleep(Duration::from_millis(20)); } } +/// `Some(bytes)` once `path` is readable and no longer a write in +/// flight; `None` while it is missing or holds a strict prefix of +/// `expected`. +fn settled_file(path: &Path, expected: &[u8]) -> Option> { + let bytes = fs::read(path).ok()?; + let in_flight = bytes.len() < expected.len() && expected.starts_with(&bytes); + (!in_flight).then_some(bytes) +} + +/// R4's discriminating witness — the zero-byte file, reproduced exactly. +/// +/// `docs/ci-red-signatures.md` R4 is `wait_for_file` returning `[]` into a +/// caller asserting `== b"1"`, because `open(path,'wb')` creates the file +/// before `write()` fills it. The file here is created empty and filled +/// later, which makes that window certain instead of load-dependent. +/// +/// Revert the predicate to "`fs::read` succeeded" and this fails with +/// `left: []`, `right: [49]` — R4's two required fragments, verbatim. +#[test] +fn wait_for_file_does_not_return_a_zero_byte_readiness_file() { + let temp = tempfile::TempDir::new().expect("tempdir"); + let path = temp.path().join("ready"); + // The state `open(path,'wb')` leaves behind before its `write`. + drop(fs::File::create(&path).expect("create the empty readiness file")); + + let filled = path.clone(); + let writer = thread::spawn(move || { + thread::sleep(Duration::from_millis(150)); + fs::write(&filled, b"1").expect("fill the readiness file"); + }); + + assert_eq!(wait_for_file(&path, b"1", Duration::from_secs(5)), b"1"); + writer.join().expect("writer thread"); +} + +/// The same predicate, one step past R4: a **partial** write is a torn +/// read, not a readiness signal. +/// +/// R4's occurrence was the zero-byte case because the marker is one byte. +/// The multi-byte callers in this suite (`b"\x1bv\x03\x1bw"`, +/// `b"VTERM_INPUT_SMOKE\n"`) can observe a prefix for the same reason, and +/// a helper that only rejected *empty* would hand one to `assert_eq!`. +#[test] +fn wait_for_file_does_not_return_a_partial_write() { + let expected = b"VTERM_INPUT_SMOKE\n"; + let temp = tempfile::TempDir::new().expect("tempdir"); + let path = temp.path().join("child-input"); + drop(fs::File::create(&path).expect("create the empty input file")); + + let filled = path.clone(); + let writer = thread::spawn(move || { + thread::sleep(Duration::from_millis(100)); + fs::write(&filled, b"VTERM").expect("partial write"); + thread::sleep(Duration::from_millis(100)); + fs::write(&filled, expected).expect("complete write"); + }); + + assert_eq!( + wait_for_file(&path, expected, Duration::from_secs(5)), + expected + ); + writer.join().expect("writer thread"); +} + +/// Content that is *wrong* rather than *unfinished* comes straight back, +/// so the caller's `assert_eq!` reports it. +/// +/// This is what keeps the strengthened predicate from moving the +/// assertion into the helper: waiting for an exact match would turn a +/// child that published the wrong thing into a timeout in a function that +/// does not know what was expected of it, and delete the diff that says +/// what arrived instead. +#[test] +fn wait_for_file_returns_divergent_content_rather_than_timing_out() { + let temp = tempfile::TempDir::new().expect("tempdir"); + let path = temp.path().join("child-size"); + fs::write(&path, b"29 90\n").expect("publish the wrong geometry"); + + assert_eq!( + wait_for_file(&path, b"28 90\n", Duration::from_millis(500)), + b"29 90\n" + ); +} + #[test] #[allow(clippy::too_many_lines, reason = "one real-host lifecycle scenario")] fn real_tui_terminal_smoke_restores_host_after_output_input_resize_scroll_copy_and_bell() { @@ -888,7 +1001,13 @@ fn real_tui_terminal_smoke_restores_host_after_output_input_resize_scroll_copy_a // are protocol escapes pmacs writes directly, never painted cells, so the // differ cannot split them. assert_eq!( - wait_for_published_file(&mut pty, &alt_ready_path, Duration::from_secs(10), startup), + wait_for_published_file( + &mut pty, + &alt_ready_path, + b"1", + Duration::from_secs(10), + startup + ), b"1", "alt-screen readiness breadcrumb was published but malformed" ); @@ -904,12 +1023,19 @@ fn real_tui_terminal_smoke_restores_host_after_output_input_resize_scroll_copy_a pty.write_input(b"\x03\x1bw") .expect("escaped copy selection binding"); wait_for_output(&mut pty, b"\x1b]52;c;", Duration::from_secs(5), startup); - let input = wait_for_file(&input_path, Duration::from_secs(5)); - assert_eq!(input, b"VTERM_INPUT_SMOKE\n"); + let expected_input = b"VTERM_INPUT_SMOKE\n"; + let input = wait_for_file(&input_path, expected_input, Duration::from_secs(5)); + assert_eq!(input, expected_input); pty.write_input(b"\x1b[200~PASTE_AFTER_EXIT\x1b[201~") .expect("route a real host paste event"); - let size = String::from_utf8(wait_for_file(&size_path, Duration::from_secs(5))) - .expect("UTF-8 stty size"); + // The probe writes `f'{lines} {columns}\n'`, so the settled content + // carries the trailing newline the assertion below trims. + let size = String::from_utf8(wait_for_file( + &size_path, + b"28 90\n", + Duration::from_secs(5), + )) + .expect("UTF-8 stty size"); assert_eq!(size.trim(), "28 90", "child PTY must receive cell geometry"); thread::sleep(Duration::from_millis(200)); pty.write_input(b"\x03\x18\x03").expect("quit pmacs");