feat(background-agent): add model-based concurrency management #548
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🤖 GENERATED WITH ASSISTANCE OF OhMyOpenCode (https://github.com/code-yeongyu/oh-my-opencode)
🤖 GENERATED WITH ASSISTANCE OF [OhMyOpenCode](https://github.com/code-yeongyu/oh-my-opencode)
…anager 🤖 GENERATED WITH ASSISTANCE OF OhMyOpenCode (https://github.com/code-yeongyu/oh-my-opencode)
🤖 GENERATED WITH ASSISTANCE OF [OhMyOpenCode](https://github.com/code-yeongyu/oh-my-opencode)
Setting concurrency to 0 means unlimited (Infinity). Works for defaultConcurrency, providerConcurrency, and modelConcurrency. 🤖 Generated with [OhMyOpenCode](https://github.com/code-yeongyu/oh-my-opencode)
Greptile SummaryAdded model-based concurrency management for background tasks with configurable limits per model, provider, and default fallback. The Major Changes:
Critical Issues Found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant BackgroundManager
participant ConcurrencyManager
participant OpenCodeClient
participant BackgroundTask
User->>BackgroundManager: launch(input)
BackgroundManager->>BackgroundManager: validate agent
BackgroundManager->>ConcurrencyManager: acquire(model)
alt limit not reached
ConcurrencyManager-->>BackgroundManager: resolve immediately
else limit reached
ConcurrencyManager->>ConcurrencyManager: queue promise
Note over ConcurrencyManager: waits in queue
end
BackgroundManager->>OpenCodeClient: session.create()
OpenCodeClient-->>BackgroundManager: sessionID
BackgroundManager->>BackgroundManager: create BackgroundTask
BackgroundManager->>OpenCodeClient: session.promptAsync()
BackgroundManager-->>User: return task
par Task Execution
OpenCodeClient->>BackgroundTask: execute prompt
BackgroundTask-->>OpenCodeClient: completion/error
and Polling
BackgroundManager->>BackgroundManager: poll status
end
alt on completion/error/timeout
BackgroundManager->>BackgroundTask: mark complete
BackgroundManager->>ConcurrencyManager: release(model)
alt queue not empty
ConcurrencyManager->>ConcurrencyManager: resolve next queued promise
else queue empty
ConcurrencyManager->>ConcurrencyManager: decrement count
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (2)
-
src/features/background-agent/manager.ts, line 82-92 (link)logic: If
client.session.createthrows or returns error after acquiring concurrency, the slot is never released (leaks).Wrap in try-catch:
await this.concurrencyManager.acquire(model) try { const createResult = await this.client.session.create({ body: { parentID: input.parentSessionID, title: `Background: ${input.description}`, }, }) if (createResult.error) { throw new Error(`Failed to create background session: ${createResult.error}`) } // ... rest of code } catch (error) { this.concurrencyManager.release(model) throw error }
-
src/index.ts, line 177 (link)logic:
BackgroundManageraccepts a config parameter, but it's not being passed here. Concurrency limits won't work.
7 files reviewed, 3 comments
Comment on lines
+236
to
+238
| defaultConcurrency: z.number().min(1).optional(), | ||
| providerConcurrency: z.record(z.string(), z.number().min(1)).optional(), | ||
| modelConcurrency: z.record(z.string(), z.number().min(1)).optional(), |
There was a problem hiding this comment.
logic: Schema doesn't allow 0 as value, but implementation treats 0 as unlimited. According to PR description and tests, 0 means unlimited concurrency.
Suggested change
| defaultConcurrency: z.number().min(1).optional(), | |
| providerConcurrency: z.record(z.string(), z.number().min(1)).optional(), | |
| modelConcurrency: z.record(z.string(), z.number().min(1)).optional(), | |
| defaultConcurrency: z.number().min(0).optional(), | |
| providerConcurrency: z.record(z.string(), z.number().min(0)).optional(), | |
| modelConcurrency: z.record(z.string(), z.number().min(0)).optional(), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/schema.ts
Line: 236:238
Comment:
**logic:** Schema doesn't allow 0 as value, but implementation treats 0 as unlimited. According to PR description and tests, 0 means unlimited concurrency.
```suggestion
defaultConcurrency: z.number().min(0).optional(),
providerConcurrency: z.record(z.string(), z.number().min(0)).optional(),
modelConcurrency: z.record(z.string(), z.number().min(0)).optional(),
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 7 files
Confidence score: 2/5
- Double releasing the concurrency slot in
src/features/background-agent/manager.ts(catch block plusnotifyParentSession()finally) risks racey state and blocked tasks, so the current behavior isn’t safe to ship. - If
session.create()fails insrc/features/background-agent/manager.ts, the slot is never released, leading to a leak that could eventually halt all queued work. - Both
assets/oh-my-opencode.schema.jsonandsrc/config/schema.tsreject the documented “0 = unlimited concurrency”, so user configs that rely on unlimited agents would break immediately. - Pay close attention to
src/features/background-agent/manager.ts,assets/oh-my-opencode.schema.json,src/config/schema.ts- concurrency slot handling and schema limits must match documented behavior.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/features/background-agent/manager.ts">
<violation number="1" location="src/features/background-agent/manager.ts:82">
P1: Missing concurrency release on session creation failure. If `session.create()` throws or returns an error, the acquired concurrency slot is never released, causing a resource leak that could eventually block all tasks for this model.</violation>
<violation number="2" location="src/features/background-agent/manager.ts:144">
P1: Double release of concurrency slot. This release in the catch block is followed by a call to `notifyParentSession()` which also releases in its finally block. Remove this release since `notifyParentSession()` handles cleanup.</violation>
</file>
<file name="assets/oh-my-opencode.schema.json">
<violation number="1" location="assets/oh-my-opencode.schema.json:1665">
P1: Schema `minimum: 1` contradicts the documented behavior that "0 means unlimited concurrency". Change to `minimum: 0` to allow users to configure unlimited concurrency as described in the PR.</violation>
</file>
<file name="src/config/schema.ts">
<violation number="1" location="src/config/schema.ts:236">
P1: Schema validation contradicts documented behavior. The PR states "0 means unlimited concurrency", but `.min(1)` rejects 0 as invalid. Change to `.min(0)` to allow unlimited concurrency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| existingTask.error = errorMessage | ||
| } | ||
| existingTask.completedAt = new Date() | ||
| if (existingTask.model) { |
There was a problem hiding this comment.
P1: Double release of concurrency slot. This release in the catch block is followed by a call to notifyParentSession() which also releases in its finally block. Remove this release since notifyParentSession() handles cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/manager.ts, line 144:
<comment>Double release of concurrency slot. This release in the catch block is followed by a call to `notifyParentSession()` which also releases in its finally block. Remove this release since `notifyParentSession()` handles cleanup.</comment>
<file context>
@@ -132,6 +141,9 @@ export class BackgroundManager {
existingTask.error = errorMessage
}
existingTask.completedAt = new Date()
+ if (existingTask.model) {
+ this.concurrencyManager.release(existingTask.model)
+ }
</file context>
|
|
||
| const model = input.agent | ||
|
|
||
| await this.concurrencyManager.acquire(model) |
There was a problem hiding this comment.
P1: Missing concurrency release on session creation failure. If session.create() throws or returns an error, the acquired concurrency slot is never released, causing a resource leak that could eventually block all tasks for this model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/manager.ts, line 82:
<comment>Missing concurrency release on session creation failure. If `session.create()` throws or returns an error, the acquired concurrency slot is never released, causing a resource leak that could eventually block all tasks for this model.</comment>
<file context>
@@ -60,19 +62,25 @@ export class BackgroundManager {
+ const model = input.agent
+
+ await this.concurrencyManager.acquire(model)
+
const createResult = await this.client.session.create({
</file context>
| "maximum": 1000 | ||
| }, | ||
| "state_dir": { | ||
| "type": "string" |
There was a problem hiding this comment.
P1: Schema minimum: 1 contradicts the documented behavior that "0 means unlimited concurrency". Change to minimum: 0 to allow users to configure unlimited concurrency as described in the PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At assets/oh-my-opencode.schema.json, line 1665:
<comment>Schema `minimum: 1` contradicts the documented behavior that "0 means unlimited concurrency". Change to `minimum: 0` to allow users to configure unlimited concurrency as described in the PR.</comment>
<file context>
@@ -1658,6 +1658,35 @@
+ "background_task": {
+ "type": "object",
+ "properties": {
+ "defaultConcurrency": {
+ "type": "number",
+ "minimum": 1
</file context>
Suggested change
| "type": "string" | |
| "defaultConcurrency": { | |
| "type": "number", | |
| "minimum": 0 | |
| }, | |
| "providerConcurrency": { | |
| "type": "object", | |
| "propertyNames": { | |
| "type": "string" | |
| }, | |
| "additionalProperties": { | |
| "type": "number", | |
| "minimum": 0 | |
| } | |
| }, | |
| "modelConcurrency": { | |
| "type": "object", | |
| "propertyNames": { | |
| "type": "string" | |
| }, | |
| "additionalProperties": { | |
| "type": "number", | |
| "minimum": 0 | |
| } | |
| } |
Comment on lines
+236
to
+238
| defaultConcurrency: z.number().min(1).optional(), | ||
| providerConcurrency: z.record(z.string(), z.number().min(1)).optional(), | ||
| modelConcurrency: z.record(z.string(), z.number().min(1)).optional(), |
There was a problem hiding this comment.
P1: Schema validation contradicts documented behavior. The PR states "0 means unlimited concurrency", but .min(1) rejects 0 as invalid. Change to .min(0) to allow unlimited concurrency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/config/schema.ts, line 236:
<comment>Schema validation contradicts documented behavior. The PR states "0 means unlimited concurrency", but `.min(1)` rejects 0 as invalid. Change to `.min(0)` to allow unlimited concurrency.</comment>
<file context>
@@ -232,6 +232,12 @@ export const RalphLoopConfigSchema = z.object({
})
+export const BackgroundTaskConfigSchema = z.object({
+ defaultConcurrency: z.number().min(1).optional(),
+ providerConcurrency: z.record(z.string(), z.number().min(1)).optional(),
+ modelConcurrency: z.record(z.string(), z.number().min(1)).optional(),
</file context>
Suggested change
| defaultConcurrency: z.number().min(1).optional(), | |
| providerConcurrency: z.record(z.string(), z.number().min(1)).optional(), | |
| modelConcurrency: z.record(z.string(), z.number().min(1)).optional(), | |
| defaultConcurrency: z.number().min(0).optional(), | |
| providerConcurrency: z.record(z.string(), z.number().min(0)).optional(), | |
| modelConcurrency: z.record(z.string(), z.number().min(0)).optional(), |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ConcurrencyManagerfor model-based rate limitingConfiguration
{ "background_task": { "defaultConcurrency": 5, "providerConcurrency": { "anthropic": 3 }, "modelConcurrency": { "anthropic/claude-sonnet-4-5": 10 } } }Priority: modelConcurrency > providerConcurrency > defaultConcurrency
Summary by cubic
Adds model-based concurrency control to the background agent to cap parallel tasks and reduce provider throttling. Default limit is 5; setting a limit to 0 makes it unlimited.
Written for commit ca4e1a7. Summary will update on new commits.