From 19365b5d602a5f67ffc26b1b31a916c101165977 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=B3=E4=BF=A1?= Date: Tue, 21 Jul 2026 20:07:38 +0800 Subject: [PATCH] fix(core): harden the usage salvage around session deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Post-merge review follow-ups on #7391 (three findings): - Salvage the archived transcript in the active-branch deletion too: when both copies co-exist (an interrupted archive) and the fresh active transcript carries no telemetry, the archived copy holds the session's usage history and was deleted unsalvaged. The dedup guard makes the extra call a no-op whenever the active copy already wrote. - Enforce the "never blocks deletion" contract at the call site: a salvageUsageBestEffort wrapper catches and warns, so the guarantee is structural rather than an implementation detail of persistUsageBeforeTranscriptDeletion. The new failure-tolerance test (salvage rejects -> deletion still succeeds) fails without the wrapper — the bare await let the rejection escape through removeSessionFiles' rethrowing catch. - Clear the salvage module mock in beforeEach so the wiring test's invocationCallOrder assertions can never read stale calls. Co-Authored-By: Claude Fable 5 --- .../core/src/services/sessionService.test.ts | 16 ++++++++++++ packages/core/src/services/sessionService.ts | 26 +++++++++++++++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/core/src/services/sessionService.test.ts b/packages/core/src/services/sessionService.test.ts index 286296b2ceb..be8b4390dc8 100644 --- a/packages/core/src/services/sessionService.test.ts +++ b/packages/core/src/services/sessionService.test.ts @@ -68,6 +68,9 @@ describe('SessionService', () => { }); sessionService = new SessionService('/test/project/root'); + // Module mocks are not reset by restoreAllMocks; clear the salvage spy + // so per-test call/order assertions never read stale invocations. + vi.mocked(persistUsageBeforeTranscriptDeletion).mockClear(); readdirSyncSpy = vi.spyOn(fs, 'readdirSync').mockReturnValue([]); statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation( @@ -1458,6 +1461,19 @@ describe('SessionService', () => { ); }); + it('still deletes the session when the usage salvage fails', async () => { + // Contract: the salvage must never block deletion. + vi.mocked(persistUsageBeforeTranscriptDeletion).mockRejectedValueOnce( + new Error('salvage exploded'), + ); + vi.mocked(jsonl.readLines).mockResolvedValue([recordA1]); + + await expect(sessionService.removeSession(sessionIdA)).resolves.toBe( + true, + ); + expect(unlinkSyncSpy).toHaveBeenCalled(); + }); + it('should clear session organization when removing a session', async () => { const warnings: string[] = []; sessionService = new SessionService('/test/project/root', { diff --git a/packages/core/src/services/sessionService.ts b/packages/core/src/services/sessionService.ts index 4d9f17949b1..ca7a7ff38ea 100644 --- a/packages/core/src/services/sessionService.ts +++ b/packages/core/src/services/sessionService.ts @@ -1333,6 +1333,22 @@ export class SessionService { return removed; } + /** + * Usage salvage wrapper enforcing the "never blocks deletion" contract at + * the call site: persistUsageBeforeTranscriptDeletion catches its own + * errors, but this second layer keeps the guarantee structural rather + * than an implementation detail of another module. + */ + private async salvageUsageBestEffort(transcriptPath: string): Promise { + try { + await persistUsageBeforeTranscriptDeletion(transcriptPath); + } catch (error) { + this.warn( + `usage salvage failed for ${transcriptPath}: ${error}; deleting anyway`, + ); + } + } + private async removeSessionFiles(sessionId: string): Promise { if (!SESSION_FILE_PATTERN.test(`${sessionId}.jsonl`)) { return false; @@ -1345,10 +1361,16 @@ export class SessionService { // #7384: the usage-history rebuild reads transcripts, so salvage // the session's usage summary before the file is gone. Never // blocks deletion (the salvage swallows its own errors). - await persistUsageBeforeTranscriptDeletion(activePath); + await this.salvageUsageBestEffort(activePath); this.removeFileIfExists(activePath); const archivedPath = this.getSessionFilePath(sessionId, 'archived'); if (fs.existsSync(archivedPath)) { + // When both copies co-exist (e.g. an interrupted archive), the + // active transcript may hold no telemetry while the archived one + // carries the session's history — salvage it too. The dedup + // guard inside the salvage makes this a no-op whenever the + // active copy already produced a record. + await this.salvageUsageBestEffort(archivedPath); this.removeFileIfExists(archivedPath); } this.removeWorktreeSidecars(sessionId); @@ -1363,7 +1385,7 @@ export class SessionService { if (!archived) { return false; } - await persistUsageBeforeTranscriptDeletion(archivedPath); + await this.salvageUsageBestEffort(archivedPath); this.removeFileIfExists(archivedPath); this.removeWorktreeSidecars(sessionId); this.removeFileHistoryBackups(sessionId);