fix(multi-review): retry transient reviewer fetch failures#286
Conversation
Long-running reviewers (reasoning-effort=max) on self-hosted runners
were wiped out by undici 'fetch failed' when an upstream stream
(opencode server -> litellm -> model) went quiet past ~300s. Observed
on PR dogfood: 4/6 reviewers (quality/security/performance/architecture)
died in the same second at the 5m mark, while 2 short reviewers finished
in <1m. A single transient network blip permanently killed the review.
Now the orchestrator retries transient network errors (fetch failed,
ECONNRESET, ETIMEDOUT, UND_ERR_*, socket hang up, ...) up to 3 times
with exponential backoff (1s, 2s), creating a fresh opencode session
each attempt to avoid partial-history pollution in extractText(). The
reviewer's own deadline timeout ('X timed out after Nms') is excluded
since retrying an expired global budget is pointless; non-transient
errors (e.g. 'context length exceeded') still fail immediately.
- OrchestratorOptions.retryBackoffMs (optional) lets tests pass 0.
- Failed attempts tear down their session unconditionally; success
still respects skipSessionCleanup for the live session.
- 3 new tests: retry-then-succeed, retry-exhausted, non-transient no-retry.
- Rebuilt dist/index.cjs.
|
所有 reviewer 均未发现阻塞性问题。综合意见认为设计合理、可合并，但有以下值得关注的项目。 🟡 警告项 / Warnings (4)
🟢 建议项 / Suggestions (10)
💰 Review Cost — $0.0000
📋 各 Reviewer 详细审查结果quality有条件合并 / CONDITIONAL MERGE 本 PR 为 阻塞项：无 警告项：
建议项：
security安全无虞 / SECURE 该 PR 为多 Reviewer 编排器增加了网络瞬态错误的重试机制，未引入新的安全风险。变更聚焦在错误分类（瞬态 vs 永久）、指数退避重试和会话生命周期管理。 阻塞项：无 警告项：无 建议项：
performance性能有疑虑 / CONCERNS 该 PR 为多 reviewer 并行评审引入了重试机制（最多 3 次，指数退避），整体思路合理。经逐项审查，未发现阻塞性性能问题，但有以下几个值得关注的方面： 阻塞项：无 警告项：
建议项：
architecture架构合理 / SOUND 此 PR 在
阻塞项：无 警告项：
建议项：
regression-test缺少回归测试
阻塞项：无 警告项：
建议项：
test-value测试全部有价值 / ALL TESTS HAVE VALUE 严重问题 / CRITICAL： 中等问题 / MEDIUM： 低等问题 / LOW： 阻塞项 / Blocking Issues： 警告项 / Warnings： 建议项 / Suggestions：
|
Svitter
added
fix
Svitter
left a comment
Svitter
left a comment
There was a problem hiding this comment.
P1 — mergeable after a touch. Solid root-cause analysis (undici fetch failed wiping 4/6 reviewers at the ~300s mark of a single upstream blip), and the fix is well-scoped: exponential backoff, fresh session per attempt, owner-deadline excluded, non-transient errors untouched. dist/index.cjs is a clean rebuild (the sleep→sleep2 rename inside the bundled SSE client is just a bundler variable collision, harmless). CHANGELOG entry under [Unreleased] follows the multi-review bullet convention from 4.2.x.
Three should-fix items:
-
Untested resume+retry path (
orchestrator.test.ts:284). The three new tests only exerciseexistingSessions: undefined, but the v2 export path the PR cites as the motivator (index.ts:217-218passes bothexistingSessionsandskipSessionCleanup: willExportSessions) combinesattempt === 1 && resumedFromwith a transient failure andskipSessionCleanup: true. Add a test that asserts the resumed session is deleted, a fresh session is created, andsessionIDis the fresh one. -
Silent contract change under
skipSessionCleanup(orchestrator.ts:204). The pre-PRfinallywas gated on!opts.skipSessionCleanup; this catch deletes the session unconditionally, including on the final non-transient attempt where no retry follows. A non-transient failure on a resumed v2 reviewer now drops the resumable session entirely. Intentional per the in-code comment, but undocumented in the PR body — please add one line under "Fix" and a regression test forskipSessionCleanup: true+ non-transient failure. -
Deadline-blind backoff (
orchestrator.ts:148).sleep(backoff)doesn't consultdeadline. If the reviewer is in its lastglobalTimeoutMs - 30_000window (the dogfood scenario), the 1s+2s sleep burns budget for doomed retries. Cap bydeadline - Date.now() - 30_000or break early.
Nit: the // Unreachable: ... defensive return at orchestrator.ts:218-220 is dead — the for-loop body always returns on success or terminal failure — but harmless. The dead-code return's sessionID: lastSessionId would also be the wrong field for a hypothetical reach (a fresh-create retry's lastSessionId is the resumed session, not the final attempt). Can be deleted.
Thanks for the thorough analysis and the dogfood repro — this is exactly the kind of failure the v2 export path surfaces and the fix targets the right layer.
| const backoff = backoffBase * 2 ** (attempt - 2); | ||
| const prevMsg = lastError instanceof Error ? lastError.message : String(lastError); | ||
| console.log(`[${reviewer.name}] Retry attempt ${attempt}/${REVIEWER_MAX_ATTEMPTS} after ${backoff}ms (previous: ${prevMsg})...`); | ||
| await sleep(backoff); |
There was a problem hiding this comment.
should-fix — sleep(backoff) is deadline-blind. If we are inside the last globalTimeoutMs - 30_000 window (e.g. 4/6 reviewers in the dogfood died at the ~300s mark of a 900s budget, leaving only seconds for the survivors), a 2s sleep + a doomed retry burns the budget for nothing. Consider either capping by deadline (e.g. const cap = Math.max(0, deadline - Date.now() - 30_000); await sleep(Math.min(backoff, cap))) or breaking the loop when Date.now() + backoff > deadline.
| // Failed attempt: tear down THIS session unconditionally — even | ||
| // under skipSessionCleanup we only keep SUCCESSFUL sessions for | ||
| // export; a half-run session is garbage. | ||
| if (sessionId) { |
There was a problem hiding this comment.
should-fix — behavior change worth flagging. Pre-PR, the finally block was gated on !opts.skipSessionCleanup, so a failed reviewer under skipSessionCleanup: true (the v2 export path: index.ts:210-219 sets skipSessionCleanup: willExportSessions) kept the session alive for opencode export to salvage. This catch block now deletes the session unconditionally on every failed attempt, including the final non-transient one — even though there's no retry after the final fail. On the dominant dogfood path, a non-transient failure on a resumed reviewer now drops the resumable session entirely (no fresh session is created on the non-transient exit path). Intentional per the in-code comment, but:
- Not mentioned in the PR body — please add one line under "Fix" describing the new "failed sessions are always torn down, even under
skipSessionCleanup" contract. - Add a test that wires up
skipSessionCleanup: true+ non-transient prompt failure and asserts the session is indeletedSessionsand the failed result carriessuccess: false.
| await cleanupAllSessions(client); | ||
| }); | ||
|
|
||
| it("retries transient fetch failures on a fresh session, then succeeds", async () => { |
There was a problem hiding this comment.
should-fix — coverage gap on the dominant dogfood path. The three new tests exercise existingSessions: undefined (the fresh-create + retry path), but the production code path under v2 cache (index.ts:217 passes existingSessions, index.ts:218 passes skipSessionCleanup: willExportSessions) combines:
- attempt 1 resumes
existingSessions.get(reviewer.name), - prompt throws
fetch failedon attempt 1, - catch block deletes the resumed session and retries with a fresh session.create,
- attempt 2 succeeds, fresh session stays in
activeSessions(skipSessionCleanup=true) so step 7b exports it.
The PR body cites v2 export as the main motivator ("a half-run session is garbage") but the test suite doesn't defend this path. Add one test that wires existingSessions: new Map([["quality", "ses_resumed"]]) + skipSessionCleanup: true + a transient failure on the first prompt → asserts the resumed session was deleted, a fresh session was created, the result is success: true, and sessionID is the fresh session (not the resumed one).
…egression tests Code review on #286 (P1 should-fix items): 1. Deadline-aware backoff. sleep(backoff) ignored the global deadline; in the dogfood scenario (4/6 reviewers dying near the 300s mark of a 900s budget) a doomed retry would burn the survivors' remaining window. Now gives up when remaining budget < backoff + 30s (withTimeout's floor). 2. Jitter. The original failure was N reviewers dying in the SAME second (one upstream blip); unjittered backoff would have them all retry in the same second, re-imposing burst load likely to trip the same limit. backoff now spreads across [base, base+backoffBase]. 3. Tightened isTransientReviewerError: 'terminated' is now scoped ('stream terminated'|'connection terminated') so content-moderation terminations aren't mistakenly retried; timeout exclusion gets word boundaries (\btimed out after \d+ms\b). 4. Hardened the unreachable defensive return so a latent undefined lastError can never surface as the literal string 'undefined'. Contract change (now documented): a failed attempt always tears down its session even under skipSessionCleanup — half-run sessions are garbage, only successful ones are preserved for the v2 export path. Regression tests (3, all green; suite 154/154): - resume + transient retry → fresh session, resumed session deleted - skipSessionCleanup=true + non-transient failure → session torn down - reviewer deadline timeout NOT retried (guards the regex) Rebuilt dist/index.cjs.
|
Pushed 1. Deadline-aware backoff ( 2. Jitter ( 3. Resume + retry path covered ( 4. Contract change documented + tested ( 5. Regex tightened ( 6. Hardened unreachable return ( Suite: 154/154 green (was 151; +3 regression tests). Skipped (non-blocking, per the minimal-fix scope): |
|
✅ 可合并 / CAN MERGE 所有6位 reviewer 均未发现阻塞性问题或警告项。上一轮指出的问题已全部修复，新增的 jitter、deadline-aware 提前退出、改进的正则和 null guard 均获认可。无合并障碍。 🟢 建议项 / Suggestions (6)
💰 Review Cost — $0.0000
📋 各 Reviewer 详细审查结果quality可合并 / CAN MERGE 本次修订已修复上一轮 review 指出的所有问题： 阻塞项：无 警告项：无 建议项：
security安全无虞 / SECURE 该 PR 的更新版本保留了原有的良好安全设计，新增的 jitter 使用 阻塞项：无 警告项：无 建议项：
performance性能良好 / GOOD 与上一版相比，本次修订已解决了之前提出的主要性能疑虑：引入了随机 jitter 避免惊群效应，并增加了 deadline-aware 提前终止机制防止无效重试。当前无新的性能问题。 阻塞项：无 / Blocking Issues: None 警告项：无 / Warnings: None 建议项 / Suggestions:
architecture架构合理 / SOUND 本次修订中，之前提出的大部分架构建议已被采纳： 阻塞项：无 警告项：无 建议项：
regression-test回归测试完整 / REGRESSION TESTS COMPLETE PR类型：BUGFIX 阻塞项：无 警告项：无 建议项：
test-value测试全部有价值 / ALL TESTS HAVE VALUE 严重问题 / CRITICAL： 中等问题 / MEDIUM： 低等问题 / LOW： 阻塞项 / Blocking Issues： 警告项 / Warnings： 建议项 / Suggestions：
|
### Fixed - multi-review: retry transient reviewer failures (undici fetch failed on long-running reviewers) with deadline-aware, jittered exponential backoff. PR #286.
2 participants
问题 / Problem
PR dogfood（latex-agent #4158）里 4/6 reviewer（quality/security/performance/architecture）在启动后约 5 分 2 秒、同一秒全部以 `fetch failed` 失败，2 个浅层 reviewer（<1 分钟完成）正常：
```
15:07:51.9 6 个 reviewer 同时启动
15:08:08.3 regression-test 完成 (299 chars, 16s)
15:08:37.1 test-value 完成 (669 chars, 45s)
15:12:54.8 security/architecture/performance/quality 同一秒 Failed: fetch failed
Reviews: 2/6 succeeded
```
`fetch failed` 是 Node undici 的网络错误。链路是 Node 进程 →（HTTP 流式）→ 本地 opencode server →（SSE）→ litellm 代理 → deepseek。`litellm/deepseek-v4-flash` + `reasoning-effort: max` + `enable-thinking` 在深度思考阶段会长时间无 token 输出，上游任一环节（最可能是 ~300s 流静默）一旦掐断连接，undici 就抛 `fetch failed`。
根因：瞬时网络中断。但 `orchestrator.ts` 对任何抛错零重试——一次连接重置就永久杀死整个 reviewer。
修复 / Fix
`runParallelReviewers` 对瞬时网络错误做指数退避重试：
失败的中间 attempt 无条件销毁其 session（即使是 `skipSessionCleanup` 也只保留成功 session 供 export）；成功仍按原语义尊重 `skipSessionCleanup`。
测试 / Tests
`npm test` 全绿（151 pass），新增 3 个用例：
既有 `captures sessionID on the failure path too`（`LLM down` 非瞬时）仍通过。
未覆盖 / Not in scope
改动文件