Fix flaky pty test: openpty(3) is not thread-safe on macOS - #894
Merged
Conversation
io::pty_tests::pty_child_gets_default_sigpipe failed intermittently under `cargo test --workspace` but passed alone. The panic is at the spawn_with_config().expect() line, not the read-with-timeout below it: the 5s READ_TIMEOUT is never reached and SIGPIPE is not implicated. openpty() itself fails before a child is ever spawned. Measured with a standalone C harness on a 12-core machine: 48 threads calling openpty in a loop failed 107 times in 57,600 calls; the same calls serialized failed 0 times in 57,600. At 8 threads it does not reproduce, which is why the flake only appeared under full-workspace thread oversubscription. The failing calls return errno -6 -- a negative, invalid errno. nix has no arm for it and falls through to `_ => UnknownErrno`, which is the signature seen in the wild. That also rules out the plausible alternatives: pty exhaustion fails cleanly with a valid ENXIO (cap is 511, 5 were in use), fd limits are over a million, and the failure precedes posix_spawn so the signal-attribute path is not involved. Which internal call races is unconfirmed; ptsname(3) is the suspect but a corrupted path would yield a valid errno, so the fix is justified by the measurement, not the mechanism. No production lock. Production opens exactly one PTY per run -- main spawns it on the main thread before the troupe's threads exist and hands it to PtyTroupe::new, which delivers it to the actors as a Bind message -- so spawn_with_config now documents that single-threaded contract instead. The contract is upheld inside the test binary by a #[cfg(test)] OPENPTY_TEST_LOCK, gated inside spawn_with_config rather than sprinkled across the tests: spawn sites live in four different test modules, and a lock a new test can forget to take would let the flake back in silently. No integration test spawns a PTY, which makes cfg(test) a sufficient boundary. Poisoning is expect()-ed so it fails loudly. Deliberately not done: widening the timeout or retrying the spawn, either of which would mask a real libc thread-safety bug behind a timing knob. Verified: 7 full `cargo test --workspace` runs with zero failures across all 74 test-result lines each; targeted io::pty_tests passes; non-test `cargo build --workspace` is clean with the guard compiled out entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🤖 Hi @jppittman, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
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 freeto 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.
What
io::pty_tests::pty_child_gets_default_sigpipefailed intermittently undercargo test --workspacewhile passing reliably when run alone. Root cause:openpty(3)is not thread-safe on macOS. It is not a timeout and not a SIGPIPE bug.The tell is where it panics — pty_tests.rs:230 is the
spawn_with_config(...).expect(...)line, not the read-with-timeout on 231. The 5-secondREAD_TIMEOUTis never reached and the SIGPIPE behaviour the test asserts is never exercised:openptyfails before a child is ever spawned. The test was a canary, not the culprit.Evidence
Standalone C harness,
openptyin a loop across N threads on a 12-core machine:It only reproduces once threads oversubscribe the cores — exactly what
cargo test --workspacedoes with many test binaries running concurrently, and why it never showed up under-p core-term.The failing calls return errno -6, a negative and therefore invalid errno.
nixhas no arm for it and falls through to_ => UnknownErrno, which is the signature observed in the wild. That detail also rules out the alternatives:kern.tty.ptmx_maxis 511, exhausting it fails cleanly with a validENXIO, and only 5 were in use.ulimit -nis 1048576.posix_spawn.posix_spawnsignal-attribute race — the failure precedesposix_spawnentirely.Which internal call races is unconfirmed.
ptsname(3)is the obvious suspect (man ptsname: not guaranteed reentrant or thread safe), but a corrupted replica path would makeopen()fail with a valid errno, not -6. The fix is justified by the measurement, not the mechanism — the doc says so explicitly rather than asserting a tidy story.The fix — no production lock
Production opens exactly one PTY per run:
mainspawns it on the main thread before the troupe's threads exist, then hands it toPtyTroupe::new, which delivers it to the actors as aBindmessage. Every other call site is inside#[cfg(test)], and there is no multi-window/multi-PTY design. So there is no live production race, andspawn_with_confignow documents that single-threaded contract instead of carrying a lock for a race that cannot occur.The contract is upheld in the test binary by a
#[cfg(test)] OPENPTY_TEST_LOCK. It is gated insidespawn_with_configrather than sprinkled across the tests, because spawn sites live in four different test modules (io::pty_tests,io::event_monitor_actor::{mod, writer},terminal_app) and a lock a new test can forget to take would let the flake back in silently. No integration test spawns a PTY, which is what makescfg(test)a sufficient boundary. Poisoning is.expect()-ed so it fails loudly, per the project's no-silent-failures rule.Deliberately not done: widening the timeout or retrying the spawn. Either would mask a genuine libc thread-safety bug behind a timing knob.
Reviewer notes
Mutex(machinery for an impossible race, andMutex<()>guards no data), and giving PTY spawning its own actor (the tidiest answer, but the natural home is the engine andpixelflow-runtimeis deliberately generic while a PTY is terminal-specific).spawn_with_configreentrant. Noted in the rustdoc.nix::unistd::ttynamein the same spawn path is not a second instance of this bug; it already uses the reentrantttyname_r.Verification
cargo test --workspaceruns, zero failures and zero panics across all 74 test-result lines each.cargo test -p core-term --lib io::pty_testspasses.cargo build --workspaceclean, no unused-import warnings.pixelflow-runtime/src/platform/macos/cocoa.rs(raw-pointer lint), a file this PR does not touch.🤖 Generated with Claude Code