feat: limit concurrent chat agents with pooled admission - #27902
feat: limit concurrent chat agents with pooled admission#27902ibetitsmike wants to merge 24 commits into
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
@codex review |
Documentation CheckNew Documentation Needed
Note The auto-generated doc changes in this PR ( Automated review via Coder Agents |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 488948e1e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
DanielleMaywood
left a comment
There was a problem hiding this comment.
Will perform a more thorough review of the backend tomorrow when I'm more awake but the frontend changes are fine with me
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eac94c228
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8959eba6c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05375b8e7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b5a180a2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21e7706a55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review
|
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| @@ -0,0 +1,68 @@ | |||
| ALTER TABLE chats | |||
| ADD COLUMN capacity_queued_at TIMESTAMPTZ; | |||
There was a problem hiding this comment.
If the reason for adding this column is only to be able to show "chat is queued" in the UI, then there is a simpler approach. Assuming max 10 concurrent root chats, you can determine if a root chat is queued because of no capacity by checking that:
- The chat is in a runnable state (see ARCHITECTURE.md, it's defined there)
- The chat has no owner (either
worker_idis NULL or it's not but there's no current heartbeat) - There are 10 root chats with owners (
worker_idnot NULL and non-expired heartbeat).
That's one SQL query with a join.
There was a problem hiding this comment.
The only gotcha there is you can't distinguish between "queued because you reached your limit" and "queued because a worker hasn't picked it up yet". Adding some kind of delay that is a multiple of ChatAcquireInterval would help.
There was a problem hiding this comment.
You can distinguish between those 2. In the first case there are 10 or more claimed and running root chats, in the latter there are fewer.
There was a problem hiding this comment.
Adopted. The column is gone entirely: no migration for it, no view change, no mark/clear queries. Queued state is now derived exactly as you describe, one SQL query (GetChatQueuedForCapacity) checking runnable + unowned (no live owner heartbeat) + pool full, where pool fullness distinguishes a capacity wait from an ordinary not-yet-picked-up chat (resolving Cian's gotcha per your follow-up). The single-chat GET computes it on read; list responses skip it.
Mux replied on Mike's behalf.
| } | ||
| newRows := 0 | ||
| progressed := false | ||
| for _, row := range rows { |
There was a problem hiding this comment.
This is racy unless you synchronize all acquires on a single lock (e.g. an advisory db lock). I'd recommend only running the gatekeeping logic if the deployment doesn't have a valid license. Otherwise the lock will become a bottleneck for deployments with a high chat volume.
There was a problem hiding this comment.
+1; the enterprise admitter should be a no-op essentially.
There was a problem hiding this comment.
Partly adopted. refusedPools is now write-free (it only skips re-admission attempts within one pass), so the worst case of the race is a one-tick (~1s) admission delay corrected by the next acquisition pass.
On the lock: the entitlement check short-circuits before AcquireLock, so licensed deployments with remaining agent runtime hours never take it. Only capped deployments reach the lock, and those are bounded at 5+10 concurrent agents by construction, so the serialized section can't see high volume. Gating on "no valid license" alone would change the PRD policy (CODAGT-845): licensed-without-agent-hours deployments are meant to behave as community, so the gate keys on the agent_runtime_hours feature rather than license presence.
Mux replied on Mike's behalf.
| if row.Status == database.ChatStatusRunning && refusedPools[row.ParentChatID.Valid] { | ||
| if !row.CapacityQueuedAt.Valid { | ||
| progressed = true | ||
| w.markCapacityQueued(ctx, row.ID) |
There was a problem hiding this comment.
I'd advise against manually keeping capacity_queued_at up to date with MarkChatCapacityQueued andClearChatCapacityQueued. It seems prone to getting out of sync because of worker crashes etc. This PR would be simpler and the code more straightforward without capacity_queued_at.
There was a problem hiding this comment.
Adopted. capacity_queued_at, MarkChatCapacityQueued, ClearChatCapacityQueued, and every clear site (UpdateChatExecutionState CASE, archive, auto-archive) are deleted. Queued state is derived from ownership + status + pool fullness, so there is nothing to desync across worker crashes. The only remaining worker-local state is an in-memory map of refused chats that drives capacity_change events and the wait histogram; it is advisory only, and API reads always derive from the database.
Mux replied on Mike's behalf.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| return | ||
| } | ||
| if w.opts.AgentAdmission != nil { | ||
| w.opts.AgentAdmission.RecordQueued() |
There was a problem hiding this comment.
Could this not be done inside the AgentAdmission interface?
Then you get a nice single-method Admitter interface.
There was a problem hiding this comment.
Done, and further: RecordQueued is deleted outright (the exactly-once queue-entry counter needed the persisted marker to be exact; its information now lives in the wait histogram's count). AgentAdmission is the single-method Admit interface. Queue metrics moved to the OSS worker, which is the component that actually observes refusals and admissions.
Mux replied on Mike's behalf.
| } | ||
| newRows := 0 | ||
| progressed := false | ||
| for _, row := range rows { |
There was a problem hiding this comment.
+1; the enterprise admitter should be a no-op essentially.
| } | ||
|
|
||
| // AgentAdmissionFactory builds an admission gate. A nil factory leaves capacity uncapped. | ||
| type AgentAdmissionFactory func(AgentAdmissionOptions) AgentAdmission |
There was a problem hiding this comment.
This almost looks like functional options. The non-negotiables are LifetimeCtx and Store. Registerer, Logger, and HeartbeatStaleSeconds can get sensible default values.
There was a problem hiding this comment.
Resolved by dissolution: with metrics moved to the OSS worker, the enterprise implementation no longer needs Store, Logger, Clock, Registerer, or LifetimeCtx. The options struct is gone; the factory is now NewAgentAdmissionFactory(entitlements) and receives only heartbeatStaleSeconds from chatd at construction.
Mux replied on Mike's behalf.
| type AgentAdmission interface { | ||
| // Admit reports whether the worker may acquire the chat. Refused chats | ||
| // remain unowned and are retried from the capacity queue. | ||
| Admit(ctx context.Context, store database.Store, chat database.Chat) (bool, error) |
There was a problem hiding this comment.
Can we drop the database.Store from the Admit() signature here? The AgentAdmission should already have one available to use from AgentAdmissionOptions. Thinking down the line when we carve out a dRPC interface.
There was a problem hiding this comment.
Kept deliberately, with a comment documenting why: the store parameter is the acquisition transaction. AcquireLock is pg_advisory_xact_lock (transaction-scoped), so the count + lock must run inside the same transaction as the ownership write for the serialization to hold until commit; an admission gate holding its own store reopens the double-admit race. For a future dRPC carve-out the seam would need to move the whole claim (admit + acquire) behind the interface rather than just the check.
Mux replied on Mike's behalf.
| @@ -0,0 +1,68 @@ | |||
| ALTER TABLE chats | |||
| ADD COLUMN capacity_queued_at TIMESTAMPTZ; | |||
There was a problem hiding this comment.
The only gotcha there is you can't distinguish between "queued because you reached your limit" and "queued because a worker hasn't picked it up yet". Adding some kind of delay that is a multiple of ChatAcquireInterval would help.
Add a two-pool concurrency cap for chatd agents, enforced when a worker acquires chat ownership: at most 5 root chats and 10 subagent chats generate concurrently on unlicensed deployments. Refused chats stay running but unowned, stamped with capacity_queued_at for FIFO admission and a queued banner in the UI. Deployments with remaining licensed agent runtime hours are uncapped. The admission policy lives in enterprise/coderd/x/chatd; OSS carries only the AgentAdmission seam invoked inside the acquisition transaction. Active use is counted from ownership plus fresh chat_heartbeats, so slots self-free when a replica crashes or a chat leaves running/interrupting. Interrupting chats bypass the cap so over-cap users can stop their own chats.
…dmission Each guard was verified red-green: dropping the capacity_queued_at ORDER BY key fails the FIFO test, refusing interrupting chats fails the interrupt claim test, and removing the advisory lock over-admits in the concurrency test.
… query Sort interrupting chats ahead of the capacity queue so a full-pool backlog cannot delay a stop request, make the exclude_ids predicate nil-safe, and query the capacity callout story by its alert role.
…ests A full root pool with a deep queued backlog previously buried subagent candidates behind it: acquisition retried every queued root before reaching an admittable subagent. Candidates now interleave across the two pools with FIFO order inside each, and one refusal marks a pool full for the rest of the pass so later chats in it are queue-marked without an acquisition transaction. Also adds the missing dbauthz method-suite tests for the four new capacity queries and renames capacityCounted to occupiesCapacitySlot per review.
The acquisition query interleaves the two capacity pools, and the no-progress pass termination is only sound when every batch can hold both pools' oldest candidates. A configured batch size of one surfaces only the tie-break-favored root pool: with two already-marked queued roots the pass refuses one, re-skips the other, and ends with the subagent pool unexamined.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7300dcc2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Replica A can refuse a chat and publish the queued capacity event while replica B later admits it. B has no local refusal entry, so gating the clear event on the local capacity queue left the banner stale until the next refetch. On capped deployments, admission of a running chat now always publishes the clear. Also assert FIFO admission on the serial Admit call order recorded by fakeAdmission instead of task-start order, which goroutine scheduling can reorder, and document the requires_action resume overage bound.
Review feedback from cian: AgentAdmission and AgentCapacityPolicy always shipped as the same object, so replace them with a single AgentCapacityLimiter interface (Admit + Limits). Limits returns the caps plus a capped bool instead of a Capped field. A noop limiter replaces nil checks; chatd wires it whenever no factory is configured. Consolidate CountChatCapacityActiveByPool and CountChatCapacityUnownedByPool into CountChatCapacityByPool so admission and the metrics loop share one scan, and widen idx_chats_capacity_active to cover unowned chats so the merged query stays indexed.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 546e293293
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ents Codex review findings on 546e293: - An interrupting chat acquired by a replica that never refused it kept the queued banner because the clear only covered running acquisitions. Interrupting acquisitions now publish the clear too. - A delayed queued capacity event could arrive after another replica's clear and restore the banner. Acquisition bumps updated_at while refusals do not, so the frontend merge now applies capacity events only when the snapshot is not older than the cache. Also add the concurrent-agents docs section and point the queued banner's learn-more links at it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f479526e34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex found the cap can flip to uncapped (license update) between one replica's refusal, which published queued=true, and another replica's admission, which then read capped=false and skipped the clear, leaving a stale banner. The clear condition now depends only on whether a real limiter is configured: the noop limiter never refuses, so no queued event can exist, while any real limiter may have refused while capped. No dynamic state can desync the clear from the queued event anymore.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e7c35ee66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pshot Codex found the skipped-pool path published queued=true from a stale candidate row: another replica can acquire the chat between the batch query and the publish, and because the event payload reloaded the chat, it carried the acquisition's new updated_at and defeated the frontend freshness guard when delivered after the owner's clear. enterCapacityQueue now validates and publishes from one snapshot: it only queues chats that are still running, unarchived, and without a live owner heartbeat, and the event payload is that same snapshot, so a stale queued event either never publishes or carries pre-acquisition updated_at that loses to the clear. Crashed-owner takeovers still queue because their heartbeat is stale.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5fcb03a20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…kipped passes Codex found prune starvation: with a sustained full-pool backlog every acquisition pass ends on an all-skipped batch, which returned before pruneCapacityQueue could run, so an entry for a chat another replica acquired persisted indefinitely, suppressing the next episode's queued event and measuring wait time from the previous episode. An all-skipped pass now reconciles the local entries it did not see with one targeted FilterChatCapacityWaiting query and drops those that stopped being capacity-wait candidates. One bounded query per pass, instead of paging the global candidate stream, which under churn had no termination bound.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6796c34075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
An all-skipped acquisition pass ended before reaching chats sorted behind the known queue heads, so a chat arriving on a saturated pool never got its queued event and open tabs kept showing plain running state. Reconcile now lists all waiting chats once per all-skipped pass, pruning departed entries and queueing arrivals in pools the pass proved full.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18e4af8d75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…races Two races found by review: Resuming a chat from waiting or error during the runner's abandon gap preserved worker_id/runner_id, so generation restarted on the retained runner without capacity admission while the slot release may have already admitted a queued chat. Idle-state resumes now clear ownership, routing the resume through worker acquisition and re-admission. A capacity clear published at acquisition could be rejected by the frontend freshness guard when a newer status event advanced the cached updated_at first, leaving the queued banner stuck on a generating chat. Capacity events now order against a per-chat capacity revision (the newest capacity event's updated_at) instead of the general guard.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afd69ab78a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ity invalidation The ordering guards can reject a legitimate capacity event when delivery reorders against message sends that bump updated_at (for example a queued event loaded before but delivered after a send to the same still-unowned chat). The worker keeps its local queue entry and emits no replacement event, so the banner would stay wrong until an unrelated refetch. After merging a capacity event into the caches, invalidate the chat entity when the merged result does not reflect the event; the single-chat GET derives the authoritative queued state.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Caps how many chat agents can generate concurrently on unlicensed deployments: at most 5 root chats and 10 subagent chats, enforced as two independent deployment-wide pools. Deployments with remaining licensed agent runtime hours (
agent_runtime_hours) are uncapped.Problem
Nothing bounds concurrent chat generation today. A community deployment can start unlimited simultaneous agents, and the cost of that generation has no license-tied limit.
How it works
enterprise/coderd/x/chatd; OSS carries only anAgentCapacityLimiterseam (AdmitplusLimits), with a noop limiter that admits everything when no factory is configured (AGPL builds stay uncapped).running/interrupting, owned, and its runner heartbeat is fresh. Slots held by crashed replicas free automatically once heartbeats go stale.requires_actionchats keep their runner but do not count.runningbut unowned: no runner, no parked goroutine, no column.capacity_changewatch events (banner shows and clears live) and the wait-time histogram. It is a per-replica approximation; API reads always derive from the database.wait_agentkeep their root slot; children draw from the separate subagent pool. Subagents cannot nest, so the subagent pool cannot deadlock on itself.coderd_chatd_agents_activeandcoderd_chatd_agents_queued_for_capacity(per pool, database-derived), plus thecoderd_chatd_agent_capacity_wait_secondshistogram.Accepted approximations
updated_at, so sending a message to a queued chat re-queues it at the back.capacity_changepubsub event leaves a stale banner until the next single-chat refetch.Validation
updated_atincluding the message re-queue caveat, queued-event dedup, queue pruning on archive, interrupt claim under a full pool, worker paging past refused candidates, and a concurrent-admission race test. New guards verified red-green.