diff --git a/backend/internal/adapters/runtime/zellij/commands.go b/backend/internal/adapters/runtime/zellij/commands.go index 20446ddec..bc7ed5b92 100644 --- a/backend/internal/adapters/runtime/zellij/commands.go +++ b/backend/internal/adapters/runtime/zellij/commands.go @@ -50,8 +50,14 @@ func listSessionsArgs() []string { return []string{"list-sessions", "--no-formatting"} } -func killSessionArgs(id string) []string { - return []string{"kill-session", id} +// deleteSessionArgs builds the teardown command. `delete-session --force` +// kills a running session AND removes its serialized resurrection state in one +// step. Plain `kill-session` is not enough: zellij can keep the session in its +// global resurrection cache as "(EXITED - attach to resurrect)", and any later +// `zellij attach ` (e.g. the terminal mux re-opening a pane) would resurrect +// it — re-running the agent command for a session the daemon already destroyed. +func deleteSessionArgs(id string) []string { + return []string{"delete-session", "--force", id} } func attachArgs(id string) []string { diff --git a/backend/internal/adapters/runtime/zellij/zellij.go b/backend/internal/adapters/runtime/zellij/zellij.go index 536ad28a7..cfb12aaa1 100644 --- a/backend/internal/adapters/runtime/zellij/zellij.go +++ b/backend/internal/adapters/runtime/zellij/zellij.go @@ -152,14 +152,15 @@ func (r *Runtime) Create(ctx context.Context, cfg ports.RuntimeConfig) (ports.Ru return ports.RuntimeHandle{ID: handleIDValue(id, paneID)}, nil } -// Destroy kills the handle's zellij session. An already-gone session is treated -// as success. +// Destroy kills the handle's zellij session and deletes its serialized state, +// so the session can never be resurrected by a later `zellij attach`. An +// already-gone session is treated as success. func (r *Runtime) Destroy(ctx context.Context, handle ports.RuntimeHandle) error { id, _, err := handleID(handle) if err != nil { return err } - if _, err := r.run(ctx, killSessionArgs(id)...); err != nil { + if _, err := r.run(ctx, deleteSessionArgs(id)...); err != nil { var exitErr *exec.ExitError if errors.As(err, &exitErr) { return nil @@ -204,7 +205,10 @@ func (r *Runtime) GetOutput(ctx context.Context, handle ports.RuntimeHandle, lin } // IsAlive reports whether the handle's session still appears in `zellij -// list-sessions`. +// list-sessions`. Only the documented "no sessions exist" failure counts as a +// definitive "not alive"; any other list-sessions failure is reported as a +// probe error so callers (the reaper feeding the LCM) treat it as a failed +// probe, never as proof of death. func (r *Runtime) IsAlive(ctx context.Context, handle ports.RuntimeHandle) (bool, error) { id, _, err := handleID(handle) if err != nil { @@ -213,7 +217,7 @@ func (r *Runtime) IsAlive(ctx context.Context, handle ports.RuntimeHandle) (bool out, err := r.run(ctx, listSessionsArgs()...) if err != nil { var exitErr *exec.ExitError - if errors.As(err, &exitErr) { + if errors.As(err, &exitErr) && noActiveSessionsOutput(string(out)) { return false, nil } return false, fmt.Errorf("zellij runtime: probe session %s: %w", id, err) @@ -221,6 +225,15 @@ func (r *Runtime) IsAlive(ctx context.Context, handle ports.RuntimeHandle) (bool return sessionListedAlive(string(out), id), nil } +// noActiveSessionsOutput reports whether a non-zero `zellij list-sessions` +// failed because no sessions exist at all — the one exit-error case that is a +// definitive "dead" rather than a probe failure. zellij 0.44 emits either +// "No active zellij sessions found." or "There is no active session!". +func noActiveSessionsOutput(out string) bool { + s := strings.ToLower(out) + return strings.Contains(s, "no active") && strings.Contains(s, "session") +} + // AttachCommand returns the argv a human runs to attach their terminal to the // session. func (r *Runtime) AttachCommand(handle ports.RuntimeHandle) ([]string, error) { diff --git a/backend/internal/adapters/runtime/zellij/zellij_test.go b/backend/internal/adapters/runtime/zellij/zellij_test.go index 22e2e5092..f91879dbc 100644 --- a/backend/internal/adapters/runtime/zellij/zellij_test.go +++ b/backend/internal/adapters/runtime/zellij/zellij_test.go @@ -42,6 +42,12 @@ func TestCommandBuilders(t *testing.T) { if got, want := dumpScreenArgs("sess-1", "terminal_0"), []string{"--session", "sess-1", "action", "dump-screen", "--pane-id", "terminal_0", "--full"}; !reflect.DeepEqual(got, want) { t.Fatalf("dumpScreenArgs = %#v, want %#v", got, want) } + // delete-session --force (not kill-session): teardown must also purge the + // serialized resurrection state, or a later `zellij attach` re-creates the + // session — and re-runs its agent — after the daemon destroyed it. + if got, want := deleteSessionArgs("sess-1"), []string{"delete-session", "--force", "sess-1"}; !reflect.DeepEqual(got, want) { + t.Fatalf("deleteSessionArgs = %#v, want %#v", got, want) + } } func TestZellijSessionNameSanitizesIssueRefs(t *testing.T) { @@ -375,8 +381,11 @@ func TestIsAliveParsesNoFormattingOutput(t *testing.T) { } } -func TestIsAliveTreatsExitStatusAsNotAlive(t *testing.T) { - fr := &fakeRunner{err: &exec.ExitError{}} +// IsAlive may treat a non-zero list-sessions ONLY as "not alive" when zellij +// says no sessions exist at all. Any other exit failure is a probe error: the +// reaper reports it as a failed probe and the LCM must never read it as death. +func TestIsAliveTreatsNoSessionsExitAsNotAlive(t *testing.T) { + fr := &fakeRunner{outputs: [][]byte{[]byte("No active zellij sessions found.")}, err: &exec.ExitError{}} r := New(Options{Timeout: time.Second}) r.runner = fr @@ -389,6 +398,20 @@ func TestIsAliveTreatsExitStatusAsNotAlive(t *testing.T) { } } +func TestIsAliveReportsOtherExitFailuresAsProbeErrors(t *testing.T) { + fr := &fakeRunner{outputs: [][]byte{[]byte("thread 'main' panicked")}, err: &exec.ExitError{}} + r := New(Options{Timeout: time.Second}) + r.runner = fr + + alive, err := r.IsAlive(context.Background(), ports.RuntimeHandle{ID: "sess-1/terminal_0"}) + if err == nil { + t.Fatal("IsAlive: got nil error, want probe failure — a failed probe must not read as dead") + } + if alive { + t.Fatal("alive = true on probe failure") + } +} + func TestDestroyIsIdempotentWhenSessionMissing(t *testing.T) { fr := &fakeRunner{err: &exec.ExitError{}} r := New(Options{Timeout: time.Second}) @@ -397,8 +420,27 @@ func TestDestroyIsIdempotentWhenSessionMissing(t *testing.T) { if err := r.Destroy(context.Background(), ports.RuntimeHandle{ID: "sess-1/terminal_0"}); err != nil { t.Fatalf("Destroy: %v", err) } - if len(fr.calls) != 1 || fr.calls[0].args[0] != "kill-session" { - t.Fatalf("calls = %#v, want only kill-session", fr.calls) + if len(fr.calls) != 1 || fr.calls[0].args[0] != "delete-session" { + t.Fatalf("calls = %#v, want only delete-session", fr.calls) + } +} + +// Destroy must delete the session's serialized state, not merely kill it: a +// killed-but-cached session is resurrected (agent re-run included) by any later +// `zellij attach`, bringing a terminated session's runtime back to life. +func TestDestroyForceDeletesSerializedSession(t *testing.T) { + fr := &fakeRunner{} + r := New(Options{Timeout: time.Second}) + r.runner = fr + + if err := r.Destroy(context.Background(), ports.RuntimeHandle{ID: "sess-1/terminal_0"}); err != nil { + t.Fatalf("Destroy: %v", err) + } + if len(fr.calls) != 1 { + t.Fatalf("calls = %d, want 1", len(fr.calls)) + } + if got, want := fr.calls[0].args, []string{"delete-session", "--force", "sess-1"}; !reflect.DeepEqual(got, want) { + t.Fatalf("destroy args = %#v, want %#v", got, want) } } diff --git a/backend/internal/httpd/terminal_mux_test.go b/backend/internal/httpd/terminal_mux_test.go index a044629dd..01c056953 100644 --- a/backend/internal/httpd/terminal_mux_test.go +++ b/backend/internal/httpd/terminal_mux_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "runtime" "strings" + "sync/atomic" "testing" "time" @@ -19,13 +20,21 @@ import ( // stubSource attaches a throwaway shell command instead of a real Zellij pane, so // the /mux path exercises the genuine upgrade + wsjson + Serve + creack/pty flow -// without needing Zellij. IsAlive=false means the pane is treated as gone once the -// command exits (no re-attach). -type stubSource struct{ argv []string } +// without needing Zellij. The pane reports alive until the first attach happens +// (the mux refuses to attach to a dead pane), then dead, so the command's exit is +// treated as the pane being gone (no re-attach). +type stubSource struct { + argv []string + attached atomic.Bool +} -func (s stubSource) AttachCommand(ports.RuntimeHandle) ([]string, error) { return s.argv, nil } -func (stubSource) IsAlive(context.Context, ports.RuntimeHandle) (bool, error) { - return false, nil +func (s *stubSource) AttachCommand(ports.RuntimeHandle) ([]string, error) { + s.attached.Store(true) + return s.argv, nil +} + +func (s *stubSource) IsAlive(context.Context, ports.RuntimeHandle) (bool, error) { + return !s.attached.Load(), nil } type terminalMuxFrame struct { @@ -72,7 +81,7 @@ func TestMuxUpgradeStreamsTerminal(t *testing.T) { t.Skip("PTY spawning not supported on Windows") } mgr := terminal.NewManager( - stubSource{argv: []string{"/bin/sh", "-c", "printf MUXOK; exit 0"}}, + &stubSource{argv: []string{"/bin/sh", "-c", "printf MUXOK; exit 0"}}, nil, discardLogger(), ) defer mgr.Close() @@ -101,7 +110,7 @@ func TestMuxSystemPingPong(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("PTY spawning not supported on Windows") } - mgr := terminal.NewManager(stubSource{argv: []string{"/bin/sh"}}, nil, discardLogger()) + mgr := terminal.NewManager(&stubSource{argv: []string{"/bin/sh"}}, nil, discardLogger()) defer mgr.Close() c, done := dialMux(t, mgr) diff --git a/backend/internal/terminal/fakes_test.go b/backend/internal/terminal/fakes_test.go index 247c33bd9..dad7a7203 100644 --- a/backend/internal/terminal/fakes_test.go +++ b/backend/internal/terminal/fakes_test.go @@ -41,6 +41,13 @@ func (f *fakeSource) setAlive(v bool) { f.mu.Unlock() } +func (f *fakeSource) setAliveResult(v bool, err error) { + f.mu.Lock() + f.alive = v + f.aliveErr = err + f.mu.Unlock() +} + // fakePTY is a scripted ptyProcess: Read drains the out channel, Write records, // Resize records, and Close unblocks reads. type fakePTY struct { diff --git a/backend/internal/terminal/manager_test.go b/backend/internal/terminal/manager_test.go index cd957ca72..1584487d8 100644 --- a/backend/internal/terminal/manager_test.go +++ b/backend/internal/terminal/manager_test.go @@ -69,7 +69,7 @@ func recv(t *testing.T, c *fakeConn, ch, typ string, d time.Duration) serverMsg } func TestServeOpenStreamsAndWritesTerminal(t *testing.T) { - src := &fakeSource{} + src := &fakeSource{alive: true} pty := newFakePTY() sp := &fakeSpawner{ptys: []*fakePTY{pty}} mgr := NewManager(src, nil, testLogger(), WithSpawn(sp.spawn), WithHeartbeat(0)) @@ -152,8 +152,7 @@ func TestServeOpenAlreadyExitedSessionDoesNotBlockReopen(t *testing.T) { // exit, so a later open for the same id is served rather than dropped by the // already-open guard. func TestServeExitAfterOpenClearsEntryAllowingReopen(t *testing.T) { - src := &fakeSource{} - src.setAlive(false) // a dropped pty must not re-attach -> session exits + src := &fakeSource{alive: true} // alive for the first attach p := newFakePTY() sp := &fakeSpawner{ptys: []*fakePTY{p}} mgr := NewManager(src, nil, testLogger(), WithSpawn(sp.spawn), WithHeartbeat(0)) @@ -167,7 +166,8 @@ func TestServeExitAfterOpenClearsEntryAllowingReopen(t *testing.T) { conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} recv(t, conn, chTerminal, msgOpened, time.Second) - p.Close() // drop the pty; IsAlive false => session exits, no re-attach + src.setAlive(false) // a dropped pty must not re-attach -> session exits + p.Close() // drop the pty; IsAlive false => session exits, no re-attach recv(t, conn, chTerminal, msgExited, time.Second) conn.in <- clientMsg{Ch: chTerminal, ID: "t1", Type: msgOpen} diff --git a/backend/internal/terminal/session.go b/backend/internal/terminal/session.go index 02d99cbdf..f0ab25fbf 100644 --- a/backend/internal/terminal/session.go +++ b/backend/internal/terminal/session.go @@ -102,6 +102,30 @@ func (s *session) run(ctx context.Context) { return } + // Gate EVERY attach (including the first) on the runtime actually + // being alive. `zellij attach` resurrects EXITED sessions — re-running + // the serialized agent command — so attaching to a dead handle would + // re-create a runtime the daemon already destroyed, outside lifecycle + // control. A definitive "not alive" is a clean exit. A probe ERROR is + // not proof of death: it retries with backoff up to the same + // consecutive-failure cap as attach failures. + alive, err := s.src.IsAlive(ctx, s.handle) + if err != nil { + failures++ + if failures > s.maxReattach { + s.fail("liveness probe: " + err.Error()) + return + } + if !s.backoff(ctx, failures) { + return + } + continue + } + if !alive { + s.markExited() + return + } + argv, err := s.src.AttachCommand(s.handle) if err != nil { s.fail("attach command: " + err.Error()) @@ -110,10 +134,13 @@ func (s *session) run(ctx context.Context) { p, err := s.spawn(ctx, argv) if err != nil { failures++ - if !s.shouldReattach(ctx, failures) { + if failures > s.maxReattach { s.fail("spawn pty: " + err.Error()) return } + if !s.backoff(ctx, failures) { + return + } continue } @@ -127,10 +154,13 @@ func (s *session) run(ctx context.Context) { } failures++ - if !s.shouldReattach(ctx, failures) { + if failures > s.maxReattach { s.markExited() return } + if !s.backoff(ctx, failures) { + return + } s.log.Debug("terminal re-attaching", "id", s.id, "failures", failures) } } @@ -152,17 +182,11 @@ func (s *session) copyOut(p ptyProcess) { } } -// shouldReattach decides whether a dropped/failed PTY warrants another attempt: -// only while not closed/cancelled, the Zellij session still exists, and we are -// under the consecutive-failure cap. A backoff sleep separates attempts. -func (s *session) shouldReattach(ctx context.Context, failures int) bool { - if s.isClosed() || ctx.Err() != nil || failures > s.maxReattach { - return false - } - alive, err := s.src.IsAlive(ctx, s.handle) - if err != nil || !alive { - return false - } +// backoff sleeps between attach attempts; false means ctx was cancelled. +// Whether another attempt is warranted at all (liveness, failure cap) is +// decided at the top of the run loop, so a re-attach and a first attach share +// one gate. +func (s *session) backoff(ctx context.Context, failures int) bool { select { case <-ctx.Done(): return false diff --git a/backend/internal/terminal/session_test.go b/backend/internal/terminal/session_test.go index 5483117ac..4de33d305 100644 --- a/backend/internal/terminal/session_test.go +++ b/backend/internal/terminal/session_test.go @@ -17,7 +17,7 @@ func newTestSession(src PTYSource, spawn spawnFunc) *session { } func TestSessionFansOutLiveOutputToSubscribers(t *testing.T) { - src := &fakeSource{} + src := &fakeSource{alive: true} pty := newFakePTY() sp := &fakeSpawner{ptys: []*fakePTY{pty}} s := newTestSession(src, sp.spawn) @@ -35,7 +35,7 @@ func TestSessionFansOutLiveOutputToSubscribers(t *testing.T) { } func TestSessionReplaysRingBufferOnSubscribe(t *testing.T) { - src := &fakeSource{} + src := &fakeSource{alive: true} pty := newFakePTY() sp := &fakeSpawner{ptys: []*fakePTY{pty}} s := newTestSession(src, sp.spawn) @@ -59,7 +59,7 @@ func ringLen(s *session) int { } func TestSessionWriteAndResizeReachPTY(t *testing.T) { - src := &fakeSource{} + src := &fakeSource{alive: true} pty := newFakePTY() sp := &fakeSpawner{ptys: []*fakePTY{pty}} s := newTestSession(src, sp.spawn) @@ -81,7 +81,7 @@ func TestSessionWriteAndResizeReachPTY(t *testing.T) { } func TestSessionSkipsReattachOnCleanExit(t *testing.T) { - src := &fakeSource{alive: false} // Zellij session gone -> no re-attach + src := &fakeSource{alive: true} // alive for the first attach pty := newFakePTY() sp := &fakeSpawner{ptys: []*fakePTY{pty}} s := newTestSession(src, sp.spawn) @@ -93,7 +93,9 @@ func TestSessionSkipsReattachOnCleanExit(t *testing.T) { go s.run(ctx) s.subscribe(func([]byte) {}, func() { close(exited) }) - pty.Close() // pane ends + eventually(t, time.Second, func() bool { return sp.calls() == 1 }) + src.setAlive(false) // Zellij session gone -> no re-attach + pty.Close() // pane ends select { case <-exited: case <-time.After(time.Second): @@ -104,6 +106,61 @@ func TestSessionSkipsReattachOnCleanExit(t *testing.T) { } } +// TestSessionNeverAttachesToDeadRuntime covers the resurrection bug: `zellij +// attach` on a killed-but-cached session resurrects it, re-running the agent +// command. A session whose runtime probes definitively dead must therefore +// report exited WITHOUT ever spawning an attach PTY — even on the very first +// open (the original code only checked liveness on re-attach). +func TestSessionNeverAttachesToDeadRuntime(t *testing.T) { + src := &fakeSource{alive: false} + sp := &fakeSpawner{} + s := newTestSession(src, sp.spawn) + + exited := make(chan struct{}) + s.subscribe(func([]byte) {}, func() { close(exited) }) + + go s.run(context.Background()) + select { + case <-exited: + case <-time.After(time.Second): + t.Fatal("expected exit when runtime is dead before first attach") + } + if got := sp.calls(); got != 0 { + t.Fatalf("attach must never run against a dead runtime, got %d spawns", got) + } +} + +// TestSessionRetriesProbeErrorsBeforeAttaching pins the hard rule that a +// failed liveness probe is NOT proof of death: a transient probe error must +// not flip the terminal to exited, and the attach proceeds once the probe +// recovers. +func TestSessionRetriesProbeErrorsBeforeAttaching(t *testing.T) { + src := &fakeSource{aliveErr: io.ErrUnexpectedEOF} + pty := newFakePTY() + sp := &fakeSpawner{ptys: []*fakePTY{pty}} + s := newTestSession(src, sp.spawn) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go s.run(ctx) + + // While probes error the session must neither exit nor attach. + time.Sleep(50 * time.Millisecond) + if s.isExited() { + t.Fatal("probe error must not be treated as runtime death") + } + if got := sp.calls(); got != 0 { + t.Fatalf("attach must wait for a successful probe, got %d spawns", got) + } + + // Probe recovers -> the attach goes through. + src.setAliveResult(true, nil) + eventually(t, 2*time.Second, func() bool { return sp.calls() == 1 }) + if s.isExited() { + t.Fatal("session exited despite a live runtime") + } +} + func TestSessionReattachesWhileSessionAlive(t *testing.T) { src := &fakeSource{alive: true} // session still alive -> re-attach on drop p1, p2 := newFakePTY(), newFakePTY() @@ -125,7 +182,7 @@ func TestSessionReattachesWhileSessionAlive(t *testing.T) { } func TestSessionFailsWhenAttachCommandErrors(t *testing.T) { - src := &fakeSource{attachErr: io.ErrUnexpectedEOF} + src := &fakeSource{alive: true, attachErr: io.ErrUnexpectedEOF} sp := &fakeSpawner{} s := newTestSession(src, sp.spawn)