Skip to content

fix(core): harden the usage salvage around session deletion #7425

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 1 commit into from
Jul 22, 2026
Merged

fix(core): harden the usage salvage around session deletion #7425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/core/src/services/sessionService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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', {
Expand Down
26 changes: 24 additions & 2 deletions packages/core/src/services/sessionService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
try {
await persistUsageBeforeTranscriptDeletion(transcriptPath);
} catch (error) {
this.warn(
`usage salvage failed for ${transcriptPath}: ${error}; deleting anyway`,
);
}
}

private async removeSessionFiles(sessionId: string): Promise<boolean> {
if (!SESSION_FILE_PATTERN.test(`${sessionId}.jsonl`)) {
return false;
Expand All @@ -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);
Comment on lines +1373 to 1374

@ZijianZhang989 ZijianZhang989 Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The existing "both copies conflict" test (should remove both JSONL files when active and archived copies conflict) only asserts unlinkSyncSpy calls — it never checks that persistUsageBeforeTranscriptDeletion was called with the archived path. If someone later removes this salvage call (e.g., considering it redundant with the active-path salvage above), the test still passes while usage data from archived transcripts is silently lost — the exact interrupted-archive scenario the comment describes.

Concrete cost: a future cleanup removes salvageUsageBestEffort(archivedPath) → both files still unlinked → test green → usage records silently lost for sessions with co-existing active and archived transcripts.

Consider adding to the "both copies" test:

expect(persistUsageBeforeTranscriptDeletion).toHaveBeenCalledWith(
  expect.stringContaining('/chats/archive/'),
);
expect(persistUsageBeforeTranscriptDeletion).toHaveBeenCalledTimes(2);

— qwen3.7-max via Qwen Code /review

@zjunothing zjunothing Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in follow-up #7604 — the conflict test now pins persistUsageBeforeTranscriptDeletion being called with the archived path, so removing that salvage fails a test. 中文：已在 follow-up #7604 落地——冲突测试钉住以 archived 路径调用 salvage，删除该调用会跑挂测试。

}
this.removeWorktreeSidecars(sessionId);
Expand All @@ -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);
Expand Down
Loading