Removing a workload can delete the shared toolhive-external network out from under a concurrent create
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 2.2k
- Forks
- 300
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 184
Description
Summary
deleteNetworks (pkg/container/docker/client.go:1807-1831) runs on every workload removal. It lists all toolhive=true containers and, if that list is empty, deletes the shared toolhive-external docker network.
The zero-container check is not atomic with anything. A concurrent thv run or thv restart that has already passed its own network-exists check — createNetwork treats "already exists" as success (client.go:1180-1206) — but has not yet created its containers is invisible to that list. So the removal deletes the network out from under the in-flight create, which then fails:
Error response from daemon: failed to set up container networking:
network toolhive-external not found
Observed
main, run 30336616955 on commit 2e05f919, job E2E Tests Core (core):
THVIgnore E2E Tests … should apply global ignore patterns from custom config file (test/e2e/thvignore_test.go:283) — the workload restart's dns-container create failed with exactly that error, while a parallel spec's cleanup removed its last workload.
This presents as an e2e flake, but it is a product race: any two concurrent thv invocations where one removes its last workload while the other is starting one can hit it. CI just makes the interleaving likely.
Note the concurrent-create half of this race was already found and handled — that is why createNetwork tolerates "already exists". The delete side is the remaining half.
Options
(a) Stop deleting toolhive-external on last-container removal. Recommended. The network is recreated on demand, so deleting it buys nothing, and any counting scheme over a shared docker daemon is inherently racy. An explicit thv system cleanup-style path can still remove it deliberately, where the user has opted into it and no create is in flight.
(b) Tolerate-and-retry. On "network not found" during container create, re-create the network and retry once. Fixes the symptom at every attach point — but there are several attach points, so it is more surface to get right.
(c) Reference counting / ownership. Heaviest, and still racy without a daemon-side transaction.
Notes
Diagnosed while investigating failing E2E Tests Core shards after #6050/#6051/#6061 merged. Those turned out to be flakes rather than a regression; this is one of three distinct mechanisms found. The others are the thv list status-poisoning race (#6076, refs #4432) and an Envoy AllowPort DNS warm-up flake (#6077).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with deleteNetworks in pkg/container/docker/client.go:1807-1831, then compare its container check with createNetwork in client.go:1180-1206. Reproduce or inspect the concurrent thv run/restart scenario and test the affected test/e2e/thvignore_test.go:283 path; done means concurrent removal and creation no longer produce a missing toolhive-external network.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- docker, go
- Domain
- cli, infrastructure
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100