amadan
edge: PurgeListener.Close no longer races the Serve goroutine
Close delegated to http.Server.Close, which only closes listeners its own Serve call has already registered — inside a map net/http populates at the TOP of Serve, on entry. ListenPurge starts Serve in a goroutine it never waits on, so a Close arriving before that goroutine gets scheduled found nothing registered and silently no-op'd: the listener was never closed, the socket file never unlinked, and the abandoned goroutine went on to Accept on a socket nobody asked it to stop. Found while wiring UnitBackend's purge listener (next commit): its Start/Stop cycle opens and closes a purge listener back-to-back around one systemctl call, with none of the hundreds of milliseconds of health- wait slack that happened to keep ExecBackend's equivalent test off this race. Closing p.ln directly sidesteps the tracking map entirely — a raw unix listener always unlinks its socket file on Close, regardless of whether the Serve goroutine ever got scheduled. Regression test: Close called immediately after ListenPurge returns, with no synchronization giving the Serve goroutine a chance to run first — reproduced 5/5 before the fix, clean after.
2 files changed,
+47
−2
internal/edge/purge.go+19 −2internal/edge/purge_test.go+28 −0
diff --git a/internal/edge/purge.go b/internal/edge/purge.go| index 4817763e7..8de60d8dc 100644 |
| --- a/internal/edge/purge.go |
| +++ b/internal/edge/purge.go |
| @@ -101,10 +101,27 @@ func ListenPurge(socketPath, host string, cache *Cache) (*PurgeListener, error) |
| srv := &http.Server{Handler: mux} |
| p := &PurgeListener{ln: ln, srv: srv} |
| - go srv.Serve(ln) //nolint:errcheck // Close() below triggers the expected ErrServerClosed |
| + // Close (below) closes p.ln directly, not through srv — so the error |
| + // Serve returns here is whatever a closed-listener Accept produces, not |
| + // necessarily http.ErrServerClosed. Either way it's expected and unused. |
| + go srv.Serve(ln) //nolint:errcheck |
| return p, nil |
| } |
| +// Close stops accepting purge requests and removes the socket file. |
| +// |
| +// It closes p.ln directly rather than p.srv.Close(): net/http's Server only |
| +// closes listeners its own Serve call has already registered (in an |
| +// internal map populated by Serve itself, on entry — see trackListener in |
| +// net/http/server.go), and ListenPurge starts that Serve in a goroutine it |
| +// does not wait on. A Close arriving before that goroutine has been |
| +// scheduled loses the race and finds nothing registered to close: Serve |
| +// then proceeds to Accept on a listener nobody asked it to stop, and the |
| +// socket file it owns is never unlinked. Closing the raw net.Listener sidesteps |
| +// the tracking map entirely — it always unlinks (Go's unix listener does |
| +// this on Close for any listener it created via Listen) and makes the |
| +// goroutine's next Accept fail, which ends Serve regardless of whether it |
| +// had reached trackListener yet. |
| func (p *PurgeListener) Close() error { |
| - return p.srv.Close() |
| + return p.ln.Close() |
| } |
diff --git a/internal/edge/purge_test.go b/internal/edge/purge_test.go| index 6d126fa2f..9393d5349 100644 |
| --- a/internal/edge/purge_test.go |
| +++ b/internal/edge/purge_test.go |
| @@ -95,6 +95,34 @@ func TestListenPurgeSocketFileIsOwnerOnly(t *testing.T) { |
| } |
| } |
| +// Close, called immediately after ListenPurge returns (before the Serve |
| +// goroutine it started has necessarily run at all), must still unlink the |
| +// socket file. PurgeListener.Close used to delegate to http.Server.Close, |
| +// which only closes listeners its own Serve call has already registered in |
| +// its internal tracking map (net/http's trackListener, set inside Serve |
| +// itself) — a real race, since Serve runs in a goroutine ListenPurge doesn't |
| +// wait on. Losing that race silently no-ops Close: the goroutine's later |
| +// Serve(ln) call then Accepts on a listener nobody ever asked to stop, and |
| +// the socket file is never removed. A caller like UnitBackend.Start/Stop |
| +// that opens and closes a purge listener back-to-back around a single unit |
| +// start hits this every time, not just occasionally. |
| +func TestListenPurgeCloseImmediatelyAfterOpenRemovesSocketFile(t *testing.T) { |
| + dir := t.TempDir() |
| + sock := filepath.Join(dir, "instance.sock") |
| + pl, err := ListenPurge(sock, "a.example", NewCache(10, 1<<20)) |
| + if err != nil { |
| + t.Fatalf("ListenPurge: %v", err) |
| + } |
| + |
| + if err := pl.Close(); err != nil { |
| + t.Fatalf("Close: %v", err) |
| + } |
| + |
| + if _, err := os.Stat(sock); err == nil { |
| + t.Fatal("expected the socket file removed immediately after Close, even called before the Serve goroutine could have run") |
| + } |
| +} |
| + |
| func TestListenPurgeRemovesStaleSocketFile(t *testing.T) { |
| dir := t.TempDir() |
| sock := filepath.Join(dir, "instance.sock") |