-
Notifications
You must be signed in to change notification settings - Fork 0
fix(daemon): queue limit, discovery coalescing, body release #100
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| import { describe, expect, it, beforeEach, afterEach, vi } from "bun:test"; | ||
| import { Database } from "bun:sqlite"; | ||
| import fs from "node:fs"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import { BunSessionRepository } from "../src/host/bun-session-repository"; | ||
| import { DaemonConfigPresenter } from "../src/host/daemonConfigPresenter"; | ||
| import { AcpLaunchSpecService } from "@argos/acp-runtime/config/acpLaunchSpecService"; | ||
|
|
||
| /** | ||
| * Hygiene sweep (docs/issues/upstream-hygiene-sweep): | ||
| * - pending-input queue limit raised to 10 (DeepChat #2236) | ||
| * - concurrent model discovery coalesces into one upstream request (#2248) | ||
| * - failed downloads release the response body before throwing (#2251) | ||
| */ | ||
|
|
||
| describe("pending input queue limit", () => { | ||
| let repo: BunSessionRepository; | ||
| let db: Database; | ||
|
|
||
| beforeEach(() => { | ||
| db = new Database(":memory:"); | ||
| repo = new BunSessionRepository(db as never); | ||
| db.run( | ||
| `INSERT INTO daemon_sessions (id, agent_id, title, status, generation_status, created_at, updated_at) | ||
| VALUES (?, 'argos', 'Test', 'idle', 'idle', ?, ?)`, | ||
| ["session-1", Date.now(), Date.now()], | ||
| ); | ||
| }); | ||
|
|
||
| it("accepts a queue of 10 and rejects the 11th", async () => { | ||
| for (let index = 0; index < 10; index += 1) { | ||
| await expect( | ||
| repo.queuePendingInput("session-1", `message ${index}`, { source: "queue" } as never), | ||
| ).resolves.toBeTruthy(); | ||
| } | ||
|
|
||
| await expect(repo.queuePendingInput("session-1", "message 11", { source: "queue" } as never)).rejects.toThrow( | ||
| "Pending input limit reached", | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("model discovery coalescing", () => { | ||
| const originalFetch = globalThis.fetch; | ||
| const roots: string[] = []; | ||
|
|
||
| afterEach(() => { | ||
| globalThis.fetch = originalFetch; | ||
| while (roots.length > 0) { | ||
| const root = roots.pop(); | ||
| if (root) fs.rmSync(root, { recursive: true, force: true }); | ||
| } | ||
| }); | ||
|
|
||
| const tempRoot = (): string => { | ||
| const dir = fs.mkdtempSync(path.join(os.tmpdir(), "argos-hygiene-")); | ||
| roots.push(dir); | ||
| return dir; | ||
| }; | ||
|
|
||
| it("coalesces concurrent refreshes for the same provider into one upstream call", async () => { | ||
| const dataDir = tempRoot(); | ||
| const presenter = new DaemonConfigPresenter(dataDir, dataDir); | ||
| presenter.setProviders([ | ||
| { | ||
| id: "prov-1", | ||
| name: "Coalesced Provider", | ||
| type: "openai", | ||
| apiType: "openai", | ||
| baseUrl: "https://models.example.invalid/v1", | ||
| apiKey: "sk-test", | ||
| } as never, | ||
| ]); | ||
|
|
||
| let upstreamCalls = 0; | ||
| globalThis.fetch = (async () => { | ||
| upstreamCalls += 1; | ||
| await new Promise((resolve) => setTimeout(resolve, 25)); | ||
| return new Response(JSON.stringify({ data: [{ id: "model-a", name: "Model A" }] }), { | ||
| status: 200, | ||
| headers: { "content-type": "application/json" }, | ||
| }); | ||
| }) as typeof fetch; | ||
|
|
||
| const [first, second, third] = await Promise.all([ | ||
| presenter.refreshProviderModels("prov-1"), | ||
| presenter.refreshProviderModels("prov-1"), | ||
| presenter.refreshProviderModels("prov-1"), | ||
| ]); | ||
|
|
||
| expect(upstreamCalls).toBe(1); | ||
| expect(first.map((model) => model.id)).toEqual(["model-a"]); | ||
| expect(second).toBe(first); | ||
| expect(third).toBe(first); | ||
|
|
||
| // After the coalesced call settles, a new call is a fresh refresh. | ||
| const fourth = await presenter.refreshProviderModels("prov-1"); | ||
| expect(upstreamCalls).toBe(2); | ||
| expect(fourth).not.toBe(first); | ||
| }); | ||
|
|
||
| it("does not coalesce different providers", async () => { | ||
| const dataDir = tempRoot(); | ||
| const presenter = new DaemonConfigPresenter(dataDir, dataDir); | ||
| presenter.setProviders([ | ||
| { | ||
| id: "prov-1", | ||
| name: "P1", | ||
| type: "openai", | ||
| apiType: "openai", | ||
| baseUrl: "https://a.example.invalid/v1", | ||
| apiKey: "k", | ||
| } as never, | ||
| { | ||
| id: "prov-2", | ||
| name: "P2", | ||
| type: "openai", | ||
| apiType: "openai", | ||
| baseUrl: "https://b.example.invalid/v1", | ||
| apiKey: "k", | ||
| } as never, | ||
| ]); | ||
|
|
||
| let upstreamCalls = 0; | ||
| globalThis.fetch = (async () => { | ||
| upstreamCalls += 1; | ||
| return new Response(JSON.stringify({ data: [{ id: "m" }] }), { | ||
| status: 200, | ||
| headers: { "content-type": "application/json" }, | ||
| }); | ||
| }) as typeof fetch; | ||
|
|
||
| await Promise.all([presenter.refreshProviderModels("prov-1"), presenter.refreshProviderModels("prov-2")]); | ||
| expect(upstreamCalls).toBe(2); | ||
| }); | ||
| }); | ||
|
|
||
| describe("download archive response release", () => { | ||
| const originalFetch = globalThis.fetch; | ||
|
|
||
| afterEach(() => { | ||
| globalThis.fetch = originalFetch; | ||
| }); | ||
|
|
||
| it("cancels the error body and throws on a failed archive download", async () => { | ||
| const registryRoot = fs.mkdtempSync(path.join(os.tmpdir(), "argos-hygiene-acp-")); | ||
| const service = new AcpLaunchSpecService(registryRoot); | ||
| const cancel = vi.fn(async () => undefined); | ||
|
|
||
| globalThis.fetch = (async () => { | ||
| return { | ||
| ok: false, | ||
| status: 500, | ||
| statusText: "Internal Server Error", | ||
| body: { cancel }, | ||
| } as unknown as Response; | ||
| }) as typeof fetch; | ||
|
|
||
| await expect( | ||
| (service as any).downloadArchive("https://mirror.example.invalid/agent.zip", { id: "agent-1" }), | ||
| ).rejects.toThrow("Failed to download archive"); | ||
| expect(cancel).toHaveBeenCalled(); | ||
|
|
||
| fs.rmSync(registryRoot, { recursive: true, force: true }); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Plan: Upstream hygiene sweep | ||
|
|
||
| 1. `apps/daemon/src/host/bun-session-repository.ts`: `MAX_ACTIVE_PENDING_INPUTS` 5 → 10. | ||
| 2. `apps/daemon/src/host/daemonConfigPresenter.ts`: add | ||
| `inFlightModelRefreshes: Map<string, Promise<MODEL_META[]>>` wrapping `refreshProviderModels`, | ||
| keyed by a provider settings fingerprint (id + apiType + baseUrl + apiKey) so a mid-flight | ||
| settings change starts a fresh discovery instead of returning results fetched with stale | ||
| credentials, and `inFlightOllamaFetches: Map<string, Promise<OllamaModel[]>>` | ||
| (providerId+path key) wrapping `fetchOllamaModels`. Entries delete on settle; failures | ||
| propagate to every waiter and clear the key (next call retries). | ||
| 3. Release unconsumed fetch error bodies (best-effort `response.body?.cancel()`) in: | ||
| - `packages/acp-runtime/src/config/acpLaunchSpecService.ts` (downloadArchive), | ||
| - `packages/mcp-runtime/src/config/mcprouterManager.ts` (list + get), | ||
| - `packages/backend-core/src/provider/providerDbLoader.ts` (refresh), | ||
| - `packages/acp-runtime/src/config/acpRegistryService.ts` (icon fetch, found during the sweep). | ||
| Done inline per file — no new cross-package dependency for a three-line helper. | ||
| 4. Tests: pending-input limit (daemon), refresh coalescing (daemon), downloadArchive body | ||
| release (daemon test dir hosts it against the exported class). |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| # Spec: Upstream hygiene sweep (queue limit, model-discovery coalescing, response-body release) | ||
|
|
||
| Three small, independent fixes identified while sweeping recent DeepChat work | ||
| (#2236, #2248, #2251 — plus #1946 assessed as already-satisfied). | ||
|
|
||
| ## 1. Pending-input queue limit 5 → 10 (DeepChat #2236) | ||
|
|
||
| `MAX_ACTIVE_PENDING_INPUTS` in `bun-session-repository.ts` caps queued messages per session at | ||
| 5. Users who queue several messages during a long agent turn hit the limit with a bare error. | ||
| Raise the cap to 10 (matching upstream's post-fix behavior) and add regression coverage that 10 | ||
| succeed and the 11th rejects. | ||
|
|
||
| ## 2. Coalesce model discovery requests (DeepChat #2248) | ||
|
|
||
| Two daemon surfaces fetch model lists over the network with no in-flight dedup, so N concurrent | ||
| UI callers (model picker, model store init, per-agent config surfaces) produce N identical | ||
| upstream requests: | ||
|
|
||
| - `DaemonConfigPresenter.refreshProviderModels(providerId)` — the OpenAI-compatible `/models` | ||
| fetch, lazily triggered by `models.getProviderCatalog` whenever a provider has credentials but | ||
| no stored catalog yet; | ||
| - `DaemonConfigPresenter.fetchOllamaModels` via `listOllamaModels`/`listOllamaRunningModels` — | ||
| `/api/tags` and `/api/ps` per Ollama provider. | ||
|
|
||
| Coalesce with a per-key in-flight promise map (keyed by providerId, and providerId+path for | ||
| Ollama): concurrent callers share one upstream request; the map entry clears on settle so the | ||
| next call after completion is a fresh refresh. | ||
|
|
||
| ## 3. Release unconsumed fetch response bodies (DeepChat #2251) | ||
|
|
||
| Five sites throw/return on `!response.ok` without consuming the error body, which keeps the | ||
| underlying socket busy until GC: | ||
|
|
||
| - `AcpLaunchSpecService.downloadArchive` (`packages/acp-runtime`); | ||
| - `mcprouterManager` list + get (`packages/mcp-runtime`); | ||
| - `providerDbLoader` refresh (`packages/backend-core`); | ||
| - `AcpRegistryService` icon fetch (`packages/acp-runtime`, found during the sweep). | ||
|
|
||
| Fix: cancel the body (`response.body?.cancel()`, best-effort try/catch) before each | ||
| error return. Success paths already consume. Done inline per file — no new cross-package | ||
| dependency for a three-line helper. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - No UI changes (the pending-input lane already reflects live queue contents; no rendered | ||
| static limit text exists to update). | ||
| - No changes to the ACP registry refresh or Ollama pull (both already consume bodies). | ||
|
|
||
| ## Tests | ||
|
|
||
| - Daemon: pending-input limit test (10 succeed, 11th throws) against the in-memory SQLite repo. | ||
| - Daemon: `refreshProviderModels` coalescing test — two concurrent calls with a counting fetch | ||
| stub produce exactly one upstream request and identical results. | ||
| - acp-runtime: `downloadArchive` non-ok response releases the body (cancel spy) and throws. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| # Tasks: Upstream hygiene sweep | ||
|
|
||
| - [x] T1 Pending-input queue limit 5 → 10 + limit regression test (10 OK, 11th rejects) | ||
| - [x] T2 Coalesce `refreshProviderModels` (per provider) and Ollama tag/ps lookups (per | ||
| provider+suffix) + coalescing tests (same-provider → 1 upstream call; different providers | ||
| → separate calls; post-settle calls refresh) | ||
| - [x] T3 Release unconsumed fetch error bodies: `downloadArchive`, `mcprouterManager` list+get, | ||
| `providerDbLoader` refresh, `acpRegistryService` icon fetch + downloadArchive release test | ||
| - [x] T4 `bun run format` + `bun run lint` + `bun run typecheck` + `bun run test` | ||
|
|
||
| ## Verification results | ||
|
|
||
| - Daemon: 409 tests pass (4 new); `tsc --noEmit` clean. | ||
| - Desktop + UI: typechecks clean; full `bun run test` green. | ||
| - `bun run lint`: all architecture guards + oxlint clean. |
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
The specification describes four response-body release sites but omits the
AcpRegistryServiceicon-fetch path changed by this PR. The implementation plan also lists only the other packages, while the task list includes this fifth site. Update the SDD documents so the recorded scope and verification plan cover every implementation change.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!