Skip to content

Recreate the shared external network on a lost teardown race - #6083

Merged
JAORMX merged 1 commit into
stacklok:mainfrom
jhrozek:fix-toolhive-external-network-race
Jul 28, 2026
Merged

Recreate the shared external network on a lost teardown race#6083
JAORMX merged 1 commit into
stacklok:mainfrom
jhrozek:fix-toolhive-external-network-race

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Why: pkg/container/docker/client.go's deleteNetworks removes the
    shared toolhive-external Docker network opportunistically whenever a
    workload teardown observes zero remaining toolhive=true-labeled
    containers — a point-in-time check with no cross-process synchronization.
    Since concurrent Ginkgo processes (or any concurrent thv run invocations
    on one host) share a single Docker daemon, one process's teardown can
    delete the network while another process's container is mid-creation and
    still needs it, surfacing as network toolhive-external not found from
    ContainerCreate/ContainerStart. Hit live in CI on PR Retry dual-era backend startup on a lost port-bind race #6080: a DNS
    sidecar container failed this way in the E2E Tests Core (core) job after
    it moved to 4 concurrent Ginkgo processes (Speed up e2e(core) suite with Ginkgo procs and concurrent waits #6070) sharing one daemon.
  • What: createContainer now retries once, recreating
    toolhive-external first, if container create/start fails with a
    not-found error and the container's endpoint config references that
    network. createAndStartContainer (extracted from the existing
    create+start logic, unchanged) also force-removes a container Docker
    leaves behind in "created" state after a failed start, so the retry
    doesn't collide with it on the same name.

Type of change

  • Bug fix

Test plan

  • Manual testing (describe below)

go build ./... and go vet ./pkg/container/docker/... both pass cleanly.
Not yet covered by a unit or e2e test — this was root-caused and fixed from
a single CI occurrence rather than a local stress reproduction. Suggested
verification: run the "core" e2e suite with concurrent procs against a
workload-churn script that creates/removes workloads on the same daemon
while another spec is mid-creation, to force the teardown-vs-creation race
directly (e.g. ginkgo --label-filter='core && groups' --repeat=20 -v ./test/e2e/ -- --procs=4).

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.

Does this introduce a user-facing change?

No. Internal container-runtime resilience fix; no API or CLI surface change.

Special notes for reviewers

This closes the loop on the opposite side of the same shared-network
lifecycle that #6071 already made resilient to concurrent creation races
(treating NetworkCreate conflict as success). This PR handles the
teardown side: a concurrent deletion racing another workload's creation.

No local stress reproduction was performed for this one (unlike the
port-bind-race fix in #6080) — the root cause and fix were derived from
tracing the actual CI failure and the relevant code paths. Flagging this
explicitly since it means the fix is confidence-based on code inspection
rather than confirmed-live-under-stress; happy to attempt a local repro if
useful before merge.

Generated with Claude Code

deleteNetworks removes the shared "toolhive-external" Docker network
opportunistically whenever a workload teardown observes zero remaining
toolhive-labeled containers -- a point-in-time check with no
cross-process synchronization. Since concurrent Ginkgo processes (or
any concurrent `thv run` invocations) share one Docker daemon, one
process's teardown can delete the network while another's container
is mid-creation and still needs it, surfacing as "network
toolhive-external not found" from ContainerCreate/ContainerStart.

Observed in CI: a DNS sidecar container failed this way in the "core"
e2e job after it moved to 4 concurrent Ginkgo processes.

Recreate the network and retry once when this specific not-found
condition is hit, and clean up any container Docker leaves behind in
"created" state after a failed start so the retry doesn't collide on
the same name.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.30%. Comparing base (53908ef) to head (6a2129c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/container/docker/client.go 33.33% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6083      +/-   ##
==========================================
- Coverage   72.30%   72.30%   -0.01%     
==========================================
  Files         728      728              
  Lines       75496    75505       +9     
==========================================
+ Hits        54590    54591       +1     
- Misses      16987    16991       +4     
- Partials     3919     3923       +4     

☔ 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 e8f84ef into stacklok:main Jul 28, 2026
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants