Skip to content

Deflake CI: TUF warn mode, readiness diagnostics, test races - #6063

Open
JAORMX wants to merge 6 commits into
mainfrom
deflake-ci-e2e
Open

Deflake CI: TUF warn mode, readiness diagnostics, test races#6063
JAORMX wants to merge 6 commits into
mainfrom
deflake-ci-e2e

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

The merge queue is being blocked by a handful of recurring CI flakes. Analysis of the last three weeks of Main build runs on main (146 runs, 75 failed) ranks the group-workload readiness family as the top three flakes (8, 6, and 4 failures respectively across rm_group_test.go and group_rm_test.go), with a conformance gating gap, an operator acceptance-test race, and an envtest watch-test race behind them. This PR fixes one proven product bug, closes the conformance gating gap, fixes two racy test assertions, and makes the remaining (not yet fully explained) readiness stall diagnosable from CI output instead of invisible.

What the failure evidence actually shows, per flake:

1. Sigstore TUF outages hard-fail thv run in warn mode (product bug, root-caused). In run 30297766499 (the push of #6043 itself), five specs across THVIgnore, List Group, and the group suites failed in one job while tuf-repo-cdn.sigstore.dev was resetting connections: thv run fetch exited 1 with failed to create TUF client ... connection reset by peer. The cause is an inconsistency in VerifyImage: in the default warn mode, an actual failed verification only warns, but a verifier construction failure (TUF unreachable) and any non-signature verification error were returned as fatal. Warn mode now warns and continues on every verification failure; enabled still fails closed. The new unit test forces the same construction-failure path deterministically (a SigstoreURL with no embedded TUF root) with no network involved — it fails against the old code and passes with the fix.

2. Readiness timeouts (should delete group with workloads and siblings, #6040/#6043) — diagnosability, not a timeout bump. #6040 is closed and #6043 ("Harden group e2e workload naming and waits", be0e0c34) was its fix, but the failure is still live: PR #6054, whose base is be0e0c34, failed identically. Mining the post-#6043 failures shows the 2-minute budget is not obviously the problem, but not for the reason previously assumed:

  • In Let Cedar read user claims the upstream asserts only in its ID token #6054's failure the third workload did not have "well over 2m of wall clock" — the first two waits returned near-instantly (their workloads were already running), so the failed workload had almost exactly its own 2m budget from launch.
  • Its container was up and its server listening within 1 second of thv run returning (Server listening on 8080 at 20:22:27; timeout at 20:24:29). The stall is inside the detached supervisor, somewhere between transport start and the running status flip.
  • On main (run 30297766499) the same family failed differently: workloads flipped to a bare status "error" with no context, container again healthy.

Neither CI log can say why, because of three diagnosability gaps this PR closes:

  • startWorkload and the detached-spawn failure path set status error with an empty context, clobbering the failure reason RunWorkload's retry loop had just recorded. They now record err.Error().
  • The readiness probe (waitForInitializeSuccess) logged per-attempt outcomes at DEBUG only, invisible in CI; a workload stuck in starting left no trace of what the probe observed. It now logs an INFO progress line every 30s with the last observed outcome and includes it in the timeout error.
  • DebugServerState dumped only container logs, which show a healthy server precisely in these failures; the supervisor's own output goes to the proxy log file. It now dumps thv logs --proxy too.

I deliberately did not raise ServerReadyTimeout: the two observed post-#6043 failure modes are a hard error (a bump is irrelevant) and a stall of unknown depth (a bump is speculation). The product's own probe budget is 5m before it continues anyway, so if the next diagnosable failure shows slow-but-recovering supervisors, TOOLHIVE_E2E_SERVER_READY_TIMEOUT (already supported, no code change) is the follow-up knob.

3. Conformance quarantine list was incomplete. Suite v0.1.16 has exactly four scenarios that register client-side handlers for server→client requests and are therefore exposed to the upstream ordering race (modelcontextprotocol/conformance#407): tools-call-sampling, tools-call-elicitation, elicitation-sep1034-defaults, elicitation-sep1330-enums — verified from the suite source (src/scenarios/server/tools.ts, elicitation-{defaults,enums}.ts), not by name-matching. The gate only ignored the first two; on #6054 the race hit tools-call-sampling and elicitation-sep1034-defaults (both timing out at exactly 60s), so the job hard-failed. The alternation now covers all four; anything else still fails the job, and the block still documents its removal condition (#407 fixed upstream + CONFORMANCE_VERSION bump).

4. dual_era_k8s_test.go asserted reconcile-lagging status bare. MCPServer.Status (the SessionStorageWarning condition and ReadyReplicas) is written by the operator's reconcile loop, which lags pod readiness; the spec asserted it immediately after the pods-ready polls and failed with ReadyReplicas 1 vs 2 observed 2ms after the pod check passed (PR #6055's run). The status read is now polled like every other eventually-consistent read in the file. The sibling specs and the rest of test/e2e/thv-operator were checked for the same bare-assert-on-status pattern; this was the only instance.

5. CompositeToolDefinition watch test races controller settle (3 main failures since 2026-07-06). The "should NOT trigger reconciliation" spec captures a Ready-condition-timestamp baseline right after a BeforeAll gate that only proves ObservedGeneration > 0, so a still-settling controller flips the condition inside the 2s observation window and fails the spec (confirmed in run 30250838615: timestamp changed within the 2.78s spec). The spec now waits for the Ready condition's transition time to hold stable across ~2s of polls before taking the baseline.

References: #6040 (closed), #6043, #5886, #5888, modelcontextprotocol/conformance#407. Note for maintainers: #6040 is closed against a failure that is still live post-#6043 — per the analysis in point 2, the remaining starting-stall mechanism is not yet root-caused; this PR makes the next occurrence name its cause. I have deliberately not reopened or commented on the issue.

Type of change

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

Test plan

  • Unit tests

  • E2E tests (task test-e2e)

  • Linting (task lint-fix)

  • Manual testing (describe below)

  • go build ./..., go vet and go test on every touched package (pkg/runner/retriever, pkg/runner, pkg/workloads) are green. task test was not used — it panics locally on the known gotestfmt v2.5.0 bug — so the underlying go test was run directly; CI runs the real thing. task lint-fix reports 0 issues.

  • The new TestVerifyImage case warn with unavailable verifier continues was proven to be a real regression guard: it fails against the pre-fix retriever.go (verified by stashing the fix) and passes after.

  • The conformance gate change was exercised with a scripted reproduction feeding synthetic suite output through the exact sed/grep pipeline, old regex vs new: the Let Cedar read user claims the upstream asserts only in its ID token #6054 combination (sampling + sep1034) fails old / passes new; a quarantined-only failure passes both; a genuine regression — alone, alongside a quarantined flake, or as a prefix-lookalike scenario id (tools-call-sampling-extended) — fails both.

  • Not run locally (no cluster/docker in this environment): the k8s operator suites and the CLI e2e suites. The dual_era_k8s_test.go and virtualmcpserver_compositetool_watch_test.go changes are compile-verified (go vet) only; the former is CI-only by its own header comment. The watch-test change runs in Operator CI on this PR.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File Change
pkg/runner/retriever/retriever.go Warn-mode VerifyImage warns and continues on verifier construction and verification errors; enabled unchanged (fails closed)
pkg/runner/retriever/retriever_test.go Table-driven TestVerifyImage: warn vs enabled vs disabled vs invalid, incl. a deterministic offline verifier-construction failure
pkg/workloads/manager.go Record err.Error() as status context in startWorkload and the detached-spawn failure path instead of clobbering it with ""
pkg/runner/runner.go Readiness probe: 30s INFO progress line with last observed outcome; timeout error includes it
test/e2e/helpers.go DebugServerState also dumps thv logs --proxy
test/e2e/thv-operator/acceptance_tests/dual_era_k8s_test.go Poll MCPServer.Status (SessionStorageWarning + ReadyReplicas) instead of asserting bare after pod readiness
test/conformance/run-conformance.sh Quarantine alternation extended to all four server→client scenarios; comment records the source-verified set
cmd/thv-operator/test-integration/virtualmcp/virtualmcpserver_compositetool_watch_test.go Wait for Ready-condition transition time to settle before capturing the no-reconcile baseline

Does this introduce a user-facing change?

Yes. With the default --image-verification=warn, thv run no longer fails when the sigstore TUF repository is unreachable (e.g. offline or a CDN outage) — it logs a warning and continues, consistent with how a failed verification already behaved in warn mode. --image-verification=enabled still fails closed. Additionally, a workload that fails to start now records the failure reason in its status context, and a slow-starting workload logs readiness-probe progress every 30s.

Special notes for reviewers

Generated with Claude Code

JAORMX and others added 6 commits July 27, 2026 20:56
In the default warn mode, VerifyImage returned verifier construction
errors and non-signature verification errors as fatal, while an actual
failed verification only warned. A sigstore TUF CDN connection reset
therefore hard-failed 'thv run' for any registry server with provenance
(observed repeatedly in CI, e.g. run 30297766499). Warn mode now warns
and continues on every verification failure; enabled mode still fails
closed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
startWorkload and the detached-spawn failure path wrote status error
with an empty context, clobbering the failure reason RunWorkload's
retry loop had just recorded; CI readiness timeouts then showed only a
bare status "error". Record err.Error() instead.

The readiness probe logged per-attempt outcomes at DEBUG only, so a
workload stuck in starting left no visible trace of what the probe
observed. Log an INFO progress line every 30s with the last observed
outcome, and include it in the timeout error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
DebugServerState only printed container logs, which show a healthy
server while the workload sits in starting or error: the failure is in
the detached supervisor, whose output goes to the proxy log file. Dump
'thv logs --proxy' too so a CI timeout names the actual blocker.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
The SessionStorageWarning condition and ReadyReplicas are written by
the operator's reconcile loop, which lags pod readiness; asserting them
bare right after the pods-ready polls flaked with ReadyReplicas 1 vs 2
observed 2ms after the pod check passed (PR #6055's run). Poll the
status like every other eventually-consistent read in the spec.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
Suite v0.1.16 has four scenarios that drive a server->client request
and are exposed to the upstream ordering race (conformance#407):
tools-call-sampling, tools-call-elicitation,
elicitation-sep1034-defaults and elicitation-sep1330-enums. The gate
only ignored the first two, so runs where the race hit an
elicitation-sep scenario (both hit on one PR run, each timing out at
exactly 60s) still hard-failed. Extend the alternation to the set
verified from the suite source; any other scenario still fails the job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
The unrelated-composite-tool spec captured its Ready-condition
timestamp baseline right after a gate that only proves
ObservedGeneration > 0, so a controller still settling flipped the
condition inside the 2s observation window and failed the spec (3
main-branch failures since 2026-07-06). Wait for the transition time
to hold stable across ~2s of polls before taking the baseline.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.25%. Comparing base (be0e0c3) to head (d851789).

Files with missing lines Patch % Lines
pkg/runner/runner.go 58.33% 4 Missing and 1 partial ⚠️
pkg/workloads/manager.go 0.00% 2 Missing ⚠️
pkg/runner/retriever/retriever.go 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6063      +/-   ##
==========================================
+ Coverage   72.18%   72.25%   +0.06%     
==========================================
  Files         721      721              
  Lines       75062    75075      +13     
==========================================
+ Hits        54183    54242      +59     
+ Misses      17017    16953      -64     
- Partials     3862     3880      +18     

☔ 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.

@ChrisJBurns

Copy link
Copy Markdown
Collaborator

/retest

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.

3 participants