stacklok / stacklok/toolhive

Removing a workload can delete the shared toolhive-external network out from under a concurrent create

Open
#6,078 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.