Deflake vMCP forwarding notification wait - #5941
Merged
Merged
Conversation
TestForwarding_Progress_RealBackend (and its sibling forwarding tests) flaked on CI: waitNotification used a hardcoded 5s deadline for an async, server-initiated notification relayed backend -> vMCP -> downstream. Under the full-suite parallel -race load the round trip exceeds 5s and the test times out, while it passes in milliseconds locally. Raise the wait deadline to 15s, leaving headroom under the callers' 20s context so a genuine hang still fails cleanly instead of blocking to the context deadline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JAORMX
requested review from
ChrisJBurns,
amirejaz,
jerm-dro,
jhrozek and
tgrunnagle
as code owners
July 23, 2026 12:48
3 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5941 +/- ##
==========================================
- Coverage 71.81% 71.76% -0.06%
==========================================
Files 705 705
Lines 72164 72164
==========================================
- Hits 51828 51790 -38
- Misses 16621 16673 +52
+ Partials 3715 3701 -14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
amirejaz
approved these changes
Jul 23, 2026
This was referenced Jul 24, 2026
Flaky: TestForwarding_*_RealBackend time out waiting for forwarded notifications under CI load
#5962
Closed
JAORMX
added a commit
that referenced
this pull request
Jul 24, 2026
TestForwarding_*_RealBackend intermittently timed out on CI waiting for a forwarded, server-initiated notification. waitNotification used a hardcoded time.After deadline (5s originally, 15s after #5941) that sat inside the per-test context: a timer shorter than the context flaked under the full-suite parallel -race load (the async backend -> vMCP -> downstream relay can take many seconds), while a longer one would mask a genuine hang. Collapse the two deadlines into one: waitNotification now selects on the caller's ctx.Done() instead of its own timer, and every real-backend forwarding test shares a single, generous forwardingRealBackendTimeout (60s) — well under the 10m go-test global timeout, so a real hang still fails fast with a clear ctx error. notifCh stays buffered at 8; each test emits and reads a single notification, so drop-on-full is not a factor. Verified with `go test -race -count=5 -run TestForwarding_ ./pkg/vmcp/server/`. Closes #5962. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6 tasks
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
2 participants
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.
Summary
Why:
TestForwarding_Progress_RealBackend(and its siblingTestForwarding_*_RealBackendtests) flake on CI.waitNotificationused a hardcoded 5s deadline while waiting for an async, server-initiated notification relayed backend → vMCP → downstream client. Under the full-suite parallel-race+-coverpkg=./...load on the shared CI runner, that round trip can exceed 5s and the test times out (forwarding_realbackend_integration_test.go:292: timed out waiting for notifications/progress notification) — while it passes in ~30ms locally with no load. Observed failing twice consecutively on an unrelated PR's CI, passing 3/3 locally.What:
waitNotification's deadline from 5s to 15s, with a comment explaining the async/CI-load rationale. It stays under the callers' 20s test context, so a genuine hang still fails cleanly rather than blocking to the context deadline.Type of change
Test plan
go test -race -run 'TestForwarding_' ./pkg/vmcp/server/passes.task lintreports 0 issues.Does this introduce a user-facing change?
No. Test-only change.
Special notes for reviewers
notifCh) uses a non-blocking send that drops on a full buffer — not the cause here (the buffer isn't the bottleneck under these tests), so this PR only addresses the timeout. If the tests flake again after this, a follow-up could make the notification capture more robust.Generated with Claude Code