fix(tests): drain pty master so macOS integration tests cannot hang; bound all waits - #254
Merged
Merged
Conversation
…bound all waits The `pytest -m integration` job added in #251 and wired up in #253 hangs forever on the macOS runner. Root-caused on real macOS hardware to a test-harness bug (not a product bug): an un-drained pty master wedges the child on macOS. Both symptoms are harness bugs. The product is correct on macOS. Root cause: macOS wedges an exiting pty child whose output queue is never drained — not slowly, unreapably. A wedged child lands in ps state `?Es` (Exiting, session leader, controlling terminal already revoked). The fix introduces a shared pty harness (`tests/pty_harness.py`) that: - Drains the pty master via a dedicated thread on a dup() so the caller's master_fd keeps blocking-write semantics - Replaces parent-side sleeps with `wait_for_marker()` readiness handshake so sends land inside the child's live window - Bounds all `waitpid` calls — no blocking waits that can hang forever - Removes the un-drained-pty + SIGKILL condition that was a landmine in test_stdout_offload_freeze_integration.py:198 CI hardening: `timeout-minutes: 10` on both jobs. Typical runtime is 20-30s. A hung job now fails loudly in 10 minutes rather than burning runner hours. Verification: 5 consecutive integration runs on macOS, all green, no stray `?Es` children left behind. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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.
What
The
pytest -m integrationjob added in #251 and wired up in #253 hangs forever on the macOS runner (18-24+ min before I cancelled it; the ubuntu job finishes in 39s). #253 was merged believing that job had passed. It had not — it had never completed on macOS, and it never passed.Root-caused on real macOS hardware (26.6, Darwin 25.6.0, arm64). Both symptoms are harness bugs. The product is correct on macOS —
git diff --stat -- amplifier_app_cli/for this PR is empty.Root cause: an un-drained pty master wedges the child on macOS
macOS wedges an exiting pty child whose output queue is never drained — not slowly, unreapably:
A wedged child lands in
psstate?Es— Exiting, session leader, controlling terminal already revoked. That one state produces both failures:1.
OSError: [Errno 5]in 3 echo tests — the EIO is on a write, not a read (test_terminal_echo_integration.py:260, insidesend()). The platform difference:Caught in the act with
pssampled per write:w1@1.11s ps='Ss+' OK | w2@1.30s ps='Z' EIO. The child had already wedged and died before the parent's hardcodedsleep(0.6)+sleep(0.3)fired. On Linux the child is still alive at 0.9s, so the byte lands and the race is invisible.Note what the tempting fix would have done: swallowing the EIO would make macOS green while never delivering the Ctrl-C — a vacuously passing test. Fixed properly instead: drain the pty so the child never wedges, and replace fixed sleeps with a readiness handshake so the send lands inside the child's live window.
2. The hang —
--faulthandler-timeoutpinned it exactly:The product is acquitted, with evidence
Ctrl-C cancellation works on macOS — the wedged child had already written its result before dying:
{"scenario": "single_ctrlc", "elapsed": 4.07, "state": "graceful", "is_cancelled": true, "messages": ["\n[yellow]Stopping after current operation completes... (Ctrl+C again to force)[/yellow]"]}That is exactly what the test asserts.
Termios restoration works on macOS. Same driver, only difference is a drain thread:
The fix
New
tests/pty_harness.py; both drivers converted (124 insertions, 174 deletionsin the two test files — net smaller).dup()of the master, so the caller'smaster_fdkeeps blocking-write semantics andtermios.tcgetattr()still reads real state.send()/signal()raiseAssertionErroron undeliverable input — a test can never silently verify nothing.waitpidanywhere.wait()andkill()are boundedWNOHANGpolls. A test may fail; a test must never hang.wait_for_marker()readiness handshake replaces parent-side sleeps.waitpid(pid, 0)removed fromtest_stdout_offload_freeze_integration.py:198— that test deliberately builds the un-drained-pty + SIGKILL condition, so it was the next landmine.CI hardening:
timeout-minutes: 10on both jobs. Typical runtime is 20-30s. A job stuck atin_progressreads as "not done yet" rather than "broken" — which is precisely how a false green got merged. Now a hang fails loudly in 10 minutes instead of burning runner hours toward the 6h default on runners that bill at 10x.Deliberate-breakage proof (tests still catch regressions)
raw_mode.__exit__never restores (the literal user-facing symptom)assert 'none' == 'graceful'The Ctrl-C break failing on macOS also directly disproves vacuity: the
\x03byte really does reach the child's_CtrlCInterruptpath there.The 1/6 on the last row is a property of the bug, not the probe — it depends on asyncio shutdown sweep order. When the last orphan to restore happens to be the one holding pristine attrs, the terminal comes out healthy by luck:
exit_order=[1,3,0,2] -> CAUGHT,exit_order=[2,1,3,0] -> missed.Verification — all four combinations
macOS: 5 consecutive integration runs, all green, no stray
?Eschildren left behind. (Before this fix, one was still parked hours after the run that spawned it.)Follow-up to #251 and #253.