fix(review): bound in-flight conversations across scheduled scans

[neubig]

Problem

The scheduled GitHub PR reviewer limits new conversations per scan, but does not cap conversations that remain active across scans. On the OSS Agent Canvas VM, scans accumulated enough Docker runtimes to cause repeated container OOM kills, one host-wide OOM, 159 GB of retained runtime data, and eventual loss of guest networking.

Acceptance criteria

• The reviewer counts its nonterminal dispatched conversations before starting more.
• A configurable global in-flight limit bounds active review conversations across scheduled scans.
• Completed, errored, stale, and missing conversations release capacity.
• Existing per-scan ordering, deduplication, and max-new-per-run behavior remain unchanged.
• Tests cover capacity across multiple scans and capacity recovery.

---

OpenHands AI triage

The following comments and acceptance criteria were added by the OpenHands AI agent.

Triage

The scheduled reviewer already bounds how many conversations a single scan starts (ReviewIntake / max_new_per_run, #659), but it does not bound conversations that remain nonterminal across scans. Repeated scans therefore stack Docker runtimes until the OSS Agent Canvas VM exhausts memory. This is bounded scheduling work in the existing reviewer automation, so no further design decision is needed.

Scope is skills/github-pr-reviewer/scripts/worker.py, its rendered config.json, and the agent-driven setup path, reusing the machinery #654/#659 already added: the KV-backed AgentConversationDispatcher (per-subject records under agent-conversation-{conversation_id}), the exact-head check gate, _finish_completed_review, run_repositories, and the agent server's per-conversation execution status. The count is the reviewer's own dispatched conversations across all configured repositories, not a new queue or runtime layer.

Counting is reviewer scheduling behavior, so it stays in the reviewer worker rather than in the shared agent_conversation.py dispatcher that github-issue-to-pr, gitlab-issue-to-mr, and github-issue-triage also import.

Non-goals: no repository-specific policy, VM tuning, or deployment control; no second automation record; no new secret, token scope, or GitHub permission; no change to the review prompt, verdict parsing, maintainer handoff, secret selection, per-scan ordering or deduplication, the trigger-label scan, or the explicit review_requested event path.

Desired Behavior

A scheduled scan determines how many of its dispatched review conversations are still nonterminal before it starts new ones, and starts no more than a configurable global in-flight limit allows. Capacity is released as conversations complete, error, go stale, or disappear, so later scans resume starting reviews. Existing per-scan ordering, deduplication, and max_new_per_run behavior are unchanged.

Acceptance Criteria

• Before starting any new review conversation, a scheduled scan counts the reviewer's own nonterminal dispatched conversations across all configured repositories (not per repository), and starts only as many new ones as remain below the configured global in-flight limit; when the count is already at or above the limit it starts none.
• The global in-flight limit is separate from max_new_per_run: both apply to a scheduled scan, max_new_per_run still bounds new starts per scan, and the number started is the smaller of the two remainders.
• Capacity is released when a conversation is completed, errored, stale, or missing: a conversation whose execution status is terminal (finished, idle, error, or stuck), one whose reviewed head is no longer the pull request's current head, one abandoned past the existing maximum-active-age bound, and one the agent server reports as missing (404) are all not counted, so a later scheduled scan can start again.
• An unreadable conversation status (a status lookup that raises) does not allow the limit to be exceeded: the scan treats the unknown conversation as occupying capacity and reports it.
• Reaching the limit does not cut the scan short: the remaining eligible candidates still get their exact-head check evaluation and waiting/blocked dispositions, completed reviews are still reconciled, and maintainer handoff still runs.
• A dispatch that raises is reported and consumes no capacity, and does not abort the run or the other repositories.
• Existing behavior is unchanged: eligible-candidate ordering, per-(request event, head SHA) deduplication so a repeated scan creates no second conversation or native review, the trigger-label scan, and the explicit review_requested event path. The global in-flight limit bounds only the scheduled scan's new conversations.
• The limit is configurable through the reviewer's rendered config.json and the agent-driven setup substitutions alongside max_new_per_run; an unset value applies a default documented in SKILL.md and README.md; a non-positive, non-integer, or boolean value fails the run at config load, matching max_new_per_run validation.
• No new secret, token scope, or GitHub permission is required, and no secret value appears in run logs, comments, or the PR.
• Focused automated tests driving the shipped reviewer entrypoint through the catalog-bundle test helper cover: (a) capacity persisting across scans, where a first scan fills the limit and a second scan with those conversations still nonterminal starts none; (b) capacity recovery, where once those conversations are terminal, stale, or missing a later scan starts the next eligible conversation; (c) the limit being shared across two repositories; and (d) config validation rejecting a non-positive or boolean value.
• SKILL.md and README.md document the global in-flight limit, its config key, its default, and how it composes with max_new_per_run; the catalog entry and bundle versions are bumped and generated catalogs regenerated so python scripts/sync_extensions.py --check and npm run build report no drift.
• Where the OSS Agent Canvas deployment is reachable, the PR's evidence links the scheduled run's conversations showing the in-flight count was not exceeded; if it is not reachable, the PR states that explicitly instead of claiming live validation.

[neubig]

@all-hands-bot implement this feature

[all-hands-bot]

Implemented the global in-flight bound for the scheduled PR reviewer. It is a working-tree change, not a pushed branch; say the word if you want a PR.

What changed

skills/github/scripts/agent_conversation.py (the shared dispatcher)

• Added an opt-in in-flight registry: a dispatcher constructed with track_in_flight=True records every conversation it starts in the Automation KV store under agent-conversations-in-flight, keyed by conversation id with {subject, started_at}.
• in_flight() returns the number of still-nonterminal conversations, pruning the rest: a finished/error/stuck status releases the slot, a 404 from the agent server releases it, and an entry older than two hours is dropped as abandoned. An entry with no timestamp counts as live. Unknown statuses count as live too, so a transient read failure never lets a scan overshoot, and a read that raises counts the conversation as live rather than losing it.
• Added release(conversation_id) for a worker that wants to free a slot explicitly.
• Tracking is opt-in so the other automations sharing this class (github-issue-to-pr, github-issue-triage) keep writing exactly one KV record per subject.

skills/github-pr-reviewer/scripts/worker.py

• ReviewIntake now takes a capacity callable. The per-scan budget becomes min(max_new_per_run, max(0, max_in_flight - in_flight)), so the cap is what actually holds live runtimes down across scans. The count is read once before the drain.
• An over-cap count clamps to zero rather than going negative, and a missing/failing capacity signal falls back to the per-scan bound alone, so the scan keeps working with no runtime to talk to.
• run_scan() and a standalone run() both wire capacity from the dispatcher; the worker still imports no conversation runtime (the existing no-runtime-code contract test still passes).

skills/github-pr-reviewer/scripts/main.py

• Added MAX_IN_FLIGHT = 4 with validation of the max_in_flight config key (positive int, bool rejected), plus the config override.

Catalog / docs

• automations/catalog/github-pr-reviewer/manifest.json: new maxInFlight form field (default 4), max_in_flight in the bundle config, version bumped 1.8.0 -> 1.9.0, description and exampleImplementation updated. Regenerated bundle-index.js and skills/index.js.
• Updated SKILL.md (new "Global in-flight cap" section, eighth constant substitution, troubleshooting rows) and README.md; refreshed the contract fixture.

Acceptance criteria

• The reviewer counts its nonterminal dispatched conversations before starting more - yes, via in_flight() read once per scan.
• A configurable global in-flight limit bounds active reviews across scans - max_in_flight / MAX_IN_FLIGHT, default 4.
• Completed, errored, stale, and missing conversations release capacity - terminal status, stale registry entry, and 404 all free the slot; a stale review also releases its slot once its conversation is terminal.
• Existing per-scan ordering, deduplication, and max-new-per-run are unchanged - the per-scan bound is now min'd with the cap and the dedupe/ordering paths are untouched.
• Tests cover capacity across multiple scans and capacity recovery - added ~10 worker tests (cap leaves one slot, capacity freed and reused on a later scan, full cap starts nothing, over-cap clamps, no-signal / non-int / raising fallbacks, single read across repositories) and ~8 dispatcher tests (recorded and counted, opt-out returns None, terminal vs nonterminal statuses, abandoned entry, 404 release, explicit release).

uv run pytest -q is green: 1098 passed, 23 skipped.

Generated by OpenHands AI on behalf of the user.

[neubig]

@all-hands-bot Please open a PR with these changes, link it to this issue, and request review from all-hands-bot.

[all-hands-bot]

Opened the PR: #690

• Branch: fix/689-bound-in-flight-review-conversations
• Base: main
• Title: fix(review): bound in-flight conversations across scheduled scans
• Body links this issue (Closes #689).

Reviewer request: I could not add all-hands-bot as a reviewer. The GITHUB_TOKEN used here authenticates as all-hands-bot itself, which makes that account the PR author, and GitHub rejects review requests from the PR author (422 Review cannot be requested from pull request author). The request needs to come from a maintainer account, or via the reviewer automation's own review_requested path. Everything else you asked for is done.

Summary of the change

• skills/github/scripts/agent_conversation.py: opt-in in-flight registry (agent-conversations-in-flight in the Automation KV store), in_flight() prunes terminal (finished/error/stuck), 404-missing, and two-hour-abandoned entries, and release(conversation_id).
• skills/github-pr-reviewer/scripts/worker.py: ReviewIntake takes a capacity callable; the per-scan budget is min(max_new_per_run, max(0, max_in_flight - in_flight)), read once before the drain. Over-cap clamps to zero; a missing or raising signal falls back to the per-scan bound.
• skills/github-pr-reviewer/scripts/main.py: MAX_IN_FLIGHT = 4 plus max_in_flight config validation.
• Catalog/docs: maxInFlight form field (default 4), bundle 1.8.0 -> 1.9.0, regenerated bundle-index.js / skills/index.js, SKILL.md / README.md, contract fixture refreshed.

Verification

• uv run pytest -q — 1098 passed, 23 skipped.
• uv run python scripts/sync_extensions.py --check — no drift.
• npm run build — no drift after regeneration.

The OSS Agent Canvas deployment was not reachable from this environment, so the PR says so explicitly rather than claiming live validation.

Generated by OpenHands AI on behalf of the user.

[neubig]

@all-hands-bot The implementation is incomplete: worker.py explicitly bypasses ReviewIntake in event mode, so simultaneous review_requested webhooks can still exceed max_in_flight. Fix this before opening the PR. Enforce the capacity atomically in the shared dispatcher for every created/retried/resumed launch path, including event mode. When capacity is full, preserve the delivery as pending/deferred so a later scheduled scan processes it; do not consume or lose the request. Add tests for concurrent/burst event deliveries and mixed event+scheduled launches, then open the PR and request all-hands-bot review.

[all-hands-bot]

Fixed on PR #690 (branch fix/689-bound-in-flight-review-conversations). The cap is now enforced atomically in the shared dispatcher, for every launch path including event mode, and a full cap defers the delivery instead of consuming it.

What changed

skills/github/scripts/agent_conversation.py (the shared dispatcher)

• deliver() now takes a max_in_flight cap and is the single admission gate. It reserves a slot with a conditional read-modify-write of the registry in the Automation KV store (nx for the first write, if_version compare-and-set otherwise) and retries a lost race against fresh state, so two concurrent deliveries - two review_requested webhooks, or an event and a scheduled scan - cannot both take the last slot.
• When the cap is full, deliver returns a deferred disposition, starts no runtime, and writes no delivery record. The GitHub request is therefore left outstanding for a later scheduled scan; nothing is consumed or lost.
• Reserving a slot that starts a runtime covers creating a conversation and resuming one that is idle/paused. A conversation already running, or already finished for the same delivery, consumes no new slot and is never deferred.
• A reserved slot is now held for a short grace window before a 404 from the agent server is believed. This closes a remaining overshoot: because the slot is reserved just before RemoteConversation.create, a concurrent admission reading the registry in that window would otherwise prune the reservation as "gone", free the slot, and start past the cap. With max_in_flight=1 two deliveries could both start before this fix.
• A failed RemoteConversation.create releases its reservation.

skills/github-pr-reviewer/scripts/worker.py

• Both launch paths go through the dispatcher: the scheduled drain, and the event path that previously called _start_review directly and bypassed ReviewIntake. Event mode still bypasses the per-scan max_new_per_run budget (it is a caller's explicit request), but it no longer bypasses the global cap.
• _start_review passes max_in_flight; the drain stops on the first deferred delivery, since nothing behind it could be admitted either. The scan's earlier in_flight() read is now only a pre-filter and is no longer trusted as the bound.

Docs/catalog updated (SKILL.md, README.md, manifest description/exampleImplementation); bundle-index.js regenerated.

Tests

• tests/test_agent_conversation_dispatch.py: atomic admission (refuse at cap, reserve under cap, zero cap, create-registry atomically, retry a lost race without overshooting), a 10-thread concurrent burst under a cap of two admitting exactly two, a failed creation releasing its slot, the new reserved-slot grace regression, and deferred deliveries leaving no state record.
• tests/test_github_reviewer_delivery.py: event-mode delivery deferred at the cap with its request preserved, event mode starting when capacity is available, and mixed event + scheduled sharing one cap.
• uv run pytest -q — 1110 passed, 23 skipped. npm run build and sync_extensions.py --check clean.

PR state

PR #690 is open, all CI checks pass, and the description now uses the repo template.

Reviewer request: I still cannot add all-hands-bot as a reviewer. The GITHUB_TOKEN authenticates as all-hands-bot, which is the PR author, and GitHub rejects a review request from the PR author (422 Review cannot be requested from pull request author; gh pr edit --add-reviewer silently no-ops for the same reason). The request needs to come from a maintainer account, or through the reviewer automation's own review_requested path.

Generated by OpenHands AI on behalf of the user.