Deflake real-backend forwarding tests - #5963
Merged
Merged
Conversation
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>
JAORMX
requested review from
ChrisJBurns,
amirejaz,
jerm-dro,
jhrozek and
tgrunnagle
as code owners
July 24, 2026 08:24
aponcedeleonch
approved these changes
Jul 24, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5963 +/- ##
==========================================
- Coverage 71.84% 71.84% -0.01%
==========================================
Files 708 708
Lines 72811 72811
==========================================
- Hits 52311 52310 -1
- Misses 16765 16767 +2
+ Partials 3735 3734 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rdimitrov
approved these changes
Jul 24, 2026
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
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_*_RealBackend(pkg/vmcp/server/forwarding_realbackend_integration_test.go) intermittently time out on CI waiting for a forwarded, server-initiated notification (#5962).waitNotificationused a hardcodedtime.Afterdeadline (5s originally, 15s after #5941) inside the per-test context — a two-deadline design where a timer shorter than the context flaked under the full-suite parallel-raceload (the async backend → vMCP → downstream relay can take several seconds), while a longer timer would have masked a genuine hang. The 15s bump proved insufficient (TestForwarding_Logging_RealBackendtimed out at 15.09s).What:
waitNotificationnow selects on the caller'sctx.Done()instead of its own timer (and reportsctx.Err()on timeout) — a single source of truth for the deadline.const forwardingRealBackendTimeout = 60 * time.Second, replacing the per-test 20s/30s literals. 60s is well under the 10mgo testglobal timeout, so a real hang still fails fast with a clear error rather than blocking the whole suite.notifChis left buffered at 8; each test emits and reads a single notification, so drop-on-full is not a realistic flake source.Closes #5962.
Type of change
Test plan
go test -race -count=5 -run TestForwarding_ ./pkg/vmcp/server/— passes 5×.task lint0 issues;task buildsucceeds.Does this introduce a user-facing change?
No. Test-only change.
Special notes for reviewers
waitNotification'st.Fatalfruns on the test goroutine (called directly, not in a spawned goroutine), and eachdefer cancel()only fires on function return, so tying to the caller ctx is sound.withHandlers=false) that assertrequire.NotErrorIs(err, context.DeadlineExceeded)now use 60s instead of 20s. They fail near-instantly in practice, so the widened window is a negligible loss of timing sensitivity, kept for the single-const simplicity.Generated with Claude Code