Skip to content

cmux-imessage: a send never waits on a descendant holding stderr (#33301) - #33687

Merged
teamleaderleo merged 3 commits into
feat-cmux-nextfrom
fix/imessage-send-timeout
Nov 3, 2026
Merged

teamleaderleo merged 3 commits into
feat-cmux-nextfrom
fix/imessage-send-timeout

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #33301, a follow-up from the Codex review of #33219: a send could hang past its timeout when a descendant of osascript inherited stderr and kept it open.

What changes

  • stderr is drained on its own thread into a shared buffer, and nothing joins that thread.
  • On timeout: the send returns as soon as the child is killed and reaped.
  • On a normal exit: it keeps what stderr has delivered within a 2-second grace, then returns.
  • TimedRunner { timeout } is the runner. SystemRunner keeps its name and API and delegates to it with SEND_TIMEOUT.
  • Stop condition: the drain thread ends at end of file, when the last holder of the pipe closes it. It never blocks a send.

Tests

Two commits: the failing tests first (both red on Linux CI, each waiting the full 20 s on the descendant), then the fix. Green: 9 send tests pass in about 2 s, including a 1-second timeout with sleep 20 & wait and an exit that leaves sleep 20 & holding stderr. The 13 reader tests still pass. clippy -D warnings, the panic ratchet and rustfmt are clean.

Agent session: agent session

🤖 Generated with Claude Code


Autofix with [code]smith
Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.


Summary by cubic

Fixes #33301: an iMessage send no longer hangs past its timeout when a descendant of the child keeps stderr open.

  • stderr is drained on its own thread into a shared buffer and never joined, so a descendant holding the pipe open can't block the send; on timeout the send returns as soon as the child is killed and reaped, and on a normal exit it takes what stderr delivered within a 2-second grace.
  • Adds TimedRunner { timeout }; SystemRunner keeps its API and delegates with SEND_TIMEOUT.
  • Adds two tests: a 1-second timeout with sleep 20 & wait and an exit that leaves sleep 20 & holding stderr; both previously waited the full 20 seconds.

Written for commit ac50c35. Summary will update on new commits.

View guided diff Turn on auto-fix

teamleaderleo and others added 2 commits November 3, 2026 03:16
… stderr (#33301)

Moves the runner into TimedRunner { timeout } (SystemRunner keeps its API
and delegates with SEND_TIMEOUT) so tests can use a short timeout, and adds
two tests where a backgrounded descendant keeps stderr open: after a
timeout, and after a normal exit. Both still wait for the descendant.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-9bf3963ac29d9d619047
…err (#33301)

stderr is drained into a shared buffer on its own thread, and nothing joins
that thread. After a timeout the send returns as soon as the child is killed
and reaped; after a normal exit it takes what arrived within a 2-second
grace. The drain thread ends at end of file, when the last holder of the
pipe closes it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-9bf3963ac29d9d619047
@cursor

cursor Bot commented Nov 3, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review ac50c35: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr33687 -f ref=fix/imessage-send-timeout -f platform=macos -f backend_mode=local

web and size checks: green

Shipped in:

@teamleaderleo teamleaderleo added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (subagent claude-opus-5-5 medium): SHIP at cba52bf

Visual verdict: unchanged by this diff (no UI).

Checked the drain, the exit and timeout paths, and the two new tests. In the normal case nothing is lost: collect waits for EOF, which comes as soon as the child's pipe closes, so every byte is buffered before the lock is read. The mutex is poisoned only if the reader panics mid-extend, and that case falls back to an empty string. A dead reader disconnects the channel, so recv_timeout returns at once. SystemRunner is still a unit struct with the same trait impl, so its other user is unaffected; TimedRunner is new and only adds to the API. Killing only the direct child is what base did too, so it is no regression. The test bounds (8 s against a 1 s timeout or 2 s grace) leave room on a loaded CI.

Follow-ups:

  • [follow-up] If the child exits just before the deadline, grace can be ~0, so recv_timeout(0) may return before the reader has drained the last bytes. A floor such as max(grace, 100ms) would keep the tail.
  • [follow-up] When a descendant keeps the pipe open, the reader thread and its buffer live until that descendant exits, and the buffer has no size cap. Base's read_to_end had no cap either. Capping the buffer (say 64 KiB) would bound the memory per send.
  • [follow-up] Only the direct child is killed on timeout, so an osascript descendant keeps running. Running the child in its own process group and killing the group would clean it up.
  • [follow-up] Each test leaves a sleep 20 orphan running. That is harmless, but sleep 20 could be shortened to about 10.

@blacksmith-sh

blacksmith-sh Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Found 2 test failures on Blacksmith runners:

Failures

Test View Logs
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)/
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)
View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)/
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)
View Logs

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex gpt-6.1-sol xhigh, router-review): BLOCK at cba52bf

BLOCK at cba52bf

Visual verdict: no images; Leo tries the build

  1. [P1] [blocking] Restore the measurement build defaults in scripts/reload.sh:905. Exclusive measurement jobs now use step-local HOME instead of the preserved checkout’s .build; the related deletions also remove native-architecture builds and completion markers. This reinstates the cold universal builds documented as exceeding 30 minutes, beyond the perf cron’s 1,200-second build limit. Both deleted measurement tests pass on base and fail against head. Restore this functionality and its tests; it is unrelated to the stderr fix.

The red-test commit picked up stale copies of scripts/reload.sh and two
reload tests from the worktree. Restores them to the PR base so this PR
changes only cmux-imessage.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Agent-Session: https://cmux.com/deeplink/agent?id=aid-9bf3963ac29d9d619047
@github-actions github-actions Bot added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 3, 2026
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Review (codex, router-review): SHIP at ac50c35

Reviewer: codex gpt-6.1-sol xhigh, fast tier.

Follow-ups: 1. Each lands as an issue in this repo that links this receipt; the lane that opened the PR fixes it forward.

SHIP at ac50c35

Visual verdict: no images; Leo tries the build

  1. [P2] [follow-up] send.rs:178: Retry ErrorKind::Interrupted. A handled Unix signal can currently terminate the drain and close stderr early, truncating diagnostics or causing subsequent child writes to fail; the previous read_to_end retried these interruptions.

Rust tooling is unavailable, so tests could not be rerun.

Follow-ups:

  • Retry interrupted stderr reads as described in finding 1.

@teamleaderleo
teamleaderleo merged commit 78a9933 into feat-cmux-next Nov 3, 2026
160 of 162 checks passed
@teamleaderleo
teamleaderleo deleted the fix/imessage-send-timeout branch November 3, 2026 12:57

This branch was previously deployed

3 inactive (2 outdated) deployments
ci-routing-agent — ac50c35d Deployed Nov 3, 2026 by teamleaderleo via wait for same-key tree publisher #106155
artifacts — cba52bf8 Deployed Nov 3, 2026 by teamleaderleo via publish tree-addressed cmux-tui #105711
sentry-dsyms — cba52bf8 Deployed Nov 3, 2026 by teamleaderleo via upload cmux-tui dSYMs to Sentry #105711
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant