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

[all-hands-bot]

HUMAN:

---

AGENT:

Why

The scheduled GitHub PR reviewer limited new conversations per scan, but not conversations still running across scans. On the OSS Agent Canvas VM, scans accumulated Docker runtimes until containers were OOM-killed. The host then ran out of memory, kept 159 GB of runtime data, and lost guest networking (#689). The review_requested event path skipped the scan's intake entirely, so a burst of webhook deliveries could also start unbounded runtimes. A count-then-start check also races across concurrent automation runs.

Summary

• Enforce a global max_in_flight cap (default 4) atomically in the shared AgentConversationDispatcher, which both the scheduled scan and the event path call. A slot is reserved with a conditional read-modify-write of a registry in the Automation KV store (nx for the first write, if_version compare-and-set after that), and a lost race re-reads. The cap is checked whenever a conversation is created, resumed, sent a new revision, or retried after an ERROR (fix(github): retry errored deliveries and wait on action_required heads #688). Past the cap, deliver returns deferred: nothing starts, no delivery record is written, and a deferred retry does not use up one of its two allowed attempts, so the GitHub request stays outstanding for a later scheduled scan. A failed launch releases its slot, and a just-reserved slot is held for a grace window before a 404 is believed.
• The cap applies to both delivery paths: the local Agent Server path (where the OOM happened) and the OpenHands Cloud/Enterprise path, where each review conversation has its own sandbox. On Cloud, slot state comes from the OpenHands API. A paused, errored, or missing sandbox frees its slot, and a start the API doesn't list yet is held for the existing 10-minute Cloud start grace.
• The scan's in_flight() read stays a pre-filter for min(max_new_per_run, max_in_flight - in_flight). The drain counts created and retried against max_new_per_run and stops at the first deferred. Bundles: github-pr-reviewer 1.10.0 (adds the maxInFlight setup field), github-issue-to-pr 1.2.3, github-issue-triage 1.3.2.

Issue Number

Closes #689

How to Test

uv sync --group test
uv run pytest -q                                   # 1216 passed, 23 skipped
uv run pytest -q tests/test_agent_conversation_dispatch.py tests/test_github_reviewer_delivery.py   # 183 passed
uv run python scripts/sync_extensions.py --check   # only the existing issue-duplicate-checker coverage warning
npm run build && git status                        # no drift

The focused tests drive the shipped dispatcher and reviewer entrypoint against a fake KV store that models the real store's versioned document, if_version, and nx:

• A 10-thread burst under a cap of 2 admits exactly 2, on both the local and the Cloud path.
• An event-mode request past the cap is deferred, and no delivery record is written.
• An event takes the last slot between a scheduled scan's count and its launch. The dispatcher still defers the scheduled launch; with admission disabled, the test creates two conversations.
• An ERROR retry past the cap is deferred without consuming a retry attempt, and a failed retry releases its slot, on both paths.
• Cloud tests cover deferral for new and resumed subjects, slot release for each sandbox/execution state, the Cloud start grace, and release on a failed start.

The KV conditional-write contract was checked against openhands/automation/kv_router.py in OpenHands/automation: meta=true returns the version, an if_version/nx PUT answers 409, and lock timeouts also answer 409.

Live validation was not performed. The OSS Agent Canvas deployment where the OOM occurred was not reachable from this environment, and no Cloud run was made. This still needs either a run of the scheduled and event-triggered reviewer under concurrent delivery on a real deployment, or maintainer confirmation that the automated coverage is acceptable.

Video/Screenshots

None; there is no UI change. Runtime logs from a deployment are the evidence still outstanding (see How to Test).

Notes

• max_in_flight / MAX_IN_FLIGHT defaults to 4 and is read from the rendered config.json; a non-positive or boolean value is rejected at load. Like MAX_NEW_PER_RUN, it is honored by the catalog worker.py, not by the standalone agent-driven main.py flow.
• An automation that runs only on the event trigger has no scheduled scan to retry a deferred request, the same as a waiting-on-checks outcome; it waits for a new review request.
• github-issue-to-pr and github-issue-triage ship the updated agent_conversation.py (bumped to 1.2.3 and 1.3.2) but don't opt into tracking, so their behavior is unchanged.

Generated by OpenHands AI on behalf of the user; merged with current main (#647, #688) and extended to the Cloud path by an AI agent (Claude Code, Opus 5.5) helping Engel Nyst (@enyst).

[neubig]

The capacity check still applies only to scheduled scans. In PullRequestReviewer.run(), event_mode calls _start_review(...) directly and bypasses ReviewIntake, so simultaneous review_requested deliveries can still create an unbounded number of Docker runtimes.

Please make admission atomic in the shared dispatcher and apply it to every launch path, including event mode. A read-count-then-create sequence is racy across concurrent automation runs; use the Automation KV store's optimistic concurrency or atomic nx operations to reserve capacity before starting/resuming a conversation. If capacity is full, return a deferred disposition without consuming the GitHub review request so a later scheduled scan retries it. Add burst-event and mixed event/scheduled concurrency tests. Keep the existing scheduled ordering and per-scan bound.

[neubig]

@all-hands-bot fix the capacity race and event-mode bypass described above, then update this PR.

[all-hands-bot]

Done — pushed ced325b to this branch. Both problems are fixed in the shared dispatcher, so every launch path is now bounded.

Capacity race

Admission is now atomic and lives in AgentConversationDispatcher, at the moment a conversation is created or resumed. The dispatcher reserves the slot with the Automation KV store's optimistic concurrency: it reads the registry at a document version, writes it back conditional on that version (if_version), and uses the store's atomic nx operation to create the registry when it does not exist. A write that loses the race (409) is retried against a fresh read, up to a bounded budget; a reservation that still cannot be won is refused rather than launched ungated. So two concurrent automation runs can no longer both observe one free slot and both start a runtime.

in_flight() remains, but only as a pre-filter for the scan's min(max_new_per_run, max_in_flight - in_flight) budget. The authoritative bound is the reservation. A conversation whose status cannot be read counts as live, so a transient failure never lets the deployment overshoot.

Event-mode bypass

The gate is now in deliver(), which both launch paths call — the scheduled drain and the event-triggered _start_review. A burst of simultaneous review_requested deliveries is admitted up to MAX_IN_FLIGHT and refused past it. The worker now constructs the dispatcher with max_in_flight=MAX_IN_FLIGHT, so event mode shares the same cap as scheduled scans.

Deferral semantics

A delivery refused at the cap returns {"disposition": "deferred"}. No conversation is started and no delivery record is written, so the GitHub review request is not consumed — a later scheduled scan sees the same outstanding request and retries it once capacity frees. The drain stops at the first deferral, since nothing behind it could be admitted either. A creation that fails after reserving a slot releases that slot so it is not held until the abandonment window.

Resuming an inactive conversation (idle / paused) also reserves a slot now, since it starts work.

Tests added

Dispatcher (tests/test_agent_conversation_dispatch.py), against a fake KV store that models the real store's versioned document, row-locked read-modify-write, if_version, and nx:

• refusal at the cap leaves the live set untouched and creates nothing
• admission under the cap reserves the slot; a first write creates the registry atomically
• a zero cap refuses the first conversation
• a deliberately lost conditional write re-reads and refuses instead of overshooting
• a threaded burst of 10 deliveries under a cap of 2 admits exactly 2 and defers 8
• a failed creation releases its reservation

Worker (tests/test_github_reviewer_delivery.py):

• event-mode request past the cap is deferred, and no delivery record is written (request unconsumed)
• event-mode request under the cap starts exactly one conversation
• mixed event + scheduled scan share one cap: the event fills it and the scheduled scan starts nothing
• a deferred deliver stops the drain without consuming a slot

Verification

• uv run pytest -q — 1109 passed, 23 skipped.
• node scripts/build-automation-catalog.mjs / build-skills-catalog.mjs regenerated automations/bundle-index.js and skills/index.js (the bundle inlines agent_conversation.py and worker.py); catalog tests pass.
• uv run python scripts/sync_extensions.py --check — only the pre-existing non-blocking coverage warning for plugins/issue-duplicate-checker.

Docs updated in SKILL.md ("Global in-flight cap" now describes the atomic dispatcher gate, event-mode coverage, and deferral) and README.md; the catalog manifest description notes the atomically enforced cap and event coverage.

Generated by OpenHands AI on behalf of the user.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Assessment

Reviewed head 8d08a82 against the repository guidance in AGENTS.md, the current PR body/threads/checks, and issue #689 (still ready-for-dev, priority:high). The change is in scope: it stays inside the reviewer extension's own skills/github-pr-reviewer worker plus the shared skills/github/scripts/agent_conversation.py dispatcher, does not touch the review prompt, verdict parsing, secret selection, per-scan ordering, or deduplication, and every check for this head passes. The one PR Description Check failure (run 985) was superseded by the successful re-run 986 on the same SHA.

I verified the mechanics the fix depends on: the Automation KV store really implements the optimistic-concurrency contract the dispatcher assumes (meta=true returns a document version; if_version=N and nx=true on PUT reject a lost race with 409), and the per-automation store is a single locked document, which is what makes the read-modify-write meaningful. The atomic admission, the deferral that leaves the GitHub request unconsumed, the reservation release on a failed create, and the pending-grace window are all driven by real dispatcher code and covered by the new tests (including the 10-thread burst that admits exactly the cap, and the mixed event + scheduled case sharing one cap).

Executed locally:

• uv run pytest -q tests/test_agent_conversation_dispatch.py tests/test_github_reviewer_delivery.py - 134 passed
• uv run pytest -q - 1110 passed, 23 skipped
• uv run python scripts/sync_extensions.py --check - no drift

I found no material correctness, security, or design issue. Non-blocking note below.

Non-blocking observation

MAX_IN_FLIGHT is dead configuration in the agent-driven standalone path: it is defined (line 67) and overridden from config.json (line 178) in skills/github-pr-reviewer/scripts/main.py, but nothing in that file ever reads it to bound starts - the only logic that consumes max_in_flight lives in the catalog worker.py / shared dispatcher. Meanwhile SKILL.md Step 6 now tells the agent-driven setup to set MAX_IN_FLIGHT = 4 "to cap the review conversations running at once across scans". A reader of that table could set the constant and see no effect on the manual path (which is uncapped per-run in both base and head). Worth either wiring main.py to the same cap or noting that the constant is only honored by the catalog worker. This does not affect the OSS Agent Canvas deployment this PR targets, which uses the catalog worker.

The remaining behavior I traced - dedupe reusing a runtime, deferred not consuming the request, release on failed create, in_flight() as a pre-filter with _admit as the authoritative bound, and the other dispatcher consumers (github-issue-to-pr, github-issue-triage) not opting in - is consistent and correct.

✅ APPROVED

[neubig]

The PR explicitly states that the OSS Agent Canvas deployment was unreachable and that no live validation was performed. The extensions review guide requires exercising the changed automation through its documented entrypoint in a supported environment; the concurrency tests are valuable regression coverage but do not replace runtime evidence for the dispatcher/deployment behavior. Please attach current-head evidence from the scheduled and event-driven reviewer entrypoints showing the cap under concurrent delivery, or obtain maintainer confirmation that the unavailable deployment limitation is acceptable. This comment was generated by an AI agent (OpenHands) on behalf of Graham Neubig.

[neubig]

Blocked on a rebase — main has moved and this now has 22 conflict hunks across 7 files:

Generated (should be regenerated, not hand-merged):

• automations/bundle-index.js — 2 hunks
• automations/catalog/github-pr-reviewer/manifest.json — 3
• skills/index.js — 1
• tests/fixtures/automations/github-pr-reviewer.json — 9

Hand-written (needs real resolution):

• skills/github/scripts/agent_conversation.py — 4 hunks
• tests/test_agent_conversation_dispatch.py — 2
• skills/github-pr-reviewer/SKILL.md — 1

The 15 hunks in generated artifacts shouldn't be resolved by hand — that's how catalog drift gets introduced silently. Resolve the 7 hand-written hunks, then re-run uv run python scripts/sync_extensions.py and npm run build so the generated files come back consistent.

The substance looks good: the atomic reservation in AgentConversationDispatcher with nx/if_version compare-and-set is the right place for this, and the grace window before believing a 404 is a genuinely subtle fix. Noting the PR is honest that the OSS Agent Canvas deployment wasn't reachable for live validation — automated coverage is fine for the cap, but I'd want to see the OOM/retained-data symptom actually disappear before calling #689 closed.

This comment was posted by an AI agent (OpenHands) on behalf of @neubig.

[enyst]

I'm an AI agent (Claude Code, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.

Brought this up to date with main at 516285e. The conflicts from neubig's 2026-10-04 list are resolved, and the description is updated. What changed, besides the mechanical merge:

• The cap now covers the Cloud path too. main split delivery into an OpenHands API path and a local Agent Server path. Admission is enforced on both: a new start, resuming an idle conversation, sending a revision, and #688's ERROR retry each need a slot. On Cloud, a slot's state comes from the API.
• The retry from #688 goes through admission. A retry past the cap is deferred without using up one of its two attempts, and a failed retry releases its slot.
• One test was strengthened. The earlier mixed event + scheduled test only ever reached the scan's pre-count. test_a_scheduled_launch_behind_a_stale_count_is_refused_by_admission now drives the real admission check on the scheduled path.

Full suite: 1216 passed, 23 skipped.

I'm leaving a comment rather than approving. neubig's 2026-10-03 request for runtime evidence, or a maintainer decision that the automated coverage is enough, still applies to this head, and nothing here was run against a live deployment.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Assessment

Reviewed head 516285e fresh (base main, open, not merged, not draft) against AGENTS.md and .agents/skills/custom-codereview-guide.md, the PR discussion, the current checks, and issue #689 (open, ready-for-dev, priority:high). The change is in scope: it is confined to the reviewer extension's own skills/github-pr-reviewer worker plus the shared skills/github/scripts/agent_conversation.py dispatcher, and does not touch the review prompt, verdict parsing, secret selection, ordering, or deduplication.

The two concerns raised on the earlier head are addressed here:

• Admission is atomic. _admit reserves a slot with a KV read-modify-write guarded by nx for the first write and if_version after, retries a lost race against a fresh read, and returns deferred (writing no delivery record) past the cap. I verified the store contract against the released openhands-automation package: in kv_router.py, meta=true returns the document version, an nx/if_version PUT answers 409, and lock timeouts also answer 409, so the CAS this code depends on is real.
• The event path no longer bypasses the cap. review_requested still skips the per-scan maximum but routes through _start_review -> dispatcher.deliver -> _admit, so a burst is admitted only up to max_in_flight. test_event_mode_cap_defers_an_extra_request_and_leaves_it_unconsumed and test_mixed_event_and_scheduled_share_one_cap exercise exactly this, and the drain stops at the first deferral without counting it.

I found no material code defect. Capacity is released on the SDK's terminal statuses (finished/error/stuck, matching ConversationExecutionStatus.is_terminal()), on a 404 past the reservation grace, on the abandonment window, and on the Cloud stopped-sandbox statuses; a failed launch releases its reservation; in_flight() is a pre-filter with _admit authoritative.

Executed locally on this head (no tracked files modified):

• uv run pytest -q tests/test_agent_conversation_dispatch.py tests/test_github_reviewer_delivery.py -> 183 passed
• uv run pytest -q -> 1216 passed, 23 skipped
• uv run python scripts/sync_extensions.py --check -> exit 0 (only the pre-existing, non-blocking issue-duplicate-checker coverage warning)

Blocking items

1. Runtime-parity checkpoint unmet, and a maintainer request is open. The repository review guide lists "Executable instructions and runtime parity" as a blocking checkpoint: a changed automation must be exercised through its documented entrypoint in a supported environment. This head states it was not - the OSS Agent Canvas deployment was unreachable and no Cloud run was made. neubig's 2026-10-03 review asks for current-head runtime evidence from the scheduled and event-driven entrypoints under concurrent delivery, or a maintainer decision that the automated coverage is enough. That request is unresolved on this head, and the author's own comment agrees. Code inspection cannot satisfy it; it needs a live run or a maintainer sign-off.

2. The PR description gate is red on this head. Validate PR description fails (both runs for 516285e) because the HUMAN: section is empty: the validator requires a >=20-character human note before AGENT:. This is a repo policy gate, not a code defect, but it is a current-head, merge-relevant fact. It is not in the branch ruleset's required set (test, sync-extensions, validate-claude-code, all passing), so it is a process blocker rather than a merge-protection one.

Note (non-blocking)

MAX_IN_FLIGHT is read and validated in the standalone agent-driven main.py, but only the catalog worker.py/dispatcher consumes it. The PR body now discloses this ("honored by the catalog worker.py, not by the standalone ... main.py flow"), so it is documented rather than misleading.

🔄 CHANGES REQUESTED

[neubig]

🟡 Acceptable

[TESTING GAPS]

• Failed CI: The current head has a failing Validate PR description check. A red head cannot be approved.
• Missing runtime evidence: This changes the production admission/dispatch path for scheduled and event-triggered reviews, but the PR explicitly says the OSS Agent Canvas deployment was unreachable and no live validation was performed. The repository guide requires following the changed automation's documented entrypoint in a supported environment. Please attach current-head runtime evidence from both scheduled and event-driven delivery under concurrent load, showing the global cap and slot release behavior. Unit tests and fake KV coverage do not replace this required live proof.

The security screen found no embedded secrets, prompt-injection payload, dependency addition, unsafe workflow change, or pipe-to-shell behavior. The implementation and test coverage appear thoughtful, but the failed validation and acknowledged evidence gap are independently blocking.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This is cross-run concurrency and backpressure logic on a credentialed production automation path; an error can recreate resource exhaustion or stall reviews globally.

VERDICT:
❌ Needs rework: Fix the failing PR-description check and provide live current-head runtime evidence for the affected delivery paths.

KEY INSIGHT:
Atomic admission is the correct architecture, but concurrency control is merge-ready only after the real dispatcher/KV/runtime path proves the modelled behavior.

This review was generated by OpenHands AI on behalf of neubig.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing. See the customization docs.
2. Re-request a review - the reviewer reads guidelines from the PR branch.
3. When merged, the guideline file goes through normal code review.

Resolve with AI? Install the iterate skill and run /iterate.

Was this review helpful? React with 👍 or 👎.