fix(qqbot): restore AcpBridge session loading — return input sessionId, patch catch path - #7722
Conversation
…d, patch catch path AcpBridge.loadSession() has been returning response.sessionId since introduction, but ACP's LoadSessionResponse schema does not include a sessionId field — so response.sessionId is always undefined. The QQChannel originally worked around this with fixRestoredSessions() that reads the raw persist file and patches the router maps. However, after SessionRouter added validation in PR QwenLM#5978, restoreSessions() throws on undefined sessionId, and the .catch() handler never called fixRestoredSessions(). Before PR QwenLM#6457 this was masked by WebSocket RESUME — the QQ-level session recovery meant restoreSessions() was never reached on reconnect. PR QwenLM#6457 added tryResume=false on non-1000 close, triggering full cold-start restore on every reconnect. Fix: 1. AcpBridge.loadSession() returns sessionId directly (ACP protocol design: LoadSessionResponse omits sessionId because client knows it) 2. QQChannel READY catch handler now calls fixRestoredSessions() so the workaround runs regardless of restoreSessions() outcome Fixes QwenLM#7721
github-actions
Bot
added
the
review/self-reported
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with solid evidence. Issue #7721 documents the root cause chain with logs ( Direction: aligned — this is a straightforward bug fix restoring broken session routing for QQ Bot channels after WebSocket reconnect. No direction concerns. Size: not applicable (channels package, not core paths). Approach: minimal and focused — two small changes, both directly needed. Returning the input Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，证据充分。Issue #7721 记录了完整的根因链和日志（ 方向：对齐——这是一个直接的 bug 修复，恢复 QQ Bot 频道在 WebSocket 重连后的 session 路由。无方向性顾虑。 规模：不适用（channels 包，非核心路径）。 方案：最小且聚焦——两处小改动，都是直接必要的。返回输入的 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the root cause (ACP Findings — no blockers. The The Both consumers of No correctness bugs, security issues, regressions, or convention violations found. The diff is 3 additions / 2 deletions across 2 files — about as minimal as a fix gets. CI Test EvidenceFinal CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Ubuntu unit suite is still running; macOS/Windows tests and integration tests are skipped (likely path-filtered — this PR only touches Real-scenario testing: N/A (unattended CI run — a maintainer can trigger the isolated 中文说明代码审查独立方案： 根据根因（ACP 发现——无阻塞项。
无正确性 bug、安全问题、回归或规范违反。diff 为 2 个文件 3 行新增 / 2 行删除——尽可能精简。 CI 测试证据Ubuntu 单元测试仍在运行；macOS/Windows 测试和集成测试已跳过（可能是路径过滤——此 PR 只触及 真实场景测试：不适用（无人值守 CI 运行）。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean, minimal bug fix with a verified root cause and a solution that matches the existing This is exactly the kind of PR that's easy to review and easy to trust. The root cause is well-documented in #7721 and I confirmed it independently: the ACP If I had to maintain this in six months, I'd thank the author — the fix is obvious in hindsight, well-scoped, and the issue writeup makes the regression chain clear. Approval deferred until CI lands green on 中文说明置信度：5/5 —— 干净、最小化的 bug 修复，根因已验证，方案与现有 这个 PR 正是那种容易审查、容易信任的类型。根因在 #7721 中有详细记录，我也独立确认了：ACP 审批延迟至 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
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
| .catch(() => { | ||
| this.fixRestoredSessions(); |
There was a problem hiding this comment.
[Suggestion] The .then() and .catch() handlers now share four calls — fixRestoredSessions(), finalizeReady(), _checkGroupAllPolicyRequireMention(), and onReady() — duplicated across both paths. A future change to post-restore finalization must be mirrored in two places; if a developer adds a step in one handler but forgets the other, the error path silently diverges from the success path.
Consider extracting the shared calls into .finally():
.then(() => {
// success-specific: session count logging
})
.catch(() => {
// error-specific: warning log
})
.finally(() => {
this.fixRestoredSessions();
this.finalizeReady();
this._checkGroupAllPolicyRequireMention();
onReady();
});— qwen3.7-max via Qwen Code /review
| await conn.loadSession({ | ||
| sessionId, | ||
| cwd, | ||
| mcpServers: [], | ||
| }); | ||
| return response.sessionId; | ||
| return sessionId; |
There was a problem hiding this comment.
[Suggestion] AcpBridge.loadSession has no unit test — AcpBridge.test.ts has zero matches for loadSession. The very return-value bug this PR fixes (returning response.sessionId which was always undefined) went undetected because the SessionRouter tests mock loadSession at the interface level and never exercise the real implementation. Without a test asserting the return value, a future regression re-introducing return response.sessionId would silently pass all existing tests.
Suggested: add a test that mocks conn.loadSession to return a LoadSessionResponse (no sessionId field) and asserts that bridge.loadSession() returns the input sessionId.
— qwen3.7-max via Qwen Code /review
| .catch(() => { | ||
| this.fixRestoredSessions(); |
There was a problem hiding this comment.
[Suggestion] The READY cold-start .catch() handler has no test coverage. The success path (.then()) is tested in events.test.ts, but the failure path — where fixRestoredSessions() is newly added — is never exercised. If a future refactor accidentally removes or misplaces this call, no test would detect the regression.
Suggested: add a test that mocks router.restoreSessions() to reject, dispatches a READY with coldStart=true, and asserts that fixRestoredSessions() is called.
— 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.
LGTM, looks ready to ship — CI landed green after the review. ✅
Review + live ACP protocol verification report (real build, tmux, two-process restart simulation)Verdict: Fix 1 (return the input Fix 1: premise proven on the wireI built the PR (full Three independent layers agree:
So the old Fix 2: the catch-path
|
|
Released in v0.21.1. |
4 participants
What this PR does
Two changes to fix QQ Bot session restore after WebSocket reconnect:
AcpBridge.loadSession() returns the input
sessionIdinstead ofresponse.sessionId— ACP'sLoadSessionResponseschema deliberately omitssessionId(the client already knows it), soresponse.sessionIdis alwaysundefined.QQChannel READY catch handler now calls
fixRestoredSessions()— the existing workaround that reads the rawsessions.jsonpersist file and patches the router maps with correct session IDs. Previously it only ran in the.then()path, which was never reached after PR feat(channels): Add channel agent bridge abstraction #5978 added validation inrestoreSessions().Why it's needed
The QQ Bot loses all session routing after every WebSocket reconnect when QQ sends 4009 (session timeout). Logs show
[SessionRouter] Failed to restore session X: Invalid restored session IDandReady (0 sessions)on every reconnect. All conversation context is lost until a new message creates a fresh session.Reviewer Test Plan
How to verify
qwen channel startwith a QQ Bot channel configuredqwen channel startprocess (or trigger a QQ WebSocket 4009 reconnect)Evidence (Before & After)
Before: After reconnect, logs show:
After: The session is restored from the persist file via
fixRestoredSessions(), andReady (N sessions)shows the count of restored sessions.Tested on
Environment
qwen channel startmode (notqwen serve --channel).Risk & Scope
fixRestoredSessions()uses type coercion to access private SessionRouter fields — it's already labeled as fragile in the existing JSDoc. But this is the same workaround that was working in PR feat(channel): add QQ Bot (QQ机器人) channel adapter #5202, only now also triggered from the catch path.qwen serve --channelmode usesDaemonChannelBridgewhich has a separate loadSession implementation and is unaffected.Linked Issues
Fixes #7721
中文说明
修复 QQ Bot session 在 WebSocket 重连后丢失的问题。两个改动：
response.sessionId——ACP 协议的LoadSessionResponse设计上不含 sessionId，所以response.sessionId一直是 undefined。fixRestoredSessions()——原有的 workaround 会从sessions.json文件直接读取正确 sessionId 并修复路由映射。之前它只在.then()路径执行，但 PR feat(channels): Add channel agent bridge abstraction #5978 后restoreSessions()走.catch()路径了。关联 Issue #7721