Skip to content

fix: 修复 SDK result 后流卡死导致的误判 idle 超时#255

Open
lishuceo wants to merge 3 commits into
mainfrom
feat/claude-session-8346ca
Open

fix: 修复 SDK result 后流卡死导致的误判 idle 超时#255
lishuceo wants to merge 3 commits into
mainfrom
feat/claude-session-8346ca

Conversation

@lishuceo
Copy link
Copy Markdown
Owner

@lishuceo lishuceo commented Jun 1, 2026

问题

黎叔反馈 query 频繁报 idle timeout。查线上日志(feishu-claude-out.log)定位到一次超时记录:

{"msg":"Claude query idle timeout — no SDK message received, aborting",
 "idleTimeoutMs":600000,"hasToolActivity":true,
 "lastResetSource":"msg:result:success","elapsedMs":851539}

关键信号 lastResetSource: "msg:result:success":超时前最后一次活动是 result(success)消息。也就是说 query 在约第 4.2 分钟就已经发出终止消息、逻辑上成功了,但 for await 循环没有结束,空转了整整 600s 才被 idle timer 误判超时 abort,最终把一个已成功的 query 当成错误抛给用户。

根因

executor.ts 的消息循环在 case 'result' 里只存下 resultMessage 就继续 for await,依赖 SDK 的异步迭代器自然关闭。但 SDK 偶发在发出 result不关闭迭代器,循环就一直阻塞等待永不到来的下一条消息 → idle timer 600s 后误杀。

修复

  • 给消息循环加 messageLoop: 标签
  • case 'result' 处理后 break messageLoop 立即跳出循环(result 是 query 的终止消息,其后不会再有有效消息),不再依赖 SDK 自然关闭流
  • break 加标签确保跳出 for-await 而非仅 switch

测试

新增回归测试:构造「SDK 发出 resultnext() 永不 resolve」的迭代器,验证 execute() 仍能立即返回成功结果、且只读取 init+result 两条消息。

已验证该测试能抓住 bug:临时把 break messageLoop 退回普通 break(旧逻辑),测试因第三次 next() 永久阻塞而超时失败;恢复修复后通过。

  • npx vitest run src/claude/__tests__/executor.test.ts → 34 passed
  • npm run typecheck → 通过
  • 完整套件仅 memory/quality.test.ts 1 项失败,系缺少 DASHSCOPE_API_KEY 的环境性失败,与本改动无关

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Claude finished @lishuceo's task in 1m 19s —— View job


Claude Code Review

  • Check for previous review comments and resolve fixed ones (none found)
  • Read CLAUDE.md and analyze PR intent
  • Review src/claude/executor.ts changes
  • Review src/claude/__tests__/executor.test.ts changes
  • Post final review summary

✅ Approved — looks good to merge

Verdict: 修复方向正确、改动最小、回归测试充分。可以合并。

修复正确性

messageLoop: 标签 + break messageLoop 是从 switch 内跳出外层 for-await 的标准做法(纯 break 只跳 switch)。这里完全契合:

  • JS 规范保证 for-await-of 通过 break 提前退出时,会调用 iterator 的 return(),触发 SDK 内部清理。
  • result 按 SDK 契约就是 query 的终止消息,其后不会再有有效消息。
  • 跳出后落到 executor.ts:1173-1180 的清理逻辑(clearTimeout / await callbacks / runningQueries.delete),都正常执行。
  • 根因诊断(lastResetSource=msg:result:success 表示最后一次活动就是 result)和修复一一对应。

回归测试质量

executor.test.ts:552-583 构造了「result 后第三次 next() 永不 resolve」的迭代器,并验证了:

  • result.success === true(不再被误判为超时)
  • nextCallCount === 2(确实没尝试读第三条会永远阻塞的消息)

PR 描述里还说明了「临时把 break messageLoop 退回普通 break,测试因第三次 next() 永久阻塞而超时失败」—— 这是对回归测试有效性的关键性验证,做得很扎实。

一些可选的小改进(不阻塞合并)

  1. 测试可额外断言 returnSpy 被调用过executor.test.ts:563):这能更精确地证明 for-await 真的触发了 iterator 关闭,而不只是没继续 pull。当前的 nextCallCount === 2 已经足够推断这点,所以只是 nice-to-have。

  2. iterator.return() 拒绝场景:理论上如果 SDK 的 return() reject,会被 executor.ts:1130 的 catch 捕获,把一个已成功的 query 转成 error。对于当前 SDK 实现这风险极小(return() 通常是 no-op 或安全清理),不需要为此加防御。但如果以后 idle timeout 日志再出现 lastResetSource=msg:result:* 之类的诡异 case,可以回到这里看一眼。

  3. 代码注释里的「SDK 偶发在发出 result 后不关闭异步迭代器」:如果你有 SDK 侧的 issue 链接,可以补一行 // upstream: <url>,方便后续 SDK 升级时回看是否已修复并能撤回 workaround。
    · Branch: feat/claude-session-8346ca

lishuceo and others added 2 commits June 1, 2026 14:29
线上案例:idle timeout 日志中 lastResetSource=msg:result:success、
elapsedMs=851539(14分钟),说明 query 在约第 4 分钟已发出 result(success)
终止消息,但 SDK 偶发未关闭异步迭代器,for-await 继续阻塞等待永不到来的下一条
消息,600s 后被 idle timer 误判超时并 abort,把一个已经成功的 query 当成错误
抛给用户。

修复:给消息循环加 messageLoop 标签,case 'result' 处理后 break messageLoop
立即结束循环(result 是 query 的终止消息,其后不会再有有效消息),不再依赖 SDK
自然关闭流。break 加标签确保跳出 for-await 而非仅 switch。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
构造 SDK 在发出 result(success) 后 next() 永不 resolve 的迭代器,验证
execute() 仍能立即返回成功结果且只读取 init+result 两条消息。该测试在修复
前会因第三次 next() 永久阻塞而超时失败,修复后通过。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lishuceo lishuceo force-pushed the feat/claude-session-8346ca branch from 22d8ba1 to d29a0a5 Compare June 1, 2026 06:31
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Claude finished @lishuceo's task in 48s —— View job


✅ Approved — safe to merge

Verdict: 修复方向正确、改动最小、回归测试设计严谨。

修复正确性 (src/claude/executor.ts:987,1132)

  • messageLoop: 标签 + break messageLoop 是从 switch 内跳出外层 for-await 的标准做法(纯 break 只会跳出 switch)。
  • result 消息的 SDK 契约一致:result 是 query 的终止消息,其后不应再有有效消息,主动 break 不会丢数据。
  • 跳出后落到 executor.ts:1184-1257 的正常成功路径(clearTimeout(idleTimer) / clearTimeout(hardTimer) / await 回调 / runningQueries.delete → 解析 resultMessage 返回 success),所有清理都正确执行。
  • 根因诊断(线上日志 lastResetSource=msg:result:success)与修复一一对应。
  • JS 规范保证 for-await-of 通过 break 提前退出时会调用 iterator 的 return(),自然触发 SDK 内部清理 — 这点测试里的 returnSpy 也间接覆盖到了。

回归测试质量 (src/claude/__tests__/executor.test.ts:585-616)

  • 用「第三次 next() 永不 resolve 的 Promise」精准还原线上 SDK 卡死的场景,比依赖 idle timer 触发(要等 600s)实用得多。
  • 双重断言锁死正确性:result.success === true(不被误判为超时)+ nextCallCount === 2绝不尝试读第三条会阻塞的消息)。
  • PR 描述里"临时把 break messageLoop 退回普通 break、测试因第三次 next() 永久阻塞而超时失败" — 对回归测试有效性的关键反向验证,做得扎实。

可选优化(不阻塞合并)

  1. 测试可加 expect(returnSpy).toHaveBeenCalled():能更精确地证明 for-await 主动触发了 iterator 关闭。当前 nextCallCount === 2 已足够推断,nice-to-have。
  2. iterator.return() 拒绝场景:理论上 SDK 的 return() 如果 reject,会被 executor.ts:1141 的 catch 捕获,把一个已成功的 query 转成 error。当前 SDK 这风险极小(return() 通常 no-op),不需要为此加防御代码,但值得记住 — 万一未来又出现 lastResetSource=msg:result:* 类诡异 case,可以回到这里看一眼。
  3. SDK 侧 issue 链接:注释里"SDK 偶发在发出 result 后不关闭异步迭代器",如果有 upstream issue,加一行 // upstream: <url> 方便后续升级回看。

文件影响

  • src/claude/executor.ts +7/-1:仅修改 query 消息循环退出语义,零外部 API 变动,无副作用。
  • src/claude/__tests__/executor.test.ts +33/-0:新增独立 describe 块,不影响其他用例。

PR 说明 + 代码 + 测试三者吻合,可以合。

--- · Branch: feat/claude-session-8346ca

采纳 review 建议:for-await 通过 break 提前退出时会调用 iterator.return(),
触发 SDK 内部清理。补一条 expect(returnSpy).toHaveBeenCalled() 让该语义在
测试中显式可见,而非仅靠 nextCallCount===2 间接推断。

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lishuceo
Copy link
Copy Markdown
Owner Author

lishuceo commented Jun 1, 2026

Addressed — 已采纳建议 #1:回归测试补充 expect(returnSpy).toHaveBeenCalled(),显式验证 for-await 经 break 退出时触发了 iterator.return() 清理(commit 5327d5a)。

建议 #2(iterator.return() reject 场景)按 review 意见不加防御代码——当前 SDK return() 实质 no-op,风险极小;已在代码注释记录该路径,未来若再现 lastResetSource=msg:result:* 类异常可回看此处。
建议 #3(upstream issue 链接)暂无对应的 SDK upstream issue,不加占位链接,现有注释已说明该偶发行为。

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Claude finished @lishuceo's task in 57s —— View job


✅ Approved — safe to merge

Verdict: 修复方向正确、改动最小、新增的 returnSpy 断言把回归测试的有效性又拔了一档。

本次同步新增(commit 5327d5a

src/claude/__tests__/executor.test.ts:596,606,616 — 按上一轮 review 建议 #1 补强:

const returnSpy = vi.fn(() => Promise.resolve({ value: undefined, done: true }));
mockQueryInstance[Symbol.asyncIterator].mockReturnValue({
  next: () => { /* ... */ },
  return: returnSpy,
});
// ...
expect(returnSpy).toHaveBeenCalled();

这条断言把"for-await 通过 break 提前退出时会自动调用 iterator.return()" 从推断变成直接验证。配合原有的 nextCallCount === 2,等于双重锁死:

  • nextCallCount === 2 → 证明没去读那条永远阻塞的第三条消息
  • returnSpy.toHaveBeenCalled() → 证明真的触发了 SDK 的清理路径(不是单纯"没继续 pull")

未来 SDK 升级时如果不慎引入"break 不调 return()"之类的回归,这个断言能直接抓住。

修复正确性复核(src/claude/executor.ts:987,1125-1132)

  • messageLoop: + break messageLoop 是从 switch 内跳出外层 for-await 的标准做法 ✅
  • 跳出后落到 executor.ts:1184-1257 的成功路径(clearTimeout(idleTimer) / clearTimeout(hardTimer) / await 回调 / 解析 resultMessage 返回 success)✅
  • 注释清楚说明了根因 + 线上日志特征 (lastResetSource=msg:result:success) + 标签必要性,未来读代码不会一头雾水 ✅

回应作者对建议 #2#3 的说明

完全同意:

  • iterator.return() reject 防御:当前 SDK return() 实质是 no-op,加防御代码反而是过度设计。作者保留代码注释作为"未来再现时的回看锚点"已经足够。
  • upstream issue 链接:没有就别加占位 URL,避免误导。注释里已经说明"SDK 偶发"行为,足够了。

文件影响汇总

文件 +/- 风险
src/claude/executor.ts +7/-1 仅修改消息循环退出语义,零外部 API 变动
src/claude/__tests__/executor.test.ts +35/-0 独立新增 describe 块,不影响其他用例
  • Check previous review comments and resolve fixed ones(无既有 inline 评论)
  • Read CLAUDE.md and PR intent
  • Review latest commit (5327d5a) — returnSpy assertion
  • Review src/claude/executor.ts changes
  • Review src/claude/__tests__/executor.test.ts changes
  • Post final review summary

可以合。


· Branch: feat/claude-session-8346ca ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant