Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions backend/internal/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions backend/internal/daemon/lifecycle_wiring.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions backend/internal/daemon/wiring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
48 changes: 48 additions & 0 deletions backend/internal/session_manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
32 changes: 32 additions & 0 deletions backend/internal/session_manager/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"}}}
Expand Down
116 changes: 106 additions & 10 deletions backend/internal/terminal/attachment.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,20 @@ 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
// 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 {
Expand Down Expand Up @@ -153,7 +167,7 @@ func (a *attachment) run(ctx context.Context) {
continue
}

if !a.setPTY(p) {
if !a.setPTY(ctx, p) {
_ = p.Close()
return
}
Expand Down Expand Up @@ -271,7 +285,16 @@ 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 {
//
// 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 {
a.mu.Unlock()
Expand All @@ -285,6 +308,8 @@ func (a *attachment) setPTY(p ports.Stream) bool {
a.opened = true
}
onOpen := a.onOpen
gate := a.inputGate
window := a.inputGraceWindow
a.mu.Unlock()
if rows > 0 && cols > 0 {
_ = p.Resize(rows, cols)
Expand All @@ -293,23 +318,94 @@ func (a *attachment) setPTY(p ports.Stream) bool {
onOpen()
}

if gate == nil {
a.releaseInput(ctx)
return true
}
gate.start(window)
go a.releaseInputWhenGateOpens(ctx, gate)
return true
}

// 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(ctx)
}

// 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()
pending := append([][]byte(nil), a.pendingInput...)
a.pendingInput = nil
if len(pending) == 0 {
if a.closed || a.exited || a.draining {
a.mu.Unlock()
return
}
pty := a.pty
if pty == nil {
a.mu.Unlock()
return
}
if len(a.pendingInput) == 0 {
a.inputReady = true
a.mu.Unlock()
return true
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 false
_, 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()
}
}

Expand Down
90 changes: 90 additions & 0 deletions backend/internal/terminal/attachment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package terminal

import (
"context"
"errors"
"io"
"log/slog"
"sync"
Expand Down Expand Up @@ -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{})
Expand Down
Loading
Loading