fix(tests): synchronize load-sensitive Darwin tests on the event they measure, add Darwin CI - #358
Merged
Merged
Conversation
…ure (#356) Three tests spent their behavioral deadline on fixture scheduling, so a loaded aarch64-darwin host failed them while asserting nothing about the behavior: - `input_write_failure_terminates_and_reaps_the_child` gave EPIPE a 1s budget that also had to cover fork+exec of the fixture shell, and reported `timed out after 1.0s` instead. - `the_group_kill_reaps_a_descendant_that_outlives_the_direct_child` read the descendant pidfile after a 2s deadline that the child had not yet met. - `deferred_delivery_backoff_bounds_short_lived_pty_attempts` stopped the DING loop after a fixed 250ms window that ended before its first poll. All three now synchronize on the lifecycle event first, then apply a bounded assertion. `run_captured` already starts the child deadline after `on_spawn` returns, so `on_spawn` is the readiness barrier: `await_fixture_ready` blocks there until the fixture published an atomically renamed marker, and fork+exec is paid outside the deadline rather than out of it. The DING test stops on the loop's own poll counter instead of wall clock. Production cleanup semantics are unchanged; both call sites pass a no-op observer. That ordering is load-bearing and was proven by nothing, so `the_spawn_observer_runs_before_the_child_deadline_starts` pins it: its barrier outlasts the timeout by construction, so only the order decides the outcome. `input_write_obeys_the_child_deadline` had the same latent defect — it timed from before the call, charging fork+exec to the 1s budget it polices — and now times from the observed spawn instant. It cannot use a barrier: it exists to cover the case where the child is never scheduled at all. Counter-tests verified by mutation: dropping `kill(-pid, SIGKILL)`, dropping the `terminate_and_reap_before` on the stdin-failure path, reordering the deadline before `on_spawn`, and dropping the DING backoff each fail their exact test. Adds a hosted `macos-15` job running `nix build .#st2`, which is the surface #356 lives on. st2 drives process groups, signals, pipes and reaping — OS behavior no Linux run can gate — and the 3-core hosted runner is a stricter scheduler than any Mac in the fleet. Closes #356 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.237 agent-runtime: Claude Code 2.1.237 tooling-profile: dotfiles@11eaf2d-dirty
`check` and `build-darwin` described different things — one the verb, one the platform — so neither name said which system it gates. `check-x86_64-linux` and `check-aarch64-darwin` use the flake's own system identifiers, and the size difference between them moves into the comment where it can be explained. No required-status-check rule references the old name, so the rename is inert for branch protection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.237 agent-runtime: Claude Code 2.1.237 tooling-profile: dotfiles@11eaf2d-dirty
`startup_backlog_gets_one_generic_recovery_then_new_arrivals_poke` waits for two events — the loop's first poll, then two delivered pokes — but shared one 3s deadline across both. Under sustained load the first wait spent it, so the test stopped the loop after a single poke and failed on the count while naming neither barrier. Each wait now has its own budget, and the ceiling is generous: it bounds a loop that never ran at all, which is not the behaviour under test. The barriers report rather than panic, because an unwind from inside `thread::scope` would leave `run_ding` without the `stop` flag that ends it, turning a failed assertion into a hang. Found by the loaded aarch64-darwin rebuild added in this branch — the fourth instance of the same defect, and the reason the new Darwin job is worth having. Counter-test: dropping the `new_arrivals` queueing so post-start messages never poke fails it with `the ding loop never delivered both notices`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> agent-identity: unknown agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.237 agent-runtime: Claude Code 2.1.237 tooling-profile: dotfiles@11eaf2d-dirty
schickling
marked this pull request as ready for review
August 27, 2026 11:50
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c20ae4327
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Four st2 tests spend their behavioral deadline on fixture scheduling. On a loaded
aarch64-darwinhost the deadline expires before the fixture reaches the state undertest, so the test fails while asserting nothing about the behavior it names. They pass
in isolation, which is why this reads as flakiness rather than as a defect in the tests.
run::tests::input_write_failure_terminates_and_reaps_the_childfork+execof the fixture shell, thenexec 0<&-unexpected write error: timed out after 1.0srun::tests::the_group_kill_reaps_a_descendant_that_outlives_the_direct_childthe child never forked a descendant, so this case tested nothing: NotFoundding::tests::deferred_delivery_backoff_bounds_short_lived_pty_attemptsleft: 0, right: 1ding::tests::startup_backlog_gets_one_generic_recovery_then_new_arrivals_pokeleft: 1, right: 2fork+execof/bin/shmeasured 0.554s on an idle 10-core M1 Max — over half ofthe first test's entire budget before the behavior under test has begun.
The first two are #356. The third is #255, open since 2026-08-18. The fourth was found by
the loaded Darwin rebuild added in this branch — it had never been reported, which is the
argument for the CI job in one line. #255 already named the cause of its own invisibility: "CI is
ubuntu-latestonly, so every green check on thisrepository proves Linux and says nothing about Darwin." This PR closes that gap too.
Goal
Each test synchronizes on the lifecycle event it intends to measure, then applies a
bounded assertion — and a hosted Darwin job gates the surface these failures live on, so
the next one is caught by CI rather than by someone running the suite locally.
Decisions
The barrier goes in
on_spawn, becauserun_capturedalready starts the child deadlineafter
on_spawnreturns. A test blocks there until its fixture published a marker, sofork+execis paid outside the deadline instead of out of it. No productionsemantics change: both real call sites pass a no-op observer, and the seam already existed.
That ordering was load-bearing and proven by nothing — a future harmless-looking reorder
would put every barrier test back on a race with the scheduler while leaving the suite
green.
the_spawn_observer_runs_before_the_child_deadline_startsnow pins it. Its barrieroutlasts the timeout by construction, so the outcome depends on the order alone and on
nothing the host's scheduler does.
Markers are published by atomic rename, not polled until parseable. A shell redirection
truncates the target before the bytes land, so a barrier on bare existence can read an empty
file.
mvfrom a sibling temp file makes the marker atomic. #318 solved the same shape byretrying the read until it parsed; the rename removes the retry rather than bounding it,
and a genuinely bad pid stays a parse error instead of becoming a confusing timeout.
The DING test stops on the loop's own poll counter.
session_aliveruns exactly onceper iteration, so
probescounts polls directly and load-independently. The test nowstops once the first poke landed and the loop polled five more times — precisely when a
missing backoff would poke again — instead of wherever a 250ms window happened to land. An
added assertion that the loop really did poll again keeps the counter-test non-vacuous:
one poke across one poll would otherwise prove nothing about backoff.
A barrier ceiling is not a deadline.
await_fixture_readyand its DING counterpart wait30s. That number bounds a fixture that never ran at all — it is not the behaviour under
test, and waiting longer costs nothing when the fixture does run.
startup_backlog_…hadthe right barriers already but gave both of them one shared 3s budget, so the first could
spend it; each now has its own. Those barriers report rather than panic, because an unwind
from inside
thread::scopewould leaverun_dingwithout thestopflag that ends it,turning a failed assertion into a hang.
Rejected: widening the deadlines. It buys margin proportional to nothing, leaves the
tests measuring scheduling plus behavior, and the existing in-tree comments already
identify widening as the wrong answer. The barrier removes the dependency instead.
Darwin CI scope is
nix build .#st2, notnix flake check. That builds the packageand runs its hermetic suite via
doCheck— exactly the surface these failures live on —without also demanding cold
aarch64-darwinbuilds of theptyandoteliteinputs thatchecks.parked-recoveryandchecks.otel-exportpull in. Widening once it is stable is afollow-up, not a prerequisite. Hosted
macos-15rather than a self-hosted Mac: the repo ispublic so hosted arm64 runners are free, and the fleet's self-hosted runners are currently
down.
Verification
Root cause proven by shrinking the deadline — if the budget is what is being consumed,
shrinking it must reproduce both failures deterministically on an idle Linux box. It does:
Identical to the messages the loaded Darwin host produces.
Reproduced on Apple Silicon, first attempt, packaged suite at
19485cf:Counter-tests confirmed by mutation. Each mutation was applied to the fixed tree and
must fail its exact test — the whole reason these tests exist:
kill(-pid, SIGKILL)fromterminate_and_reap_beforethe_group_kill_reaps_a_descendant_…terminate_and_reap_beforeon the stdin-failure pathinput_write_failure_terminates_…on_spawnthe_spawn_observer_runs_before_…the child deadline started before on_spawn returned: timed out after 0.2sNonedeferred_delivery_backoff_bounds_…left: 6, right: 1startup_backlog_gets_one_generic_recovery_…the ding loop never delivered both noticesThe ordering guard caught a real gap while being written: its first version asserted on a
child that exits, and
run_capturedcheckstry_waitbefore the deadline, so it passedunder the reorder. Rewriting it around the stdin path makes it non-vacuous — the FAILED
row above is the rewritten version.
Linux, full lib+bin suite:
482 passed; 0 failed(was 481 — one test added).CI, on the final commit — both platforms green:
check-x86_64-linuxnix flake checkcheck-aarch64-darwinnix build .#st2nix build .#st2runs the suite viadoCheck, so a green Darwin build is every gated testpassing on a 3-core runner that is a stricter scheduler than any Mac in the fleet.
Repeated rebuilds on Apple Silicon under sustained load — 8 spinners on 10 cores plus a
known
fseventsddefect holding a further core, load average 21–38. A single green runproves little about a load-sensitive failure, so this was run repeatedly:
19485cf(no fix), load 3.5461 passed; 3 failed— the repro, first attempt465 passed; 0 failed464 passed; 1 failed— surfaced the fourth defect465 passed; 0 failed465 passed; 0 failedEach row is a build that actually re-ran the suite. Grading is on the
test result:lines,not on the exit status:
nix build --rebuildalso compares output bytes, and st2's Darwinbuild is not bit-reproducible, so a run whose tests all pass can still exit non-zero with
may not be deterministic: output ... differs. Conversely a plainnix buildis a cachehit once the output is valid and runs nothing at all. Both traps produced misleading rows
during this work before the loop was corrected.
Linux, full lib+bin suite:
482 passed; 0 failed.cargo fmt/clippyintroduce no new findings in the touched files. Pre-existing drift insrc/lib.rs,src/main.rs,src/message.rs,src/metrics.rs,src/omp_session.rs,src/service.rs,tests/otel_export.rsand two untouched spots insrc/run.rsis leftalone — the flake deliberately does not gate either.
Complexity
Two test helpers (
await_fixture_readyandawait_ding_progress, ~15 lines each) and oneguard test. No production code paths added or changed; the two doc comments on
run_capturedstate an ordering the code already had.Concerns
on_spawnblocking is notobvious from its signature. The guard test is the mitigation; a type that made the
barrier explicit would be more machinery than the two call sites justify.
are metered differently from Linux.
nix build .#st2rather thannix flake checkkeepsthat bounded, and
concurrency(added here) stops superseded pushes from stacking runs.the fourth test above was found by the loaded rebuild, not by a report. That is the job
working, but it means the first weeks may cost some triage.
codex_app_server::tests::process_group_cleanup_reaps_a_native_launcher_descendantfailedon my highest-load Darwin rebuild with
the launcher did not create its native descendant: NotFound. fix(codex-app-server): deflake darwin-parallel test races #318 addresses it by raising that deadline from 1s to 5s and polling until thepid parses — a wider window rather than a barrier on the event — so it may still go red on
a 3-core runner. Worth watching once both land; not a reason to hold either.
Friction & bottlenecks
only reproduces inside the packaged suite. Shrinking the deadline locally on Linux gave
the same evidence in seconds; that trick is worth reaching for first on any
"load-sensitive test" report.
no good primitive.
--rebuildrefuses when the output is not already valid — includingafter a failed run — and its exit status also folds in a bit-reproducibility comparison
that st2's Darwin build does not satisfy. Plain
nix buildis a cache hit that runs notests. Three separate verification loops produced meaningless rows before I stopped
trusting the exit status and graded on
test result:lines instead.aarch64-darwinbuild is not bit-reproducible (output ... differson rebuild).Pre-existing and unrelated to this PR, but it is why
--rebuildcannot be used as apass/fail signal here.
pkill -f verify.sh, a patternkill on a host others use. It matched only my own processes, but killing the recorded PID
was the correct move and I should have done that.
Follow-ups
nix flake checkonceptyandotelitebuild reliably foraarch64-darwinin CI.patch_presentationandlist_entriesgive theptychild a 2s budget thatlikewise spans
fork+exec. On a loaded Darwin host that can time out spuriously; theeffect is a logged error and a retry on the next reconcile pass, not a crash, so it is not
addressed here. Worth a separate look — flagged rather than folded in.
src/codex_app_server.rs. No file overlap withthis PR, and both can land independently.
References
Posted on behalf of @schickling
agent_identityagent_personaagent_supervisoragent_toolagent_tool_versionagent_runtimetooling_profile