Skip to content

sweep: concurrent codex reviews via CODEX_JOBS worker pool - #48

Merged
Octember merged 2 commits into
mainfrom
claude/stupifai-not-responding-2e2281
Jul 20, 2026
Merged

sweep: concurrent codex reviews via CODEX_JOBS worker pool#48
Octember merged 2 commits into
mainfrom
claude/stupifai-not-responding-2e2281

Conversation

@Octember

Copy link
Copy Markdown
Owner

A sweep's wall-clock was the SUM of its codex runs — each review is minutes, so a busy sweep serialized PRs behind each other for up to an hour while the lock held off every cron tick.

Now the loop collects candidates through the same cheap serial gates (dedup, throttle, diff cap, MAX_PRS, daily budget), then reviews them with a pool of up to CODEX_JOBS (default 3) concurrent codex children. Only the codex spawn went async (execAsync, mirroring the kit's exec result shape); every gh call stays the kit's sync exec, and all shared-state mutation stays on the one JS thread between awaits. A quota limit from any worker stops new launches while in-flight runs drain, then the un-started candidates get the usual deferred status.

@exe-dev-github-integration exe-dev-github-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, one real issue in the async child wrapper.

Comment thread src/review-sweep.ts Outdated
let done = false
const timer = opts.timeoutMs === undefined ? undefined : setTimeout(() => {
extra += `\n${cmd}: process killed by SIGTERM (timeout ${opts.timeoutMs}ms)`
child.kill('SIGTERM')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 src/review-sweep.ts:967 · footgun · conf 0.77
The timeout path only sends SIGTERM and then lets the normal close handler decide success from the child's exit code. A codex process that traps SIGTERM and exits 0 can still be accepted as ok, and one that does not exit leaves the worker pending and the sweep lock held indefinitely.
→ Fix: Track a timedOut flag and make timeout a failed outcome regardless of exit code; preferably add a short SIGKILL fallback so the promise always settles.

@Octember
Octember merged commit 60abb5d into main Jul 20, 2026
2 checks passed
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