Repository navigation
Harden socket response writes - #3355
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR hardens Unix-socket response writes by replacing the single-shot Confidence Score: 4/5Safe to merge; the one P2 gap (no SO_RCVTIMEO on accepted sockets) is pre-existing and out of scope for this PR. No P0 or P1 findings. The single P2 observation (missing receive timeout) is pre-existing behavior not worsened by this change. Write-hardening logic, EINTR handling, and the send-timeout integration are all correct. Sources/TerminalController.swift — configureAcceptedClientSocket missing SO_RCVTIMEO Important Files Changed
Sequence DiagramsequenceDiagram
participant Listener as Accept Loop
participant Config as configureAcceptedClientSocket
participant Handler as handleClient (thread)
participant Client as Unix Socket Client
Listener->>Config: fd (accepted socket)
Config->>Config: configureBlocking(fd)
Config->>Config: SO_SNDTIMEO = 5 s
Config->>Config: SO_NOSIGPIPE (macOS)
Config-->>Listener: nil (ok) or (stage, errno)
alt config failed
Listener->>Client: close(fd)
else config ok
Listener->>Handler: spawnClientHandler(fd, peerPid)
Handler->>Client: read() [no RCVTIMEO]
Client-->>Handler: command line
Handler->>Handler: processSocketLine()
Handler->>Handler: writeAllToSocket() loop
loop partial write / EINTR
Handler->>Client: write(offset…count)
Client-->>Handler: n bytes written
end
alt write timeout (EAGAIN) or error
Handler->>Client: close(fd) — only this client
end
end
Reviews (1): Last reviewed commit: "fix: harden socket response writes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private nonisolated static func configureAcceptedClientSocket(_ fd: Int32) -> (stage: String, errnoCode: Int32)? { | ||
| if let errnoCode = configureBlocking(fd) { | ||
| return ("accept_client_configure_blocking", errnoCode) | ||
| } | ||
| if let errnoCode = configureSocketSendTimeout(fd, timeout: socketClientWriteTimeout) { | ||
| return ("accept_client_configure_send_timeout", errnoCode) | ||
| } | ||
| if let errnoCode = configureNoSigPipe(fd) { | ||
| return ("accept_client_configure_no_sigpipe", errnoCode) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
configureAcceptedClientSocket now applies SO_SNDTIMEO to guard against slow-reading clients, but it doesn't apply SO_RCVTIMEO. The read() call in handleClient (line 1882) will block indefinitely if a client connects and then goes silent without closing the socket — stalling that handler thread for the connection's lifetime. The code comment at line 1864 acknowledges connect-only probes, but a client that sends a partial command and then stalls would hold the thread forever. Consider adding SO_RCVTIMEO alongside SO_SNDTIMEO to fully bound the accepted-socket lifecycle.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request adds comprehensive socket write handling improvements to TerminalController, including staged client socket configuration with timeout and signal handling, refactored response writing with partial write recovery, and a new test suite exercising the socket write functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
This branch was successfully deployed
1 participant
Follow-up to #3340.
Summary:
Verification:
Note:
Summary by cubic
Hardened socket response writes to handle backpressure and avoid hangs. We now send full responses with retries, enforce a 5s send timeout and
SO_NOSIGPIPEon client sockets, and only close the failing client on write errors.writeAllToSocket(handles partial writes andEINTR).SO_NOSIGPIPEto accepted client sockets to prevent indefinite blocks and SIGPIPE.writecalls withwriteSocketResponse; return early on write failures to keep the listener responsive.Written for commit ddf8f99. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests