mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(aibridge): record token usage without an MCP proxier (#27886)
_Disclosure: investigated and drafted with Claude Opus 5. I reviewed the
change and ran the tests locally._
Streaming Responses interceptions recorded no token usage when the
bridge was built with a nil `mcp.ServerProxier`, because
`recordTokenUsage` was called from inside the `i.mcpProxy != nil` branch
in `aibridge/intercept/responses/streaming.go`. Requests completed
normally and returned `200`, so traffic was served but metered as zero,
with no error surfaced. Upstream reports usage on the
`response.completed` event independently of tool injection, so the
proxier is not a valid precondition for recording it.
That state is reachable in production:
`coderd/aibridged/pool.go:255-265` treats proxier construction failure
as non-fatal ("MCP server injection can gracefully degrade") and caches
the resulting bridge via `SetWithTTL`, so one transient config-retrieval
error suppressed usage recording for every streaming Responses request
served by that bridge until its TTL expired.
This records usage for every completed response, guarded only on
`completedResponse`, matching `responses/blocking.go` and both
`chatcompletions` implementations. Per-iteration semantics are preserved
for the inner agentic loop.
The integration harness substituted a non-nil noop manager whenever no
proxier was supplied (`setupbridge.go:153-155`), so the nil path was
never exercised. `withoutMCP()` covers it.
Verification, with the fix reverted:
```
--- FAIL: TestResponsesStreamingRecordsTokenUsageWithoutMCP/without_mcp_proxy
Error: "[]" should have 1 item(s), but has 0
--- PASS: TestResponsesStreamingRecordsTokenUsageWithoutMCP/with_noop_mcp_proxy
```
and with it applied:
```
--- PASS: TestResponsesStreamingRecordsTokenUsageWithoutMCP/without_mcp_proxy
--- PASS: TestResponsesStreamingRecordsTokenUsageWithoutMCP/with_noop_mcp_proxy
--- PASS: TestResponsesStreamingRecordsTokenUsagePerAgenticIteration
```
The per-iteration case asserts exactly two records for the injected-tool
fixture, so decoupling the call from the proxier does not double-count
when the agentic loop iterates. `go test -race ./aibridge/...` passes
across all 15 packages.
Fixes #27885
One caveat on verification: I was unable to run `make gen` / `make
pre-commit` locally, as I do not have the full mise toolchain installed.
The change touches no codegen inputs (no SQL, protos, mocks, or
TypeScript), so I do not expect generated-file drift, but flagging it
rather than leaving it implied.
This commit is contained in:
@@ -241,6 +241,14 @@ func (i *StreamingResponsesInterceptor) ProcessRequest(w http.ResponseWriter, r
|
||||
return err
|
||||
}
|
||||
|
||||
// Record token usage for every iteration, whether or not tools are
|
||||
// injected. Usage is reported by upstream independently of the MCP
|
||||
// proxy, so gating this on the proxy drops usage entirely for
|
||||
// deployments that run without one.
|
||||
if completedResponse != nil {
|
||||
i.recordTokenUsage(ctx, completedResponse)
|
||||
}
|
||||
|
||||
if i.mcpProxy != nil && completedResponse != nil {
|
||||
pending := i.getPendingInjectedToolCalls(completedResponse)
|
||||
shouldLoop, innerLoopErr = i.handleInnerAgenticLoop(ctx, pending, completedResponse)
|
||||
@@ -248,9 +256,6 @@ func (i *StreamingResponsesInterceptor) ProcessRequest(w http.ResponseWriter, r
|
||||
i.sendCustomErr(ctx, w, http.StatusInternalServerError, innerLoopErr)
|
||||
shouldLoop = false
|
||||
}
|
||||
|
||||
// Record token usage for each inner loop iteration
|
||||
i.recordTokenUsage(ctx, completedResponse)
|
||||
}
|
||||
|
||||
i.recordModelThoughts(ctx, completedResponse)
|
||||
|
||||
@@ -2486,3 +2486,65 @@ func extractSigV4Field(authHeader, prefix string) string {
|
||||
}
|
||||
return strings.TrimSpace(val)
|
||||
}
|
||||
|
||||
// TestTokenUsageRecordedWithoutMCPProxier asserts that an interception records
|
||||
// token usage when no MCP server proxier is configured. Upstream reports usage
|
||||
// independently of tool injection, and coderd/aibridged tolerates a nil
|
||||
// proxier when proxier construction fails, so usage must not depend on one.
|
||||
func TestTokenUsageRecordedWithoutMCPProxier(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
fixture []byte
|
||||
path string
|
||||
expectedInputTokens, expectedOutputTokens int64
|
||||
}{
|
||||
{
|
||||
name: "openai responses",
|
||||
fixture: fixtures.OaiResponsesStreamingSimple,
|
||||
path: pathOpenAIResponses,
|
||||
expectedInputTokens: 11,
|
||||
expectedOutputTokens: 18,
|
||||
},
|
||||
{
|
||||
name: "anthropic messages",
|
||||
fixture: fixtures.AntSimple,
|
||||
path: pathAnthropicMessages,
|
||||
expectedInputTokens: 18,
|
||||
expectedOutputTokens: 241,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx, cancel := context.WithTimeout(t.Context(), testutil.WaitLong)
|
||||
t.Cleanup(cancel)
|
||||
|
||||
fix := fixtures.Parse(t, tc.fixture)
|
||||
upstream := testutil.NewMockUpstream(ctx, t, testutil.NewFixtureResponse(fix))
|
||||
|
||||
bridgeServer := newBridgeTestServer(ctx, t, upstream.URL, func(c *bridgeConfig) {
|
||||
c.noMCPProxy = true
|
||||
})
|
||||
|
||||
inputBefore := bridgeServer.Recorder.TotalInputTokens()
|
||||
outputBefore := bridgeServer.Recorder.TotalOutputTokens()
|
||||
|
||||
reqBody, err := sjson.SetBytes(fix.Request(), "stream", true)
|
||||
require.NoError(t, err)
|
||||
resp, err := bridgeServer.makeRequest(t, http.MethodPost, tc.path, reqBody)
|
||||
require.NoError(t, err)
|
||||
defer resp.Body.Close()
|
||||
require.Equal(t, http.StatusOK, resp.StatusCode)
|
||||
_, err = io.ReadAll(resp.Body)
|
||||
require.NoError(t, err)
|
||||
|
||||
require.NotEmpty(t, bridgeServer.Recorder.RecordedTokenUsages(), "token usage must be recorded without an MCP proxier")
|
||||
assert.EqualValues(t, tc.expectedInputTokens, bridgeServer.Recorder.TotalInputTokens()-inputBefore, "input tokens miscalculated")
|
||||
assert.EqualValues(t, tc.expectedOutputTokens, bridgeServer.Recorder.TotalOutputTokens()-outputBefore, "output tokens miscalculated")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,9 +50,12 @@ type bridgeConfig struct {
|
||||
metrics *metrics.Metrics
|
||||
tracer trace.Tracer
|
||||
mcpProxy mcp.ServerProxier
|
||||
userID string
|
||||
metadata recorder.Metadata
|
||||
logger slog.Logger
|
||||
// noMCPProxy leaves the proxier nil instead of falling back to
|
||||
// NoopMCPManager, which is non-nil and reports zero tools.
|
||||
noMCPProxy bool
|
||||
userID string
|
||||
metadata recorder.Metadata
|
||||
logger slog.Logger
|
||||
}
|
||||
|
||||
// bridgeTestServer wraps an httptest.Server running a RequestBridge.
|
||||
@@ -150,7 +153,7 @@ func newBridgeTestServer(
|
||||
cfg.tracer = defaultTracer
|
||||
}
|
||||
cfg.logger = newLogger(t)
|
||||
if cfg.mcpProxy == nil {
|
||||
if cfg.mcpProxy == nil && !cfg.noMCPProxy {
|
||||
cfg.mcpProxy = newNoopMCPManager()
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user