From 78cdc6d5cab15ff5142d607ee0501d66767de234 Mon Sep 17 00:00:00 2001 From: i-trytoohard Date: Sun, 21 Jun 2026 14:30:00 +0530 Subject: [PATCH] fix(session-manager): terminate sessions even when handle is missing (#360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(session-manager): terminate sessions even when handle is missing Kill hard-failed with ErrIncompleteHandle before recording terminal intent, so a session that lost its runtime/workspace handle (crash, partial spawn) was un-killable and stuck forever on the dashboard. MarkTerminated now runs unconditionally; each destroy step is gated individually on whether that handle exists. If nothing is present to tear down, the session still terminates — freed=false just signals nothing was freed. ErrIncompleteHandle is still returned by Restore and runtimeMessenger.Send, where a missing handle is genuinely fatal. Updates TestKill_RefusesIncompleteHandle → TestKill_TerminatesIncompleteHandle. * style: gofmt blank line between test functions --------- Co-authored-by: AO Bot --- backend/internal/session_manager/manager.go | 38 ++++++++++++------- .../internal/session_manager/manager_test.go | 18 +++++++-- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index 19a548120..104982bd9 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -361,8 +361,6 @@ func (m *Manager) markSpawnFailedTerminated(ctx context.Context, id domain.Sessi // don't accumulate terminated rows in session lists. DeleteSession only removes // rows still in seed state; if the row has progressed or the delete itself // fails, fall back to parking it terminated so a phantom row never looks live. -// (Kill is not a usable fallback here: it refuses seed rows with -// ErrIncompleteHandle before recording terminal intent.) func (m *Manager) rollbackSpawnSeedRow(ctx context.Context, id domain.SessionID) { if deleted, err := m.store.DeleteSession(ctx, id); err == nil && deleted { return @@ -405,6 +403,11 @@ func (m *Manager) RollbackSpawn(ctx context.Context, id domain.SessionID) (delet // workspace. A workspace teardown refused by the worktree-remove safety // (uncommitted work) is never forced: the session still terminates and Kill // succeeds with freed=false, signalling the workspace was preserved. +// +// A session whose runtime handle or workspace path is missing (e.g. spawn +// failed partway, handle lost after a crash) is still terminated — the destroy +// steps are skipped for whatever is absent, but the session record always +// moves to terminal state so it can be cleaned up from the dashboard. func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { rec, ok, err := m.store.GetSession(ctx, id) if err != nil { @@ -415,22 +418,31 @@ func (m *Manager) Kill(ctx context.Context, id domain.SessionID) (bool, error) { } handle := runtimeHandle(rec.Metadata) ws := workspaceInfo(rec) - if handle.ID == "" || ws.Path == "" { - return false, fmt.Errorf("kill %s: %w", id, ErrIncompleteHandle) - } + + // Always record terminal intent so the session is marked terminated even + // when the runtime/workspace handle is missing. if err := m.lcm.MarkTerminated(ctx, id); err != nil { return false, fmt.Errorf("kill %s: %w", id, err) } - if err := m.runtime.Destroy(ctx, handle); err != nil { - return false, fmt.Errorf("kill %s: runtime: %w", id, err) - } - if err := m.workspace.Destroy(ctx, ws); err != nil { - if errors.Is(err, ports.ErrWorkspaceDirty) { - return false, nil + + // 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 != "" { + if err := m.runtime.Destroy(ctx, handle); err != nil { + return false, fmt.Errorf("kill %s: runtime: %w", id, err) } - return false, fmt.Errorf("kill %s: workspace: %w", id, err) } - return true, nil + freed := false + if ws.Path != "" { + if err := m.workspace.Destroy(ctx, ws); err != nil { + if errors.Is(err, ports.ErrWorkspaceDirty) { + return false, nil + } + return false, fmt.Errorf("kill %s: workspace: %w", id, err) + } + freed = true + } + return freed, nil } // Restore relaunches a torn-down session in its workspace. The fallible I/O runs diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index e3a8afd49..b7f3dccac 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -425,11 +425,23 @@ func TestKill_TearsDownRuntimeAndWorkspace(t *testing.T) { t.Fatal("kill should destroy runtime and workspace") } } -func TestKill_RefusesIncompleteHandle(t *testing.T) { + +// TestKill_TerminatesIncompleteHandle: a session whose runtime handle or +// workspace path is missing is still terminated — the destroy steps are +// skipped, but the session moves to terminal state so it can be cleaned up +// from the dashboard. +func TestKill_TerminatesIncompleteHandle(t *testing.T) { m, st, _, _ := newManager() st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer", Activity: domain.Activity{State: domain.ActivityActive}} - if _, err := m.Kill(ctx, "mer-1"); !errors.Is(err, ErrIncompleteHandle) { - t.Fatalf("want ErrIncompleteHandle, got %v", err) + freed, err := m.Kill(ctx, "mer-1") + if err != nil { + t.Fatalf("want nil error, got %v", err) + } + if freed { + t.Fatal("freed = true, want false for session with no workspace") + } + if !st.sessions["mer-1"].IsTerminated { + t.Fatal("session should be terminated even without a handle") } }