fix(daemon): queue limit, discovery coalescing, body release - #100
Conversation
Three small fixes from a DeepChat upstream sweep (#2236, #2248, #2251): - raise the per-session pending-input queue limit from 5 to 10 so users can line up several messages during long agent turns - coalesce concurrent model discovery: refreshProviderModels shares one upstream /models request per provider and Ollama tag/ps lookups share one request per provider+suffix instead of fanning out per caller - release unconsumed fetch response bodies on error paths (ACP archive download, McpRouter list/get, provider DB refresh, registry icon) so failed requests return their connections to the pool SDD: docs/issues/upstream-hygiene-sweep
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 4/5The PR should not merge until model-discovery coalescing accounts for provider configuration changes so obsolete requests cannot populate or serve the current provider catalog. Coalescing by provider ID alone allows a refresh started under old credentials or an old endpoint to satisfy a post-update caller and, for general provider discovery, persist stale models onto the updated provider. Files Needing Attention: apps/daemon/src/host/daemonConfigPresenter.ts; docs/issues/upstream-hygiene-sweep/spec.md
|
| Filename | Overview |
|---|---|
| apps/daemon/src/host/bun-session-repository.ts | Raises the active pending-input limit from five to ten. |
| apps/daemon/src/host/daemonConfigPresenter.ts | Adds provider and Ollama in-flight request coalescing, but configuration changes can reuse and persist results from obsolete requests. |
| apps/daemon/test/upstreamHygieneSweep.test.ts | Adds focused regression tests for the queue limit, provider refresh coalescing, and archive response cancellation. |
| packages/acp-runtime/src/config/acpLaunchSpecService.ts | Best-effort cancels failed archive-download response bodies before throwing. |
| packages/acp-runtime/src/config/acpRegistryService.ts | Best-effort cancels failed icon-fetch response bodies before throwing. |
| packages/backend-core/src/provider/providerDbLoader.ts | Cancels non-successful provider-database response bodies before returning an error result. |
| packages/mcp-runtime/src/config/mcprouterManager.ts | Cancels failed list and get response bodies before propagating HTTP errors. |
| docs/issues/upstream-hygiene-sweep/spec.md | Documents the hygiene sweep but omits the newly changed ACP registry icon-fetch path from its stated scope. |
| docs/issues/upstream-hygiene-sweep/plan.md | Plans the fixes but does not include the ACP registry icon-fetch change. |
| docs/issues/upstream-hygiene-sweep/tasks.md | Records all completed implementation and verification tasks, including the registry icon path. |
Prompt To Fix All With AI
### Issue 1
apps/daemon/src/host/daemonConfigPresenter.ts:653-655
**Stale model discovery results**
If a provider's URL, credentials, or type changes while model discovery is still running, the next refresh joins the existing promise because the map is keyed only by `providerId`. That request used the old provider settings, but its result is then saved to the current provider, leaving the updated configuration with a stale or incorrect model catalog. Ollama lookups have the same issue and can return old `/api/tags` or `/api/ps` results after an endpoint or credential change.
### Issue 2
docs/issues/upstream-hygiene-sweep/spec.md:26-35
**Documented scope omits fix**
The specification describes four response-body release sites but omits the `AcpRegistryService` icon-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.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(daemon): queue limit, discovery coal..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const inFlight = this.inFlightModelRefreshes.get(providerId); | ||
| if (inFlight) { | ||
| return inFlight; |
There was a problem hiding this comment.
If a provider's URL, credentials, or type changes while model discovery is still running, the next refresh joins the existing promise because the map is keyed only by providerId. That request used the old provider settings, but its result is then saved to the current provider, leaving the updated configuration with a stale or incorrect model catalog. Ollama lookups have the same issue and can return old /api/tags or /api/ps results after an endpoint or credential change.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/daemon/src/host/daemonConfigPresenter.ts
Line: 653-655
Comment:
**Stale model discovery results**
If a provider's URL, credentials, or type changes while model discovery is still running, the next refresh joins the existing promise because the map is keyed only by `providerId`. That request used the old provider settings, but its result is then saved to the current provider, leaving the updated configuration with a stale or incorrect model catalog. Ollama lookups have the same issue and can return old `/api/tags` or `/api/ps` results after an endpoint or credential change.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| 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) | ||
|
|
||
| Four 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`); |
There was a problem hiding this comment.
The specification describes four response-body release sites but omits the AcpRegistryService icon-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
This is a comment left during a code review.
Path: docs/issues/upstream-hygiene-sweep/spec.md
Line: 26-35
Comment:
**Documented scope omits fix**
The specification describes four response-body release sites but omits the `AcpRegistryService` icon-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.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
Address reviewer findings on the hygiene sweep: - key model-discovery coalescing on the provider settings fingerprint (id + apiType + baseUrl + apiKey) instead of providerId alone, so a mid-flight settings change starts fresh discovery rather than joining results fetched with stale credentials - record the registry icon fetch as a fifth response-body release site in the SDD spec and plan (the task list already had it)
Summary
Three small, independent fixes from a sweep of recent DeepChat work (#2236, #2248, #2251). Also assessed #1946 (global trace-scan fix) — not needed: Argos' message reads are already trace-free (
daemon_messagessingle indexed read, denormalizedtrace_count, traces only read per-message in diagnostics), and #2240's tape-projection series doesn't apply because Argos never adopted the fact-store architecture those bugs live in.Changes
1. Pending-input queue limit 5 → 10 (DeepChat #2236)
MAX_ACTIVE_PENDING_INPUTSraised so users can line up more messages during long agent turns. The 11th queued message still fails with the existing "Pending input limit reached" error. Regression test fills 10 (all succeed) and asserts the 11th rejects.2. Coalesce model discovery (DeepChat #2248)
Two daemon surfaces fetched model lists with no in-flight dedup, so concurrent UI callers (model picker, model store init, per-agent config surfaces) each produced an identical upstream request:
refreshProviderModels— now coalesced per provider: concurrent callers share one/modelsfetch; the entry clears on settle so a later call is a genuine refresh.listOllamaModels/listOllamaRunningModels— coalesced per provider+suffix (/api/tags,/api/ps).3. Release unconsumed fetch response bodies (DeepChat #2251)
Five error paths threw/returned without consuming the response body, keeping the underlying connection busy until GC:
AcpLaunchSpecService.downloadArchivemcprouterManagerlist + getproviderDbLoaderrefreshacpRegistryServiceicon fetchAll now cancel the body (
response.body?.cancel(), best-effort) before the error return. Success paths already consumed.Testing
bun run testgreen; lint guards + oxlint clean; format clean.