Skip to content

fix(mcp): defer tool-call telemetry via waitUntil instead of firing it forgotten#7277

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/mcp-telemetry-wait-until
Jul 19, 2026
Merged

fix(mcp): defer tool-call telemetry via waitUntil instead of firing it forgotten#7277
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/mcp-telemetry-wait-until

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Summary

  • src/mcp/server.ts's handleMcpRequest called recordMcpToolTelemetry(...) synchronously and un-awaited, immediately before returning the response (or re-throwing) — never registered via Cloudflare's waitUntil. recordMcpToolCall (src/mcp/telemetry.ts) does real async I/O internally (a PostHog client.capture() call, scheduling a network request), so on Workers, any in-flight work not passed to waitUntil can be silently cut off once the request's execution context tears down — dropping the mcp_tool_call event with no error surfaced anywhere.
  • This repo already has an established pattern for exactly this situation: src/orb/webhook.ts's scheduleAfterResponse wraps c.executionCtx.waitUntil(task) so a slow background task can't delay the response but also isn't silently lost. handleMcpRequest already had direct access to the execution context via getExecutionContext(c) in this exact function (used two lines earlier for createMcpHandler's third argument), so the fix needed no new plumbing — just capturing it once and reusing it.
  • Both recordMcpToolTelemetry(...) call sites (the success path and the catch path) now pass the returned promise to executionCtx.waitUntil(...) instead of firing it and discarding the result.
  • recordMcpToolCall and recordMcpToolTelemetry are now async, both awaiting through to client.flush() — giving waitUntil real awaitable work to hold the execution context open for, since capture() alone is fire-and-forget by the PostHog SDK's own design and returns before its network request actually lands.
  • No change to recordMcpToolCall's no-throw guarantee (a PostHog init/capture/flush failure still degrades to recording nothing, never surfacing into the MCP tool response) or its safe-no-op-when-unconfigured behavior — both existing invariants are still covered by the (updated, still-passing) existing test suite.
  • No change to handleMcpRequest's own response latency — the telemetry work is deferred past the response via waitUntil, never awaited inline (that would reintroduce the added-latency tradeoff the neighboring recordProductUsageEvent call already deliberately accepts for a different call).

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves.

Closes #7233

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck — the whole-repo tsc --noEmit reliably OOMs on this shared, memory-constrained sandbox regardless of diff size (confirmed again this run: only ~1GB free / 3.4GB available system memory, swap fully used). Verified instead by hand against the real type declarations this diff actually touches: ExecutionContext<unknown>.waitUntil(promise: Promise<any>): void (worker-configuration.d.ts:529) trivially accepts the Promise<void> recordMcpToolTelemetry now returns, and McpTelemetryEnv's two fields are both optional in the real Env (src/env.d.ts:582,586POSTHOG_API_KEY?/POSTHOG_HOST?), which I did not change. (A scoped tsc attempt against just the changed files without the full src/env.d.ts augmentation produced misleading false positives across dozens of unrelated files/lines I never touched, confirming it was an incomplete-include artifact, not a real error — abandoned in favor of the manual check above plus the full targeted test run below.)
  • npx vitest run test/unit/mcp-server-telemetry.test.ts test/unit/mcp-telemetry.test.ts test/unit/mcp-local-telemetry.test.ts — 26/26 passing. Confirmed via grep -l "handleMcpRequest(" test/unit/*.test.ts that mcp-server-telemetry.test.ts is the only test file that actually invokes handleMcpRequest (dozens of other mcp-*.test.ts files import unrelated exports from the same module, e.g. LoopoverMcp, and never touch this code path), so this is the complete relevant test scope.
  • codecov/patch (99% target, branch-counted, src/** gated) — verified locally via npm run test:coverage's v8 report scoped to the two changed source files: src/mcp/telemetry.ts is 100% statements/branches/functions/lines when exercised by its own test file alone. For src/mcp/server.ts (a large multi-thousand-line file most of which is exercised by dozens of other test files this PR doesn't run), I instead pulled the raw per-statement hit counts (coverage-final.json) for every changed line (1648–1707) with just the one relevant test file running, and confirmed every changed statement has a non-zero hit count — the file's low aggregate percentage in that reduced run is from the thousands of unrelated lines/tools elsewhere in the same file, not from my diff.
  • npm run test:workers / npm run build:mcp / npm run test:mcp-pack / npm run ui:openapi:check (not applicable — no Worker-binding, MCP-package-build, or OpenAPI/UI surface changed; this is an internal handleMcpRequest telemetry-dispatch change only)
  • npm audit --audit-level=moderate (no dependency changes)

If any required check was skipped, explain why:

  • The whole-repo npm run typecheck OOMs on this specific sandbox under current memory pressure regardless of diff size; the manual type-declaration verification above (citing the exact ExecutionContext/Env types this diff's signatures interact with) plus CI's isolated runner's own tsc --noEmit are the checks that cover this.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/cookie/CORS/session/GitHub-App changes; this only changes how a telemetry side-effect is scheduled relative to the response.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (The MCP tool-call response shape and timing are unchanged — this is purely about not losing the telemetry event; covered by the updated/new tests above.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots. (N/A — no UI changes; this is a backend-only telemetry-dispatch fix.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

…t forgotten

recordMcpToolTelemetry was invoked synchronously and un-awaited right
before handleMcpRequest returned its response. On Cloudflare Workers,
any in-flight I/O not registered via waitUntil can be torn down once
the request context finishes, silently dropping the PostHog capture
before its network request lands. Both call sites now pass the
telemetry promise to the execution context's waitUntil, matching the
scheduleAfterResponse pattern already used in src/orb/webhook.ts.
recordMcpToolCall now awaits client.flush() so waitUntil has real
async work to hold the context open for.

Closes JSONbored#7233
@galuis116
galuis116 requested a review from JSONbored as a code owner July 19, 2026 12:02
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.81%. Comparing base (c743eca) to head (d94eaa9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7277   +/-   ##
=======================================
  Coverage   93.81%   93.81%           
=======================================
  Files         704      704           
  Lines       69458    69460    +2     
  Branches    18896    18896           
=======================================
+ Hits        65163    65165    +2     
  Misses       3302     3302           
  Partials      993      993           
Flag Coverage Δ
shard-1 43.16% <100.00%> (-0.45%) ⬇️
shard-2 37.39% <0.00%> (+0.16%) ⬆️
shard-3 33.05% <0.00%> (-0.05%) ⬇️
shard-4 32.73% <0.00%> (-1.60%) ⬇️
shard-5 32.78% <16.66%> (+0.59%) ⬆️
shard-6 46.06% <83.33%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/mcp/server.ts 96.39% <100.00%> (+<0.01%) ⬆️
src/mcp/telemetry.ts 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 19, 2026
@loopover-orb

loopover-orb Bot commented Jul 19, 2026

Copy link
Copy Markdown

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-19 12:14:41 UTC

4 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR correctly wires MCP tool-call telemetry through Cloudflare's `waitUntil` instead of firing it and forgetting it, and makes `recordMcpToolCall`/`recordMcpToolTelemetry` async, awaiting `client.flush()` so `waitUntil` has real work to hold the execution context open for. The reasoning matches PostHog's own fire-and-forget `capture()` semantics, the no-throw and safe-no-op invariants are preserved and covered by updated tests, and a new test explicitly asserts the telemetry promise is handed to `waitUntil` rather than awaited inline before the response returns. This is a narrow, well-targeted fix for a real dropped-telemetry bug on Workers, with an issue reference (#7233) and solid test coverage of both success and flush-failure paths.

Nits — 4 non-blocking
  • src/mcp/server.ts:1661 — the `400` status threshold is a repeated magic number; consider a named constant if it appears elsewhere in the file.
  • The PR description claims 'no change to handleMcpRequest's own response latency,' but since `recordMcpToolTelemetry` is invoked before `return response`/`throw error`, confirm the `waitUntil` call itself doesn't block the return (it shouldn't, since `waitUntil` doesn't await, but worth a one-line comment near the call site for future readers).
  • Consider extracting the `400`-status-as-failure threshold in src/mcp/server.ts into a small named check (e.g. `response.status < 400`) if the pattern recurs elsewhere in the file, for readability.
  • The doc comment in src/mcp/telemetry.ts is fairly long for a function-level comment; consider trimming to the essential why (flush is needed because capture() is fire-and-forget) and moving historical issue narrative to the PR description.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7233
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 1953 registered-repo PR(s), 1282 merged, 51 issue(s).
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1953 PR(s), 51 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff makes recordMcpToolCall/recordMcpToolTelemetry async and awaits client.flush(), and both call sites in handleMcpRequest (success and catch) now pass the returned promise to executionCtx.waitUntil, matching the scheduleAfterResponse pattern requested; it also adds regression tests covering waitUntil usage and the flush no-throw paths.

Review context
  • Author: galuis116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: JavaScript, Python, Dart, TypeScript, HTML, MDX, Rust, C++
  • Official Gittensor activity: 1953 PR(s), 51 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 80f4aec into JSONbored:main Jul 19, 2026
15 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 19, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP tool-call telemetry fires without waitUntil, unlike this repo's own scheduleAfterResponse pattern

1 participant