Stop deriving the ingress proxy port from a fixed upstream port - #6069
Merged
Conversation
Every workload of the same MCP server image shares the same fixed UpstreamPort (the container's internal listen port), so deriving the Squid/Envoy ingress proxy's host port as UpstreamPort+1 made every concurrently-starting workload of that image request the identical port. Whichever container's Docker networking setup lost the race failed outright with "port is already allocated", surfacing as e2e workloads stuck in "starting" or flipped to "error" when several workloads of the same image start back-to-back. Pick the ingress port at random instead, matching how the per-workload host proxy port is already chosen elsewhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt and
rdimitrov
as code owners
July 27, 2026 22:06
11 tasks
ChrisJBurns
approved these changes
Jul 27, 2026
ChrisJBurns
left a comment
ChrisJBurns
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for this, I feel guilty having done the envoy tests!!!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6069 +/- ##
==========================================
- Coverage 72.20% 72.19% -0.01%
==========================================
Files 721 721
Lines 75062 75062
==========================================
- Hits 54198 54192 -6
- Misses 17003 17012 +9
+ Partials 3861 3858 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
size/XS
Collaborator
|
/retest |
Collaborator
|
Merging because it fixes most of the issues, there's a small number remaining that I can fix in a separate PR. |
ChrisJBurns
added a commit
that referenced
this pull request
Jul 28, 2026
After the ingress port collision (#6069) is fixed, concurrent isolated startup still leaves a workload stuck in status "starting" behind a persistent ingress 502, even though the MCP server is healthy and listening. The ingress Squid cache_peer targets the MCP container by name; under concurrent startup the container's Docker DNS record can lag, Squid caches the failed lookup, and the peer never revives within the 2-minute readiness window. Ordering ingress creation after the MCP container (the existing mitigation) is not enough when DNS registration itself races. Resolve the MCP container's IP on the internal network and give it to the ingress proxy as the upstream address, so the cache_peer has no DNS lookup to latch on. A bad IP just yields connection-refused, which Squid retries and recovers from within seconds. The Envoy backend's ingress cluster is pointed at the same IP for parity. UpstreamHost falls back to the workload name when unset, so non-isolated paths are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 tasks
ChrisJBurns
added a commit
that referenced
this pull request
Jul 28, 2026
…atch) (#6071) * Make shared network creation idempotent under races Concurrently starting workloads share the "toolhive-external" Docker network, and createNetwork checks for existence and then creates in two non-atomic steps. When several `thv run` supervisors start at once (e.g. multiple workloads in a group), more than one passes the existence check and races on NetworkCreate. Docker rejects the duplicate name with a conflict error, which propagated up as "failed to create external networks" and left every losing workload in status "error". Treat a conflict from NetworkCreate as success: a lost race means the network already exists, which is exactly the desired state. The new real-daemon test fires eight concurrent createNetwork calls for the same name and asserts every caller succeeds with a single network created; it fails against the old code and passes with the fix (skips without Docker). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Point ingress proxy at MCP container IP, not name After the ingress port collision (#6069) is fixed, concurrent isolated startup still leaves a workload stuck in status "starting" behind a persistent ingress 502, even though the MCP server is healthy and listening. The ingress Squid cache_peer targets the MCP container by name; under concurrent startup the container's Docker DNS record can lag, Squid caches the failed lookup, and the peer never revives within the 2-minute readiness window. Ordering ingress creation after the MCP container (the existing mitigation) is not enough when DNS registration itself races. Resolve the MCP container's IP on the internal network and give it to the ingress proxy as the upstream address, so the cache_peer has no DNS lookup to latch on. A bad IP just yields connection-refused, which Squid retries and recovers from within seconds. The Envoy backend's ingress cluster is pointed at the same IP for parity. UpstreamHost falls back to the workload name when unset, so non-isolated paths are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
group_rm_test.go/rm_group_test.goe2e specs that create severalworkloads of the same MCP server image back-to-back have been failing on
almost every PR: workloads observed stuck in
startingor flipped to a bareerrorstatus, timing out after 2 minutes even though the underlyingcontainer started and logged readiness within seconds. Related to Flaky: group rm "should delete group with workloads" times out waiting for readiness #6040
(closed against an earlier, incomplete diagnosis) and the diagnostics added
in Deflake CI: TUF warn mode, readiness diagnostics, test races #6063, whose new proxy-log dump captured the actual Docker error:
Bind for 127.0.0.1:8081 failed: port is already allocated.setupIngressContainer(Squid backend) andenvoyProxy.SetupIngress(Envoy backend) both picked the ingress reverse-proxy's host port as
UpstreamPort + 1, whereUpstreamPortis the container's fixed internalMCP listen port (e.g. 8080 for the
fetch/gofetch test image) — identicalacross every workload instance of that image. Every concurrently-starting
workload of the same image therefore requested the same preferred port.
networking.FindOrUsePort's availability check is a bind-then-close probewith no reservation, so it can't see a sibling process's in-flight (not yet
bound) intent to use that port — all instances see it as free, and whichever
one's Docker networking setup runs last loses with "port is already allocated".
FindOrUsePort(0)) instead of derivingit from the container's fixed internal port, matching how the per-workload
host proxy port is already chosen elsewhere in the codebase. This doesn't
eliminate the underlying TOCTOU in
pkg/networking/port.goin principle, butremoves the deterministic same-image collision, making an actual collision
astronomically unlikely for a handful of concurrently-starting workloads.
Type of change
Test plan
task test)task test-e2e)task lint-fix)Added
TestEnvoyProxy_SetupIngress_ConcurrentSameUpstreamPort, which runs 4goroutines through
SetupIngresssharing the sameUpstreamPortand assertsevery returned ingress port is distinct and none equals
upstreamPort+1. Failsagainst the old code, passes with the fix. Full
task build/task testgreen.
API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
pkg/container/docker/client.gosetupIngressContainerpicks the Squid ingress port at random instead ofupstreamPort + 1pkg/container/docker/envoy.goenvoyProxy.SetupIngresspicks the ingress port at random instead ofspec.UpstreamPort + 1pkg/container/docker/envoy_test.goDoes this introduce a user-facing change?
No. The ingress proxy port is an internal container-to-container detail (clients connect to the workload's regular host proxy port, unaffected by this change).
Special notes for reviewers
This is not a fixup of #6063 — it doesn't touch any file #6063 changes, and #6063 is diagnostics-only by design (its own description states the readiness-stall root cause was still unknown). This PR is the root-cause fix that #6063's new proxy-log diagnostic made possible to find. The two can merge in either order; no dependency between them.
Generated with Claude Code