Skip to content

Drain backend notifications before per-call client close#5960

Merged
JAORMX merged 1 commit into
mainfrom
deflake-integration-tests
Jul 24, 2026
Merged

Drain backend notifications before per-call client close#5960
JAORMX merged 1 commit into
mainfrom
deflake-integration-tests

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

TestForwarding_Progress_RealBackend (and _Logging_) flaked on CI because a fire-and-forget backend notification emitted mid tools/call (notifications/progress, notifications/message) was dropped under load, not merely delayed.

The notification and the tool result travel on separate streams — the notification on the standalone SSE stream, the result on the tools/call response stream — read through a single FIFO receive loop. CallTool returns as soon as the result is read, and its deferred Close cancels that loop; a notification not yet drained off the channel is then discarded and never reaches the downstream client. That is why the earlier fix (bumping the downstream wait 5s→15s) never helped — the message is lost, not slow. Blocking server→client requests (elicitation/sampling) are immune because the backend tool blocks on the response, so the client is still reading when they arrive.

Fix: before Close, when server→client forwarding is bound, issue a synchronous Ping as a drain barrier. The backend flushes the notification onto the wire before returning the result, so by the time CallTool returns its bytes are already buffered on the client; the ping's round-trip lets the receive loop drain that buffered notification off the shared channel and enqueue it for handling, and Close then waits for enqueued handlers before teardown. Best-effort (the result is already in hand) and skipped entirely when forwarding is not bound, so non-forwarding calls keep the fast path with no extra round-trip.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Test plan

  • Unit tests (task test) — pkg/vmcp/server, pkg/vmcp/client pass with -race
  • Linting (task lint-fix) — 0 issues
  • Manual testing (describe below)

Reproduced reliably under CPU contention before the fix (progress + logging forwarding tests time out); after the fix the full TestForwarding cluster passes 30/30 under 24× load and under -race -count=20.

API Compatibility

  • This PR does not break the v1beta1 API (no operator/CRD surface is touched).

Special notes for reviewers

  • Rebased onto latest main. This PR originally also carried a fix for the standalone-SSE filtered-tool-call test, but that was landed independently in Update standalone-SSE test for filtered-tool error #5958, so that commit was dropped on rebase — only the forwarding fix remains.
  • This is the minimal in-repo mitigation. The robust fix belongs upstream in the SDK / toolhive-core mcpcompat: route mid-call notifications on the correlated request stream instead of the lossy standalone stream, or drain gracefully on close. Worth a linked upstream issue as follow-up.
  • Tradeoff: one extra backend round-trip per forwarding tools/call, only when server→client forwarding is bound. The per-call client already does Start+Initialize+Close, so this is marginal.

Generated with Claude Code

@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 24, 2026
rdimitrov
rdimitrov previously approved these changes Jul 24, 2026
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.89%. Comparing base (00b8159) to head (c103295).

Files with missing lines Patch % Lines
pkg/vmcp/client/forwarding.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5960      +/-   ##
==========================================
+ Coverage   71.82%   71.89%   +0.07%     
==========================================
  Files         708      708              
  Lines       72798    72805       +7     
==========================================
+ Hits        52287    52345      +58     
+ Misses      16772    16708      -64     
- Partials     3739     3752      +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TestForwarding_Progress_RealBackend (and _Logging_) flaked because a
fire-and-forget backend notification emitted mid tools/call was lost
under load. The notification and the tool result travel on separate
streams (standalone SSE vs the tools/call response), read through one
FIFO receive loop; CallTool returns as soon as the result is read and
its deferred Close cancels that loop, so a not-yet-drained notification
is discarded and never forwarded downstream -- a permanent loss, which
is why bumping the downstream wait timeout never fixed it. Blocking
server->client requests (elicitation/sampling) are immune because the
backend tool blocks on the response.

Before Close, when server->client forwarding is bound, issue a
synchronous ping as a drain barrier: its round-trip lets the receive
loop drain the already-buffered notification off the shared channel and
enqueue it for handling before teardown. Best-effort (the result is
already in hand) and skipped entirely when forwarding is not bound, so
non-forwarding calls keep the fast path.

This is the minimal in-repo mitigation; the robust fix is upstream in
the SDK (route mid-call notifications on the correlated request stream,
or drain gracefully on close). Tracked for follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79
@JAORMX
JAORMX force-pushed the deflake-integration-tests branch from 58ea112 to c103295 Compare July 24, 2026 07:23
@JAORMX JAORMX changed the title Fix broken filtered-tool-call test and drop-notification race Drain backend notifications before per-call client close Jul 24, 2026
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Jul 24, 2026
@JAORMX
JAORMX requested a review from rdimitrov July 24, 2026 07:24
@JAORMX

JAORMX commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto latest main — which now includes #5958, the independent fix for the standalone-SSE filtered-tool-call test. That made this PR's SSE commit redundant, so it was dropped on rebase; only the notification-drain forwarding fix remains (the exact pkg/vmcp/client change already reviewed, now on a current base). The force-push dismissed the prior approval — @rdimitrov, re-requesting since it's the same forwarding code you looked at, minus the SSE commit. CI is re-running.

@JAORMX
JAORMX merged commit 0192285 into main Jul 24, 2026
169 of 176 checks passed
@JAORMX
JAORMX deleted the deflake-integration-tests branch July 24, 2026 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants