perf: memoize provider CLI status, discovery, and model-list failures (S1, S4, S2) - #2347
Open
SawyerHood wants to merge 9 commits into
Open
perf: memoize provider CLI status, discovery, and model-list failures (S1, S4, S2)#2347SawyerHood wants to merge 9 commits into
SawyerHood wants to merge 9 commits into
Conversation
Every provider roster read (/system/providers, /providers/state, /execution-options, usage limits, CLI status) sent provider.health to the host for each visibility:"installed" registration with no cache, so idle bridges were respawned on every request (30 ms warm, 400 ms-1.3 s cold). Add a 5-minute AsyncTtlMemo (installedProviderProbe) keyed by host, daemon session, registration revision, and the full command; only the installed-or-not boolean is stored and 502/504 handling stays outside the memo. The provider-clis/install route clears it. Readiness in getProviderState stays live. Verified: turbo typecheck @bb/server; vitest execution-options, public-provider-installations, provider-states, provider-routing (53 passed). The three count-based new tests fail with the memo bypassed (8/12/8 probes instead of 4). Co-Authored-By: Claude <noreply@anthropic.com>
AsyncTtlMemo dropped every rejection, so with a provider CLI installed but not logged in (or missing) each execution-options read re-spawned it and failed again (847 ms per thread open). Add an optional failures window to AsyncTtlMemo (ttl + predicate, two-arg then so only the task's own rejection is stored). providerModelList memoizes 502/504 ApiErrors other than host_unavailable for 30 s; the provider-clis/install route clears the model-list memo so a fresh install is not reported as missing_executable. Verified: turbo typecheck @bb/server; vitest async-ttl-memo, execution-options, public-provider-installations, public-threads.defaults, thread-model-recovery (66 passed). The replay test fails with the failure predicate disabled. Co-Authored-By: Claude <noreply@anthropic.com>
GET /hosts/:id/provider-clis/status ran which, --version, npm view, npm list -g and claude doctor on the host on every call (2-3 s), and the sidebar Updates badge plus the compose view asked for it on every boot, gating network-idle.
Memoize each provider's installation.status RPC for 10 min (key: host, daemon session, registration revision, command); omission on 502/504/deadline stays outside the memo. ?force=true clears the status and installed-only discovery memos first; the install route clears them too. Wired end to end: contract query schema, sdk.hosts.providerCliStatus({ force }), bb machine provider-cli status --force, bb updates [status|apply] --force, guide/skill/mobile README, web Check-for-updates and Retry via recheckHostProviderCliStatus (fetchQuery with force), mobile Check-for-updates and Recheck via recheckHostProviderCliStatus / useRecheckHostProviderCliStatus.
Verified: turbo typecheck server-contract, sdk, server, cli, app, mobile; vitest public-provider-installations + execution-options (50), cli updates/machine (15) and full @bb/cli (474), UpdatesSettingsSection (28), cache-owner-registry, mobile recheck helper (2), server-contract + sdk suites. The memo-hit route test fails with the memo bypassed (8 probes instead of 4).
Co-Authored-By: Claude <noreply@anthropic.com>
Review found that AsyncTtlMemo.clear() only emptied the settled and pending maps: a task already in flight still ran store() when it settled, so a provider.list_models / provider.installation.status / provider.health probe that started before a CLI install (or a forced recheck) re-inserted its pre-install answer after the route's clear and served it for the full TTL (30 s missing_executable, 10 min stale catalog or CLI version, 5 min hidden agent). This is the S2 major, the S1 major, and the S4 'uncertain' finding; all three memos share this module. clear() now bumps a generation counter; run() captures it before calling the task and both settle handlers skip store() when it has moved (callers still get their value or rejection). The pending-map cleanup already guarded by promise identity. Verified: turbo typecheck @bb/server; vitest async-ttl-memo + public-provider-installations (20 passed). The three new memo unit tests (clear while a success / memoizable failure is in flight; a post-clear probe is not overwritten by a stale one) and the new route test (claude-code status probe deferred across POST provider-clis/install, non-forced GET must re-probe and report the post-update version) all fail with HEAD's memo swapped in and pass with the fence. Co-Authored-By: Claude <noreply@anthropic.com>
Review found that the installed-only discovery memo held a 'not installed' answer for the same 5 minutes as a 'present' one, although the daemon's bridge reports not_installed for any which failure (spawn error, 5 s probe timeout) and while the daemon runs on its fallback PATH during the first ~10 s after boot. Before the memo the next roster read corrected such a miss; after it the agent stayed hidden from every picker for 5 minutes. The TTL comment also gave an inverted reason for the window: a memo hit sends no maintenance traffic, so any window above the bridge's 60 s idle timeout lands the expiry probe cold; lengthening it makes expiries rarer, not warmer. AsyncTtlMemo gains an optional ttlMsForValue(value) window used by the success path. installedProviderProbe keeps 5 min for true and holds false for 30 s (INSTALLED_PROVIDER_ABSENT_MEMO_TTL_MS); the comment now states the real trade-off (probe count vs staleness) and why absence is weaker evidence. The in-flight clear race from the same review is fixed by the S2 follow-up (generation fence). Verified: turbo typecheck @bb/server; vitest async-ttl-memo + execution-options (51 passed). The new memo unit test (false re-runs at 30 s, true still served at 4 min) and the new roster test (fake Date installed before the harness builds the dedupers; opencode's not_installed answer is re-probed at +31 s and then listed, the present agents are still served from the memo at +4 min) both fail with HEAD's memo and dedupers swapped in and pass with the change. Co-Authored-By: Claude <noreply@anthropic.com>
…therwise join
Review found that recheckHostProviderCliStatus (web cache owner and mobile helper) relied on queryClient.fetchQuery, which in query-core 5.90 returns the in-flight retryer's promise whenever the query is already fetching: a Check for updates / Recheck that overlapped the plain boot/mount fetch (web: the on-open check fires in the same commit that creates useUpdateInventory's observers; mobile: the Check button is enabled while useHostsProviderCliStatus is fetching, and every 'host' realtime message starts a plain refetch) never sent ?force=true and presented the server's memoized answer as a fresh check. The mobile Updates screen also had no on-open check, so its 'Checked <age>' label dated a memo hit.
Both helpers now await queryClient.cancelQueries({ queryKey }) before the forced fetchQuery: the plain request's abort signal fires, the forced fetch really runs, and observers keep following the same Query. (cancelRefetch would not do: Query.fetch only honours it when data already exists, so the boot case would still join.) The mobile Updates screen mirrors the web section's on-open forced check once the host list has settled. The install-route clear race from this review is fixed by the S2 follow-up (memo generation fence) together with the route-level in-flight-install test.
Verified: turbo typecheck @bb/app + @bb/mobile; vitest apps/app provider-cli-status-cache-owner + cache-owner-registry + UpdatesSettingsSection (33 passed), apps/mobile recheck-host-provider-cli-status (3 passed). The new web and mobile tests (a QueryObserver with the plain SESSION_STATIC options mid-fetch, then a recheck) fail with HEAD's helpers swapped in (one call, no force) and pass with the cancel.
Co-Authored-By: Claude <noreply@anthropic.com>
… doc Round-2 review found that docs/provider-plugin-api.md still said the server caches the installed-or-not answer per host for a few minutes. Since 514aff096 that is true only for a present answer; a not-installed answer is held for about 30 s (INSTALLED_PROVIDER_ABSENT_MEMO_TTL_MS), which is the case a plugin author hits right after installing their agent, so the sentence overstated the hold by 10x. The experimental_visibility note now states both windows and keeps every re-probe trigger (daemon reconnect, plugin reload, provider CLI install, forced CLI status check). Verified: prose only; the behaviour the sentence describes is pinned by the existing execution-options.test.ts case "holds an absent answer for 30 s while a present one keeps the full window", which passes at HEAD. Co-Authored-By: Claude <noreply@anthropic.com>
Round-2 review of 610acae6a: recheckHostProviderCliStatus cancels the plain status fetch client-side only. The status route never reads the request signal, so a plain GET that missed the memo keeps running its provider.installation.status RPCs, and the forced GET that replaced it called providerInstallationStatus.clear() + installedProviderProbe.clear(), which empties pendingByKey and bumps the generation: the host ran a second full probe set concurrently (which/--version/npm view/npm list -g/claude doctor per provider, twice) and the S2 fence then threw the first set's answers away. Mobile's new on-open check and the desktop boot of Settings > Updates hit this overlap routinely. AsyncTtlMemo gains forgetSettled(): it drops settled entries only, with no generation bump and pendingByKey intact, so a task already running can still be joined and still stores its outcome. getProviderInstallations uses it for ?force=true on both memos: a forced read is never served a settled answer, but a probe already in flight describes the host now and is joined instead of duplicated. The install route keeps clear(), where the in-flight probe really is pre-install. Verified: vitest async-ttl-memo + public-provider-installations (23 passed); turbo typecheck @bb/server; oxlint/oxfmt clean. With HEAD's two source files swapped in, the new memo test fails (forgetSettled is not a function) and the new route test fails with a second probe set in flight (6 status probes instead of 3); both pass with the fix. Co-Authored-By: Claude <noreply@anthropic.com>
S1 adds `force` to `sdk.hosts.providerCliStatus`, which changes the generated bundled-types surface (new `HostProviderCliStatusArgs`). 0.4.16 is already published, so the CI npm version guard fails: that change would never reach npm consumers. Bump the patch version, moving packages/plugin-sdk/package.json and packages/domain/src/plugin-sdk-version.ts together. Verified: check-npm-version-guard.mjs now passes; typecheck and tests green for plugin-sdk, domain, plugin-build, server, sdk. Co-Authored-By: Claude <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What was wrong
Provider maintenance had no memory on the server (an internal performance sweep, findings S1, S4, S2):
GET /hosts/:id/provider-clis/statusran oneprovider.installation.statusRPC per installation-capable provider on every call (six subprocesses per provider on the host:which,--version,npm view,npm prefix -g,npm list -g,doctor), and the sidebar's update inventory fires it on every boot. 2–3 s per call; it was the last request in flight at boot.listInstalledPluginProviderInfosranprovider.healthfor every installed-only ACP agent on every/system/providers,/system/providers/stateand/system/execution-optionsread, so the daemon's shell-env refresh and maintenance-bridge respawn were paid per request (33 ms → 1.3 s after idle).AsyncTtlMemonever stored a rejection, so with a CLI installed but not logged in every execution-options read re-spawned the model-list probe and failed again (847 ms per thread open).What changed
getProviderInstallationsmemoizes each provider's RPC result per host for 10 min (key: host, daemon session, registration revision, full command incl. bridge launch).?force=truebypasses it: newhostProviderCliStatusQuerySchemain the contract,forceonsdk.hosts.providerCliStatus,--forceonbb machine provider-cli statusandbb updates [status|apply], the web Check-for-updates / Retry and the mobile Check-for-updates / Recheck go through a forcedfetchQuerythat first cancels any plain fetch in flight. The install route clears the memo. Guide template (bb-guide-machines.md), the built-inbb-cliskill and the mobile data README are updated. A forced read never gets a settled answer but joins a probe already in flight (forgetSettled()).not_installedfor anywhichfailure and during its first seconds after boot). Readiness (getProviderState) stays live.docs/provider-plugin-api.mddocuments the windows.AsyncTtlMemogainsfailures: { ttlMs, shouldMemoize }; the model-list memo replaysApiError502/504 (nothost_unavailable) for 30 s.clear()now fences tasks still in flight with a generation counter so a pre-install answer can never be re-stored after the install route clears a memo (this also protects the S1 and S4 memos).ttlMsForValuelets a memo hold different values for different windows.apps/server/src/lifecycle-dedupers.ts: CLI status 10 min, installed agent 5 min, absent agent 30 s, model-list failure 30 s. User-visible effect: a CLI or agent installed outside bb can show the old state for up to that window unless the user presses Check for updates / Recheck (which force) or installs through bb (which clears).HOST_DAEMON_PROTOCOL_VERSIONbump.How you verified
New tests, each failing on the pre-fix code:
apps/server/test/public/public-provider-installations.test.ts(memo hit, force re-probe, install clears, failure not memoized, re-probe after daemon reconnect, in-flight probe across an install, forced read joins an in-flight probe),apps/server/test/system/execution-options.test.ts(discovery memo, readiness stays live, absent answer held 30 s),apps/server/test/services/async-ttl-memo.test.ts(failure window, clear fence,forgetSettled,ttlMsForValue),apps/cli/src/__tests__/command-output/{updates,machine}.test.ts(--force→query: { force: "true" }),apps/app/src/components/settings/UpdatesSettingsSection.test.tsxandapps/app/src/hooks/cache-owners/provider-cli-status-cache-owner.test.ts,apps/mobile/src/data/hosts/recheck-host-provider-cli-status.test.ts.On this branch:
pnpm exec turbo run typecheck(76/76),pnpm exec turbo run lint,pnpm exec turbo run test(all packages green).Fixes: no issue — source is an internal performance sweep (findings S1, S4, S2).