Fix unlimited task concurrency#3306
Conversation
daandden
force-pushed
the
fix/task-max-concurrency-unlimited
branch
from
35794e2 to
15bfa7a
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35794e283d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - Fixed MCP tool calls forwarding unused optional placeholder arguments to servers, allowing schema-optional fields to be omitted before `tools/call`. | ||
| - Fixed `task.maxConcurrency: 0` serializing subagent spawns instead of disabling the task semaphore limit ([#3305](https://github.com/can1357/oh-my-pi/issues/3305)). |
There was a problem hiding this comment.
These new entries are being added under the already released 16.1.16 section while ## [Unreleased] remains empty. The repo's AGENTS.md changelog rules say new entries always go under [Unreleased] and released sections are immutable; leaving this here mutates historical release notes and means the next release will not pick up this task-concurrency fix in its unreleased notes.
Useful? React with 👍 / 👎.
roboomp
left a comment
roboomp
left a comment
There was a problem hiding this comment.
Thanks for the fix, @daandden — the bug and root cause match #3305 exactly, and the two regression tests pin the contract well (both task.batch sync fan-out and background spawns assert all spawns clear the semaphore before any release). Code change in Semaphore's constructor is correct and handles 0/negative/NaN/Infinity symmetrically; aligns with the task.maxConcurrency: 0 → "Unlimited" UI label and runEvalConcurrency's existing semantics.
Two CHANGELOG fixes before merge:
- The unrelated MCP bullet on line 30 has no corresponding code change in this diff — looks like a stray rebase artifact, please drop it.
- The remaining task-concurrency bullet lands inside the released
## [16.1.16]section; move it to the empty## [Unreleased]block at the top perAGENTS.md.
One nit: the new task-batch.test.ts case races on Bun.sleep(250) while task-spawn.test.ts uses a pollUntil helper — same gate, more CI-robust. Not blocking.
FYI for the maintainer: parallel PR #3307 (by @roboomp) targets the same constructor with the same Infinity-on-non-positive fix. Functionally equivalent — pick one.
|
|
||
| ### Fixed | ||
|
|
||
| - Fixed `task.maxConcurrency: 0` serializing subagent spawns instead of disabling the task semaphore limit ([#3305](https://github.com/can1357/oh-my-pi/issues/3305)). |
There was a problem hiding this comment.
blocking — this bullet ("Fixed MCP tool calls forwarding unused optional placeholder arguments…") has no corresponding code change in the PR diff (git diff origin/main...HEAD only touches parallel.ts plus two test files). Looks like a stray entry from an unrelated branch. Please drop it.
| ### Fixed | ||
|
|
||
| - Fixed `task.maxConcurrency: 0` serializing subagent spawns instead of disabling the task semaphore limit ([#3305](https://github.com/can1357/oh-my-pi/issues/3305)). | ||
|
|
There was a problem hiding this comment.
should-fix — this entry lands inside ## [16.1.16] - 2026-06-23 (header at line 5), which is already a released section. Per AGENTS.md: "Never modify already-released sections (e.g. ## [0.12.2]) — they are immutable. New entries always go under ## [Unreleased]." The [Unreleased] block on line 3 is currently empty — please move the task.maxConcurrency: 0 bullet there.
|
|
||
| const race = await Promise.race([ | ||
| bothStarted.promise.then(() => "started" as const), | ||
| Bun.sleep(250).then(() => "timeout" as const), |
There was a problem hiding this comment.
nit — the sibling task-spawn.test.ts test already imports a pollUntil helper and uses it for "both started". A fixed Bun.sleep(250) race here makes the test flake-prone on slow CI without any benefit; consider polling on started.length === 2 for consistency.
Minor: Deferred/deferred() are now duplicated between task-batch.test.ts (lines 80-88, new) and task-spawn.test.ts (lines 68-76, existing). Same shape in test/memories-runtime.test.ts and test/registry/agent-lifecycle.test.ts too — a shared test/helpers/deferred.ts would avoid the 4th copy, but that's out of scope for this PR.
|
|
||
| constructor(max: number) { | ||
| this.#max = Math.max(1, max); | ||
| const normalizedMax = Number.isFinite(max) ? Math.floor(max) : Number.POSITIVE_INFINITY; |
There was a problem hiding this comment.
Code fix is correct — 0, negatives, NaN, and +Infinity all collapse to POSITIVE_INFINITY; positive finite values get Math.floor'd. Matches the task.maxConcurrency schema's "0" → "Unlimited" UI label (src/config/settings-schema.ts:3816) and the runEvalConcurrency 0 = unbounded contract (src/eval/concurrency-bridge.ts:31-33).
nit — Math.trunc would mirror runEvalConcurrency exactly; Math.floor only differs for negatives, which already fall through to Infinity here, so the runtime behaviour is identical. Not worth changing on its own.
Also note: there's a parallel PR #3307 (by @roboomp) targeting the same constructor with the same Infinity-on-non-positive shape. Functionally equivalent — maintainer's call which lands.
2 participants
What
task.maxConcurrency: 0as unlimited for task spawns.Why
Fixes #3305
Testing
bun --cwd=packages/coding-agent test test/task/task-spawn.test.ts test/task/task-batch.test.ts— 17 pass, 0 fail.bun --cwd=packages/coding-agent run check— fails in pre-existing unrelated type errors:../ai/src/providers/cursor.ts:Message<string>not assignable to generatedagent.v1.*message types; missingmessage,turn,matches,customSystemPrompt.../ai/src/providers/devin.ts:Message<string>missingmessageId,deltaThinking,deltaText,usage,userJwt.src/modes/acp/acp-agent.ts/test/acp-agent.test.ts:@agentclientprotocol/sdkmissing elicitation exports and client capability fields.src/internal-urls/docs-index.generated.tsexceeds configured 1.0 MiB max size.bun checkpasses