From 076c767347ba4936a701e0ba0ac73b6127f30ae9 Mon Sep 17 00:00:00 2001 From: svtter Date: Sun, 28 Jun 2026 11:07:31 +0800 Subject: [PATCH 1/2] fix(multi-review): serial import + wait for serve ready line 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. --- multi-review/bun.lock | 77 ------- multi-review/dist/index.cjs | 60 ++++-- multi-review/package.json | 2 +- multi-review/src/session-resume.test.ts | 270 ++++++++++++++++++++++++ multi-review/src/session-resume.ts | 119 +++++++---- multi-review/tsconfig.json | 1 + 6 files changed, 387 insertions(+), 142 deletions(-) create mode 100644 multi-review/src/session-resume.test.ts diff --git a/multi-review/bun.lock b/multi-review/bun.lock index 37125f0..dc4e015 100644 --- a/multi-review/bun.lock +++ b/multi-review/bun.lock @@ -11,7 +11,6 @@ "devDependencies": { "@types/js-yaml": "^4.0.9", "@types/node": "^22.0.0", - "better-sqlite3": "^12.11.1", "tsup": "^8.0.0", "tsx": "^4.0.0", "typescript": "^5.7.0", @@ -143,24 +142,12 @@ "argparse": ["argparse@2.0.1", "", {}, "sha512-8+9WqebbFzpX9OR+Wa6O29asIogeRMzcGtAINdpMHHyAg10f05aSFVBbcEqGf/PXw1EjAZ+q2/bEBg3DvurK3Q=="], - "base64-js": ["base64-js@1.5.1", "", {}, "sha512-AKpaYlHn8t4SVbOHCy+b5+KKgvR4vrsD8vbvrbiQJps7fKDTkjkDry6ji0rUJjC0kzbNePLwzxq8iypo41qeWA=="], - - "better-sqlite3": ["better-sqlite3@12.11.1", "", { "dependencies": { "bindings": "^1.5.0", "prebuild-install": "^7.1.1" } }, "sha512-dq9AtApgg5PGFtBzPFSBl3HZQjHok5gaQCM6zh2Yk0aSmDCs1CbnVI8/HgASQkNKsWFpseIO9beg5xxpYhbIfA=="], - - "bindings": ["bindings@1.5.0", "", { "dependencies": { "file-uri-to-path": "1.0.0" } }, "sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ=="], - - "bl": ["bl@4.1.0", "", { "dependencies": { "buffer": "^5.5.0", "inherits": "^2.0.4", "readable-stream": "^3.4.0" } }, "sha512-1W07cM9gS6DcLperZfFSj+bWLtaPGSOHWhPiGzXmvVJbRLdG82sH/Kn8EtW1VqWVA54AKf2h5k5BbnIbwF3h6w=="], - - "buffer": ["buffer@5.7.1", "", { "dependencies": { "base64-js": "^1.3.1", "ieee754": "^1.1.13" } }, "sha512-EHcyIPBQ4BSGlvjB16k5KgAJ27CIsHY/2JBmCRReo48y9rQ3MaUzWX3KVlBa4U7MyX02HdVj0K7C3WaB3ju7FQ=="], - "bundle-require": ["bundle-require@5.1.0", "", { "dependencies": { "load-tsconfig": "^0.2.3" }, "peerDependencies": { "esbuild": ">=0.18" } }, "sha512-3WrrOuZiyaaZPWiEt4G3+IffISVC9HYlWueJEBWED4ZH4aIAC2PnkdnuRrR94M+w6yGWn4AglWtJtBI8YqvgoA=="], "cac": ["cac@6.7.14", "", {}, "sha512-b6Ilus+c3RrdDk+JhLKUAQfzzgLEPy6wcXqS7f/xe1EETvsDP6GORG7SFuOs6cID5YkqchW/LXZbX5bc8j7ZcQ=="], "chokidar": ["chokidar@4.0.3", "", { "dependencies": { "readdirp": "^4.0.1" } }, "sha512-Qgzu8kfBvo+cA4962jnP1KkS6Dop5NS6g7R5LFYJr4b8Ub94PPQXUksCw9PvXoeXPRRddRNC5C1JQUR2SMGtnA=="], - "chownr": ["chownr@1.1.4", "", {}, "sha512-jJ0bqzaylmJtVnNgzTeSOs8DPavpbYgEr/b0YL8/2GO3xJEhInFmhKMUnEJQjZumK7KXGFhUy89PrsJWlakBVg=="], - "commander": ["commander@4.1.1", "", {}, "sha512-NOKm8xhkzAjzFx8B2v5OAHT+u5pRQc2UCa2Vq9jYL/31o2wi9mxBA7LIFs3sV5VSC49z6pEhfbMULvShKj26WA=="], "confbox": ["confbox@0.1.8", "", {}, "sha512-RMtmw0iFkeR4YV+fUOSucriAQNb9g8zFR52MWCtl+cCZOFRNL6zeB395vPzFhEjjn4fMxXudmELnl/KF/WrK6w=="], @@ -171,36 +158,14 @@ "debug": ["debug@4.4.3", "", { "dependencies": { "ms": "^2.1.3" } }, "sha512-RGwwWnwQvkVfavKVt22FGLw+xYSdzARwm0ru6DhTVA3umU5hZc28V3kO4stgYryrTlLpuvgI9GiijltAjNbcqA=="], - "decompress-response": ["decompress-response@6.0.0", "", { "dependencies": { "mimic-response": "^3.1.0" } }, "sha512-aW35yZM6Bb/4oJlZncMH2LCoZtJXTRxES17vE3hoRiowU2kWHaJKFkSBDnDR+cm9J+9QhXmREyIfv0pji9ejCQ=="], - - "deep-extend": ["deep-extend@0.6.0", "", {}, "sha512-LOHxIOaPYdHlJRtCQfDIVZtfw/ufM8+rVj649RIHzcm/vGwQRXFt6OPqIFWsm2XEMrNIEtWR64sY1LEKD2vAOA=="], - - "detect-libc": ["detect-libc@2.1.2", "", {}, "sha512-Btj2BOOO83o3WyH59e8MgXsxEQVcarkUOpEYrubB0urwnN10yQ364rsiByU11nZlqWYZm05i/of7io4mzihBtQ=="], - - "end-of-stream": ["end-of-stream@1.4.5", "", { "dependencies": { "once": "^1.4.0" } }, "sha512-ooEGc6HP26xXq/N+GCGOT0JKCLDGrq2bQUZrQ7gyrJiZANJ/8YDTxTpQBXGMn+WbIQXNVpyWymm7KYVICQnyOg=="], - "esbuild": ["esbuild@0.27.7", "", { "optionalDependencies": { "@esbuild/aix-ppc64": "0.27.7", "@esbuild/android-arm": "0.27.7", "@esbuild/android-arm64": "0.27.7", "@esbuild/android-x64": "0.27.7", "@esbuild/darwin-arm64": "0.27.7", "@esbuild/darwin-x64": "0.27.7", "@esbuild/freebsd-arm64": "0.27.7", "@esbuild/freebsd-x64": "0.27.7", "@esbuild/linux-arm": "0.27.7", "@esbuild/linux-arm64": "0.27.7", "@esbuild/linux-ia32": "0.27.7", "@esbuild/linux-loong64": "0.27.7", "@esbuild/linux-mips64el": "0.27.7", "@esbuild/linux-ppc64": "0.27.7", "@esbuild/linux-riscv64": "0.27.7", "@esbuild/linux-s390x": "0.27.7", "@esbuild/linux-x64": "0.27.7", "@esbuild/netbsd-arm64": "0.27.7", "@esbuild/netbsd-x64": "0.27.7", "@esbuild/openbsd-arm64": "0.27.7", "@esbuild/openbsd-x64": "0.27.7", "@esbuild/openharmony-arm64": "0.27.7", "@esbuild/sunos-x64": "0.27.7", "@esbuild/win32-arm64": "0.27.7", "@esbuild/win32-ia32": "0.27.7", "@esbuild/win32-x64": "0.27.7" }, "bin": "bin/esbuild" }, "sha512-IxpibTjyVnmrIQo5aqNpCgoACA/dTKLTlhMHihVHhdkxKyPO1uBBthumT0rdHmcsk9uMonIWS0m4FljWzILh3w=="], - "expand-template": ["expand-template@2.0.3", "", {}, "sha512-XYfuKMvj4O35f/pOXLObndIRvyQ+/+6AhODh+OKWj9S9498pHHn/IMszH+gt0fBCRWMNfk1ZSp5x3AifmnI2vg=="], - "fdir": ["fdir@6.5.0", "", { "peerDependencies": { "picomatch": "^3 || ^4" } }, "sha512-tIbYtZbucOs0BRGqPJkshJUYdL+SDH7dVM8gjy+ERp3WAUjLEFJE+02kanyHtwjWOnwrKYBiwAmM0p4kLJAnXg=="], - "file-uri-to-path": ["file-uri-to-path@1.0.0", "", {}, "sha512-0Zt+s3L7Vf1biwWZ29aARiVYLx7iMGnEUl9x33fbB/j3jR81u/O2LbqK+Bm1CDSNDKVtJ/YjwY7TUd5SkeLQLw=="], - "fix-dts-default-cjs-exports": ["fix-dts-default-cjs-exports@1.0.1", "", { "dependencies": { "magic-string": "^0.30.17", "mlly": "^1.7.4", "rollup": "^4.34.8" } }, "sha512-pVIECanWFC61Hzl2+oOCtoJ3F17kglZC/6N94eRWycFgBH35hHx0Li604ZIzhseh97mf2p0cv7vVrOZGoqhlEg=="], - "fs-constants": ["fs-constants@1.0.0", "", {}, "sha512-y6OAwoSIf7FyjMIv94u+b5rdheZEjzR63GTyZJm5qh4Bi+2YgwLCcI/fPFZkL5PSixOt6ZNKm+w+Hfp/Bciwow=="], - "fsevents": ["fsevents@2.3.3", "", { "os": "darwin" }, "sha512-5xoDfX+fL7faATnagmWPpbFtwh/R77WmMMqqHGS65C3vvB0YHrgF+B1YmZ3441tMj5n63k0212XNoJwzlhffQw=="], - "github-from-package": ["github-from-package@0.0.0", "", {}, "sha512-SyHy3T1v2NUXn29OsWdxmK6RwHD+vkj3v8en8AOBZ1wBQ/hCAQ5bAQTD02kW4W9tUp/3Qh6J8r9EvntiyCmOOw=="], - - "ieee754": ["ieee754@1.2.1", "", {}, "sha512-dcyqhDvX1C46lXZcVqCpK+FtMRQVdIMN6/Df5js2zouUsqG7I6sFxitIC+7KYK29KdXOLHdu9zL4sFnoVQnqaA=="], - - "inherits": ["inherits@2.0.4", "", {}, "sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ=="], - - "ini": ["ini@1.3.8", "", {}, "sha512-JV/yugV2uzW5iMRSiZAyDtQd+nxtUnjeLt0acNdw98kKLrvuRVyB80tsREOE7yvGVgalhZ6RNXCmEHkUKBKxew=="], - "isexe": ["isexe@2.0.0", "", {}, "sha512-RHxMLp9lnKHGHRng9QFhRCMbYAcVpn69smSGcq3f36xjgVVWThj4qqLbTLlq7Ssj8B+fIQ1EuCEGI2lKsyQeIw=="], "joycon": ["joycon@3.1.1", "", {}, "sha512-34wB/Y7MW7bzjKRjUKTa46I2Z7eV62Rkhva+KkopW7Qvv/OSWBqvkSY7vusOPrNuZcUG3tApvdVgNB8POj3SPw=="], @@ -215,26 +180,14 @@ "magic-string": ["magic-string@0.30.21", "", { "dependencies": { "@jridgewell/sourcemap-codec": "^1.5.5" } }, "sha512-vd2F4YUyEXKGcLHoq+TEyCjxueSeHnFxyyjNp80yg0XV4vUhnDer/lvvlqM/arB5bXQN5K2/3oinyCRyx8T2CQ=="], - "mimic-response": ["mimic-response@3.1.0", "", {}, "sha512-z0yWI+4FDrrweS8Zmt4Ej5HdJmky15+L2e6Wgn3+iK5fWzb6T3fhNFq2+MeTRb064c6Wr4N/wv0DzQTjNzHNGQ=="], - - "minimist": ["minimist@1.2.8", "", {}, "sha512-2yyAR8qBkN3YuheJanUpWC5U3bb5osDywNB8RzDVlDwDHbocAJveqqj1u8+SVD7jkWT4yvsHCpWqqWqAxb0zCA=="], - - "mkdirp-classic": ["mkdirp-classic@0.5.3", "", {}, "sha512-gKLcREMhtuZRwRAfqP3RFW+TK4JqApVBtOIftVgjuABpAtpxhPGaDcfvbhNvD0B8iD1oUr/txX35NjcaY6Ns/A=="], - "mlly": ["mlly@1.8.2", "", { "dependencies": { "acorn": "^8.16.0", "pathe": "^2.0.3", "pkg-types": "^1.3.1", "ufo": "^1.6.3" } }, "sha512-d+ObxMQFmbt10sretNDytwt85VrbkhhUA/JBGm1MPaWJ65Cl4wOgLaB1NYvJSZ0Ef03MMEU/0xpPMXUIQ29UfA=="], "ms": ["ms@2.1.3", "", {}, "sha512-6FlzubTLZG3J2a/NVCAleEhjzq5oxgHyaCU9yYXvcLsvoVaHJq/s5xXI6/XXP6tz7R9xAOtHnSO/tXtF3WRTlA=="], "mz": ["mz@2.7.0", "", { "dependencies": { "any-promise": "^1.0.0", "object-assign": "^4.0.1", "thenify-all": "^1.0.0" } }, "sha512-z81GNO7nnYMEhrGh9LeymoE4+Yr0Wn5McHIZMK5cfQCl+NDX08sCZgUc9/6MHni9IWuFLm1Z3HTCXu2z9fN62Q=="], - "napi-build-utils": ["napi-build-utils@2.0.0", "", {}, "sha512-GEbrYkbfF7MoNaoh2iGG84Mnf/WZfB0GdGEsM8wz7Expx/LlWf5U8t9nvJKXSp3qr5IsEbK04cBGhol/KwOsWA=="], - - "node-abi": ["node-abi@3.92.0", "", { "dependencies": { "semver": "^7.3.5" } }, "sha512-KdHvFWZjEKDf0cakgFjebl371GPsISX2oZHcuyKqM7DtogIsHrqKeLTo8wBHxaXRAQlY2PsPlZmfo+9ZCxEREQ=="], - "object-assign": ["object-assign@4.1.1", "", {}, "sha512-rJgTQnkUnH1sFw8yT6VSU3zD3sWmu6sZhIseY8VX+GRu3P6F7Fu+JNDoXfklElbLJSnc3FUQHVe4cU5hj+BcUg=="], - "once": ["once@1.4.0", "", { "dependencies": { "wrappy": "1" } }, "sha512-lNaJgI+2Q5URQBkccEKHTQOPaXdUxnZZElQTZY0MFUAuaEqe1E+Nyvgdz/aIyNi6Z9MzO5dv1H8n58/GELp3+w=="], - "path-key": ["path-key@3.1.1", "", {}, "sha512-ojmeN0qd+y0jszEtoY48r0Peq5dwMEkIlCOu6Q5f41lfkswXuKtYrhgoTpLnyIcHm24Uhqx+5Tqm2InSwLhE6Q=="], "pathe": ["pathe@2.0.3", "", {}, "sha512-WUjGcAqP1gQacoQe+OBJsFA7Ld4DyXuUIjZ5cc75cLHvJ7dtNsTugphxIADwspS+AraAUePCKrSVtPLFj/F88w=="], @@ -249,44 +202,20 @@ "postcss-load-config": ["postcss-load-config@6.0.1", "", { "dependencies": { "lilconfig": "^3.1.1" }, "peerDependencies": { "jiti": ">=1.21.0", "postcss": ">=8.0.9", "tsx": "^4.8.1", "yaml": "^2.4.2" }, "optionalPeers": ["jiti", "postcss", "yaml"] }, "sha512-oPtTM4oerL+UXmx+93ytZVN82RrlY/wPUV8IeDxFrzIjXOLF1pN+EmKPLbubvKHT2HC20xXsCAH2Z+CKV6Oz/g=="], - "prebuild-install": ["prebuild-install@7.1.3", "", { "dependencies": { "detect-libc": "^2.0.0", "expand-template": "^2.0.3", "github-from-package": "0.0.0", "minimist": "^1.2.3", "mkdirp-classic": "^0.5.3", "napi-build-utils": "^2.0.0", "node-abi": "^3.3.0", "pump": "^3.0.0", "rc": "^1.2.7", "simple-get": "^4.0.0", "tar-fs": "^2.0.0", "tunnel-agent": "^0.6.0" }, "bin": { "prebuild-install": "bin.js" } }, "sha512-8Mf2cbV7x1cXPUILADGI3wuhfqWvtiLA1iclTDbFRZkgRQS0NqsPZphna9V+HyTEadheuPmjaJMsbzKQFOzLug=="], - - "pump": ["pump@3.0.4", "", { "dependencies": { "end-of-stream": "^1.1.0", "once": "^1.3.1" } }, "sha512-VS7sjc6KR7e1ukRFhQSY5LM2uBWAUPiOPa/A3mkKmiMwSmRFUITt0xuj+/lesgnCv+dPIEYlkzrcyXgquIHMcA=="], - - "rc": ["rc@1.2.8", "", { "dependencies": { "deep-extend": "^0.6.0", "ini": "~1.3.0", "minimist": "^1.2.0", "strip-json-comments": "~2.0.1" }, "bin": { "rc": "./cli.js" } }, "sha512-y3bGgqKj3QBdxLbLkomlohkvsA8gdAiUQlSBJnBhfn+BPxg4bc62d8TcBW15wavDfgexCgccckhcZvywyQYPOw=="], - - "readable-stream": ["readable-stream@3.6.2", "", { "dependencies": { "inherits": "^2.0.3", "string_decoder": "^1.1.1", "util-deprecate": "^1.0.1" } }, "sha512-9u/sniCrY3D5WdsERHzHE4G2YCXqoG5FTHUiCC4SIbr6XcLZBY05ya9EKjYek9O5xOAwjGq+1JdGBAS7Q9ScoA=="], - "readdirp": ["readdirp@4.1.2", "", {}, "sha512-GDhwkLfywWL2s6vEjyhri+eXmfH6j1L7JE27WhqLeYzoh/A3DBaYGEj2H/HFZCn/kMfim73FXxEJTw06WtxQwg=="], "resolve-from": ["resolve-from@5.0.0", "", {}, "sha512-qYg9KP24dD5qka9J47d0aVky0N+b4fTU89LN9iDnjB5waksiC49rvMB0PrUJQGoTmH50XPiqOvAjDfaijGxYZw=="], "rollup": ["rollup@4.61.0", "", { "dependencies": { "@types/estree": "1.0.9" }, "optionalDependencies": { "@rollup/rollup-android-arm-eabi": "4.61.0", "@rollup/rollup-android-arm64": "4.61.0", "@rollup/rollup-darwin-arm64": "4.61.0", "@rollup/rollup-darwin-x64": "4.61.0", "@rollup/rollup-freebsd-arm64": "4.61.0", "@rollup/rollup-freebsd-x64": "4.61.0", "@rollup/rollup-linux-arm-gnueabihf": "4.61.0", "@rollup/rollup-linux-arm-musleabihf": "4.61.0", "@rollup/rollup-linux-arm64-gnu": "4.61.0", "@rollup/rollup-linux-arm64-musl": "4.61.0", "@rollup/rollup-linux-loong64-gnu": "4.61.0", "@rollup/rollup-linux-loong64-musl": "4.61.0", "@rollup/rollup-linux-ppc64-gnu": "4.61.0", "@rollup/rollup-linux-ppc64-musl": "4.61.0", "@rollup/rollup-linux-riscv64-gnu": "4.61.0", "@rollup/rollup-linux-riscv64-musl": "4.61.0", "@rollup/rollup-linux-s390x-gnu": "4.61.0", "@rollup/rollup-linux-x64-gnu": "4.61.0", "@rollup/rollup-linux-x64-musl": "4.61.0", "@rollup/rollup-openbsd-x64": "4.61.0", "@rollup/rollup-openharmony-arm64": "4.61.0", "@rollup/rollup-win32-arm64-msvc": "4.61.0", "@rollup/rollup-win32-ia32-msvc": "4.61.0", "@rollup/rollup-win32-x64-gnu": "4.61.0", "@rollup/rollup-win32-x64-msvc": "4.61.0", "fsevents": "~2.3.2" }, "bin": "dist/bin/rollup" }, "sha512-T9mWdbWfQtp0B5lv/HX+wrhYsmXRlcWnXXmJbXqKJhlRaoS6KMhq0gpyzW4UJfclcxrEdLnTgjT2NjruLONu0g=="], - "safe-buffer": ["safe-buffer@5.2.1", "", {}, "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ=="], - - "semver": ["semver@7.8.5", "", { "bin": { "semver": "bin/semver.js" } }, "sha512-Y7/KDsb8LjooZpwaqGyulO6DQlksgCncchHGk+sZIY4SBvUocMBEFH5Ur1fI4dV+Jvl0w6cjvucaIi40puRioA=="], - "shebang-command": ["shebang-command@2.0.0", "", { "dependencies": { "shebang-regex": "^3.0.0" } }, "sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA=="], "shebang-regex": ["shebang-regex@3.0.0", "", {}, "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A=="], - "simple-concat": ["simple-concat@1.0.1", "", {}, "sha512-cSFtAPtRhljv69IK0hTVZQ+OfE9nePi/rtJmw5UjHeVyVroEqJXP1sFztKUy1qU+xvz3u/sfYJLa947b7nAN2Q=="], - - "simple-get": ["simple-get@4.0.1", "", { "dependencies": { "decompress-response": "^6.0.0", "once": "^1.3.1", "simple-concat": "^1.0.0" } }, "sha512-brv7p5WgH0jmQJr1ZDDfKDOSeWWg+OVypG99A/5vYGPqJ6pxiaHLy8nxtFjBA7oMa01ebA9gfh1uMCFqOuXxvA=="], - "source-map": ["source-map@0.7.6", "", {}, "sha512-i5uvt8C3ikiWeNZSVZNWcfZPItFQOsYTUAOkcUPGd8DqDy1uOUikjt5dG+uRlwyvR108Fb9DOd4GvXfT0N2/uQ=="], - "string_decoder": ["string_decoder@1.3.0", "", { "dependencies": { "safe-buffer": "~5.2.0" } }, "sha512-hkRX8U1WjJFd8LsDJ2yQ/wWWxaopEsABU1XfkM8A+j0+85JAGppt16cr1Whg6KIbb4okU6Mql6BOj+uup/wKeA=="], - - "strip-json-comments": ["strip-json-comments@2.0.1", "", {}, "sha512-4gB8na07fecVVkOI6Rs4e7T6NOTki5EmL7TUduTs6bu3EdnSycntVJ4re8kgZA+wx9IueI2Y11bfbgwtzuE0KQ=="], - "sucrase": ["sucrase@3.35.1", "", { "dependencies": { "@jridgewell/gen-mapping": "^0.3.2", "commander": "^4.0.0", "lines-and-columns": "^1.1.6", "mz": "^2.7.0", "pirates": "^4.0.1", "tinyglobby": "^0.2.11", "ts-interface-checker": "^0.1.9" }, "bin": { "sucrase": "bin/sucrase", "sucrase-node": "bin/sucrase-node" } }, "sha512-DhuTmvZWux4H1UOnWMB3sk0sbaCVOoQZjv8u1rDoTV0HTdGem9hkAZtl4JZy8P2z4Bg0nT+YMeOFyVr4zcG5Tw=="], - "tar-fs": ["tar-fs@2.1.4", "", { "dependencies": { "chownr": "^1.1.1", "mkdirp-classic": "^0.5.2", "pump": "^3.0.0", "tar-stream": "^2.1.4" } }, "sha512-mDAjwmZdh7LTT6pNleZ05Yt65HC3E+NiQzl672vQG38jIrehtJk/J3mNwIg+vShQPcLF/LV7CMnDW6vjj6sfYQ=="], - - "tar-stream": ["tar-stream@2.2.0", "", { "dependencies": { "bl": "^4.0.3", "end-of-stream": "^1.4.1", "fs-constants": "^1.0.0", "inherits": "^2.0.3", "readable-stream": "^3.1.1" } }, "sha512-ujeqbceABgwMZxEJnk2HDY2DlnUZ+9oEcb1KzTVfYHio0UE6dG71n60d8D2I4qNvleWrrXpmjpt7vZeF1LnMZQ=="], - "thenify": ["thenify@3.3.1", "", { "dependencies": { "any-promise": "^1.0.0" } }, "sha512-RVZSIV5IG10Hk3enotrhvz0T9em6cyHBLkH/YAZuKqd8hRkKhSfCGIcP2KUY0EPxndzANBmNllzWPwak+bheSw=="], "thenify-all": ["thenify-all@1.6.0", "", { "dependencies": { "thenify": ">= 3.1.0 < 4" } }, "sha512-RNxQH/qI8/t3thXJDwcstUO4zeqo64+Uy/+sNVRBx4Xn2OX+OZ9oP+iJnNFqplFra2ZUVeKCSa2oVWi3T4uVmA=="], @@ -303,20 +232,14 @@ "tsx": ["tsx@4.22.4", "", { "dependencies": { "esbuild": "~0.28.0" }, "optionalDependencies": { "fsevents": "~2.3.3" }, "bin": "dist/cli.mjs" }, "sha512-X8EX+XV4QR5xCsrgxaED954zTDfY8KqlDtskKEL0cHhyS/P8b4IFOvGDQpsC9Q1XnLq915wEfwwY/zzskCtmhg=="], - "tunnel-agent": ["tunnel-agent@0.6.0", "", { "dependencies": { "safe-buffer": "^5.0.1" } }, "sha512-McnNiV1l8RYeY8tBgEpuodCC1mLUdbSN+CYBL7kJsJNInOP8UjDDEwdk6Mw60vdLLrr5NHKZhMAOSrR2NZuQ+w=="], - "typescript": ["typescript@5.9.3", "", { "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" } }, "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw=="], "ufo": ["ufo@1.6.4", "", {}, "sha512-JFNbkD1Svwe0KvGi8GOeLcP4kAWQ609twvCdcHxq1oSL8svv39ZuSvajcD8B+5D0eL4+s1Is2D/O6KN3qcTeRA=="], "undici-types": ["undici-types@6.21.0", "", {}, "sha512-iwDZqg0QAGrg9Rav5H4n0M64c3mkR59cJ6wQp+7C4nI0gsmExaedaYLNO44eT4AtBBwjbTiGPMlt2Md0T9H9JQ=="], - "util-deprecate": ["util-deprecate@1.0.2", "", {}, "sha512-EPD5q1uXyFxJpCrLnCc1nHnq3gOa6DZBocAIiI2TaSCA7VCJ1UJDMagCzIkXNsUYfD1daK//LTEQ8xiIbrHtcw=="], - "which": ["which@2.0.2", "", { "dependencies": { "isexe": "^2.0.0" }, "bin": { "node-which": "bin/node-which" } }, "sha512-BLI3Tl1TW3Pvl70l3yq3Y64i+awpwXqsGBYWkkqMtnbXgrMD+yj7rhW0kuEDxzJaYXGjEW5ogapKNMEKNMjibA=="], - "wrappy": ["wrappy@1.0.2", "", {}, "sha512-l4Sp/DRseor9wL6EvV2+TuQn63dMkPjZ/sp9XkghTEbV9KlPS1xUsZ3u7/IQO4wxtcFB4bgpQPRcR3QCvezPcQ=="], - "tsx/esbuild": ["esbuild@0.28.0", "", { "optionalDependencies": { "@esbuild/aix-ppc64": "0.28.0", "@esbuild/android-arm": "0.28.0", "@esbuild/android-arm64": "0.28.0", "@esbuild/android-x64": "0.28.0", "@esbuild/darwin-arm64": "0.28.0", "@esbuild/darwin-x64": "0.28.0", "@esbuild/freebsd-arm64": "0.28.0", "@esbuild/freebsd-x64": "0.28.0", "@esbuild/linux-arm": "0.28.0", "@esbuild/linux-arm64": "0.28.0", "@esbuild/linux-ia32": "0.28.0", "@esbuild/linux-loong64": "0.28.0", "@esbuild/linux-mips64el": "0.28.0", "@esbuild/linux-ppc64": "0.28.0", "@esbuild/linux-riscv64": "0.28.0", "@esbuild/linux-s390x": "0.28.0", "@esbuild/linux-x64": "0.28.0", "@esbuild/netbsd-arm64": "0.28.0", "@esbuild/netbsd-x64": "0.28.0", "@esbuild/openbsd-arm64": "0.28.0", "@esbuild/openbsd-x64": "0.28.0", "@esbuild/openharmony-arm64": "0.28.0", "@esbuild/sunos-x64": "0.28.0", "@esbuild/win32-arm64": "0.28.0", "@esbuild/win32-ia32": "0.28.0", "@esbuild/win32-x64": "0.28.0" }, "bin": "bin/esbuild" }, "sha512-sNR9MHpXSUV/XB4zmsFKN+QgVG82Cc7+/aaxJ8Adi8hyOac+EXptIp45QBPaVyX3N70664wRbTcLTOemCAnyqw=="], "tsx/esbuild/@esbuild/aix-ppc64": ["@esbuild/aix-ppc64@0.28.0", "", { "os": "aix", "cpu": "ppc64" }, "sha512-lhRUCeuOyJQURhTxl4WkpFTjIsbDayJHih5kZC1giwE+MhIzAb7mEsQMqMf18rHLsrb5qI1tafG20mLxEWcWlA=="], diff --git a/multi-review/dist/index.cjs b/multi-review/dist/index.cjs index ce540be..760008b 100644 --- a/multi-review/dist/index.cjs +++ b/multi-review/dist/index.cjs @@ -6280,8 +6280,23 @@ async function restoreSessionBundles(reviewBundles, options) { env: { ...process.env, XDG_DATA_HOME: tempDataHome }, stdio: ["ignore", "pipe", "pipe"] }); - bsProc.on("error", (err) => { - console.warn(`v2 resume: opencode serve spawn failed: ${err.message}`); + const { promise: readyPromise, resolve: readyResolve, reject: readyReject } = Promise.withResolvers(); + let serveOutput = ""; + const onServeData = (chunk) => { + serveOutput += typeof chunk === "string" ? chunk : chunk.toString("utf8"); + if (/listening on https?:\/\//i.test(serveOutput)) { + readyResolve(); + } + }; + bsProc.stdout?.on("data", onServeData); + bsProc.stderr?.on("data", onServeData); + bsProc.on("error", (err) => readyReject(err)); + bsProc.on("exit", (code) => { + if (code !== null) { + readyReject(new Error( + `opencode serve exited ${code} before schema was ready${serveOutput ? `: ${serveOutput.trim().slice(0, 500)}` : ""}` + )); + } }); const bootstrapAbort = new AbortController(); const killBootstrap = () => { @@ -6290,33 +6305,32 @@ async function restoreSessionBundles(reviewBundles, options) { bootstrapAbort.signal.addEventListener("abort", killBootstrap); const bootstrapTimeoutMs = options.bootstrapTimeoutMs ?? 1e4; const bootstrapStartedAt = Date.now(); - const bootstrapDeadline = bootstrapStartedAt + bootstrapTimeoutMs; - while (Date.now() < bootstrapDeadline) { - if ((0, import_node_fs5.existsSync)(dbPath)) break; - await new Promise((r) => setTimeout(r, 100)); - } - bootstrapAbort.abort(); - if (!(0, import_node_fs5.existsSync)(dbPath)) { - console.warn(`v2 resume: schema bootstrap timed out after ${bootstrapTimeoutMs}ms; bundles may fail to import`); - } else { + const timer = setTimeout( + () => readyReject(new Error(`schema bootstrap timed out after ${bootstrapTimeoutMs}ms`)), + bootstrapTimeoutMs + ); + try { + await readyPromise; console.log(`v2 resume: schema bootstrapped in ${Date.now() - bootstrapStartedAt}ms`); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + const haveDb = (0, import_node_fs5.existsSync)(dbPath) ? "db file exists" : "no db file"; + console.warn(`v2 resume: ${reason} (${haveDb}); bundles may fail to import`); + } finally { + clearTimeout(timer); + bootstrapAbort.abort(); } - const importResults = await Promise.allSettled( - reviewBundles.bundles.map(async (b) => { + const existingSessions = /* @__PURE__ */ new Map(); + for (const b of reviewBundles.bundles) { + try { const importedId = await importBundle(b.bundle, { ...process.env, XDG_DATA_HOME: tempDataHome }); - return { name: b.name, sessionID: importedId }; - }) - ); - const existingSessions = /* @__PURE__ */ new Map(); - for (const r of importResults) { - if (r.status === "fulfilled") { - existingSessions.set(r.value.name, r.value.sessionID); - console.log(`v2 resume: imported bundle for "${r.value.name}" \u2192 ${r.value.sessionID}`); - } else { - const reason = r.reason instanceof Error ? r.reason.message : String(r.reason); + existingSessions.set(b.name, importedId); + console.log(`v2 resume: imported bundle for "${b.name}" \u2192 ${importedId}`); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); console.warn(`v2 resume: failed to import bundle: ${reason}`); } } diff --git a/multi-review/package.json b/multi-review/package.json index d8dc474..1f17277 100644 --- a/multi-review/package.json +++ b/multi-review/package.json @@ -6,7 +6,7 @@ "scripts": { "build": "tsup", "check": "tsc --noEmit", - "test": "node --import tsx --test src/platform.test.ts src/reviewers.test.ts src/custom-reviewers.test.ts src/diff-filter.test.ts src/context-cache.test.ts src/opencode-bundle.test.ts src/orchestrator.test.ts src/severity-parser.test.ts" + "test": "node --import tsx --test src/platform.test.ts src/reviewers.test.ts src/custom-reviewers.test.ts src/diff-filter.test.ts src/context-cache.test.ts src/opencode-bundle.test.ts src/session-resume.test.ts src/orchestrator.test.ts src/severity-parser.test.ts" }, "dependencies": { "@opencode-ai/sdk": "^1.0.0", diff --git a/multi-review/src/session-resume.test.ts b/multi-review/src/session-resume.test.ts new file mode 100644 index 0000000..6dfdd9b --- /dev/null +++ b/multi-review/src/session-resume.test.ts @@ -0,0 +1,270 @@ +import { describe, it, before, after } from "node:test"; +import assert from "node:assert"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { restoreSessionBundles, type ResumeContext } from "./session-resume.js"; +import type { ReviewContextV2, SessionBundle } from "./types.js"; + +/** + * Tests for session-resume.ts. + * + * These lock down the two fixes for the "v2 resume: 2/7 bundles restored" + * failure observed on PR sun-praise/latex-agent#4165: + * + * 1. Bootstrap must wait for `opencode serve`'s `listening on http://` + * line (emitted AFTER migrations commit), NOT for the `opencode.db` + * file to appear — SQLite creates the file before migrations finish. + * 2. Bundles must be imported SERIALLY. Parallel `opencode import` + * against one SQLite DB races the non-idempotent migration DDL. + * + * The real `opencode` CLI is faked by a Node script on PATH. The fake + * gates each import on a per-name release file so the test can detect + * serial-vs-parallel execution DETERMINISTICALLY — no wall-clock timing + * assumption. (Serve still uses a real delay; that is the documented + * integration-test exception: fake timers cannot drive a child + * process's own clock, and the assertion checks which branch ran, not + * a duration.) + */ + +/** + * Install a fake `opencode` on PATH that branches on the subcommand. + * + * - `serve`: creates `$XDG_DATA_HOME/opencode/opencode.db` immediately + * (mimicking SQLite creating the file pre-migration), then — after + * `OAC_TEST_SERVE_DELAY_MS` — prints the listening line and stays + * alive until killed. + * - `import `: writes `/.started`, then blocks until + * `/.release` exists (deterministic handshake so the test + * controls completion order), then writes `/.done` and + * prints the `Imported session:` contract line. If no gate dir is set + * the import completes immediately. + */ +function installFakeOpencode(): { binDir: string; restore: () => void } { + const binDir = mkdtempSync(join(tmpdir(), "opencode-fake-")); + const binPath = join(binDir, "opencode"); + const script = `#!/usr/bin/env node +const fs = require("fs"); +const path = require("path"); +const cmd = process.argv[2]; +const waitFile = (p, capMs) => { + const deadline = Date.now() + capMs; + while (!fs.existsSync(p)) { + if (Date.now() > deadline) { process.stderr.write("gate timeout: " + p + "\\n"); process.exit(1); } + } +}; +if (cmd === "serve") { + const xdg = process.env.XDG_DATA_HOME; + if (xdg) { + fs.mkdirSync(path.join(xdg, "opencode"), { recursive: true }); + // SQLite creates the db file BEFORE migrations commit — the bug was + // treating this file's existence as the ready signal. + fs.writeFileSync(path.join(xdg, "opencode", "opencode.db"), ""); + } + const delay = Number(process.env.OAC_TEST_SERVE_DELAY_MS || "0"); + setTimeout(() => { + process.stdout.write("opencode server listening on http://127.0.0.1:0\\n"); + }, delay); + // Stay alive like a real server until SIGTERM. + setInterval(() => {}, 1 << 30); +} else if (cmd === "import") { + const file = process.argv[3]; + // Key the marker/gate by the session id parsed from the bundle JSON + // (info.id), NOT by the bundle file's basename — importBundle always + // writes the bundle to "/bundle.json", so basename collisions + // would make all concurrent imports share one marker. + let id = "unknown"; + try { id = JSON.parse(fs.readFileSync(file, "utf8")).info.id || id; } catch (e) {} + const key = id.replace(/[^A-Za-z0-9_.-]/g, "_"); + const markerDir = process.env.OAC_TEST_MARKER_DIR; + const gateDir = process.env.OAC_TEST_IMPORT_GATE_DIR; + if (markerDir) fs.writeFileSync(path.join(markerDir, key + ".started"), ""); + if (gateDir) waitFile(path.join(gateDir, key + ".release"), 15000); + if (markerDir) fs.writeFileSync(path.join(markerDir, key + ".done"), ""); + process.stdout.write("Imported session: " + id + "\\n"); +} else { + process.stdout.write("{}"); +} +`; + writeFileSync(binPath, script); + chmodSync(binPath, 0o755); + + const originalPath = process.env.PATH; + process.env.PATH = `${binDir}:${originalPath ?? ""}`; + + return { + binDir, + restore: () => { + process.env.PATH = originalPath; + rmSync(binDir, { recursive: true, force: true }); + }, + }; +} + +function makeBundles(names: string[]): ReviewContextV2 { + const bundles: SessionBundle[] = names.map((name) => ({ + name, + sessionID: `ses_src_${name}`, + // importBundle only writes the JSON to a temp file and passes the + // path to the fake; the fake never reads the content, so a minimal + // object is enough. savedAt is required by the type. + bundle: { info: { id: `ses_src_${name}` }, messages: [] }, + savedAt: new Date().toISOString(), + })); + return { version: 2, repo: "owner/repo", prNumber: "123", savedAt: "now", bundles }; +} + +/** Bases that have a `.started` marker but not yet a `.done` marker. */ +function inFlightBases(markerDir: string): string[] { + const files = new Set(readdirSync(markerDir)); + const inFlight: string[] = []; + for (const f of files) { + if (!f.endsWith(".started")) continue; + const base = f.slice(0, -".started".length); + if (!files.has(`${base}.done`)) inFlight.push(base); + } + return inFlight; +} + +/** Short deterministic wait for cross-process file signals. Integration- + * test exception to fake timers: the marker files are written by a + * separate spawned `opencode` process whose clock we cannot drive from + * the test. 15ms is the smallest gap that reliably lets the child's + * fs.writeFileSync flush land. */ +const pollTick = (): Promise => new Promise((r) => setTimeout(r, 15)); + +describe("session-resume", { concurrency: false }, () => { + let savedXdg: string | undefined; + let savedTmpdir: string | undefined; + // Sandbox TMPDIR so importBundle's mkdtempSync(tmpdir()) lands inside a + // per-suite dir instead of polluting the shared /tmp. Without this, the + // oac-bundle-* dirs this suite creates (and cleans up) race the + // opencode-bundle suite's "no oac-bundle-* in tmpdir()" assertion when + // both run in one node --test process. + let sandboxTmp: string | undefined; + + before(() => { + savedXdg = process.env.XDG_DATA_HOME; + savedTmpdir = process.env.TMPDIR; + sandboxTmp = mkdtempSync(join(tmpdir(), "oac-resume-sandbox-")); + process.env.TMPDIR = sandboxTmp; + }); + + after(() => { + if (savedXdg === undefined) delete process.env.XDG_DATA_HOME; + else process.env.XDG_DATA_HOME = savedXdg; + if (savedTmpdir === undefined) delete process.env.TMPDIR; + else process.env.TMPDIR = savedTmpdir; + if (sandboxTmp) rmSync(sandboxTmp, { recursive: true, force: true }); + }); + + it("imports bundles SERIALLY (max in-flight == 1, not N)", async () => { + // Regression for sun-praise/latex-agent#4165: parallel imports raced + // migration DDL and most failed. Each fake import blocks on a + // per-name release file, so under SERIAL execution only ONE import + // is ever in-flight (started-but-not-done) at a time; under the old + // PARALLEL impl all N launch at once and all are in-flight + // simultaneously. Deterministic — no timing assumption. + const fake = installFakeOpencode(); + const markerDir = mkdtempSync(join(tmpdir(), "oac-resume-markers-")); + const gateDir = mkdtempSync(join(tmpdir(), "oac-resume-gate-")); + process.env.OAC_TEST_MARKER_DIR = markerDir; + process.env.OAC_TEST_IMPORT_GATE_DIR = gateDir; + process.env.OAC_TEST_SERVE_DELAY_MS = "10"; + const names = ["quality", "security", "performance", "architecture"]; + let maxInFlight = 0; + let ctx: ResumeContext | undefined; + try { + const promise = restoreSessionBundles(makeBundles(names), { + baseTempDir: tmpdir(), + prLabel: "test-serial", + }); + + // Orchestrate: repeatedly observe in-flight imports, release one, + // wait for it to finish. Track the high-water mark of concurrent + // in-flight imports — must be 1 (serial), never N (parallel bug). + let released = 0; + while (released < names.length) { + let inflight = inFlightBases(markerDir); + // Wait for at least one import to have started. + let spins = 0; + while (inflight.length === 0) { + if (++spins > 2000) throw new Error("timed out waiting for an import to start"); + await pollTick(); + inflight = inFlightBases(markerDir); + } + if (inflight.length > maxInFlight) maxInFlight = inflight.length; + // Release exactly one in-flight import. + const target = inflight[0]; + writeFileSync(join(gateDir, `${target}.release`), ""); + released++; + // Wait for it to finish before observing again. + spins = 0; + while (!existsSync(join(markerDir, `${target}.done`))) { + if (++spins > 2000) throw new Error(`timed out waiting for ${target}.done`); + await pollTick(); + } + } + + ctx = await promise; + + assert.strictEqual( + maxInFlight, + 1, + `imports must run serially (max in-flight 1); under the parallel race this was ${names.length}. Observed max in-flight: ${maxInFlight}`, + ); + assert.strictEqual(ctx.existingSessions.size, names.length); + for (const name of names) { + assert.ok(ctx.existingSessions.has(name), `missing restored session for ${name}`); + } + } finally { + fake.restore(); + delete process.env.OAC_TEST_MARKER_DIR; + delete process.env.OAC_TEST_IMPORT_GATE_DIR; + delete process.env.OAC_TEST_SERVE_DELAY_MS; + rmSync(markerDir, { recursive: true, force: true }); + rmSync(gateDir, { recursive: true, force: true }); + if (ctx?.tempDataHome) rmSync(ctx.tempDataHome, { recursive: true, force: true }); + } + }); + + it("bootstrap waits for the listening line, NOT for the db file to appear", async () => { + // The fake serve creates opencode.db INSTANTLY but delays the + // listening line beyond the bootstrap cap. A db-file poller would + // "succeed" immediately; the listening-line waiter correctly times + // out. Asserting the timeout warning fired proves we waited for the + // real signal and did NOT short-circuit on the pre-migration db + // file. Deterministic branch check, not a timing assertion. + const fake = installFakeOpencode(); + const warns: string[] = []; + const origWarn = console.warn; + console.warn = (...args: unknown[]) => { warns.push(args.map(String).join(" ")); }; + let ctx: Awaited> | undefined; + try { + process.env.OAC_TEST_SERVE_DELAY_MS = "500"; + // No IMPORT_GATE_DIR → imports run instantly via the fallback + // path after bootstrap times out, so restore returns promptly. + ctx = await restoreSessionBundles(makeBundles(["quality"]), { + baseTempDir: tmpdir(), + prLabel: "test-listening", + bootstrapTimeoutMs: 150, + }); + assert.ok( + warns.some((w) => /schema bootstrap timed out after 150ms/.test(w)), + `expected bootstrap timeout warning (proves we waited for the listening line, not the db file); got: ${warns.join(" | ")}`, + ); + assert.ok(ctx.tempDataHome, "tempDataHome must still be returned for caller cleanup"); + } finally { + console.warn = origWarn; + fake.restore(); + delete process.env.OAC_TEST_SERVE_DELAY_MS; + if (ctx?.tempDataHome) rmSync(ctx.tempDataHome, { recursive: true, force: true }); + } + }); + + it("returns early with empty map when there are no bundles", async () => { + const ctx = await restoreSessionBundles(null, { baseTempDir: tmpdir(), prLabel: "empty" }); + assert.strictEqual(ctx.existingSessions.size, 0); + assert.strictEqual(ctx.tempDataHome, null); + }); +}); diff --git a/multi-review/src/session-resume.ts b/multi-review/src/session-resume.ts index f0f6ff1..2a690ae 100644 --- a/multi-review/src/session-resume.ts +++ b/multi-review/src/session-resume.ts @@ -35,11 +35,13 @@ export interface ResumeOptions { * * 1. Allocate a temp XDG_DATA_HOME so we don't pollute the runner's * main `~/.local/share/opencode`. - * 2. Bootstrap the DB schema by running `opencode serve` and polling - * for the materialized `opencode.db` file. - * 3. Import each bundle in parallel — one bad bundle shouldn't - * prevent the rest from being restored. - * 4. Return the resolved name→sessionID map for the orchestrator. + * 2. Bootstrap the DB schema by running `opencode serve` and waiting + * for its `listening on http://` ready line (emitted AFTER all + * migrations commit). Polling for `opencode.db` alone is unsafe — + * SQLite creates the file before migrations finish. + * 3. Import each bundle SERIALLY. Parallel imports race the + * non-idempotent migration DDL against the same SQLite DB; one + * bad bundle still doesn't block the rest (per-bundle try/catch). * * On any failure, returns an empty map and null tempDataHome — the * caller should fall back to the "no resume, fresh start" path. @@ -64,26 +66,55 @@ export async function restoreSessionBundles( process.env.XDG_DATA_HOME = tempDataHome; console.log(`v2 resume: using temp XDG_DATA_HOME=${tempDataHome}`); - // Bootstrap the DB schema by running `opencode serve` and waiting for - // `opencode.db` to materialize on disk. The `opencode import` CLI - // requires the schema to exist before it can write session rows. - // We poll instead of using a fixed 3s timeout — on a cold runner the - // migration can take >1s, and on a warm cache it's instant. A safety - // cap protects against a wedged `opencode` binary. + // Bootstrap the DB schema. `opencode import` requires a FULLY-migrated + // schema before it can write session rows. The authoritative "schema + // ready" signal is the `opencode server listening on http://...` line + // that `opencode serve` prints AFTER all migrations commit. Polling + // for the `opencode.db` file alone is insufficient — SQLite creates + // the file before migrations finish, so a file-exists check can leave + // us with a half-migrated schema. When the `opencode import` calls + // then race the non-idempotent migration DDL (`ALTER TABLE … ADD`, + // `CREATE INDEX`, `INSERT INTO migration`), most fail with + // "index/column/migration row already exists" — observed in the wild + // as `v2 resume: 2/7 bundles restored` and empty reviewer output. const dbPath = join(tempDataHome, "opencode", "opencode.db"); const bin = options.opencodeBin ?? "opencode"; const bsProc = spawn(bin, ["serve", "--port", "0"], { env: { ...process.env, XDG_DATA_HOME: tempDataHome }, stdio: ["ignore", "pipe", "pipe"], }); - bsProc.on("error", (err) => { - console.warn(`v2 resume: opencode serve spawn failed: ${err.message}`); + + // Single promise resolved by the listening line, rejected on spawn + // error, premature exit, or timeout. Promise.withResolvers keeps one + // resolver wired across multiple event handlers without callback + // nesting; later resolve/reject calls on the settled promise are + // no-ops, so every handler can call them unconditionally. + const { promise: readyPromise, resolve: readyResolve, reject: readyReject } = + Promise.withResolvers(); + let serveOutput = ""; + const onServeData = (chunk: Buffer | string): void => { + serveOutput += typeof chunk === "string" ? chunk : chunk.toString("utf8"); + if (/listening on https?:\/\//i.test(serveOutput)) { + readyResolve(); + } + }; + bsProc.stdout?.on("data", onServeData); + bsProc.stderr?.on("data", onServeData); + bsProc.on("error", (err) => readyReject(err)); + bsProc.on("exit", (code) => { + if (code !== null) { + readyReject(new Error( + `opencode serve exited ${code} before schema was ready${ + serveOutput ? `: ${serveOutput.trim().slice(0, 500)}` : "" + }`, + )); + } }); - // AbortController to guarantee `opencode serve` is killed even if - // the function exits before reaching the SIGTERM below (e.g. the - // outer flow throws during bundle import). Without this, a leaked - // `opencode serve` could pin the temp XDG_DATA_HOME and block the - // cleanup in the finally block. + + // AbortController guarantees `opencode serve` is killed even if we + // exit early (timeout, or a thrown import later on). Without it a + // leaked serve could pin the temp XDG_DATA_HOME and block the + // finally cleanup in the caller. const bootstrapAbort = new AbortController(); const killBootstrap = () => { if (!bsProc.killed) bsProc.kill("SIGTERM"); @@ -92,36 +123,42 @@ export async function restoreSessionBundles( const bootstrapTimeoutMs = options.bootstrapTimeoutMs ?? 10_000; const bootstrapStartedAt = Date.now(); - const bootstrapDeadline = bootstrapStartedAt + bootstrapTimeoutMs; - while (Date.now() < bootstrapDeadline) { - if (existsSync(dbPath)) break; - await new Promise((r) => setTimeout(r, 100)); - } - bootstrapAbort.abort(); - if (!existsSync(dbPath)) { - console.warn(`v2 resume: schema bootstrap timed out after ${bootstrapTimeoutMs}ms; bundles may fail to import`); - } else { + const timer = setTimeout( + () => readyReject(new Error(`schema bootstrap timed out after ${bootstrapTimeoutMs}ms`)), + bootstrapTimeoutMs, + ); + + try { + await readyPromise; console.log(`v2 resume: schema bootstrapped in ${Date.now() - bootstrapStartedAt}ms`); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); + const haveDb = existsSync(dbPath) ? "db file exists" : "no db file"; + // Best-effort fallback for opencode versions that don't emit the + // listening line: if the db file at least exists, try the imports + // anyway — per-bundle failures are isolated below. + console.warn(`v2 resume: ${reason} (${haveDb}); bundles may fail to import`); + } finally { + clearTimeout(timer); + bootstrapAbort.abort(); } - // Import each bundle in parallel. Per-bundle failures are isolated — - // one bad bundle should not prevent the rest from restoring. - const importResults = await Promise.allSettled( - reviewBundles.bundles.map(async (b) => { + // Import bundles SERIALLY. Even with a fully-migrated schema above, + // parallel `opencode import` against the same SQLite DB can race on + // WAL write locks and re-trigger migration DDL if any import detects + // a stale schema version. Serializing costs a few seconds but makes + // the restore deterministic. Per-bundle failures are still isolated. + const existingSessions = new Map(); + for (const b of reviewBundles.bundles) { + try { const importedId = await importBundle(b.bundle, { ...process.env, XDG_DATA_HOME: tempDataHome, }); - return { name: b.name, sessionID: importedId }; - }), - ); - const existingSessions = new Map(); - for (const r of importResults) { - if (r.status === "fulfilled") { - existingSessions.set(r.value.name, r.value.sessionID); - console.log(`v2 resume: imported bundle for "${r.value.name}" → ${r.value.sessionID}`); - } else { - const reason = r.reason instanceof Error ? r.reason.message : String(r.reason); + existingSessions.set(b.name, importedId); + console.log(`v2 resume: imported bundle for "${b.name}" → ${importedId}`); + } catch (err) { + const reason = err instanceof Error ? err.message : String(err); console.warn(`v2 resume: failed to import bundle: ${reason}`); } } diff --git a/multi-review/tsconfig.json b/multi-review/tsconfig.json index cd9fbd8..0ce6db8 100644 --- a/multi-review/tsconfig.json +++ b/multi-review/tsconfig.json @@ -1,6 +1,7 @@ { "compilerOptions": { "target": "ES2022", + "lib": ["ES2024"], "module": "ES2022", "moduleResolution": "bundler", "strict": true, From b1b25571b3992644f9079a5090b6f1ef26e50f57 Mon Sep 17 00:00:00 2001 From: svtter Date: Sun, 28 Jun 2026 11:15:05 +0800 Subject: [PATCH 2/2] address review: Node20-safe polyfill, stream error handling, bounded output, more tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- multi-review/dist/index.cjs | 22 ++++- multi-review/src/session-resume.test.ts | 111 ++++++++++++++++++++++-- multi-review/src/session-resume.ts | 40 +++++++-- multi-review/tsconfig.json | 1 - 4 files changed, 157 insertions(+), 17 deletions(-) diff --git a/multi-review/dist/index.cjs b/multi-review/dist/index.cjs index 760008b..ea924a0 100644 --- a/multi-review/dist/index.cjs +++ b/multi-review/dist/index.cjs @@ -6267,6 +6267,15 @@ var import_node_fs5 = require("fs"); var import_node_child_process4 = require("child_process"); var import_node_crypto = require("crypto"); var import_node_path5 = require("path"); +function withResolvers() { + let resolve2; + let reject; + const promise = new Promise((res, rej) => { + resolve2 = res; + reject = rej; + }); + return { promise, resolve: resolve2, reject }; +} async function restoreSessionBundles(reviewBundles, options) { const empty = { existingSessions: /* @__PURE__ */ new Map(), tempDataHome: null }; if (!reviewBundles || reviewBundles.bundles.length === 0) return empty; @@ -6280,16 +6289,25 @@ async function restoreSessionBundles(reviewBundles, options) { env: { ...process.env, XDG_DATA_HOME: tempDataHome }, stdio: ["ignore", "pipe", "pipe"] }); - const { promise: readyPromise, resolve: readyResolve, reject: readyReject } = Promise.withResolvers(); + const { promise: readyPromise, resolve: readyResolve, reject: readyReject } = withResolvers(); + const MAX_SERVE_OUTPUT = 4096; let serveOutput = ""; + const appendServe = (text) => { + serveOutput += text; + if (serveOutput.length > MAX_SERVE_OUTPUT) { + serveOutput = serveOutput.slice(serveOutput.length - MAX_SERVE_OUTPUT); + } + }; const onServeData = (chunk) => { - serveOutput += typeof chunk === "string" ? chunk : chunk.toString("utf8"); + appendServe(typeof chunk === "string" ? chunk : chunk.toString("utf8")); if (/listening on https?:\/\//i.test(serveOutput)) { readyResolve(); } }; bsProc.stdout?.on("data", onServeData); bsProc.stderr?.on("data", onServeData); + bsProc.stdout?.on("error", readyReject); + bsProc.stderr?.on("error", readyReject); bsProc.on("error", (err) => readyReject(err)); bsProc.on("exit", (code) => { if (code !== null) { diff --git a/multi-review/src/session-resume.test.ts b/multi-review/src/session-resume.test.ts index 6dfdd9b..a626d53 100644 --- a/multi-review/src/session-resume.test.ts +++ b/multi-review/src/session-resume.test.ts @@ -54,6 +54,10 @@ const waitFile = (p, capMs) => { } }; if (cmd === "serve") { + // OAC_TEST_SERVE_EXIT: simulate 'opencode serve' crashing before the + // listening line (covers the bsProc "exit" reject path). + const exitCode = process.env.OAC_TEST_SERVE_EXIT; + if (exitCode) { process.stderr.write("serve crashed\\n"); process.exit(Number(exitCode)); } const xdg = process.env.XDG_DATA_HOME; if (xdg) { fs.mkdirSync(path.join(xdg, "opencode"), { recursive: true }); @@ -65,8 +69,9 @@ if (cmd === "serve") { setTimeout(() => { process.stdout.write("opencode server listening on http://127.0.0.1:0\\n"); }, delay); - // Stay alive like a real server until SIGTERM. - setInterval(() => {}, 1 << 30); + // Stay alive like a real server until SIGTERM. unref() so the keepalive + // timer never pins the test process exit. + setInterval(() => {}, 1 << 30).unref(); } else if (cmd === "import") { const file = process.argv[3]; // Key the marker/gate by the session id parsed from the bundle JSON @@ -75,6 +80,10 @@ if (cmd === "serve") { // would make all concurrent imports share one marker. let id = "unknown"; try { id = JSON.parse(fs.readFileSync(file, "utf8")).info.id || id; } catch (e) {} + // OAC_TEST_IMPORT_FAIL_IDS: comma-sep session ids whose import should + // exit non-zero (covers the per-bundle try/catch isolation path). + const failIds = (process.env.OAC_TEST_IMPORT_FAIL_IDS || "").split(",").map((s) => s.trim()).filter(Boolean); + if (failIds.includes(id)) { process.stderr.write("import failed for " + id + "\\n"); process.exit(1); } const key = id.replace(/[^A-Za-z0-9_.-]/g, "_"); const markerDir = process.env.OAC_TEST_MARKER_DIR; const gateDir = process.env.OAC_TEST_IMPORT_GATE_DIR; @@ -239,7 +248,7 @@ describe("session-resume", { concurrency: false }, () => { const warns: string[] = []; const origWarn = console.warn; console.warn = (...args: unknown[]) => { warns.push(args.map(String).join(" ")); }; - let ctx: Awaited> | undefined; + let ctx: ResumeContext | undefined; try { process.env.OAC_TEST_SERVE_DELAY_MS = "500"; // No IMPORT_GATE_DIR → imports run instantly via the fallback @@ -254,6 +263,10 @@ describe("session-resume", { concurrency: false }, () => { `expected bootstrap timeout warning (proves we waited for the listening line, not the db file); got: ${warns.join(" | ")}`, ); assert.ok(ctx.tempDataHome, "tempDataHome must still be returned for caller cleanup"); + // The fallback path must still have imported the single bundle — + // without this assertion the test would pass even if imports were + // skipped entirely after the timeout. + assert.strictEqual(ctx.existingSessions.size, 1, "fallback import must succeed after bootstrap timeout"); } finally { console.warn = origWarn; fake.restore(); @@ -262,9 +275,93 @@ describe("session-resume", { concurrency: false }, () => { } }); - it("returns early with empty map when there are no bundles", async () => { - const ctx = await restoreSessionBundles(null, { baseTempDir: tmpdir(), prLabel: "empty" }); - assert.strictEqual(ctx.existingSessions.size, 0); - assert.strictEqual(ctx.tempDataHome, null); + it("returns early with empty map for null / undefined / empty bundles", async () => { + const inputs: Array = [ + null, + undefined, + { version: 2, repo: "o/r", prNumber: "1", savedAt: "now", bundles: [] }, + ]; + for (const input of inputs) { + const ctx = await restoreSessionBundles(input, { baseTempDir: tmpdir(), prLabel: "empty" }); + assert.strictEqual(ctx.existingSessions.size, 0); + assert.strictEqual(ctx.tempDataHome, null); + } + }); + + it("survives `opencode serve` spawn failure (missing binary) without throwing", async () => { + // Covers the bsProc "error" reject path: a non-existent opencodeBin + // makes spawn emit 'error'. restoreSessionBundles must NOT throw — + // it logs a warning and falls through to (failing) imports, which + // are themselves isolated. The result is an empty map but a real + // tempDataHome for the caller to clean up. + const warns: string[] = []; + const origWarn = console.warn; + console.warn = (...args: unknown[]) => { warns.push(args.map(String).join(" ")); }; + let ctx: ResumeContext | undefined; + try { + ctx = await restoreSessionBundles(makeBundles(["quality"]), { + baseTempDir: tmpdir(), + prLabel: "test-spawn-fail", + opencodeBin: "/nonexistent/opencode-binary-that-does-not-exist", + bootstrapTimeoutMs: 1000, + }); + assert.ok(ctx.tempDataHome, "tempDataHome must still be returned for caller cleanup"); + assert.strictEqual(ctx.existingSessions.size, 0, "no bundle should import when serve can't start"); + assert.ok(warns.length > 0, "spawn failure must produce a warning"); + } finally { + console.warn = origWarn; + if (ctx?.tempDataHome) rmSync(ctx.tempDataHome, { recursive: true, force: true }); + } + }); + + it("survives `opencode serve` crashing before the listening line", async () => { + // Covers the bsProc "exit" (non-null code) reject path. The fake + // serve exits 1 immediately. restoreSessionBundles must warn and + // fall through; imports are attempted via the fallback path. + const fake = installFakeOpencode(); + const warns: string[] = []; + const origWarn = console.warn; + console.warn = (...args: unknown[]) => { warns.push(args.map(String).join(" ")); }; + let ctx: ResumeContext | undefined; + try { + process.env.OAC_TEST_SERVE_EXIT = "1"; + ctx = await restoreSessionBundles(makeBundles(["quality"]), { + baseTempDir: tmpdir(), + prLabel: "test-serve-crash", + bootstrapTimeoutMs: 1000, + }); + assert.ok(ctx.tempDataHome, "tempDataHome must still be returned for caller cleanup"); + assert.ok( + warns.some((w) => /serve exited 1 before schema was ready/.test(w)), + `expected serve-crash warning; got: ${warns.join(" | ")}`, + ); + } finally { + console.warn = origWarn; + fake.restore(); + delete process.env.OAC_TEST_SERVE_EXIT; + if (ctx?.tempDataHome) rmSync(ctx.tempDataHome, { recursive: true, force: true }); + } + }); + + it("isolates per-bundle import failures: one bad bundle doesn't block the rest", async () => { + // Covers the per-bundle try/catch in the serial loop. 'security' + // is configured to exit 1; the other three must still import. + const fake = installFakeOpencode(); + let ctx: ResumeContext | undefined; + try { + process.env.OAC_TEST_IMPORT_FAIL_IDS = "ses_src_security"; + ctx = await restoreSessionBundles( + makeBundles(["quality", "security", "performance"]), + { baseTempDir: tmpdir(), prLabel: "test-partial-fail" }, + ); + assert.strictEqual(ctx.existingSessions.size, 2, "2 of 3 bundles should import despite one failure"); + assert.ok(ctx.existingSessions.has("quality")); + assert.ok(ctx.existingSessions.has("performance")); + assert.ok(!ctx.existingSessions.has("security"), "the failing bundle must not appear in the map"); + } finally { + fake.restore(); + delete process.env.OAC_TEST_IMPORT_FAIL_IDS; + if (ctx?.tempDataHome) rmSync(ctx.tempDataHome, { recursive: true, force: true }); + } }); }); diff --git a/multi-review/src/session-resume.ts b/multi-review/src/session-resume.ts index 2a690ae..c41a207 100644 --- a/multi-review/src/session-resume.ts +++ b/multi-review/src/session-resume.ts @@ -5,6 +5,20 @@ import { join } from "node:path"; import { importBundle } from "./opencode-bundle.js"; import type { ReviewContextV2 } from "./types.js"; +/** + * Node 20-compatible resolver. `Promise.withResolvers` only lands in + * Node 22+, but this action runs on consumer runners that may still be + * on Node 20 (`engines.node: ">=20"`). Manual form keeps the same + * "one resolver wired across many handlers, later settle calls are + * no-ops" guarantee without a runtime version gate. + */ +function withResolvers(): { promise: Promise; resolve: (v: T | PromiseLike) => void; reject: (e: unknown) => void } { + let resolve!: (v: T | PromiseLike) => void; + let reject!: (e: unknown) => void; + const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); + return { promise, resolve, reject }; +} + /** * Result of attempting to restore previous v2 session bundles into a * fresh opencode DB on a (possibly different) runner. @@ -85,21 +99,33 @@ export async function restoreSessionBundles( }); // Single promise resolved by the listening line, rejected on spawn - // error, premature exit, or timeout. Promise.withResolvers keeps one - // resolver wired across multiple event handlers without callback - // nesting; later resolve/reject calls on the settled promise are - // no-ops, so every handler can call them unconditionally. - const { promise: readyPromise, resolve: readyResolve, reject: readyReject } = - Promise.withResolvers(); + // error, premature exit, stream error, or timeout. One resolver is + // wired across every handler; later settle calls on an already-settled + // promise are no-ops, so handlers can call them unconditionally. + const { promise: readyPromise, resolve: readyResolve, reject: readyReject } = withResolvers(); + // Cap captured serve output: listening-line detection only needs the + // tail, and an unbounded buffer could grow to MB if serve logs + // verbosely before printing the ready line. Keep the last 4KB. + const MAX_SERVE_OUTPUT = 4096; let serveOutput = ""; + const appendServe = (text: string): void => { + serveOutput += text; + if (serveOutput.length > MAX_SERVE_OUTPUT) { + serveOutput = serveOutput.slice(serveOutput.length - MAX_SERVE_OUTPUT); + } + }; const onServeData = (chunk: Buffer | string): void => { - serveOutput += typeof chunk === "string" ? chunk : chunk.toString("utf8"); + appendServe(typeof chunk === "string" ? chunk : chunk.toString("utf8")); if (/listening on https?:\/\//i.test(serveOutput)) { readyResolve(); } }; bsProc.stdout?.on("data", onServeData); bsProc.stderr?.on("data", onServeData); + // Stream errors must reject too, otherwise a broken pipe would leave + // us awaiting a promise that never settles. + bsProc.stdout?.on("error", readyReject); + bsProc.stderr?.on("error", readyReject); bsProc.on("error", (err) => readyReject(err)); bsProc.on("exit", (code) => { if (code !== null) { diff --git a/multi-review/tsconfig.json b/multi-review/tsconfig.json index 0ce6db8..cd9fbd8 100644 --- a/multi-review/tsconfig.json +++ b/multi-review/tsconfig.json @@ -1,7 +1,6 @@ { "compilerOptions": { "target": "ES2022", - "lib": ["ES2024"], "module": "ES2022", "moduleResolution": "bundler", "strict": true,