fix(den): restore the external MCP tool-call budget on the live client path - #3122
Merged
Merged
Conversation
…t path An external MCP capability got 30s for the whole session -- connect, OAuth refresh, initialize and tools/call shared one budget -- and execute_capability spent that budget twice, once for schema-digest discovery and once for the call. Slow providers died at ~31s. The 120s/150s tool budget from #2750 was never reached. #2810 deleted the legacy runtime, so callExternalMcpTool resolves to the enterprise client, and the adapter's tool-call path forwarded no lifecycle deadline -- unlike listExternalMcpTools beside it -- leaving the package's 30s default in charge. A Math.min(operationTimeoutMs, ...) clamp meant injecting the longer deadline would have been clipped back to 30s anyway. The only test asserting 120s exercised the bypassed file. Make an injected lifecycle deadline authoritative in both directions and demote operationTimeoutMs to the fallback default. Forward the deadline on the tool-call path, and share one deadline between discovery and the call so the handshake and the budget are paid once. Split the per-request timeout from the absolute bound and stop cancelling a provider that is still reporting progress, matching the calling harness instead of being stricter than it; the SDK only attaches a progressToken when an onprogress handler is present, so the flag alone was inert.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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 broken
An external MCP capability called through
execute_capabilitygot 30 seconds for the entire session — connect + OAuth refresh +initialize+tools/callall shared one budget, so the provider's actual work got whatever was left. ThenexecuteExternalCapabilityspent that budget twice: once for schema-digest discovery, once for the call, each paying its own handshake.Real failure that started this, reproduced twice against deployed den-api at ~31s (30s + a full second discovery session):
What introduced it
4c85588e6env.enterpriseMcpClientEnabled. Shipped theMath.min(operationTimeoutMs, …)clamp,resetTimeoutOnProgress: false, and an adapter tool-call path that forwards no lifecycle deadline. Latent — flag was off.152a0c229external-mcp-client.ts. Correct and effective at the time, because the legacy client was still selected.2b72a99aacallExternalMcpToolnow resolves to the adapter, which forwards no deadline — so the package's 30s default took over and #2750's fix silently stopped applying. Its 120s/150s constants became dead code, as did the only test asserting them.218a13cb3-32001, i.e. blamed on the provider — which is why this only became visible ten days after it broke.Compounding it, the
Math.min(operationTimeoutMs, …)clamp atenterprise-mcp-client.ts:209meant that even injecting the intended 150s deadline would have been clipped back to 30s, so the obvious one-line fix would not have worked.The change
operationTimeoutMsdemotes to the fallback default used only when no deadline is injected.listExternalMcpToolsbeside it, and sets the 120s per-request budget.executeExternalCapabilitycreates one deadline and shares it between schema discovery and the call, so the handshake and the budget are paid once instead of twice.timeoutis now strictly below the absolutemaxTotalTimeout, withresetTimeoutOnProgress: true. We were stricter than our own caller — opencode usestrue; we usedfalse. The SDK only attaches aprogressTokenwhen anonprogresshandler is present (shared/protocol.js:643-649), so the flag alone would have been inert; a handler is included.Net effect: our ceiling becomes a backstop and the calling harness decides. opencode's own per-tool-call timeout (
mcp[name].timeout→experimental.mcp_timeout→ 60s) is now the binding constraint, which is the intended design.30 lines of production code across 3 files.
Tests run
pnpm --filter @openwork/enterprise-mcp-client testpnpm --filter @openwork/enterprise-mcp-client typecheckpnpm exec tsc -p ee/apps/den-api/tsconfig.json --noEmitbun test ee/apps/den-api/test/external-capabilities-search-divergence.test.tsbun test— search-divergence + adapter-budget + agent-timeouts + diagnosticsEvery new test was confirmed to fail on the bug and pass on the fix, by reverting each production file individually:
expect(maxTotalTimeout).toBeGreaterThanOrEqual(149000)got30000external-capabilities.ts→ same,30000, from the shared-deadline testNew coverage: a live-path test capturing the real
RequestOptionsthe MCP SDK receives viacallExternalMcpTool(maxTotalTimeout≈ 150s,timeout≥ 120s,timeout < maxTotalTimeout,resetTimeoutOnProgress,onprogress), plus a shared-budget test asserting the tool call's window is strictly smaller than discovery's — one draining budget rather than two fresh ones. The previous 120s assertion tested the bypassed file and passed throughout the outage.Known caveats, stated honestly
No fraimz. This change is entirely inside the cloud service. The observable win — a slow capability completing instead of dying at 30s — can only be seen against a deployed den-api; the desktop talks to deployed cloud, which still runs the old code. I am not claiming an end-to-end proof I do not have. Reviewer repro after deploy: call any slow external MCP capability that previously died at ~31s.
One test reds in a full-directory run,
bun test ee/apps/den-api/test:execute_capability shares one external MCP lifecycle budget…. It passes in isolation and in every targeted combination. Cause is pre-existing and unrelated:marketplace-cloud-readiness.test.ts:67registers a process-globalmock.modulethat throws on any external MCP runtime call, andmock.modulehas no per-file scoping — so 18 of the 22 tests already in that file also fail in the base directory run (base 166 fail / branch 167). My test becomes victim 19. Fixing that global mock is separate cleanup. CI does not run den-api tests.Worst case is deliberately slower. A hanging provider used to be cut at 30s + 30s; there is now one 150s bound with the existing 180s
execute_capabilityrace as backstop, and opencode cutting at 60s in practice.Also affects
search_capabilities: its probes were silently clamped to 30s each by the same bug and can now use the shared 45s window. The absolute 45s fan-out bound is unchanged, so total search time is not affected.