Skip to content

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

Description

@JAORMX

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).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageIssue needs initial triage by a maintainer

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions