feat(base): persist channel sessions across restarts - #3865
Conversation
Session routing was lost on channel restart because clearAll() deleted sessions.json and restoreSessions() was never called at startup. Additionally, AcpBridge.loadSession() always returned undefined due to reading a non-existent field on LoadSessionResponse. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
OverviewThe PR fixes a real bug that prevented persisted channel sessions from being restored after a clean restart of
Bug analysis — confirmed correctI verified against Issues1. No test for the bug — a test-mock false-positive masked it
2. Inconsistent log output between startup and crash recoveryCrash recovery ( if (restoreResult.restored > 0) {
writeStdoutLine(
`[Channel] Sessions restored: ${restoreResult.restored}` +
(restoreResult.failed > 0 ? `, failed: ${restoreResult.failed}` : ''),
);
}If 3. No filtering of persisted entries by configured channels
4. Stale-session accumulation has no escape hatchWith
5. Minor: redundant set after fix
6. Minor: comment wording
Risk assessment
RecommendationApprove with requested changes. The bug fix is correct and the feature is reasonable. Before merging, I'd ask for:
|
gemini-cli/qwen-code/crush/goose coverage: - google-gemini/gemini-cli#26551 externalize https-proxy-agent (man) - QwenLM/qwen-code#3865 persist channel sessions across restarts (man) - charmbracelet/crush#2807 oauth refresh in summarize path (man) - aaif-goose/goose#9047 settings moved into app shell with URL state (man)
- Add restoreSessions() tests: persist file restoration, stale entry skipping, missing file, and empty file cases - Fix log consistency: log restore results when failed > 0 even if restored is 0 (both startSingle and startAll) - Throw in acpAgent.loadSession when session does not exist on disk, preventing stale session accumulation in the persist file - Fix clearAll() JSDoc wording Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the thorough review! I've addressed all requested changes:
|
| } | ||
|
|
||
| /** Clear in-memory state and delete persist file. Used on clean shutdown. */ | ||
| /** Clear in-memory state. Persist file is left intact for the next start |
There was a problem hiding this comment.
[Suggestion] clearAll() now intentionally preserves the persist file, but the test coverage only verifies that the in-memory maps are cleared. This leaves the new shutdown persistence contract unprotected: a future change could re-delete or truncate sessions.json while the current tests would still pass.
Please add a regression test that writes a session through a router with a real persistPath, calls clearAll(), and then asserts that the file still exists and can be restored by a fresh router.
— gpt-5.5 via Qwen Code /review
| }, | ||
| ); | ||
|
|
||
| if (!exists) { |
There was a problem hiding this comment.
[Suggestion] The new sessionExists === false branch is the behavior that lets SessionRouter.restoreSessions() prune stale persisted sessions, but it is not directly covered by ACP agent tests. The router tests mock loadSession, so they would not catch a regression where the real agent accidentally resumes or creates a session for a nonexistent ID again.
Please add an ACP agent test that mocks SessionService.sessionExists(params.sessionId) to return false, expects agent.loadSession(...) to reject with this error, and verifies that session creation/config loading is not attempted.
— gpt-5.5 via Qwen Code /review
…tion Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed both suggestions: Suggestion 1 — clearAll() regression test in SessionRouter.test.ts:
Suggestion 2 — loadSession rejection test in acpAgent.test.ts:
|
| channels.set(name, channel); | ||
| registerToolCallDispatch(bridge, router, channels); | ||
|
|
||
| // Restore sessions from previous run |
There was a problem hiding this comment.
[Suggestion] startSingle 和 startAll 中插入了完全相同的 restoreSessions + 日志代码块（6 行）。建议提取为私有辅助函数以避免未来维护时两处产生分歧：
| // Restore sessions from previous run | |
| async function restoreAndLogSessions(router: SessionRouter): Promise<void> { | |
| const result = await router.restoreSessions(); | |
| if (result.restored > 0 || result.failed > 0) { | |
| writeStdoutLine( | |
| `[Channel] Sessions restored: ${result.restored}` + | |
| (result.failed > 0 ? `, failed: ${result.failed}` : ''), | |
| ); | |
| } | |
| } |
— deepseek-v4-pro via Qwen Code /review
| expect(bridge.newSession).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Suggestion] describe('restoreSessions') 下的 3 个新测试直接调用 rmSync 清理临时目录，没有 try/finally 包裹。若测试中途失败（断言异常等），/tmp/test-restore-* 目录会残留。建议与同文件中 preserves persist file 测试保持一致，使用 try { ... } finally { rmSync(tmpDir, { recursive: true }) } 包裹测试体。
— deepseek-v4-pro via Qwen Code /review
| mockConnectionState.resolve(); | ||
| await agentPromise; | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Suggestion] loadSession rejects when sessionExists returns false 测试通过 vi.mocked(SessionService).mockImplementation(...) 全局覆盖 mock，但测试结束时未恢复默认实现。虽然当前恰好是该 describe 块的最后一个测试，但若未来有人在该块末尾新增测试，残留的 mock 状态可能造成污染。建议在测试中手动保存/恢复原始 mock，或使用 beforeEach 钩子统一重置。
— deepseek-v4-pro via Qwen Code /review
|
Addressed all three suggestions: |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
PR direction is correct — the loadSession return value fix is the right root cause. However, this PR activates a pre-existing data-loss path in restoreSessions() that was dormant before (because loadSession always returned undefined).
Critical issues that cannot be mapped to diff lines (in existing SessionRouter.restoreSessions):
-
All-restore-failure erases persist file: when
restored=0, failed>0,persist()writes empty{}tosessions.json, irreversibly deleting all session routing data. Any transient failure (ACP not ready, NFS delay) triggers permanent data loss. Fix: change guard toif (failed > 0 && restored > 0). -
Restore failures are silent:
catch { failed++ }swallows all error details. At 3 AM the only signal isfailed: 5with no indication of why. Fix: logerr.messageto stderr in the catch block. -
Corrupted persist file silently ignored:
JSON.parsefailure returns{restored:0, failed:0}— indistinguishable from "no sessions to restore". The broken file is never cleaned up, so every subsequent restart silently fails. Fix: emit a warning to stderr.
Suggestion: persist() uses non-atomic writeFileSync — a crash mid-write can corrupt the file. Consider write-to-temp + renameSync.
— deepseek-v4-pro via Qwen Code /review
| } finally { | ||
| // Restore the default mock to avoid polluting subsequent tests | ||
| vi.mocked(SessionService).mockImplementation( | ||
| savedImpl as typeof vi.mocked<typeof SessionService>, |
There was a problem hiding this comment.
[Critical] Type error TS2345/TS2352 — savedImpl as typeof vi.mocked<typeof SessionService> cast is incompatible. getMockImplementation() returns NormalizedProcedure<...> | undefined which cannot be cast to vitest's overloaded MockedObject type. This breaks npm run build.
| savedImpl as typeof vi.mocked<typeof SessionService>, | |
| vi.mocked(SessionService).mockImplementation( | |
| (savedImpl ?? vi.fn()) as (cwd: string) => SessionService, | |
| ); |
— deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings from deeper analysis:
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional Critical findings (cannot be mapped to diff lines):
1. SessionRouter.restoreSessions ~L168 — JSON.parse(null) causes uncaught TypeError crash. JSON.parse can return null (e.g., file content is literally null), but the code only handles parse-throw via catch. If JSON.parse returns null, Object.entries(null) throws an uncaught TypeError, crashing channel start instead of gracefully returning {restored:0, failed:0}.
Fix: add a null/type guard after JSON.parse:
const parsed = JSON.parse(readFileSync(this.persistPath, "utf-8"));
if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) {
return { restored: 0, failed: 0 };
}
entries = parsed as Record<string, PersistedEntry>;2. start.ts crash recovery ~L274 — race condition between channel.setBridge and restoreSessions. In both startSingle and startAll crash handlers, channel.setBridge(bridge) runs before await router.restoreSessions(). The channel is already connected, so messages arriving in this gap hit the router which finds pre-crash session IDs in memory (now populated by initial restoreAndLogSessions) — but the new ACP process hasn't loaded those sessions yet.
Fix: move restoreSessions() between router.setBridge and channel.setBridge:
router.setBridge(bridge);
await router.restoreSessions();
channel.setBridge(bridge);— deepseek-v4-pro via Qwen Code /review
|
Addressed all 5 critical issues: |
| bridge = new AcpBridge(bridgeOpts); | ||
| await bridge.start(); | ||
| router.setBridge(bridge); | ||
| await router.restoreSessions(); |
There was a problem hiding this comment.
[Suggestion] 桥接重启路径丢失了 [Channel] Bridge restarted. Sessions restored: X, failed: Y 日志。本 PR 把 startup 路径用 restoreAndLogSessions(router) 封装并加了日志，但 restart 路径（这里以及 startAll 中 line 440）只裸调用了 router.restoreSessions()，结果不再被读取。运维在 bridge crash 后只能从 SessionRouter 内部逐条 stderr 失败日志反推总数，看不到「成功 N 条、失败 M 条」的概览，是一次明显的可观测性退步。
建议恢复日志：
router.setBridge(bridge);
const result = await router.restoreSessions();
writeStdoutLine(
`[Channel] Bridge restarted. Sessions restored: ${result.restored}` +
(result.failed > 0 ? `, failed: ${result.failed}` : ''),
);或者直接复用 restoreAndLogSessions（restart 时也输出 [Channel] Sessions restored: X 是可以接受的简化）。startAll 的对应位置同改。
|
|
||
| return { restored, failed }; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] 新加的 && restored > 0 守护合理（避免一次性失败清空文件），但反向意味着：当 failed > 0 && restored === 0 时，持久文件永远不会被修剪。如果底层 qwen-code session 存储被清掉、或所有 sessionId 都真的失效，每次启动都会重试同一批死 ID 并写 stderr 失败日志；同时文件还会随后续 resolve() 单调增长。
可考虑：
- 在「至少加载到 1 条」之外，再加一个「连续 N 次全失败则强制修剪」的逃生口；或
- 区分「桥未就绪/瞬时错误」与「agent 明确拒绝（如 sessionExists=false）」，只对后者修剪。
若不想现在改，建议至少留 TODO 标注，避免后续遗忘。
|
|
||
| // Update persist file to only include successfully restored sessions | ||
| if (failed > 0) { | ||
| // Update persist file to remove failed entries. |
There was a problem hiding this comment.
[Nit] 函数顶部的 doc 注释（line 155 附近）仍写着「Failed loads are silently dropped (new session on next message)」，但本 PR 已在此处加了 process.stderr.write(...)，行为不再 silent。建议同步更新 doc：
/**
* ...
* Failed loads are logged to stderr and dropped from the persist file
* (a new session will be created on the next incoming message).
*/| ); | ||
| return { restored: 0, failed: 0 }; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] 这里把 parsed 直接 as Record<string, PersistedEntry>，但没有验证每个 entry 的 shape。如果 sessions.json 被外部改坏，或将来被一个不兼容版本写入，entry.sessionId / entry.cwd / entry.target 可能 undefined，下面的 await this.bridge.loadSession(undefined, undefined) 会被当成普通失败计入 failed，stderr 出现 Failed to restore session undefined: ...，且因为 line 225 的 restored > 0 守护，这种错误条目不一定能被修剪（见我另一条 comment）。
便宜的修法：
if (
typeof entry?.sessionId !== 'string' ||
typeof entry?.cwd !== 'string' ||
typeof entry?.target?.channelName !== 'string'
) {
process.stderr.write(`[SessionRouter] Skipping malformed entry for key ${key}\n`);
failed++;
continue;
}| const { mkdirSync, writeFileSync, rmSync, readFileSync, existsSync } = | ||
| await import('node:fs'); | ||
| const { join } = await import('node:path'); | ||
| const tmpDir = join('/tmp', `test-cleara11-persist-${Date.now()}`); |
There was a problem hiding this comment.
[Nit] 4 个新测试都用 join('/tmp', `test-...-${Date.now()}`) 拼临时目录名。Vitest 文件级并行下，两个测试在同一毫秒启动会撞同名目录；/tmp 也不是 Windows 友好。改用 fs.mkdtempSync 既保证唯一性又跨平台：
import { mkdtempSync } from 'node:fs';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
const tmpDir = mkdtempSync(join(tmpdir(), 'test-restore-'));顺便：本行的目录前缀写成了 test-cleara11-persist-，应是 test-clearAll-persist- 的笔误。
| sessionId, | ||
| cwd, | ||
| mcpServers: [], | ||
| }); |
There was a problem hiding this comment.
[Nit] 行为契约从「返回 agent 报告的 session ID」改成了「直接回显调用者传入的 sessionId，信任 ACP agent 用同样的 ID 加载」。这是正确的——LoadSessionResponse 规范里就没这个字段——但建议加一行注释，避免未来 reader 觉得「这看起来像 bug」又把它改回去：
// LoadSessionResponse has no sessionId field — the ACP agent loads the
// session under the caller-supplied ID, so we just echo it back.
return sessionId;- Add restoreAndLogSessions() in bridge crash-recovery paths (startSingle/startAll) - Add TODO for all-failure accumulation when restored === 0 - Update JSDoc to reflect stderr logging of failed loads - Validate entry shape before calling loadSession to guard corrupted persist files - Fix test temp dir: typo 'cleara11' and use mkdtempSync+tmpdir for cross-platform - Add comment explaining AcpBridge.loadSession returns caller-supplied sessionId - Fix TS cast in acpAgent.test.ts: savedImpl type error
|
Addressed all 6 issues from the latest review round:
Also fixed the TS type error in All tests pass: SessionRouter 28/28 ✓, acpAgent 24/24 ✓. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
This review found 4 Critical and 5 Suggestion issues beyond what was already discussed. Key concerns: (1) restoreSessions catch block doesn't clean in-memory maps during bridge restart, causing dead session IDs to be re-persisted; (2) bridge restart ordering puts restoreAndLogSessions before channel.setBridge/attachDisconnectHandler, risking zombie processes and silent message loss.
| this.toCwd.set(sessionId, entry.cwd); | ||
| restored++; | ||
| } catch { | ||
| } catch (err: unknown) { |
There was a problem hiding this comment.
[Critical] restoreSessions() 的 catch 块在 loadSession 失败时只记录 failed++，不清除 this.toSession、this.toTarget、this.toCwd 中已有的映射。
在桥接重启路径（startSingle / startAll crash recovery）中，router 的内存态残留崩溃前的条目。失败条目的 key 留在 toSession 中 → 后续 persist() 将其重新写回文件 → resolve() 返回死 sessionId → 静默消息丢失。
| } catch (err: unknown) { | |
| } catch (err: unknown) { | |
| const msg = err instanceof Error ? err.message : String(err); | |
| process.stderr.write( | |
| `[SessionRouter] Failed to restore session ${entry.sessionId}: ${msg}\n`, | |
| ); | |
| this.toSession.delete(key); | |
| this.toTarget.delete(entry.sessionId); | |
| this.toCwd.delete(entry.sessionId); | |
| failed++; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| bridge = new AcpBridge(bridgeOpts); | ||
| await bridge.start(); | ||
| router.setBridge(bridge); | ||
| await restoreAndLogSessions(router); |
There was a problem hiding this comment.
[Critical] startSingle 桥接重启路径中 restoreAndLogSessions(router) 被调用于 channel.setBridge(bridge)、registerToolCallDispatch()、attachDisconnectHandler() 之前。
后果：(a) restore 期间 channel 仍持有已死 bridge，到达的消息无法处理；(b) disconnect handler 未注册——bridge 在 restore 中途死亡则进程变为僵尸。
| await restoreAndLogSessions(router); | |
| bridge = new AcpBridge(bridgeOpts); | |
| await bridge.start(); | |
| router.setBridge(bridge); | |
| channel.setBridge(bridge); | |
| registerToolCallDispatch(bridge, router, channels); | |
| attachDisconnectHandler(bridge); | |
| await restoreAndLogSessions(router); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| bridge = new AcpBridge(bridgeOpts); | ||
| await bridge.start(); | ||
| router.setBridge(bridge); | ||
| await restoreAndLogSessions(router); |
There was a problem hiding this comment.
[Critical] startAll 桥接重启路径存在与 startSingle 相同的顺序问题。
| await restoreAndLogSessions(router); | |
| bridge = new AcpBridge(bridgeOpts); | |
| await bridge.start(); | |
| router.setBridge(bridge); | |
| for (const channel of channels.values()) { | |
| channel.setBridge(bridge); | |
| } | |
| registerToolCallDispatch(bridge, router, channels); | |
| attachDisconnectHandler(bridge); | |
| await restoreAndLogSessions(router); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| entries = parsed as Record<string, PersistedEntry>; | ||
| } catch { |
There was a problem hiding this comment.
[Critical] restoreSessions() 中 corrupt JSON（JSON.parse 抛异常）的 catch 分支未被测试覆盖。同时，损坏的 persist 文件永不被清理——每次重启都重复打印 stderr 警告，无法自愈。
建议：(1) 新增测试写入非法 JSON 如 {bad，验证返回 {restored: 0, failed: 0}；(2) 在 catch 分支中重命名损坏文件为 sessions.json.corrupted 或删除，避免永久日志噪音。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const raw = readFileSync(this.persistPath, 'utf-8'); | ||
| const parsed: unknown = JSON.parse(raw); | ||
|
|
||
| // Guard against JSON.parse(null) and other non-object results |
There was a problem hiding this comment.
[Critical] JSON.parse 返回 null / [] / "string" 等非普通对象的 3 条 guard 分支均未被测试。这是本 PR 专门添加的防御代码——评审中明确指出 JSON.parse(null) 会导致 TypeError 崩溃。
建议：新增测试写入 null 和 [] 到 persist 文件，验证返回 {restored: 0, failed: 0} 且不调用 bridge.loadSession。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // an escape hatch: after N consecutive all-failure cycles, force-prune | ||
| // entries that the ACP agent explicitly rejects (sessionExists=false). | ||
| if (failed > 0 && restored > 0) { | ||
| this.persist(); |
There was a problem hiding this comment.
[Suggestion] loadSession 可能因暂时性错误（ensureAuthenticated 网络故障、磁盘 I/O）失败，但 restoreSessions 将所有失败同等对待 → persist() 从文件中永久删除条目。
acpAgent.loadSession() 流程：sessionExists → newSessionConfig → ensureAuthenticated → createAndStoreSession。只有第一步是确定性的。后续步骤的临时错误不应导致路由数据永久丢失。
建议：区分「session 确定不存在」和「加载过程出错」，只有前者才删除条目。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| let restored = 0; | ||
| let failed = 0; | ||
|
|
||
| for (const [key, entry] of Object.entries(entries)) { |
There was a problem hiding this comment.
[Suggestion] restoreSessions 用 for...of 顺序 await 每个 loadSession。每个调用是一次 ACP 往返 + agent 端处理（ensureAuthenticated、createAndStoreSession 等）。启动延迟随持久化条目数线性增长。
多 session 场景下，channel 在 restore 完成前无法接收消息。建议确认 agent 的 loadSession 是否可重入，然后考虑有限并发（如 3-5 个）。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // | ||
| // TODO: When failed > 0 && restored === 0, the persist file is never | ||
| // trimmed — every restart retries the same dead session IDs. Consider | ||
| // an escape hatch: after N consecutive all-failure cycles, force-prune |
There was a problem hiding this comment.
[Suggestion] if (failed > 0 && restored > 0) { this.persist(); } 中全部失败（restored === 0）时跳过 persist 的分支未被测试。这是一个关键的数据安全决策——防止一次性全失败清空所有路由数据。
建议：新增测试，所有条目的 loadSession 都抛异常，验证 restored=0, failed=N，且 persist 文件内容未被修改。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
The startup-path fix is correct and well-targeted — the root cause for loadSession returning undefined is verified against the ACP schema, the clearAll no-op-on-shutdown change is right, and calling restoreSessions before connect() closes the obvious race. Two issues in the bridge crash-recovery handler that this PR also modifies — both narrow, both fixable in a few lines. Happy to approve once they're addressed.
1. Failed restore in crash recovery leaves stale router entries pointing at the dead bridge (severity: medium · confidence: very high)
On bridge crash recovery the router is not cleared before restoreSessions runs, so the pre-crash key → sessionId maps are still in memory. The catch on a failed restore only increments the failure counter; it never drops the key. So if a session that worked before the crash fails to reload on the new bridge (e.g. its underlying file was GC'd), the router keeps resolving that key to a session ID the new bridge has never heard of, and every subsequent envelope for that key fails until the process restarts. The post-loop persist() then writes the stale entry back to disk.
2. Crash-recovery race: channel bridge swapped after the restore await (severity: medium · confidence: high)
In the disconnect handler, restoreAndLogSessions is awaited before channel.setBridge(bridge). The channel transport stays connected across the bridge restart, so during that await the router already points at the live bridge while the channel still holds the dead one. Any inbound envelope landing in this window routes through the new-bridge router but calls prompt/cancel on the dead old bridge held by the channel. Same ordering issue in the startAll disconnect handler. Reordering so channel.setBridge(bridge) (and the per-channel loop in startAll) runs immediately after router.setBridge(bridge), before the restore await, closes both windows.
Verdict
COMMENT — solid main-path fix; happy to approve once the two crash-recovery issues above are addressed.
…rdering Two crash-recovery issues: 1. restoreSessions() now deletes in-memory toSession/toTarget/toCwd entries when loadSession fails, preventing stale session IDs from being re-persisted and silently dropping messages after bridge restart. 2. Reorder crash-recovery in startSingle/startAll: channel.setBridge(), registerToolCallDispatch(), and attachDisconnectHandler() now run before restoreAndLogSessions(), closing a race where the channel held a dead bridge while the router already pointed at the new one.
|
Addressed the two crash-recovery issues raised in the latest review: Issue 1 — Stale router entries after failed restore
Issue 2 — Crash-recovery setBridge ordering Reordered the disconnect handler in both All 29 SessionRouter tests pass. |
|
|
||
| try { | ||
| const sessionId = await this.bridge.loadSession( | ||
| entry.sessionId, |
There was a problem hiding this comment.
[Critical] restoreSessions() 中对 this.bridge.loadSession() 的调用没有超时机制。如果 ACP agent 挂起（网络分区、agent 死锁），该循环将永久阻塞——后续 session 无法恢复，channel 永远无法就绪。
| entry.sessionId, | |
| const LOAD_SESSION_TIMEOUT_MS = 30_000; | |
| try { | |
| const sessionId = await Promise.race([ | |
| this.bridge.loadSession(entry.sessionId, entry.cwd), | |
| new Promise<string>((_, reject) => | |
| setTimeout(() => reject(new Error('loadSession timed out')), LOAD_SESSION_TIMEOUT_MS) | |
| ), | |
| ]); | |
| this.toSession.set(key, sessionId); | |
| this.toTarget.set(sessionId, entry.target); | |
| this.toCwd.set(sessionId, entry.cwd); | |
| restored++; | |
| } catch (err: unknown) { | |
| // ... existing cleanup ... | |
| failed++; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
…ing session restore
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Critical (non-diff-line findings):
-
packages/channels/base/src/SessionRouter.ts:276—persist()非原子写入：writeFileSync采用 truncate-then-write。若进程崩溃或磁盘满，sessions.json损坏为部分 JSON。下次restoreSessions的 catch 返回{restored:0, failed:0}，下一次resolve()调用的persist()仅写入单个新条目——其他所有已持久化的 session 路由永久丢失。修复：atomic write via temp file + rename。 -
SessionRouter.tspersist()— 静默失败使restoreSessions清理不可靠：persist()的 catch 块完全为空。若restoreSessions依赖persist()移除失败条目时写入失败（磁盘满等），清理逻辑静默失效，无任何日志。在 catch 中添加process.stderr.write。
— deepseek-v4-pro via Qwen Code /review
| continue; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
[Suggestion] Promise.race 中 setTimeout 未在成功时清理。loadSession 在 30 秒内返回时，timeoutId 被丢弃，定时器继续运行完整的 30 秒。N 个 session = N 个孤儿定时器。
| try { | |
| try { | |
| let timeoutId: ReturnType<typeof setTimeout>; | |
| const sessionId = await Promise.race([ | |
| this.bridge.loadSession(entry.sessionId, entry.cwd), | |
| new Promise<string>((_, reject) => { | |
| timeoutId = setTimeout( | |
| () => reject(new Error('loadSession timed out')), | |
| LOAD_SESSION_TIMEOUT_MS, | |
| ); | |
| }), | |
| ]); | |
| clearTimeout(timeoutId!); |
— deepseek-v4-pro via Qwen Code /review
| new Promise<string>((_, reject) => | ||
| setTimeout( | ||
| () => reject(new Error('loadSession timed out')), | ||
| LOAD_SESSION_TIMEOUT_MS, |
There was a problem hiding this comment.
[Critical] Promise.race 超时逻辑（LOAD_SESSION_TIMEOUT_MS = 30_000）完全无测试覆盖。超时 reject 路径、stderr 日志、catch 块中的 stale mapping 清理均未被触发。这是本轮 diff 新增的关键防御代码，缺乏覆盖意味着超时处理的任何 bug 只能在生产环境暴露。
建议使用 vi.useFakeTimers() 编写超时测试。
— deepseek-v4-pro via Qwen Code /review
| new Promise<string>((_, reject) => | ||
| setTimeout( | ||
| () => reject(new Error('loadSession timed out')), | ||
| LOAD_SESSION_TIMEOUT_MS, |
There was a problem hiding this comment.
[Critical] Promise.race 超时不取消底层 ACP loadSession 调用。超时触发后 reject 进入 catch 块，标记失败并移除路由，但 ACP agent 的 loadSession（内含 ensureAuthenticated → createAndStoreSession）仍在后台运行。若它最终完成，agent 端残留 router 不知情的孤儿 session。下次同一 key 的 resolve() 通过 bridge.newSession() 创建第二个重复 session——本 PR 恢复历史会话的核心目标在超时场景下被静默打破。
修复方向：(1) 超时后对 loadSession Promise 追加 .catch(() => {}) 防止 unhandled rejection；(2) 中期在 AcpBridge 层支持 AbortSignal 真正取消底层请求。
— deepseek-v4-pro via Qwen Code /review
| @@ -365,6 +374,9 @@ async function startAll(proxy?: string): Promise<void> { | |||
| } | |||
| registerToolCallDispatch(bridge, router, channels); | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] startAll 初始启动路径（line 376）中 await restoreAndLogSessions(router) 无 try/catch 保护，而崩溃恢复路径（line 442）有。虽然 restoreSessions 内部有错误处理，但同步异常（如 EACCES）仍可能传播为未处理 rejection，导致 channel 服务崩溃。
| try { | |
| await restoreAndLogSessions(router); | |
| } catch (err) { | |
| writeStderrLine( | |
| `[Channel] Failed to restore sessions: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| } |
— deepseek-v4-pro via Qwen Code /review
- Atomic write: use temp file + rename for persist() to prevent data corruption on crash or disk full - Add error logging in persist() catch block instead of silent failure - Fix timeout handling: clear setTimeout on success and attach no-op catch to loadSession promise to prevent unhandled rejection - Add try/catch around restoreAndLogSessions in startSingle/startAll initial startup paths for consistent error handling - Add tests for loadSession timeout and timer cleanup scenarios
Addressed all Critical and Suggestion issuesCritical fixes:
Suggestion fixes:
Changes:
All tests pass: 111 tests ✓ |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
补充发现（无法映射到 diff 行）：
-
[Suggestion] 崩溃恢复中
setBridge在restoreSessions之前执行 —startSingle(L279-283) 和startAll(L451-457) 的attachDisconnectHandler内，Bridge 设置后 channel 立即可以接收消息触发resolve()。本 PR 引入的 30 秒超时大幅拉长了 restore 阻塞窗口，resolve()创建的新 session 会被 restore 的陈旧数据覆盖。建议将restoreSessions移到setBridge之前。 -
[Suggestion] 崩溃恢复中误导性错误消息 — L284-286 / L458-460 的 catch 块始终打印
"Failed to restart bridge"，即使 bridge 重启成功但restoreAndLogSessions失败。建议区分两种失败场景。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // is on the same filesystem as the target for atomic rename. | ||
| // rename is atomic on POSIX and works reliably on Windows when | ||
| // source and target are on the same filesystem. | ||
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); |
There was a problem hiding this comment.
[Critical] Cross-device rename failure (EXDEV) — persist() uses mkdtempSync(join(tmpdir(), ...)) to create a temp dir in the OS temp directory (e.g., /tmp on Linux), then renameSync to ~/.qwen/channels/sessions.json. When /tmp is a tmpfs or separate partition (common in Docker / systemd), renameSync throws EXDEV: cross-device link not permitted. The code comment says the temp dir ensures same-filesystem, but tmpdir() defeats that guarantee.
Impact: ALL persist() calls silently fail on affected systems. Session routing data is never persisted — every restart loses all sessions.
Additionally: If writeFileSync or renameSync throws after mkdtempSync succeeds, the temp directory is never cleaned up (catch block only logs stderr).
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); | |
| // Create temp file in the same directory as the target to guarantee | |
| // same-filesystem for atomic rename. No need for a temp directory. | |
| import { dirname } from 'node:path'; | |
| const persistDir = dirname(this.persistPath); | |
| const tmpFile = join(persistDir, '.sessions.json.tmp.' + process.pid); | |
| writeFileSync(tmpFile, JSON.stringify(data, null, 2), 'utf-8'); | |
| renameSync(tmpFile, this.persistPath); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ); | ||
| }), | ||
| ]); | ||
| clearTimeout(timeoutId!); |
There was a problem hiding this comment.
[Suggestion] clearTimeout(timeoutId!) is never reached when loadSession rejects before the timeout (non-timeout failure). The Promise.race jumps to the catch block, leaving the setTimeout timer active for 30s — a resource leak on fast-fail paths.
| clearTimeout(timeoutId!); | |
| let timeoutId: ReturnType<typeof setTimeout> | undefined; | |
| try { | |
| loadPromise = this.bridge.loadSession(entry.sessionId, entry.cwd); | |
| const sessionId = await Promise.race([ | |
| loadPromise, | |
| new Promise<string>((_, reject) => { | |
| timeoutId = setTimeout( | |
| () => reject(new Error('loadSession timed out')), | |
| LOAD_SESSION_TIMEOUT_MS, | |
| ); | |
| }), | |
| ]); | |
| if (timeoutId !== undefined) clearTimeout(timeoutId); | |
| this.toSession.set(key, sessionId); | |
| // ... | |
| } catch (err: unknown) { | |
| if (timeoutId !== undefined) clearTimeout(timeoutId); | |
| loadPromise?.catch(() => {}); | |
| // ... | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| // Restore sessions from previous run | ||
| try { | ||
| await restoreAndLogSessions(router); |
There was a problem hiding this comment.
[Suggestion] The try/catch wrapping restoreAndLogSessions() is duplicated identically in startSingle (L228-234) and startAll (L383-389). Meanwhile, the crash recovery paths (L283, L457 after attachDisconnectHandler) call restoreAndLogSessions() without try/catch, so a restore failure there crashes the process rather than logging and continuing.
| await restoreAndLogSessions(router); | |
| // Extract a shared helper to cover all 4 call sites: | |
| async function safeRestoreAndLogSessions(router: SessionRouter): Promise<void> { | |
| try { | |
| await restoreAndLogSessions(router); | |
| } catch (err) { | |
| writeStderrLine( | |
| `[Channel] Failed to restore sessions: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| } | |
| } |
Replace all 4 await restoreAndLogSessions(router) calls with await safeRestoreAndLogSessions(router).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
- Move restoreAndLogSessions before channel.setBridge to prevent new sessions from being overwritten by stale restore data - Split error handling to distinguish bridge startup failures from session restore failures with appropriate error messages
已处理最新两条建议Issue #1: 崩溃恢复中 setBridge 在 restoreSessions 之前执行修复: 将 这样 restore 完成后才让 channel 开始接收消息，避免了新 session 被 restore 的陈旧数据覆盖的问题。 Issue #2: 崩溃恢复中误导性错误消息修复: 将单一的 try-catch 拆分为两个独立的错误处理
用户现在能清楚区分两种失败场景。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings (cannot be mapped to specific diff lines):
-
[Suggestion] Stale README —
packages/channels/base/README.mdstill describesclearAll()as "Clear all sessions and delete persist file". Update to reflect that the persist file is now preserved. -
[Suggestion]
start.test.tsmissing assertions — The PR addsrestoreAndLogSessions(router)calls to both startup paths, but no test verifies the call happens or tests the error-handling branch. -
[Suggestion] Test coverage gaps — No test for non-object JSON guard (
null/[]/42) or malformed entries with missingsenderId/chatIdfields inSessionRouter.test.ts.
— glm-5.1 via Qwen Code /review
| // is on the same filesystem as the target for atomic rename. | ||
| // rename is atomic on POSIX and works reliably on Windows when | ||
| // source and target are on the same filesystem. | ||
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); |
There was a problem hiding this comment.
[Critical] EXDEV cross-device rename — persist() broken on Linux
mkdtempSync(join(tmpdir(), 'session-persist-')) creates the temp directory in /tmp (typically a tmpfs mount), but the target is ~/.qwen/channels/sessions.json. On Linux where /tmp and /home are separate filesystems, renameSync fails with EXDEV: cross-device link not permitted.
The comment "ensures the temp file is on the same filesystem as the target" is incorrect — tmpdir() provides no such guarantee.
Additionally, when renameSync throws, rmSync(tmpDir) is never reached (no finally), leaking one temp directory per persist() call.
Impact: Session persistence completely broken on standard Linux deployments. Every resolve() and removeSession() leaks a temp dir.
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); | |
| // Atomic write: temp dir adjacent to target for same-filesystem guarantee. | |
| let tmpDir: string | undefined; | |
| try { | |
| tmpDir = mkdtempSync(join(dirname(this.persistPath), '.session-persist-')); | |
| const tmpFile = join(tmpDir, 'sessions.json'); | |
| writeFileSync(tmpFile, JSON.stringify(data, null, 2), 'utf-8'); | |
| renameSync(tmpFile, this.persistPath); | |
| } catch (err: unknown) { | |
| const msg = err instanceof Error ? err.message : String(err); | |
| process.stderr.write( | |
| `[SessionRouter] Failed to persist sessions to ${this.persistPath}: ${msg}\n`, | |
| ); | |
| } finally { | |
| if (tmpDir) { | |
| try { rmSync(tmpDir, { recursive: true, force: true }); } catch { /* best-effort */ } | |
| } | |
| } |
— glm-5.1 via Qwen Code /review
| if ( | ||
| typeof entry?.sessionId !== 'string' || | ||
| typeof entry?.cwd !== 'string' || | ||
| typeof entry?.target?.channelName !== 'string' |
There was a problem hiding this comment.
[Suggestion] Incomplete entry validation — missing senderId/chatId checks
The guard checks entry?.target?.channelName but not senderId or chatId, which are also required fields of SessionTarget. A corrupted persist file entry with a valid channelName but missing senderId/chatId would pass validation, storing an incomplete target in toTarget.
| typeof entry?.target?.channelName !== 'string' | |
| typeof entry?.target?.channelName !== 'string' || | |
| typeof entry?.target?.senderId !== 'string' || | |
| typeof entry?.target?.chatId !== 'string' |
— glm-5.1 via Qwen Code /review
| let restored = 0; | ||
| let failed = 0; | ||
|
|
||
| for (const [key, entry] of Object.entries(entries)) { |
There was a problem hiding this comment.
[Suggestion] Sequential restore blocks startup with N×30s for dead sessions
The for...of + await loop processes sessions sequentially. When loadSession hangs (network partition, agent deadlock), each dead session incurs the full 30s timeout. With N dead sessions, startup is blocked for N×30s with no stdout progress — only individual stderr lines per failure. restoreAndLogSessions writes the summary to stdout only after all sessions are processed.
Consider at minimum logging per-session progress, or adding an overall timeout budget, or running restores in parallel with Promise.allSettled.
— glm-5.1 via Qwen Code /review
| @@ -261,11 +280,7 @@ async function startSingle(name: string, proxy?: string): Promise<void> { | |||
| channel.setBridge(bridge); | |||
There was a problem hiding this comment.
[Suggestion] Crash recovery: channel holds dead bridge during restoreAndLogSessions
In both startSingle and startAll crash-recovery paths, channel.setBridge(bridge) (this line) is called after restoreAndLogSessions(router) (line 283). During the restoration window (potentially N×30s for dead sessions), the channel still holds the old dead bridge. Any incoming messages call this.bridge.prompt() → ensureConnection() throws "Not connected to ACP agent" — messages are silently dropped.
Previously, channel.setBridge was called before restoration, so the channel had the new bridge during this window (though it risked duplicate sessions if a message triggered resolve() before restoration completed).
Consider calling channel.setBridge(bridge) before restoreAndLogSessions, or queuing incoming messages during the restoration window.
— glm-5.1 via Qwen Code /review
| // Using a temp directory (not just a temp file) ensures the temp file | ||
| // is on the same filesystem as the target for atomic rename. | ||
| // rename is atomic on POSIX and works reliably on Windows when | ||
| // source and target are on the same filesystem. |
There was a problem hiding this comment.
[Critical] persist() temp dir leaks session data on write/rename failure
tmpDir is declared inside the try block (line 303), so the catch block cannot access it for cleanup. When writeFileSync or renameSync fails (ENOSPC, EACCES, EXDEV), the temp directory and its fully-written sessions.json file are abandoned in the OS temp area — leaking both disk space and session routing data (sender IDs, chat IDs, session IDs, cwd paths) into a world-readable /tmp.
Additionally, using mkdtempSync + writeFileSync + renameSync + rmSync requires 4 syscalls when a temp file written next to the target (.sessions.json.tmp in dirname(this.persistPath)) achieves the same atomic guarantee with only 2 syscalls and automatically avoids cross-filesystem EXDEV issues (the current comment on lines 298-301 incorrectly claims tmpdir() ensures same-filesystem).
| // source and target are on the same filesystem. | |
| // Atomic write: write to temp file in target's directory then rename. | |
| // This avoids cross-filesystem EXDEV that can occur with os.tmpdir() | |
| // when /tmp is a separate filesystem from the target. | |
| const targetDir = dirname(this.persistPath); | |
| const tmpFile = join(targetDir, '.sessions.json.tmp'); | |
| writeFileSync(tmpFile, JSON.stringify(data, null, 2), 'utf-8'); | |
| renameSync(tmpFile, this.persistPath); |
This also eliminates the need for mkdtempSync, rmSync, and tmpdir imports from the persist() path.
— deepseek-v4-pro via Qwen Code /review
| `[SessionRouter] Corrupted persist file (expected object, got ${parsed === null ? 'null' : Array.isArray(parsed) ? 'array' : typeof parsed}).\n`, | ||
| ); | ||
| return { restored: 0, failed: 0 }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] persistPath logged to stderr exposes home directory
[SessionRouter] Failed to parse persist file: ${this.persistPath} writes the full path (e.g., /Users/<username>/.qwen/channels/sessions.json) to stderr, exposing the OS username on multi-tenant systems where stderr is aggregated.
| } | |
| process.stderr.write( | |
| `[SessionRouter] Failed to parse persist file: ${this.persistPath.replace(os.homedir(), '~')}\n`, | |
| ); |
The same exposure occurs in persist() at line 315.
— deepseek-v4-pro via Qwen Code /review
| entry.cwd, | ||
| // Guard against malformed entries from a corrupted persist file. | ||
| if ( | ||
| typeof entry?.sessionId !== 'string' || |
There was a problem hiding this comment.
[Suggestion] Routing key logged to stderr may expose sender/chat identifiers
The routing key format channelName:senderId:chatId is written directly to stderr when skipping malformed entries. senderId and chatId are externally-visible messaging platform identifiers that may constitute PII or reveal communication patterns.
| typeof entry?.sessionId !== 'string' || | |
| const safeKey = key.slice(0, key.indexOf(':')) + ':***'; | |
| process.stderr.write( | |
| `[SessionRouter] Skipping malformed entry for key ${safeKey}\n`, | |
| ); |
— deepseek-v4-pro via Qwen Code /review
| const tmpFile = join(tmpDir, 'sessions.json'); | ||
| writeFileSync(tmpFile, JSON.stringify(data, null, 2), 'utf-8'); | ||
| renameSync(tmpFile, this.persistPath); | ||
| // Clean up the now-empty temp directory (file was moved by rename) |
There was a problem hiding this comment.
[Suggestion] persist() catch block has no test coverage
The new atomic-write error-handling path (stderr log on writeFileSync/renameSync failure) is completely untested. No test triggers this path via filesystem error injection or spies on process.stderr.write.
When persist() fails (ENOSPC, EACCES, EXDEV), this catch block is the only defense against an unhandled crash. If the message format or error extraction breaks in production, tests won't catch it.
Suggested test: spy process.stderr.write, mock renameSync to throw, call persist() indirectly via resolve(), and assert: (a) the stderr message contains the persist path and error info, (b) the catch block returns gracefully without propagating the exception.
— deepseek-v4-pro via Qwen Code /review
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
C1 (stale router entries on failed restore) is solidly closed with a dedicated test — thanks. The 30s loadSession timeout, atomic-write attempt, and try/catch around the initial restoreAndLogSessions paths are good additions. Two blockers remain before merge — both bugs that break behavior in common deployments. A few one-line follow-ups noted at the end.
1. The channel.setBridge ordering fix from 06036bd6b was reverted in the wrong direction (severity: high · confidence: very high)
The original PR put restoreAndLogSessions before channel.setBridge in the disconnect handler. The prior review pointed out that the channel transport stays connected across the bridge restart, so during the restore await ChannelBase.bridge still holds the dead bridge; inbound messages route through the new-bridge router but call prompt()/cancel() on the dead bridge, throw Not connected to ACP agent, and the adapter replies "Sorry, something went wrong" to the user.
You correctly fixed this in 06036bd6b (moved channel.setBridge and registerToolCallDispatch to run before the restore await), then reverted the fix in 7e40926e3 ("Move restoreAndLogSessions before channel.setBridge to prevent new sessions from being overwritten by stale restore data"). The reverted commit message describes a concern that doesn't materialize — restoreSessions doesn't overwrite a fresher session ID, it sets a freshly-loaded one from disk. The actual catastrophe is the one the revert reintroduced: prompts on a dead bridge.
This is materially worse now because the new 30s × N loadSession timeout from ac56d92b9 extends the dead-bridge window to potentially minutes for N dead sessions, and the user sees error replies for the entire window. @wenshao independently flagged the same problem on start.ts:280. Fix shape: revert to the 06036bd6b ordering — channel.setBridge(bridge) (and the per-channel loop in startAll) runs immediately after router.setBridge(bridge), before the restore await.
2. persist() atomic-write crosses filesystems on Docker, systemd-default Linux, and most CI runners (severity: high · confidence: very high)
The atomic-write rewrite uses mkdtempSync(join(tmpdir(), 'session-persist-')) then renameSyncs sessions.json into ~/.qwen/channels/. On the common case where /tmp is a tmpfs and /home lives on the root or a separate data filesystem (default Docker base images, systemd-default Linux, GitHub/GitLab CI), renameSync across filesystems throws EXDEV: cross-device link not permitted. The catch swallows it to stderr and persist() silently no-ops. Every subsequent resolve() and removeSession() then writes nothing. The core feature this PR exists to add (session persistence across restarts) is broken in those environments. The comment claiming "temp directory on the same filesystem as the target" is incorrect.
There's a privacy compound: when the rename fails, rmSync(tmpDir, ...) is unreached (it sits inside the try block after renameSync), and tmpDir is out of scope in the catch. The temp directory persists with sessions.json containing senderIds, chatIds, sessionIds, and cwds sitting in world-readable /tmp — one PII-leak per failed write.
Fix shape: write the temp file in the same directory as the persist target — either <persistPath>.tmp or mkdtempSync(dirname(persistPath)) — and put rmSync in a finally block. This also covers the pre-existing concern that ~/.qwen/channels/ may not exist on a fresh install (the same mkdirSync(dirname(persistPath), {recursive: true}) once before write closes both).
Verdict
REQUEST_CHANGES — both items above need fixing before merge. While you're in there, three one-line follow-ups worth folding into the same round: clearTimeout(timeoutId!) isn't reached on non-timeout rejection so each fast-fail restore leaks a 30s timer (@wenshao's W7); the persist-entry type guard validates only target.channelName and would let a missing senderId/chatId through (@wenshao's W10); the all-failure-restore case still skips trimming the persist file (the TODO at SessionRouter.ts:268 from the prior review).
| // is on the same filesystem as the target for atomic rename. | ||
| // rename is atomic on POSIX and works reliably on Windows when | ||
| // source and target are on the same filesystem. | ||
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); |
There was a problem hiding this comment.
[Critical] EXDEV cross-device rename — persist() atomic write broken on Linux
mkdtempSync(join(tmpdir(), 'session-persist-')) creates the temp directory in the OS temp directory (e.g. /tmp, typically tmpfs), then renameSync to ~/.qwen/channels/sessions.json. When /tmp and /home are on different filesystems (common on Linux, Docker default), renameSync throws EXDEV: cross-device link not permitted. The comment claims "ensures the temp file is on the same filesystem" — this is incorrect; tmpdir() is system temp, NOT the target directory.
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); | |
| // Atomic write: write to a sibling temp file in the target directory, | |
| // then rename. This guarantees same-filesystem atomic rename on all | |
| // platforms without the overhead of mkdtemp/rmSync. | |
| const tmpFile = this.persistPath + '.tmp'; | |
| writeFileSync(tmpFile, JSON.stringify(data, null, 2), 'utf-8'); | |
| renameSync(tmpFile, this.persistPath); |
This also eliminates the temp directory leak risk (no rmSync needed).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| writeStderrLine( | ||
| `[Channel] Failed to restart bridge: ${err instanceof Error ? err.message : String(err)}`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
[Critical] Bridge restart failure leaves zombie process — return skips channel reconnect
When bridge.start() fails in the disconnect handler, the return statement exits early, skipping channel.setBridge(bridge), registerToolCallDispatch(), and attachDisconnectHandler(). The channel still references the dead bridge and can never process messages again. The process hangs forever on await new Promise<void>(() => {}) — requiring kill -9 to exit.
| return; | |
| } catch (err) { | |
| writeStderrLine( | |
| `[Channel] Failed to restart bridge: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| writeStderrLine( | |
| `[Channel] Bridge restart failed. Channel will remain connected but unable to process messages. Exiting for supervisor restart.`, | |
| ); | |
| channel.disconnect(); | |
| router.clearAll(); | |
| removeServiceInfo(); | |
| process.exit(1); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| await restoreAndLogSessions(router); | ||
| } catch (err) { | ||
| writeStderrLine( | ||
| `[Channel] Bridge restarted but session restore failed: ${err instanceof Error ? err.message : String(err)}`, |
There was a problem hiding this comment.
[Critical] Crash recovery ordering: restoreSessions blocks before channel.setBridge
restoreAndLogSessions(router) (up to 30s per persisted session) runs BEFORE channel.setBridge(bridge) and attachDisconnectHandler(bridge). During the restore window: (a) the channel still holds the old dead bridge, so all incoming messages are silently dropped; (b) no disconnect handler is attached to the new bridge, so if it crashes during restore, no recovery triggers. The old code correctly ordered channel.setBridge + attachDisconnectHandler before restore.
| `[Channel] Bridge restarted but session restore failed: ${err instanceof Error ? err.message : String(err)}`, | |
| channel.setBridge(bridge); | |
| registerToolCallDispatch(bridge, router, channels); | |
| attachDisconnectHandler(bridge); | |
| try { | |
| await restoreAndLogSessions(router); | |
| } catch (err) { | |
| writeStderrLine( | |
| `[Channel] Bridge restarted but session restore failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| failed++; | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Critical] Sequential restore loop — N×30s blocking startup
for...of + await restores sessions one at a time. With N persisted sessions, cold-start and crash-recovery time grows to N × 30s (timeout per session). Each loadSession is an independent ACP IPC call — there is no ordering constraint requiring sequential execution.
| // Parallel restore with per-session timeout | |
| const results = await Promise.allSettled( | |
| Object.entries(entries).map(async ([key, entry]) => { | |
| if ( | |
| typeof entry?.sessionId !== 'string' || | |
| typeof entry?.cwd !== 'string' || | |
| typeof entry?.target?.channelName !== 'string' | |
| ) { | |
| process.stderr.write( | |
| `[SessionRouter] Skipping malformed entry for key ${key}\n`, | |
| ); | |
| return { key, status: 'failed' as const }; | |
| } | |
| try { | |
| const sessionId = await Promise.race([ | |
| this.bridge.loadSession(entry.sessionId, entry.cwd), | |
| new Promise<string>((_, reject) => | |
| setTimeout(() => reject(new Error('loadSession timed out')), LOAD_SESSION_TIMEOUT_MS), | |
| ), | |
| ]); | |
| this.toSession.set(key, sessionId); | |
| this.toTarget.set(sessionId, entry.target); | |
| this.toCwd.set(sessionId, entry.cwd); | |
| return { key, status: 'restored' as const }; | |
| } catch (err: unknown) { | |
| const msg = err instanceof Error ? err.message : String(err); | |
| process.stderr.write( | |
| `[SessionRouter] Failed to restore session ${entry.sessionId}: ${msg}\n`, | |
| ); | |
| return { key, status: 'failed' as const }; | |
| } | |
| }), | |
| ); | |
| restored = results.filter(r => r.status === 'restored').length; | |
| failed = results.filter(r => r.status === 'failed').length; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); | ||
| const tmpFile = join(tmpDir, 'sessions.json'); | ||
| writeFileSync(tmpFile, JSON.stringify(data, null, 2), 'utf-8'); | ||
| renameSync(tmpFile, this.persistPath); |
There was a problem hiding this comment.
[Suggestion] persist() temp directory leaks on write/rename failure
rmSync(tmpDir, { recursive: true }) is inside the try block. If writeFileSync or renameSync throws, the catch block logs the error but the temp directory created by mkdtempSync is never cleaned up. Over time this accumulates orphaned /tmp/session-persist-* directories.
The same-directory temp file approach suggested in the EXDEV fix above eliminates this issue entirely. If keeping the temp-directory approach, move rmSync to a finally block:
| renameSync(tmpFile, this.persistPath); | |
| const tmpDir = mkdtempSync(join(tmpdir(), 'session-persist-')); | |
| try { | |
| const tmpFile = join(tmpDir, 'sessions.json'); | |
| writeFileSync(tmpFile, JSON.stringify(data, null, 2), 'utf-8'); | |
| renameSync(tmpFile, this.persistPath); | |
| } catch (err: unknown) { | |
| const msg = err instanceof Error ? err.message : String(err); | |
| process.stderr.write( | |
| `[SessionRouter] Failed to persist sessions to ${this.persistPath}: ${msg}\n`, | |
| ); | |
| } finally { | |
| try { rmSync(tmpDir, { recursive: true }); } catch {} | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Atomic write: write to temp file in a temp directory then rename. | ||
| // Using a temp directory (not just a temp file) ensures the temp file | ||
| // is on the same filesystem as the target for atomic rename. | ||
| // rename is atomic on POSIX and works reliably on Windows when |
There was a problem hiding this comment.
[Suggestion] persist() silently fails when parent directory does not exist
renameSync(tmpFile, this.persistPath) requires the target parent directory (~/.qwen/channels/) to exist. Nothing in the startup path guarantees this directory is created before the first resolve() triggers persist(). On first install, this causes ENOENT — session routing data is silently lost with only a stderr log line.
| // rename is atomic on POSIX and works reliably on Windows when | |
| private persist(): void { | |
| if (!this.persistPath) return; | |
| // Ensure the parent directory exists | |
| const dir = path.dirname(this.persistPath); | |
| try { mkdirSync(dir, { recursive: true }); } catch { /* best-effort */ } | |
| const data: Record<string, PersistedEntry> = {}; |
Requires importing mkdirSync from node:fs and dirname from node:path.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Update persist file to remove failed entries. | ||
| // Only persist if at least one session was successfully restored — | ||
| // an all-failure persist() would write empty {} and permanently delete | ||
| // routing data that might be recoverable on a subsequent attempt. |
There was a problem hiding this comment.
[Suggestion] All-failure restore never trims persist file — stale entries retried every startup
The guard if (failed > 0 && restored > 0) skips persist() when all sessions fail to restore. The TODO comment acknowledges this: dead session IDs are retried on every restart. If the ACP agent's session store is cleared while the persist file remains, every startup wastes N×30s retrying irrecoverable sessions with no escape hatch.
The TODO suggests an escape hatch after N consecutive all-failure cycles. A simpler approach: when loadSession rejects with a clear "session does not exist" error (i.e., sessionExists returned false, not a network timeout), the entry is provably dead and should be pruned. Timeout errors should still be retried.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| let restored = 0; | ||
| let failed = 0; | ||
|
|
||
| for (const [key, entry] of Object.entries(entries)) { |
There was a problem hiding this comment.
[Suggestion] No progress logging during restore — silent until completion
restoreSessions logs individual failures but emits no "starting restore" message. If the process hangs during restore (e.g., loadSession timeout), the operator sees no indication that restore is in progress — only a silent wait followed by error logs 30s later. If the bridge crashes before any session completes, the summary line never appears.
| for (const [key, entry] of Object.entries(entries)) { | |
| const entryCount = Object.keys(entries).length; | |
| if (entryCount > 0) { | |
| process.stderr.write( | |
| `[SessionRouter] Restoring ${entryCount} sessions from persist file...\n`, | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return path.join(os.homedir(), '.qwen', 'channels', 'sessions.json'); | ||
| } | ||
|
|
||
| async function restoreAndLogSessions(router: SessionRouter): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] restoreAndLogSessions swallows the result — callers can't distinguish partial vs total failure
The helper returns void, discarding { restored, failed }. In the crash-recovery path, the error message says "Bridge restarted but session restore failed" regardless of whether 1/50 or 50/50 sessions failed. Returning the result object would let callers provide better diagnostics.
| async function restoreAndLogSessions(router: SessionRouter): Promise<void> { | |
| async function restoreAndLogSessions(router: SessionRouter): Promise<{ restored: number; failed: number }> { | |
| const result = await router.restoreSessions(); | |
| if (result.restored > 0 || result.failed > 0) { | |
| writeStdoutLine( | |
| `[Channel] Sessions restored: ${result.restored}` + | |
| (result.failed > 0 ? `, failed: ${result.failed}` : ''), | |
| ); | |
| } | |
| return result; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
3 participants
Motivation
Channel sessions are lost every time the channel process restarts (e.g., Ctrl+C then qwen channel start). Users lose their conversation context and a new session is created each time.
Changes
Root cause: AcpBridge.loadSession() always returned undefined
AcpBridge.loadSession() read response.sessionId from the ACP response, but LoadSessionResponse has no sessionId field — so the session router always stored undefined as the session ID, meaning restored sessions were never matched on incoming messages.
Fix: Return the passed-in sessionId directly, since conn.loadSession() confirms the session exists but does not return its ID.
SessionRouter.clearAll() no longer deletes sessions.json
Previously, clearAll() called unlinkSync on the persist file during shutdown, erasing all session routing data. Now it only clears in-memory state, keeping the file for the next startup.
startSingle() and startAll() call restoreSessions() at startup
Both entry points now call router.restoreSessions() before channel.connect(), restoring the routing map from the persisted sessions.json.
How to verify