fix(desktop): stop pruning session mirrors a listing never reported - #8420
fix(desktop): stop pruning session mirrors a listing never reported#8420harjothkhara wants to merge 6 commits into
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Test Plan (not a blocker): packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。 Test Plan（非阻断）：packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| if (!managed.sdkSessionId || managed.id !== managed.sdkSessionId) | ||
| continue | ||
| if (seenSdkSessionIds.has(managed.sdkSessionId)) continue | ||
| if (!everListed.has(managed.sdkSessionId)) continue |
There was a problem hiding this comment.
[Suggestion] The prune decision now depends on per-process in-memory state (everListed) that resets on restart, yet the deletion path emits no log — pruned mirrors are unauditable. The only caller-side log is Synced N provider session(s) (it never names pruned ids, and is skipped entirely for an empty listing), while the user-initiated RPC path logs Deleted session <id> (SessionManager.ts:8129). Pre-change the prune rule was stateless ("absent from the current listing"); this change makes it process-history-dependent, and the deciding state dies with the process. Failure scenario: a "my session vanished from the sidebar" support report arrives after a restart → nothing records which refresh pruned which id or whether the ratchet allowed or blocked it, so the exact bug class this PR addresses becomes unreproducible.
// inside the prune loop, next to the delete:
sessionLog.info(
`Pruned external session mirror ${managed.id} (workspace ${workspace.id}, ${connectionSlug}): absent from completed provider listing`,
)中文说明
清理（prune）决策现在依赖于进程内的内存状态（everListed），该状态在重启后会重置，但删除路径没有输出任何日志——被清理的镜像无法审计。调用方唯一的日志是 Synced N provider session(s)（它从不记录被清理的 id，且在空列表时完全跳过），而用户主动发起的 RPC 路径会记录 Deleted session <id>（SessionManager.ts:8129）。改动之前，清理规则是无状态的（"不在当前列表中"）；本次改动使其依赖于进程历史，而做出决策所依据的状态会随进程退出而消失。失败场景：重启后收到"我的会话从侧边栏消失了"的支持工单 → 没有任何日志记录是哪次刷新清理了哪个 id、棘轮是放行还是拦截了它，于是本 PR 所要修复的这类 bug 变得无法复现。建议在清理循环中、删除操作旁增加一行日志（见上方代码块）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| // Prune only mirrors this workspace has positively seen listed before. | ||
| // The provider's listing is scoped to one working directory, so a session | ||
| // started in a different project folder is simply absent from it — and |
There was a problem hiding this comment.
[Suggestion] The fix is right for the hazard it closes, but the description attributes #8400's observed data loss to this prune path, while the issue's own logs show the deletions went through the renderer auto-delete → deleteSession RPC path, which this PR does not touch. The Deleted session <id> log format exists at exactly one site in server-core — the deleteSession RPC (SessionManager.ts:8129) — and the prune path deletes with no per-session log (as this PR's own description notes); the staggered timestamps in the issue (5s, 63s apart) corroborate per-session RPC deletes via the NavigationContext.tsx auto-delete predicate (meta && !meta.lastFinalMessageId && !meta.name && !meta.isProcessing) → App.tsx handleAutoDeleteEmptySession, not one synchronous prune pass. Failure scenario: after this merge, #8400's repro can still silently delete sessions (stripped name + the wrong-cwd load failure still match the renderer predicate on navigate-away) → the merge may nonetheless be read as resolving the issue. Suggested action (no code change): restate the motivation — the #8400 incident executed via the renderer/deleteSession RPC path; this PR closes the separate, maintainer-confirmed list-sync prune hazard of the same cwd-mismatch family — and ensure follow-up tracking for the deferred paths disclosed in Risk & Scope so #8400 stays open until they land.
中文说明
就本 PR 所关闭的危害而言，修复本身是正确的；但描述把 #8400 中观察到的数据丢失归因于这条清理（prune）路径，而该 issue 自身的日志显示删除是通过渲染进程自动删除 → deleteSession RPC 路径执行的，本 PR 并未触及该路径。Deleted session <id> 这一日志格式在 server-core 中只存在于一处——deleteSession RPC（SessionManager.ts:8129）——而清理路径删除时不输出任何逐会话日志（本 PR 的描述自己也承认了这一点）；issue 中错开的时间戳（相隔 5 秒、63 秒）印证了这是经由 NavigationContext.tsx 的自动删除判定（meta && !meta.lastFinalMessageId && !meta.name && !meta.isProcessing）→ App.tsx handleAutoDeleteEmptySession 发起的逐会话 RPC 删除，而不是一次同步的批量清理。失败场景：本 PR 合并后，#8400 的复现步骤仍可能静默删除会话（被剥离的 name + 错误的 cwd 导致加载失败，仍会在离开页面时命中渲染进程的删除判定）→ 但本次合并仍可能被视为已解决该 issue。建议的处理（无需改代码）：改写动机描述——#8400 的实际事故经由渲染进程 / deleteSession RPC 路径执行；本 PR 关闭的是同一 cwd 不匹配家族中另一个由维护者确认的列表同步清理危害——并为 Risk & Scope 中披露的延后路径建立后续跟踪，使 #8400 在这些路径落地前保持打开。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| expect(managed.lastMessageAt).toBe(timestamp); | ||
| }); | ||
|
|
||
| it('keeps a restored mirror the cwd-scoped session list never returned', async () => { |
There was a problem hiding this comment.
[Suggestion] [test] These regression tests are not collected by any test job in this repository: the root npm workspaces exclude packages/desktop, and no workflow at the base branch installs bun or runs tests under packages/desktop (verified against the base tree). Failure scenario: a future change that reintroduces unconditional mirror pruning — or breaks this ratchet — merges with this repo's CI fully green → the regression only surfaces when users lose session mirrors after a restart, the exact bug this PR fixes. The tests were verified to gate when actually run (bun 34/34 green here; reverting only the SessionManager.ts hunks makes keeps a restored mirror… fail), so the gap is collection, not test quality — and the author's Test Plan already discloses that CI does not cover this. Suggested fix: give these tests a gate in this repo (e.g. a job that sets up bun and runs bun test packages/server-core/src/sessions/ inside packages/desktop), or explicitly confirm and rely on the openwork-side sync CI.
中文说明
[test] 这些回归测试不会被本仓库中的任何测试任务收集：根 npm workspaces 显式排除了 packages/desktop，且基线分支上没有任何 workflow 安装 bun 或在 packages/desktop 下运行测试（已在基线树上核实）。失败场景：未来某个重新引入无条件镜像清理——或破坏该棘轮——的改动，可以在本仓库 CI 全绿的情况下合并 → 回归只会在用户重启后丢失会话镜像时才会显现，而这正是本 PR 修复的 bug。这些测试在实际运行时已被验证确实具有把关作用（此处 bun 34/34 全绿；仅回退 SessionManager.ts 的改动块会使 keeps a restored mirror… 失败），因此缺口在于"收集"而非测试质量——作者的 Test Plan 也已披露 CI 不覆盖此处。建议：在本仓库为这些测试设立关卡（例如一个安装 bun 并在 packages/desktop 内运行 bun test packages/server-core/src/sessions/ 的 job），或明确确认并依赖 openwork 侧的同步 CI。
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline. Test Plan (not a blocker): packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory.
中文说明
已审查——无阻断问题。 建议见行内评论。 Test Plan（非阻断）：packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| workspace: Workspace, | ||
| connectionSlug: string, | ||
| ): Set<string> { | ||
| const key = `${workspace.id}::${connectionSlug}` |
There was a problem hiding this comment.
[Suggestion] The ratchet key omits the listing's cwd scope — "ever listed" evidence recorded under one working directory stays valid after the workspace's workingDirectory is changed, so a listing scoped to the new cwd (which cannot contain the old-cwd sessions) still prunes mirrors that are alive on the provider. Reproduced by runtime probe: after re-pointing defaults.workingDirectory from A to B, a session positively listed under A was deleted from memory and disk by the next refresh even though it still exists. This matches pre-change behavior on this path (the diff strictly narrows the pruned set, so nothing is newly destroyed) — an incomplete-fix note rather than a blocker. — Failure scenario: workspace W lists session S under cwd A → everListed[W::slug] += S; the user re-points workingDirectory to B via settings (persisted; config is re-read on every refresh); the next refresh lists cwd B, where S is absent but present in everListed → the guard passes and deleteStoredSession destroys S's mirror. Fix shape (spans the caller and the getter; probed: the mirror then survives the scope change and all 34 tests in the suite still pass):
// thread sessionListCwd down from doRefreshExternalSessionsForWorkspace
const key = `${workspace.id}::${connectionSlug}::${sessionListCwd}`中文说明
棘轮（ratchet）键缺少本次列表的 cwd 作用域——在某个 working directory 下记录的"曾被列出"证据，在 workspace 的 workingDirectory 被修改后仍然有效，因此按新 cwd 划定的列表（不可能包含旧 cwd 的会话）仍会清理在 provider 侧实际存在的镜像。已用运行时探针复现：将 defaults.workingDirectory 从 A 改为 B 后，下一次刷新就把确实在 A 下列出过的会话从内存和磁盘一并删除，而该会话仍然存在。此路径与改动前行为一致（本 diff 严格收窄清理集合，不会新增任何误删）——故为修复未完全覆盖的补充说明，而非阻断项。— 失败场景：workspace W 在 cwd A 下列出会话 S → everListed[W::slug] += S；用户通过设置将 workingDirectory 改为 B（已持久化，且每次刷新都会重新读取配置）；下一次刷新按 cwd B 列表，S 不在其中但在 everListed 里 → 守卫放行，deleteStoredSession 销毁 S 的镜像。修复形态（跨调用方与 getter；已探针验证：修复后镜像在作用域变更时存活，套件全部 34 个测试仍通过）：见上方代码块。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| // Provider session ids this process has seen in a completed external listing, | ||
| // keyed by `${workspace.id}::${connectionSlug}`. Gates mirror pruning so an | ||
| // unlisted session is only removed once we have positively seen it listed. | ||
| private everListedExternalSessionIds: Map<string, Set<string>> = new Map() |
There was a problem hiding this comment.
[Suggestion] The ratchet is volatile process state guarding mirrors that persist across restarts — a session deleted externally (CLI / filesystem) while the app is closed never appears in a listing again, never enters everListed, and is therefore exempt from pruning in every future process lifetime. Confirmed by runtime probe: a fresh SessionManager over the same storage keeps the dead mirror forever. The trade-off is disclosed in the description, and we verified the stated mitigation is real: restored stripped mirrors are filtered from getSessions via isUnresolvedQwenCanonicalMirror, so the rows are invisible in normal listings. Remaining cost: indefinite storage retention with no user-visible way to clean it up. — Failure scenario: session S is listed while the app runs; the user quits, deletes S in the CLI, and relaunches — the mirror is restored from disk, the fresh-process ratchet is empty and S can never re-enter it, so every refresh skips S at the guard and the dead row persists indefinitely. Possible fix: persist the positive evidence — e.g. write a listedOnce/lastListedAt marker onto the stored mirror on first successful upsert, and prune only marked mirrors.
中文说明
棘轮是进程内的易失状态，守护的却是跨重启持久化的镜像——应用关闭期间被外部（CLI / 文件系统）删除的会话不会再出现在任何列表中，也就永远不会进入 everListed，因此在此后每一个进程生命周期里都被豁免清理。已用运行时探针确认：对同一存储新建 SessionManager 后，失效镜像会永久保留。该取舍在 PR 描述中已披露，且我们核实了所述缓解措施真实存在：恢复出来的、字段被剥离的镜像会经 isUnresolvedQwenCanonicalMirror 被 getSessions 过滤，普通列表里不可见。剩余代价：存储上的无限期保留，且用户没有可见的手段去清理。— 失败场景：会话 S 在应用运行时被列出；用户退出应用、在 CLI 中删除 S、再重新启动——镜像从磁盘恢复，新进程的棘轮为空且 S 永远无法再进入，于是每次刷新都在守卫处跳过 S，失效条目无限期保留。可行修复：持久化"正向证据"——例如在首次成功 upsert 时给存储的镜像写入 listedOnce/lastListedAt 标记，只清理带标记的镜像。
— qwen3.8-max via Qwen Code /review (v0.21.4)
| workspace: Workspace, | ||
| connectionSlug: string, | ||
| ): Set<string> { | ||
| const key = `${workspace.id}::${connectionSlug}` |
There was a problem hiding this comment.
[Suggestion] [test] The workspace.id component of this key is pinned by no test — both new tests use a single workspace and a single slug, so a regression narrowing the key to connectionSlug alone survives the suite. Probe-verified: degrading the key to ${connectionSlug} makes workspace W2 prune a restored mirror on workspace W1's listing evidence while every PR test stays green. The trigger is concrete: the built-in qwen-code slug is shared by all workspaces and refreshExternalSessions() refreshes them all in one process; nested or overlapping working directories can put one provider session inside two workspaces' listing scopes. — Failure scenario: W1's completed refresh lists provider session X and records it; X is absent from W2's cwd-scoped listing; under a key without workspace.id, X sits in the shared set and W2 prunes its mirror — the exact data loss the ratchet exists to prevent. Suggested test: a two-workspace variant of the first test — W1's listSessions returns an id that W2 holds as a restored mirror but W2's listing never returns; refresh both workspaces and assert W2's mirror survives in memory and on disk.
中文说明
[test] 该键中的 workspace.id 分量没有任何测试钉住——两个新测试都只用单个 workspace 和单个 slug，因此把键收窄为仅 connectionSlug 的回归仍能通过整个套件。已用探针验证：把键降级为 ${connectionSlug} 后，workspace W2 会基于 W1 的列表证据清理自己恢复出来的镜像，而 PR 的所有测试依然全绿。触发条件是具体的：内置的 qwen-code slug 为所有 workspace 共享，且 refreshExternalSessions() 会在同一进程内刷新全部 workspace；嵌套或重叠的工作目录完全可能让同一个 provider 会话同时落在两个 workspace 的列表作用域内。— 失败场景：W1 的一次完整刷新列出了 provider 会话 X 并将其记录；X 不在 W2 按 cwd 划定的列表中；若键中没有 workspace.id，X 就会存在于共享集合里，W2 随即清理它的镜像——正是棘轮本要防止的数据丢失。建议补充测试：第一个测试的双 workspace 变体——W1 的 listSessions 返回一个 id，W2 持有其恢复镜像但 W2 自己的列表从不返回它；刷新两个 workspace，断言 W2 的镜像在内存和磁盘上均存活。
— qwen3.8-max via Qwen Code /review (v0.21.4)
…tchet
The two new pruning tests use a single workspace, so the `workspace.id`
component of the everListed key was unpinned — degrading the key to
`${connectionSlug}` alone left the whole suite green. The built-in qwen-code
slug is shared across workspaces refreshed in one process, so that degraded
key lets one workspace's listing prune another workspace's mirror of the same
provider session: the data loss the ratchet exists to prevent.
Add a two-workspace test — A lists the session (recording it under A's key), B
holds it as a restored mirror its own listing never returns; refreshing B must
not prune it. Fails against the workspace.id-dropped key, passes as is.
Reported by @qwen-code /review on QwenLM#8420 (R2-3).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — I verified all three against the source rather than taking the probes on trust. R2-3 (test gap) — closed in R2-1 (cwd not in the key) — real, holding as a follow-up. Confirmed the key omits the listing cwd and that a R2-2 (volatile ratchet) — real, holding as a follow-up. Confirmed R2-1 and R2-2 both point at the same thing — the in-memory ratchet — and your "persist the evidence on the mirror" suggestion would resolve both properly. That's a larger change than this PR's scope, so I'd rather land the workspace-scope test now and do the persisted-evidence version as a focused follow-up. Happy to fold it in here instead if you'd prefer it in one change. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.3)
The provider session listing is scoped to one working directory. A session started in a different project folder is therefore absent from the workspace's listing, and `removeMissingExternalListedSessions` treated that absence as proof the session was gone: it deleted the in-memory session and the on-disk mirror outright, with no content check and no per-session log. After a restart every such mirror is unlisted, so the first refresh silently destroyed all of them. The user's sessions disappear from the app with no confirmation and no way back from the UI. Only prune a mirror this process has positively seen in a completed listing. Absence of evidence is no longer treated as evidence of deletion, while a session that is listed and later disappears — deleted elsewhere, the case the prune exists for — is still cleaned up on the next refresh. Refs: QwenLM#8400 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tchet
The two new pruning tests use a single workspace, so the `workspace.id`
component of the everListed key was unpinned — degrading the key to
`${connectionSlug}` alone left the whole suite green. The built-in qwen-code
slug is shared across workspaces refreshed in one process, so that degraded
key lets one workspace's listing prune another workspace's mirror of the same
provider session: the data loss the ratchet exists to prevent.
Add a two-workspace test — A lists the session (recording it under A's key), B
holds it as a restored mirror its own listing never returns; refreshing B must
not prune it. Fails against the workspace.id-dropped key, passes as is.
Reported by @qwen-code /review on QwenLM#8420 (R2-3).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
05cad9e to
e2ca40e
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
@doudouOUC would you be able to take a look when you have a moment? Fixes #8400 ( @doudouOUC 方便的时候能否帮忙看一下?修复 #8400( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory.
中文说明
未发现问题。LGTM！✅
Test Plan（非阻断）：packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
Gentle ping @doudouOUC — CI is green and the branch merges cleanly against current main. I've also added the |
|
@yiliang114 tagging you as the desktop maintainer. This fixes #8400, a P1 session data loss open since Aug 3. doudouOUC confirmed the root cause on the issue that day. Two files in desktop server-core: +34 in SessionManager.ts and a regression test. CI green, bot review resolved. CODEOWNERS has no /packages/desktop/ entry, so no reviewer was auto-requested here. |
|
@yiliang114 This has been bot-approved on e2ca40e since Aug 3 and all 11 checks are green. No human reviewer is assigned to it, which looks like the desktop CODEOWNERS gap. It has been open 18 days and is blocked only on a human approval. Could you take a look, or point me at the right reviewer for packages/desktop? |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review of PR #8420
Code: Correct and well-structured. The everListedExternalSessionIds ratchet is a clean fix — it prevents the first refresh after a restart from pruning restored mirrors whose sessions are simply absent from the cwd-scoped listing, while still allowing genuine deletions (a session that was listed and later disappears) to be cleaned up on subsequent refreshes. The key composition (${workspace.id}::${connectionSlug}) correctly scopes the evidence per workspace, preventing cross-workspace contamination.
Tests: Three well-structured regression tests cover the guard in both directions (unlisted session survives, previously-listed session is pruned) and the workspace-isolation property. Cleanup via afterEach is properly implemented.
No Criticals. Two Suggestions:
Suggestion 1: "Fixes #8400" should be "Refs #8400"
The PR description ends with Fixes #8400, which would auto-close the issue on merge. Issue #8400 describes two compounding problems: (1) wrong cwd on reload — the desktop uses the workspace default cwd instead of the session's recorded cwd for session/load, and (2) destructive auto-delete — the renderer deletes any session with no loaded messages. The issue's own logs show deletions went through the deleteSession RPC path (renderer auto-delete), not the prune path this PR changes. The PR author acknowledges this in the Risk & Scope section: "The #8400 incident executed via the renderer/deleteSession RPC path; this PR closes the separate, maintainer-confirmed list-sync prune hazard."
Suggested action: change Fixes #8400 to Refs #8400 so the issue stays open until the main data-loss paths are addressed.
Suggestion 2: Missing audit log for pruned mirrors
This was already raised in a previous bot review and is a valid concern. The prune decision now depends on per-process in-memory state (everListed) that resets on restart, yet the deletion path emits no per-session log. A support report of "my session vanished from the sidebar" arriving after a restart would be unreproducible — nothing records which refresh pruned which id or whether the ratchet allowed or blocked it. A log line inside the prune loop would close this gap.
CI gap: These tests are not collected by any CI workflow (a pre-existing gap, not introduced by this PR). The author has disclosed this transparently and verified the tests locally.
中文说明
PR #8420 审查
代码： 正确且结构清晰。everListedExternalSessionIds 棘轮机制是一个简洁的修复——它防止重启后的第一次刷新清理那些只是不在当前 cwd 作用域列表中的已恢复镜像，同时仍允许真正的删除操作（曾被列出但后来消失的会话会在后续刷新中被清理）。键的构成（${workspace.id}::${connectionSlug}）正确地将证据按 workspace 隔离，防止跨 workspace 污染。
测试： 三个结构良好的回归测试覆盖了守卫的两个方向（未被列出的会话存活、曾被列出的会话被清理）以及 workspace 隔离属性。通过 afterEach 进行的清理正确实现。
无阻断项。两点建议：
建议1："Fixes #8400" 应改为 "Refs #8400"
PR 描述末尾的 Fixes #8400 会在合并时自动关闭该 issue。Issue #8400 描述了两个叠加的问题：(1) 重启时 cwd 错误——桌面端使用 workspace 默认的 cwd 而非会话记录的 cwd 去调用 session/load；(2) 渲染进程的破坏性自动删除。issue 的日志显示删除是通过 deleteSession RPC 执行的，而非本 PR 修改的清理路径。建议将 Fixes #8400 改为 Refs #8400，以便该 issue 保持打开状态。
建议2：清理路径缺少审计日志
此问题在之前的 bot 审查中已被提出。清理决策现在依赖于进程内的易失状态（everListed），但删除路径没有输出逐会话日志。建议在清理循环中添加日志。
— via Qwen Code /review
Review follow-up from @doudouOUC. The ratchet that permits a prune lives in memory and resets on restart, so a "my session vanished from the sidebar" report arriving after a restart had nothing to trace: no record of which refresh removed which id, or whether the guard allowed or blocked it. The deletion path now logs before it deletes, through the same sessionLog the rest of the file uses. Refs: QwenLM#8400 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R5-2 regression tests unreachable by any repo test command or CI job — already reported (comment 3701709157)
Test Plan (not a blocker): packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
Test Plan（非阻断）：packages/server-core — no such file or directory; packages/server-core/src/sessions/qwen-native-history.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.15)
| sessionLog.info( | ||
| `[external-prune] removing mirror ${managed.id} for workspace ` + |
There was a problem hiding this comment.
[Suggestion] The new [external-prune] audit log — the entire HEAD commit ("record which refresh pruned a session mirror") — is guarded by no test. The platform logger injected in qwen-native-history.test.ts is an all-no-op stub (lines 27–33) and no test captures logger.info, so deleting or breaking this call leaves all 35 tests green. Verified by mutation: with the call deleted, bun test src/sessions/qwen-native-history.test.ts still passes 35/35, and a grep across every *.test.ts under packages/desktop/packages/server-core/src finds zero sessionLog references. Asserting on logger output is already an established pattern in this very file (the neighboring logger.warn capture test near line 2024 uses toContainEqual), so the gap is cheap to close — and without a guard, a future restructure of the prune loop can silently drop the audit trail, making post-restart "my session vanished" reports untraceable again: exactly the regression this commit exists to prevent.
In the existing prune test still prunes a mirror once a listing that reported it stops doing so (which already drives this branch), swap the no-op logger stub's info for a capture array — the same pattern the neighboring logger.warn test uses — and assert exactly one [external-prune] entry naming the removed mirror id is emitted before deletion:
const infos: string[] = [];
// platform stub: logger: { ..., info: (m: string) => infos.push(m) }
// after the second refresh:
expect(
infos.some((m) => m.includes('[external-prune]') && m.includes(sessionId)),
).toBe(true);中文说明
新增的 [external-prune] 审计日志——即整个 HEAD 提交（"record which refresh pruned a session mirror"）——没有任何测试保护。qwen-native-history.test.ts 中注入的平台 logger 是一个全部空实现的 stub（第 27–33 行），且没有测试捕获 logger.info，因此删除或破坏该调用后，全部 35 个测试依然通过。突变验证：删除该调用后，bun test src/sessions/qwen-native-history.test.ts 仍为 35/35 通过；对 packages/desktop/packages/server-core/src 下所有 *.test.ts 的 grep 显示 sessionLog 引用数为零。对 logger 输出做断言在本文件中已有先例（约第 2024 行的 logger.warn 捕获测试使用了 toContainEqual），因此补齐该测试成本很低——而若无测试保护，未来对清理循环的重构可能静默移除审计日志，使重启后"我的会话消失了"的反馈再次无法追溯：这正是该提交要防止的回归。
建议：在已有清理测试 still prunes a mirror once a listing that reported it stops doing so（该测试已覆盖此分支）中，将 no-op logger stub 的 info 替换为捕获数组——与相邻 logger.warn 测试相同的模式——并断言删除前恰好输出一条包含被移除镜像 id 的 [external-prune] 日志。
— qwen3.8-max via Qwen Code /review (v0.21.15)
There was a problem hiding this comment.
Addressed in 2153978.
The existing prune test now captures logger.info around the refresh that deletes and asserts exactly one [external-prune] entry naming the removed mirror, using the capture-and-restore pattern from the neighbouring logger.warn test.
Mutation checked both ways: with the sessionLog.info call deleted the test fails (34 pass / 1 fail), with it restored the file is 35/35 and server-core is 266 pass / 0 fail against a 263 / 0 baseline on main.
Separately, main is merged into the branch (merge, not rebase, so threads stay attached). Test (ubuntu-latest, Node 22.x) is green now; it was failing only because the workflow calls .github/scripts/check-workflow-size.sh, which did not exist in the older checkout. Dependency CVE audit is failing repo-wide on main's own lockfile (high protobufjs advisory), not from this branch.
Review follow-up from qwen-code-ci-bot (R5-1). The log added in eeb8498 was guarded by nothing: the platform logger the suite injects is an all-no-op stub, so deleting the call left every test green. The existing prune test already drives the branch, so it now captures logger.info around the refresh that deletes and asserts exactly one [external-prune] entry naming the removed mirror, the same capture-and-restore pattern the neighbouring logger.warn test uses. Verified by mutation: with the sessionLog.info call deleted the test fails, with it restored the file is 35/35 and server-core is 266 pass / 0 fail. Refs: QwenLM#8400 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
3 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Ratchet key omits the listing's cwd scope (probe-verified pre-existing incomplete-fix note) — already reported (comment 3703006745)
- Volatile ratchet retains mirrors of externally-deleted sessions across restarts — already reported (comment 3703006749)
- Regression tests unreachable by any repo test command or CI job — already reported (comment 3701709157)
Not explored to full depth (tool budget reached): "agent 5": run the three new tests (bun) to confirm they pass and kill the mutations empirically — environment lacks bun/desktop deps and the CI test job had not completed….
Test Plan (not a blocker): src/sessions/qwen-native-history.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/desktop/packages/server-core/src/sessions/SessionManager.ts:3431 — [review] [external-prune] audit log omits the listing cwdpackages/desktop/packages/server-core/src/sessions/SessionManager.ts:3459 — [probe] connectionSlug component of the ratchet key is pinned by no test
中文说明
已审查。
本轮确认的 3 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 5"：run the three new tests (bun) to confirm they pass and kill the mutations empirically — environment lacks bun/desktop deps and the CI test job had not completed…。
Test Plan（非阻断）：src/sessions/qwen-native-history.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Ratchet key omits the listing cwd scope — already reported (comment 3703006745)
- [external-prune] audit log omits the listing cwd — already recorded in the round-6 deferral list (review 4998341027)
- Volatile ratchet retains mirrors of externally-deleted sessions across restarts — already reported (comment 3703006749)
- Regression tests unreachable by any repo test command or CI job — already reported (comment 3701709157)
Not explored to full depth (tool budget reached): "agent 6b": executing the three new bun:test regression tests locally — bun is not installed and the desktop workspace dependencies are absent from the review worktree; ver….
Test Plan (not a blocker): src/sessions/qwen-native-history.test.ts — no such file or directory.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/desktop/packages/server-core/src/sessions/SessionManager.ts:3432 — [probe] audit line interpolates unsanitized provider session id — CR/LF forges the [external-prune] trail
中文说明
已审查。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未探索到全部深度（达到工具调用预算）："agent 6b"：executing the three new bun:test regression tests locally — bun is not installed and the desktop workspace dependencies are absent from the review worktree; ver…。
Test Plan（非阻断）：src/sessions/qwen-native-history.test.ts — no such file or directory。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.15)
|
Hi @doudouOUC, when you have a chance, could you please take another look? Your earlier suggestions have been addressed. |
|
Thanks for the contribution. Closing because the Electron desktop app ( The session-mirror layer this fix protected was deleted with the Electron app, and the related report #8400 was closed today for the same reason: sessions are now owned and persisted by the qwen-code daemon, with no pruning on a failed load. If session loss is still reproducible on the current desktop, please open a new issue with fresh reproduction steps. |
4 participants
What this PR does
Stops the desktop app from deleting session mirrors it has never seen listed.
The provider session listing is scoped to a single working directory. A session started in a different project folder is therefore absent from a given workspace's listing — and
removeMissingExternalListedSessionstreated that absence as proof the session was gone, deleting both the in-memory session and the on-disk mirror with no content check and no per-session log. After a restart every such mirror is unlisted, so the first refresh silently destroyed all of them.This PR keeps a per-workspace record of the provider session ids seen in a completed listing, and prunes a mirror only once it has positively appeared in one. A session that is listed and later disappears — deleted elsewhere, the case the prune exists for — is still cleaned up on the next refresh.
Why it's needed
This is silent, unconfirmed data loss (#8400): sessions vanish from the app on restart with no prompt and no way back from the UI. The prune runs on every session-list refresh, so it removes every foreign-directory mirror at once rather than one at a time.
Reviewer Test Plan
How to verify
Three tests in
packages/desktop/packages/server-core/src/sessions/qwen-native-history.test.tsdrive the realdoRefreshExternalSessionsForWorkspacepath with a fake listing backend: an unlisted mirror survives, a previously-listed mirror is still pruned once it stops being listed (and that prune emits exactly one[external-prune]audit line naming the mirror), and one workspace's listing does not authorise pruning another workspace's mirror of the same id.Evidence (Before & After)
Before: the three tests dropped onto unmodified
mainall fail:After — both tests pass, and the whole
server-coresuite is green:(Baseline on unmodified
mainat the same merge point is263 pass / 0 fail, so the only delta is the three added tests.)The guard is pinned in both directions. Removing it fails the first test; making it skip unconditionally (prune nothing) fails the second:
keeps a restored mirror…still prunes…main)The audit log is pinned the same way: delete the
sessionLog.infocall andstill prunes…fails; restore it and the file is 35/35.Tested on
The report is from Windows, but the defect is OS-independent: the prune decision is list-membership logic with no platform-specific behavior.
Environment
bun testinpackages/desktop/packages/server-core. NoteSessionManagerconstruction reads the real~/.craft-agent/config-defaults.json, so the suite needsensureConfigDir()to have run at least once on the machine (a pre-existing property of these tests, not something this PR changes).Risk & Scope
cwdis the trigger, but fixing it means persistingworkingDirectory/sdkCwdfor Qwen mirrors, which reverses a deliberate strip (SessionManager.tsstripQwenCanonicalStoredFields, andshared/src/sessions/storage.tssaveSession) that existing tests assert. It needs your call. It should not land before a guard like this one: the renderer's unconfirmed auto-delete calls the samedeleteSessionRPC, which forwards to the cwd-scoped backend delete — today the wrong cwd is the only reason the underlying~/.qwentranscripts survive. Correcting the cwd first would point that delete at real transcripts.SessionManager.tsupsert-inspection delete — a load that throws is swallowed toundefinedand collapses into the same!inspectedMessages || length === 0delete branch as a genuinely empty session. Same "failure is not emptiness" class as this fix; left out so it can carry its own test.apps/electron/.../NavigationContext.tsxauto-delete predicate — treats a session with no name and no last message as an abandoned draft; after a restart, stripped names make real sessions match. A one-line fix is to require the absence of a provider id, but there is no React/context test harness underapps/electron, so I have not shipped a line I cannot prove..github/workflowsruns theserver-coresuite. The one Bun job,Qwen Live Host CI, is path-filtered topackages/desktop/apps/live-host/**and runslive-host:test, which is scoped to that app. A green CI run on this PR therefore says nothing about the three tests above; they were verified locally with the output shown.mainwas merged into the branch rather than rebased, so existing review threads stay attached. That clears theTestjob, which was failing on the old checkout because the workflow calls.github/scripts/check-workflow-size.sh, a script added tomainafter this branch. The diff against the merge base is still the same two files.Dependency CVE auditis red repo-wide, not from this branch.npm audit --omit=dev --audit-level=highreports a high advisory inprotobufjsonmain's own lockfile, so every open PR'sSecurity Checksrun is currently failing. This branch changes no dependency and no lockfile.Linked Issues
Refs #8400
AI-assisted. RED→GREEN, the bidirectional mutation check, and the pre-existing baseline were all run locally; the limitations above are stated rather than implied.
中文说明
这个 PR 做了什么
阻止桌面端删除它从未在列表中见过的会话镜像。
provider 的会话列表按单个工作目录(cwd)划定范围。因此在其他项目文件夹中创建的会话不会出现在某个 workspace 的列表里,而
removeMissingExternalListedSessions把这种"缺席"当成了会话已被删除的证据:它直接删除内存中的会话和磁盘镜像,既不检查内容,也没有逐条日志。重启后所有这类镜像都不在列表中,于是第一次刷新就静默销毁了它们。本 PR 为每个 workspace 记录在已完成的列表中出现过的 provider 会话 id,只有出现过的镜像才会被清理。而曾经出现、之后消失的会话(即在别处被删除——正是清理逻辑存在的意义)仍会在下次刷新时被清除。
为什么需要
这是静默且未经确认的数据丢失(#8400):重启后会话从应用中消失,没有任何提示,UI 上也无法找回。该清理逻辑在每次会话列表刷新时运行,因此会一次性删除所有外部目录的镜像。
如何验证
修复前:三个测试在未修改的
main上全部失败。修复后:三个测试均通过,server-core全量 266 通过 / 0 失败(未修改时基线为 263 通过 / 0 失败,差异仅为新增的三个测试)。守卫在两个方向上都被钉住: 移除守卫会让第一个测试失败;让守卫无条件跳过(什么都不清理)会让第二个测试失败。
风险与范围
workingDirectory/sdkCwd,这会推翻既有测试所断言的有意剥离逻辑;且不应在类似本 PR 的守卫之前落地,因为目前正是错误的 cwd 才使底层~/.qwen记录免于被自动删除波及。(2) upsert 检查删除分支中"加载抛错"与"确实为空"的混同。(3) 渲染进程NavigationContext.tsx的自动删除判定——apps/electron下没有 React 测试环境,故未提交无法证明的改动。server-core测试套件。唯一的 Bun 任务Qwen Live Host CI的路径过滤限定在packages/desktop/apps/live-host/**,且只运行live-host:test。因此本 PR 的 CI 全绿并不能证明上述三个测试;它们是在本地验证的。关联 Issue
Refs #8400
Refs #8400