Repository navigation
Conversation
When the job manager failed to enqueue a stream data packet (e.g. "timeout sending request" after 5s of backpressure), the error was only logged. The packet was already counted in sentNotAcked and is never retransmitted, while the reader parks every later packet as out-of-order without ACKing. The stream deadlocks once the 64KB window fills, but the job route stays up, so no auto-reconnect happens and the block shows "Attached" with no output. On send error, drop the stream back to disconnected (buffering) mode and close the main server connection. The job route goes down, the existing route-down auto-reconnect reattaches, and the stream resumes from the client's last received seq.
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
The PR implements the failed-send recovery path in #3439. DataSender.SendData returns errors. StreamManager enters disconnected buffering mode for the current sender. routedDataSender closes the… |
Implement and test the required end-to-end output and input health checks. Change Force Restart Controller to reconnect without terminating the preserved job, or clearly label the action as destructive. If these requirements are intentional… |
| Docstring Coverage | |
Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 9 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Out of Scope Changes check The changes remain connected to the failed-send recovery path in #3439. TheDataSendererror propagation, connection close, disconnected buffering, deferred route reconnect, closed-channel error han…Title check The title clearly summarizes the main change: recovering frozen durable SSH terminals after send failures. Description check The description directly explains the durable SSH stream failure, the recovery behavior, implementation scope, tests, and known limitations.
Full details: Linked Issues check
-
Explanation
The PR implements the failed-send recovery path in
#3439.DataSender.SendDatareturns errors.StreamManagerenters disconnected buffering mode for the current sender.routedDataSendercloses the main-server connection. The route-down path schedules reconnection. The added tests cover recovery and stale sender errors. However,#3439also requires end-to-end output and input health checks. The PR adds no such checks. The issue also requires non-destructive handling or clear labeling for Force Restart Controller. The PR does not change or test that behavior.Resolution
Implement and test the required end-to-end output and input health checks. Change Force Restart Controller to reconnect without terminating the preserved job, or clearly label the action as destructive. If these requirements are intentionally separate, separate or update the linked issue scope before merge.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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: 9bb171844e
ℹ️ 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".
…reconnects SendComplexRequest recovered a send-on-closed-channel panic but returned (nil, nil), so SendCommand/SendRpcRequest dereferenced a nil handler. That happens when the connection closes while a sender is blocked on OutputCh (e.g. sendJobExited during the close after a failed stream send), and would crash the job manager. Return the panic as an error. A route down inside the 30s auto-reconnect cooldown was dropped, so a job whose route went down twice in quick succession stayed disconnected. Defer it to the end of the cooldown instead, with at most one deferred attempt pending per job.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
kieled
changed the title
This branch has not been deployed
2 participants
Fixes #3439, and very likely #2985.
Bug: a durable SSH terminal randomly stops showing output and ignores input while the shield still says Attached. Today the only ways out are restarting Wave or Force Restart Controller, which kills the session. It's been reported with Claude Code, Codex, OpenCode, lazygit and
tail -f.After this PR: the block briefly shows as disconnected and then reattaches by itself through the existing route-down reconnect, with no lost output.
Size/risk: about 60 lines plus tests in
jobmanager,wshutilandjobcontroller. There are no new settings or periodic timers. CodeRabbit is clean, and the Codex re-review after the fixes found nothing. Verified in real use: 7 freezes recovered automatically in about 1s each (see Testing).Problem
A durable SSH block can freeze with no output while still showing Durable Session (Attached). When it happens, the remote job log shows one
SendData: error sending stream data: timeout sending requestand nothing after it, while the SSH connection stays healthy.This is a stream deadlock that one failed send causes:
prepareNextPacketincreasessentNotAckedbefore the packet is sent.SendComplexRequestblocks for more than 5s, the packet is dropped.routedDataSender.SendDataonly logged this, sosenderLoopcarried on as if the bytes had been delivered.streamclient.Readerparks every later packet (Seq > nextSeq) as out-of-order and doesn't ACK, sonextSeqcan never move past the gap.sentNotAckedreaches the 64KB window, the sender waits forever. The job route is still up, sohandleRouteDownEventand auto-reconnect never run.Fix
The main fix is in the job manager:
DataSender.SendDatanow returns an error.StreamManagergoes back to disconnected (buffering) mode, which resetssentNotAcked. It does this only if the failing sender is still the current one, so a client that reattached in the meantime isn't disconnected.routedDataSendercloses itsMainServerConnwhen a send fails. The job route then goes down on the main server, the existing route-down auto-reconnect callsReconnectJob, andPrepareConnect/StartStreamresume from the client's last received seq. This is the same path an app restart or a manual Disconnect → Reconnect already takes, and it recovers these frozen blocks with no data loss.Two small hardening changes for the recovery path:
SendComplexRequestreturns a recovered send-on-closed-channel panic as an error instead of(nil, nil). Before, closing the connection while another sender (e.g.sendJobExited) was blocked onOutputChmadeSendCommand/SendRpcRequestdereference a nil handler and crash the job manager.jobcontroller: a route down inside the 30s auto-reconnect cooldown is deferred to the end of the cooldown instead of dropped (at most one pending per job), so a second send failure in quick succession still recovers.There's no stream watchdog and no new periodic timers.
Compared with #3460, this is only its job-manager-side approach. It leaves out the 45s output watchdog in
jobcontroller, which would also fire on a shell sitting idle at a prompt.Scope
This fixes the deadlock behind #3439 and recovers through the existing route-down reconnect, so the block shows as disconnected and then reattaches instead of staying falsely Attached. It intentionally does not add end-to-end stream health checks (expected-behavior item 4), and it doesn't change Force Restart Controller, which still terminates the job. I'm happy to follow up on either separately if wanted.
Testing
TestSendErrorFallsBackToDisconnected: a failingDataSendergets exactly one send attempt. A client that connects afterwards receives the full stream starting from seq 0. Both checks fail onmain("got 2 calls" and "client already connected").TestStaleSendErrorKeepsNewClient: a late error from a previous client's sender does not disconnect the current client.TestSendOnClosedOutputReturnsError(pkg/wshutil): sending on a closed connection returns an error. It fails with a nil-pointer panic onmain.TestNextAutoReconnectDelay*(pkg/jobcontroller): a route down inside the cooldown is deferred, not dropped, and isn't scheduled twice.go test -race ./pkg/jobmanager/ ./pkg/streamclient/ ./pkg/wshutil/ ./pkg/jobcontroller/,go vet,go build ./...andGOOS=linux go build ./cmd/wshall pass.Tested end-to-end in real use. Since 2026-10-14 I've been running a v0.14.5
wshbuilt with these commits on my Linux host (macOS → Linux over Tailscale, Claude Code running in a durable block). Over the first ~21 hours the 5s enqueue timeout hit 7 times on that block. Each time the job manager loggedhandleSendError, the main server auto-reconnected about 1s later, andPrepareConnectresumed withclientSeq == serverSeq. The block never froze and I didn't have to restart anything. Before the patch, each of these was a frozen block that needed Force Restart Controller or an app restart.Only the remote
wshwas patched, so thejobcontrollercooldown change wasn't exercised (no two failures came within 30s).One recovery, both sides
Remote job log:
Main server (
waveapp.log):