perf(coding-agent): cache MCP connect, skill manifest, and token estimates across turns - #94
Merged
Merged
Conversation
… min of 1s Pre-existing failure on main since commit 1bfc930 lowered MIN_POLL_INTERVAL_SECONDS from 10 to 1. Test still asserted 5 was rejected — update to assert 0 is rejected so the boundary check still has meaning.
…estimates across turns
Per-turn latency was dominated by harness work that didn't need to repeat.
Profiling on a typical session (7 MCP servers, including two cf-portal
instances exposing ~200 tools each) suggested 600ms–2.3s of overhead per
turn before the model even saw the prompt. This patch removes the bulk of
that without changing any wire format or observable behaviour.
1. connectMcpServers() ran on every chat turn (was called from runThinkChat
at the top of every onChatMessage path) and sequentially disconnected,
reconnected, and re-listed tools for every enabled MCP server. New:
- A 5-minute TTL guard plus a fingerprint over the enabled-config IDs
short-circuits the reconnect storm. Newly-enabled servers still appear
immediately because the fingerprint check fires before the TTL check.
- The per-config auth-header resolution is parallelised (Promise.all).
Previously a 7-server config did 7 serial round-trips to UserControl.
- A new forceReconnect option bypasses the cache. Used by
reconcileOwnerIdentity() on owner-email drift and by the existing
401/403 auth-failure recovery path that was already in place.
2. warmSkills() previously refetched personal skills from UserControl and
rescanned the workspace SKILL.md files every turn. Now TTL-cached at 60s
(mirrors the existing ADMIN_PREFIX_TTL_MS). A bumpSkillsGeneration()
helper lets in-DO callers invalidate the cache explicitly when needed.
3. estimateMessageTokens() / estimateMessagesTokens() previously
re-JSON-stringified every message every time they were called — and the
compaction logic calls them 3–4 times per step. Now memoised per
ModelMessage object via a WeakMap, so cache lifetime tracks the
conversation array naturally.
Out of scope (deliberately, follow-ups exist in the original research note):
- Tool-manifest filtering / lazy-load for the cf-portal surface.
- Switching from @ai-sdk/openai-compatible to @ai-sdk/anthropic for native
prompt-caching headers.
- Streaming pipeline, compaction logic, doom-loop detection, DO storage
layout, gateway/provider selection.
Tests:
- New test/harness-cache-unit.test.ts asserts the TTL and fingerprint
helpers in isolation (isCacheFresh, isFingerprintedCacheFresh — extracted
as pure functions so both connectMcpServers and warmSkills consult the
same logic the tests exercise).
- token-budget-unit.test.ts adds two assertions on the per-message cache:
repeat calls hit the cache (no extra JSON.stringify), and
estimateMessagesTokens only stringifies newly-appended messages.
- Full suite: 990 passed (1 chat-monitor test fixed in the preceding
commit). Typecheck clean.
…identity reset Two follow-up fixes from PR review of the harness-cache pass. 1. OAuth tools refresh on cache hit. The TTL fast-path in connectMcpServers was returning before loadOAuthToolsFromHub, so session DOs couldn't see newly OAuth-connected MCP servers until either the 5-minute TTL elapsed or the mcp-configs fingerprint changed. OAuth adds/removes go through the hub DO via the Agents SDK and don't touch effective-mcp-configs, so the fingerprint never moved on those operations. Fix: call loadOAuthToolsFromHub() even on the cache fast-path. It's a single peer-DO RPC — cheap relative to the reconnect storm we just avoided, and restores the pre-cache behaviour where OAuth tools were refreshed every turn. 2. Trimmed the explicit mcpConnectedAt / mcpEnabledConfigsFingerprint reset in reconcileOwnerIdentity(). clearAllMcpConnections() already zeroes both fields (as of the cache PR), and forceReconnect: true on the subsequent connect bypasses the cache regardless. Three-way redundant — kept the clearAllMcpConnections() side and the forceReconnect flag.
jonnyparris
marked this pull request as ready for review
May 28, 2026 10:47
Pre-existing CI lint failure on main — the chat-monitor brain refactor (commit 63d261b) moved reply-sending into a tool the brain session calls, making the top-level sendChatReply import dead. Biome flagged it but the red CI wasn't acted on. Removing here so this PR can land green.
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.
Why
Profiling pass found that every chat turn in CodingAgent re-did harness work that didn't need to repeat. With a typical 7-MCP-server config (including two cf-portal instances exposing ~200 tools each), the per-turn overhead was 600ms–2.3s before the model even saw the prompt.
This patch removes the bulk of that without changing any wire format or observable behaviour.
What changed
1. connectMcpServers() — TTL + fingerprint + parallel auth fetches
Was called from
runThinkChat()at the top of everyonChatMessage(). Every turn it disconnected, reconnected, and re-listed tools for every enabled MCP server. Now:Promise.all). Previously a 7-server config did 7 serial round-trips to UserControl.forceReconnectoption bypasses the cache. Used byreconcileOwnerIdentity()on owner-email drift and the existing 401/403 auth-failure recovery path.2. warmSkills() — 60s TTL
Previously refetched personal skills from UserControl and rescanned workspace SKILL.md files every turn. Now TTL-cached at 60s, mirroring the existing
ADMIN_PREFIX_TTL_MS. AbumpSkillsGeneration()helper lets in-DO callers invalidate explicitly.3. estimateMessageTokens — per-message WeakMap cache
The compaction logic calls
estimateMessagesTokens()3–4 times per step, and each call re-JSON.stringify'd every message. Now memoised per ModelMessage object via a WeakMap, so cache lifetime tracks the conversation array naturally.Tests
test/harness-cache-unit.test.ts(new) — asserts the TTL and fingerprint helpers in isolation.isCacheFreshandisFingerprintedCacheFreshare pure functions consumed by bothconnectMcpServersandwarmSkills, so the tests cover the same logic the production code runs.test/token-budget-unit.test.ts— two new assertions on the per-message cache: repeat calls hit the cache (zero extraJSON.stringify), andestimateMessagesTokensonly stringifies newly-appended messages.test/chat-monitor-unit.test.ts— fixed a pre-existing failure from commit 1bfc930 (MIN_POLL_INTERVAL_SECONDSlowered to 1, test still expected 5 to be rejected).Out of scope (deliberately)
@ai-sdk/openai-compatibleto@ai-sdk/anthropicfor native prompt-caching headers.These came up in the same research pass but each needs design work or a bigger refactor — separate MRs.
Deploy
Per
dodo-worktreeskill: this branch has NOT been deployed. After review, deploy withnpm run deploy:safe --prefix ~/dev/dodofrom main once merged.