fix(agent): bound concurrent model calls and contain a failing sub agent

[Decipher]

Fixes #55.

When a sub agent throws during review, Promise.all in runPullRequestReview rejects on the first failure while the siblings keep running. The rejection propagates out of the try, so the finally calls workspace.clean() and deletes the checkout those siblings are still reading. Each one then fails every tool call with Workspace is not loaded., and because the agent loop returns tool errors to the model as tool result content rather than aborting, each orphan keeps stepping toward maxSteps. Those steps are billed through onUsage like any other.

Two changes

Bound concurrent model calls. Limiter already existed in apps/api/src/agent/llm/concurrency.ts and was referenced nowhere. This wraps the sender returned by createSender, so one module level limiter bounds the plan agent, the review sub agents, the writing agent, the reply agents and the issue agents together. The value comes from PROVAL_LLM_CONCURRENCY and defaults to 4, because the ceiling is a property of the user's provider plan rather than of Proval.

The sender is deliberately the place to do this rather than the sub agent fan out. On one burst I saw ten activities running together, most of them PR Reply rather than PR Review, and a reply agent never reaches the review fan out. Bounding the fan out alone leaves those unbounded and the provider still returns 429.

Contain the failure. The sub agent fan out becomes Promise.allSettled, and the first rejection is rethrown only after every sub agent has settled. The review fails exactly as before and the error reported to the user does not change, but no sub agent is left running against a cleaned workspace.

Limiter had no test coverage, so this adds some: the concurrency bound, slot release when a task throws, and the unbounded null maximum.

Scope

The review still fails when a sub agent fails. This is containment, not recovery.

A circuit breaker in the agent loop, giving up after a number of consecutive identical tool errors, would be the complementary change. That is a behaviour change rather than a fix, so I left it out.

Verification

Against 0.2.10:

• bun run typecheck reports 0 errors across api, web and client.
• bun test reports 68 pass, 0 fail, 65 of them pre-existing and 3 new.
• bunx prettier --check is clean on all four files.
• bunx eslint reports one warning in review/index.ts, a non-null assertion that predates this branch and sits outside the diff.

What I have not done is measure the saving on a live review that fails partway. The mechanism is read from the source and the containment is covered by the behaviour of allSettled, so I would describe the token figures as reasoned rather than observed.

Configuration

Variable	Default	Meaning
PROVAL_LLM_CONCURRENCY	4	Maximum model calls in flight across the process

Existing deployments do not need any change, because the default of 4 is at or above the review unit count of most merge requests.

[proval-0e6d07]

Thanks for this PR — a process-wide LLM concurrency limiter wired into the sender factory, plus switching the review sub-agents to Promise.allSettled so workspace cleanup can't race running agents, both make the review pipeline noticeably more robust.

a. Summary

This PR bounds all in-flight model calls through a shared Limiter created from PROVAL_LLM_CONCURRENCY (apps/api/src/agent/llm/concurrency.ts) and applied at the single createSender choke point (apps/api/src/agent/llm/factory.ts), with a solid test suite for the limiter. It also changes runPullRequestReview to await every sub-agent via Promise.allSettled before the finally block destroys the workspace, then rethrows the first rejection preserving order and the original error. No blocking issues found — two small improvement ideas below.

b. Good points

• 🟢 withConcurrencyLimit wraps every provider return inside createSender (apps/api/src/agent/llm/factory.ts), so the process-wide bound holds without any caller opt-in and future call sites can't accidentally bypass the limiter.
• 🟢 The limiter tests pin the three failure modes of a slot-based limiter — peak enforcement, slot release on throw, and null-means-unbounded — using deferred gates instead of timing-sensitive sleeps (apps/api/src/agent/llm/concurrency.test.ts).
• 🟢 readLlmConcurrency rejects NaN and values below 1, falling back to a bounded default of 4 rather than ever starting with a broken limit (apps/api/src/agent/llm/concurrency.ts, line 63).
• 🟢 Switching to Promise.allSettled before the finally cleanup guarantees no sub-agent is still running tools against a destroyed workspace, and the comment at apps/api/src/agent/pull-request/review/index.ts line 130 states exactly why the ordering matters.
• 🟢 The reassembly loop preserves submission order and rethrows settled.reason as-is, keeping results index-aligned with planResult.reviewUnitList and preserving instanceof JobCanceledError checks (apps/api/src/agent/pull-request/review/index.ts, line 157).

Main Issues

None.

Additional Notes

• apps/api/src/agent/llm/concurrency.ts, line 58: PROVAL_LLM_CONCURRENCY appears nowhere else in the repo (.env.example, README), and an invalid value silently falls back to 4 — consider documenting the knob and logging a warning when a set-but-invalid value is ignored.
• apps/api/src/agent/pull-request/review/index.ts, line 157: the settled loop rethrows only the first rejection in submission order, so other rejected reasons (e.g. the "failed to submit handoff" throw in apps/api/src/agent/pull-request/review/sub.service.ts, line 65, for a non-first sub-agent) are dropped without logging — logging every rejected reason before rethrowing would make multi-failure diagnosis order-independent.

[seoes]

@Decipher

Thanks for handling this. I added Limiter a while ago and never used it up actually and you uses it in the right place.

and I have a suggestion. Concurrency might fit better as a value on each model provider, since each endpoint has its own ceiling.

• Add model_provider.concurrency as an integer. null goes unlimited
• Add the field to the model provider form in dashboard
• share one Limiter for that model provider and apply it in Sender.send

Would you be okay if I make some more updates on this branch?

[Decipher]

Before and after numbers from running this branch in production for a day, plus the
lifetime cost of not having it.

The limiter works

Rate-limit failures on my instance, by day, counted from the activity table:

2026-10-02   49
2026-10-03   46
2026-10-04  124
2026-10-05   83
2026-10-06   54
2026-10-07  123
2026-10-08    0     <- this branch deployed

Zero. Not reduced, eliminated. Every day for the previous two weeks had between 28 and
124 rate-limit failures, and the first full day with a bounded sender had none. Eleven
reviews completed that day, so the instance was not idle.

What the unbounded version cost

Over the life of this instance, 1,141 activities:

• 260 completed, 881 failed.
• 756 of those failures were rate limits, costing 462,150,694 input tokens that
  were spent and then discarded.
• Total input spend 652,354,790 tokens, of which 507,829,109, or 77.8%, went to
  activities that never completed.

The 429 failures alone account for 71% of all input tokens this instance has ever spent.
That is the cost of fanning sub-agents out through an unthrottled Promise.all.

Why it compounds rather than merely wastes

A rate-limited review does not reach completed, so under on_first_push it does not
count as reviewed, so the next push starts another one, which raises concurrency, which
causes more rate limits. I have written that up separately in #64, where the worst case
on my instance is one merge request that consumed 179,090,453 input tokens across 81
reviews, of which exactly one completed.

So this branch is not only a fix for 429 noise. It breaks the input side of that loop.
#64 covers the other side.

One note for reviewers

PROVAL_LLM_CONCURRENCY is read only on this branch. On the default branch the Limiter
class in apps/api/src/agent/llm/concurrency.ts has no importers, so anything documenting
that variable needs to land after this, not before. I had a documentation change queued
that made exactly that mistake.

Environment

Proval 0.2.12 with this branch applied, published image otherwise, GitLab CE 18.9
provider, 26 registered repositories. Figures from the full activity table.

[seoes]

@Decipher

Zero. Not reduced, eliminated. Every day for the previous two weeks had between 28 and 124 rate-limit failures, and the first full day with a bounded sender had none. Eleven reviews completed that day, so the instance was not idle.

• 260 completed, 881 failed.
• 756 of those failures were rate limits, costing 462,150,694 input tokens that were spent and then discarded.
• Total input spend 652,354,790 tokens, of which 507,829,109, or 77.8%, went to activities that never completed.

Oh that proves everything

400M token is crazy, I should make a quick update.

Thank you for the additional data

[proval-0e6d07]

Thanks @Decipher — this push cleanly replaces the process-wide PROVAL_LLM_CONCURRENCY knob with a per-provider concurrency setting, end to end.

a. Summary

This follow-up swaps the env-var limiter for a DB-backed one: migration 0046 adds a nullable model_provider.concurrency column, loadLimiter keeps one shared Limiter per provider id at the createSender choke point (apps/api/src/agent/llm/concurrency.ts), the model API validates the field on create and update, and the dashboard gains a matching field with a new numeric mode on InputText. All 14 production createSender call sites across GitHub/GitLab/Forgejo and activity retries now thread modelProviderId + concurrency uniformly. This also makes my earlier note about documenting PROVAL_LLM_CONCURRENCY moot — the env var is gone from the repo entirely. No blocking issues found.

b. Good points

• 🟢 Migration 0046 is a single additive nullable column (packages/db/src/migration/0046_overconfident_triathlon.sql, line 1) and the rollout chain is consistent: 0046_snapshot.json prevId matches 0045_snapshot.json id, so existing rows read NULL → unlimited and the rollout can't throttle deployed providers.
• 🟢 invalidConcurrency rejects 0 and negatives on both create and update (apps/api/src/api/model/model.controller.ts, line 9) — rejecting 0 is load-bearing, since a Limiter with max 0 would park every acquire in the queue forever.
• 🟢 All 14 production createSender sites pass modelProviderId + concurrency together (e.g. apps/api/src/webhook/github/github.controller.ts, line 199), so the shared per-provider bound holds no matter which webhook or retry path triggers the LLM run.
• 🟢 The new loadLimiter tests assert observed peak concurrency through a deferred gate rather than just Map identity (apps/api/src/agent/llm/concurrency.test.ts, line 84), so a broken setMax or reuse path would fail the suite.

Main Issues

None.

Additional Notes

• apps/api/src/agent/llm/concurrency.ts, line 65: loadLimiter applies existing.setMax(concurrency ?? null) on every lookup, so a future call site that passes only modelProviderId (both fields are optional on SenderConfig) would silently reset that provider's shared cap to unlimited — consider calling setMax only when concurrency !== undefined, so "omitted" means "keep the configured limit" rather than "no limit".
• apps/api/src/agent/llm/concurrency.test.ts, line 81: the describe block is named "limiterFor" but the function under test is loadLimiter — stale name from an earlier iteration, worth renaming while it's fresh.

[seoes]

@Decipher

Hi. I added a little change to your PR. mostly about configuration on each model provider and db schema. now you can set concurrency limit by each provider

Sorry for take this so long. I'll merge other PRs and release it with new version very soon.

No more token leaks (Thanks to you)

[proval-0e6d07]

⚫️ [PROBLEM] debug import drags DB side effects and a module cycle into the limiter unit test

This push adds import { debug } from "../../util/log.js" at apps/api/src/agent/llm/concurrency.ts:1, and that import pulls a heavy graph into what was a dependency-free module: log.ts:12 imports ActivityService, log.ts:27 instantiates it at top level, activity.service.ts:15 imports db/index.js whose line 5 does new Database(process.env.DB_FILE_NAME ?? "local.db") at module load, and activity.service.ts:17 imports factory.ts, which imports this file (factory.ts:5).

Two consequences: concurrency.test.ts no longer runs in isolation — merely importing the SUT transitively opens/creates a SQLite file — and a module cycle factory → concurrency → log → activity.service → factory now exists with top-level side effects inside it, making evaluation order fragile.

Consider keeping the limiter dependency-free: accept an optional logger callback (factory wires in debug), or copy the tiny NODE_ENV-guarded console.debug helper locally instead of importing util/log.js.

[proval-0e6d07]

Thanks @Decipher — this follow-up adds targeted Limiter debug tracing and renames the leftover test describe block.

a. Summary

The push threads a debug() trace through grant/queue/resume and limiter create/reuse in apps/api/src/agent/llm/concurrency.ts, with a formatMax helper so null/Infinity caps print as unlimited. The logging is dev-only and the traces line up with the limiter's state machine, but the new util/log.js import has a heavier cost than it appears — see the Main Issue below. One naming nit remains in the test rename.

b. Good points

• 🟢 formatMax normalizes both null and non-finite caps to "unlimited" (apps/api/src/agent/llm/concurrency.ts, line 3), so debug output describes the effective bound instead of printing Infinity or null.
• 🟢 Debug output stays out of production: debug() returns early when NODE_ENV === "production" (apps/api/src/util/log.ts, line 72), so the added trace volume is confined to dev/test.

Main Issues

• ⚫️ apps/api/src/agent/llm/concurrency.ts, line 1: importing debug from util/log.js pulls in ActivityService (top-level new ActivityService() at log.ts:27), which imports db/index.js and runs new Database(...) at module load, so importing the SUT in concurrency.test.ts now opens/creates a SQLite file and creates a factory → concurrency → log → activity.service → factory cycle. Fix: keep the limiter dependency-free via an optional logger callback or a local NODE_ENV-guarded console.debug.

Additional Notes

• apps/api/src/agent/llm/concurrency.test.ts, line 82: the describe block was renamed to "loadLimit" but the function under test is loadLimiter — it's still off by the trailing "er", so rename it to match.