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

[zjunothing]

What this PR does

Hardens the usage salvage that #7391 added around session deletion, addressing its three post-merge review findings: (1) the active-branch deletion now salvages the archived transcript too before removing it — when both copies co-exist after an interrupted archive and the fresh active transcript carries no telemetry, the archived copy holds the session's usage history and was previously deleted unsalvaged (the salvage's dedup guard makes the extra call a no-op whenever the active copy already wrote a record); (2) the "never blocks deletion" contract is now enforced at the call site via a salvageUsageBestEffort wrapper that catches and warns — previously the bare await let a salvage rejection escape through removeSessionFiles' rethrowing catch, so the guarantee rested entirely on the salvage's internal error handling; (3) the salvage module mock is cleared in beforeEach so the wiring test's invocationCallOrder assertions can never read stale calls.

Why it's needed

Post-merge review on #7391 flagged all three: the archived-copy edge loses real usage data in the interrupted-archive scenario; the failure-tolerance contract was documented in a comment but untested and, as the new test proves, not actually structural (a rejecting salvage failed the deletion); and the module-scope mock's monotonically increasing invocation order made the ordering assertion fragile to test reordering.

Reviewer Test Plan

How to verify

1. npx vitest run src/services/sessionService.test.ts src/services/usageHistoryService.test.ts (packages/core): 151/151.
2. The new failure-tolerance test (still deletes the session when the usage salvage fails) fails without the call-site wrapper — verified by running it against the bare-await wiring: the mocked rejection propagates and removeSession rejects instead of returning true.
3. Interrupted-archive edge: with both transcripts present and a telemetry-less active copy, deletion now writes the archived copy's summary to usage_record.jsonl; when the active copy already produced a record, the second salvage is a no-op (dedup guard).

Evidence (Before & After)

The failure-tolerance contract is the demonstrable delta: with the wrapper removed, the new test fails with the salvage rejection propagating out of removeSession (Error: salvage exploded), because removeSessionFiles' catch rethrows non-ENOENT errors — deletion was blocked exactly the way the #7391 comment claimed it never would be. With the wrapper, deletion succeeds and the failure is downgraded to a warning. The archived-salvage addition reuses the merged salvage function unchanged; its dedup behavior is already pinned by the #7391 suite (150 tests), which passes untouched alongside the new one.

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows	⚠️
🐧 Linux	⚠️

Environment (optional)

macOS (Darwin 24.6), Node v22.23.1; vitest. npm run typecheck, eslint --max-warnings 0, prettier all clean.

Risk & Scope

• Main risk or tradeoff: deleting a session with both copies present now reads the archived transcript once more; bounded by transcript size, on a path already dominated by file I/O. The wrapper adds a warning line on salvage failure where there was silence.
• Not validated / out of scope: same as fix(core): salvage session usage into the history before deleting transcripts #7391 — external deletions bypassing removeSessionFiles() cannot be salvaged.
• Breaking changes / migration notes: none.

Linked Issues

Follow-up to #7391 (issue #7384).

中文说明

本 PR 做了什么

对 #7391 引入的删除前用量 salvage 做三项加固（其合并后 review 的三条发现）：(1) active 分支删除 archived 副本前也执行 salvage——中断的归档会让两份并存，若新的 active transcript 无遥测，archived 副本才是用量历史所在，此前会被无 salvage 删除（salvage 的查重门保证 active 已写记录时第二次调用为 no-op）；(2) 「永不阻塞删除」契约收到调用点——新增 salvageUsageBestEffort 包装 catch+warn，此前裸 await 会让 salvage 的 rejection 穿透 removeSessionFiles 的重抛 catch，契约完全依赖另一模块的内部实现；(3) beforeEach 清理 salvage 模块 mock，接线测试的 invocationCallOrder 断言不会读到陈旧调用。

为什么需要

三条均来自 #7391 的合并后 review：archived 副本边缘会真实丢数据；失败容忍契约写在注释里但未测试、且如新测试所证并非结构性（rejection 会让删除失败）；模块级 mock 的单调递增调用序使顺序断言对测试重排脆弱。

审阅测试计划

两套件 151/151；新失败容忍测试在无包装的裸 await 接线上失败（rejection 穿透、removeSession 变 reject）——这正是可演示的 before/after 差异；archived salvage 复用已合并函数，其查重行为由 #7391 原有 150 个测试钉住且原样通过。

测试平台

macOS 已本地验证（✅）；Windows / Linux 依赖 CI（⚠️）。

风险与范围

• 双副本并存的删除多一次 archived transcript 读取（按大小有界）；salvage 失败从静默变为一条 warning。
• 与 fix(core): salvage session usage into the history before deleting transcripts #7391 相同：绕过 removeSessionFiles() 的外部删除无法挽救。
• 无破坏性变更。

关联 Issue

Follow-up to #7391（issue #7384）。

🤖 Generated with Claude Code

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓

Problem: observed bug with evidence. This is a direct follow-up to #7391's post-merge review — the new test (still deletes the session when the usage salvage fails) proves the contract violation: a rejecting salvage propagates through removeSessionFiles' rethrowing catch and blocks deletion. The archived-copy edge (interrupted archive leaves both transcripts, archived one deleted unsalvaged) is a concrete data-loss scenario.

Direction: aligned. Hardening the "never blocks deletion" contract at the call site is the right move — it makes the guarantee structural rather than an implementation detail of usageHistoryService. The archived-salvage addition closes a real gap.

Size: 2 files, +40/−2. Production: ~24 lines (wrapper method + 3 call-site changes + comment). Test: ~16 lines (mockClear + new test). Well under any threshold.

Approach: scope is tight — three focused fixes, all directly from the #7391 review findings, no unrelated changes. The salvageUsageBestEffort wrapper is the minimal structural fix.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓

问题：已观测到的 bug，有证据。这是 #7391 合并后 review 的直接跟进——新测试（still deletes the session when the usage salvage fails）证明了契约违反：salvage 的 rejection 会穿透 removeSessionFiles 的重抛 catch 并阻塞删除。archived 副本边缘（中断的归档让两份并存，archived 被无 salvage 删除）是真实的数据丢失场景。

方向：对齐。在调用点加固「永不阻塞删除」契约是正确做法——让保证成为结构性的，而非 usageHistoryService 的实现细节。archived salvage 填补了真实缺口。

规模：2 个文件，+40/−2。生产代码约 24 行（包装方法 + 3 个调用点变更 + 注释）。测试约 16 行（mockClear + 新测试）。远低于任何阈值。

方案：范围紧凑——三个聚焦修复，全部来自 #7391 的 review 发现，无无关改动。salvageUsageBestEffort 包装是最小的结构性修复。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

_{Reviewed at 19365b5d602a5f67ffc26b1b31a916c101165977 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code Review

Independent proposal: given the #7391 review findings, I would (1) wrap the salvage call in a try/catch at the call site to make the "never blocks deletion" contract structural, (2) add salvage for the archived path when both copies co-exist, and (3) clear the module mock in beforeEach to prevent stale invocation-order assertions. Exactly what this PR does.

Comparison with the diff: the PR matches my independent proposal precisely. No correctness bugs, no security issues, no regressions. Specific observations:

• salvageUsageBestEffort is a clean, minimal wrapper — catches, warns via the existing this.warn() pattern, and moves on. The JSDoc explains why the second layer exists (structural guarantee vs. implementation detail). Good.
• The archived-salvage addition correctly relies on the dedup guard inside persistUsageBeforeTranscriptDeletion (sessionId check) to make the second call a no-op when the active copy already wrote a record. No double-write risk.
• The mockClear() in beforeEach is standard vitest hygiene for module-level mocks that restoreAllMocks doesn't reset.
• The new test is well-constructed: mocks the salvage to reject, asserts deletion still succeeds and files are unlinked. This is the exact contract being hardened.
• All three call sites (activePath, archivedPath in the active branch, archivedPath in the archived-only branch) are consistently updated.

No issues found. The change is minimal, focused, and follows existing patterns.

Testing

This is a non-user-visible internal change (error handling in session deletion). Verification via unit tests and typecheck:

$ cd packages/core && npx vitest run src/services/sessionService.test.ts

 ✓ src/services/sessionService.test.ts (121 tests) 172ms

 Test Files  1 passed (1)
      Tests  121 passed (121)
   Duration  3.83s

$ npx vitest run src/services/usageHistoryService.test.ts

 ✓ src/services/usageHistoryService.test.ts (30 tests) 62ms

 Test Files  1 passed (1)
      Tests  30 passed (30)
   Duration  3.41s

$ npx tsc --noEmit -p packages/core/tsconfig.json
(clean — no errors)

CLI smoke test (tmux, npm run dev -- -p 'say hello'):

> @qwen-code/qwen-code@0.20.0 dev
> node scripts/dev.js -p say hello

Hello! How can I help you today?

151/151 tests pass, typecheck clean, CLI starts and responds normally.

中文说明

代码审查

独立方案： 根据 #7391 的 review 发现，我会 (1) 在调用点用 try/catch 包装 salvage 调用，使「永不阻塞删除」契约成为结构性的；(2) 在两份副本并存时为 archived 路径添加 salvage；(3) 在 beforeEach 中清理模块 mock。与本 PR 完全一致。

与 diff 对比： PR 与我的独立方案精确匹配。无正确性 bug、无安全问题、无回归。

• salvageUsageBestEffort 是干净的最小包装——catch、通过现有 this.warn() 模式告警、继续执行。
• archived salvage 正确依赖 persistUsageBeforeTranscriptDeletion 内部的查重门（sessionId 检查），active 已写记录时第二次调用为 no-op。
• beforeEach 中的 mockClear() 是模块级 mock 的标准 vitest 卫生做法。
• 新测试构造良好：mock salvage 为 reject，断言删除仍成功且文件被 unlink。
• 三个调用点全部一致更新。

无问题。变更最小、聚焦、遵循现有模式。

测试

非用户可见的内部变更（session 删除的错误处理）。通过单元测试和类型检查验证：151/151 测试通过，类型检查干净，CLI 正常启动和响应。

— Qwen Code · qwen3.7-max

_{Reviewed at 19365b5d602a5f67ffc26b1b31a916c101165977 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 5/5 — clean across every stage; would merge without hesitation.

This is exactly what a follow-up hardening PR should look like: three small, focused fixes from a post-merge review, each with a clear rationale and a test that proves the contract. The salvageUsageBestEffort wrapper makes the "never blocks deletion" guarantee structural at the call site instead of relying on another module's internal catch — that's the right architectural instinct. The archived-salvage addition closes a real data-loss edge case with zero risk of double-writes (dedup guard). No scope creep, no unrelated changes, no over-engineering.

The new test is the strongest evidence: remove the wrapper and it fails with the salvage rejection propagating out of removeSession — exactly the contract violation the PR describes. 151/151 tests pass, typecheck is clean, CLI smoke test is normal.

If I had to maintain this in six months, I'd thank the author — the code is self-documenting, the comments explain why not what, and the test pins the contract.

中文说明

置信度：5/5 — 每个阶段都干净；毫不犹豫地合并。

这正是跟进加固 PR 应有的样子：三个来自合并后 review 的小而聚焦的修复，每个都有清晰的理由和证明契约的测试。salvageUsageBestEffort 包装让「永不阻塞删除」的保证在调用点成为结构性的，而非依赖另一个模块的内部 catch——这是正确的架构直觉。archived salvage 填补了真实的数据丢失边缘场景，且无双写风险（查重门）。无范围蔓延、无无关改动、无过度工程。

新测试是最强证据：移除包装后测试失败，salvage 的 rejection 穿透 removeSession——正是 PR 描述的契约违反。151/151 测试通过，类型检查干净，CLI 冒烟测试正常。

— Qwen Code · qwen3.7-max

_{Reviewed at 19365b5d602a5f67ffc26b1b31a916c101165977 · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[wenshao]

Local verification report ✅

I built and ran this PR locally as a merge reference. Beyond re-running the author's suites, I wrote an independent real-filesystem integration test (un-mocked usageHistoryService + SessionService against a temp QWEN_HOME) and used two negative controls to prove each production change actually discriminates the bug it claims to fix.

Environment: worktree off PR HEAD 19365b5d6, Linux, Node v22.22.2, vitest 3.2.4.

1. Author's suites + quality gates — all green

• sessionService.test.ts (121) + usageHistoryService.test.ts (30) → 151/151, exactly as the PR states.
• eslint --max-warnings 0 → exit 0 · prettier --check → clean · tsc --noEmit → 0 errors on the changed files.

2. Negative control — the "never blocks deletion" contract (change #1)

I reverted only the production file to its pre-PR state (957a93fc9, bare await, no salvageUsageBestEffort) and kept the new test. It fails — promise rejected "Error: salvage exploded" instead of resolving — confirming that before this PR a rejecting salvage really did escape removeSessionFiles' rethrowing catch and block deletion. Restore the wrapper → it passes. So the guarantee is now structural at the call site, not an implementation detail of another module.

3. Negative control — archived-copy salvage recovers otherwise-lost usage (change #2)

My integration test plants the interrupted-archive scenario on a real FS: a telemetry-less active copy and an archived copy carrying 1000 tokens, then calls removeSession().

• Before (active branch salvages the active copy only): usage_record.jsonl ends up empty — expected [] to have a length of 1 but got 0. The archived session's usage is lost forever. This reproduces the exact data-loss the PR targets.
• After (PR HEAD): the archived copy's usage lands in usage_record.jsonl (models["qwen-max"].totalTokens === 1000), and a second test confirms the dedup guard makes the extra salvage a no-op when the active copy already produced a record — no duplicate, so /stats permanently double-counts a session if /stats is opened during the first-ever turn (introduced by #4779) #4994 stays closed.

(The integration test is a scratch file I wrote to verify the fix — not proposed for this PR. Change #3, the beforeEach mockClear, is covered by the existing invocationCallOrder wiring test, which stays green.)

Verdict

LGTM — the three post-merge findings from #7391 are each addressed and independently proven. No behavioral regressions, quality gates clean. 👍

中文说明

本地验证报告 ✅

作为合并参考，我在本地构建并运行了本 PR。除了复跑作者的两个测试套件，我还额外写了一个独立的真实文件系统集成测试（不 mock usageHistoryService，用真实 SessionService + 临时 QWEN_HOME），并用**两组反向对照（negative control）**证明每一处生产代码改动确实能区分它所声称修复的 bug。

环境： 基于 PR HEAD 19365b5d6 的 worktree，Linux，Node v22.22.2，vitest 3.2.4。

1. 作者套件 + 质量门 —— 全绿

• sessionService.test.ts（121）+ usageHistoryService.test.ts（30）→ 151/151，与 PR 描述完全一致。
• eslint --max-warnings 0 → exit 0 · prettier --check → clean · tsc --noEmit → 改动文件0 error。

2. 反向对照 —— 「永不阻塞删除」契约（改动 #1）

我只把生产文件回退到 PR 前状态（957a93fc9，裸 await，无 salvageUsageBestEffort），保留新测试。它失败——promise rejected "Error: salvage exploded" instead of resolving——证明本 PR 之前，salvage 的 rejection 确实会穿透 removeSessionFiles 的重抛 catch 并阻塞删除。恢复包装后即通过。所以该保证现在是调用点上的结构性保证，而非依赖另一模块的内部实现。

3. 反向对照 —— archived 副本 salvage 找回本会丢失的用量（改动 #2）

集成测试在真实 FS 上还原「中断归档」场景：active 副本无遥测 + archived 副本携带 1000 tokens，随后调用 removeSession()。

• 修复前（active 分支只 salvage active 副本）：usage_record.jsonl 为空——expected [] to have a length of 1 but got 0，archived 会话的用量永久丢失，精确复现了本 PR 针对的数据丢失。
• 修复后（PR HEAD）：archived 副本用量写入 usage_record.jsonl（models["qwen-max"].totalTokens === 1000）；另一条测试确认查重门在 active 已写记录时让第二次 salvage 成为 no-op——不产生重复，/stats permanently double-counts a session if /stats is opened during the first-ever turn (introduced by #4779) #4994 依旧闭合。

（该集成测试是我为验证而写的临时文件，并非提交进本 PR。改动 #3，即 beforeEach 里的 mockClear，由现有 invocationCallOrder 接线测试覆盖，保持绿色。）

结论

LGTM —— #7391 合并后 review 的三条发现均已逐一处理并独立验证。无行为回归，质量门全绿。👍

---

🤖 Generated with Claude Code — Claude Opus 4.8 (1M context)

[gwinthis]

Architecture Review — PR #7425

Verdict: ✅ Approve — makes a documented contract structural.

Key design pattern: Defense in depth for contracts

The salvage module already catches its own errors. But the "never blocks deletion" contract was an implementation detail of another module — a rejecting salvage could escape through removeSessionFiles' rethrowing catch.

The fix wraps the call in a call-site try/catch (salvageUsageBestEffort), making the contract structural rather than relying on the callee's internal error handling.

Principle: If module A promises "I never throw", module B should still wrap the call. The contract should be enforced by the caller, not be an implementation detail of the callee.

Archived transcript edge case

When both active and archived copies co-exist (interrupted archive), the active transcript may hold no telemetry while the archived one carries the session's history. The dedup guard inside the salvage makes the extra call a no-op when the active copy already produced a record.

[ZijianZhang989]

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

[ZijianZhang989]

[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]

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，删除该调用会跑挂测试。