diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 9eee7a573..4ae540c72 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -96,6 +96,8 @@ type Store interface { // presence of any row is the marker; preserved_ref may be empty for clean // worktrees. ListSessionWorktrees(ctx context.Context, id domain.SessionID) ([]domain.SessionWorktreeRecord, error) + // DeleteSessionWorktrees clears the "shutdown-saved" restore marker. + DeleteSessionWorktrees(ctx context.Context, id domain.SessionID) error } // Manager coordinates internal session spawn, restore, kill, and cleanup over @@ -448,6 +450,12 @@ func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { return false, fmt.Errorf("kill %s: %w", id, err) } + // Clear the restore marker so the next boot's RestoreAll cannot resurrect a + // killed session (#2319). Best-effort: teardown below still matters. + if err := m.store.DeleteSessionWorktrees(ctx, id); err != nil { + m.logger.Warn("kill: delete restore marker failed", "sessionID", id, "error", err) + } + // Only tear down what exists. A session may have lost its handle after a // crash or never acquired one if spawn failed partway. if handle.ID != "" { @@ -841,6 +849,12 @@ func (m *Manager) RestoreAll(ctx context.Context) error { m.logger.Error("restore-all: relaunch failed", "sessionID", rec.ID, "error", err) } } + + // One-shot: drop the consumed marker so it never outlives one restart + // (#2319). A still-live session re-acquires it at the next quit. + if err := m.store.DeleteSessionWorktrees(ctx, rec.ID); err != nil { + m.logger.Warn("restore-all: delete restore marker failed", "sessionID", rec.ID, "error", err) + } } return nil } diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index d12943c37..0ea59273d 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -113,6 +113,10 @@ func (f *fakeStore) UpsertSessionWorktree(_ context.Context, row domain.SessionW func (f *fakeStore) ListSessionWorktrees(_ context.Context, id domain.SessionID) ([]domain.SessionWorktreeRecord, error) { return f.worktrees[id], nil } +func (f *fakeStore) DeleteSessionWorktrees(_ context.Context, id domain.SessionID) error { + delete(f.worktrees, id) + return nil +} type fakeLCM struct { store *fakeStore @@ -570,6 +574,29 @@ func TestKill_OtherWorkspaceErrorStillFails(t *testing.T) { t.Fatalf("kill err = %v, want workspace error surfaced", err) } } + +// TestKill_DeletesRestoreMarker covers issue #2319 (a): a user kill is explicit +// terminal intent and must delete the session_worktrees "shutdown-saved" marker. +// A session that carried a marker (e.g. it survived a prior reopen cycle) and is +// then killed must not keep that marker, or the next boot's RestoreAll would +// resurrect it. +func TestKill_DeletesRestoreMarker(t *testing.T) { + m, st, _, _ := newManager() + st.sessions["mer-1"] = mkLive("mer-1") + // The session carries a leftover shutdown-saved marker from a prior cycle. + st.worktrees["mer-1"] = []domain.SessionWorktreeRecord{{SessionID: "mer-1", RepoName: "__root__"}} + + if _, err := m.Kill(ctx, "mer-1"); err != nil { + t.Fatalf("kill err = %v", err) + } + rows, err := st.ListSessionWorktrees(ctx, "mer-1") + if err != nil { + t.Fatal(err) + } + if len(rows) != 0 { + t.Fatalf("kill must delete the restore marker, got %d rows", len(rows)) + } +} func TestRestore_ReopensTerminal(t *testing.T) { m, st, rt, _ := newManager() seedTerminal(st, "mer-1", domain.SessionMetadata{WorkspacePath: "/ws/mer-1", Branch: "b", AgentSessionID: "agent-x"}) @@ -1578,6 +1605,81 @@ func TestRestoreAll_SkipsSessionsKilledBeforeShutdown(t *testing.T) { } } +// TestRestoreAll_DeletesMarkerAfterRelaunch covers issue #2319 (b): the +// shutdown-saved marker is one-shot. After RestoreAll relaunches a session, its +// session_worktrees marker is deleted, so a second RestoreAll (with no fresh +// marker) does NOT relaunch it again. +func TestRestoreAll_DeletesMarkerAfterRelaunch(t *testing.T) { + m, st, rt, _ := newLifecycleManager() + + st.sessions["mer-1"] = domain.SessionRecord{ + ID: "mer-1", + ProjectID: "mer", + Kind: domain.KindWorker, + Harness: domain.HarnessClaudeCode, + IsTerminated: true, + Metadata: domain.SessionMetadata{WorkspacePath: "/ws/mer-1", Branch: "ao/mer-1/root", AgentSessionID: "agent-w"}, + Activity: domain.Activity{State: domain.ActivityExited}, + } + st.worktrees["mer-1"] = []domain.SessionWorktreeRecord{{SessionID: "mer-1", RepoName: "__root__"}} + + if err := m.RestoreAll(ctx); err != nil { + t.Fatalf("RestoreAll err = %v", err) + } + if rt.created != 1 { + t.Fatalf("first RestoreAll must relaunch once, runtime.Create called %d times", rt.created) + } + rows, err := st.ListSessionWorktrees(ctx, "mer-1") + if err != nil { + t.Fatal(err) + } + if len(rows) != 0 { + t.Fatalf("RestoreAll must delete the one-shot marker, got %d rows", len(rows)) + } +} + +// TestRestoreAll_KilledSessionNotResurrectedOnSecondBoot covers issue #2319 (c), +// the killed-session-resurrection scenario. A terminated session WITH a marker +// is relaunched exactly once; on a second RestoreAll (no new marker) it stays +// terminated and is not relaunched again. +func TestRestoreAll_KilledSessionNotResurrectedOnSecondBoot(t *testing.T) { + m, st, rt, _ := newLifecycleManager() + + st.sessions["mer-1"] = domain.SessionRecord{ + ID: "mer-1", + ProjectID: "mer", + Kind: domain.KindWorker, + Harness: domain.HarnessClaudeCode, + IsTerminated: true, + Metadata: domain.SessionMetadata{WorkspacePath: "/ws/mer-1", Branch: "ao/mer-1/root", AgentSessionID: "agent-w"}, + Activity: domain.Activity{State: domain.ActivityExited}, + } + st.worktrees["mer-1"] = []domain.SessionWorktreeRecord{{SessionID: "mer-1", RepoName: "__root__"}} + + // First boot: marker present, session relaunches once. + if err := m.RestoreAll(ctx); err != nil { + t.Fatalf("first RestoreAll err = %v", err) + } + if rt.created != 1 { + t.Fatalf("first RestoreAll must relaunch once, runtime.Create called %d times", rt.created) + } + + // Simulate the user killing the relaunched session before the next quit, so + // it has no fresh marker, then a second boot. + if _, err := m.Kill(ctx, "mer-1"); err != nil { + t.Fatalf("kill err = %v", err) + } + if err := m.RestoreAll(ctx); err != nil { + t.Fatalf("second RestoreAll err = %v", err) + } + if rt.created != 1 { + t.Fatalf("killed session must NOT be resurrected on second boot, runtime.Create total = %d, want 1", rt.created) + } + if !st.sessions["mer-1"].IsTerminated { + t.Error("killed session must remain terminated after second RestoreAll") + } +} + // TestRestoreAll_AppliesPreservedRef: when the session_worktrees row has a // non-empty preserved_ref, RestoreAll calls ApplyPreserved after workspace // restore but before relaunching.