fix: hold terminal input until a fresh pane's agent TUI is ready#3105
fix: hold terminal input until a fresh pane's agent TUI is ready#3105ikeshavvarshney wants to merge 2 commits into
Conversation
tmux forwards keystrokes to a pane's foreground process the instant attach succeeds, whether or not that process has entered raw mode yet. A full-screen TUI agent (Claude Code, Codex, ...) can take a moment after launch to do so, so fast typing right after a session opens lands as raw, echoed text in the scrollback instead of the agent's input box. Hold input on a pane's first-ever attach for a short grace window, buffering keystrokes via the existing pendingInput mechanism instead of forwarding them early. Only a pane's genuine first attach waits; reattaching to an already-running pane or a second client is unaffected. Fixes AgentWrapper#3023
|
Thanks for contributing to Agent Orchestrator. This PR is being picked up by the current external contributor on-call pair: If someone is already working on this, please continue as usual. For faster context or live questions, you can also join the AO Discord. Join the session here: Come by if you want to see what is being built, ask questions, or just hang around with the community. |
Addresses review on the terminal input-hold fix (AgentWrapper#3023): - The hold now starts on a pane's first SUCCESSFUL PTY publish instead of at open-request time. A shared one-shot inputGate per pane id replaces the eager deadline: only setPTY's own successful attach can arm it, so a slow Attach or a failed-then-retried attach can no longer let the window expire before the pane is even up. - setPTY now publishes the Stream and returns immediately so copyOut starts reading output right away; releasing buffered input happens on a separate goroutine gated on the shared inputGate, so a boot-time output burst is no longer held hostage behind the input hold. The release goroutine checks the Stream is still current before flushing, so a stale waiter can't write into a Stream a later reattach replaced. - The gate is kept for the Manager's lifetime instead of being time-pruned, so reopening a long-running pane can never be mistaken for a fresh one and re-delayed. Adds regression tests for a slow attach, a failed-then-successful attach, output streaming during the hold, and a reattach long after the window elapsed.
|
Fixed all three. Gate redesigned per your suggestion: shared one-shot inputGate per pane id, armed only by setPTY's own first successful publish — so a slow Attach or a failed-then-retried attach can no longer let the window expire early or start prematurely. setPTY now returns immediately after publishing so copyOut streams output right away; input release moved to a background goroutine gated on inputGate, checking a.pty == p before flushing so a stale waiter can't write into a superseded Stream. Gate is kept for the Manager's lifetime, no more time-pruning, so a long-running reattach never gets re-delayed. Added the four regression tests requested: slow-attach (window measured from attach success, not open request), failed-then-successful attach, output-not-stalled-during-hold, and no-new-delay-on-late-reattach. Full suite (38 tests) passes, gofmt clean, and re-verified live in a real Claude Code session — no leaked/garbled boot text, output streamed immediately. |
| // gets the SAME gate back and shares its one-shot timer (see inputGate) — | ||
| // callers still must call gate.start on their own successful attach, since | ||
| // only the pane's first successful publish may actually arm it. | ||
| func (m *Manager) inputGateFor(id string) *inputGate { |
There was a problem hiding this comment.
High — key/reset this gate by runtime incarnation, not only terminal ID.
ResumeAgentWithMode deliberately respawns the agent while preserving the existing runtime handle, and tmux Restart preserves attached clients. After the original gate has opened, those existing attachments remain inputReady and later opens receive this same permanently-open gate. Typing while the replacement TUI enters raw mode can therefore reproduce #3023 on resume.
Please associate the gate with a launch/runtime generation (or explicitly reset it from the restart lifecycle) so a genuinely new agent process on the same handle receives a fresh gate while ordinary reattaches to the same process do not. A regression should open a pane, let its gate expire, restart the agent on the same handle, and prove immediate input is buffered for the replacement TUI.
| a.mu.Unlock() | ||
| return | ||
| } | ||
| pending := append([][]byte(nil), a.pendingInput...) |
There was a problem hiding this comment.
Medium — ownership can change after this queue is removed, so a stale drain can still write to the superseded Stream.
The a.pty == p check occurs before the complete pending queue is copied locally. If chunk A is written to p1, copyOut(p1) then drops and publishes p2, this goroutine can resume and write chunk B to detached p1 (or lose it on error). Input subsequently released through p2 can overtake B, violating the stale-waiter and byte-order guarantees.
Please make release ownership generation-aware throughout the drain. If the PTY changes, preserve the current and remaining chunks at the front of the queue for the new Stream instead of continuing on the old one. Add a deterministic test that replaces p1 between two buffered writes and asserts A/B ordering on the surviving attachment.
| } | ||
|
|
||
| // wait blocks until the gate opens or ctx ends, whichever comes first. | ||
| func (g *inputGate) wait(ctx context.Context) { |
There was a problem hiding this comment.
Medium — cancellation is currently indistinguishable from the gate opening.
wait returns on either branch without reporting which one fired, and releaseInputWhenGateOpens proceeds to flush whenever the attachment still looks current. Manager.Close cancels the manager context before it marks each attachment closed, so the waiter can win that race and forward buffered keystrokes during shutdown; a blocking Write can also outlive the intended cancellation path.
Please return a boolean/error from wait (true only when g.ch opens) and abandon pending input on ctx.Done(). Add a close-during-hold regression that proves no bytes are written and shutdown completes promptly.
| a.mu.Unlock() | ||
|
|
||
| for _, chunk := range pending { | ||
| if _, err := p.Write(chunk); err != nil { |
There was a problem hiding this comment.
Medium — an asynchronous flush error emits exited but no longer terminates the Stream/run loop.
Before this refactor, a pending-input Write error made setPTY return false, so run closed p and returned. Here fail only marks the attachment exited and invokes onExit; it neither closes this Stream nor cancels run, which may remain blocked in copyOut(p). The UI can receive exited while the old PTY and manager-tracked goroutine continue emitting output, potentially interleaving with a reopened attachment.
Please close/cancel the identity-matched Stream when this async drain fails so copyOut unblocks and run actually exits. Cover it with a Stream whose Write fails while Read remains blocked, and assert the Stream closes and the attachment goroutine terminates.
| // releaseInput drains any input buffered while p was not yet accepting it and | ||
| // marks the attachment ready for direct writes. Called either immediately | ||
| // (grace window disabled) or once the pane's input gate opens. | ||
| func (a *attachment) releaseInput(p ports.Stream) { |
There was a problem hiding this comment.
Standards — pass context.Context into this blocking I/O helper.
releaseInput performs Stream.Write calls but has no context argument. AGENTS.md requires context.Context as the first argument for functions that perform I/O or blocking work.
Please change this to releaseInput(ctx context.Context, p ports.Stream) (and thread ctx through both callers), checking cancellation before draining each batch/write. This also gives the cancellation fix above a consistent boundary and should be covered by the close-during-hold test.
5 participants
Summary
Fixes #3023 — terminal accepted keystrokes as soon as
tmux attachsucceeded, before the launched agent's TUI had entered raw mode. Fast typing right after opening a fresh session landed as raw, echoed text in the pane's scrollback instead of the agent's input box, and was unrecoverable there.Root cause
Attach-readiness and agent-readiness are two different things.
tmuxforwards bytes to the pane's foreground process the moment attach succeeds — regardless of whether that process has actually read stdin yet. A full-screen TUI agent (Claude Code, Codex, ...) can take a moment after launch to enter raw mode, and until it does, the underlying line discipline echoes typed characters straight into the terminal's canonical-mode output.Fix
Hold input on a pane's first-ever attach for a short grace window (500ms), reusing the existing
pendingInputbuffering inattachment.write()— no new buffering logic needed. The window is tracked per pane id inManager(inputHoldDeadline), keyed off the first time this daemon process sees that id, so:WithInputGraceWindow(defaults to 500ms, tests disable it for determinism).Changes
backend/internal/terminal/manager.go—inputHoldDeadlinefirst-seen tracking per pane id,WithInputGraceWindowoption, default 500ms grace window.backend/internal/terminal/attachment.go—setPTYwaits out the hold (if any) on a pane's first attach before flippinginputReady, cancellable viactxso shutdown/close is never blocked.backend/internal/terminal/manager_test.go— two new tests (TestServeFreshPaneHoldsInputUntilGraceWindowElapses,TestServeReattachSkipsInputGraceWindow) plusWithInputGraceWindow(0)added to existing timing-sensitive tests to keep them deterministic.Test plan
go build ./...go test ./internal/terminal/...— full suite (33 tests) passes, including the two new grace-window tests.gofmtclean.