fix(tmux,session): stop the attach stdin reader so a session exit doesn't eat a keypress - #1773
Open
scottyallen wants to merge 3 commits into
Open
fix(tmux,session): stop the attach stdin reader so a session exit doesn't eat a keypress#1773scottyallen wants to merge 3 commits into
scottyallen wants to merge 3 commits into
Conversation
… a keypress When an attached session's pane process exits on its own, nothing pressed the detach key, so the stdin reader goroutine in AttachWithOptions had no reason to stop. It stayed parked in a blocking os.Stdin.Read that cancel() cannot interrupt (a tty inherited from the shell is left in blocking mode, so Go never registers fd 0 with the netpoller and SetReadDeadline returns ErrNoDeadline), and AttachWithOptions returned anyway - its sync.WaitGroup was populated but never waited on. Back on the deck, that stale reader and Bubble Tea's reader were both queued on the same tty. Waiters are woken in FIFO order and the stale one had waited longer, so it won the next keystroke, forwarded it to the now-closed PTY, hit the write error and died. The user's first keypress after a session ended did nothing; everything after it worked. Pressing the detach key never showed the bug, because there the reader is what detects the key and returns on its own. Extract the loop into attachStdinPump.run(ctx), which polls the descriptor before each read so cancellation is observable, and have cleanupAttach wait for it to exit before handing the terminal back. Also ungate the post-attach stdin flush and reply quarantine, which previously ran only on the detach path. The stale reader had been incidentally swallowing the terminal capability replies emitted during teardown; now that it stops cleanly, the flush is what keeps those bytes out of the TUI on the process-exit path too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpRhBnzjSCPBqhrEwKU8k2
…n half Review follow-ups on the local-attach keypress fix. The SSH attach path had the identical defect: its stdin reader exits only on a read/write error or Ctrl+Q, so the <-cmdDone branch (remote process exits on its own) left it parked in a blocking os.Stdin.Read. It then outlived the attach and ate the user's next keystroke on the deck, exactly as the local path did. It also never flushed the input queue at all. Give it a stop signal, poll before each read so the signal is observable, join it before flushing, and flush. Extract the teardown into quiesceAttachInput so the ordering is testable without a live tty. Previously nothing covered it: deleting the reader join, moving the flush ahead of it, or re-gating the flush on the detach path all left the suite green. Both mutations are now caught. Tighten the cancel-exit bound in the pump regression test. It allowed attachStdinReaderStopTimeout+1s, which is looser than the backstop cleanupAttach actually enforces, so a pump exiting in (1s, 2s] passed the test while the reader still outlived the attach in production. Assert the real invariant: the pump exits before the backstop fires. Export PollFdReady and FlushInput from internal/tmux so both attach paths share one implementation rather than growing a second copy. Document why the WaitGroup in AttachWithOptions is never Wait()ed on: the SIGWINCH goroutine only exits via a defer that runs after cleanupAttach, and the cmd.Wait() goroutine can outlive a detach, so waiting would deadlock the first and stall the second. stdinReaderDone is the join that matters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpRhBnzjSCPBqhrEwKU8k2
📝 WalkthroughWalkthroughInteractive SSH and tmux attach input handling now uses interruptible polling, coordinated reader shutdown, ordered input flushing, and terminal reply quarantine. Tests cover cancellation, detach and switch keys, EOF, cleanup ordering, and wedged readers. ChangesAttach input lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SSHRunner.Attach
participant stdinReader
participant PTY
participant Terminal
SSHRunner.Attach->>stdinReader: start poll-based reader
stdinReader->>Terminal: poll stdin readiness
stdinReader->>PTY: forward input bytes
SSHRunner.Attach->>stdinReader: signal stop and await completion
SSHRunner.Attach->>Terminal: flush input and quarantine replies
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 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 |
Owner
|
Reviewed — this is strong work and the mechanism analysis is exactly right (blocking tty read → ErrNoDeadline → FIFO wakeup steals the keystroke). We reproduced your regression harness outside the repo: pre-fix it hung to the 1s backstop, post-fix it exited in 102ms. Thank you also for volunteering the still-unpinned list against your own interest. Three things before it lands:
Timely PR, by the way: we are actively working the same attach/detach path for latency at large fleet sizes (#1753, #1764), so this dovetails. |
|
👋 Thanks for the contribution — intake looks complete. Your PR body carries everything the maintainer's validation pipeline reads first: the problem, the reasoning, the human intent behind it, and an AI-disclosure. It will be applied, built, and tested against gate marker read: ai= |
![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
🧹 Nitpick comments (2)
internal/session/ssh.go (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse a shared quiescer instead of re-implementing the join/flush/quarantine sequence.
The ordering invariant (wait for reader → flush → quarantine) is covered by tests only for the tmux copy in
internal/tmux/pty.go; this hand-rolled duplicate can drift silently, and the mirrored constants at Lines 34-41 rely on a comment to stay in sync. ExportingquiesceAttachInput(and the two timing constants) frominternal/tmuxand calling it here keeps both attach paths on one tested implementation — this is also the follow-up requested on the PR.As per path instructions: "RemoteSession vs LocalSession parity".
♻️ Proposed shape
- close(stdinReaderStop) - select { - case <-stdinReaderDone: - case <-time.After(sshStdinReaderStopTimeout): - } - _ = tmux.FlushInput(int(os.Stdin.Fd())) // `#nosec` G115 -- fd is a small positive int - termreply.QuarantineFor(sshAttachReplyQuarantine) + close(stdinReaderStop) + tmux.QuiesceAttachInput( + stdinReaderDone, + tmux.AttachStdinReaderStopTimeout, + func() { _ = tmux.FlushInput(int(os.Stdin.Fd())) }, // `#nosec` G115 + func() { termreply.QuarantineFor(sshAttachReplyQuarantine) }, + )Also applies to: 421-431
🤖 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 `@internal/session/ssh.go` around lines 34 - 42, Replace the SSH-specific quiescer implementation and mirrored timing constants with the shared exported quiesceAttachInput implementation and timing constants from internal/tmux. Update the SSH attach path to call that shared helper, preserving the wait-for-reader, flush, and quarantine ordering and maintaining RemoteSession/LocalSession parity.Source: Path instructions
internal/tmux/pty.go (1)
54-67: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
PollFdReadyconflates "not ready" with "poll failed".Any non-EINTR error (e.g.
EBADFafter the fd is closed, orEINVAL) returnsfalseimmediately without consuming the timeout. Both callers (attachStdinPump.runand the SSH reader) thencontinue, producing a tight zero-delay spin until cancellation/stop. Consider returning the error, or at least having callers treat a persistent poll error as a terminal condition.♻️ Sketch
-func PollFdReady(fd int, timeout time.Duration) bool { +// PollFdReadyErr additionally reports poll failures so callers can stop +// instead of spinning. +func PollFdReadyErr(fd int, timeout time.Duration) (bool, error) { ms := int(timeout.Milliseconds()) if ms < 1 { ms = 1 } fds := []unix.PollFd{{Fd: int32(fd), Events: unix.POLLIN}} // `#nosec` G115 for { n, err := unix.Poll(fds, ms) if err == unix.EINTR { continue } - return err == nil && n > 0 + return err == nil && n > 0, err } }🤖 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 `@internal/tmux/pty.go` around lines 54 - 67, Update PollFdReady to preserve non-EINTR poll errors instead of collapsing them into false, and adjust attachStdinPump.run and the SSH reader to treat persistent poll failures as terminal conditions rather than immediately retrying. Continue retrying only after EINTR, while retaining the existing readiness and timeout behavior.
🤖 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 `@internal/tmux/pty.go`:
- Around line 648-661: Synchronize access to switchOutcome across the pump
goroutine and the return path, including the attachStdinReaderStopTimeout
backstop where quiesceAttachInput may return before the pump exits. Update the
switchOutcome write in the pump goroutine and its read near the function’s
return to use an atomic or mutex-guarded value, preserving the existing outcome
semantics.
---
Nitpick comments:
In `@internal/session/ssh.go`:
- Around line 34-42: Replace the SSH-specific quiescer implementation and
mirrored timing constants with the shared exported quiesceAttachInput
implementation and timing constants from internal/tmux. Update the SSH attach
path to call that shared helper, preserving the wait-for-reader, flush, and
quarantine ordering and maintaining RemoteSession/LocalSession parity.
In `@internal/tmux/pty.go`:
- Around line 54-67: Update PollFdReady to preserve non-EINTR poll errors
instead of collapsing them into false, and adjust attachStdinPump.run and the
SSH reader to treat persistent poll failures as terminal conditions rather than
immediately retrying. Continue retrying only after EINTR, while retaining the
existing readiness and timeout 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: CHILL
Plan: Pro Plus
Run ID: e6c98155-e0dd-485a-885b-0546654fe20b
📒 Files selected for processing (5)
internal/session/ssh.gointernal/tmux/attach_stdin_pump_test.gointernal/tmux/pty.gointernal/tmux/pty_flush_linux.gointernal/tmux/pty_flush_other.go
Comment on lines
648
to
661
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| buf := make([]byte, 32) | ||
| var replyFilter termreply.Filter | ||
| for { | ||
| n, err := os.Stdin.Read(buf) | ||
| if err != nil { | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| // Report stdin read error | ||
| select { | ||
| case ioErrors <- fmt.Errorf("stdin read error: %w", err): | ||
| default: | ||
| } | ||
| return | ||
| } | ||
|
|
||
| chunk := buf[:n] | ||
| // Always run the reply filter: escape-string replies (DCS/OSC/etc.) | ||
| // can arrive long after the initial quarantine (e.g. iTerm2 | ||
| // XTVERSION reply on window focus/resize — #731). `armed` stays | ||
| // gated to the quarantine window so generic CSI pass-through | ||
| // works for keyboard input outside it. | ||
| armed := time.Since(startTime) < attachReplyQuarantine | ||
| chunk = replyFilter.Consume(chunk, armed, false) | ||
| if len(chunk) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| // Check for the detach key and any session-switch keys anywhere in | ||
| // the input chunk. Some terminals coalesce reads, so these must not | ||
| // require a single-byte read. Handles raw byte, xterm | ||
| // modifyOtherKeys, and kitty CSI u encodings. | ||
| // Whichever interrupt key appears first in the buffer wins, with | ||
| // detach > switch > scrollback precedence on a tie. Resolved by a | ||
| // pure helper so the precedence is unit-testable. | ||
| interruptIdx, outcome := resolveAttachInterrupt(chunk, detach, opts) | ||
|
|
||
| if interruptIdx >= 0 { | ||
| // Forward any bytes before the interrupt key, then stop. | ||
| if interruptIdx > 0 { | ||
| if _, err := ptmx.Write(chunk[:interruptIdx]); err != nil { | ||
| select { | ||
| case ioErrors <- fmt.Errorf("PTY write error: %w", err): | ||
| default: | ||
| } | ||
| return | ||
| } | ||
| } | ||
| switchOutcome = outcome | ||
| close(detachCh) | ||
| cancel() | ||
| return | ||
| } | ||
|
|
||
| // Forward other input to tmux PTY | ||
| if _, err := ptmx.Write(chunk); err != nil { | ||
| // Report PTY write error | ||
| select { | ||
| case ioErrors <- fmt.Errorf("PTY write error: %w", err): | ||
| default: | ||
| } | ||
| return | ||
| } | ||
| defer close(stdinReaderDone) | ||
| outcome, interrupted := pump.run(ctx) | ||
| if !interrupted { | ||
| return | ||
| } | ||
| // Write switchOutcome before closing detachCh: the close establishes | ||
| // the happens-before edge the main goroutine relies on after <-detachCh. | ||
| switchOutcome = outcome | ||
| close(detachCh) | ||
| cancel() | ||
| }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
switchOutcome is read without synchronization on the backstop path.
The happens-before edge for switchOutcome comes from either close(detachCh) or close(stdinReaderDone). If quiesceAttachInput gives up at attachStdinReaderStopTimeout and the pump then writes switchOutcome and closes detachCh, the return switchOutcome at Line 736 races with that write. Rare (requires a wedged/slow reader), but this package runs under -race in CI. Making switchOutcome an atomic.Int32/mutex-guarded value, or only reading it when quiesceAttachInput reports exited, closes the hole.
As per path instructions: "race conditions (this code runs with -race in CI)".
🤖 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 `@internal/tmux/pty.go` around lines 648 - 661, Synchronize access to
switchOutcome across the pump goroutine and the return path, including the
attachStdinReaderStopTimeout backstop where quiesceAttachInput may return before
the pump exits. Update the switchOutcome write in the pump goroutine and its
read near the function’s return to use an atomic or mutex-guarded value,
preserving the existing outcome semantics.
Source: Path instructions
…path Addresses review point 2 from @asheshgoplani on asheshgoplani#1773. The SSH teardown re-implemented the join -> flush -> quarantine sequence inline, so it inherited none of the mutation-checked tests that protect that ordering, and the two timing constants it used were hand-mirrored from internal/tmux behind a keep-in-sync comment. Both the inline copy and the mirrored constants were introduced by this PR's own a99b301, so this removes duplication the PR created rather than pre-existing code. Export QuiesceAttachInput, AttachStdinPollInterval and AttachStdinReaderStopTimeout from internal/tmux and have ssh.go call them. The ordering invariant now has one implementation, one set of constants, and one set of tests covering both attach paths. Exporting the two constants goes slightly beyond the literal request (export the function): it trades three exported symbols for removing the drift risk the review flagged. Happy to narrow it to the function alone if the wider surface is not wanted. Behaviour is unchanged - same ordering, same 100ms poll and 1s backstop. Verified by hand on both attach paths after the refactor: local session exit and SSH session exit each register the first keypress on return, and Ctrl+Q detach is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LpRhBnzjSCPBqhrEwKU8k2
github-actions
Bot
added
intake:clean
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this solve?
After an attached session's process exits — you type
exit, or quit Claude — the first keypress back on the deck is silently discarded. The second and everything after it work normally. Detaching with the detach key never shows it. Fixes #1783.AttachWithOptions' stdin-reader goroutine had four exits: EOF, a read error, a PTY write error, or seeing the detach key itself. That covers the detach path — the reader is what detects the detach key — but not the<-cmdDonepath, where the pane process exits and nothing pressed anything. The reader stayed parked in a blockingos.Stdin.Readthatcancel()cannot interrupt: a tty inherited from the shell is left in blocking mode, so Go never registers fd 0 with the netpoller andSetReadDeadlinereturnsErrNoDeadline.AttachWithOptionsreturned regardless, because itssync.WaitGroupwas populated four times and never waited on.Two readers were then blocked on the same tty. They wake in FIFO order, so the stale one — which had waited longer — won the next keystroke, forwarded it to the now-closed PTY, hit the write error and died. Exactly one key lost, then normal behaviour.
SSHRunner.Attachhad the identical shape and the identical symptom.Why this change
The read has to become abandonable, and
SetReadDeadlineis unavailable for the reason above, so the reader polls the descriptor (100ms) and re-checks its stop signal between polls. Polling adds no keystroke latency —pollreturns immediately when input is available — and costs ~10 idle wakeups/sec while attached, against the 4/sec the badge watcher already runs.The teardown is extracted into
QuiesceAttachInput, which joins the reader and then flushes the input queue and arms the reply quarantine. Both halves of that order are load-bearing: returning to Bubble Tea with the reader alive loses a keypress, and flushing before it stops just lets it consume the post-flush bytes. It takesflush/quarantineas parameters so the ordering is testable without a live tty, and both attach paths call it rather than keeping a second hand-written copy.User impact
The first keypress after a session ends now registers, on both local and SSH attach.
One deliberate behaviour change, called out because it is not purely a fix:
flushDetachInput+termreply.QuarantineForpreviously ran only on the detach path (if didDetach). On the process-exit path the stale reader was incidentally swallowing the terminal's capability replies. With the reader stopping cleanly that accidental shield is gone, so the flush must now run on every attach exit — otherwise those bytes surface in the TUI as literal fragments (terminal version strings,rgb:payloads). The consequence is that a key typed in the short window betweenexitand the flush is still lost, now discarded byTCIFLUSHrather than by a zombie reader. That trade and its residual window are documented in #1783 so it is not re-reported as the same bug.Evidence
The regression test fails against the pre-fix code and passes after. Reverting
run()to the blocking read:Teardown latency after the fix (
-count=5), well inside the 1s backstopcleanupAttachenforces:Teardown ordering is mutation-checked. Moving the flush ahead of the reader join, and re-gating the flush so it only runs when the reader exited, are both caught:
Hand-verified on real sessions, both paths. Local: attach,
exit, first keypress on the deck registers. SSH: same result throughSSHRunner.Attach, exercised against a loopback remote (agent-deck remote addatlocalhostunder a separate profile so the remote's state DB did not collide with the local one). Ctrl+Q detach re-checked on both afterward — first keypress registers, no stray capability-reply fragments in the UI.Lint parity:
golangci-lint run ./internal/tmux/... ./internal/session/...reports exactly the same 4 pre-existing gosec findings as the base commit, none new.Sandboxed suite: does not pass on macOS, and fails identically on the base commit. I am leaving that checklist box unchecked rather than checking it on a technicality.
HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./...exits 1 on this branch and on an unmodified checkout of0fc83cd:0fc83cdTestIssue1421_CleanStaleSSHSocketsTestTestMainDoesNotLeakBootstrapServerTestTmuxBootstrap_ServerIsRunning…PreservesLiveSiblingtmux testTestKillStaleControlClients_…TestPipeManager_Connect…TestIssue1421isbind: invalid argument— macOS's 104-bytesun_pathlimit against the long sandboxed$TMPDIR. The others are load-sensitive wall-clock assertions: run sandboxed in isolation,internal/tmuxandinternal/testutilpass on both trees, and the fourth row lands on a different test each run, which is contention rather than a defect. Linux CI runs the same suite green (14/14 on2099367). Flagging in case the macOSsun_pathfailure is news — it is fully reproducible and independent of this PR.AI disclosure
Model(s), if AI helped: claude-opus-5
What actually bothered you
My human asked: "It seems like after I end a Claude session in agent-deck, the first key press when I'm returned back to the main agent-deck view is ignored. Can you look into this?"
He then hand-verified the fix on both attach paths himself, including standing up a loopback SSH remote specifically so the SSH half would not ship on reasoning alone.
Checklist
HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./...— not checked: exits 1 on macOS, with identical failures on the base commit. Breakdown in Evidence above. Green on Linux CI.