Flaky: group rm "should delete group with workloads" times out waiting for readiness

[JAORMX]

Summary

Group RM E2E Tests > thv group rm command > [It] should delete group with workloads (test/e2e/group_rm_test.go:124) fails intermittently in the E2E Tests Core (core) shard, on commits that have nothing to do with groups or workloads.

Summarizing 1 Failure:
  [FAIL] Group RM E2E Tests  thv group rm command  [It] should delete group with workloads  [core, groups, e2e]
  /home/runner/work/toolhive/toolhive/test/e2e/group_rm_test.go:124

Ran 60 of 507 Specs in 506.503 seconds
FAIL! -- 59 Passed | 1 Failed | 0 Pending | 447 Skipped

Where it fails, and why that matters

Line 124 is the readiness assertion inside the wait loop, not one of the group rm assertions:

// Verify all workloads are running
for _, workloadName := range []string{groupWorkload1, groupWorkload2, nonGroupWorkload1, nonGroupWorkload2} {
    err := e2e.WaitForMCPServer(config, workloadName, e2e.ServerReadyTimeout())
    Expect(err).ToNot(HaveOccurred())   // <-- line 124
}

So the spec is not finding a bug in thv group rm — it is failing to get four containerised workloads ready in time. This is the heaviest setup of any spec in the file (the neighbouring specs start two), and it runs on a CI runner concurrently hosting other e2e shards. That makes it the natural first casualty of any transient slowness in image pull, container start, or proxy readiness.

Notably the four workloads are started sequentially and each waited on separately, but the failure surfaces as a bare "workload didn't become ready" without saying which of the four, because the loop reuses one assertion with no message. That alone makes triage harder than it needs to be.

Evidence it is pre-existing, not caused by any PR

The core shard has failed on main on unrelated commits, and passed on others:

Run	Commit	E2E Tests Core (core)
30268388184	f1dae946	❌
30264963508	d3907301	❌
30264573849	96f25d05	❌
—	8a860d9b	✅
—	e837d694	✅

It most recently blocked #6031, whose diff touches only pkg/transport/session and pkg/transport/proxy/transparent — nothing reachable from thv group rm.

Secondary hazard worth fixing at the same time

Workload names in this file are built from GinkgoRandomSeed():

groupWorkload1 := fmt.Sprintf("group-rm-group-workload-1-%d", GinkgoRandomSeed())

GinkgoRandomSeed() is the suite's seed — constant for the whole run, not per-spec. So these names are stable across every spec in a run and, more importantly, are not unique across runs. A leftover container from a previous run that used the same seed collides by name. Every other e2e suite in this repo uses e2e.GenerateUniqueServerName for exactly this reason; group_rm_test.go has zero uses of it. That is a latent second flake source even if the timeout is addressed.

Suggested directions

Not prescribing, since the right call depends on whether the timeout is genuinely too tight or the spec is simply doing too much:

• Name the failing workload. Add the workload name to the assertion message so the next failure says which of the four timed out — cheap, and turns this from a guess into a fact.
• Reduce the setup. The spec's actual subject is "group workloads survive group rm, non-group workloads keep running". It may not need four workloads to establish that; two (one in-group, one out) might suffice.
• Switch to e2e.GenerateUniqueServerName throughout the file, matching the rest of the suite.
• Check whether ServerReadyTimeout() is realistic for four sequential container starts on a shared runner, versus the two the neighbouring specs use.

Context

This is the fourth distinct pre-existing flake to surface in our PRs today, alongside #6026 (optimizer search-quality spec), #6034 (getFreePort bind-close-rebind race), and a transient curl: (35) fetching the kind binary in helm/kind-action. Filing this one because it has a clear signature and a reproducible history on main, and because it has already sent one investigation down the wrong path — a red core shard on an unrelated PR reads as that PR's fault until someone opens the log.

Generated with Claude Code

[JAORMX]

Additional corroboration: fails on an unrelated third-party branch too

Independent confirmation while investigating #6031, worth adding because it rules out the two remaining "maybe it is that PR's fault" readings:

• E2E Tests Core (core) failed as the only failing job on the unrelated PR branch gautam/metrics-std-toolhive (run 30279727366) — a different author, a different subsystem (metrics standardisation), no involvement with groups or the transparent proxy.
• Same shard on main: failed at f1dae946, d3907301, 96f25d05; passed at 8a860d9b, e837d694.

So it is reproducibly intermittent across at least three unrelated branches and main itself, which is consistent with the readiness-timeout mechanism described above rather than anything code-specific.

One clarification on provenance, since it affects how much weight to give each claim: the spec identification (group_rm_test.go:124, the readiness wait for the 4th workload) comes from the Ginkgo summary quoted in this issue's description, read with an authenticated token. The cross-branch failure history above was gathered separately from the Actions API. Both point the same way, but they are two different sources rather than one restated.

Cost so far: this has consumed a CI cycle on #6031 and briefly sent that investigation toward the wrong subsystem — a red core shard reads as the PR author's fault until someone opens the log, which needs privileges most contributors reading a failed check will not use.

[JAORMX]

Correcting this issue: the workload-count premise is wrong

I wrote the description above and got its central inference wrong. Recording it here rather than only in the PR, because a wrong lead in an issue costs whoever picks it up next.

What I claimed: the failing spec is "the heaviest setup of any spec in the file (the neighbouring specs start two)", implying four concurrent workloads is what makes it the first casualty.

What the evidence says. GitHub check-run annotations are publicly readable — GET /repos/{owner}/{repo}/check-runs/{job_id}/annotations works unauthenticated and carries the failing file and line, even where job logs (403) and JUnit artifacts (401) do not. For the three failing core runs on main:

Run	Commit	Failure location	Workloads that spec starts
30264573849	96f25d05	group_rm_test.go:124 and list_group_e2e_test.go:200	4, and 2
30264963508	d3907301	group_rm_test.go:159	2
30268388184	f1dae946	group_rm_test.go:159	2

Three of the four observed failures are two-workload specs, and one is in a different file entirely (list_group_e2e_test.go) that I never looked at. Line 159 is the --with-workloads spec, not the one this issue is titled after.

Corrected diagnosis: the common factor is not workload count but starting two or more fetch workloads back to back. thv run detaches, so containers, proxies and initialize probes come up concurrently. Every group spec that does this has flaked; every group spec that starts one workload at a time has not.

Two consequences for anyone working this:

• Do not reduce the workload count. It would not have prevented three of the four failures, and it would weaken the only thing that spec proves (group rm leaves every member and every non-member running). Harden group e2e workload naming and waits #6043 records that reasoning at the site so it does not get "fixed" that way later.
• Do not raise the timeout again. I suggested checking whether ServerReadyTimeout() is realistic without noticing that Make e2e workload readiness waits reliable #6012 (f1dae946) had already raised it 60s → 2m, citing the product's 5-minute initialize budget. Exactly one failure has occurred since. A further bump hides the cause, and the core shard has a 15-minute Ginkgo budget against a ~506s passing run.

Scope also wider than filed: rm_group_test.go contains a near-identical four-workload spec with the same seed-only names, and list_group_e2e_test.go has already flaked on the same wait. #6043 covers all three.

Still not fixed, and #6043 says so plainly: nothing there makes a slow start faster. What it adds is an assertion that names which workload stalled — the missing input. Getting further needs the last observed: status WaitForMCPServer prints on timeout, which lives in the auth-gated job logs.

Leaving this issue open: #6043 improves diagnosability and fixes a distinct name-collision hazard, but the underlying slow-concurrent-start problem is unresolved.

[JAORMX]

#6043 merged as be0e0c34. This issue stays open — the flake is not fixed.

Recording that explicitly because the PR was originally written with Closes #6040, which was downgraded to Refs before merge. What landed is diagnosability, not a fix:

• e2e.ExpectMCPServersRunning now names the workload whose readiness wait timed out, and reports at the caller's line so the CI annotation still points at the spec.
• Workload names in group_rm_test.go and rm_group_test.go no longer come from GinkgoRandomSeed() alone (a real hazard for local reruns, but almost certainly not the cause here — CI runners are ephemeral).

#6043's own CI reproduced the flake, and broader than baseline. Run 30293849786 on 014b5a82 failed two group specs rather than one:

[FAIL] Group RM E2E Tests     [It] should delete group with workloads    group_rm_test.go:123
[FAIL] Group Remove E2E Tests [It] should remove workloads from group    rm_group_test.go:82
Ran 60 of 507 Specs — 58 Passed | 2 Failed        (781s)

The :124 → :123 line move confirms the new helper is in the failing path, so the next occurrence should name the stalled workload. Suite runtime also rose from ~506s to 781s (+54%) on a change that adds no work, which points at host-level contention on the runner.

Two corrections to my original framing in this issue, both now evidenced:

• Workload count is not the factor. Check-run annotations (publicly readable, no auth needed) show three of the four observed failures were in two-workload specs, not the four-workload one. Reducing the setup would not have prevented any of them.
• The timeout was already raised 60s → 2m in Make e2e workload readiness waits reliable #6012 (f1dae946), so "raise the timeout" was already spent.

The actual common factor looks like starting two or more fetch workloads back to back — thv run is detached, so containers, proxies and initialize probes come up concurrently. Every group spec that does this has now flaked; every spec that starts one workload at a time has not.

What's still needed: the last observed: status that WaitForMCPServer prints on timeout, for the stalled workload. That lives in the job logs and the JUnit artifact (e2e-test-results-core), which need credentials to download. With the helper now in place, the next red run should surface the workload name in the annotation itself.