perf(cli): list connected accounts once per execute - #4475
perf(cli): list connected accounts once per execute#4475Daksh (sudodaksh) wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Preview this PR's installerThe hermetic install E2E suite passed for this commit. These commands run the PR's installer scripts against the latest published CLI release — they preview installer behavior, not unreleased binaries. curl -fsSL "https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install.sh" | sh
curl -fsSL "https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install.sh" | COMPOSIO_INSTALL_SHELL=none shThe first command is the default flow: it installs the CLI and configures your shell automatically. The second installs only, without touching shell startup files. Shell-specific setup routes (the override points the variant at this PR's base installer): curl -fsSL "https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install/bash.sh" | COMPOSIO_INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install.sh" sh
curl -fsSL "https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install/zsh.sh" | COMPOSIO_INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install.sh" sh
curl -fsSL "https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install/fish.sh" | COMPOSIO_INSTALL_SCRIPT_URL="https://raw.githubusercontent.com/ComposioHQ/composio/0a572b0421c5a3cc8e8b4b670ccbb3cb7db10822/install.sh" sh |
|
Not in this PR, but worth a look: The two requests differ by two headers. The command passes the resolved org and project ( That looks like a bug rather than a design choice. Arguments are validated against the scoped definition, then file uploads are normalized against the unscoped one. If the backend ever answers per project (version overrides, custom tools), the two lookups disagree, each overwrites the other's cache entry, and every run downloads the definition again. I probed the endpoint with and without the org header for The fix is small: pass the command's |
Alberto Schiabel (jkomyno)
left a comment
Alberto Schiabel (jkomyno)
left a comment
There was a problem hiding this comment.
Reviewed and simplified this locally. The request savings stay the same: one GET /connected_accounts and one shared get_latest_version per execute. With the suggestions below applied on top of this head: pnpm run typecheck, validate:boundaries, and oxlint on the changed files pass, and vitest run passes (130 files, 1338 passed, 1 skipped).
Checked and sound
total_itemsis a requirednumberin@composio/client, so the complete-list check only falls back to the filtered request when the shared list is actually truncated.tools.execute.cmd.tspasses the same client instance into the account picker and the executor, so both memo keys match and the request is shared.- Process-lifetime memoization can't go stale inside
composio run:execute()in run helpers spawns a separatecomposio executeprocess per tool.
Suggestions (inline)
memoizeInProcessevicts only on typed failures (tapError).Effect.cachedstores the wholeExit, so a defect or an interruption of the first caller stays cached and is replayed to every later caller for the rest of the process.Effect.onErrorcovers every failure cause. A regression test for the defect case is included.ActiveConnectedAccountsListErroris built but only its.causeis ever read; both callers unwrap it and rewrap in their own error. Failing with the raw rejection (asfetchConnectedAccountsForToolkitalready does) removes the wrap/unwrap round trip. Error messages are unchanged.- Doc comment wording: describe the current behavior rather than what the code used to do.
Not changed
Cachefromeffectlooks like a replacement formemoizeInProcess, but itslookuponly receives the key, so the client object can't reach the request through a string key. The small helper is justified.tools-executor.tsstill callsgetOrFetchToolInputDefinition(slug)without org/project, so itsget_latest_versiongets a different memo key. The description already calls that out as a semantics decision.
| * between concurrent callers. A success stays cached; a failure is dropped so | ||
| * the next caller retries instead of replaying the error. |
There was a problem hiding this comment.
Effect.cached memoizes the full Exit, so defects and interruptions need evicting too (see next comment).
| * between concurrent callers. A success stays cached; a failure is dropped so | |
| * the next caller retries instead of replaying the error. | |
| * between concurrent callers. A success stays cached; a failure, defect, or | |
| * interruption is dropped so the next caller retries instead of replaying it. |
| // another fiber. | ||
| const cached = Effect.runSync( | ||
| Effect.cached( | ||
| options.make(input).pipe(Effect.tapError(() => Effect.sync(() => cache.delete(key)))) |
There was a problem hiding this comment.
tapError only runs on typed failures. If make dies, or the first caller is interrupted mid-request, Effect.cached keeps that exit and every later caller in the process gets it replayed. onError runs for any failure cause.
| options.make(input).pipe(Effect.tapError(() => Effect.sync(() => cache.delete(key)))) | |
| options.make(input).pipe(Effect.onError(() => Effect.sync(() => cache.delete(key)))) |
| expect(yield* load('k')).toBe(2); | ||
| expect(calls).toBe(2); | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Regression test for the eviction change above (fails with tapError, passes with onError).
| ); | |
| ); | |
| it.effect('drops a defect so the next caller retries', () => | |
| Effect.gen(function* () { | |
| let calls = 0; | |
| const load = memoizeInProcess({ | |
| keyOf: (key: string) => key, | |
| make: () => | |
| Effect.suspend(() => | |
| ++calls === 1 ? Effect.die('first call dies') : Effect.succeed(calls) | |
| ), | |
| }); | |
| const first = yield* Effect.exit(load('k')); | |
| expect(Exit.isFailure(first)).toBe(true); | |
| expect(yield* load('k')).toBe(2); | |
| expect(calls).toBe(2); | |
| }) | |
| ); |
| export class ActiveConnectedAccountsListError extends Data.TaggedError( | ||
| 'services/ActiveConnectedAccountsListError' | ||
| )<{ | ||
| readonly message: string; | ||
| readonly cause: unknown; | ||
| }> {} | ||
|
|
There was a problem hiding this comment.
Only .cause of this error is ever read: listConnectedAccountsForToolkit unwraps it, and resolveToolRouterSessionConnections rewraps error.cause in its own error. Suggest dropping it and failing with the raw rejection, like fetchConnectedAccountsForToolkit already does.
| export class ActiveConnectedAccountsListError extends Data.TaggedError( | |
| 'services/ActiveConnectedAccountsListError' | |
| )<{ | |
| readonly message: string; | |
| readonly cause: unknown; | |
| }> {} |
| * builds the session's connection context from the full list. Both used to | ||
| * issue their own `GET /connected_accounts`, one toolkit-filtered and one not. | ||
| * The per-toolkit view is a subset of this list, so both read from here. | ||
| * | ||
| * `limit: 1000` is the session path's existing page size; a user with more | ||
| * active accounts than that was already truncated there. |
There was a problem hiding this comment.
| * builds the session's connection context from the full list. Both used to | |
| * issue their own `GET /connected_accounts`, one toolkit-filtered and one not. | |
| * The per-toolkit view is a subset of this list, so both read from here. | |
| * | |
| * `limit: 1000` is the session path's existing page size; a user with more | |
| * active accounts than that was already truncated there. | |
| * builds the session's connection context from the full list. The per-toolkit | |
| * view is a subset of this list, so both read from here instead of issuing | |
| * their own `GET /connected_accounts`. | |
| * | |
| * `limit: 1000` is the session path's existing page size; a user with more | |
| * active accounts than that was already truncated there. Fails with the raw | |
| * rejection; each caller wraps it in its own error. |
| catch: cause => | ||
| new ActiveConnectedAccountsListError({ | ||
| message: `Failed to list connected accounts for user "${userId}".`, | ||
| cause, | ||
| }), |
There was a problem hiding this comment.
| catch: cause => | |
| new ActiveConnectedAccountsListError({ | |
| message: `Failed to list connected accounts for user "${userId}".`, | |
| cause, | |
| }), | |
| catch: cause => cause, |
| const shared = yield* listActiveConnectedAccounts({ | ||
| client: params.client, | ||
| userId: params.userId, | ||
| }).pipe(Effect.mapError(error => error.cause)); |
There was a problem hiding this comment.
| }).pipe(Effect.mapError(error => error.cause)); | |
| }); |
| error => | ||
| new ToolRouterSessionConnectionsError({ | ||
| message: `Failed to list connected accounts for user "${userId}".`, | ||
| cause: error.cause, |
There was a problem hiding this comment.
| error => | |
| new ToolRouterSessionConnectionsError({ | |
| message: `Failed to list connected accounts for user "${userId}".`, | |
| cause: error.cause, | |
| cause => | |
| new ToolRouterSessionConnectionsError({ | |
| message: `Failed to list connected accounts for user "${userId}".`, | |
| cause, |
`composio execute` listed the user's connected accounts twice on every call: once toolkit-filtered by the account picker, once unfiltered by session creation. The two code paths cannot see each other. The unfiltered list is now fetched once per process and shared, and the picker derives its toolkit subset from it: same slug match, server order kept, first 100, exactly what its own query returned. If the shared list is truncated (more active accounts than one page), the picker falls back to its original request, so results are identical in every case. Interleaved A/B against the parent commit, compiled binaries, 15 runs each on a small response: 1735 -> 1636ms best, 1934 -> 1845ms median. One fewer request on the critical path, ~140ms. `memoizeInProcess` memoizes an Effect per key for the process lifetime, shares one run between concurrent callers, and drops failures so the next caller retries. It also covers `get_latest_version`, which the definition refresh fetched twice with the same key on the stale path. The executor's own version lookup stays unscoped and separate; scoping it to org and project would change which definition it resolves, so that is left as is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wx9gEjuiHux2weiHjdNcDs
…error memoizeInProcess evicted a key only on typed failures, but Effect.cached stores the whole Exit, so a defect or an interrupted first caller was replayed for the rest of the process. Evict on any failure cause. ActiveConnectedAccountsListError was only ever unwrapped to its cause by both callers, so the shared list now fails with the raw rejection.
fd6e869 to
0a572b0
Compare
2 participants
Summary
composio executemade eight backend requests. Two of them were the same list of the user's connected accounts, fetched by two code paths that cannot see each other. It is fetched once now. Interleaved A/B against #4469, compiled binaries, 15 runs each on a small response: 1735ms to 1636ms best, 1934ms to 1845ms median. That is one round trip (~140ms) off the critical path.Results are identical to before in every case. The picker derives its toolkit subset from the shared list with the exact semantics of its old query, and falls back to that query when the shared list is truncated.
Fifth PR in the stack. Stacked on #4469; review #4463, #4464, #4468 and #4469 first.
Changes
src/utils/memoize-in-process.ts(new). Memoizes an Effect per key for the process lifetime, shares one run between concurrent callers, and drops a failure, defect or interruption so the next caller retries.listActiveConnectedAccountsinconnected-account-selection.ts: the unfilteredGET /connected_accountsfor a user, memoized by client identity and user id. It fails with the raw rejection, and each caller wraps that in its own error.resolveToolRouterSessionConnectionsreads from it when it has no toolkit filter, which is the execute path. With a filter it keeps its own request.resolveConnectedAccountForToolkitused to issue its own request, toolkit-filtered,limit: 100. It now derives that from the shared list: same slug match, server order preserved, first 100. If the shared list has anext_cursorortotal_itemsabove what it holds, the toolkit's accounts may sit past the cut, so the original filtered request runs instead.get_latest_versiongoes through the same memo. The definition refresh fetched it twice with identical headers on the stale path; that is one request now. The executor's own version lookup sends no org or project headers and stays a separate request. Scoping it would change which definition it resolves under, which is a semantics decision, not a perf one.What does not change: the request list on a normal execute is now
project/resolve,connected_accounts,get_latest_versiontwice,consumer/config,session,execute. Error messages are unchanged; the fallback passes the raw rejection through so the picker's message reads as before.Type of change
How Has This Been Tested?
Bun 1.4.1+4661e494f, Node 24.20.0, pnpm 11.8.0, linux-x64.
cd ts/packages/cli && pnpm run typecheck && pnpm run validate:boundariespnpm exec vitest run: 131 files, 1341 passed, 1 skipped (whole stack). New tests cover the memo sharing one run per key and retrying after a failure or a defect. The execute suite already covers account selection with and without a selector and passes unchanged. The test layer builds a fresh client per test, so the client-keyed memo does not bleed between tests.fetchwhile runningexecute HACKERNEWS_GET_ITEM_WITH_IDfrom source.connected_accountsappears once, the rest of the list as before.pnpm build:binary, then the interleaved A/B above against perf(cli): move the compiler and tokenizer out of the executable #4469's binary. A second round of 12 gave 1788 to 1621ms best, 2045 to 1963ms median.Screenshots (if applicable)
Not applicable.
Checklist
No docs describe the request sequence.
@composio/cliis private, so no changeset.Additional context
The rest of an execute, from the same trace:
project/resolve140 to 640ms with no cache,tool_router/session385 to 655ms created per invocation, and the execute call itself 500 to 730ms. The connected-account cache inconsumer-short-term-cache.tswould takeconnected_accountsoff the path entirely, butDISABLE_CONNECTED_ACCOUNT_CACHEdefaults to on, and enabling it fails no-auth toolkits with "not connected" because the cached list does not include them. Both are separate changes.🤖 Generated with Claude Code
https://claude.ai/code/session_01Wx9gEjuiHux2weiHjdNcDs