Skip to content

Make e2e workload readiness waits reliable - #6012

Merged
JAORMX merged 3 commits into
mainfrom
flaky-e2e-workload-readiness
Jul 27, 2026
Merged

Make e2e workload readiness waits reliable#6012
JAORMX merged 3 commits into
mainfrom
flaky-e2e-workload-readiness

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

The Group Remove spec times out waiting for a freshly started workload to reach running. Three separate problems, all in test infrastructure:

  • The spec-side budget is far tighter than the product's own. Reaching running is not just "container started": the detached worker starts the container, brings up the transparent proxy, then polls the workload's own MCP endpoint until initialize succeeds, and only then flips the status (pkg/runner.waitForInitializeSuccess, pkg/runner/runner.go:572) — a probe the product is willing to wait 5 minutes for. The group specs gave it 60s while starting up to 4 workloads at once, so they fail while the workload is still legitimately starting on a loaded runner. Waits now take their budget from e2e.ServerReadyTimeout() (2m default, overridable via TOOLHIVE_E2E_SERVER_READY_TIMEOUT).

  • The readiness wait was not actually checking the named workload. It matched strings.Contains(stdout, serverName) && strings.Contains(stdout, "running") against the whole thv list table, i.e. "this name appears somewhere and some workload is running" — so it could return while the named workload was still starting. It now reads that workload's own record from thv list --all --format json. The same defect was present in three places, all fixed: WaitForMCPServer, IsServerRunning, and waitForIsolatedMCPServer (a copy of the old body in the upgrade spec). The lookup is now shared via an exported e2e.FindWorkload, which takes the command builder so the isolated config/home/data spec can supply its own and observe its own state.

  • The failure was undiagnosable. timeout waiting for MCP server X to be running said nothing about why. The error now carries the last status observed (including status_context) and dumps server state via the existing DebugServerState helper, so the next occurrence tells us whether the workload was still starting, had errored, or never appeared. The poll's own thv list is bounded at 30s, since Run() would apply TestConfig.TestTimeout (10m) and let one hung list outlast the wait it is being polled for.

  • CI's pre-pull had silently gone stale. The step exists so workload startup does not pay the image-pull cost, but it hardcoded tags for images the tests reach by registry name. Renovate bumps github.com/stacklok/toolhive-catalog daily and has no manager for these docker pull lines, so they drifted:

    server catalog (what tests run) what CI pre-pulled
    fetch gofetch/server:1.0.5 gofetch/server:1.0.2
    time mcp-server-time:2026.7.10 mcp-server-time:2026.1.26
    osv osv-mcp/server:0.1.3 osv-mcp/server:0.1.3

    Those tags are now resolved from the binary under test (thv registry info <server> --format json), so they cannot drift again, and the step fails loudly if an image did not land locally (wait reports success even when a background pull failed).

Fixes #6011

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Docker is unavailable in my environment, so the e2e suite itself is verified by this PR's CI — that is the point of opening it as a draft. Verified locally:

  • task lint → 0 issues; go vet ./test/e2e/... clean.
  • The catalog drift, by resolving the images through the registry provider the tests actually use: fetchghcr.io/stackloklabs/gofetch/server:1.0.5, timeghcr.io/stacklok/dockyard/uvx/mcp-server-time:2026.7.10.
  • The new pre-pull shell logic, executed for the core, proxy && !isolation and network-isolation matrix entries against a stubbed docker, confirming the resolved tags and per-matrix image sets.
  • That thv registry info --format json emits clean JSON on stdout (the update-notification banner goes to stderr, and is skipped in CI anyway), so both the jq call and the JSON parse in FindWorkload are safe.

Changes

File Change
test/e2e/helpers.go ServerReadyTimeout(); exported FindWorkload; WaitForMCPServer/IsServerRunning read the workload's own record; timeout diagnostics; bounded list poll
.github/workflows/e2e-tests.yml Resolve registry-server image tags from the binary under test; fail if a pre-pull did not land
test/e2e/upgrade_e2e_test.go waitForIsolatedMCPServer uses the shared lookup instead of a copy of the old loose match
test/e2e/rm_group_test.go, group_rm_test.go, group_test.go, list_group_e2e_test.go Use the shared readiness budget instead of a hardcoded 60s

Does this introduce a user-facing change?

No.

Special notes for reviewers

  • Watch the full e2e matrix on this PR, not just the core entry. Tightening the wait to the named workload makes ~50 call sites wait for something they previously could skip. That is the correct contract, but any spec that was passing only because another workload happened to be running will now surface. If something unrelated goes red, that is the signal — I'd rather find it here than leave the helper accepting the wrong thing.
  • No fail-fast on error status, deliberately. It is tempting to abort the wait as soon as the workload reports error, but the status is not monotonic across restarts (thv restart writes stopped/starting after a previous error), so a spec that restarts a workload could read a stale error and fail spuriously. The status is reported in the timeout message instead, which never fails earlier than the old code did.
  • upgrade_e2e_test.go keeps its two explicit 120*time.Second waits: that equals the new default anyway, and the author raised them above 60s deliberately for the slower upgrade flow, so leaving them hardcoded preserves that intent against an env override. Only its 60s waits moved to the shared budget. Remaining 60s call sites elsewhere are untouched to keep this reviewable; ServerReadyTimeout() is there to adopt if the same family shows up again (E2E tests are intermittently flaky on CI #5690, Flaky api-workloads E2E suite: 15m TEST_TIMEOUT too tight for the workload-heavy matrix entry #5265, e2e vmcp tests are flaky #2741).
  • The stale pre-pull does not by itself explain the 60s timeout — the pull happens synchronously inside thv run (cmd/thv/app/run_flags.go:417), which the specs allow 10 minutes for. It burns suite wall-clock and pushes the workload-heavy matrix entries toward TEST_TIMEOUT, which is the Flaky api-workloads E2E suite: 15m TEST_TIMEOUT too tight for the workload-heavy matrix entry #5265 family, so it is fixed here rather than filed separately.

Generated with Claude Code

The group specs time out waiting for a freshly started workload to reach
the running state on loaded CI runners. Reaching running covers the
container start plus ToolHive's own initialize probe against the proxy,
which the product waits up to 5 minutes for, so a 60s spec-side budget
fails while the workload is still legitimately starting.

WaitForMCPServer also matched the name and the word running anywhere in
the list table, so it could return before the named workload was running,
and its timeout carried no diagnostics.

Separately, CI pre-pulled hardcoded image tags for servers the tests
reach by registry name. Renovate bumps toolhive-catalog daily but has no
manager for those docker pull lines, so fetch (1.0.2 vs 1.0.5) and time
(2026.1.26 vs 2026.7.10) had drifted and the pre-pull no longer covered
what the tests run. Resolve those tags from the binary under test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 27, 2026
Run() applies TestConfig.TestTimeout (10 minutes), so a single hung
list could outlast the readiness budget it is polling for. Callers
poll, so an overrunning list is retried rather than fatal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
waitForIsolatedMCPServer was a copy of WaitForMCPServer's old body and
carried the same defect: it matched the name and the word running
anywhere in the list table. Export FindWorkload, which takes the command
builder so an isolated config/home/data spec can supply its own, and
have both waits read the named workload's own record.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.14%. Comparing base (d390730) to head (19c2144).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6012      +/-   ##
==========================================
+ Coverage   72.11%   72.14%   +0.03%     
==========================================
  Files         720      720              
  Lines       74745    74745              
==========================================
+ Hits        53899    53923      +24     
+ Misses      16993    16952      -41     
- Partials     3853     3870      +17     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX
JAORMX marked this pull request as ready for review July 27, 2026 12:59
@JAORMX
JAORMX merged commit f1dae94 into main Jul 27, 2026
45 checks passed
@JAORMX
JAORMX deleted the flaky-e2e-workload-readiness branch July 27, 2026 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky E2E: Group Remove "should remove workloads from group" times out waiting for workload to be running

2 participants