fix(sync): guard delete actions by default #2578

packages/ui/src/sync/DOCUMENTATION.md

@@ -214,19 +214,42 @@ Examples of global-store updates performed in `session-actions.ts`:
 - `updateSessionTitle()` -> `upsertSession(result.data)`
 - `shareSession()` / `unshareSession()` -> `upsertSession(result.data)`
 - `archiveSession()` / `archiveSessions()` -> wait for server confirmation, then upsert each archived session
-- `deleteSession()` -> waits for server confirmation or `404`, then removes the session and its persisted state
+- `deleteSession()` / `deleteSessions()` -> wait for server confirmation or `404`, then remove the session and its persisted state
 - `moveSessionToDirectory()` -> move the session between directory stores and update the global directory index
 
-Archive actions capture the active runtime key when they start and recheck it
-before every store reconciliation, so a response
+Archive and delete actions capture the active runtime key when they start and
+recheck it before every store reconciliation, so a response
 produced by the previous runtime is rejected instead of mutating the current
 runtime's live or global session state. A guarded batch stops at the first
 observed runtime change: sessions the server already confirmed remain archived
-and stay in `archivedIds`, while every ID not confirmed on the captured runtime
-is returned in `failedIds` so existing partial-failure feedback stays truthful.
+or deleted and stay in `archivedIds`/`deletedIds`, while every ID not confirmed
+on the captured runtime is returned in `failedIds` so existing partial-failure
+feedback stays truthful.
 Callers whose confirmation can span a runtime switch may pass an
 `expectedRuntimeKey` captured earlier; ordinary callers are guarded by default.
 
+Deletion needs this guard more than archiving does. Session IDs are not unique
+across runtimes, and a committed deletion does more than hide a row: it evicts
+the session from every live store, removes it from the global cache, clears the
+current-session pointer, and calls `cleanupPersistedSessionState`, which erases
+that session's queued messages, todos, folder membership, inline-comment drafts,
+chat draft, and pins. Committing a stale deletion can therefore destroy user
+state belonging to an unrelated session on the new runtime.
+
+`cleanupPersistedSessionState` already refuses an identity whose runtime is no
+longer active, so `finalizeConfirmedSessionDeletion` must forward the **captured**
+runtime key. Passing the live key would make that check compare a value with
+itself and always pass. The in-memory live, global, and UI stores it mutates are
+not runtime-scoped, so the calling action must reject a stale runtime before
+committing rather than relying on that helper alone.
+
+A `404` still means "already deleted" and commits cleanup, but only while the
+captured runtime is active. After a runtime change the `404` describes either
+the previous runtime or one this session never belonged to, so the action
+reports failure instead of committing. The deletion already accepted by the
+server stays deleted there; its persisted state is left as harmless stale
+metadata and the next authoritative load reconciles it.
+
 ## The golden rule
 
 When creating a draft in `handleDirectoryEvent`, **only clone the state fields the event will mutate**. Never spread all fields eagerly.

packages/ui/src/sync/session-actions.test.ts

@@ -15,6 +15,7 @@ let sessionUpdateResult: { data?: unknown; error?: unknown; response?: { status?
 let sessionMessagesResult: { data?: unknown; error?: unknown; response?: { status?: number } } = { data: [] }
 let sessionDeleteError: unknown | null = null
 let beforeSessionUpdateResolve: ((sessionId: string) => void) | null = null
+let beforeSessionDeleteResolve: ((sessionId: string) => void) | null = null
 const globalUpsertedSessions: unknown[] = []
 const globalRemovedSessionIds: string[] = []
 const deletedCleanupIdentities: Array<{ runtimeKey: string; directory: string; sessionId: string }> = []
@@ -153,6 +154,9 @@ mock.module("@/lib/opencode/client", () => ({
     }),
     deleteSession: mock((sessionId: string, directory?: string | null) => {
       replyCalls.push({ method: "session.delete", params: { sessionID: sessionId, directory } })
+      // Lets a test switch runtime while the delete is in flight, so the action
+      // observes the change only after awaiting (or catching) the response.
+      beforeSessionDeleteResolve?.(sessionId)
       if (sessionDeleteError) throw sessionDeleteError
       return Promise.resolve(true)
     }),
@@ -373,6 +377,7 @@ describe("confirmed session removal", () => {
     sessionDeleteError = null
     sessionUpdateResult = {}
     beforeSessionUpdateResolve = null
+    beforeSessionDeleteResolve = null
   })
 
   test("does not remove live or persisted state when delete fails", async () => {
@@ -406,6 +411,107 @@ describe("confirmed session removal", () => {
     }).toEqual({ directory: "/test/project", sessionId: "session-a" })
   })
 
+  test("scopes persisted cleanup to the runtime captured when the delete started", async () => {
+    const source = createStore({}, {
+      session: [{ id: "session-a", directory: "/test/project", time: { created: 1 } } as Session],
+    })
+    const { getRuntimeKey, switchRuntimeEndpoint } = await import("../lib/runtime-switch")
+    switchRuntimeEndpoint({ apiBaseUrl: "http://delete-scope.test", runtimeKey: "delete-scope" })
+    const { deleteSession, setActionRefs } = await import("./session-actions")
+    setActionRefs(mockSdk as unknown as OpencodeClient, createChildStores([["/test/project", source]]), () => "/test/project")
+
+    expect(await deleteSession("session-a")).toBe(true)
+    // The cleanup identity must carry the captured runtime, which is what lets
+    // cleanupPersistedSessionState reject a stale identity instead of comparing
+    // the live runtime key with itself.
+    expect(deletedCleanupIdentities[0]?.runtimeKey).toBe("delete-scope")
+    expect(deletedCleanupIdentities[0]?.runtimeKey).toBe(getRuntimeKey())
+  })
+
+  test("rejects a delete response that arrives after a runtime switch", async () => {
+    const source = createStore({}, {
+      session: [{ id: "session-a", directory: "/test/project", time: { created: 1 } } as Session],
+    })
+    const { switchRuntimeEndpoint } = await import("../lib/runtime-switch")
+    switchRuntimeEndpoint({ apiBaseUrl: "http://delete-runtime-a.test", runtimeKey: "delete-runtime-a" })
+    beforeSessionDeleteResolve = () => {
+      switchRuntimeEndpoint({ apiBaseUrl: "http://delete-runtime-b.test", runtimeKey: "delete-runtime-b" })
+    }
+    const { deleteSession, setActionRefs } = await import("./session-actions")
+    setActionRefs(mockSdk as unknown as OpencodeClient, createChildStores([["/test/project", source]]), () => "/test/project")
+
+    expect(await deleteSession("session-a")).toBe(false)
+    // Session IDs are not unique across runtimes: committing here could evict an
+    // unrelated session and erase its queue, todos, drafts, folders, and pins.
+    expect(source.getState().session.map((item) => item.id)).toEqual(["session-a"])
+    expect(globalRemovedSessionIds).toEqual([])
+    expect(deletedCleanupIdentities).toEqual([])
+  })
+
+  test("does not treat a 404 as an already-completed deletion after a runtime switch", async () => {
+    sessionDeleteError = Object.assign(new Error("not found"), { status: 404 })
+    const source = createStore({}, {
+      session: [{ id: "session-a", directory: "/test/project", time: { created: 1 } } as Session],
+    })
+    const { switchRuntimeEndpoint } = await import("../lib/runtime-switch")
+    switchRuntimeEndpoint({ apiBaseUrl: "http://delete-404-a.test", runtimeKey: "delete-404-a" })
+    beforeSessionDeleteResolve = () => {
+      switchRuntimeEndpoint({ apiBaseUrl: "http://delete-404-b.test", runtimeKey: "delete-404-b" })
+    }
+    const { deleteSession, setActionRefs } = await import("./session-actions")
+    setActionRefs(mockSdk as unknown as OpencodeClient, createChildStores([["/test/project", source]]), () => "/test/project")
+
+    // A 404 only proves "already deleted" for the captured runtime. After a
+    // switch it describes the wrong runtime, so it must not commit cleanup.
+    expect(await deleteSession("session-a")).toBe(false)
+    expect(source.getState().session.map((item) => item.id)).toEqual(["session-a"])
+    expect(globalRemovedSessionIds).toEqual([])
+    expect(deletedCleanupIdentities).toEqual([])
+  })
+
+  test("still treats a 404 as an already-completed deletion while the runtime is stable", async () => {
+    sessionDeleteError = Object.assign(new Error("not found"), { status: 404 })
+    const source = createStore({}, {
+      session: [{ id: "session-a", directory: "/test/project", time: { created: 1 } } as Session],
+    })
+    const { deleteSession, setActionRefs } = await import("./session-actions")
+    setActionRefs(mockSdk as unknown as OpencodeClient, createChildStores([["/test/project", source]]), () => "/test/project")
+
+    expect(await deleteSession("session-a")).toBe(true)
+    expect(source.getState().session).toEqual([])
+    expect(globalRemovedSessionIds).toEqual(["session-a"])
+    expect(deletedCleanupIdentities).toHaveLength(1)
+  })
+
+  test("keeps committed deletions and fails the rest when the runtime changes mid-batch", async () => {
+    const source = createStore({}, {
+      session: [
+        { id: "session-a", directory: "/test/project", time: { created: 1 } } as Session,
+        { id: "session-b", directory: "/test/project", time: { created: 1 } } as Session,
+        { id: "session-c", directory: "/test/project", time: { created: 1 } } as Session,
+      ],
+    })
+    const { switchRuntimeEndpoint } = await import("../lib/runtime-switch")
+    switchRuntimeEndpoint({ apiBaseUrl: "http://delete-batch-a.test", runtimeKey: "delete-batch-a" })
+    beforeSessionDeleteResolve = (sessionId) => {
+      if (sessionId === "session-b") {
+        switchRuntimeEndpoint({ apiBaseUrl: "http://delete-batch-b.test", runtimeKey: "delete-batch-b" })
+      }
+    }
+    const { deleteSessions, setActionRefs } = await import("./session-actions")
+    setActionRefs(mockSdk as unknown as OpencodeClient, createChildStores([["/test/project", source]]), () => "/test/project")
+
+    const result = await deleteSessions(["session-a", "session-b", "session-c"])
+
+    // session-a was committed before the switch; session-b's response is stale
+    // and session-c is never attempted, so both are reported as failures.
+    expect(result).toEqual({ deletedIds: ["session-a"], failedIds: ["session-b", "session-c"] })
+    expect(source.getState().session.map((item) => item.id)).toEqual(["session-b", "session-c"])
+    expect(globalRemovedSessionIds).toEqual(["session-a"])
+    expect(replyCalls.filter((call) => call.method === "session.delete").map((call) => call.params.sessionID))
+      .toEqual(["session-a", "session-b"])
+  })
+
   test("does not archive locally until the server returns the archived session", async () => {
     const source = createStore({}, {
       session: [{ id: "session-a", directory: "/test/project", time: { created: 1 } } as Session],

packages/ui/src/sync/session-actions.ts

@@ -664,6 +664,16 @@ function isStaleRuntime(expectedRuntimeKey: string | undefined): boolean {
   return expectedRuntimeKey !== undefined && getRuntimeKey() !== expectedRuntimeKey
 }
 
+/**
+ * Read a session, apply `updater` to its metadata, and persist the result.
+ *
+ * `expectedRuntimeKey` is optional here and unguarded when omitted, unlike the
+ * archive and delete actions. When supplied, the runtime is rechecked before
+ * the read, before the write, and before the global store is updated; a change
+ * at any of those points **throws** `"runtime changed"` rather than returning a
+ * value, because this function must resolve to a `Session`. Callers that pass a
+ * key must therefore be prepared to catch that rejection.
+ */
 export async function patchSessionMetadata(
   sessionId: string,
   directory: string | null | undefined,
@@ -764,7 +774,21 @@ function cleanupSessionWorktreeMetadata(sessionId: string): void {
   useSessionUIStore.getState().setWorktreeMetadata(sessionId, null)
 }
 
-function finalizeConfirmedSessionDeletion(sessionId: string, sessionDirectory?: string): void {
+/**
+ * Commit a server-confirmed deletion.
+ *
+ * `expectedRuntimeKey` is the runtime the deletion was confirmed on. It is
+ * forwarded to `cleanupPersistedSessionState`, which rejects an identity whose
+ * runtime is no longer active. Passing the live `getRuntimeKey()` here would
+ * make that existing check a tautology, so the captured key is required to keep
+ * it meaningful. Callers must still reject a stale runtime themselves, because
+ * the in-memory live/global/UI stores mutated below are not runtime-scoped.
+ */
+function finalizeConfirmedSessionDeletion(
+  sessionId: string,
+  sessionDirectory?: string,
+  expectedRuntimeKey = getRuntimeKey(),
+): void {
   const snapshots = removeSessionFromLiveStores(sessionId, sessionDirectory)
   invalidateSessionLoads(sessionId, [...snapshots.map((snapshot) => snapshot.directory), sessionDirectory])
   useGlobalSessionsStore.getState().removeSessions([sessionId])
@@ -773,23 +797,59 @@ function finalizeConfirmedSessionDeletion(sessionId: string, sessionDirectory?:
   cleanupSessionWorktreeMetadata(sessionId)
   if (sessionDirectory) {
     cleanupPersistedSessionState({
-      runtimeKey: getRuntimeKey(),
+      runtimeKey: expectedRuntimeKey,
       directory: sessionDirectory,
       sessionId,
     })
   }
 }
 
-// eslint-disable-next-line @typescript-eslint/no-unused-vars
-export async function deleteSession(sessionId: string, _options?: Record<string, unknown>): Promise<boolean> {
+export type DeleteSessionOptions = {
+  /**
+   * Runtime key the deletion is scoped to. Defaults to the active runtime when
+   * the action starts; callers may supply a key captured earlier when
+   * confirmation spans a runtime switch.
+   */
+  expectedRuntimeKey?: string
+  /**
+   * Worktree flags accepted from `SessionDialogs`. This action does not consume
+   * them today — the dialog performs worktree removal itself. They are declared
+   * so the call site stays type-checked instead of hidden behind an untyped
+   * bag; wiring or removing them is separate follow-up work.
+   */
+  archiveWorktree?: boolean
+  deleteRemoteBranch?: boolean
+  deleteLocalBranch?: boolean
+}
+
+/**
+ * Delete one session.
+ *
+ * The runtime is rechecked before the request and again before any store is
+ * reconciled, so a response produced by the previous runtime cannot mutate the
+ * current runtime's state. Session IDs are not unique across runtimes, so
+ * committing a stale deletion could otherwise evict an unrelated session and
+ * erase its persisted queue, todos, drafts, folders, and pins.
+ *
+ * A `404` is treated as an already-completed deletion, but only when it is
+ * still authoritative for the captured runtime. After a runtime change the
+ * `404` describes either the previous runtime or a runtime this session never
+ * belonged to; neither justifies committing cleanup here, so the action reports
+ * failure and leaves reconciliation to the next authoritative load.
+ */
+export async function deleteSession(sessionId: string, options?: DeleteSessionOptions): Promise<boolean> {
+  const expectedRuntimeKey = options?.expectedRuntimeKey ?? getRuntimeKey()
+  if (isStaleRuntime(expectedRuntimeKey)) return false
   const sessionDirectory = getSessionDirectory(sessionId)
   try {
-    await cleanupReviewMetadataBeforeDelete(sessionId, sessionDirectory)
+    await cleanupReviewMetadataBeforeDelete(sessionId, sessionDirectory, expectedRuntimeKey)
+    if (isStaleRuntime(expectedRuntimeKey)) return false
     const deleted = await opencodeClient.deleteSession(sessionId, sessionDirectory)
+    if (isStaleRuntime(expectedRuntimeKey)) return false
     if (deleted !== true) {
       throw new Error("session.delete failed: server did not confirm deletion")
     }
-    finalizeConfirmedSessionDeletion(sessionId, sessionDirectory)
+    finalizeConfirmedSessionDeletion(sessionId, sessionDirectory, expectedRuntimeKey)
     return true
   } catch (error) {
     console.error("[session-actions] deleteSession failed", error)
@@ -797,7 +857,8 @@ export async function deleteSession(sessionId: string, _options?: Record<string,
     // Subsequent delete attempts for those children return 404; treat as
     // success since the session was already deleted by the cascade.
     if ((error as { status?: number })?.status === 404) {
-      finalizeConfirmedSessionDeletion(sessionId, sessionDirectory)
+      if (isStaleRuntime(expectedRuntimeKey)) return false
+      finalizeConfirmedSessionDeletion(sessionId, sessionDirectory, expectedRuntimeKey)
       return true
     }
     return false
@@ -805,25 +866,70 @@ export async function deleteSession(sessionId: string, _options?: Record<string,
 }
 
 /** Delete a session specifying which directory it lives in. Used by agent groups for cross-directory deletes. */
-export async function deleteSessionInDirectory(sessionId: string, directory: string): Promise<boolean> {
+export async function deleteSessionInDirectory(
+  sessionId: string,
+  directory: string,
+  expectedRuntimeKey = getRuntimeKey(),
+): Promise<boolean> {
+  if (isStaleRuntime(expectedRuntimeKey)) return false
   try {
-    await cleanupReviewMetadataBeforeDelete(sessionId, directory)
+    await cleanupReviewMetadataBeforeDelete(sessionId, directory, expectedRuntimeKey)
+    if (isStaleRuntime(expectedRuntimeKey)) return false
     const deleted = await opencodeClient.deleteSession(sessionId, directory)
+    if (isStaleRuntime(expectedRuntimeKey)) return false
     if (deleted !== true) {
       throw new Error("session.delete failed: server did not confirm deletion")
     }
-    finalizeConfirmedSessionDeletion(sessionId, directory)
+    finalizeConfirmedSessionDeletion(sessionId, directory, expectedRuntimeKey)
     return true
   } catch (error) {
     console.error("[session-actions] deleteSessionInDirectory failed", error)
     if ((error as { status?: number })?.status === 404) {
-      finalizeConfirmedSessionDeletion(sessionId, directory)
+      if (isStaleRuntime(expectedRuntimeKey)) return false
+      finalizeConfirmedSessionDeletion(sessionId, directory, expectedRuntimeKey)
       return true
     }
     return false
   }
 }
 
+export type DeleteSessionsOptions = {
+  /**
+   * Runtime key captured when the batch was confirmed. When supplied, the batch
+   * stops as soon as the active runtime differs.
+   */
+  expectedRuntimeKey?: string
+}
+
+/**
+ * Delete several sessions sequentially, preserving partial results.
+ *
+ * One failed session never blocks or erases the others: it is reported in
+ * `failedIds` while the remaining IDs are still attempted. When the runtime
+ * changes mid-batch, the sessions already committed on the captured runtime
+ * stay in `deletedIds` and every ID that was not committed there is reported in
+ * `failedIds`, so existing partial-failure feedback stays truthful.
+ */
+export async function deleteSessions(
+  ids: string[],
+  options?: DeleteSessionsOptions,
+): Promise<{ deletedIds: string[]; failedIds: string[] }> {
+  const deletedIds: string[] = []
+  const failedIds: string[] = []
+  const expectedRuntimeKey = options?.expectedRuntimeKey ?? getRuntimeKey()
+
+  for (const [index, id] of ids.entries()) {
+    if (isStaleRuntime(expectedRuntimeKey)) {
+      failedIds.push(...ids.slice(index))
+      break
+    }
+    if (await deleteSession(id, { expectedRuntimeKey })) deletedIds.push(id)
+    else failedIds.push(id)
+  }
+
+  return { deletedIds, failedIds }
+}
+
 /**
  * Archive one session.
  *

packages/ui/src/sync/session-ui-store.test.js

@@ -484,3 +484,42 @@ describe('archiveSessions option forwarding', () => {
     expect(updateSessionCalls).toEqual([]);
   });
 });
+
+describe('deleteSessions option forwarding', () => {
+  let originalDeleteSession;
+  let deleteSessionCalls;
+
+  beforeEach(() => {
+    deleteSessionCalls = [];
+    originalDeleteSession = opencodeClient.deleteSession;
+    opencodeClient.deleteSession = (sessionId) => {
+      deleteSessionCalls.push(sessionId);
+      return Promise.resolve(true);
+    };
+  });
+
+  afterEach(() => {
+    opencodeClient.deleteSession = originalDeleteSession;
+  });
+
+  // The store accepted an options object and dropped it on both the single and
+  // batch delete paths. A key that cannot match the active runtime must abort
+  // before any SDK call rather than deleting and erasing persisted state.
+  test('honors expectedRuntimeKey on the batch delete instead of discarding options', async () => {
+    const result = await useSessionUIStore.getState().deleteSessions(['session-x', 'session-y'], {
+      expectedRuntimeKey: 'runtime-that-is-not-active',
+    });
+
+    expect(result).toEqual({ deletedIds: [], failedIds: ['session-x', 'session-y'] });
+    expect(deleteSessionCalls).toEqual([]);
+  });
+
+  test('honors expectedRuntimeKey on the single delete instead of discarding options', async () => {
+    const deleted = await useSessionUIStore.getState().deleteSession('session-x', {
+      expectedRuntimeKey: 'runtime-that-is-not-active',
+    });
+
+    expect(deleted).toBe(false);
+    expect(deleteSessionCalls).toEqual([]);
+  });
+});

packages/ui/src/sync/session-ui-store.ts

@@ -46,6 +46,7 @@ import { setActiveSession } from "./sync-context"
 import {
   createSession as createSessionAction,
   deleteSession as deleteSessionAction,
+  deleteSessions as deleteSessionsAction,
   archiveSession as archiveSessionAction,
   archiveSessions as archiveSessionsAction,
   updateSessionTitle as updateSessionTitleAction,
@@ -58,6 +59,8 @@ import {
   forkFromMessage as forkFromMessageAction,
   fetchMessagesForSession,
   type ArchiveSessionsOptions,
+  type DeleteSessionOptions,
+  type DeleteSessionsOptions,
 } from "./session-actions"
 import { useInputStore, type SyntheticContextPart } from "./input-store"
 import { useSessionGoalArmStore } from "@/stores/useSessionGoalArmStore"
@@ -322,8 +325,8 @@ export type SessionUIState = {
   ) => Promise<void>
 
   createSession: (title?: string, directoryOverride?: string | null, parentID?: string | null, metadata?: Record<string, unknown>) => Promise<Session | null>
-  deleteSession: (id: string, options?: Record<string, unknown>) => Promise<boolean>
-  deleteSessions: (ids: string[], options?: Record<string, unknown>) => Promise<{ deletedIds: string[]; failedIds: string[] }>
+  deleteSession: (id: string, options?: DeleteSessionOptions) => Promise<boolean>
+  deleteSessions: (ids: string[], options?: DeleteSessionsOptions) => Promise<{ deletedIds: string[]; failedIds: string[] }>
   archiveSession: (id: string) => Promise<boolean>
   archiveSessions: (ids: string[], options?: ArchiveSessionsOptions) => Promise<{ archivedIds: string[]; failedIds: string[] }>
   updateSessionTitle: (sessionId: string, title: string) => Promise<void>
@@ -1291,18 +1294,9 @@ export const useSessionUIStore = create<SessionUIState>()((set, get) => ({
   // ---------------------------------------------------------------------------
   // deleteSession — calls SDK, SSE event updates child store
   // ---------------------------------------------------------------------------
-  deleteSession: (id) => deleteSessionAction(id),
+  deleteSession: (id, options) => deleteSessionAction(id, options),
 
-  deleteSessions: async (ids) => {
-    const deletedIds: string[] = []
-    const failedIds: string[] = []
-    for (const id of ids) {
-      const ok = await deleteSessionAction(id)
-      if (ok) deletedIds.push(id)
-      else failedIds.push(id)
-    }
-    return { deletedIds, failedIds }
-  },
+  deleteSessions: (ids, options) => deleteSessionsAction(ids, options),
 
   archiveSession: (id) => archiveSessionAction(id),