Repository navigation
Fix macOS PTY deadlocks and run the mxc_pty tests in CI - #1476
Merged
Huzaifa Danish (huzaifa-d) merged 12 commits intoOct 15, 2026
Merged
Conversation
The mxc_pty unit tests ran on no CI lane: the macOS job skipped the module, the Linux job selects lib tests by module and never matched it, and Windows compiles only a stub. Give the Linux lane an explicit step and drop the macOS skip. Enabling them exposed three test defects. native_bridge_cancellation_does_not_inject_eof backgrounded its reader, so the parent shell's exit orphaned the reader's process group. Reading a controlling terminal from an orphaned group fails with EIO, and the `;` separator ran the marker printf regardless, so the test reported an injected end-of-input that never happened. A probe confirmed the marker was already in the pipe before the bridge was torn down. Keep the reader in the foreground process group and wait for it to block in `read`, so only the bridge can supply end-of-input. native_bridge_finishes_when_descendant_holds_terminal left its `sleep` running, which the runner reported as an orphan process. Kill the process group once the assertions are done. assert_canonical_eof_after_input read to end-of-input on the test thread, so a reader that never observes EOF hung the whole suite instead of failing. Read on a worker and report what was collected. Also give reader_from_file the same Darwin end-of-input probe that take_native_stdio already applies, so the boxed reader and the native output bridge agree on how end of input is detected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The drain-after-EPIPE fix lacks a regression test that would fail under the previous behavior.
1 open finding
What changed in this PR
Fixes macOS PTY deadlocks by ensuring terminal output remains drained and replacing FIFO-based pipes.
Changes:
- Keeps draining PTY output after consumer closure.
- Uses Darwin
pipe(2)and simplifies interruptible reads. - Expands PTY tests, CI coverage, and API guidance.
| File | Description |
|---|---|
src/mxc-sdk/src/core/mxc_pty/mod.rs |
Fixes bridges, Darwin pipes, and PTY tests. |
src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs |
Removes obsolete EOF probing. |
docs/api-reference/rust/v1/api.md |
Documents concurrent output draining. |
docs/api-reference/node/v1/api.md |
Documents concurrent output draining. |
docs/api-reference/dotnet/v1/api.md |
Documents concurrent output draining. |
.github/workflows/Build.MacOS.Job.yml |
Enables PTY tests with a timeout. |
.github/workflows/Build.Linux.Job.yml |
Adds explicit PTY test coverage. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
macOS has no atomic close-on-exec pipe: there is no `pipe2`, and `socketpair` has no `SOCK_CLOEXEC`. Both ends are inheritable between `pipe(2)` and the `fcntl` that marks them, so a spawn on another thread during that window captures them and the child holds a live handle to another container's stdin or stdout. Linux has `pipe2`, so the wakeup self-pipe now uses it and the race is gone outright there. macOS closes the window from the other side: a new `inherited_descriptors` module drops every descriptor that is not marked close-on-exec, called from the `pre_exec` hook `LivePty` already installs. A full descriptor-table scan costs ~15ms per spawn, so the scrub walks `/dev/fd` instead. The libc `getdirentries` wrapper is unavailable once 64-bit inodes are in effect, so the syscall is issued directly, with the full scan kept as a fallback. The walk completes before anything is closed, since removing entries mid-walk shifts the directory offset and skips descriptors. Descriptors already marked close-on-exec are spared, including the pipe the Rust runtime uses to report a failed `exec`. Closing that one makes every failure look like a success, so a test pins the behaviour. Also adds coverage for the public surface the backends depend on, which had none: `size`, `is_canonical_mode`, SIGWINCH delivery on resize, `attach_forwarded` against a raw-mode terminal, and `RawSecondaryGuard`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
I have decided to remove the file descriptor deletion from this PR. MacOS (Unix) simply cannot atomically create a pipe with close-on-exec. Rust's built-in calls have the same race condition. Only Linux can using pipe2(). Deleting all non-marked fds is something Go does, and I think another PR on top of this one could implement a block on all non-stdout/stderr/stdin fds. I'm not introducing a bug here, it was already present. |
Elliot (theelliotm)
marked this pull request as draft
October 14, 2026 19:02
Elliot (theelliotm)
marked this pull request as ready for review
October 14, 2026 21:26
| // check it before the kill can suppress it. | ||
| std::thread::sleep(Duration::from_millis(200)); | ||
| assert!( | ||
| child.try_wait().expect("poll child").is_none(), |
Contributor
There was a problem hiding this comment.
This check can still miss the bug. finish_native_bridge() stops the output reader before this assertion. If the shell receives EOF after that reader stops, it can write the marker into the unread terminal and become stuck while exiting. try_wait() then returns None, and the final pipe read cannot see that marker. Please keep output draining while checking for EOF, and use a timeout for the final cleanup.
`try_wait()` alone conflated two states: a shell still blocked in `read`, and one that received EOF, wrote the marker into an unread terminal and wedged while exiting. Both report the child as running, and because `finish_native_bridge()` has stopped the output reader, the marker never reaches the pipe either. Watch the primary directly and keep draining it, so the marker is detected positively instead of inferred from the child still running. Reap on a deadline, still draining: a Darwin child stuck writing to a full terminal does not die on SIGKILL until the queue drains. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Huzaifa Danish (huzaifa-d)
approved these changes
Oct 15, 2026
Huzaifa Danish (huzaifa-d)
deleted the
user/emichlin/fix-pty-ci-coverage
branch
October 15, 2026 01:48
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.
TLDR;
pipe2(O_CLOEXEC)for both the PTY pipes and theInterruptibleReaderwakeup pipe, removing the race outright on that platform.pipe()+fcntl, matching the standard library, with thelimitation documented at both call sites. Limitation documented below.
📖 Description
The macOS lane skipped
core_modules::mxc_pty::tests::. Un-skipping it hung the jobuntil GitHub's 6-hour cap.
Root cause. On Darwin, the final close of a PTY secondary blocks in the kernel
until the primary's output queue is drained, and
SIGKILLdoes not release it. Achild that writes even 10 bytes never exits while the primary is undrained. Linux
has no equivalent behavior. Production backends already drain before waiting, but
two real library bugs were hiding behind the broken tests.
Library fixes
EPIPE, so a caller closing itsstdout stopped the primary drain and froze the child forever on macOS, deadlocking future operations. It now
drops the writer and keeps draining/discarding.
create_pipeused a FIFO substitute under a hardcoded/tmp. Replacedwith a real
pipe(2)+fcntl(FD_CLOEXEC).Test fixes
PtyDrain/wait_for_exithelpers; every child-spawning test drains beforewaiting.
wait_for_exithas nokillfallback on purpose —SIGKILLcan'trelease a child stuck in the Darwin drain, but unwinding drops
LivePtyandcloses the primary, which does.
child.wait(). A regression there hungrather than failed, which is the exact failure mode this PR exists to remove.
All now use the bounded
wait_for_exit.dropping_writer_does_not_wait_for_a_full_input_queue: theEWOULDBLOCKassertion is now Linux-only (Darwin's
ttyinputdiscards instead).timeout-minutes: 45on both Unix build jobs.New coverage (31 → 46 tests)
take_unclaimed_readerclose_writernative_bridge_keeps_draining_after_the_output_consumer_closessizeis_canonical_modeattach_forwardedagainst a raw-mode terminal and itsCurrentCanonicalcontrast
RawSecondaryGuardsave/restore.Limitation
There is a very small race condition (<1us) on MacOS between creating a fd and marking it
cloexec, where a concurrently made sandbox might inherit those fds. MacOS (Unix) simply cannot atomically create a pipe with close-on-exec. Rust's built-in calls have the same race condition. Deleting all non-marked fds in the child is something Go does to avoid this, and I think another PR on top of this one could implement a block on all non-stdout/stderr/stdin fds. I'm not introducing a bug here, it was already present.🔗 References
🔍 Validation
All PTY tests pass on all three Unix lanes in
run 38322386987 at
d180374:Local, macOS 27:
cargo test -p mxc-sdk --lib— 1782 passed, 0 failed, 1 ignored, repeated runscore_modules::mxc_pty::tests::run individually under a 45 s watchdog — pass, no hangscargo fmt --all -- --check— cleancargo clippy --locked --target aarch64-apple-darwin -p mxc-sdk --all-targets -- -D warnings— cleancargo build --locked --release --target aarch64-apple-darwin -p mxc_darwin -p mxc-sdk -p unix_test_proxy -p mxc_ffi— ok✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 Issue Type