From 2ec1630d7f79587a08461e11ae07dc666f1e5792 Mon Sep 17 00:00:00 2001 From: Keshav Varshney Date: Sat, 25 Jul 2026 21:19:12 +0530 Subject: [PATCH 1/4] fix: hold terminal input until a fresh pane's agent TUI is ready 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 #3023 --- backend/internal/terminal/attachment.go | 24 ++++++- backend/internal/terminal/manager.go | 77 ++++++++++++++++++++--- backend/internal/terminal/manager_test.go | 67 +++++++++++++++++++- 3 files changed, 156 insertions(+), 12 deletions(-) diff --git a/backend/internal/terminal/attachment.go b/backend/internal/terminal/attachment.go index 221b4e1791..eb8fec0382 100644 --- a/backend/internal/terminal/attachment.go +++ b/backend/internal/terminal/attachment.go @@ -65,6 +65,12 @@ type attachment struct { opened bool inputReady bool pendingInput [][]byte + + // inputHoldUntil, when set by the Manager before run() starts, delays + // flipping inputReady on this attachment's first successful PTY (see + // setPTY) — giving a just-launched agent's TUI time to reach raw mode + // before trusting keystrokes to it. Zero value disables the hold. + inputHoldUntil time.Time } func newAttachment(id string, handle ports.RuntimeHandle, src Source, onOpen func(), onData func([]byte), onExit func(), log *slog.Logger) *attachment { @@ -153,7 +159,7 @@ func (a *attachment) run(ctx context.Context) { continue } - if !a.setPTY(p) { + if !a.setPTY(ctx, p) { _ = p.Close() return } @@ -271,7 +277,7 @@ func (a *attachment) size() (rows, cols uint16) { // requested size onto it (see resize) — the attach already started at the size // read in run, but a resize frame can land between that read and registration // here; the replay (Resize) converges the late case. -func (a *attachment) setPTY(p ports.Stream) bool { +func (a *attachment) setPTY(ctx context.Context, p ports.Stream) bool { a.mu.Lock() if a.closed || a.exited { a.mu.Unlock() @@ -285,6 +291,7 @@ func (a *attachment) setPTY(p ports.Stream) bool { a.opened = true } onOpen := a.onOpen + holdUntil := a.inputHoldUntil a.mu.Unlock() if rows > 0 && cols > 0 { _ = p.Resize(rows, cols) @@ -293,6 +300,19 @@ func (a *attachment) setPTY(p ports.Stream) bool { onOpen() } + // Only the very first attach of this attachment waits: output already + // streams (copyOut starts right after this returns), only accepting typed + // input is delayed, and pendingInput below already buffers anything that + // arrives during the wait, so nothing typed in this window is lost. + if shouldOpen { + if wait := time.Until(holdUntil); wait > 0 { + select { + case <-ctx.Done(): + case <-time.After(wait): + } + } + } + for { a.mu.Lock() pending := append([][]byte(nil), a.pendingInput...) diff --git a/backend/internal/terminal/manager.go b/backend/internal/terminal/manager.go index 5ac3a8e7f2..b2140450dc 100644 --- a/backend/internal/terminal/manager.go +++ b/backend/internal/terminal/manager.go @@ -32,6 +32,22 @@ type wsConn interface { const ( defaultHeartbeat = 15 * time.Second defaultWriteBuffer = 1024 + + // defaultInputGraceWindow is how long a freshly launched pane's typed input + // is held back after attach succeeds. tmux forwards bytes to the pane's + // foreground process the instant attach succeeds, whether or not that + // process has actually entered raw mode yet — a full-screen TUI agent + // (Claude Code, Codex, ...) can take a moment after launch to do so, during + // which fast typing lands as raw, echoed text in the scrollback instead of + // the agent's input box. The window only ever applies once, to the very + // first attach of a given pane id (see inputHoldDeadline); reattaching to an + // already-running pane never adds a delay. + defaultInputGraceWindow = 500 * time.Millisecond + + // freshEntryTTL bounds how long a recorded first-seen deadline is kept + // around purely so the bookkeeping map doesn't grow unboundedly over a + // long-lived daemon; it is unrelated to the grace window itself. + freshEntryTTL = time.Minute ) // Manager serves WebSocket clients, opening one attach Stream per opened pane @@ -57,6 +73,14 @@ type Manager struct { // It arbitrates the single PTY's grid across clients (see reconcileLocked). sharedMu sync.Mutex shared map[string]*sharedTerm + + // inputGraceWindow is how long a fresh pane's input is held (see + // defaultInputGraceWindow); zero disables the hold entirely. freshMu guards + // freshUntil, the per-id record of when that hold expires — populated the + // first time this Manager ever sees an id, reused on every later open. + inputGraceWindow time.Duration + freshMu sync.Mutex + freshUntil map[string]time.Time } // sharedTerm tracks every client currently viewing one terminal id (one PTY) so @@ -85,6 +109,12 @@ type Option func(*Manager) // WithHeartbeat overrides the ping interval. func WithHeartbeat(d time.Duration) Option { return func(m *Manager) { m.heartbeat = d } } +// WithInputGraceWindow overrides how long a freshly launched pane's input is +// held after attach (see defaultInputGraceWindow); zero disables it. +func WithInputGraceWindow(d time.Duration) Option { + return func(m *Manager) { m.inputGraceWindow = d } +} + // NewManager builds a Manager. src opens attach Streams; events feeds the session // channel (may be nil to disable it). A nil logger falls back to slog.Default. func NewManager(src Source, events EventSource, log *slog.Logger, opts ...Option) *Manager { @@ -93,14 +123,15 @@ func NewManager(src Source, events EventSource, log *slog.Logger, opts ...Option } ctx, cancel := context.WithCancel(context.Background()) m := &Manager{ - src: src, - events: events, - log: log, - heartbeat: defaultHeartbeat, - ctx: ctx, - cancel: cancel, - attachments: map[*attachment]struct{}{}, - shared: map[string]*sharedTerm{}, + src: src, + events: events, + log: log, + heartbeat: defaultHeartbeat, + ctx: ctx, + cancel: cancel, + attachments: map[*attachment]struct{}{}, + shared: map[string]*sharedTerm{}, + inputGraceWindow: defaultInputGraceWindow, } for _, opt := range opts { opt(m) @@ -148,6 +179,35 @@ func (m *Manager) forget(a *attachment) { m.mu.Unlock() } +// inputHoldDeadline returns the point in time until which id's input should be +// held back on its next first-ever attach. The first caller for a given id +// mints a fresh deadline (now + inputGraceWindow); every later call for the +// same id — a reattach, a second client, a reopened tab — gets back that same +// deadline, which by then has usually already passed, so only a pane's true +// first attach ever actually waits. Disabled entirely when inputGraceWindow<=0. +func (m *Manager) inputHoldDeadline(id string) time.Time { + if m.inputGraceWindow <= 0 { + return time.Time{} + } + now := time.Now() + m.freshMu.Lock() + defer m.freshMu.Unlock() + if m.freshUntil == nil { + m.freshUntil = map[string]time.Time{} + } + for k, v := range m.freshUntil { + if now.After(v.Add(freshEntryTTL)) { + delete(m.freshUntil, k) + } + } + if deadline, ok := m.freshUntil[id]; ok { + return deadline + } + deadline := now.Add(m.inputGraceWindow) + m.freshUntil[id] = deadline + return deadline +} + // joinTerminal registers a client (its connection + attach Stream + requested // grid + role) as a viewer of terminal id, then reconciles the shared grid. func (m *Manager) joinTerminal(id string, c *connState, att *attachment, cols, rows uint16, primary bool) { @@ -377,6 +437,7 @@ func (c *connState) openTerminal(id string, rows, cols uint16, role string) { c.enqueue(serverMsg{Ch: chTerminal, ID: id, Type: msgExited}) }, c.mgr.log) + a.inputHoldUntil = c.mgr.inputHoldDeadline(id) if err := c.mgr.track(a); err != nil { c.enqueue(serverMsg{Ch: chTerminal, ID: id, Type: msgError, Error: err.Error()}) return diff --git a/backend/internal/terminal/manager_test.go b/backend/internal/terminal/manager_test.go index 5af2032daf..25e07d2b12 100644 --- a/backend/internal/terminal/manager_test.go +++ b/backend/internal/terminal/manager_test.go @@ -72,7 +72,7 @@ func TestServeOpenStreamsAndWritesTerminal(t *testing.T) { pty := newFakePTY() sp := &fakeSpawner{ptys: []*fakePTY{pty}} src := &fakeSource{alive: true, spawner: sp} - mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0)) + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(0)) defer mgr.Close() conn := newFakeConn() @@ -109,7 +109,7 @@ func TestServeBuffersInputUntilAttachReady(t *testing.T) { <-releaseSpawn return pty, nil }} - mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0)) + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(0)) defer mgr.Close() conn := newFakeConn() @@ -454,6 +454,69 @@ func TestManagerCloseKillsLiveAttachments(t *testing.T) { } } +// A pane's very first attach holds typed input until its grace window elapses +// (see Manager.inputHoldDeadline): tmux forwards bytes to the pane's foreground +// process the instant attach succeeds, whether or not a full-screen TUI agent +// has actually entered raw mode yet, so an early keystroke can otherwise land +// as raw text in the pane's scrollback instead of the agent's input box. +func TestServeFreshPaneHoldsInputUntilGraceWindowElapses(t *testing.T) { + pty := newFakePTY() + sp := &fakeSpawner{ptys: []*fakePTY{pty}} + src := &fakeSource{alive: true, spawner: sp} + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(200*time.Millisecond)) + defer mgr.Close() + + conn := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go mgr.Serve(ctx, conn) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgOpened, time.Second) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgData, Data: base64.StdEncoding.EncodeToString([]byte("h"))} + + // Give the write a real chance to land early; it must not reach the PTY + // before the grace window elapses. + time.Sleep(80 * time.Millisecond) + if got := string(pty.writtenBytes()); got != "" { + t.Fatalf("input reached the PTY before the grace window elapsed: %q", got) + } + + eventually(t, time.Second, func() bool { return string(pty.writtenBytes()) == "h" }) +} + +// A second open of a pane this Manager has already seen (a reattach, or a +// second client) must not add another delay: the grace window only protects a +// pane's genuine first attach, when the agent process is actually still +// booting. +func TestServeReattachSkipsInputGraceWindow(t *testing.T) { + p1, p2 := newFakePTY(), newFakePTY() + sp := &fakeSpawner{ptys: []*fakePTY{p1, p2}} + src := &fakeSource{alive: true, spawner: sp} + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(150*time.Millisecond)) + defer mgr.Close() + + connA := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go mgr.Serve(ctx, connA) + + connA.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, connA, chTerminal, msgOpened, time.Second) + + // Let connA's fresh-open grace window elapse. + time.Sleep(200 * time.Millisecond) + + connB := newFakeConn() + go mgr.Serve(ctx, connB) + connB.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, connB, chTerminal, msgOpened, time.Second) + + connB.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgData, Data: base64.StdEncoding.EncodeToString([]byte("status\n"))} + eventually(t, 200*time.Millisecond, func() bool { return string(p2.writtenBytes()) == "status\n" }) +} + func TestEnqueueOverflowCancelsConn(t *testing.T) { cancelled := make(chan struct{}) c := &connState{ From bdcfc8f1a7da25dc759c2460e46abf3a02a7e159 Mon Sep 17 00:00:00 2001 From: Keshav Varshney Date: Sun, 26 Jul 2026 23:03:44 +0530 Subject: [PATCH 2/4] fix: anchor input grace window to first successful attach, not output Addresses review on the terminal input-hold fix (#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. --- backend/internal/terminal/attachment.go | 70 ++++++++---- backend/internal/terminal/manager.go | 110 +++++++++++-------- backend/internal/terminal/manager_test.go | 123 +++++++++++++++++++++- 3 files changed, 239 insertions(+), 64 deletions(-) diff --git a/backend/internal/terminal/attachment.go b/backend/internal/terminal/attachment.go index eb8fec0382..cdb04b4ac7 100644 --- a/backend/internal/terminal/attachment.go +++ b/backend/internal/terminal/attachment.go @@ -66,11 +66,13 @@ type attachment struct { inputReady bool pendingInput [][]byte - // inputHoldUntil, when set by the Manager before run() starts, delays - // flipping inputReady on this attachment's first successful PTY (see - // setPTY) — giving a just-launched agent's TUI time to reach raw mode - // before trusting keystrokes to it. Zero value disables the hold. - inputHoldUntil time.Time + // inputGate, when set by the Manager before run() starts, delays flipping + // inputReady until the pane's shared gate opens (see setPTY) — giving a + // just-launched agent's TUI time to reach raw mode before trusting + // keystrokes to it. nil disables the hold entirely (e.g. attachments built + // directly in tests, bypassing Manager). + inputGate *inputGate + inputGraceWindow time.Duration } func newAttachment(id string, handle ports.RuntimeHandle, src Source, onOpen func(), onData func([]byte), onExit func(), log *slog.Logger) *attachment { @@ -277,6 +279,15 @@ func (a *attachment) size() (rows, cols uint16) { // requested size onto it (see resize) — the attach already started at the size // read in run, but a resize frame can land between that read and registration // here; the replay (Resize) converges the late case. +// +// It returns as soon as the Stream is published so run's copyOut starts +// reading output immediately — the pane's boot output (attach handshake, +// repaint) must never wait on the input hold below. Releasing buffered input +// happens on a separate goroutine gated on inputGate, which is shared across +// every attach of this pane (see Manager.inputGateFor): the gate's timer only +// ever starts on the pane's first successful publish, not on this attempt's +// open request, so a slow Attach or a failed-then-retried attach never lets +// the hold expire early or double-count. func (a *attachment) setPTY(ctx context.Context, p ports.Stream) bool { a.mu.Lock() if a.closed || a.exited { @@ -291,7 +302,8 @@ func (a *attachment) setPTY(ctx context.Context, p ports.Stream) bool { a.opened = true } onOpen := a.onOpen - holdUntil := a.inputHoldUntil + gate := a.inputGate + window := a.inputGraceWindow a.mu.Unlock() if rows > 0 && cols > 0 { _ = p.Resize(rows, cols) @@ -300,34 +312,54 @@ func (a *attachment) setPTY(ctx context.Context, p ports.Stream) bool { onOpen() } - // Only the very first attach of this attachment waits: output already - // streams (copyOut starts right after this returns), only accepting typed - // input is delayed, and pendingInput below already buffers anything that - // arrives during the wait, so nothing typed in this window is lost. - if shouldOpen { - if wait := time.Until(holdUntil); wait > 0 { - select { - case <-ctx.Done(): - case <-time.After(wait): - } - } + if gate == nil { + a.releaseInput(p) + return true + } + gate.start(window) + go a.releaseInputWhenGateOpens(ctx, p, gate) + return true +} + +// releaseInputWhenGateOpens waits for the pane's shared gate to open (or ctx +// to end, e.g. daemon shutdown) and then releases input for Stream p — unless +// p has since been superseded by a later reattach (a.pty != p), in which case +// this stale waiter abandons silently instead of flushing into a Stream this +// attachment no longer owns. +func (a *attachment) releaseInputWhenGateOpens(ctx context.Context, p ports.Stream, gate *inputGate) { + gate.wait(ctx) + a.mu.Lock() + current := a.pty == p && !a.closed && !a.exited + a.mu.Unlock() + if !current { + return } + a.releaseInput(p) +} +// 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) { for { a.mu.Lock() + if a.closed || a.exited || a.pty != p { + a.mu.Unlock() + return + } pending := append([][]byte(nil), a.pendingInput...) a.pendingInput = nil if len(pending) == 0 { a.inputReady = true a.mu.Unlock() - return true + return } a.mu.Unlock() for _, chunk := range pending { if _, err := p.Write(chunk); err != nil { a.fail("flush pending input: " + err.Error()) - return false + return } } } diff --git a/backend/internal/terminal/manager.go b/backend/internal/terminal/manager.go index b2140450dc..83fee04009 100644 --- a/backend/internal/terminal/manager.go +++ b/backend/internal/terminal/manager.go @@ -34,20 +34,15 @@ const ( defaultWriteBuffer = 1024 // defaultInputGraceWindow is how long a freshly launched pane's typed input - // is held back after attach succeeds. tmux forwards bytes to the pane's - // foreground process the instant attach succeeds, whether or not that - // process has actually entered raw mode yet — a full-screen TUI agent + // is held back after its first successful attach. tmux forwards bytes to + // the pane's foreground process the instant attach succeeds, whether or not + // that process has actually entered raw mode yet — a full-screen TUI agent // (Claude Code, Codex, ...) can take a moment after launch to do so, during // which fast typing lands as raw, echoed text in the scrollback instead of - // the agent's input box. The window only ever applies once, to the very - // first attach of a given pane id (see inputHoldDeadline); reattaching to an + // the agent's input box. The window only ever starts once, on the pane's + // first successful publish (see inputGate); reattaching to an // already-running pane never adds a delay. defaultInputGraceWindow = 500 * time.Millisecond - - // freshEntryTTL bounds how long a recorded first-seen deadline is kept - // around purely so the bookkeeping map doesn't grow unboundedly over a - // long-lived daemon; it is unrelated to the grace window itself. - freshEntryTTL = time.Minute ) // Manager serves WebSocket clients, opening one attach Stream per opened pane @@ -75,12 +70,50 @@ type Manager struct { shared map[string]*sharedTerm // inputGraceWindow is how long a fresh pane's input is held (see - // defaultInputGraceWindow); zero disables the hold entirely. freshMu guards - // freshUntil, the per-id record of when that hold expires — populated the - // first time this Manager ever sees an id, reused on every later open. + // defaultInputGraceWindow); zero disables the hold entirely. gatesMu guards + // gates, the per-id shared one-shot gate (see inputGate) — populated the + // first time this Manager ever sees an id and kept for the Manager's whole + // lifetime (deliberately not time-pruned: an id's gate records that the + // pane already had its one-time grace period, and a pane can legitimately + // stay open far longer than the grace window itself). inputGraceWindow time.Duration - freshMu sync.Mutex - freshUntil map[string]time.Time + gatesMu sync.Mutex + gates map[string]*inputGate +} + +// inputGate is a shared, one-shot "pane is old enough to trust with input" +// signal: every attachment for a given pane id (across reattaches and +// multiple clients) waits on the SAME gate, but only the first successful +// attach actually starts its timer (see start). That means a slow Attach or a +// failed-then-retried attach never lets the hold expire before the pane is +// even up, and a reattach or a second client never restarts or re-adds delay. +type inputGate struct { + once sync.Once + ch chan struct{} +} + +func newInputGate() *inputGate { + return &inputGate{ch: make(chan struct{})} +} + +// start arms the gate's timer; only the very first call has any effect. d<=0 +// opens the gate immediately (grace window disabled). +func (g *inputGate) start(d time.Duration) { + g.once.Do(func() { + if d <= 0 { + close(g.ch) + return + } + time.AfterFunc(d, func() { close(g.ch) }) + }) +} + +// wait blocks until the gate opens or ctx ends, whichever comes first. +func (g *inputGate) wait(ctx context.Context) { + select { + case <-g.ch: + case <-ctx.Done(): + } } // sharedTerm tracks every client currently viewing one terminal id (one PTY) so @@ -179,33 +212,23 @@ func (m *Manager) forget(a *attachment) { m.mu.Unlock() } -// inputHoldDeadline returns the point in time until which id's input should be -// held back on its next first-ever attach. The first caller for a given id -// mints a fresh deadline (now + inputGraceWindow); every later call for the -// same id — a reattach, a second client, a reopened tab — gets back that same -// deadline, which by then has usually already passed, so only a pane's true -// first attach ever actually waits. Disabled entirely when inputGraceWindow<=0. -func (m *Manager) inputHoldDeadline(id string) time.Time { - if m.inputGraceWindow <= 0 { - return time.Time{} - } - now := time.Now() - m.freshMu.Lock() - defer m.freshMu.Unlock() - if m.freshUntil == nil { - m.freshUntil = map[string]time.Time{} - } - for k, v := range m.freshUntil { - if now.After(v.Add(freshEntryTTL)) { - delete(m.freshUntil, k) - } - } - if deadline, ok := m.freshUntil[id]; ok { - return deadline - } - deadline := now.Add(m.inputGraceWindow) - m.freshUntil[id] = deadline - return deadline +// inputGateFor returns the shared input gate for pane id, creating it on the +// first call. Every attachment for this id (every reattach, every client) +// 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 { + m.gatesMu.Lock() + defer m.gatesMu.Unlock() + if m.gates == nil { + m.gates = map[string]*inputGate{} + } + g, ok := m.gates[id] + if !ok { + g = newInputGate() + m.gates[id] = g + } + return g } // joinTerminal registers a client (its connection + attach Stream + requested @@ -437,7 +460,8 @@ func (c *connState) openTerminal(id string, rows, cols uint16, role string) { c.enqueue(serverMsg{Ch: chTerminal, ID: id, Type: msgExited}) }, c.mgr.log) - a.inputHoldUntil = c.mgr.inputHoldDeadline(id) + a.inputGate = c.mgr.inputGateFor(id) + a.inputGraceWindow = c.mgr.inputGraceWindow if err := c.mgr.track(a); err != nil { c.enqueue(serverMsg{Ch: chTerminal, ID: id, Type: msgError, Error: err.Error()}) return diff --git a/backend/internal/terminal/manager_test.go b/backend/internal/terminal/manager_test.go index 25e07d2b12..6d54054784 100644 --- a/backend/internal/terminal/manager_test.go +++ b/backend/internal/terminal/manager_test.go @@ -455,7 +455,7 @@ func TestManagerCloseKillsLiveAttachments(t *testing.T) { } // A pane's very first attach holds typed input until its grace window elapses -// (see Manager.inputHoldDeadline): tmux forwards bytes to the pane's foreground +// (see Manager.inputGateFor): tmux forwards bytes to the pane's foreground // process the instant attach succeeds, whether or not a full-screen TUI agent // has actually entered raw mode yet, so an early keystroke can otherwise land // as raw text in the pane's scrollback instead of the agent's input box. @@ -514,7 +514,126 @@ func TestServeReattachSkipsInputGraceWindow(t *testing.T) { recv(t, connB, chTerminal, msgOpened, time.Second) connB.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgData, Data: base64.StdEncoding.EncodeToString([]byte("status\n"))} - eventually(t, 200*time.Millisecond, func() bool { return string(p2.writtenBytes()) == "status\n" }) + // Tight budget, well under the 150ms window: proves no new delay was added, + // not just that one eventually arrived. + eventually(t, 50*time.Millisecond, func() bool { return string(p2.writtenBytes()) == "status\n" }) +} + +// The grace window must be measured from the pane's first SUCCESSFUL attach, +// not from the open request that preceded it. A deadline computed at open time +// would already have expired by the time a slow Attach returns, letting the +// original early-input race back in. +func TestServeInputGraceWindowStartsOnFirstSuccessfulAttach(t *testing.T) { + pty := newFakePTY() + releaseAttach := make(chan struct{}) + attachStarted := make(chan struct{}) + src := &fakeSource{alive: true, attachFn: func(context.Context, uint16, uint16) (ports.Stream, error) { + close(attachStarted) + <-releaseAttach + return pty, nil + }} + window := 150 * time.Millisecond + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(window)) + defer mgr.Close() + + conn := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go mgr.Serve(ctx, conn) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + select { + case <-attachStarted: + case <-time.After(time.Second): + t.Fatal("attach was not reached") + } + + // Hold Attach in flight for longer than the grace window itself. If the + // window were (mis)measured from the open request instead of from the + // successful attach, it would already be expired by the time the PTY + // publishes, and the input below would reach it immediately. + time.Sleep(window + 100*time.Millisecond) + close(releaseAttach) + + recv(t, conn, chTerminal, msgOpened, time.Second) + attachReturnedAt := time.Now() + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgData, Data: base64.StdEncoding.EncodeToString([]byte("x"))} + + time.Sleep(window / 2) + if got := string(pty.writtenBytes()); got != "" { + t.Fatalf("input reached the PTY before the grace window (measured from attach success) elapsed: %q", got) + } + + eventually(t, time.Second, func() bool { return string(pty.writtenBytes()) == "x" }) + if elapsed := time.Since(attachReturnedAt); elapsed < window { + t.Fatalf("input released after only %s since attach success, want at least the %s grace window", elapsed, window) + } +} + +// A failed or dead first open must not start (or otherwise consume) the +// pane's input gate -- only the pane's first SUCCESSFUL attach may arm it. +func TestServeInputGraceWindowIgnoresFailedFirstAttach(t *testing.T) { + pty := newFakePTY() + sp := &fakeSpawner{ptys: []*fakePTY{pty}} + src := &fakeSource{alive: false, spawner: sp} + window := 150 * time.Millisecond + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(window)) + defer mgr.Close() + + conn := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go mgr.Serve(ctx, conn) + + // First open: the runtime is dead, so it exits without ever attaching. + // This must not start the pane's input gate. + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgExited, time.Second) + + // Time passes before the retry -- long enough that a wrongly-started gate + // would already be open by now. + time.Sleep(window + 100*time.Millisecond) + + src.setAlive(true) + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgOpened, time.Second) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgData, Data: base64.StdEncoding.EncodeToString([]byte("y"))} + time.Sleep(window / 2) + if got := string(pty.writtenBytes()); got != "" { + t.Fatalf("input reached the PTY before the grace window on the first SUCCESSFUL attach elapsed: %q", got) + } + eventually(t, time.Second, func() bool { return string(pty.writtenBytes()) == "y" }) +} + +// Output must stream to the client immediately, even while input is still +// held: setPTY publishes the Stream and returns before the hold is applied, so +// run's copyOut starts reading right away instead of waiting behind it. +func TestServeOutputDuringHoldReachesClientBeforeInputFlushes(t *testing.T) { + pty := newFakePTY() + sp := &fakeSpawner{ptys: []*fakePTY{pty}} + src := &fakeSource{alive: true, spawner: sp} + window := 300 * time.Millisecond + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(window)) + defer mgr.Close() + + conn := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go mgr.Serve(ctx, conn) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgOpened, time.Second) + + // Push boot output immediately, well within the still-open hold window -- + // it must reach the client right away, not wait behind the input hold. + pty.push([]byte("booting...")) + data := recv(t, conn, chTerminal, msgData, window/2) + got, _ := base64.StdEncoding.DecodeString(data.Data) + if string(got) != "booting..." { + t.Fatalf("streamed data = %q", got) + } } func TestEnqueueOverflowCancelsConn(t *testing.T) { From 106e011737015455410143fdcdfffa5b6bacb5cf Mon Sep 17 00:00:00 2001 From: Keshav Varshney Date: Mon, 27 Jul 2026 16:49:33 +0530 Subject: [PATCH 3/4] fix: make the input gate generation-safe, ordering-safe, and cancellation-safe Addresses a second review round on the terminal input-hold fix (#3023): - Reset the pane's shared input gate on every genuine (re)launch of its agent process (terminal.Manager.ResetInputGate), wired through a new session_manager.InputGateResetter hook called from relaunchSession. Without this, a resume/restart that reuses the runtime handle (ports.RuntimeRestarter) would inherit the exited process's already-open gate and let the replacement TUI's early keystrokes through unheld, reproducing the original race on every resume. - releaseInput now re-reads the live Stream fresh before every single buffered write instead of holding one Stream reference for the whole drain, guarded by a draining flag so only one drain loop runs per attachment at a time. A Stream swap mid-drain (a reattach racing the release) now hands the remainder to whichever Stream is current, instead of risking a write into one this attachment no longer owns. - inputGate.wait now reports whether the gate actually opened or ctx ended first; releaseInputWhenGateOpens abandons on the latter. Manager ../Close cancels its context before marking attachments closed, so a waiter could previously flush buffered keystrokes mid-shutdown. - A genuine (still-current) write failure during the async flush now cancels the attach loop and closes the Stream, so run's blocked copyOut unblocks and the loop actually exits instead of treating a dead Stream as an ordinary drop-and-reattach case. - releaseInput takes a context.Context (AGENTS.md: ctx first-arg for I/O/blocking work) and checks cancellation before draining each chunk. Adds regression tests for: a resume resetting the gate, a Stream swap mid-drain preserving order across the two Streams, Manager.Close abandoning a mid-hold flush promptly, and an async flush failure closing its Stream and ending the attach loop. --- backend/internal/daemon/daemon.go | 4 + backend/internal/daemon/lifecycle_wiring.go | 1 + backend/internal/daemon/wiring_test.go | 2 + backend/internal/session_manager/manager.go | 48 ++++++++++ .../internal/session_manager/manager_test.go | 32 +++++++ backend/internal/terminal/attachment.go | 96 ++++++++++++++----- backend/internal/terminal/attachment_test.go | 90 +++++++++++++++++ backend/internal/terminal/fakes_test.go | 13 +++ backend/internal/terminal/manager.go | 25 ++++- backend/internal/terminal/manager_test.go | 80 ++++++++++++++++ 10 files changed, 363 insertions(+), 28 deletions(-) diff --git a/backend/internal/daemon/daemon.go b/backend/internal/daemon/daemon.go index 19294ae510..8eca36fb38 100644 --- a/backend/internal/daemon/daemon.go +++ b/backend/internal/daemon/daemon.go @@ -156,6 +156,10 @@ func Run() error { return fmt.Errorf("wire session service: %w", err) } lcStack.LCM.SetCompletionTerminator(sessMgr) + // Lets a resume/restart that reuses a runtime handle reset that pane's + // input-grace-period gate (see terminal.Manager.ResetInputGate) instead of + // inheriting the exited process's already-open gate. + sessMgr.SetInputGateResetter(termMgr) lcStack.scmDone = startSCMObserver(ctx, store, lcStack.LCM, log) projectSvc := projectsvc.NewWithDeps(projectsvc.Deps{Store: store, Sessions: sessionSvc, DefaultHarness: domain.AgentHarness(cfg.Agent), Telemetry: telemetrySink}) if err := seedScratchProjectOnBoot(ctx, cfg, projectSvc); err != nil { diff --git a/backend/internal/daemon/lifecycle_wiring.go b/backend/internal/daemon/lifecycle_wiring.go index c7d508a9d3..d999ac55cf 100644 --- a/backend/internal/daemon/lifecycle_wiring.go +++ b/backend/internal/daemon/lifecycle_wiring.go @@ -141,6 +141,7 @@ type sessionLifecycle interface { Reconcile(ctx context.Context) error RestoreAll(ctx context.Context) error Kill(ctx context.Context, id domain.SessionID) (bool, error) + SetInputGateResetter(r sessionmanager.InputGateResetter) } // startSession builds the controller-facing session service: a session manager diff --git a/backend/internal/daemon/wiring_test.go b/backend/internal/daemon/wiring_test.go index af2e05197b..7d1418b6ea 100644 --- a/backend/internal/daemon/wiring_test.go +++ b/backend/internal/daemon/wiring_test.go @@ -619,6 +619,8 @@ type fakeSessionLifecycle struct { restoreErr error } +func (f *fakeSessionLifecycle) SetInputGateResetter(_ sessionmanager.InputGateResetter) {} + func (f *fakeSessionLifecycle) Kill(_ context.Context, _ domain.SessionID) (bool, error) { return false, nil } diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 990d67d1c7..9a15e84805 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -185,6 +185,49 @@ type Manager struct { // sendConfirm* defaults; tests in this package shrink the timings directly. sendConfirm sendConfirmConfig logger *slog.Logger + + // inputGatesMu guards inputGates. It is late-bound (set after New via + // SetInputGateResetter) rather than threaded through every constructor + // call site, matching how this package already wires other optional + // cross-cutting collaborators post-construction (e.g. SetCompletionTerminator + // on the LCM in daemon wiring). nil is a valid, tested no-op. + inputGatesMu sync.Mutex + inputGates InputGateResetter +} + +// InputGateResetter drops a pane's remembered "already past its input grace +// period" state. terminal.Manager satisfies it; the interface lives here (the +// consumer) rather than in package terminal so this package does not import +// the terminal package for one method. +type InputGateResetter interface { + ResetInputGate(id string) +} + +// SetInputGateResetter wires relaunchSession to reset a pane's input gate on +// every genuine (re)launch of its agent process — see resetInputGate. Safe to +// leave unset: a nil resetter makes resetInputGate a no-op, matching every +// existing test in this package that does not construct one. +func (m *Manager) SetInputGateResetter(r InputGateResetter) { + m.inputGatesMu.Lock() + defer m.inputGatesMu.Unlock() + m.inputGates = r +} + +// resetInputGate tells the terminal layer that handleID's pane is about to run +// a brand new agent process, not merely being reattached to. Without this, a +// resume or restart that reuses the same runtime handle (ports.RuntimeRestarter) +// would find the pane's gate already open from the process that just exited, +// and the replacement TUI's early keystrokes would race raw-mode entry all +// over again — the exact bug this gate exists to prevent, just triggered by +// resume instead of a fresh spawn. +func (m *Manager) resetInputGate(handleID string) { + m.inputGatesMu.Lock() + r := m.inputGates + m.inputGatesMu.Unlock() + if r == nil { + return + } + r.ResetInputGate(handleID) } // sendConfirmConfig bounds the best-effort activity-confirmation loop run after @@ -1048,6 +1091,11 @@ func (m *Manager) relaunchSession(ctx context.Context, operation string, rec dom m.cleanupSystemPromptDir(rec.ID) return RestoreResult{}, fmt.Errorf("%s %s: runtime: %w", operation, rec.ID, err) } + // A brand new agent process is about to run on handle.ID, whether that + // meant a fresh Create or a Restart that reused the runtime handle — either + // way the pane's remembered input-grace-period state (if any) belongs to + // the process that just exited, not this one, and must not carry over. + m.resetInputGate(handle.ID) metadata := domain.SessionMetadata{ Branch: ws.Branch, WorkspacePath: ws.Path, diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index aba772f841..5faea18841 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -889,6 +889,38 @@ func TestResumeAgent_RestartsRuntimeWithManagedGeneration(t *testing.T) { } } +// fakeInputGateResetter records every ResetInputGate call so a test can +// assert relaunchSession tells the terminal layer a pane's remembered +// input-grace-period state no longer applies. +type fakeInputGateResetter struct { + resetIDs []string +} + +func (f *fakeInputGateResetter) ResetInputGate(id string) { + f.resetIDs = append(f.resetIDs, id) +} + +// A resume that reuses the runtime handle (tmux Restart, exercised by +// TestResumeAgent_RestartsRuntimeWithManagedGeneration above) must reset that +// handle's input gate: the gate, if already open, belongs to the process that +// just exited, and without this the replacement TUI's early keystrokes would +// race raw-mode entry all over again on every resume (issue #3023 recurring). +func TestResumeAgent_ResetsInputGateForReplacementProcess(t *testing.T) { + baseRuntime := &fakeRuntime{aliveByHandle: map[string]bool{"tmux-mer-1": true}} + runtime := &fakeRestartRuntime{fakeRuntime: baseRuntime} + agent := supervisedLaunchAgent{launchArgvAgent{argv: []string{"codex", "resume", "agent-x"}}} + m, _, _ := newExitedResumeManager(t, runtime, agent) + resetter := &fakeInputGateResetter{} + m.SetInputGateResetter(resetter) + + if _, err := m.ResumeAgentWithMode(ctx, "mer-1"); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(resetter.resetIDs, []string{"tmux-mer-1"}) { + t.Fatalf("input gate resets = %v, want exactly [tmux-mer-1]", resetter.resetIDs) + } +} + func TestResumeAgent_FallsBackToRuntimeRecreateWithoutRestartCapability(t *testing.T) { runtime := &fakeRuntime{aliveByHandle: map[string]bool{"tmux-mer-1": true}} agent := supervisedLaunchAgent{launchArgvAgent{argv: []string{"codex", "resume", "agent-x"}}} diff --git a/backend/internal/terminal/attachment.go b/backend/internal/terminal/attachment.go index cdb04b4ac7..cf63fb8fd1 100644 --- a/backend/internal/terminal/attachment.go +++ b/backend/internal/terminal/attachment.go @@ -65,6 +65,12 @@ type attachment struct { opened bool inputReady bool pendingInput [][]byte + // draining is true while a releaseInput loop is actively dequeuing and + // writing pendingInput. A second caller (e.g. a reattach's own setPTY) + // finds it true and does not start a competing loop — the active loop + // already re-reads a.pty fresh before every write, so it naturally hands + // remaining input to whatever Stream ends up current. + draining bool // inputGate, when set by the Manager before run() starts, delays flipping // inputReady until the pane's shared gate opens (see setPTY) — giving a @@ -313,55 +319,93 @@ func (a *attachment) setPTY(ctx context.Context, p ports.Stream) bool { } if gate == nil { - a.releaseInput(p) + a.releaseInput(ctx) return true } gate.start(window) - go a.releaseInputWhenGateOpens(ctx, p, gate) + go a.releaseInputWhenGateOpens(ctx, gate) return true } -// releaseInputWhenGateOpens waits for the pane's shared gate to open (or ctx -// to end, e.g. daemon shutdown) and then releases input for Stream p — unless -// p has since been superseded by a later reattach (a.pty != p), in which case -// this stale waiter abandons silently instead of flushing into a Stream this -// attachment no longer owns. -func (a *attachment) releaseInputWhenGateOpens(ctx context.Context, p ports.Stream, gate *inputGate) { - gate.wait(ctx) - a.mu.Lock() - current := a.pty == p && !a.closed && !a.exited - a.mu.Unlock() - if !current { +// releaseInputWhenGateOpens waits for the pane's shared gate to open. If ctx +// ends first (daemon shutdown, attachment closed) — wait reports which one +// actually happened — buffered input is abandoned rather than flushed: a +// waiter racing Manager.Close must never forward keystrokes after shutdown +// has started, even though Close cancels the context before it marks every +// attachment closed. +func (a *attachment) releaseInputWhenGateOpens(ctx context.Context, gate *inputGate) { + if !gate.wait(ctx) { return } - a.releaseInput(p) + a.releaseInput(ctx) } -// 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) { +// releaseInput drains pendingInput to whatever Stream is currently attached, +// re-reading a.pty fresh before every single write rather than holding one +// Stream reference for the whole drain. That is what makes a Stream swap +// mid-drain (a reattach racing this release) safe: a chunk that fails against +// a Stream this attachment has since moved on from is requeued — not lost, not +// misdelivered — for whichever Stream is current on the next iteration, and a +// genuine write failure against the STILL-current Stream closes it so run's +// blocked copyOut unblocks and the attach loop actually exits instead of +// hanging on a pane this drain has already given up on. +// +// draining (see the field doc) ensures only one such loop runs per attachment +// at a time; a second caller (a fresh setPTY racing this drain) simply returns +// and trusts the active loop to deliver to whatever Stream ends up current. +func (a *attachment) releaseInput(ctx context.Context) { for { + if ctx.Err() != nil { + return + } a.mu.Lock() - if a.closed || a.exited || a.pty != p { + if a.closed || a.exited || a.draining { + a.mu.Unlock() + return + } + pty := a.pty + if pty == nil { a.mu.Unlock() return } - pending := append([][]byte(nil), a.pendingInput...) - a.pendingInput = nil - if len(pending) == 0 { + if len(a.pendingInput) == 0 { a.inputReady = true a.mu.Unlock() return } + chunk := a.pendingInput[0] + a.pendingInput = a.pendingInput[1:] + a.draining = true a.mu.Unlock() - for _, chunk := range pending { - if _, err := p.Write(chunk); err != nil { - a.fail("flush pending input: " + err.Error()) - return + _, err := pty.Write(chunk) + + a.mu.Lock() + a.draining = false + stillCurrent := a.pty == pty + cancel := a.cancel + if err != nil && stillCurrent { + a.mu.Unlock() + a.fail("flush pending input: " + err.Error()) + // Cancel run's own loop BEFORE closing the Stream: closing is what + // wakes copyOut's blocked Read, and run only stops there if ctx is + // already done by then (shouldStop) — markExited alone only fires + // onExit, it does not stop run's loop, so without the cancel it + // would treat this like an ordinary dropped-but-reattachable Stream + // and try again. + if cancel != nil { + cancel() } + _ = pty.Close() + return + } + if err != nil { + // pty was swapped out from under this write (a reattach raced the + // drain): the chunk never reached the new Stream, so hand it back + // to the front of the queue for whichever Stream is current now. + a.pendingInput = append([][]byte{chunk}, a.pendingInput...) } + a.mu.Unlock() } } diff --git a/backend/internal/terminal/attachment_test.go b/backend/internal/terminal/attachment_test.go index 634edfc492..1ca30d0f1d 100644 --- a/backend/internal/terminal/attachment_test.go +++ b/backend/internal/terminal/attachment_test.go @@ -2,6 +2,7 @@ package terminal import ( "context" + "errors" "io" "log/slog" "sync" @@ -371,6 +372,95 @@ func (p *closeOrderPTY) Close() error { return p.fakePTY.Close() } +// A Stream swap racing the input-release drain must never let a chunk queued +// behind an in-flight write land on the Stream this attachment has already +// moved on from: the release loop re-reads the current Stream before every +// single write, so a chunk that was still queued when ownership changed goes +// to the NEW Stream, in order, rather than to the one this attachment no +// longer owns. +func TestAttachmentReleaseInputHandsRemainderToSwappedInPTY(t *testing.T) { + p1, p2 := newFakePTY(), newFakePTY() + p1.writeBlock = make(chan struct{}) + + a := newTestAttachment(&fakeSource{alive: true}, nil, nil) + a.inputGate = newInputGate() + a.inputGate.start(0) // already open: release proceeds without waiting + + if err := a.write([]byte("A")); err != nil { + t.Fatalf("queue A: %v", err) + } + if err := a.write([]byte("B")); err != nil { + t.Fatalf("queue B: %v", err) + } + + ctx := context.Background() + if !a.setPTY(ctx, p1) { + t.Fatal("setPTY(p1) reported failure") + } + // The release goroutine is now blocked inside p1.Write([]byte("A")). + eventually(t, time.Second, func() bool { + a.mu.Lock() + defer a.mu.Unlock() + return a.draining + }) + + // A reattach races the in-flight write: p2 replaces p1 as the live Stream. + if !a.setPTY(ctx, p2) { + t.Fatal("setPTY(p2) reported failure") + } + + close(p1.writeBlock) // let the blocked write to p1 complete + + eventually(t, time.Second, func() bool { return string(p2.writtenBytes()) == "B" }) + if got := string(p1.writtenBytes()); got != "A" { + t.Fatalf("p1 got %q, want exactly %q (B must not reach the superseded Stream)", got, "A") + } +} + +// An async flush failure (the pending-input Write itself errors) must not just +// mark the attachment exited — it must close the Stream so run's blocked +// copyOut unblocks, and the attach loop must actually return instead of +// treating the dead Stream as an ordinary drop-and-reattach case. +func TestAttachmentAsyncFlushFailureClosesStreamAndEndsRun(t *testing.T) { + pty := newFakePTY() + pty.writeErr = errors.New("boom") + sp := &fakeSpawner{ptys: []*fakePTY{pty}} + src := &fakeSource{alive: true, spawner: sp} + + exited := make(chan struct{}) + a := newTestAttachment(src, nil, func() { close(exited) }) + a.inputGate = newInputGate() + a.inputGate.start(0) // already open + + if err := a.write([]byte("x")); err != nil { + t.Fatalf("queue input before attach: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + runDone := make(chan struct{}) + go func() { + a.run(ctx) + close(runDone) + }() + + select { + case <-exited: + case <-time.After(time.Second): + t.Fatal("expected onExit after an async flush failure") + } + select { + case <-pty.closed: + case <-time.After(time.Second): + t.Fatal("a flush failure must close its Stream so copyOut unblocks") + } + select { + case <-runDone: + case <-time.After(time.Second): + t.Fatal("run must return after an async flush failure, not keep re-attaching") + } +} + func TestAttachmentCloseClosesPTYBeforeCancel(t *testing.T) { beforeCancel := make(chan struct{}) afterCancel := make(chan struct{}) diff --git a/backend/internal/terminal/fakes_test.go b/backend/internal/terminal/fakes_test.go index b196aec429..0ffa6e8a18 100644 --- a/backend/internal/terminal/fakes_test.go +++ b/backend/internal/terminal/fakes_test.go @@ -64,6 +64,13 @@ type fakePTY struct { mu sync.Mutex written []byte resizes [][2]uint16 + // writeErr, when set, makes every Write fail with this error instead of + // recording the bytes. + writeErr error + // writeBlock, when non-nil, makes Write wait for it to be closed before + // recording the bytes -- lets a test force an in-flight Write to straddle + // some other event (a PTY swap, a Close) deterministically. + writeBlock chan struct{} } func newFakePTY() *fakePTY { @@ -82,8 +89,14 @@ func (p *fakePTY) Read(b []byte) (int, error) { } func (p *fakePTY) Write(b []byte) (int, error) { + if p.writeBlock != nil { + <-p.writeBlock + } p.mu.Lock() defer p.mu.Unlock() + if p.writeErr != nil { + return 0, p.writeErr + } p.written = append(p.written, b...) return len(b), nil } diff --git a/backend/internal/terminal/manager.go b/backend/internal/terminal/manager.go index 83fee04009..e39c1c6480 100644 --- a/backend/internal/terminal/manager.go +++ b/backend/internal/terminal/manager.go @@ -108,11 +108,17 @@ func (g *inputGate) start(d time.Duration) { }) } -// wait blocks until the gate opens or ctx ends, whichever comes first. -func (g *inputGate) wait(ctx context.Context) { +// wait blocks until the gate opens or ctx ends, whichever comes first, and +// reports which one happened: true only means the gate itself opened. A +// caller must treat false (ctx ended first) as "abandon" — never as "proceed +// anyway" — since cancellation during this wait means shutdown (or this +// attachment closing) started before the pane was ever trusted with input. +func (g *inputGate) wait(ctx context.Context) bool { select { case <-g.ch: + return true case <-ctx.Done(): + return false } } @@ -231,6 +237,21 @@ func (m *Manager) inputGateFor(id string) *inputGate { return g } +// ResetInputGate drops id's shared input gate so its NEXT attach mints and +// arms a fresh one. Call this when a genuinely new agent process starts on an +// already-known pane id — a resume or restart that reuses the same runtime +// handle (see ports.RuntimeRestarter) — because the existing gate, if already +// open, would otherwise let the replacement TUI's early keystrokes through +// before it has had any chance to reach raw mode, reproducing the original +// race on every resume. Reattaching to the SAME still-running process must +// never call this: doing so would needlessly re-delay input for a pane that +// was never actually restarted. +func (m *Manager) ResetInputGate(id string) { + m.gatesMu.Lock() + delete(m.gates, id) + m.gatesMu.Unlock() +} + // joinTerminal registers a client (its connection + attach Stream + requested // grid + role) as a viewer of terminal id, then reconciles the shared grid. func (m *Manager) joinTerminal(id string, c *connState, att *attachment, cols, rows uint16, primary bool) { diff --git a/backend/internal/terminal/manager_test.go b/backend/internal/terminal/manager_test.go index 6d54054784..55cf11b484 100644 --- a/backend/internal/terminal/manager_test.go +++ b/backend/internal/terminal/manager_test.go @@ -636,6 +636,86 @@ func TestServeOutputDuringHoldReachesClientBeforeInputFlushes(t *testing.T) { } } +// Manager.Close cancels its own context before it marks each attachment +// closed, so a waiter mid-hold must abandon on that cancellation rather than +// proceeding just because the attachment doesn't LOOK closed yet: no buffered +// keystroke may reach the PTY once shutdown has started, and Close itself must +// not be stalled by a hold that has not elapsed. +func TestServeCloseDuringHoldAbandonsBufferedInput(t *testing.T) { + pty := newFakePTY() + sp := &fakeSpawner{ptys: []*fakePTY{pty}} + src := &fakeSource{alive: true, spawner: sp} + // A window much longer than this test so Close always lands mid-hold. + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(10*time.Second)) + + conn := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go mgr.Serve(ctx, conn) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgOpened, time.Second) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgData, Data: base64.StdEncoding.EncodeToString([]byte("never"))} + time.Sleep(20 * time.Millisecond) // let it actually reach pendingInput + + closed := make(chan struct{}) + go func() { + mgr.Close() + close(closed) + }() + + select { + case <-closed: + case <-time.After(time.Second): + t.Fatal("Manager.Close must return promptly even mid-hold, not wait out the grace window") + } + + if got := string(pty.writtenBytes()); got != "" { + t.Fatalf("input reached the PTY during shutdown: %q", got) + } +} + +// A resume/restart that reuses the same runtime handle (ports.RuntimeRestarter) +// must get a FRESH hold for the replacement agent process: without an explicit +// reset, the pane's gate — already open from the process that just exited — +// would let the new TUI's early keystrokes straight through, reproducing the +// original race on every resume instead of just on first launch. +func TestServeResetInputGateReArmsHoldForNextAttach(t *testing.T) { + p1, p2 := newFakePTY(), newFakePTY() + sp := &fakeSpawner{ptys: []*fakePTY{p1, p2}} + src := &fakeSource{alive: true, spawner: sp} + window := 150 * time.Millisecond + mgr := NewManager(src, nil, testLogger(), WithHeartbeat(0), WithInputGraceWindow(window)) + defer mgr.Close() + + conn := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go mgr.Serve(ctx, conn) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgOpened, time.Second) + // Let the pane's first grace window fully elapse. + time.Sleep(window + 50*time.Millisecond) + + // The agent process is replaced on the SAME handle (what a resume/restart + // does): session_manager calls this through terminal.Manager via + // InputGateResetter once the replacement process is launched. + mgr.ResetInputGate("t1") + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgClose} + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgOpened, time.Second) + + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgData, Data: base64.StdEncoding.EncodeToString([]byte("h"))} + time.Sleep(window / 2) + if got := string(p2.writtenBytes()); got != "" { + t.Fatalf("input reached the replacement TUI's PTY before its own grace window elapsed: %q", got) + } + eventually(t, time.Second, func() bool { return string(p2.writtenBytes()) == "h" }) +} + func TestEnqueueOverflowCancelsConn(t *testing.T) { cancelled := make(chan struct{}) c := &connState{ From 367b8e4266a1f910644c67ce41d599b551cb04d0 Mon Sep 17 00:00:00 2001 From: Keshav Varshney Date: Tue, 28 Jul 2026 15:29:06 +0530 Subject: [PATCH 4/4] fmt: restore blank line dropped at the manual merge-conflict seam The GitHub web UI conflict resolution for the sessionLifecycle merge (InputGateResetter + ShellTerminalCloser) concatenated the two method bodies without a separating blank line, which gofmt/goimports requires between a func and the next top-level declaration. Fixes the golangci-lint goimports CI failure on backend/internal/session_manager/manager.go:290. --- backend/internal/session_manager/manager.go | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 9fe8a90fa3..d3cf0d5b4f 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -288,6 +288,7 @@ func (m *Manager) beginShellTerminalTeardown(ctx context.Context, id domain.Sess } return closer.BeginSessionTeardown(ctx, id) } + // sendConfirmConfig bounds the best-effort activity-confirmation loop run after // Send. AO has no delivery ack: ao send returns 200 the moment tmux send-keys // exits 0, and for a large multiline paste the single Enter may not submit the