From d19ff96c02793fcdeed2c9f6c0481bd28e14c315 Mon Sep 17 00:00:00 2001 From: Alexandre Reyes Martins Date: Sun, 2 Aug 2026 15:13:06 +0000 Subject: [PATCH] fix(sync): guard delete actions by default Follow-up to #2574 and f95f1ab18, which guarded the archive path. The delete path had the same two defects and worse consequences. `useSessionUIStore.deleteSession`/`deleteSessions` declared an `options` parameter and discarded it on both paths, so a caller-supplied runtime key was a silent no-op. `SessionDialogs.tsx:416` already passes options today and they never reach the action. The delete path also never rechecked the runtime. Session IDs are not unique across runtimes, so a response produced by a previous runtime could commit `finalizeConfirmedSessionDeletion` against the runtime the user switched to: evicting an unrelated session from the live and global stores and calling `cleanupPersistedSessionState`, which erases queued messages, todos, folder membership, inline-comment drafts, chat draft, and pins. That is user data loss, not stale cache. `cleanupPersistedSessionState` already rejects an identity whose runtime is no longer active, but `finalizeConfirmedSessionDeletion` defeated that check by passing the live `getRuntimeKey()` at commit time, comparing a value with itself. It now forwards the captured key. Adopt the default-on shape from f95f1ab18: `expectedRuntimeKey` defaults to the active runtime in `deleteSession`, `deleteSessionInDirectory` and the new canonical `deleteSessions` action, and is rechecked before the request and before every reconciliation. A `404` still means "already deleted" and commits cleanup, but only while the captured runtime is active; after a switch it describes the wrong runtime and the action reports failure instead of committing. Also documents the throw contract of `patchSessionMetadata`, a non-blocking nit raised by the review bot on #2574. --- packages/ui/src/sync/DOCUMENTATION.md | 33 ++++- packages/ui/src/sync/session-actions.test.ts | 106 +++++++++++++++ packages/ui/src/sync/session-actions.ts | 128 ++++++++++++++++-- packages/ui/src/sync/session-ui-store.test.js | 39 ++++++ packages/ui/src/sync/session-ui-store.ts | 20 +-- 5 files changed, 297 insertions(+), 29 deletions(-) diff --git a/packages/ui/src/sync/DOCUMENTATION.md b/packages/ui/src/sync/DOCUMENTATION.md index c8056edc..93a475ff 100644 --- a/packages/ui/src/sync/DOCUMENTATION.md +++ b/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. diff --git a/packages/ui/src/sync/session-actions.test.ts b/packages/ui/src/sync/session-actions.test.ts index b23f6e19..85da9ffc 100644 --- a/packages/ui/src/sync/session-actions.test.ts +++ b/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], diff --git a/packages/ui/src/sync/session-actions.ts b/packages/ui/src/sync/session-actions.ts index c554b9c6..6f1f6b4d 100644 --- a/packages/ui/src/sync/session-actions.ts +++ b/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): Promise { +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 { + 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 { +export async function deleteSessionInDirectory( + sessionId: string, + directory: string, + expectedRuntimeKey = getRuntimeKey(), +): Promise { + 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. * diff --git a/packages/ui/src/sync/session-ui-store.test.js b/packages/ui/src/sync/session-ui-store.test.js index 5d4dec03..d60be0fe 100644 --- a/packages/ui/src/sync/session-ui-store.test.js +++ b/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([]); + }); +}); diff --git a/packages/ui/src/sync/session-ui-store.ts b/packages/ui/src/sync/session-ui-store.ts index d552029a..33618f71 100644 --- a/packages/ui/src/sync/session-ui-store.ts +++ b/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 createSession: (title?: string, directoryOverride?: string | null, parentID?: string | null, metadata?: Record) => Promise - deleteSession: (id: string, options?: Record) => Promise - deleteSessions: (ids: string[], options?: Record) => Promise<{ deletedIds: string[]; failedIds: string[] }> + deleteSession: (id: string, options?: DeleteSessionOptions) => Promise + deleteSessions: (ids: string[], options?: DeleteSessionsOptions) => Promise<{ deletedIds: string[]; failedIds: string[] }> archiveSession: (id: string) => Promise archiveSessions: (ids: string[], options?: ArchiveSessionsOptions) => Promise<{ archivedIds: string[]; failedIds: string[] }> updateSessionTitle: (sessionId: string, title: string) => Promise @@ -1291,18 +1294,9 @@ export const useSessionUIStore = create()((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),