Skip to content

Fix workload status poisoning and deflake Envoy AllowPort - #6076

Merged
JAORMX merged 2 commits into
mainfrom
fix-thv-list-status-poisoning
Jul 28, 2026
Merged

Fix workload status poisoning and deflake Envoy AllowPort#6076
JAORMX merged 2 commits into
mainfrom
fix-thv-list-status-poisoning

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two independent fixes for the e2e shard failures on main around ca4eda4, combined into one PR at the maintainer's request. They are separated below (and as two commits) so each can be evaluated on its own; neither is a regression fix — #6050/#6051/#6061 were ruled out on evidence (the failing vmcp spec died before thv vmcp serve ever launched).


1. thv list status poisoning (product bug; pkg/workloads/statuses)

thv list during any workload start can permanently persist unhealthy into a healthy workload's status file — and nothing recovers it. This is user-facing state corruption, not a test problem. The E2E Tests Core (vmcp) failure on main (run 30335474873, commit ca4eda4) is the symptom that exposed it: a backend workload that had demonstrably started and stayed alive was reported unhealthy (workload not found in runtime) for two full minutes.

Mechanism:

  • ListWorkloads snapshotted the runtime before the status files. Every thv run flips the file to running moments after the container is created, so a list whose runtime snapshot predates the container while its file read postdates the running write sees "file running, runtime missing".
  • handleRuntimeMissing then persists unhealthy into the status file.
  • Nothing recovers it: validateWorkloadInList skips runtime validation for every non-running file status, so an unhealthy file entry with a perfectly healthy container stays unhealthy forever.

What changed:

  • Reorder the snapshots: files first, then runtime. The ordering is load-bearing and now commented: a file that says running at time T implies its container existed before T, so a later runtime miss is genuine. The old order was introduced without stated rationale in e6e29dc (Fix bugs found while testing file-based state tracking #1347); GetWorkload was already file-first, so the two paths are now consistent.
  • Guard the unhealthy write (setUnhealthyIfStillPresent): re-check under the file lock and skip if the file is gone or no longer says running/stopped. The reorder alone would open a narrow mirror hazard — a slow thv list bracketing an entire thv rm could recreate the just-deleted status file as an unhealthy ghost, because SetWorkloadStatus creates missing files. The guard closes that and stops a stale verdict from clobbering a fresher status such as removing.
  • waitForIsolatedMCPServer (upgrade e2e) now dumps state through its isolated env on timeout. The core shard failure on the same run was this helper timing out with a bare boolean — no status context, no logs — which made that failure undiagnosable. (e2e.DebugServerState can't be reused here: it queries the real ToolHive config, not the spec's isolated env.)

Refs #4432 — that issue documents the same write-on-read corruption from a different trigger (multi-runtime). This PR fixes the startup trigger and hardens the write, but thv list still writes on read, so #4432's root cause (and its "read-only list path" direction) remains open. Deliberately Refs, not Closes.

Proven vs inferred: the vmcp flake is proven end-to-end (the workload's own container/proxy logs show a clean, live start while thv list reported the persisted "not found in runtime" context, and the race is reproduced deterministically in a unit test). The upgrade-spec flake on the same run is inferred from the shared code path — it polls thv list every second during workload recreation — but its log had no state dump, which is exactly what the diagnostics change fixes for next time.


2. Envoy AllowPort e2e deflake (test/e2e/network_isolation_envoy_test.go)

E2E Tests Core (network-isolation) failed on the pre-merge heads of both #6050 (1588e6a4) and #6051 (5c901a9d) — the latter containing zero production code — and passed on main minutes later. Both failures were the same spec, same assertion, within the same minute: the AllowPort spec's allowed leg, https://example.com must be allowed (port 443 is in AllowPort).

Root cause: the allowed leg fetched https://example.com exactly once, immediately after workload readiness. It needs a working DNS → TLS → upstream chain through Envoy's dynamic_forward_proxy, and the first egress request after startup can race the isolation stack's own warm-up (DFP DNS cache, the per-workload DNS container), failing instantly with a local error. The observed failures returned ~40ms and ~190ms after readiness — far too fast for genuine external TLS trouble — and the boolean-only assertion discarded the fetch tool's error text, so the log could not say why.

What changed: the allowed leg retries via Eventually (60s budget, 2s poll) and records the last fetch error text, which is printed on failure.

Why this is fixing an incidental assertion, not weakening a real one:

  • The property this spec pins (Envoy backend: translate AllowPort into egress policy #5915 parity: Envoy must honour AllowPort end-to-end) is steady-state reachability of an allowed port. First-request-after-boot success is not something Envoy's DFP path promises, so a single-shot first fetch asserts warm-up behaviour the product never guaranteed.
  • A genuinely broken AllowPort still fails — after the retry budget, and now with the fetch error text surfaced, which is the diagnostic that distinguishes a warm-up race from a real enforcement break on the next occurrence.
  • The blocked port-80 leg is untouched: denial is enforced locally and deterministically once the stack is warm, which the preceding allowed leg has just proven.

No timeout was raised; the change addresses the mechanism (first-request warm-up) and improves the failure's diagnosability. The warm-up diagnosis is the best fit for the evidence but could not be pinned to the exact Envoy error because the old assertion discarded the error text; if this spec ever fails again, the surfaced text settles it.


Type of change

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

Test plan

  • Unit tests (task test)
  • Linting (task lint; go vet ./... run separately)
  • Manual testing (describe below)

The statuses regression tests were mutation-verified against the code they pin:

  • TestFileStatusManager_ListWorkloads_DoesNotPoisonWorkloadStartingDuringList flips the status file to running from inside the mocked runtime call. Fails on the old snapshot order (persisted status corrupted runningunhealthy, reproducing the CI poisoning deterministically) and passes with the reorder.
  • TestFileStatusManager_ListWorkloads_UnhealthyMarkIsGuarded deletes the file / flips it to removing from inside the runtime call. Both subtests fail with the guard reverted to a plain SetWorkloadStatus (ghost file recreated; removing clobbered) and pass with the guard.

task test shows only the three known environment-dependent failures (TestNewServer_ReadTimeoutConfigured, TestSecurityHeaders, TestCompositeProvider_RealProviders). Not verified by execution: the e2e suites themselves (no container runtime in the dev environment) — the Envoy retry's effect on the flake is only observable in CI; it is verified by compilation, lint, and by checking the failure-time evidence against the retry semantics.

Does this introduce a user-facing change?

Yes. thv list no longer intermittently (and permanently) marks a workload unhealthy when it happens to run while that workload is starting; and a thv list racing thv rm no longer resurrects the removed workload's status file as a ghost entry.

Special notes for reviewers

  • The two fixes are independent and land as two commits; review them separately. Combining them into one PR was an explicit maintainer decision to save a review cycle.
  • handleRuntimeMismatch and isProxyUnhealthy still write unconditionally. They only fire when the container is present in the snapshot, so the reorder does not change their exposure; migrating them to the guarded write would be a reasonable follow-up.

Generated with Claude Code

@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 28, 2026
JAORMX and others added 2 commits July 28, 2026 07:47
ListWorkloads snapshotted the runtime before the status files. A
workload whose file flips to running between the two snapshots — every
thv run does this, since the detached child starts the container and
then writes running — appears as "file running, runtime missing", and
handleRuntimeMissing persists unhealthy into its status file. Nothing
recovers a non-running file status against a live container, so one
concurrent `thv list` permanently marks a healthy, freshly started
workload unhealthy. This is user-facing state corruption; it also
caused the E2E Tests Core (vmcp) failure on main (run 30335474873),
where a backend that had demonstrably started and stayed alive was
reported "not found in runtime" for two minutes.

Read the files before the runtime: a file that says running at time T
implies its container existed before T, so a later runtime miss is
genuine. The old order was introduced without stated rationale in
e6e29dc.

Guard the unhealthy write itself: re-check under the file lock and skip
when the file is gone or no longer says running/stopped. Reordering
alone would let a list that brackets a whole `thv rm` recreate the
just-deleted status file as an unhealthy ghost, because
SetWorkloadStatus creates missing files.

Give waitForIsolatedMCPServer a state dump through its isolated env on
timeout; the upgrade-spec failure on the same run was undiagnosable
because the generic timeout message carries no status context.

Refs #4432 — the same write-on-read class; this fixes the startup
trigger but thv list still writes on read, so that issue stays open.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
The AllowPort spec fetched https://example.com exactly once,
immediately after workload readiness. The allowed leg needs a working
DNS -> TLS -> upstream chain through Envoy's dynamic_forward_proxy, and
the first egress request after startup can race the isolation stack's
own warm-up, failing instantly with a local error before anything
reaches the network. Observed twice on 2026-07-28 (PR heads 1588e6a
and 5c901a9, within the same minute): IsError ~40ms and ~190ms after
readiness — far too fast for genuine external TLS trouble — while the
same spec passed on main minutes later.

The property this spec pins is steady-state reachability of an allowed
port; first-request-after-boot success is not something the DFP path
promises. Retry the allowed leg until the deadline, and surface the
fetch tool's error text on failure — the diagnostic that distinguishes
a warm-up race from a genuinely broken AllowPort, which the old
boolean-only assertion discarded. A broken AllowPort still fails, now
with evidence. The blocked port-80 leg is unchanged: denial is
enforced locally and deterministically once the stack is warm, which
the preceding allowed leg has just proven.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
@JAORMX
JAORMX force-pushed the fix-thv-list-status-poisoning branch from 5b755b6 to 89ceb64 Compare July 28, 2026 07:52
@JAORMX JAORMX changed the title Fix thv list race that poisons starting workloads Fix workload status poisoning and deflake Envoy AllowPort Jul 28, 2026
@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 28, 2026
@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 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.56522% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.25%. Comparing base (4a61bd1) to head (89ceb64).

Files with missing lines Patch % Lines
pkg/workloads/statuses/file_status.go 69.56% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6076      +/-   ##
==========================================
- Coverage   72.26%   72.25%   -0.02%     
==========================================
  Files         725      725              
  Lines       75358    75376      +18     
==========================================
+ Hits        54460    54462       +2     
- Misses      17016    17029      +13     
- Partials     3882     3885       +3     

☔ 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 merged commit d79c3a9 into main Jul 28, 2026
81 of 83 checks passed
@JAORMX
JAORMX deleted the fix-thv-list-status-poisoning branch July 28, 2026 08:47
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.

2 participants