feat(channels): recover daemon sessions after restarts - #6680
Conversation
E2E Test ReportStatus: Live credential-backed Telegram group/thread E2E was not run because this environment has no configured Telegram channel or Telegram/BOT token. No live session IDs or restart logs were collected. Automated behavioral coverage
Live scenarios still to run
The PR Reviewer Test Plan contains the exact manual steps and expected outcomes. |
76ee624 to
64b54ff
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）为单个提交。 |
|
Thanks for the PR, @qqqys! Template looks good ✓ Problem: This is an observed architectural limitation — daemon-managed channel workers lose their in-memory route-to-session mapping on restart, forcing a new conversation even when the transcript still exists. Not a theoretical concern; anyone running daemon channels has hit this. Direction: Aligned. Persisting route metadata separately from live daemon bindings, with lazy recovery on next message, is the right pattern. It avoids consuming the daemon's live-session cap for historical channels while maintaining conversation continuity. No direct CHANGELOG reference, but session persistence and channel reliability are clearly within scope. Size: This touches Approach: The scope feels right for the stated goal. The PR handles the full lifecycle: metadata-only restore, lazy load on demand, atomic persistence with quarantine, operation-level invalidation for clear/dispose/restart races, and binding ownership for daemon session cleanup. The ~900 lines of new SessionRouter test coverage exercise the concurrency edge cases (concurrent resolve, invalidated operations, hung cleanup, same-ID replacement). I don't see a materially simpler path — the concurrency concerns are real and require the token/generation pattern. One observation: the operation token + lifecycle generation + route token + binding token pattern is four layers of invalidation tracking. It's necessary for correctness here, but worth a second look in six months to see if any layers can collapse. Moving on to code review. 🔍 中文说明感谢贡献 @qqqys！ 模板完整 ✓ 问题：这是一个已知的架构限制——daemon 托管的 channel worker 重启后会丢失内存中的 route-to-session 映射，即使 transcript 仍然存在也会创建新会话。不是理论问题，运行 daemon channel 的用户都会遇到。 方向：对齐。将 route 元数据与实时 daemon binding 分开持久化，按需懒恢复，是正确的模式。避免为历史 channel 占用 daemon 的 live-session 上限，同时保持会话连续性。CHANGELOG 无直接引用，但 session 持久化和 channel 可靠性明确在范围内。 规模：触及 方案：范围与目标匹配。PR 覆盖了完整生命周期：仅元数据恢复、按需懒加载、原子持久化与隔离、操作级失效处理（clear/dispose/restart 竞态）以及 binding 所有权。~900 行新增 SessionRouter 测试覆盖了并发边界情况。没有看到更简路径——并发问题是真实的，需要 token/generation 模式。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: To solve "daemon channel workers lose route-to-session mapping on restart," I would: (1) add a file-based route store mapping routing keys to session IDs, (2) restore metadata-only at startup without loading sessions, (3) lazily load the session on next message via Comparison with the diff: The PR's approach matches this exactly. The implementation adds Reuse check: No critical blockers found. The code is well-structured with clear separation:
The four-layer invalidation pattern (operation, lifecycle, route, binding tokens) is complex but each layer serves a distinct purpose verified by the test coverage. Test ResultsAll unit tests pass locally (618 tests across affected files): CI also green: Real-Scenario TestingThis PR changes daemon channel session lifecycle — routing, persistence, and lazy recovery. There are no TUI changes and the feature requires a running daemon with configured channel adapters to exercise end-to-end. The unit test suite (94 SessionRouter tests alone) covers the critical paths: metadata-only restore, lazy load/replacement, atomic persistence, clear/dispose/session-death races, concurrent same-route resolution, binding ownership, and hung cleanup. Tmux-based CLI testing would not add meaningful coverage for this type of internal infrastructure change. 中文说明代码审查独立方案与 PR 实现完全匹配。四层失效模式（operation/lifecycle/route/binding token）虽然复杂，但每层都有明确用途并由测试覆盖验证。 复用了已有的 未发现阻断性问题。 测试结果所有单元测试本地通过（618 个测试）：SessionRouter 94、DaemonChannelBridge 44、ChannelBase 393、daemon-worker 52、runtime 11、start 24。CI ubuntu-latest ✅。 真实场景测试本 PR 修改 daemon channel session 生命周期——路由、持久化和懒恢复。无 TUI 变更，需要运行中的 daemon 和配置的 channel adapter 才能端到端测试。单元测试套件覆盖了关键路径。 — Qwen Code · qwen3.7-max |
|
Stepping back to look at the whole picture. The problem is real and well-defined: daemon channel workers lose conversation continuity on restart. The PR solves it with a clean separation between durable route metadata and live daemon bindings — metadata-only restore at startup, lazy load on next message, atomic persistence for crash safety. My independent proposal matched the PR's approach exactly. The implementation is thorough without being over-engineered — each concurrency primitive (operation token, lifecycle generation, route token, binding token) addresses a specific race condition that the tests demonstrate. The The test suite is excellent. 94 SessionRouter tests cover metadata-only restore, lazy load coalescing, concurrent resolve deduplication, discard-after-clear, hung cleanup, same-ID replacement, and disposal races. 44 DaemonChannelBridge tests cover stale factory results, detach/cancel fallback, binding ownership, and lifecycle generation. The existing 393 ChannelBase tests still pass with the 618 tests pass locally. CI green. The 747 production lines are on the larger side, but the change is tightly focused on the stated goal — no drive-by refactors, no unrelated scope creep. The Verdict: This is a well-executed feature PR. The concurrency handling is careful, the test coverage is thorough, and the approach is sound. Approving. 中文说明问题是真实且明确定义的：daemon channel worker 重启后丢失会话连续性。PR 通过将持久 route 元数据与实时 daemon binding 分离来解决——启动时仅恢复元数据，下一条消息时懒加载，原子持久化保证崩溃安全。 独立方案与 PR 实现完全匹配。实现完整但不过度工程化——每个并发原语都针对测试中展示的特定竞态。 测试套件优秀。618 个测试本地通过，CI 绿色。747 行生产代码虽然较多，但紧密聚焦于目标——无顺手重构、无范围蔓延。 结论： 这是一个执行良好的功能 PR。并发处理谨慎，测试覆盖全面，方案合理。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] start.test.ts mock router missing handleSessionDied: runtime.ts (line 248) was changed from router.removeSessionId(...) to router.handleSessionDied(...), but the mock router in packages/cli/src/commands/channel/start.test.ts (~line 91) only defines removeSessionId, not handleSessionDied. Two tests fail with TypeError: router.handleSessionDied is not a function. The sister file runtime.test.ts was correctly updated with handleSessionDied: vi.fn() but start.test.ts was missed.
Add handleSessionDied: vi.fn() to the mock router in start.test.ts and update affected test assertions.
— qwen3.7-max via Qwen Code /review
| if (!this.persistPath) return; | ||
|
|
||
| const data: Record<string, PersistedEntry> = {}; | ||
| for (const [key, sessionId] of this.toSession) { |
There was a problem hiding this comment.
[Suggestion] persist() performs synchronous file I/O (mkdirSync, writeFileSync, renameSync, chmodSync) on every call — after every session creation, removal, promotion, and load. Under high message throughput with many distinct routes, this blocks the event loop cumulatively.
Consider a debounced or dirty-flag + periodic-flush approach so rapid successive state changes coalesce into a single write.
— qwen3.7-max via Qwen Code /review
| const persisted = this.readPersistedEntries(); | ||
| if (!persisted) return { restored: 0, dropped: 0 }; | ||
| this.dispose(); | ||
| let restored = 0; |
There was a problem hiding this comment.
[Suggestion] restoreRoutes() restores entries to toSession/toTarget/toCwd but never adds them to liveSessionIds. This is correct for lazy mode, but the API is fragile: if called on a router with recoveryMode: 'eager', isLive() returns true unconditionally and resolve() would return a session ID that was never actually loaded.
Add a guard: if (this.recoveryMode !== 'lazy') throw new Error('restoreRoutes() requires lazy recovery mode').
— qwen3.7-max via Qwen Code /review
| channel.onSessionDied(event.sessionId); | ||
| } else { | ||
| router.removeSessionId(event.sessionId); | ||
| router.handleSessionDied(event.sessionId); |
There was a problem hiding this comment.
[Critical] runtime.ts now calls router.handleSessionDied(event.sessionId) but the mock SessionRouter in start.test.ts:91 only provides removeSessionId — it was never updated for this API rename. This causes 2 tests to fail:
FAIL start.test.ts > removes router sessions when the bridge reports session death
TypeError: router.handleSessionDied is not a function
FAIL start.test.ts > registers session cleanup on the replacement bridge before restoring sessions
TypeError: router.handleSessionDied is not a function
| router.handleSessionDied(event.sessionId); | |
| router.handleSessionDied(event.sessionId); | |
| } | |
| }); | |
| } |
Add handleSessionDied: vi.fn() to the mock router in start.test.ts (alongside the existing removeSessionId), and update the assertion sites at lines 657, 737, and 783 to check the new mock.
— qwen3.7-max via Qwen Code /review
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/channels/base/src/SessionRouter.ts:607-613 |
restoreSessions doesn't add restored session IDs to liveSessionIds. Currently safe because eager mode's isLive() returns true unconditionally, but if restoreRoutes() is ever used with eager mode, resolve() would hand out stale session IDs the bridge never loaded. |
Add this.liveSessionIds.add(sessionId) after toSession.set(key, sessionId) in the restore loop. |
packages/channels/base/src/SessionRouter.ts:474-483 |
handleSessionDied in lazy mode doesn't call persist(). Dead sessions remain in the persist file, causing unnecessary loadSession round-trips (which fail and trigger replacement) on every daemon restart. |
Either call this.persist() after updating state, or add a comment explaining the intentional deferred persistence. |
packages/channels/base/src/SessionRouter.ts:900-912, DaemonChannelBridge.ts:504-508 |
Three cleanup paths silently swallow errors: scheduleDiscardInvalidatedSession uses .catch(() => undefined), rejectStaleSession stores errors to this.lastError without logging, and releaseSessionClient's detach fallback silently catches. Under sustained failure conditions, daemon sessions leak with zero diagnostic output. |
Add process.stderr.write logging on each catch/swallow path, e.g. [SessionRouter] Failed to discard invalidated session ${sessionId}: ${error}. |
packages/channels/base/src/SessionRouter.ts:523-536 |
restoreRoutes() unconditionally calls dispose() which clears all state and invalidates in-flight operations. The method is public with no guard against post-startup invocation. |
Add a guard if (this.toSession.size > 0) throw new Error(...) or document the startup-only precondition. |
packages/channels/base/src/SessionRouter.ts:304-316,777-808,637 |
Three critical branches lack test coverage: (1) session ID remapping when loadedSessionId !== savedSessionId, (2) persist write failure atomicity, (3) restoreSessions generation-mismatch guard skipping persist. |
Add targeted tests for each branch. |
packages/channels/base/src/AcpBridge.ts |
AcpBridge doesn't implement the optional discardSession method. When an ACP session creation is invalidated, the created session is never cleaned up by the router. |
Implement discardSession on AcpBridge, or document that ACP sessions rely on their own lifecycle cleanup. |
— qwen3.7-max via Qwen Code /review
|
Qwen Code review timed out. Qwen review timed out after 120 minutes. For large PRs, retry with a longer timeout by commenting: |
|
@qwen-code /review --timeout=180 |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/29134953001)._ |
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.
| const dir = dirname(this.persistPath); | ||
| const tempPath = join( | ||
| dir, | ||
| `${Date.now()}-${process.pid}-${Math.random().toString(16).slice(2)}.tmp`, |
There was a problem hiding this comment.
[Suggestion] The catch block in persist() now logs persist failures to stderr but no test exercises this path. Consider adding a test that mocks writeFileSync or renameSync to throw and asserts the stderr output.
— qwen3.7-max via Qwen Code /review
| typeof parsed !== 'object' || | ||
| parsed === null || | ||
| Array.isArray(parsed) | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] readPersistedEntries() has a non-object JSON guard (Array.isArray(parsed), typeof parsed !== 'object') but only syntactically invalid JSON ('{bad') is tested. A test with '[]' as file content would cover this branch.
— qwen3.7-max via Qwen Code /review
| let restored = 0; | ||
| for (const [key, entry] of Object.entries(persisted.entries)) { | ||
| this.toSession.set(key, entry.sessionId); | ||
| this.toTarget.set(entry.sessionId, entry.target); |
There was a problem hiding this comment.
[Suggestion] restoreRoutes() restores all persisted entries without filtering by whether the channel still exists in configuration. If a channel is removed between restarts, its routes persist indefinitely and are re-persisted on every startup. Consider adding TTL-based cleanup or channel-aware filtering.
— qwen3.7-max via Qwen Code /review
| await this.rejectStaleSession(session); | ||
| } | ||
| if (session.sessionId !== sessionId) { | ||
| throw new Error( |
There was a problem hiding this comment.
[Critical] loadSession leaks the factory-created session client when the daemon returns a mismatched session ID. The rejectStaleSession path (generation mismatch, line ~289) properly calls releaseSessionClient via fire-and-forget, but this session-ID-mismatch path throws without any cleanup. The orphaned DaemonChannelSessionClient is never detached or cancelled.
| throw new Error( | |
| if (session.sessionId !== sessionId) { | |
| void this.releaseSessionClient(session).catch((error: unknown) => { | |
| this.lastError = error; | |
| }); | |
| throw new Error( | |
| `Daemon returned session ${session.sessionId} while loading ${sessionId}`, | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| @@ -743,13 +799,20 @@ export class DaemonChannelBridge | |||
| } | |||
|
|
|||
| private dropSession(sessionId: string, reason: string): void { | |||
There was a problem hiding this comment.
[Suggestion] dropSession (called by attachSession when a session with the same ID already exists) calls removeSessionBinding but never calls releaseSessionClient on the replaced session. The old daemon-side client is silently abandoned — same class of leak as the Critical above but on a different code path. Now that the PR introduces releaseSessionClient and uses it in discardSession and rejectStaleSession, dropSession should follow the same pattern for consistency.
| private dropSession(sessionId: string, reason: string): void { | |
| private dropSession(sessionId: string, reason: string): void { | |
| const session = this.removeSessionBinding(sessionId); | |
| if (!session) return; | |
| void this.releaseSessionClient(session).catch((error: unknown) => { | |
| this.lastError = error; | |
| }); | |
| this.emit('sessionDied', { sessionId, reason }); | |
| } |
— qwen3.7-max via Qwen Code /review
| sessionId: string, | ||
| expectedBindingToken?: object, | ||
| ): Promise<void> { | ||
| if ( |
There was a problem hiding this comment.
[Suggestion] The unconditional discard path — calling discardSession(sessionId) without an expectedBindingToken when a token IS stored — is not tested. The existing tests always pass a token. A test that omits the token and asserts the session is unconditionally discarded would lock in the backward-compatible contract and prevent regression if the guard logic changes.
— qwen3.7-max via Qwen Code /review
| } | ||
| } catch (error) { | ||
| this.scheduleDiscardInvalidatedSession(loadedSessionId, operation); | ||
| throw error; |
There was a problem hiding this comment.
[Suggestion] loadOrReplaceSession validates that bridge.loadSession returns a non-empty, non-duplicate session ID and throws 'Invalid or dead restored session ID' otherwise. No test exercises this specific guard. A test configuring loadSession to return an empty string or a duplicate ID would verify the router falls through to replacement creation correctly.
— qwen3.7-max via Qwen Code /review
| const sessionId = this.deleteByKey(k); | ||
| if (sessionId) removedIds.push(sessionId); | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] removeSession() now invalidates in-flight creatingSessions for the matching sender across all chats when scope is not single. No test exercises this new invalidation path. A test that creates an in-flight resolve() for a sender, calls removeSession without a chatId, and verifies the pending resolve rejects with "invalidated" would cover this behavioral addition.
— qwen3.7-max via Qwen Code /review
| return removed; | ||
| } | ||
|
|
||
| handleSessionDied(sessionId: string): boolean { |
There was a problem hiding this comment.
[Suggestion] handleSessionDied returns a boolean indicating whether the session was known. All existing test assertions use toBe(true). No test asserts toBe(false) for an unknown session ID in lazy mode — the false return path (known = this.toTarget.has(sessionId) when session is not in toTarget) is untested.
— qwen3.7-max via Qwen Code /review
| parsed = JSON.parse(readFileSync(persistPath, 'utf-8')); | ||
| } catch (error) { | ||
| const quarantinePath = `${persistPath}.corrupt-${Date.now()}`; | ||
| try { |
There was a problem hiding this comment.
[Suggestion] The corrupt-file quarantine block (renameSync + stderr write + return undefined) is duplicated verbatim between the JSON parse failure path (~line 686-693) and the non-object/array shape check (~line 696-707). Only the error message differs. Extracting this into a small private helper (e.g., quarantineCorruptFile(reason)) would keep the two branches in sync if the quarantine strategy changes.
— qwen3.7-max via Qwen Code /review
| createdRouter.setChannelApprovalMode(name, config.approvalMode); | ||
| } | ||
| } | ||
| const restoredRoutes = createdRouter.restoreRoutes(); |
There was a problem hiding this comment.
[Suggestion] The restoreRoutes log output ([Channel] Restored N dormant route(s)) is untested. The mock mockRouterRestoreRoutes always returns { restored: 1, dropped: 0 }, so the dropped > 0 suffix branch is also never exercised. A test with dropped > 0 and an assertion on the full log message would cover both branches.
— qwen3.7-max via Qwen Code /review
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.
| }); | ||
| if (lifecycleGeneration !== this.lifecycleGeneration) { | ||
| await this.rejectStaleSession(session); | ||
| } |
There was a problem hiding this comment.
[Suggestion] rejectStaleSession calls releaseSessionClient as fire-and-forget with no timeout. If session.detach() hangs indefinitely, the daemon session client is never cleaned up. Consider wrapping with Promise.race and a timeout that falls back to cancel():
| } | |
| void Promise.race([ | |
| this.releaseSessionClient(session), | |
| new Promise<void>((resolve) => setTimeout(resolve, 5_000)), | |
| ]).catch((error: unknown) => { | |
| this.lastError = error; | |
| }); |
— qwen3.7-max via Qwen Code /review
| const persisted = this.readPersistedEntries(); | ||
| if (!persisted) return { restored: 0, dropped: 0 }; | ||
| this.dispose(); | ||
| let restored = 0; |
There was a problem hiding this comment.
[Suggestion] restoreRoutes() populates toSession/toTarget/toCwd without adding to liveSessionIds, which is correct for lazy mode. But there is no guard against accidental use with recoveryMode: 'eager', where isLive() ignores liveSessionIds entirely and restored sessions would appear live without actually being loaded. Consider adding a defensive assertion:
| let restored = 0; | |
| if (this.recoveryMode !== 'lazy') { | |
| throw new Error('restoreRoutes is only valid for lazy recovery mode'); | |
| } |
— qwen3.7-max via Qwen Code /review
| if (loadedSessionId !== savedSessionId) { | ||
| const target = this.toTarget.get(savedSessionId); | ||
| this.deleteByKey(key); | ||
| this.toSession.set(key, loadedSessionId); |
There was a problem hiding this comment.
[Suggestion] When loadedSessionId !== savedSessionId, the target is copied from the old mapping via this.toTarget.get(savedSessionId). If removeSessionId(savedSessionId) ran concurrently during the bridge.loadSession await, the old target is already deleted and loadedSessionId ends up with no target — making the route invisible to persist() and getAll(). Set the target from input instead, consistent with createAndStoreSession:
| this.toSession.set(key, loadedSessionId); | |
| if (loadedSessionId !== savedSessionId) { | |
| this.deleteByKey(key); | |
| this.toSession.set(key, loadedSessionId); | |
| this.toTarget.set(loadedSessionId, { | |
| channelName: input.channelName, | |
| senderId: input.senderId, | |
| chatId: input.chatId, | |
| threadId: input.threadId, | |
| isGroup: input.isGroup, | |
| }); | |
| this.toCwd.set(loadedSessionId, savedCwd); | |
| this.persist(); | |
| } |
— qwen3.7-max via Qwen Code /review
| return removed; | ||
| } | ||
|
|
||
| handleSessionDied(sessionId: string): boolean { |
There was a problem hiding this comment.
[Suggestion] handleSessionDied returns a boolean indicating whether the session was known. All existing test assertions use toBe(true). No test asserts toBe(false) for an unknown session ID in lazy mode — the false return path (early return at the if (!sessionId) guard) is untested.
— qwen3.7-max via Qwen Code /review
| @@ -966,4 +1261,583 @@ describe('SessionRouter', () => { | |||
| expect(bridge.newSession).not.toHaveBeenCalled(); | |||
There was a problem hiding this comment.
[Suggestion] No test exercises concurrent resolve() calls for different routing keys immediately after restoreRoutes(). This is the most realistic post-restart load pattern (N pending inbound messages for N distinct dormant routes). Potential gaps: contention on synchronous persist() from each successful load, and sessionLoadWindows interactions when multiple loads complete simultaneously.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x).
Two previously open Critical comments (runtime.ts:248 mock missing handleSessionDied; DaemonChannelBridge.ts:294 loadSession session client leak) are both resolved in the current code.
— qwen3.7-max via Qwen Code /review
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.
| process.stderr.write( | ||
| `[SessionRouter] Failed to load session ${sanitizeLogText(savedSessionId, 128)} for key ${sanitizeLogText(key, 256)} (${sanitizeLogText(loadError instanceof Error ? loadError.message : String(loadError), 512)}) and failed to create a replacement (${sanitizeLogText(createError instanceof Error ? createError.message : String(createError), 512)})\n`, | ||
| ); | ||
| throw createError; |
There was a problem hiding this comment.
[Suggestion] When both bridge.loadSession and the fallback createLiveSession fail, throw createError discards the original loadError. Callers only see the replacement creation failure — the root cause of why recovery was needed is lost from the error chain. The stderr log captures both messages, but programmatic error handling cannot access the original failure.
| throw createError; | |
| throw new Error( | |
| `Session load and replacement creation both failed`, | |
| { cause: { loadError, createError } }, | |
| ); |
— qwen3.7-max via Qwen Code /review
| ); | ||
| return { restored: 0, failed: 0 }; | ||
| } | ||
| const persisted = this.readPersistedEntries(); |
There was a problem hiding this comment.
[Suggestion] restoreSessions() does not call dispose() before repopulating, unlike restoreRoutes() which calls this.dispose() first. If the router has in-memory sessions for keys NOT in the persisted file (e.g., due to a persist failure between session creation and disk write), those stale entries survive the restore while the bridge has no corresponding session.
Consider adding this.dispose() at the top of restoreSessions() to match restoreRoutes(), or at minimum documenting the assumption that memory is always a subset of the persisted file.
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
| const restoredRoutes = createdRouter.restoreRoutes(); | ||
| writeStdoutLine( |
There was a problem hiding this comment.
[Suggestion] This log line fires unconditionally, printing [Channel] Restored 0 dormant route(s) on every fresh startup where no persist file exists. Consider suppressing the log when restored === 0 && dropped === 0 to avoid noise on first-run scenarios.
| writeStdoutLine( | |
| if (restoredRoutes.restored > 0 || restoredRoutes.dropped > 0) { | |
| writeStdoutLine( | |
| `[Channel] Restored ${restoredRoutes.restored} dormant route(s)` + | |
| (restoredRoutes.dropped > 0 | |
| ? `; dropped ${restoredRoutes.dropped} invalid route(s)` | |
| : ''), | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| if ([...this.toSession.values()].includes(sessionId)) return; | ||
| try { | ||
| void this.bridge | ||
| .discardSession?.(sessionId, operation) |
There was a problem hiding this comment.
[Suggestion] scheduleDiscardInvalidatedSession uses optional chaining on bridge.discardSession without falling back to cancelSession. Bridges that don't implement discardSession (e.g., AcpBridge) will silently leak sessions on every route invalidation — the PR introduces multiple invalidation paths (route removal, lifecycle mismatch, route token mismatch, dispose) that all funnel through this method.
Consider falling back to cancelSession when discardSession is unavailable:
| .discardSession?.(sessionId, operation) | |
| const discard = this.bridge.discardSession?.(sessionId, operation) | |
| ?? this.bridge.cancelSession(sessionId); | |
| void discard.catch(() => undefined); |
— qwen3.7-max via Qwen Code /review
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.
| void this.pumpEvents(session, controller.signal); | ||
| } | ||
|
|
||
| private async rejectStaleSession( |
There was a problem hiding this comment.
[Suggestion] rejectStaleSession is declared Promise<void> but unconditionally throws. Changing the return type to Promise<never> would let TypeScript correctly identify code after await this.rejectStaleSession(...) as unreachable, preventing future maintenance hazards where code could be added between the call and attachSession without a type error.
| private async rejectStaleSession( | |
| private async rejectStaleSession( | |
| session: DaemonChannelSessionClient, | |
| ): Promise<never> { |
— qwen3.7-max via Qwen Code /review
|
Implemented the lazy-only guard for |
✅ Local build & test verification — merge referenceVerified this PR locally in an isolated worktree checked out from Environment: macOS (Darwin 24.6.0) · Node.js v22.23.1 · npm 10.9.8 Results
Total: 888 tests passed · 0 failed.
Reviewer test plan → automated coverageThe 5-point reviewer test plan requires a live daemon-managed Telegram channel with credentials, which is out of scope here (same as the author's stated scope). Each scenario is, however, exercised deterministically by the new
Supporting suites also green:
Non-vacuous check (mutation)To confirm the tests actually pin the new behavior, I dropped the core Scope / not covered
RecommendationBuild, full channel test suites, typecheck, and lint all pass, and the new lazy-recovery tests are non-vacuous and map cleanly onto the reviewer test plan. From a local-verification standpoint this looks good to merge, with the single caveat that credentialed live-Telegram continuity remains verified by unit coverage only, not an end-to-end run. 🇨🇳 中文版本（点击展开）✅ 本地构建与测试验证 —— 合并参考在从 环境： macOS (Darwin 24.6.0) · Node.js v22.23.1 · npm 10.9.8 结果
合计：888 个测试通过 · 0 失败。 Reviewer 测试计划 → 自动化覆盖5 点 reviewer 测试计划需要带凭据的真实 daemon 托管 Telegram channel，这里不在范围内（与作者声明的范围一致）。但每个场景都由新增的
配套套件同样通过： 非空测试检查（变异）为确认测试确实锁定了新行为，我在 范围 / 未覆盖
结论构建、完整 channel 测试套件、类型检查与 lint 全部通过，新增的 lazy-recovery 测试非空且能干净地映射到 reviewer 测试计划。从本地验证角度看可以合并，唯一保留意见是：带凭据的真实 Telegram 连续性仅由单测覆盖，未做端到端运行。 Verified locally on an isolated worktree; no source changes were committed (the mutation was reverted, tree left clean). |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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.
— qwen3.7-max via Qwen Code /review
|
|
||
| onSessionDied(sessionId: string): void { | ||
| this.router.removeSessionId(sessionId); | ||
| this.router.handleSessionDied(sessionId); |
There was a problem hiding this comment.
[Suggestion] handleSessionDied in lazy mode preserves the route mapping (unlike the old removeSessionId), so hasSession() still returns true after a session dies. This causes /status at line 2203 (Session: ${hasSession ? 'active' : 'none'}) to report a dead-but-route-preserved session as "active", which is misleading — the user cannot distinguish a healthy session from one that is dormant and awaiting lazy recovery.
Consider either adding a isSessionLive() method to SessionRouter that checks liveSessionIds, or updating the /status display to differentiate (e.g., "active (dormant)" vs "active").
— qwen3.7-max via Qwen Code /review
| stop(): void { | ||
| this.lifecycleGeneration++; | ||
| for (const sessionId of Array.from(this.sessions.keys())) { | ||
| const session = this.sessions.get(sessionId); |
There was a problem hiding this comment.
[Suggestion] stop() calls session.cancel() directly for each attached session, while every other release path (discardSession, dropSession, attachSession replacement, rejectStaleSession, loadSession mismatch) goes through releaseSessionClient which tries detach() first. Sessions that support graceful detach are hard-cancelled during bridge stop, which may prevent the daemon from preserving session state for re-attachment on the next bridge start — undermining the session recovery goal of this PR.
| const session = this.sessions.get(sessionId); | |
| void this.releaseSessionClient(session).catch((error: unknown) => { | |
| this.lastError = error; | |
| }); |
— qwen3.7-max via Qwen Code /review
3 participants
What this PR does
This PR preserves daemon-managed channel conversations across channel-worker and daemon restarts. It stores the stable channel route separately from the live daemon binding, restores route metadata at worker startup without eagerly loading historical sessions, and lazily reloads the prior session when the next group or thread message arrives.
If the stored session cannot be loaded, the worker creates a replacement and updates the route only after creation succeeds. Explicit clear/reset/new commands remain destructive, while runtime death and normal worker shutdown leave the durable route recoverable. Route writes are atomic, workspace-isolated, permission-restricted where supported, validated on read, and quarantined when corrupted.
The lifecycle handling also prevents clear, disposal, restart, and concurrent resolution races from returning stale sessions or leaking local/remote daemon bindings. Existing standalone and QQ channel flows retain eager recovery and their previous behavior.
Why it's needed
Today, restarting a daemon-managed channel worker loses the in-memory mapping from a stable group or thread to its Qwen Code session. The next message therefore starts a new conversation even though the original transcript is still available. Persisting the route and recovering it lazily keeps conversation continuity without consuming the daemon live-session limit for every historical channel.
Reviewer Test Plan
How to verify
/clear confirmin a shared route and confirm the persisted route is removed and the next message creates a new session.Evidence (Before & After)
N/A — this is channel routing and daemon lifecycle behavior with no TUI layout change. Automated coverage exercises metadata-only restore, lazy load/replacement, atomic persistence, clear/dispose/session-death races, concurrent same-route resolution, binding ownership, and worker facade/shutdown behavior.
Tested on
Environment (optional)
Node.js 22+ local workspace, daemon sandbox disabled for focused unit tests.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 让 daemon 托管的 channel 群聊在 channel worker 或 daemon 重启后继续使用原会话。稳定的 channel route 与实时 daemon binding 分开保存；worker 启动时只恢复 route 元数据，不会立即加载全部历史 session；同一群聊或 thread 的下一条消息到达时，再按需加载之前的 session。
如果已保存的 session 无法加载，worker 会创建替代 session，并且只有创建成功后才更新 route。显式的 clear/reset/new 命令仍会删除 route；运行时 session 死亡和正常 worker 关闭则保留可恢复的持久 route。route 写入使用原子替换，按 workspace 隔离，在平台支持时限制文件权限，读取时校验内容，并隔离损坏文件。
生命周期处理还覆盖了 clear、dispose、restart 和并发 resolve 的竞态，避免返回已失效 session 或泄漏本地/远端 daemon binding。现有 standalone 与 QQ channel 继续使用 eager recovery，行为保持不变。
为什么需要
目前 daemon 托管的 channel worker 重启后，会丢失稳定群聊或 thread 到 Qwen Code session 的内存映射。即使原 transcript 仍然存在，下一条消息也会创建新会话。持久化 route 并按需恢复，可以延续群聊上下文，同时不会为了所有历史 channel 占用 daemon 的 live-session 限额。
Reviewer 测试计划
如何验证
/clear confirm，确认持久 route 被删除，下一条消息创建新 session。前后证据
N/A——这是 channel routing 与 daemon lifecycle 行为变更，不涉及 TUI 布局。自动化测试覆盖了 metadata-only restore、lazy load/replacement、原子持久化、clear/dispose/session-death 竞态、同 route 并发 resolve、binding ownership 以及 worker facade/shutdown 行为。
测试平台
环境（可选）
Node.js 22+ 本地 workspace；focused unit tests 未启用 daemon sandbox。
风险与范围
关联 Issue
N/A