fix(channel): keep qqbot token refresh retrying - #5414
Conversation
|
@qwen-code /triage |
wenshao
left a comment
There was a problem hiding this comment.
Clean extraction of token refresh retry logic. The recursive scheduleTokenRefreshRetry() is mechanically sound — stopTokenRefresh() prevents timer accumulation, disposed guards prevent orphaned retries, and fetchToken() on success properly resumes the normal TTL-based cadence. Build passes, 53/53 tests green, eslint clean.
— qwen3.7-max via Qwen Code /review
✅ Local verification — recommend mergeVerified on a clean worktree (real Scope confirmedTrue diff vs 1) Author's test plan — reproduced
2) Real-timer E2E under tmux (the core evidence)Each run drives the real compiled
The OLD vs NEW "always fail" rows are the before/after of the fix: identical setup, but OLD stops at the 2nd attempt while NEW issues the 3rd (180s) and 4th (240s) and would continue indefinitely. The dispose run confirms the loop terminates cleanly on channel teardown (no leaked timer, no retry after disposal). ConclusionCorrect, minimal, and complete. The retry loop now survives repeated token-endpoint outages and recovers when the endpoint returns, while still stopping immediately on dispose. No regression in the qqbot suite. Note (non-blocking): retries are a fixed 60s with no backoff/cap — intended here (a bot should keep trying to restore its token; attempts are cheap and 60s-spaced, and stop on dispose). Fine to merge as-is. 🇨🇳 中文版(点击展开)✅ 本地验证 — 建议合并我在干净的 worktree 中验证(真实 改动范围确认相对 1)复现作者的测试计划
2)tmux 下的实时端到端(核心证据)每次运行都用
旧/新「一直失败」两行就是修复前后的对照:相同设置下,旧代码在第 2 次尝试后停止,而新代码会发起第 3 次(180s)、第 4 次(240s)并将无限继续。dispose 场景确认循环在 channel 拆除时干净终止(无泄漏定时器,dispose 后不再重试)。 结论正确、最小、完整。重试循环现在能挺过 token 端点的反复中断、并在端点恢复时自愈,同时在 dispose 时立即停止。qqbot 套件无回归。 说明(不阻塞):重试为固定 60s、无退避/上限 —— 此处是合理的(机器人应持续尝试恢复 token;每次尝试开销很小、间隔 60s,且在 dispose 时停止)。可按现状合并。 |
|
@qwen-code /triage |
|
Thanks for the PR! The body doesn't follow the PR template headings (uses Summary/Demo/Test plan instead of What this PR does/Why it's needed/Reviewer Test Plan/Risk & Scope/Linked Issues/中文说明), but the substance is all there — clear description, test commands, and it fixes #5411. Not blocking on format. On direction: this is a straightforward P1 bug fix. The token refresh retry loop dies after two consecutive failures, leaving a long-running daemon permanently unable to refresh its QQ Bot token. Clearly a real problem for anyone running the QQ channel adapter in production. Fully aligned. On approach: the scope is tight — one extracted method ( Moving on to code review. 🔍 中文说明感谢贡献! PR body 没有使用 PR 模板 的标准标题(用了 Summary/Demo/Test plan 而不是 What this PR does/Why it's needed/Reviewer Test Plan/Risk & Scope/Linked Issues/中文说明),但实质内容齐全——描述清晰、有测试命令、修复了 #5411。不因格式阻塞。 方向:这是一个明确的 P1 bug 修复。Token 刷新重试链在两次连续失败后永久中断,导致长运行 daemon 无法刷新 QQ Bot token。对生产环境使用 QQ 频道的用户是实实在在的问题,完全对齐。 方案:范围紧凑——提取一个 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe fix extracts
No bugs, no security concerns, no convention violations. The diff is exactly what the bug needs. TestingAll verification from the PR's test plan passed: Tmux real-scenario testing: N/A — this is background token refresh logic, not user-visible behavior. The fake-timer unit test directly exercises the retry loop (failure → failure → recovery) and is the appropriate verification method here. 中文说明代码审查修复提取了
无 bug、无安全问题、无约定违反。diff 正好是修复所需。 测试PR 测试计划中的所有验证均通过(见上方输出)。 Tmux 真实场景测试:不适用——这是后台 token 刷新逻辑,非用户可见行为。fake-timer 单元测试直接覆盖了重试循环(失败→失败→恢复),是合适的验证方式。 — Qwen Code · qwen3.7-max |
|
This is a clean, minimal fix for a real P1 bug. The retry chain in I don't see anything that needs changing. The only note is the PR body template mismatch, which is cosmetic and doesn't affect reviewability. Approving. ✅ 中文说明这是一个干净、最小化的 P1 bug 修复。 没有需要修改的地方。唯一的备注是 PR body 模板标题不匹配,属于外观问题,不影响可审查性。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local real-build verification — recommend mergeVerified this fix end-to-end on a real local build (not just unit tests). It correctly resolves #5411, CI is green on the current head, and the change is well-scoped. Recommend merge. Environment: macOS (Darwin 25.5), Node 22.22, isolated What the PR doesThe old failure path scheduled a single 60s retry; if that retry also failed, the inner 1. CI — all green on
|
| result | |
|---|---|
keeps retrying … until one succeeds |
❌ FAIL — expected called 3 times, but got 2 times (base logs Token refresh failed again after retry and gives up) |
other 39 send.test.ts tests |
✅ pass (fix is scoped to the retry path only) |
The failure message is the A/B: base stops at 2 attempts, the fix continues. I also added an extended fake-timer test (5 failures → recover on the 6th) that fails identically on base (got 2) and passes on the fix.
4. Real built-dist A/B (the shipped artifact, not transpiled)
Ran the compiled dist/QQChannel.js through a harness that drives the real scheduleTokenRefresh recursion (only the network leaf fetchToken stubbed; the 60s timer compressed so the run is fast/deterministic). Same scenario: 5 consecutive failures, then recover.
| Shipped artifact | fetchToken calls |
recovered? | accessToken |
|---|---|---|---|
FIXED dist (scheduleTokenRefreshRetry present) |
6 | ✅ true | recovered-token |
BASE dist (failed again after retry present) |
2 | ❌ false | (unset — token permanently dead) |
This reproduces #5411 on the real artifact: base makes exactly 2 attempts then stops forever; the fix keeps retrying every 60s and recovers when the endpoint comes back.
5. Dispose safety (reverse-audit)
The new timer callback drops the old top-level if (this.disposed) return guard, but this is safe: disconnect() sets disposed = true and calls stopTokenRefresh() (clearTimeout), so a torn-down channel's pending timer is always cleared before it can fire, and the .catch still guards disposed before logging/rescheduling. Confirmed by a disconnect() halts the unbounded loop test (passes on both base and fix — a no-regression check).
Non-blocking observations
- Fixed 60s interval, no backoff — matches the issue's "retry every 60s" wording and is fine; a small jitter/backoff could be a later enhancement so a hard-down endpoint isn't polled at a perfectly fixed cadence.
- The token-refresh retry timer is not
.unref()'d, unlike the reconnect timer (QQChannel.ts:850, from sibling fix(channel): track qqbot close reconnect timer #5416). While the token endpoint is permanently down, the now-indefinite retry timer keeps the event loop alive every 60s. For a daemon that is arguably desirable (it stays alive to auto-recover), anddisconnect()clears it so there's no leak — but it's an asymmetry with the reconnect timer worth a conscious decision.
Verdict: LGTM — the fix is correct, tested, scoped, and reproducibly resolves #5411. Recommend merge.
🇨🇳 中文版(点击展开)
✅ 本地真实构建验证 —— 建议合并
在本地真实构建上做了端到端验证(不只是跑单测)。该修复正确解决了 #5411,当前 head 上 CI 全绿,改动范围收敛得当。建议合并。
环境: macOS(Darwin 25.5)、Node 22.22,基于 head 0eb0d241eb(base 715ef938f5)的独立 git worktree。
这个 PR 做了什么
旧的失败路径只调度一次 60s 重试;如果这次重试又失败,内层 .catch 仅打印 Token refresh failed again after retry 而不再调度任何后续重试 —— 于是连续失败约 2 次后 token 再也不会刷新,长运行 daemon 会静默丢弃此后所有 API 调用(正是 #5411)。修复抽出了 scheduleTokenRefreshRetry(),它在每次失败后都重新调度自己(受 disposed 保护,并通过 stopTokenRefresh() 清掉旧定时器),直到某次刷新成功 —— 成功后 fetchToken() 会回到正常的「TTL 80%」刷新节奏。
1. CI —— 0eb0d241eb 上全绿
Lint、Test (ubuntu / macos / windows, Node 22.x)、CodeQL、Classify PR 全部 success。
2. 单元测试(真实源码,vitest)
send.test.ts + api.test.ts → 53/53 通过。新增测试 keeps retrying scheduled token refresh failures until one succeeds 覆盖了递归重试(2 次失败 → 第 3 次恢复 → 之后不再重复刷新)。
3. 变异测试(决定性 —— 证明测试确实守护了修复)
只把 QQChannel.ts 回退到 base(保留 PR 的所有测试;git diff --stat base 为空),再重跑:
| 结果 | |
|---|---|
keeps retrying … until one succeeds |
❌ 失败 —— expected called 3 times, but got 2 times(base 打印 Token refresh failed again after retry 后放弃) |
send.test.ts 其余 39 个测试 |
✅ 通过(修复只作用于重试路径) |
这条失败信息本身就是 A/B:base 在第 2 次尝试后停住,修复后会继续。我还加了一个扩展的 fake-timer 测试(连续失败 5 次 → 第 6 次恢复),在 base 上同样失败(got 2),在修复后通过。
4. 真实构建产物 dist 的 A/B(跑的是发布产物,不是 transpile 的源码)
用一个 harness 跑编译后的 dist/QQChannel.js,驱动真实的 scheduleTokenRefresh 递归(只 stub 了网络叶子 fetchToken;把 60s 定时器压缩以便快速、确定性地跑完)。同样场景:连续失败 5 次后恢复。
| 发布产物 | fetchToken 调用次数 |
是否恢复 | accessToken |
|---|---|---|---|
修复后 dist(含 scheduleTokenRefreshRetry) |
6 | ✅ 是 | recovered-token |
base dist(含 failed again after retry) |
2 | ❌ 否 | (未设置 —— token 永久失效) |
这在真实产物上复现了 #5411:base 恰好尝试 2 次后永久停止;修复后每 60s 持续重试,端点恢复时随之恢复。
5. 销毁安全性(反向审计)
新的定时器回调去掉了旧代码顶部的 if (this.disposed) return 守卫,但这是安全的:disconnect() 会设置 disposed = true 并调用 stopTokenRefresh()(clearTimeout),所以被销毁的 channel 其待触发定时器总是先被清除、不会再触发;而 .catch 中仍然在打印/重调度前判断了 disposed。已通过 disconnect() halts the unbounded loop 测试确认(base 与修复后都通过 —— 属于无回归检查)。
非阻塞的观察
- 固定 60s 间隔、无退避 —— 与 issue 中「每 60s 重试」的描述一致,没问题;未来可加一点 jitter/退避,避免对一直挂掉的端点以完全固定的节奏轮询。
- token 刷新重试定时器没有
.unref(),这点和重连定时器(QQChannel.ts:850,来自姊妹 PR fix(channel): track qqbot close reconnect timer #5416)不一致。在 token 端点永久不可用期间,如今变为无限的重试定时器会每 60s 让事件循环保持存活。对 daemon 而言这可能是期望行为(保持存活以自动恢复),且disconnect()会清除它、不存在泄漏 —— 但与重连定时器的这处不对称值得作一次有意识的取舍。
结论:LGTM —— 修复正确、有测试、范围收敛,并可复现地解决了 #5411。建议合并。
Summary
Fixes #5411
Demo
N/A — background token refresh retry behavior covered by unit tests.
Test plan
npx -p node@22 node node_modules/vitest/vitest.mjs run --coverage.enabled=false packages/channels/qqbot/src/send.test.ts packages/channels/qqbot/src/api.test.tsnpx eslint packages/channels/qqbot/src/QQChannel.ts packages/channels/qqbot/src/send.test.tsnpx -p node@22 node node_modules/typescript/bin/tsc --noEmit --project packages/channels/qqbot/tsconfig.jsonnpx prettier --check packages/channels/qqbot/src/QQChannel.ts packages/channels/qqbot/src/send.test.tsgit diff --checkAI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.