diff --git a/backend/internal/terminal/manager.go b/backend/internal/terminal/manager.go index cda0a51fc..895edb6f8 100644 --- a/backend/internal/terminal/manager.go +++ b/backend/internal/terminal/manager.go @@ -232,6 +232,11 @@ func (c *connState) openTerminal(_ context.Context, id string) { // open acknowledgement. c.enqueue(serverMsg{Ch: chTerminal, ID: id, Type: msgOpened}) + // exitFired guards the subscribe-to-assign window: the session can exit (and + // run onExit) at any point after subscribe returns exited=false, including + // before c.terms[id] is assigned below. onExit and the assign both read/write + // this flag and the map only under c.mu, so no atomic is needed. + var exitFired bool unsub, exited := s.subscribe( func(data []byte) { c.enqueue(serverMsg{ @@ -248,6 +253,7 @@ func (c *connState) openTerminal(_ context.Context, id string) { // the delete after the frame would race that reopen. markExited fires // this without s.mu held, so taking c.mu is safe. c.mu.Lock() + exitFired = true delete(c.terms, id) c.mu.Unlock() c.enqueue(serverMsg{Ch: chTerminal, ID: id, Type: msgExited}) @@ -262,6 +268,12 @@ func (c *connState) openTerminal(_ context.Context, id string) { } c.mu.Lock() c.terms[id] = unsub + // If onExit already ran in the subscribe-to-assign window its delete was a + // no-op (the key did not exist yet), so the assign above just resurrected a + // stale entry for a dead pane. Re-apply the delete while still holding c.mu. + if exitFired { + delete(c.terms, id) + } c.mu.Unlock() } diff --git a/backend/internal/terminal/manager_test.go b/backend/internal/terminal/manager_test.go index b1d832f13..cd957ca72 100644 --- a/backend/internal/terminal/manager_test.go +++ b/backend/internal/terminal/manager_test.go @@ -174,6 +174,46 @@ func TestServeExitAfterOpenClearsEntryAllowingReopen(t *testing.T) { recv(t, conn, chTerminal, msgOpened, 2*time.Second) } +// The subscribe-to-assign window: a session can exit (running onExit, which +// deletes c.terms[id]) between subscribe returning exited=false and openTerminal +// assigning c.terms[id] = unsub. If the assign resurrects that entry without +// re-checking, the stale entry traps every later open for the id on this +// connection. Close the pty concurrently with the open (IsAlive false => no +// re-attach) so the exit races the assign across many iterations; every reopen +// must be served (opened), never silently dropped by the open guard. +func TestServeReopenAfterImmediateExitNeverStuck(t *testing.T) { + for i := 0; i < 400; i++ { + src := &fakeSource{} + src.setAlive(false) // dropped pty must not re-attach -> session exits + p := newFakePTY() // alive at subscribe; closed below to race the assign + sp := &fakeSpawner{ptys: []*fakePTY{p}} + mgr := NewManager(src, nil, testLogger(), WithSpawn(sp.spawn), WithHeartbeat(0)) + + conn := newFakeConn() + ctx, cancel := context.WithCancel(context.Background()) + go mgr.Serve(ctx, conn) + + // Send the open and close the pty concurrently: the session's exit + // (onExit -> delete c.terms[id]) then races openTerminal's assign of + // c.terms[id] = unsub. On the iterations where exit lands in the + // subscribe-to-assign window, an unguarded assign resurrects a stale + // entry for the dead pane, trapping every later open for this id. + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + go p.Close() + + recv(t, conn, chTerminal, msgOpened, time.Second) + recv(t, conn, chTerminal, msgExited, time.Second) + + // The reopen must be served even when the first open's session exited in + // the subscribe-to-assign window. + conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} + recv(t, conn, chTerminal, msgOpened, time.Second) + + cancel() + mgr.Close() + } +} + func TestServeRejectsOpenWithoutID(t *testing.T) { mgr := NewManager(&fakeSource{}, nil, testLogger(), WithSpawn((&fakeSpawner{}).spawn), WithHeartbeat(0)) defer mgr.Close()