Flaky: TestForwarding_Progress_RealBackend times out waiting for a forwarded notification
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 2.2k
- Forks
- 300
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 184
Description
Symptom
TestForwarding_Progress_RealBackend (pkg/vmcp/server/forwarding_realbackend_integration_test.go) intermittently fails in the Tests / Test Go Code job, timing out at ~61s waiting for a forwarded notifications/progress that never arrives.
Observed on commit 7f3d1b27 (PR #6051). That PR contains no production code — only _test.go files and one architecture doc — and does not touch either hop's notification streams, so it cannot be the cause. It surfaced under -race load in CI.
This is a unit-test-job flake, not one of the e2e/operator shard flakes we already know about (the group-rm one in #6040, or the container-runtime suites), so it is worth tracking separately.
Suspected mechanism
The code already documents the race: drainServerToClientNotifications uses a best-effort ping barrier to wait for in-flight server-to-client notifications before the test asserts on them. A ping round-trip is not a guarantee that a previously-sent notification has been delivered and dispatched — it only proves the connection is live and that the ping itself was processed. Under load, particularly with -race slowing every goroutine handoff, a progress notification can still be in flight when the barrier returns and the assertion runs.
Same lineage as the earlier lost-notification work tracked in #5962.
Why the durable fix is deliberate work rather than a retry
The honest options, roughly in order of preference:
- Strengthen the drain barrier so it waits on the actual notification pipeline rather than inferring liveness from a ping — e.g. an explicit sequence/ack the forwarder increments per delivered notification, which the barrier can wait on. This is production code in the forwarding path and wants its own PR and review.
- Poll the assertion with a deadline instead of draining once and asserting. Cheaper, and it converts a hang into a bounded wait, but it weakens the test into "eventually" semantics for something that should be deterministic.
- Raise the timeout. Not recommended — it hides whatever is actually slow and this already sits at 60s.
Option 1 is the real fix. Note that #6051 pins this test to the Legacy revision (progress has no deliverable channel for Modern clients — see #6058), so the flake affects only the Legacy forwarding path, which is the path that has to keep working.
What would help triage
The failure message on timeout does not currently say how many notifications were drained before giving up, or whether the backend had emitted the progress notification at all. A count in the failure output would distinguish "the backend never sent it" from "we asserted before it landed" — worth adding whichever fix is chosen.
Refs #6051, #5962.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with pkg/vmcp/server/forwarding_realbackend_integration_test.go and read drainServerToClientNotifications, then trace the forwarding path it exercises. Run TestForwarding_Progress_RealBackend under -race to reproduce the timeout. Done means the Legacy forwarding test deterministically observes the progress notification without merely increasing the timeout, with useful timeout diagnostics.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100