Repository navigation
Ignore remote stdin writes after child exit - #9080
lawrencecchen wants to merge 28 commits into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe process runner now uses an injectable stdin writer for pipe-backed input, ignores expected broken-pipe races, reports other write failures with code 3, documents the new error case, and adds regression coverage. ChangesStdin pipe handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant RemoteSessionProcessRunner
participant RemoteProcessStdinWriter
participant ChildProcess
RemoteSessionProcessRunner->>RemoteProcessStdinWriter: Write stdin data
RemoteProcessStdinWriter->>ChildProcess: Write process pipe input
ChildProcess-->>RemoteProcessStdinWriter: Return success or POSIX error
RemoteProcessStdinWriter-->>RemoteSessionProcessRunner: Return or throw
RemoteSessionProcessRunner->>ChildProcess: Terminate and wait on unexpected error
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift`:
- Around line 191-193: Update the stdin write in RemoteSessionProcessRunner to
catch errors from writeProcessPipeInput explicitly, ignore only the POSIX EPIPE
case caused by the child closing the pipe, and propagate or route all other
failures—including EBADF, EIO, fcntl, and write errors—through the runner’s
failure path instead of continuing to the normal timeout or exit-status result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9304f8bb-670e-487d-9b99-d183b7f2d2c3
📒 Files selected for processing (2)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionProcessRunnerTests.swift
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift`:
- Around line 23-24: Remove the test-only StdinWriter/writeStdin injection from
the production RemoteSessionProcessRunner type, including its stored property
and initializer plumbing. Make stdin writes use the normal FileHandle path
directly, and update callers/tests to exercise failures through a controlled
pipe or process fixture instead of injecting behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f9995a8-18df-43da-a550-cbb984d6701d
📒 Files selected for processing (2)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunning.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift (1)
221-240: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid blocking the caller during stdin-write failure cleanup.
Line 231 now invokes
terminateProcessAndWait()from synchronousrun, blocking for up to two seconds and potentially inwaitUntilExit(). Use the existing completion-driven process lifecycle or an async cancellation-aware owner instead of adding a blocking repair path here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift` around lines 221 - 240, Update the stdin-write failure cleanup in the synchronous run path to avoid calling blocking terminateProcessAndWait(). Reuse the existing completion-driven process lifecycle or an asynchronous cancellation-aware owner to terminate the process without waiting synchronously, while preserving handle cleanup, logging, and error propagation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift`:
- Around line 221-240: Update the stdin-write failure cleanup in the synchronous
run path to avoid calling blocking terminateProcessAndWait(). Reuse the existing
completion-driven process lifecycle or an asynchronous cancellation-aware owner
to terminate the process without waiting synchronously, while preserving handle
cleanup, logging, and error propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2ac85422-7d2e-47bc-872f-cd01b5cbd28c
📒 Files selected for processing (2)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionProcessRunnerTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift (1)
197-204: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid blocking the caller during stdin-write failure cleanup.
The new error path calls
terminateProcessAndWait(), which waits up to two seconds and may then callprocess.waitUntilExit(). This materially expands synchronous blocking into the write-failure path; use the runner’s lifecycle/completion signaling without adding another blocking wait.As per coding guidelines and path instructions, non-test Swift must not add or materially expand blocking waits for process or async coordination.
Also applies to: 214-218
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift` around lines 197 - 204, Update the stdin-write failure cleanup to avoid calling terminateProcessAndWait(), which can block for up to two seconds or wait for process exit. Reuse the runner’s existing lifecycle/completion signaling to initiate termination and let asynchronous cleanup handle completion without adding a synchronous process or coordination wait.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionProcessRunnerTests.swift`:
- Around line 98-122: Replace the systemUptime-based assertion in
unexpectedStdinWriteErrorTerminatesChildAndKeepsPinnedRunnerError with a
test-owned exit marker or process handle that observes /bin/sleep termination.
After runner.run throws, await or poll the termination predicate using the test
framework’s deadline-bounded mechanism, while preserving the existing error
assertions.
---
Outside diff comments:
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swift`:
- Around line 197-204: Update the stdin-write failure cleanup to avoid calling
terminateProcessAndWait(), which can block for up to two seconds or wait for
process exit. Reuse the runner’s existing lifecycle/completion signaling to
initiate termination and let asynchronous cleanup handle completion without
adding a synchronous process or coordination wait.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77c1925f-9189-47b7-99d1-191adcd734fc
📒 Files selected for processing (3)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteProcessStdinWriting.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteSessionProcessRunner.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionProcessRunnerTests.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe9f1b236c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
lawrencecchen
added
the
stale-revisit
2 participants
Closes #5750
Supersedes #6863. Current main already owns the shared POSIX process-pipe writer and the TextBox call site. The remaining unsafe production write was RemoteSessionProcessRunner feeding a child that had already closed stdin.
The runner now uses writeProcessPipeInput and treats a closed reader as a completed best-effort stdin handoff. The shared helper suppresses SIGPIPE, handles partial writes and EINTR, and reports EPIPE instead of allowing Foundation to abort the process.
Regression proof:
Validation:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Prevents aborts when a child closes stdin early, makes completion event‑driven and lock‑free, and ensures timeouts fire even if stdin is unread or descendants inherit the pipes. Surfaces unexpected stdin write errors deterministically, even if the child ignores TERM and is SIGKILLed.
Bug Fixes
RemoteProcessStdinWriter: ignore SIGPIPE; nonblocking writes with POLLOUT polling; handle partial/EINTR/EAGAIN; treatEPIPEas expected when the child closes stdin.ProcessPipeStopAwareReaderandProcessPipeStopSignal; return buffered bytes on exit so inherited output pipes can’t stall.EPIPEstdin errors ascmux.remote.processcode 3; terminate and preserve the error even after forced SIGKILL; wins over earlier exits.Refactors
RemoteProcessExitSignal,RemoteProcessCompletionWaiter, andRemoteProcessStdinWriteState; injectRemoteProcessStdinWriting(defaultRemoteProcessStdinWriter).ProcessPipeStopSignalandProcessPipeStopAwareReader; normalize non‑positive chunk sizes to 1; keep a minimal read‑handle observation seam for tests.Written for commit c54a980. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
3).Tests
Documentation
RemoteSessionProcessRunning.rundocs to explicitly document stdin-write as a possible failure mode.