stacklok / stacklok/toolhive

Flaky: TestForwarding_Progress_RealBackend times out waiting for a forwarded notification

Open
#6,062 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
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:

  1. 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.
  2. 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.
  3. 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.