fix(terminal): guard subscribe-to-assign window in openTerminal
A session can exit and run onExit (which deletes c.terms[id]) in the gap between subscribe returning exited=false and openTerminal assigning c.terms[id]. The delete is a no-op there since the key isn't set yet, so the later assign resurrects a stale entry for a dead pane, trapping every future open for that id on the connection. Re-apply the delete after the assign when onExit fired in the window, tracked by a c.mu-guarded flag. Add a stress regression test that races the exit against the assign. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
4f77062aed
commit
eda39a156a
|
|
@ -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()
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Reference in New Issue