Skip to content

test: close three pre-existing CI intermittencies - #139

Merged
Broccolito merged 3 commits into
mainfrom
claude/ci-child-process-wait-deadlines
Aug 27, 2026
Merged

test: close three pre-existing CI intermittencies#139
Broccolito merged 3 commits into
mainfrom
claude/ci-child-process-wait-deadlines

Conversation

@Broccolito

Copy link
Copy Markdown
Collaborator

Three test-only fixes for failures that are pre-existing on main and unrelated to any feature branch. All three were found while trying to get a reliably green matrix for the next release.

1. dropping_an_unread_stream_reaps_the_child — 4 failures in 20 full lib-suite runs

"the child should have started and written its pid"

The child is #!/bin/sh doing echo $$ > pidfile, so the work is trivial — what ran out was the wait. Three loops polled 100 × 50 ms, a hard five second ceiling on a process spawn: claude_code.rs (the pid wait and wait_for_exit) and once in codex.rs.

Five seconds is not a spawn budget on a CI runner — test (macos-latest) compiles the whole workspace on three cores. And it is self-perpetuating: the Rust cache is written only when a job goes green, so one red job keeps the next run cold, which makes the next timeout more likely.

Now a named CHILD_WAIT_TICKS = 1_200 (60 s). Raising it costs nothing when the child behaves — every loop exits the moment it sees what it waits for, so the ceiling is only paid by a genuine failure.

Measured after: 5 consecutive full lib-suite runs, 3138 passed / 0 failed, at load averages 73–90 — higher than where the 4/20 was observed — plus 12/12 in isolation.

2. global_memory store-path tests — 6 failures in 20 runs

developer__shell {"command":"rm -rf …/config/memory"}
  points at the machine-wide store and must be refused

That reads as a hole in the #63 consent gate. It is a harness race, and the disguise is why it is worth fixing rather than tolerating — whoever meets it cold during a release will open a privacy incident.

Every test here resolves the store twice: once via store_path_spellings() to build the expected path, once inside global_memory_gate() at assert time. Both go through config_dir(), which re-reads BIOROUTER_PATH_ROOT. Other tests legitimately point that variable at their own temp root and restore it — logging.rs, managed/mod.rs, providers/utils.rs, session/diagnostics.rs all do, correctly, under env_lock. This module took no lock, so one of them could land between the two resolutions.

The writers were never the problem, and adding a lock to them would not help: env_lock serialises only tasks that ask for it, and the reader never did. pinned_store_root() takes the lock and pins the variable to its current value — the point is to hold it still, not to change the root. Applied to all four tests that double-resolve, two more than the two observed failing.

3. skill_package_live version pin — fails against upstream

left:  Some("0.8.16")     right: Some("0.8.12")

HyperFrames shipped 0.8.16. Worse than a stale pin: the assert sits before every package-identity assertion the test exists for, so a real change to bundle handling in plan.rs went unvalidated while the failure pointed elsewhere.

A literal there asserts upstream's release cadence, not our importer. It now reads .codex-plugin/plugin.json — the manifest the evidence ladder actually resolved, which keeps Evidence::CodexPlugin honest — exactly as the test already does for the component list ("read here rather than hard-coded"). The read is hoisted above plan_from_entries, which takes fetched.entries by value.

The eight other 0.8.12 pins are synthetic fixtures asserting their own fixture and are correct; only the live test was asserting a third party.

Verified against the real repository: 2 passed, 0 failed.

Not fixed here

Two further intermittencies are known and owned elsewhere: workspace_extension::panel_secret_guard_rejects_top_level_and_wrapped_locators (a shared-fake answer theft) and subagent_handler::cancel_turn_on_the_child_stops_the_run_and_spares_the_parent (a foreign session id reaching a shared spy, confirmed foreign by running that module in isolation — 6 runs, no leak).

Three of the six families share one root cause: a reader of process-global state that never asks for the guard the writers correctly hold.

`hyperframes_main_imports_as_one_package_with_every_declared_skill`
pinned `plan.version` to the literal "0.8.12". HyperFrames has shipped
0.8.16, so the test now fails -- and it fails at that assert, which sits
*before* every package-identity assertion the test exists for. A real
change to the bundle handling in plan.rs went completely unvalidated
while the failure pointed at something else entirely.

A literal there asserts upstream's RELEASE CADENCE rather than our
importer. What is worth asserting is that the plan carries through
whatever version the plugin manifest declares, which is exactly the rule
the test already states about itself eight lines earlier, for the
component list:

    // What the repository itself says its components are -- read here
    // rather than hard-coded, so the assertion survives the package
    // gaining a skill.

The version assert was the one place that broke that rule. It now reads
`.codex-plugin/plugin.json` -- the manifest the ladder actually resolved,
which keeps `Evidence::CodexPlugin` honest -- and compares against that.

The read is hoisted above `plan_from_entries`, which takes
`fetched.entries` BY VALUE, and the version is owned rather than
borrowed; asserting after the move does not compile.

The eight other "0.8.12" pins are synthetic fixtures asserting their own
fixture, which is correct, and are left alone. Only the live test was
asserting a third party.

Verified against the real repository: 2 passed, 0 failed.
`dropping_an_unread_stream_reaps_the_child` failed 4 times in 20 full
lib-suite runs on a developer machine under load, with:

    "the child should have started and written its pid"

The child is `#!/bin/sh` doing `echo $$ > pidfile`, so the work is
trivial; what ran out was the wait. Three loops polled 100 times at 50 ms
-- a hard five second ceiling on a process spawn -- in
`claude_code.rs` (the pid wait and `wait_for_exit`) and once in
`codex.rs`.

Five seconds is not a spawn budget on a CI runner. `test (macos-latest)`
compiles the whole workspace on three cores, and the failure reads as a
defect in child reaping rather than as the scheduler not having got round
to the child yet. It is also self-perpetuating: the Rust cache is written
only when a job goes green, so one such red job keeps the next run cold,
which makes the next timeout more likely.

They become a named `CHILD_WAIT_TICKS = 1_200` (60 s). Raising it costs
nothing when the child behaves -- every one of these loops exits the
moment it sees what it is waiting for, so the ceiling is only ever paid
by a genuine failure.

Measured after the change: 5 consecutive full lib-suite runs, 3138
passed / 0 failed, at load averages 73-90 -- higher than where the 4/20
was observed -- plus 12/12 for the test in isolation.
…hecked

`a_tool_that_names_the_store_path_is_refused` and
`a_path_argument_naming_the_store_is_still_refused_after_the_narrowing`
failed 6 times in 20 full lib-suite runs, reporting:

    developer__shell {"command":"rm -rf …/config/memory"}
      points at the machine-wide store and must be refused

That reads as a hole in the #63 consent gate. It is a harness race, and
the disguise is the reason it is worth fixing rather than tolerating --
whoever meets it cold during a release will open a privacy incident.

Every test here resolves the store TWICE: once via `store_path_spellings`
to build the path it expects to see refused, and again inside
`global_memory_gate` when the assertion runs. Both go through
`biorouter_mcp::global_memory_dir()` -> `config_dir()`, which re-reads
`BIOROUTER_PATH_ROOT` each time. Other tests in this binary legitimately
point that variable at their own temporary root and put it back --
`logging.rs`, `managed/mod.rs`, `providers/utils.rs` and
`session/diagnostics.rs` all do, correctly, under `env_lock`. This module
took no lock at all, so one of them could land between the two
resolutions: the expected path was built against one root and the gate
matched against another, and no refusal fired.

The writers were never the problem, and adding a lock to them would not
have helped: `env_lock` serialises only the tasks that ASK for it, and
the reader here never did. `pinned_store_root()` takes the lock and pins
the variable to its CURRENT value -- the point is to hold it still, not
to change the root.

Applied to all four tests that resolve twice, which is two more than the
two observed failing; the other two are the same shape and were waiting
their turn.
@Broccolito
Broccolito merged commit 6d11dac into main Aug 27, 2026
15 of 16 checks passed
@Broccolito
Broccolito deleted the claude/ci-child-process-wait-deadlines branch August 27, 2026 05:18
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