fix(multi-review): serial import + wait for serve ready line#289
Conversation
The v2 session-resume path restored only 2/7 bundles on PRs with 7
reviewers, producing empty reviewer output ("failed to complete")
and forcing CANNOT MERGE. Observed on sun-praise/latex-agent#4165.
Two compounding bugs in session-resume.ts:
1. Bootstrap misjudged readiness: it polled for the opencode.db FILE
and killed `opencode serve` the instant the file appeared. SQLite
creates the file before migrations commit, so the schema was often
half-migrated when imports began.
2. Concurrent imports raced migration DDL: 7 bundles imported via
Promise.allSettled against one SQLite DB. Each `opencode import`
re-ran the non-idempotent migration DDL (ALTER TABLE ADD cost,
CREATE INDEX message_session_time_created_id_idx, INSERT INTO
migration). 5/7 died with "already exists"; only the 2 that won
the lock succeeded.
Fix:
- Wait for the authoritative `listening on http://` line that
`opencode serve` prints AFTER migrations commit (read serve
stdout/stderr instead of polling the db file).
- Import bundles SERIALLY. Per-bundle try/catch still isolates
failures; the few seconds of serialization is worth determinism.
Tests (session-resume.test.ts): deterministic regression tests using
a fake `opencode` binary on PATH. The serial test orchestrates each
import via a per-name release gate and asserts max-in-flight == 1
(under the old parallel code it was N). The bootstrap test delays the
listening line past the cap and asserts the timeout warning fires
(proving we waited for the real signal, not the db file). Both tests
fail when temporarily reverted to the buggy behavior.
Also: bump tsconfig lib to ES2024 (Promise.withResolvers), sandbox
TMPDIR in the suite to avoid cross-test pollution of the
opencode-bundle cleanup assertion, prune stale better-sqlite3
transitive entries from bun.lock.
|
此 PR 修复了两个竞态条件 bug，设计方向正确，测试结构优秀。但有一个运行时阻塞项需修复： 🔴 阻塞项 / Blocking Issues (1)
🟡 警告项 / Warnings (8)
🟢 建议项 / Suggestions (7)
💰 Review Cost — $0.0000
📋 各 Reviewer 详细审查结果quality有条件合并 / CONDITIONAL MERGE 评审总结本 PR 修复了两个关键问题：bootstrap 时等待 阻塞项 / Blocking Issues
警告项 / Warnings
建议项 / Suggestions
security安全无虞 / SECURE 本次 PR 主要修复 阻塞项：无 警告项：无 建议项：
performance性能有疑虑 / CONCERNS 性能分析概要此 PR 主要修复了 影响性能的修改
阻塞项：无无必须阻塞合并的严重性能回归。 警告项
建议项
architecture架构合理 / SOUND 该 PR 修复了 分析总结：
阻塞项：无 警告项：无 建议项：
regression-test缺少回归测试 PR类型：BUGFIX 该 PR 修复了两个关键竞态条件导致的
阻塞项：
警告项：
建议项：无 test-value发现低价值测试 / LOW-VALUE TESTS FOUND CRITICAL: 无 MEDIUM: 无 LOW:
阻塞项：无 警告项：无 建议项：
|
Svitter
left a comment
Svitter
left a comment
There was a problem hiding this comment.
P1 — clean fix, one convention touch-up.
The root-cause analysis is tight and the two fixes are both minimal and correct: the listening-line bootstrap sidesteps the SQLite-pre-migration race, and the serial import loop kills the migration-DDL collision outright. The new session-resume.test.ts is the strongest part of the change — the per-name release-file handshake gives you a deterministic serial-vs-parallel assertion (no wall-clock assumption), and the fake serve that creates opencode.db instantly but delays the listening line correctly distinguishes the two bootstrap strategies. Temp-revert validation is exactly the right move.
Findings (inline):
- The
listening on https?://regex is the bootstrap's only contract and a loose substring match; the defensive fallback at line 140 covers the missing-line case but not a misordered one. Worth pinning the phrasing or adding a secondary post-migration sentinel before the nextopencodebump. serveOutputis unbounded for the lifetime of the serve process. Minor; cap after the regex check.
One thing for the maintainer to decide:
- This PR does not add a
## [Unreleased]entry toCHANGELOG.md. PR #286 did (the prior fix that introduced the[Unreleased]section), and the current top ofCHANGELOG.mdis back to dated-only after 4.2.2. Per the review conventions a fix touching a package should land a[Unreleased]bullet; you can either ask the contributor to add one or fold it into the next release commit yourself. Not blocking the code itself.
Verification already done by the contributor: 151/151 tests pass, dist rebuilt, regex validated against real opencode serve output. The 31 pre-existing bun run check errors on main are out of scope and acceptable to defer.
Thanks for the thorough writeup — the 2/7 repro line and the temp-revert claim in the body are the kind of evidence that makes this a one-glance review.
| let serveOutput = ""; | ||
| const onServeData = (chunk: Buffer | string): void => { | ||
| serveOutput += typeof chunk === "string" ? chunk : chunk.toString("utf8"); | ||
| if (/listening on https?:\/\//i.test(serveOutput)) { |
There was a problem hiding this comment.
should-fix / worth a maintainer eye: the /listening on https?:\/\//i regex is the only contract the bootstrap depends on, and it's a free-form substring match. If a future opencode release prints any other "listening on" line (a progress message, a deprecation notice, etc.) before migrations commit, the bootstrap will fire early and the serial-import safeguard is the only thing standing between the user and the old 2/7 failure. The defensive fallback at multi-review/src/session-resume.ts:140 catches the missing-line case but not the misordered-line case. Worth pinning either to a more specific phrasing (e.g. anchor to the opencode version string you tested against) or wiring in a post-migration sentinel as a secondary confirmation. Not blocking, but flag for the next time opencode is bumped.
| // no-ops, so every handler can call them unconditionally. | ||
| const { promise: readyPromise, resolve: readyResolve, reject: readyReject } = | ||
| Promise.withResolvers<void>(); | ||
| let serveOutput = ""; |
There was a problem hiding this comment.
nit: serveOutput accumulates every byte that opencode serve writes (stdout + stderr) for the lifetime of the process. Once readyResolve fires the regex test is still a no-op cost, but the string keeps growing until the SIGTERM in the finally. On a long-lived or chatty serve this is unbounded. The exit-message truncation (.slice(0, 500)) only matters on the error path. A cap like serveOutput.length > MAX && (serveOutput = serveOutput.slice(-MAX)) after the regex check would bound it without losing the substring property the regex needs.
…output, more tests Review feedback on #289 addressed: BLOCKING - Replace Promise.withResolvers (Node 22+) with a Node 20-compatible local helper. The action's engines.node is >=20 and consumer runners may still be on Node 20, where Promise.withResolvers throws TypeError. Revert the tsconfig lib bump (no longer needed). WARNINGS - Add 'error' handlers on serve stdout/stderr streams so a broken pipe rejects the ready promise instead of hanging forever. - Bound captured serve output to the last 4KB (tail is what matters for listening-line detection and crash diagnostics; unbounded growth was possible if serve logged verbosely before the ready line). TESTS (regression-test CRITICAL/MEDIUM + test-value gaps) - serve spawn failure (missing binary) → no throw, warning, empty map. - serve crash before listening line (exit 1) → warning, fallback path. - partial bundle failure → per-bundle try/catch isolates; rest succeed. - bootstrap-timeout test now asserts existingSessions.size === 1 (proves the fallback import actually ran, not just that we warned). - empty-input test covers null, undefined, and empty bundles array. - fake serve's keepalive setInterval now calls unref() so it can't pin the test process exit. 154/154 tests pass; session-resume files typecheck clean.
|
✅ 可合并 / CAN MERGE 所有 6 位 reviewer 均同意合并，上一轮指出的所有问题已在本轮修复（ 🟢 建议项 / Suggestions (1)
💰 Review Cost — $0.0000
📋 各 Reviewer 详细审查结果quality可合并 / CAN MERGE 本次重审确认前一版所有问题均已修复：
阻塞项：无 警告项：无 建议项：无 security安全无虞 / SECURE 本次 re-review 的改动（自定义 阻塞项：无 警告项：无 建议项：无 performance性能良好 / GOOD 本版本修复了上一轮指出的所有性能问题： 阻塞项：无 警告项：无 建议项：
architecture架构合理 / SOUND 本次 re-review 的 diff 相比上一轮有显著改进：之前的架构阻断项（
阻塞项：无 警告项：无 建议项：无 regression-test回归测试完整 / REGRESSION TESTS COMPLETE PR类型：BUGFIX 上一轮评审指出的回归测试缺口已全部补充：
阻塞项：无 警告项：无 建议项：无 test-value测试全部有价值 / ALL TESTS HAVE VALUE CRITICAL: 无 MEDIUM: 无 LOW: 无 阻塞项：无 警告项：无 建议项：无 |
### Fixed - multi-review: fix v2 session-resume restoring only a fraction of bundles on PRs with many reviewers. Two compounding bugs in session-resume.ts caused 2/7 bundles restored on 7-reviewer PRs, empty reviewer output, and forced CANNOT MERGE. PR #289.
2 participants
问题
OpenCode Multi-Review在 reviewer 数量较多的 PR 上 session-resume 失败：只有 2/7 的 bundle 被恢复，其余 5 个 reviewer 输出为空，coordinator 报「failed to complete」，最终强制 CANNOT MERGE。线上实例：sun-praise/latex-agent#4165（10:07 那次 run，job 成功但 5 个 reviewer 全空）。
CI 日志关键行：
根因（两个叠加 bug，均在
multi-review/src/session-resume.ts）opencode.db文件是否存在，文件一出现就 killopencode serve。但 SQLite 在 migration 提交之前就会创建 db 文件，于是 schema 经常是半成品。Promise.allSettled并发opencode import，共用同一个 temp DB。每个 import 检测到 schema 版本旧就重跑非幂等的 migration DDL（ALTER TABLE ADD cost/CREATE INDEX/INSERT INTO migration），互相撞「already exists」，5 个失败、2 个抢到锁的成功。2/7成功 = performance + regression-test，正好对应那条 PR 评论里只有这俩 reviewer 有内容。修复
opencode serve在所有 migration 提交之后才输出opencode server listening on http://...。改为读 serve 的 stdout/stderr，匹配到这行才认为 schema 就绪（带超时 + spawn-error + premature-exit 兜底）。测试（新增
session-resume.test.ts）用 fake
opencodebinary on PATH 做确定性回归测试（无 wall-clock 时序假设）：imports bundles SERIALLY：每个 import 用 per-name release file 做门控，测试驱动释放顺序并断言 max in-flight == 1。临时回退到旧的并行实现时，该测试失败（max in-flight == 4）—— 证明能抓住 bug。bootstrap waits for the listening line：fake serve 立即创建opencode.db但延迟 listening 行，断言 bootstrap 超时告警触发（证明等的是 listening 行而非 db 文件）。临时回退到 db-file 轮询时，该测试失败。returns early with empty map when no bundles：空输入快速路径。跑前两项都做了 temp-revert 验证：回退到 bug 行为 → 测试红；恢复修复 → 测试绿。
TMPDIR 沙箱化避免与
opencode-bundle套件的「tmpdir 里无 oac-bundle-*」断言互相污染。验证
bun run test：151/151 pass（含新增 3 例）bun run build：dist 重建成功，包含 listening-line 正则与串行 import 循环/listening on https?:\/\//i实测匹配本地opencode serve真实输出附带
tsconfig.json加"lib": ["ES2024"]（Promise.withResolvers在 ES2022 lib 里不存在；Node 22.22 runtime 原生支持，无需 polyfill）bun install顺带清掉 lockfile 里陈旧的better-sqlite3传递依赖（主package.json从未直接依赖它）非本 PR 范围
bun run check在 main HEAD 上已有 31 个 error（context-cache.tsnull-narrowing、vitest模块缺失、index.ts类型收窄），按约定放下一个 PR 处理。