diff --git a/apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts b/apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts index 8abe78c452..c4c03d52f3 100644 --- a/apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts +++ b/apps/desktop/src/main/__tests__/app-shell-session-purge.test.ts @@ -33,8 +33,12 @@ function restored(id: string): SessionSummary { type SweepHarness = { removed: string[]; + /** Each `remove` call as `[sessionId, requireArchived]`. */ + removeOptions: Array<[string, boolean]>; cleared: string[]; selections: Array; + /** Titles of the success toasts a row action raised. */ + toasts: string[]; listCalls: number; }; @@ -50,6 +54,12 @@ function installWindow( surviving?: readonly SessionSummary[]; /** Runs after each accepted removal, to model what another client did meanwhile. */ onRemove?: (sessionId: string) => void; + /** + * The catalog the fake Host decides against. It checks the archived premise + * here, where the real one checks it inside its compare-and-set — not + * against whatever the renderer last saw. + */ + catalog?: readonly SessionSummary[]; } = {}, ): () => void { const target = globalThis as unknown as { window?: unknown }; @@ -60,10 +70,14 @@ function installWindow( value: { maka: { sessions: { - remove: async (id: string) => { + remove: async (id: string, removeOptions?: { requireArchived?: boolean }) => { + harness.removeOptions.push([id, removeOptions?.requireArchived === true]); if (options.rejectIds?.includes(id)) throw new Error(`busy:${id}`); + const target = options.catalog?.find((session) => session.id === id); + if (removeOptions?.requireArchived && target && !target.isArchived) return 'restored'; harness.removed.push(id); options.onRemove?.(id); + return 'removed'; }, list: async () => { harness.listCalls += 1; @@ -101,12 +115,25 @@ function createActions(input: { input.activeIdRef.current = id; }, setMessages: () => undefined, - toastApi: { success: () => undefined, error: () => undefined, confirm: async () => true }, + toastApi: { + success: (title: string) => { + input.harness.toasts.push(title); + }, + error: () => undefined, + confirm: async () => true, + }, }); } function harness(): SweepHarness { - return { removed: [], cleared: [], selections: [], listCalls: 0 }; + return { + removed: [], + removeOptions: [], + cleared: [], + selections: [], + toasts: [], + listCalls: 0, + }; } describe('purgeSessions', () => { @@ -124,7 +151,18 @@ describe('purgeSessions', () => { const outcome = await actions.purgeSessions(['a-v2', 'b']).finally(restore); assert.deepEqual(h.removed, ['a-v2', 'b']); - assert.deepEqual(outcome, { removed: 2, remaining: [], verified: true, firstError: undefined }); + assert.deepEqual(outcome, { + removed: 2, + remaining: [], + restored: [], + verified: true, + firstError: undefined, + }); + // Every delete in a sweep carries the archived premise the confirm named. + assert.deepEqual(h.removeOptions, [ + ['a-v2', true], + ['b', true], + ]); // The family goes, not just the representative, and the open member of it // stops being the active session. assert.deepEqual(h.cleared.sort(), ['a', 'a-v2', 'b']); @@ -133,43 +171,101 @@ describe('purgeSessions', () => { assert.equal(h.listCalls, 0); }); - it('leaves a task that stopped being archived before the sweep reached it', async () => { + it('reports a task restored before the sweep reached it, rather than dropping it', async () => { // The confirm named a set. One restored from another surface while the // dialog was up has left it, and a sweep that deleted it anyway would be - // acting outside what was agreed to. + // acting outside what was agreed to. Reporting it is the other half: the + // person agreed to two and one went, which needs saying. const h = harness(); - const sessions = [restored('kept'), summary('doomed')]; - const restore = installWindow(h); - const actions = createActions({ harness: h, sessions, activeIdRef: { current: undefined } }); + const catalog = [restored('kept'), summary('doomed')]; + const restore = installWindow(h, { catalog }); + const actions = createActions({ + harness: h, + sessions: [...catalog], + activeIdRef: { current: undefined }, + }); const outcome = await actions.purgeSessions(['kept', 'doomed']).finally(restore); assert.deepEqual(h.removed, ['doomed']); assert.equal(outcome.removed, 1); + assert.deepEqual(outcome.restored, ['kept']); assert.deepEqual(outcome.remaining, []); + // Kept tasks are settled by the delete itself; nothing to check back. + assert.equal(h.listCalls, 0); }); - it('leaves a task restored while the sweep was already running', async () => { + it('reports a task restored while the sweep was already running', async () => { // The page disables its own controls during a sweep, so the restore comes - // from a second window. A set of archived ids snapshotted before the loop - // would not see it, and would delete a task that had left the set the - // confirm named — the catalog has to be read as each task is reached. + // from a second window, landing after the sweep started and before it + // reached this task. const h = harness(); - const sessions = [summary('first'), summary('second')]; + const catalog = [summary('first'), summary('second')]; const restore = installWindow(h, { + catalog, onRemove: (id) => { - if (id === 'first') sessions[1] = restored('second'); + if (id === 'first') catalog[1] = restored('second'); }, }); - const actions = createActions({ harness: h, sessions, activeIdRef: { current: undefined } }); + const actions = createActions({ + harness: h, + sessions: [...catalog], + activeIdRef: { current: undefined }, + }); const outcome = await actions.purgeSessions(['first', 'second']).finally(restore); assert.deepEqual(h.removed, ['first']); assert.equal(outcome.removed, 1); + assert.deepEqual(outcome.restored, ['second']); assert.deepEqual(outcome.remaining, []); }); + it('keeps everything the renderer holds for a task the delete left alone', async () => { + const h = harness(); + const catalog = [summary('first'), restored('rescued')]; + const restore = installWindow(h, { catalog }); + const activeIdRef = { current: 'rescued' as string | undefined }; + const actions = createActions({ harness: h, sessions: [...catalog], activeIdRef }); + + const outcome = await actions.purgeSessions(['first', 'rescued']).finally(restore); + + assert.deepEqual(outcome.restored, ['rescued']); + assert.equal(outcome.firstError, undefined); + // A task that is still there keeps its renderer state, including being the + // open one. + assert.deepEqual(h.cleared, ['first']); + assert.deepEqual(h.selections, []); + assert.equal(activeIdRef.current, 'rescued'); + }); + + it('sends every id to the delete instead of deciding against its own snapshot', async () => { + // The renderer's list is one observer of a state the Host owns, and a + // serial sweep gives a second window plenty of room to outdate it — in + // either direction. Here the snapshot is stale in the direction that + // silently spares a task: it reads `stale` as no longer archived while the + // catalog the delete commits against still has it archived. Filtering here + // would drop the id with no outcome at all, which is how a confirmed count + // stops adding up. + const h = harness(); + const restore = installWindow(h, { catalog: [summary('stale'), summary('plain')] }); + const actions = createActions({ + harness: h, + sessions: [restored('stale'), summary('plain')], + activeIdRef: { current: undefined }, + }); + + const outcome = await actions.purgeSessions(['stale', 'plain']).finally(restore); + + assert.deepEqual(h.removeOptions, [ + ['stale', true], + ['plain', true], + ]); + assert.deepEqual(h.removed, ['stale', 'plain']); + assert.equal(outcome.removed, 2); + assert.deepEqual(outcome.restored, []); + }); + it('skips an id whose row action is already in flight instead of racing it', async () => { const h = harness(); const sessions = [summary('busy'), summary('free')]; @@ -228,3 +324,44 @@ describe('purgeSessions', () => { assert.equal(outcome.removed, 0); }); }); + +describe('deleteSession', () => { + it('carries the archived premise of the row the confirm named', async () => { + // Deleting from 已归档任务 is the same decision a sweep makes, one row at a + // time, so a restore revokes it the same way. + const h = harness(); + const sessions = [summary('archived-row'), restored('active-row')]; + const restore = installWindow(h); + const actions = createActions({ harness: h, sessions, activeIdRef: { current: undefined } }); + + await actions.deleteSession('archived-row'); + await actions.deleteSession('active-row'); + restore(); + + // An active task never had an archived premise to lose, so requiring one + // would refuse every delete from the rail. + assert.deepEqual(h.removeOptions, [ + ['archived-row', true], + ['active-row', false], + ]); + }); + + it('keeps a task the Host reports as restored, and says so', async () => { + const h = harness(); + // The row was archived when the confirm named it; the catalog the delete + // commits against says otherwise by the time it lands. + const sessions = [summary('rescued')]; + const restore = installWindow(h, { catalog: [restored('rescued')] }); + const activeIdRef = { current: 'rescued' as string | undefined }; + const actions = createActions({ harness: h, sessions, activeIdRef }); + + await actions.deleteSession('rescued'); + restore(); + + assert.deepEqual(h.removed, []); + assert.deepEqual(h.cleared, []); + assert.equal(activeIdRef.current, 'rescued'); + // Not "Deleted rescued": nothing was. + assert.deepEqual(h.toasts, ['rescued was restored, so it was kept']); + }); +}); diff --git a/apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts b/apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts index 9e29039ede..b26c28f272 100644 --- a/apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts +++ b/apps/desktop/src/main/__tests__/app-shell-session-row-actions-revisions.test.ts @@ -34,7 +34,7 @@ function installWindow(calls: string[]): () => void { archive: async (id: string, options?: { revisionFamily?: boolean }) => { calls.push(`archive:${id}:${options?.revisionFamily === true}`); }, unarchive: async (id: string, options?: { revisionFamily?: boolean }) => { calls.push(`unarchive:${id}:${options?.revisionFamily === true}`); }, rename: async (id: string, name: string, options?: { revisionFamily?: boolean }) => { calls.push(`rename:${id}:${name}:${options?.revisionFamily === true}`); }, - remove: async (id: string, options?: { revisionFamily?: boolean }) => { calls.push(`remove:${id}:${options?.revisionFamily === true}`); }, + remove: async (id: string, options?: { revisionFamily?: boolean; requireArchived?: boolean }) => { calls.push(`remove:${id}:${options?.revisionFamily === true}:${options?.requireArchived === true}`); return 'removed' as const; }, }, }, }, @@ -88,7 +88,9 @@ describe('revision-family session row actions', () => { 'flag:version:true:true', 'rename:branch:Independent branch:true', 'archive:version:true', - 'remove:root:true', + // `root` is not archived, so the delete states no archived premise — + // requiring one would refuse every delete from the rail. + 'remove:root:true:false', ]); assert.deepEqual(selections, [undefined, undefined]); assert.deepEqual(cleared, ['root', 'version', 'root', 'version']); diff --git a/apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts b/apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts index 3a3705c0aa..c3d605d424 100644 --- a/apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts +++ b/apps/desktop/src/main/__tests__/runtime-host-client-operations.test.ts @@ -240,6 +240,57 @@ test('retries a Session update through transient revision churn', async () => { assert.equal(updated.collaborationMode, 'plan'); }); +test('abandons a remove whose task was restored under it', async () => { + // A lifecycle write bumps the revision, so the conflict IS the restore: the + // premise the caller decided on ("this task is archived") no longer holds, + // and replaying the delete at the fresh revision destroys a task somebody + // just pulled back out of the archive. + const { client, requests } = clientWithResponses([ + { kind: 'session', session: session('session-1', 4, { isArchived: true }) }, + { kind: 'revision_conflict', expectedRevision: 4, actualRevision: 5 }, + { kind: 'session', session: session('session-1', 5, { isArchived: false }) }, + // Only a replayed delete reaches this, and reaching it is the defect. + { kind: 'removed' }, + ]); + + assert.equal(await client.removeSession('session-1', { requireArchived: true }), 'restored'); + assert.deepEqual( + requests.map(({ operation }) => operation), + ['session.catalog.query', 'session.remove', 'session.catalog.query'], + ); +}); + +test('retries a remove through revision churn that left the task archived', async () => { + // Not every conflict is a restore. A task still archived at the fresh + // revision was only written around, and the delete still means what it did. + const { client, requests } = clientWithResponses([ + { kind: 'session', session: session('session-1', 4, { isArchived: true }) }, + { kind: 'revision_conflict', expectedRevision: 4, actualRevision: 5 }, + { kind: 'session', session: session('session-1', 5, { isArchived: true }) }, + { kind: 'removed' }, + ]); + + assert.equal(await client.removeSession('session-1', { requireArchived: true }), 'removed'); + assert.deepEqual( + requests.filter(({ operation }) => operation === 'session.remove').map(({ input }) => input), + [ + { sessionId: 'session-1', expectedRevision: 4 }, + { sessionId: 'session-1', expectedRevision: 5 }, + ], + ); +}); + +test('removes a task that was never archived when no premise was stated', async () => { + // Deleting an active task from the rail has no archived premise to lose, so + // the precondition is the caller's to ask for, not the client's to assume. + const { client } = clientWithResponses([ + { kind: 'session', session: session('session-1', 4) }, + { kind: 'removed' }, + ]); + + assert.equal(await client.removeSession('session-1'), 'removed'); +}); + test('rebuilds a Runtime Policy mutation from each fresh CAS projection', async () => { const initial = createDefaultRuntimePolicy(); const concurrent = { diff --git a/apps/desktop/src/main/__tests__/runtime-host-client-uds.test.ts b/apps/desktop/src/main/__tests__/runtime-host-client-uds.test.ts index 77954d09d6..03b4fd06a2 100644 --- a/apps/desktop/src/main/__tests__/runtime-host-client-uds.test.ts +++ b/apps/desktop/src/main/__tests__/runtime-host-client-uds.test.ts @@ -103,6 +103,8 @@ test('drives the renderer Session catalog facade through real UDS framing', asyn const base = await mkdtemp(join(tmpdir(), 'maka-desktop-host-ipc-')); let host: RuntimeHostKernel | undefined; let projected: SessionCatalogProjection | undefined; + /** Arms one concurrent restore, landing between the Client's read and its remove. */ + let restoreUnderNextRemove = false; try { const capability = await resolveStorageRoot({ path: base, kind: 'interactive' }); const owner = await tryAcquireInteractiveRootOwner(capability); @@ -180,6 +182,26 @@ test('drives the renderer Session catalog facade through real UDS framing', asyn }, 'session.remove': async (input) => { assert.ok(projected); + if (restoreUnderNextRemove) { + // Another window restored the task between the Client's read and + // this write. The Host rejects the stale revision, which is what + // a restore looks like from here. + restoreUnderNextRemove = false; + projected = session(projected.id, { + ...projected, + revision: projected.revision + 1, + isArchived: false, + status: 'active', + }); + return { + ok: true, + result: { + kind: 'revision_conflict', + expectedRevision: input.expectedRevision, + actualRevision: projected.revision, + }, + }; + } assert.equal(input.expectedRevision, projected.revision); const sessionId = projected.id; projected = undefined; @@ -240,12 +262,24 @@ test('drives the renderer Session catalog facade through real UDS framing', asyn ); await ipc.invoke('sessions:archive', 'session-ipc'); assert.equal((await ipc.invoke('sessions:list') as Array<{ isArchived: boolean }>)[0]?.isArchived, true); - await ipc.invoke('sessions:remove', 'session-ipc'); + // A purge sweep asks for the task it saw archived. Restored under it, the + // deletion is called off rather than replayed at the fresh revision (#3050). + restoreUnderNextRemove = true; + assert.equal( + await ipc.invoke('sessions:remove', 'session-ipc', { revisionFamily: true, requireArchived: true }), + 'restored', + ); + assert.equal((await ipc.invoke('sessions:list') as Array<{ isArchived: boolean }>)[0]?.isArchived, false); + await ipc.invoke('sessions:archive', 'session-ipc'); + assert.equal(await ipc.invoke('sessions:remove', 'session-ipc'), 'removed'); assert.deepEqual(await ipc.invoke('sessions:list'), []); + // Nothing was retired for the restored task: no `deleted` between the two + // archives, and the renderer keeps everything it holds for it. assert.deepEqual(changes, [ { reason: 'created', sessionId: 'session-ipc' }, { reason: 'mode-change', sessionId: 'session-ipc' }, { reason: 'archived', sessionId: 'session-ipc' }, + { reason: 'archived', sessionId: 'session-ipc' }, { reason: 'deleted', sessionId: 'session-ipc' }, ]); diff --git a/apps/desktop/src/main/runtime-host-client.ts b/apps/desktop/src/main/runtime-host-client.ts index 47ba8a108e..5743c99b22 100644 --- a/apps/desktop/src/main/runtime-host-client.ts +++ b/apps/desktop/src/main/runtime-host-client.ts @@ -113,6 +113,12 @@ const MAX_PRICING_SNAPSHOT_ATTEMPTS = 3; export type DesktopSessionConfigurationPatch = Partial; +/** + * How a remove settled. `restored` is not a failure: the task left the state + * the caller decided against, so nothing was destroyed and nothing is wrong. + */ +export type SessionRemoveDisposition = "removed" | "restored"; + export type DesktopRuntimeHostClientErrorCode = | "catalog_unstable" | "client_closed" @@ -838,14 +844,36 @@ export class DesktopRuntimeHostClient { ); } - async removeSession(sessionId: string): Promise { + /** + * Removes a Session, optionally only while it is still archived. + * + * Replaying a rejected write at the fresh revision is right for a rename or a + * configuration patch — the write means the same thing either way. It is + * wrong for a remove: a lifecycle write bumps the revision, so a conflict can + * be the task being restored, and replaying then destroys a task whose + * deletion nobody asked for any more. + * + * `requireArchived` states the premise the caller decided on. Re-asserting it + * against each fresh read is enough to hold it through the commit, because + * the Host serializes the two writes that could disagree: `session.remove` + * and `session.lifecycle.set` both enter `#withStableFamily`, which queues + * per Session id through the admission gate, and the remove compares the + * revision on the way in. So a restore either lands before that comparison — + * bumping `metadataVersion` and rejecting the remove — or waits until the + * retirement has finished. It cannot land between the check and the delete. + */ + async removeSession( + sessionId: string, + options: { requireArchived?: boolean } = {}, + ): Promise { for (let attempt = 0; attempt < MAX_SESSION_REVISION_ATTEMPTS; attempt += 1) { const current = await this.#requireSession(sessionId); + if (options.requireArchived && !current.isArchived) return "restored"; const result = await this.request("session.remove", { sessionId, expectedRevision: current.revision, }); - if (result.kind === "removed") return; + if (result.kind === "removed") return "removed"; } throw revisionConflict("remove", sessionId); } diff --git a/apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts b/apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts index 8168b4bc7b..1140123360 100644 --- a/apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts +++ b/apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts @@ -191,11 +191,35 @@ export function registerRuntimeHostSessionCatalogIpc( ipcMain.handle('sessions:remove', async (_event, sessionId: string, options?: unknown) => { requestsRevisionFamily(options); const ids = await actionIds(sessionId, { revisionFamily: true }); - await deps.client.removeSession(sessionId); - await finishSessionRetirement(deps, ids, 'deleted'); + // A task restored under the caller's decision is left alone, and nothing + // downstream of the deletion runs for it. + const disposition = await deps.client.removeSession(sessionId, { + requireArchived: requiresArchivedSession(options), + }); + if (disposition === 'removed') await finishSessionRetirement(deps, ids, 'deleted'); + return disposition; }); } +/** + * Reads the archived premise off the remove options. + * + * This guards a permanent deletion, so it refuses anything it cannot read + * rather than falling through to "no premise stated" — which would be the + * destructive answer. It repeats the shape check its sibling does instead of + * relying on the caller running that one first. + */ +function requiresArchivedSession(options: unknown): boolean { + if (options === undefined) return false; + if (!options || typeof options !== 'object' || Array.isArray(options)) { + throw new Error('Invalid session family action options'); + } + const value = (options as { requireArchived?: unknown }).requireArchived; + if (value === undefined) return false; + if (typeof value !== 'boolean') throw new Error('Invalid requireArchived option'); + return value; +} + async function finishSessionRetirement( deps: RuntimeHostSessionCatalogIpcDeps, sessionIds: readonly string[], diff --git a/apps/desktop/src/preload/bridge-contract.d.ts b/apps/desktop/src/preload/bridge-contract.d.ts index 4c7002974b..4218aa4693 100644 --- a/apps/desktop/src/preload/bridge-contract.d.ts +++ b/apps/desktop/src/preload/bridge-contract.d.ts @@ -522,7 +522,14 @@ export interface MakaBridge { abandonPlanExecution(sessionId: string, executionId: string): Promise; setModel(sessionId: string, input: { llmConnectionSlug: string; model: string }): Promise; setThinkingLevel(sessionId: string, level: ThinkingLevel | undefined | null): Promise; - remove(sessionId: string, options?: { revisionFamily?: boolean }): Promise; + /** + * `requireArchived` holds the caller's premise through the deletion: a task + * restored meanwhile answers `restored` and is kept. + */ + remove( + sessionId: string, + options?: { revisionFamily?: boolean; requireArchived?: boolean }, + ): Promise<'removed' | 'restored'>; cleanupSessionCopy(sessionId: string): Promise; abandonSessionCopy(sourceSessionId: string, copyId: string): Promise; }; diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index 90fbcfc828..a1a2f181b2 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -1345,7 +1345,10 @@ const makaBridge = { setThinkingLevel(sessionId: string, level: ThinkingLevel | undefined | null): Promise { return invokeSessionSummary('sessions:setThinkingLevel', sessionId, level ?? undefined); }, - remove(sessionId: string, options?: { revisionFamily?: boolean }): Promise { + remove( + sessionId: string, + options?: { revisionFamily?: boolean; requireArchived?: boolean }, + ): Promise<'removed' | 'restored'> { return invokeSessionRuntimeHost('sessions:remove', sessionId, options); }, cleanupSessionCopy(sessionId: string): Promise { diff --git a/apps/desktop/src/renderer/app-shell-session-row-actions.ts b/apps/desktop/src/renderer/app-shell-session-row-actions.ts index 4ba076c25c..ba51998638 100644 --- a/apps/desktop/src/renderer/app-shell-session-row-actions.ts +++ b/apps/desktop/src/renderer/app-shell-session-row-actions.ts @@ -5,6 +5,9 @@ import { revisionFamilySessionIds } from '@maka/core/session-revisions'; type RefBox = { current: T }; +/** What `sessions.remove` settled on. `restored` means the task is still there. */ +type SessionRemoveDisposition = 'removed' | 'restored'; + type ToastApi = { success(title: string, description?: string): void; error(title: string, description?: string): void; @@ -26,6 +29,11 @@ export interface SessionPurgeOutcome { removed: number; /** Tasks the catalog still reports. Empty when `verified` is false. */ remaining: string[]; + /** + * Tasks restored while the sweep was reaching them. Neither removed nor + * failed: the deletion was called off because its premise was gone. + */ + restored: string[]; verified: boolean; /** First rejection, so the caller can show a reason rather than a count. */ firstError: unknown; @@ -129,9 +137,14 @@ export function createAppShellSessionRowActions(deps: { destructive: true, }); if (!ok) return; - await removeSessionFamily(sessionId); + // The confirm named an archived task, so a restore revokes it. An active + // task has no such premise to lose. + const disposition = await removeSessionFamily(sessionId, { + requireArchived: session?.isArchived === true, + }); await refreshSessions(); - toastApi.success(copy.deletedTitle(name)); + if (disposition === 'restored') toastApi.success(copy.deleteRestoredTitle(name)); + else toastApi.success(copy.deletedTitle(name)); }); } @@ -139,27 +152,42 @@ export function createAppShellSessionRowActions(deps: { * Removes one task's whole revision family and drops what the renderer was * holding for it. A resolved `remove` means the IPC both committed the * deletion and released those resources, so the cleanup below is only ever - * reached for a task that is really gone. + * reached for a task that is really gone — and `restored` means it was never + * deleted, so there is nothing to drop. */ - async function removeSessionFamily(sessionId: string): Promise { + async function removeSessionFamily( + sessionId: string, + options: { requireArchived: boolean }, + ): Promise { // Read before the write: the family comes off the live catalog, which no // longer lists it afterwards. const familyIds = revisionFamilySessionIds(sessionsRef.current, sessionId); - await window.maka.sessions.remove(sessionId, { revisionFamily: true }); + const disposition = await window.maka.sessions.remove(sessionId, { + revisionFamily: true, + requireArchived: options.requireArchived, + }); + if (disposition === 'restored') return disposition; if (activeIdRef.current && familyIds.includes(activeIdRef.current)) { setActiveId(undefined); setMessages([]); } for (const id of familyIds) clearSessionRendererState(id); + return disposition; } /** * Deletes a set of archived tasks in one sweep. * - * Only tasks still archived when the sweep reaches them are touched: the - * caller named a set to a person, and one restored while that dialog was up - * has left it. Ids with a row action already in flight are skipped for the - * same reason single-row actions skip each other. + * Every id takes one path and lands in exactly one outcome. A task still + * archived is removed; one restored meanwhile answers `restored` and is kept; + * one already gone elsewhere rejects and settles as removed against the + * catalog; anything else is an error to explain. The archived premise is + * asserted where it can be held — inside the Host's compare-and-set (#3050) — + * rather than against a renderer snapshot that a second window can outdate + * between the check and the write. + * + * Ids with a row action already in flight are skipped for the same reason + * single-row actions skip each other. * * A rejection is not evidence the task survived — the delete IPC commits the * removal before it releases renderer resources — so the rejected ids, and @@ -173,20 +201,10 @@ export function createAppShellSessionRowActions(deps: { */ async function purgeSessions(sessionIds: readonly string[]): Promise { const unsettled: string[] = []; + const restored: string[] = []; let firstError: unknown; let removed = 0; for (const sessionId of sessionIds) { - // Read the catalog as the sweep reaches each task, not once up front. A - // sweep is serial, and a task restored from another window while it runs - // has left the set the confirm named; a snapshot taken at the start would - // delete it anyway. - // - // This narrows the window, it does not close it: nothing here can stop a - // restore that lands between this check and the removal. Two calls cannot - // be made atomic by the side that makes them — only the Host, which owns - // the lifecycle, can require the task to still be archived as it removes - // it (#3050). - if (!sessionsRef.current.some((s) => s.id === sessionId && s.isArchived)) continue; const key = `${sessionId}:delete`; if ( Array.from(pendingSessionRowActionsRef.current).some((pending) => @@ -198,8 +216,9 @@ export function createAppShellSessionRowActions(deps: { } pendingSessionRowActionsRef.current.add(key); try { - await removeSessionFamily(sessionId); - removed += 1; + const disposition = await removeSessionFamily(sessionId, { requireArchived: true }); + if (disposition === 'restored') restored.push(sessionId); + else removed += 1; } catch (error) { unsettled.push(sessionId); firstError ??= error; @@ -209,7 +228,7 @@ export function createAppShellSessionRowActions(deps: { } if (unsettled.length === 0) { await refreshSessions(); - return { removed, remaining: [], verified: true, firstError }; + return { removed, remaining: [], restored, verified: true, firstError }; } let listed: SessionSummary[] | undefined; try { @@ -218,10 +237,16 @@ export function createAppShellSessionRowActions(deps: { listed = undefined; } await refreshSessions(); - if (!listed) return { removed, remaining: [], verified: false, firstError }; + if (!listed) return { removed, remaining: [], restored, verified: false, firstError }; const present = new Set(listed.map((session) => session.id)); const remaining = unsettled.filter((sessionId) => present.has(sessionId)); - return { removed: removed + (unsettled.length - remaining.length), remaining, verified: true, firstError }; + return { + removed: removed + (unsettled.length - remaining.length), + remaining, + restored, + verified: true, + firstError, + }; } return { diff --git a/apps/desktop/src/renderer/locales/settings-tasks-copy.ts b/apps/desktop/src/renderer/locales/settings-tasks-copy.ts index 8de4c6f9d2..8dc786ed73 100644 --- a/apps/desktop/src/renderer/locales/settings-tasks-copy.ts +++ b/apps/desktop/src/renderer/locales/settings-tasks-copy.ts @@ -16,6 +16,12 @@ export type SettingsTasksCopy = { purgeConfirmBody: string; purgeConfirmAction: string; purgedToast(count: number): string; + /** + * Tasks a sweep kept because they were restored while it ran. Reads after + * either outcome, so a sweep never has to choose between reporting a failure + * and reporting what it deliberately left alone. + */ + purgeKeptRestored(count: number): string; purgeFailedTitle: string; purgeFailedBody(count: number): string; purgeUnverified: string; @@ -41,6 +47,7 @@ const SETTINGS_TASKS_COPY_BY_LOCALE = { purgeConfirmBody: '这些任务及其全部消息会被永久删除,无法撤销。', purgeConfirmAction: '永久删除', purgedToast: (count: number) => `已删除 ${count} 条任务`, + purgeKeptRestored: (count: number) => `另有 ${count} 条在此期间被恢复,已保留。`, purgeFailedTitle: '删除任务失败', purgeFailedBody: (count: number) => `${count} 条仍在,请重试。`, purgeUnverified: '任务已删除,但无法读取列表确认结果。请重新打开本页查看。', @@ -67,6 +74,10 @@ const SETTINGS_TASKS_COPY_BY_LOCALE = { 'The tasks and all of their messages are removed permanently. This cannot be undone.', purgeConfirmAction: 'Delete permanently', purgedToast: (count: number) => (count === 1 ? 'Deleted 1 task' : `Deleted ${count} tasks`), + purgeKeptRestored: (count: number) => + count === 1 + ? '1 more was restored meanwhile and kept.' + : `${count} more were restored meanwhile and kept.`, purgeFailedTitle: 'Could not delete the tasks', purgeFailedBody: (count: number) => count === 1 ? '1 task is still there. Try again.' : `${count} tasks are still there. Try again.`, diff --git a/apps/desktop/src/renderer/locales/shell-copy.ts b/apps/desktop/src/renderer/locales/shell-copy.ts index bd604b6d4e..c1d69a50ea 100644 --- a/apps/desktop/src/renderer/locales/shell-copy.ts +++ b/apps/desktop/src/renderer/locales/shell-copy.ts @@ -229,6 +229,8 @@ type ShellCopy = { deleteLabel: string; cancelLabel: string; deletedTitle(name: string): string; + /** The task was restored elsewhere, so the delete was called off. */ + deleteRestoredTitle(name: string): string; }; skillActions: { refreshSkillsFailedTitle: string; @@ -795,6 +797,7 @@ const SHELL_COPY_BY_LOCALE = { deleteLabel: '删除', cancelLabel: '取消', deletedTitle: (name: string) => `已删除 ${name}`, + deleteRestoredTitle: (name: string) => `${name} 已被恢复,未删除`, }, skillActions: { refreshSkillsFailedTitle: '刷新技能失败', @@ -1262,6 +1265,7 @@ const SHELL_COPY_BY_LOCALE = { deleteLabel: 'Delete', cancelLabel: 'Cancel', deletedTitle: (name: string) => `Deleted ${name}`, + deleteRestoredTitle: (name: string) => `${name} was restored, so it was kept`, }, skillActions: { refreshSkillsFailedTitle: 'Could not refresh Skills', diff --git a/apps/desktop/src/renderer/settings/tasks-settings-page.tsx b/apps/desktop/src/renderer/settings/tasks-settings-page.tsx index 3d0bdcaef0..b9b5bde06c 100644 --- a/apps/desktop/src/renderer/settings/tasks-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/tasks-settings-page.tsx @@ -90,8 +90,8 @@ export function TasksSettingsPage(props: ArchivedTasksBridge) { // Frozen at the click. A confirm names a number to a person, and a set // re-read afterwards can be larger than the one they agreed to — another // client archiving a task while the dialog is up would add it. Shrinking is - // safe and happens at the other end: `onPurge` skips anything no longer - // archived, so a task restored meanwhile is left alone. + // safe and happens at the other end: `onPurge` keeps anything restored + // meanwhile and says so. const ids = purgeTargets.map((session) => session.id); const confirmed = await toast.confirm({ title: isSearching @@ -106,19 +106,23 @@ export function TasksSettingsPage(props: ArchivedTasksBridge) { setPurging(true); try { const outcome = await props.onPurge(ids); - if (!outcome.verified) { - toast.error(copy.purgeFailedTitle, copy.purgeUnverified); - } else if (outcome.remaining.length > 0) { + // The person agreed to a number, so a sweep that lands on a smaller one + // owes them the whole account rather than whichever single fact a branch + // picked. Kept tasks and failures are independent — reporting one and + // dropping the other is how a count quietly stops adding up. + const kept = + outcome.restored.length > 0 ? copy.purgeKeptRestored(outcome.restored.length) : undefined; + if (!outcome.verified || outcome.remaining.length > 0) { // A reason beats a count: a task refuses to retire while its turn is // still running, and "N still there" gives the reader nothing to do. - toast.error( - copy.purgeFailedTitle, - outcome.firstError + const reason = !outcome.verified + ? copy.purgeUnverified + : outcome.firstError ? settingsActionErrorMessage(outcome.firstError, locale) - : copy.purgeFailedBody(outcome.remaining.length), - ); + : copy.purgeFailedBody(outcome.remaining.length); + toast.error(copy.purgeFailedTitle, kept ? `${reason} ${kept}` : reason); } else { - toast.success(copy.purgedToast(outcome.removed)); + toast.success(copy.purgedToast(outcome.removed), kept); } } finally { if (mountedRef.current) setPurging(false); diff --git a/apps/desktop/stories/settings/settings-pages.stories.tsx b/apps/desktop/stories/settings/settings-pages.stories.tsx index b14c9029e2..3b3d3a9d86 100644 --- a/apps/desktop/stories/settings/settings-pages.stories.tsx +++ b/apps/desktop/stories/settings/settings-pages.stories.tsx @@ -863,7 +863,13 @@ function useArchivedTasksStoryBridge(seed: readonly SessionSummary[]): ArchivedT }, onPurge: async (sessionIds) => { drop(sessionIds); - return { removed: sessionIds.length, remaining: [], verified: true, firstError: undefined }; + return { + removed: sessionIds.length, + remaining: [], + restored: [], + verified: true, + firstError: undefined, + }; }, }; }