From df42ab815bac85a62740dd67d2c066304b2b7035 Mon Sep 17 00:00:00 2001 From: AstroHan Date: Sat, 15 Aug 2026 17:21:10 +0800 Subject: [PATCH 1/2] fix(desktop): keep a remove from destroying a restored task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a task is a read of the catalog followed by a remove, and the Host already refuses the remove when the record changed underneath: the retirement coordinator compares the revision inside its admission lock, and the metadata store re-checks the version in the same transaction as the DELETE. Unarchiving writes through that same header, so a concurrent restore reliably rejects the delete. The Desktop Client threw that rejection away. `removeSession` treated a `revision_conflict` as a stale read and replayed the delete at the fresh revision, up to eight times. Replay is right for a rename or a configuration patch, where the write means the same thing either way. It is wrong for a remove: the conflict is the signal that the task was touched after the caller decided to destroy it, so the replay executed a destruction whose premise was gone — and the retry widened the race from the gap between two calls to the whole loop. The bulk purge in 设置 › 活动 › 已归档任务 made it easy to hit, because a serial sweep holds that window open for every task in it. `removeSession` now takes the premise the caller decided on. With `requireArchived`, each fresh read re-asserts that the task is still archived and answers `restored` instead of replaying. That is enough to hold the premise through the commit: the version check and the DELETE share one transaction, so a remove committing at the revision just read is a remove of the record that was read as archived. `restored` is not a failure and is reported apart from one. A sweep does not count it as removed, does not check it back against the catalog, and names it in the toast rather than quietly deleting fewer tasks than the confirm agreed to. Single-row delete carries the premise of the row it was raised from, so deleting an active task from the rail is unaffected. No Runtime Host or protocol change: the compare-and-set that makes this work is already there. Fixes #3050 Generated-by: Claude Code --- .../__tests__/app-shell-session-purge.test.ts | 106 +++++++++++++++++- .../runtime-host-client-operations.test.ts | 51 +++++++++ .../__tests__/runtime-host-client-uds.test.ts | 36 +++++- apps/desktop/src/main/runtime-host-client.ts | 29 ++++- .../runtime-host-session-catalog-ipc-main.ts | 21 +++- apps/desktop/src/preload/bridge-contract.d.ts | 9 +- apps/desktop/src/preload/preload.ts | 5 +- .../renderer/app-shell-session-row-actions.ts | 62 +++++++--- .../renderer/locales/settings-tasks-copy.ts | 8 ++ .../src/renderer/locales/shell-copy.ts | 4 + .../renderer/settings/tasks-settings-page.tsx | 4 + .../settings/settings-pages.stories.tsx | 8 +- 12 files changed, 316 insertions(+), 27 deletions(-) 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..88144540e9 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,8 @@ function installWindow( surviving?: readonly SessionSummary[]; /** Runs after each accepted removal, to model what another client did meanwhile. */ onRemove?: (sessionId: string) => void; + /** Ids the Host answers `restored` for, standing in for a lost archived premise. */ + restoredIds?: readonly string[]; } = {}, ): () => void { const target = globalThis as unknown as { window?: unknown }; @@ -60,10 +66,13 @@ 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}`); + if (options.restoredIds?.includes(id)) return 'restored'; harness.removed.push(id); options.onRemove?.(id); + return 'removed'; }, list: async () => { harness.listCalls += 1; @@ -101,12 +110,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 +146,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']); @@ -170,6 +203,32 @@ describe('purgeSessions', () => { assert.deepEqual(outcome.remaining, []); }); + it('reports a task the Host kept because it was restored under the delete', async () => { + // The renderer's own check cannot see a restore that lands after it and + // before the removal. The Host answers `restored` there, and a sweep that + // counted it as deleted would be claiming a deletion that never happened. + const h = harness(); + const sessions = [summary('first'), summary('rescued')]; + const restore = installWindow(h, { restoredIds: ['rescued'] }); + const activeIdRef = { current: 'rescued' as string | undefined }; + const actions = createActions({ harness: h, sessions, activeIdRef }); + + const outcome = await actions.purgeSessions(['first', 'rescued']).finally(restore); + + assert.deepEqual(h.removed, ['first']); + assert.equal(outcome.removed, 1); + assert.deepEqual(outcome.restored, ['rescued']); + // Neither a failure to explain nor a task to check back against the catalog. + assert.deepEqual(outcome.remaining, []); + assert.equal(outcome.firstError, undefined); + assert.equal(h.listCalls, 0); + // A task that is still there keeps everything the renderer holds for it, + // including being the open one. + assert.deepEqual(h.cleared, ['first']); + assert.deepEqual(h.selections, []); + assert.equal(activeIdRef.current, 'rescued'); + }); + 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 +287,42 @@ 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'), summary('active-row', { isArchived: false })]; + 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(); + const sessions = [summary('rescued')]; + const restore = installWindow(h, { restoredIds: ['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__/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..abad870b48 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,33 @@ 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: unarchiving + * bumps `metadataVersion`, and the Host checks that version in the same + * transaction as the `DELETE`, so a remove that commits at the revision just + * read is a remove of the record that was read as archived. + */ + 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..c337ae81a6 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,28 @@ 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 options `requestsRevisionFamily` has already + * checked the shape of. + */ +function requiresArchivedSession(options: unknown): boolean { + if (options === undefined || options === null) return false; + 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..907e3ab601 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,18 +152,27 @@ 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; } /** @@ -168,11 +190,16 @@ export function createAppShellSessionRowActions(deps: { * which would read as "none of them went". When the catalog cannot be read at * all, `verified` is false and the caller claims nothing. * + * A task restored while the sweep was reaching it is reported apart from both: + * the delete was called off on purpose, so it is neither a removal to count + * nor an error to explain. + * * No confirm and no toast: the caller owns the wording for a sweep, which is * the one thing single-row delete cannot phrase. */ async function purgeSessions(sessionIds: readonly string[]): Promise { const unsettled: string[] = []; + const restored: string[] = []; let firstError: unknown; let removed = 0; for (const sessionId of sessionIds) { @@ -181,11 +208,9 @@ export function createAppShellSessionRowActions(deps: { // 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). + // This is a cheap filter, not the guarantee: a restore landing between + // this check and the removal is caught by `requireArchived` below, which + // holds the premise through the Host's compare-and-set (#3050). if (!sessionsRef.current.some((s) => s.id === sessionId && s.isArchived)) continue; const key = `${sessionId}:delete`; if ( @@ -198,8 +223,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 +235,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 +244,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..1816e7a516 100644 --- a/apps/desktop/src/renderer/locales/settings-tasks-copy.ts +++ b/apps/desktop/src/renderer/locales/settings-tasks-copy.ts @@ -16,6 +16,8 @@ export type SettingsTasksCopy = { purgeConfirmBody: string; purgeConfirmAction: string; purgedToast(count: number): string; + /** A sweep that left tasks alone because they were restored while it ran. */ + purgedWithRestoredToast(removed: number, restored: number): string; purgeFailedTitle: string; purgeFailedBody(count: number): string; purgeUnverified: string; @@ -41,6 +43,8 @@ const SETTINGS_TASKS_COPY_BY_LOCALE = { purgeConfirmBody: '这些任务及其全部消息会被永久删除,无法撤销。', purgeConfirmAction: '永久删除', purgedToast: (count: number) => `已删除 ${count} 条任务`, + purgedWithRestoredToast: (removed: number, restored: number) => + `已删除 ${removed} 条任务,${restored} 条已被恢复,予以保留`, purgeFailedTitle: '删除任务失败', purgeFailedBody: (count: number) => `${count} 条仍在,请重试。`, purgeUnverified: '任务已删除,但无法读取列表确认结果。请重新打开本页查看。', @@ -67,6 +71,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`), + purgedWithRestoredToast: (removed: number, restored: number) => + `${removed === 1 ? 'Deleted 1 task' : `Deleted ${removed} tasks`}; ${ + restored === 1 ? '1 was restored meanwhile and kept' : `${restored} 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..5c1bd207ce 100644 --- a/apps/desktop/src/renderer/settings/tasks-settings-page.tsx +++ b/apps/desktop/src/renderer/settings/tasks-settings-page.tsx @@ -117,6 +117,10 @@ export function TasksSettingsPage(props: ArchivedTasksBridge) { ? settingsActionErrorMessage(outcome.firstError, locale) : copy.purgeFailedBody(outcome.remaining.length), ); + } else if (outcome.restored.length > 0) { + // Naming them beats a count that quietly does not add up: the person + // agreed to a number, and a smaller one went. + toast.success(copy.purgedWithRestoredToast(outcome.removed, outcome.restored.length)); } else { toast.success(copy.purgedToast(outcome.removed)); } 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, + }; }, }; } From f0fa5fef4f7862c1d23ed5a287b304286662cd84 Mon Sep 17 00:00:00 2001 From: AstroHan Date: Sat, 15 Aug 2026 18:01:43 +0800 Subject: [PATCH 2/2] fix(desktop): settle every swept id against the delete, not a snapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep decided an id's fate in two places. It skipped anything its own catalog snapshot no longer showed as archived, and separately reported what the Host answered `restored`. The renderer's list is written synchronously as the sweep runs, so the skip is the path a restore usually takes — and it dropped the id with no outcome at all: not removed, not restored, not remaining. A person who confirmed five and got "deleted two" was told nothing about the other three. The reporting added for that only covered the narrow window where a restore lands after the snapshot read and before the Host commits. Deciding here was never sound. The snapshot is one observer of state the Host owns, a second window can outdate it in either direction, and it cannot tell a restored task from one already deleted elsewhere. So the filter is gone rather than taught to classify: every id goes to the delete, which already distinguishes all four outcomes. Still archived removes; restored answers `restored`; already gone rejects and settles as removed against the catalog, which is what the rejected-id check has always done; anything else is an error with a reason. One path, one outcome, one source of truth. The purge toast stopped choosing which single fact to report. Failures and kept tasks are independent, so a sweep that hit both said only the first and silently dropped the second — and an unverified sweep claimed "the tasks were deleted" over tasks it had deliberately kept. Kept tasks are now a fragment that reads after either outcome. Also corrects the reasoning in `removeSession`'s contract. Re-asserting the premise holds through the commit because the Host serializes `session.remove` and `session.lifecycle.set` through the same per-Session admission queue, not because the client's revision reaches the DELETE transaction — the coordinator refreshes family records inside that lock and commits with those versions. The conclusion was right, the mechanism named was not, and the version check alone would not have carried it. The remove options now refuse a shape they cannot read instead of falling through to "no premise stated", which is the destructive answer, and no longer depend on a sibling validator running first. Refs #3050 Generated-by: Claude Code --- .../__tests__/app-shell-session-purge.test.ts | 101 ++++++++++++------ ...hell-session-row-actions-revisions.test.ts | 6 +- apps/desktop/src/main/runtime-host-client.ts | 11 +- .../runtime-host-session-catalog-ipc-main.ts | 13 ++- .../renderer/app-shell-session-row-actions.ts | 27 ++--- .../renderer/locales/settings-tasks-copy.ts | 19 ++-- .../renderer/settings/tasks-settings-page.tsx | 30 +++--- 7 files changed, 127 insertions(+), 80 deletions(-) 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 88144540e9..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 @@ -54,8 +54,12 @@ function installWindow( surviving?: readonly SessionSummary[]; /** Runs after each accepted removal, to model what another client did meanwhile. */ onRemove?: (sessionId: string) => void; - /** Ids the Host answers `restored` for, standing in for a lost archived premise. */ - restoredIds?: readonly string[]; + /** + * 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 }; @@ -69,7 +73,8 @@ function installWindow( remove: async (id: string, removeOptions?: { requireArchived?: boolean }) => { harness.removeOptions.push([id, removeOptions?.requireArchived === true]); if (options.rejectIds?.includes(id)) throw new Error(`busy:${id}`); - if (options.restoredIds?.includes(id)) return 'restored'; + 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'; @@ -166,69 +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('reports a task the Host kept because it was restored under the delete', async () => { - // The renderer's own check cannot see a restore that lands after it and - // before the removal. The Host answers `restored` there, and a sweep that - // counted it as deleted would be claiming a deletion that never happened. + it('keeps everything the renderer holds for a task the delete left alone', async () => { const h = harness(); - const sessions = [summary('first'), summary('rescued')]; - const restore = installWindow(h, { restoredIds: ['rescued'] }); + const catalog = [summary('first'), restored('rescued')]; + const restore = installWindow(h, { catalog }); const activeIdRef = { current: 'rescued' as string | undefined }; - const actions = createActions({ harness: h, sessions, activeIdRef }); + const actions = createActions({ harness: h, sessions: [...catalog], activeIdRef }); const outcome = await actions.purgeSessions(['first', 'rescued']).finally(restore); - assert.deepEqual(h.removed, ['first']); - assert.equal(outcome.removed, 1); assert.deepEqual(outcome.restored, ['rescued']); - // Neither a failure to explain nor a task to check back against the catalog. - assert.deepEqual(outcome.remaining, []); assert.equal(outcome.firstError, undefined); - assert.equal(h.listCalls, 0); - // A task that is still there keeps everything the renderer holds for it, - // including being the open one. + // 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')]; @@ -293,7 +330,7 @@ describe('deleteSession', () => { // 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'), summary('active-row', { isArchived: false })]; + const sessions = [summary('archived-row'), restored('active-row')]; const restore = installWindow(h); const actions = createActions({ harness: h, sessions, activeIdRef: { current: undefined } }); @@ -311,8 +348,10 @@ describe('deleteSession', () => { 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, { restoredIds: ['rescued'] }); + const restore = installWindow(h, { catalog: [restored('rescued')] }); const activeIdRef = { current: 'rescued' as string | undefined }; const actions = createActions({ harness: h, sessions, activeIdRef }); 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/runtime-host-client.ts b/apps/desktop/src/main/runtime-host-client.ts index abad870b48..5743c99b22 100644 --- a/apps/desktop/src/main/runtime-host-client.ts +++ b/apps/desktop/src/main/runtime-host-client.ts @@ -854,10 +854,13 @@ export class DesktopRuntimeHostClient { * 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: unarchiving - * bumps `metadataVersion`, and the Host checks that version in the same - * transaction as the `DELETE`, so a remove that commits at the revision just - * read is a remove of the record that was read as archived. + * 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, 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 c337ae81a6..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 @@ -202,11 +202,18 @@ export function registerRuntimeHostSessionCatalogIpc( } /** - * Reads the archived premise off options `requestsRevisionFamily` has already - * checked the shape of. + * 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 || options === null) return false; + 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'); 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 907e3ab601..ba51998638 100644 --- a/apps/desktop/src/renderer/app-shell-session-row-actions.ts +++ b/apps/desktop/src/renderer/app-shell-session-row-actions.ts @@ -178,10 +178,16 @@ export function createAppShellSessionRowActions(deps: { /** * 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 @@ -190,10 +196,6 @@ export function createAppShellSessionRowActions(deps: { * which would read as "none of them went". When the catalog cannot be read at * all, `verified` is false and the caller claims nothing. * - * A task restored while the sweep was reaching it is reported apart from both: - * the delete was called off on purpose, so it is neither a removal to count - * nor an error to explain. - * * No confirm and no toast: the caller owns the wording for a sweep, which is * the one thing single-row delete cannot phrase. */ @@ -203,15 +205,6 @@ export function createAppShellSessionRowActions(deps: { 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 is a cheap filter, not the guarantee: a restore landing between - // this check and the removal is caught by `requireArchived` below, which - // holds the premise through the Host's compare-and-set (#3050). - if (!sessionsRef.current.some((s) => s.id === sessionId && s.isArchived)) continue; const key = `${sessionId}:delete`; if ( Array.from(pendingSessionRowActionsRef.current).some((pending) => diff --git a/apps/desktop/src/renderer/locales/settings-tasks-copy.ts b/apps/desktop/src/renderer/locales/settings-tasks-copy.ts index 1816e7a516..8dc786ed73 100644 --- a/apps/desktop/src/renderer/locales/settings-tasks-copy.ts +++ b/apps/desktop/src/renderer/locales/settings-tasks-copy.ts @@ -16,8 +16,12 @@ export type SettingsTasksCopy = { purgeConfirmBody: string; purgeConfirmAction: string; purgedToast(count: number): string; - /** A sweep that left tasks alone because they were restored while it ran. */ - purgedWithRestoredToast(removed: number, restored: 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; @@ -43,8 +47,7 @@ const SETTINGS_TASKS_COPY_BY_LOCALE = { purgeConfirmBody: '这些任务及其全部消息会被永久删除,无法撤销。', purgeConfirmAction: '永久删除', purgedToast: (count: number) => `已删除 ${count} 条任务`, - purgedWithRestoredToast: (removed: number, restored: number) => - `已删除 ${removed} 条任务,${restored} 条已被恢复,予以保留`, + purgeKeptRestored: (count: number) => `另有 ${count} 条在此期间被恢复,已保留。`, purgeFailedTitle: '删除任务失败', purgeFailedBody: (count: number) => `${count} 条仍在,请重试。`, purgeUnverified: '任务已删除,但无法读取列表确认结果。请重新打开本页查看。', @@ -71,10 +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`), - purgedWithRestoredToast: (removed: number, restored: number) => - `${removed === 1 ? 'Deleted 1 task' : `Deleted ${removed} tasks`}; ${ - restored === 1 ? '1 was restored meanwhile and kept' : `${restored} were restored meanwhile and kept` - }`, + 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/settings/tasks-settings-page.tsx b/apps/desktop/src/renderer/settings/tasks-settings-page.tsx index 5c1bd207ce..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,23 +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), - ); - } else if (outcome.restored.length > 0) { - // Naming them beats a count that quietly does not add up: the person - // agreed to a number, and a smaller one went. - toast.success(copy.purgedWithRestoredToast(outcome.removed, outcome.restored.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);