Repository navigation
test: hold the daemon writer before the PTY attach deadline handler runs - #15921
Conversation
The blocked-cancellation test held the transport writer only after a fake daemon event arrived, so the writer had to be held within the 1 s attach RPC deadline. When that round trip took longer on a loaded runner, the cancellation write ran first, succeeded, cancelled the write deadline, and the expected transport stop never came (line 182 timed out after 10 s). The test now holds the writer first and then runs the handler a timed-out pty.attach invokes, keeping the writer held until the test ends. The companion test still covers the timeout-to-cancellation wiring. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe timeout-isolation test now uses an idle transport and blocks the client write queue before invoking PTY-attach cancellation on a separate queue. It checks that cancellation returns while the write remains blocked. The test no longer checks the timed-out ChangesPTY Attach Cancellation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change isolates cancellation under a blocked writer while retaining transport-termination and attach-timeout coverage. No merge-blocking issue remains. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI failure attributionCI failed on
Matched log linesNot re-run automatically: Written by |
Dogfood tours of
|
|
Your package-test fix looks right and
#15930 restores main's pointer. Once it lands, a rerun of Separately, thank you for this one. The same test was failing on #14876, #14855 and #15229 at the same time and your diagnosis of the ordering is better than the one I had. |
|
Same diagnosis as mine for the #15488 main full-suite repair: the product is right, and the test races its own setup. The writer block is queued from a Two gaps in this version:
An alternative keeps the real path. Call through _ = try client.callIfIdle(method: "pty.attach", params: [...], timeout: 1) {
client.writeQueue.async {
writeBlockEntered.signal()
releaseWrite.wait() // held until every expectation is checked; a 60 s safety timer is failure-only
}
}The file goes from 256 to 261 lines. I have the full diff and can push it here or open it separately, whichever you prefer. |
|
This branch's red checks come from a 10:00 UTC run, when main didn't compile: the |
|
I merged main into this branch with main still hits this failure: swift-package-tests in run 36741080703 failed at |
|
Pre-merge review (correctness first, subagent): approve.
This run's one red check is |
|
Merge receipt for
Labeled |
github-actions
Bot
added
the
merged-unverified
3 participants
RemoteDaemonRPCClient timeout isolation / a timed-out PTY attach does not wait for a blocked cancellation writehas failed in about half of main'smacos / swift-package-testsruns since 2026-09-29 07:43 PDT, always at line 182 (unexpectedTermination.wait(timeout: .now() + 10)returned.timedOut).The test held the transport writer only after the fake daemon echoed a
pty.dataevent, so the writer had to be held within the 1 spty.attachdeadline. That round trip (a shellsedfork, the stdout reader, the state queue, a hop to a global queue, and one more to the write queue) has no bound on a loaded runner. When it took longer than 1 s, thepty.attach.cancelwrite ran first on an idle writer, succeeded, and cancelled the write deadline, so the transport stop the test waits for never happened. Raising the final wait (2 s to 10 s in #15116) can't help, because nothing is left to stop the transport. Delaying the fake daemon's event by 1.2 s reproduces the CI failure locally with the same line and duration (11.36 s).The test now holds the writer before running the handler a timed-out
pty.attachinvokes (sendPTYAttachCancellation), and keeps it held until the test ends. With that order fixed, only the product's 1 s cancellation-write deadline can stop the transport, and the wait budgets are failure bounds only. The test also no longer needs the 5 s safety timer that released the writer. The companion test in the same suite still drives a realpty.attachtimeout end to end and checks that the cancellation reaches the daemon with the right request id and tokens.Validation:
swift testinPackages/macOS/CmuxRemoteDaemonpassed 3 times in a row locally. The target test passed in 40 of 40 concurrent full-package runs with 60 CPU hogs on an 18-core Mac. The fullci.ymlrun on this branch is linked below.Changelog
none
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the flaky
timedOutPTYAttachBoundsCancellationWritetest by holding the transport writer before the attach deadline handler runs. The test previously held the writer only after a fake daemon echo, so on a loaded runner the echo could arrive after the 1 s attach deadline, letting the cancellation write complete and the expected transport stop never happen.sendPTYAttachCancellationdirectly, keeping the writer held until the test ends.helloand then only drains input.Written for commit d7b51d6. Summary will update on new commits.
Summary by CodeRabbit