-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix unlimited task concurrency #3306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,8 @@ | |
|
|
||
| ### 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)). | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should-fix — this entry lands inside |
||
| - Fixed `local://` URLs decoding images as corrupted text (mojibake) instead of showing the image | ||
| - Fixed `omp --resume` hanging instead of exiting when the startup session picker is cancelled (Esc) or there are no sessions to resume. Startup arms long-lived handles (theme/appearance listeners, settings save timer, model registry), so the cancel/empty paths' bare `return` left the event loop alive and the process stuck after the picker cleared the alternate screen. These paths now exit cleanly via `process.exit(0)`, matching the `--version`/`--export` early-exit convention. The in-session `/resume` picker is unaffected — it keeps its own cancel handler that just closes the overlay. | ||
| - Fixed the `/resume` session picker scrolling down after a session is deleted. The delete-confirmation dialog was mounted as a sibling below the picker's bottom border, briefly growing the picker past the terminal height; the TUI committed the picker's header rows into native scrollback to fit, and when the dialog closed `windowTop` stayed pinned at the new commit boundary, leaving the header stranded above the viewport. The picker now hosts the `SessionList` in a single content slot and swaps the dialog INTO that slot (replacing the `SessionList`) while it is open, so the dialog only competes with the `SessionList`'s rendered budget — not the `SessionList` AND the picker chrome — and the picker frame stays inside the viewport. ([#3283](https://github.com/can1357/oh-my-pi/issues/3283)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -92,7 +92,8 @@ export class Semaphore { | |
| #queue: Array<() => void> = []; | ||
|
|
||
| constructor(max: number) { | ||
| this.#max = Math.max(1, max); | ||
| const normalizedMax = Number.isFinite(max) ? Math.floor(max) : Number.POSITIVE_INFINITY; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code fix is correct — nit — Also note: there's a parallel PR #3307 (by |
||
| this.#max = normalizedMax > 0 ? normalizedMax : Number.POSITIVE_INFINITY; | ||
| } | ||
|
|
||
| async acquire(): Promise<void> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| * item; with `async.enabled=false`, it blocks and returns merged results. | ||
| * Both modes forward the shared `context`; the flat form stays accepted at | ||
| * runtime for internal callers. | ||
| * 4. task.maxConcurrency=0 preserves full-width synchronous batch fan-out. | ||
| */ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from "bun:test"; | ||
| import { toolWireSchema } from "@oh-my-pi/pi-ai/utils/schema"; | ||
|
|
@@ -76,6 +77,16 @@ function makeResult(id: string, overrides: Partial<SingleResult> = {}): SingleRe | |
| }; | ||
| } | ||
|
|
||
| interface Deferred { | ||
| promise: Promise<void>; | ||
| resolve: () => void; | ||
| } | ||
|
|
||
| function deferred(): Deferred { | ||
| const { promise, resolve } = Promise.withResolvers<void>(); | ||
| return { promise, resolve }; | ||
| } | ||
|
|
||
| function mockDiscovery(): void { | ||
| vi.spyOn(discoveryModule, "discoverAgents").mockResolvedValue({ | ||
| agents: [taskAgent], | ||
|
|
@@ -364,4 +375,48 @@ describe("task.batch spawning", () => { | |
| "# Goal\nShared synchronous context.", | ||
| ]); | ||
| }); | ||
|
|
||
| it("treats task.maxConcurrency 0 as unlimited for synchronous batch fan-out", async () => { | ||
| mockDiscovery(); | ||
| const bothStarted = deferred(); | ||
| const release = deferred(); | ||
| const started: string[] = []; | ||
| vi.spyOn(executorModule, "runSubprocess").mockImplementation(async options => { | ||
| const id = options.id ?? "?"; | ||
| started.push(id); | ||
| if (started.length === 2) bothStarted.resolve(); | ||
| await release.promise; | ||
| return makeResult(id); | ||
| }); | ||
|
|
||
| const manager = createManager(); | ||
| const tool = await TaskTool.create( | ||
| createSession({ | ||
| manager, | ||
| settings: { "async.enabled": false, "task.batch": true, "task.maxConcurrency": 0 }, | ||
| }), | ||
| ); | ||
|
|
||
| const pending = tool.execute("tc-sync-unlimited", { | ||
| agent: "task", | ||
| context: "# Goal\nShared synchronous context.", | ||
| tasks: [ | ||
| { id: "Alpha", assignment: "Do A." }, | ||
| { id: "Beta", assignment: "Do B." }, | ||
| ], | ||
| } as TaskParams); | ||
|
|
||
| const race = await Promise.race([ | ||
| bothStarted.promise.then(() => "started" as const), | ||
| Bun.sleep(250).then(() => "timeout" as const), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit — the sibling Minor: |
||
| ]); | ||
| expect(race).toBe("started"); | ||
| expect([...started].sort()).toEqual(["Alpha", "Beta"]); | ||
|
|
||
| release.resolve(); | ||
| const result = await pending; | ||
| expect(getFirstText(result)).toContain("All done."); | ||
| expect(result.details?.async).toBeUndefined(); | ||
| expect(result.details?.results.map(item => item.id).sort()).toEqual(["Alpha", "Beta"]); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...HEADonly touchesparallel.tsplus two test files). Looks like a stray entry from an unrelated branch. Please drop it.