Skip to content

test: a pipe end leaked into a sweep no longer fails the concurrent-sweeps capacity check on macOS - #208

Merged
vjovanov merged 2 commits into
mainfrom
fix/issue-203
Oct 8, 2026
Merged

vjovanov merged 2 commits into
mainfrom
fix/issue-203

Conversation

@vjovanov

@vjovanov vjovanov commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #203

review_repro_concurrent_sweeps_share_the_global_ceiling failed intermittently on macOS at its last check, the one that reads whether the started root's run lock is still held. The defect was in the test's fake runner, not in ephor. The lock holder that the fake rhei leaves behind kept a pipe end it had inherited by accident. This pull request changes only tests.

Before this change, the race can be forced on any platform by handing each sweep a write end of its own stdout at fd 9, and the new test does exactly that. At cba2f56d5e, where the test exists but the fix does not, the test sits out the lock holder's 20s sleep and then fails with the panic CI reported (at :195 on main; the assertion is now at :235):

$ cargo test --locked --test work_capacity_test concurrent_sweeps_share_the_global_ceiling
test review_repro_concurrent_sweeps_share_the_global_ceiling ... ok
test concurrent_sweeps_share_the_global_ceiling_while_holding_a_leaked_pipe ... FAILED

thread 'concurrent_sweeps_share_the_global_ceiling_while_holding_a_leaked_pipe' (3611632) panicked at tests/integration/work_capacity_test.rs:235:5:
assertion `left == right` failed: global cap=1 must leave at most one live root
  left: 0
 right: 1

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 13 filtered out; finished in 20.48s

With this change, the holder closes the leaked pipe end before it takes the lock, and both tests pass in half a second:

$ cargo test --locked --test work_capacity_test concurrent_sweeps_share_the_global_ceiling
test concurrent_sweeps_share_the_global_ceiling_while_holding_a_leaked_pipe ... ok
test review_repro_concurrent_sweeps_share_the_global_ceiling ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 13 filtered out; finished in 0.50s

Why it failed

The cause was not a slow runner, which is what the issue guessed. The CI log rules that out. In the failing job, the other 13 tests in the binary kept the green run's pace and were all done 6.7s in. This test alone finished at 25.54s, about 20s late, and 20s is exactly the lock holder's time.sleep(20). The test was waiting for the holder to exit.

On macOS, Rust std has no pipe2. Command::output() makes each pipe with pipe() and only then marks it FD_CLOEXEC, in two separate steps. The test starts its two work run --due sweeps from two threads at once, so one sweep can be spawned between those two steps of the other and inherit a write end of a pipe the test reads. That descriptor then passes from the sweep's ephor to sh -c, then to the bash fake rhei, then to the backgrounded python … time.sleep(20) that holds .rhei/run.lock. The holder redirected fds 0-2 and nothing else, so it kept the write end for its whole 20s. output() reached EOF only after the holder had exited and dropped its flock, and the live check read 0.

The behaviour the test pins holds. Filtered sweeps in separate processes share the global ceiling, so exactly one starts a root and the other is passed over (§FS-005-dispatch.24). In each of triage's 21 control runs, the started root's lock was still held after both sweeps returned. Nothing in ephor ends the holder early.

What changed

  • One lock holder for all three fake runners. The holder had three copies: capacity_runner and overlapping_capacity_runner in tests/integration/work_capacity_test.rs, and holding_runner in tests/integration/work_checkout_test.rs. All three had the same hazard. They are now one RUN_LOCK_HOLDER const in tests/integration/common/mod.rs, and each fixture's format! interpolates it as {holder}.
  • The holder closes every descriptor above stderr before it locks. It lists /dev/fd, closes each descriptor numbered above 2, and only then opens and flocks the lock file. Its stdout and stderr still go to /dev/null. Its stdin stays the /dev/null that bash gives a background job. Its time.sleep(20) is unchanged. A pipe end leaked into a sweep now goes away with the launcher, so output() returns at once and the check sees the lock still held.
  • /dev/fd rather than a fixed range. macOS has no close_range, so os.closerange(3, 65536) would cost about 65k close() calls per holder there. /dev/fd lists exactly what is open on Linux and macOS, and it is where CPython's own subprocess looks on macOS. Each listed descriptor is closed through os.closerange(fd, fd + 1). That call ignores the one entry already closed by then, the descriptor listdir read the directory through.
  • A regression test that does not need to win the race. concurrent_sweeps_share_the_global_ceiling_while_holding_a_leaked_pipe is new. It starts each sweep through /bin/sh -c 'exec 9>&1; exec "$0" "$@"' (holding_its_stdout), with the same program, arguments and environment. So on every platform, each sweep holds a write end of its own stdout at fd 9, which is the state the macOS race leaves a sweep in. There is no wall-clock assertion. The live check is the pin.
  • The original test keeps its name and its behaviour. review_repro_concurrent_sweeps_share_the_global_ceiling is now a one-line call into the shared body concurrent_sweeps_share_the_global_ceiling(leak: bool) with leak = false, so the fixture setup is not duplicated.

The specification. No spec point is written or moved. Both tests cite §FS-005-dispatch.24, and nothing under src/ changed. There is no changelog entry, on purpose. Section 1.1 of docs/changelog.md says no change edits that file, because the release takes each pull request's title as its line (§FS-002-release.1.3).

Found and deliberately not fixed

  • Product-side close-on-exec (triage's remedy C). ephor could mark fds 3 and above close-on-exec before it launches a runner. Triage showed that this also fixes the test (variant C, a pre_exec in captured()). It was not taken, because it would change what a runner launched by the shipped tool inherits, and this ticket does not need that.
  • The same std race inside ephor, unverified. ephor itself spawns children from concurrent threads, for example in the std::thread::scope of feed refresh (src/feed/refresh.rs:183). On macOS the same race could leak one child's pipe into another, and a long-lived descendant could then hold a capture open until its timeout. Nobody has observed this, and it is left for a person to file.
  • Review finding R1-01, deferred. The doc comments on RUN_LOCK_HOLDER and on the fixture runners say that the holder closes "every other descriptor it inherited". In fact it closes every descriptor above stderr and keeps stdin. The finding is a nit, and the comments were left unchanged.

How it was verified

The branch is test first. cba2f56d5e holds only the new test, its helper and the shared body, and 64ce551967 holds the fix.

  • CI on cba2f56d5e, before the fix (run 37743882941). The new test failed at work_capacity_test.rs:235:5 with left: 0 on both macos-latest and ubuntu-latest. Each time it was the only failure in its binary (14 passed; 1 failed). So the test catches the defect on macOS as well as on Linux.
  • CI on 64ce551967, with the fix (run 37745221301). cargo test (ubuntu-latest) passed. The macos-latest job failed earlier, in its python tests step, on the known flake test_prepare_changelog_release CompletenessTests fail intermittently on macOS: release_forge.py:269 git 'unable to create temporary file: Invalid argument' #202 (release_forge.py:269, git add -A failed: error: unable to create temporary file), so its cargo test step was skipped. The fixed tree has not yet been seen passing cargo test on macOS. The ship step reruns CI and collects that result.
  • Triage's reproducer forces the leak and watches the holder's descriptors. It exited 1 at 798f3077c1 and at cba2f56d5e. At 64ce551967 it exits 0 (DEFECT GONE), with the forced run passing in 0.54s.
  • The local gate on 64ce551967, on Linux. Five commands and the reproducer ran, 4m01s in all, and all exited 0:
    • pre-commit run --all-files: grund check and grund fmt with CI's pinned grund 0.14.0, fissile, the private-words hook and the attribution hook;
    • the Python integration suite, 101 tests, OK;
    • check_boundary.py;
    • check_parity.py: 25 abilities, 26 presentation keys, 54 bindings accounted for, and every command has --json;
    • the full cargo test, with TMPDIR on a symlinked directory.
  • The fix's own runs, also on Linux. cargo test --all-targets --locked passed 1821 tests with 0 failures, both plain and with TMPDIR on a symlinked directory. work_capacity_test passed all 15 tests in 1.13s, where at cba2f56d5e it took 20.53s and failed the new test. work_checkout_test passed all 12 tests in 0.81s.
AI workflow: `rhei`, 11 agent invocations across 1 model; 6 tasks completed, 10 in progress.
  1. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket supervising (visit 1) — cld, anthropic/claude-opus-5-5 — 1m53s — 844.5k in / 11.1k out
  2. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket.triage assessing (visit 1) — cld, anthropic/claude-opus-5-5 — 14m23s — 7.3M in / 70.5k out
  3. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket.triage reproducing (visit 1) — cld, anthropic/claude-opus-5-5 — 8m12s — 3.0M in / 37.6k out
  4. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket supervising (visit 2) — cld, anthropic/claude-opus-5-5 — 3m01s — 878.4k in / 16.0k out
  5. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket.specify specify — cld, anthropic/claude-opus-5-5 — 6m02s — 3.1M in / 25.7k out
  6. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket supervising (visit 3) — cld, anthropic/claude-opus-5-5 — 1m24s — 702.6k in / 8.4k out
  7. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket.implement implement — cld, anthropic/claude-opus-5-5 — 9m40s — 2.1M in / 23.7k out
  8. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket supervising (visit 4) — cld, anthropic/claude-opus-5-5 — 1m29s — 732.9k in / 8.0k out
  9. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket.review-1 review — cld, anthropic/claude-opus-5-5 — 10m42s — 5.6M in / 36.7k out
  10. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket supervising (visit 5) — cld, anthropic/claude-opus-5-5 — 1m10s — 753.6k in / 5.8k out
  11. github-issues-agent-grounds-ephor-203-implement-2ece1d3c.ticket supervising (visit 6) — cld, anthropic/claude-opus-5-5 — 1m32s — 1.0M in / 8.9k out
Accounting Value
cost $15.56
total tokens 26.3M
input tokens (incl. cache) 26.1M
input cache read 25.0M
input cache write 1.1M
output tokens (incl. cache) 252.5k
output cache read -
output cache write -
coverage Complete

@vjovanov vjovanov changed the title test: the capacity check holds while each sweep carries a leaked pipe end test: a pipe end leaked into a sweep no longer fails the concurrent-sweeps capacity check on macOS Oct 8, 2026
… end

On macOS std makes a pipe with pipe() and only then sets FD_CLOEXEC, so a
sweep spawned while another test thread spawns can inherit a write end of
a pipe the test reads. The fake runner's backgrounded lock holder
redirects only fds 0-2 and keeps that end, so output() returns only once
the holder has exited and released its lock, and the live-root check
reads 0.

The new case runs the concurrent-sweeps scenario with each sweep started
through sh, holding a write end of its own stdout at fd 9, which puts
every platform in that state on purpose. It fails today at the live-root
check, after the holder's 20 seconds. The existing case keeps its plain
reading of the shared ceiling.
Each fake detached runner left a backgrounded python holding the root's
run lock for 20s, with stdout and stderr redirected but every other
inherited descriptor kept. A sweep that carried a leaked pipe end - which
macOS std leaves when another thread spawns between pipe() and the
close-on-exec mark - handed it down to that holder, so the test's
output() waited for the holder to exit and then read the lock as
released (#203).

The three holders become one shared RUN_LOCK_HOLDER that closes every
descriptor above stderr, listed from /dev/fd, before it takes the lock.
@vjovanov
vjovanov marked this pull request as ready for review October 8, 2026 08:05
@vjovanov
vjovanov merged commit c3697d5 into main Oct 8, 2026
5 checks passed
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.

review_repro_concurrent_sweeps_share_the_global_ceiling fails intermittently on macOS: work_capacity_test.rs:195

1 participant