amadan
edge: PurgeListener.Close also tears down established connections
The listener-only Close fix (1b16157) closed the unlink race but introduced a different gap: closing only the raw net.Listener stops new Accepts but does nothing to a connection already handed off to its own handler goroutine, which keeps right on serving requests over a still-open keep-alive connection. The reviewer demonstrated this concretely — a purge issued over a connection established before Close still succeeded after Close returned. Fix: close p.ln first (deterministic unlink, unaffected by whether Serve's goroutine ever registered the listener — see the existing doc comment), then p.srv.Close() to tear down every connection srv is independently tracking in its own connection map. That map isn't tied to the listener-registration race the first fix addressed, so ordering the raw close before it is free: p.ln is already gone by the time srv.Close() redundantly (and harmlessly) tries to close it again. Regression test: a keep-alive connection completes one purge, Close runs, then a second purge attempted over the SAME (transport-pooled) connection must fail — reproduced consistently pre-fix, clean after (10x local, -race x5).
2 files changed,
+80
−14
internal/edge/purge.go+32 −14internal/edge/purge_test.go+48 −0
diff --git a/internal/edge/purge.go b/internal/edge/purge.go| index 1f232f055..471876ac0 100644 |
| --- a/internal/edge/purge.go |
| +++ b/internal/edge/purge.go |
| @@ -118,20 +118,38 @@ func ListenPurge(socketPath, host string, cache *Cache) (*PurgeListener, error) |
| return p, nil |
| } |
| -// Close stops accepting purge requests and removes the socket file. |
| +// Close stops accepting purge requests, removes the socket file, and tears |
| +// down any connection already accepted before this call — in that order, |
| +// and both steps matter independently: |
| // |
| -// 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. |
| +// 1. p.ln.Close() first, deterministically unlinking the socket file and |
| +// ending Accept regardless of whether the Serve goroutine ListenPurge |
| +// started has ever been scheduled. p.srv.Close() alone can't be trusted |
| +// for this: 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 a Close |
| +// arriving before that goroutine runs loses the race and finds nothing |
| +// registered — Serve then proceeds to Accept on a listener nobody asked |
| +// it to stop, and the socket file is never unlinked. Closing the raw |
| +// net.Listener sidesteps the tracking map entirely. |
| +// 2. p.srv.Close() second, to tear down connections ALREADY accepted |
| +// before step 1 — closing only the listener stops new Accepts but does |
| +// nothing to a connection already handed off to its own handler |
| +// goroutine, which keeps right on serving requests over a still-open |
| +// keep-alive connection. A client that pooled such a connection could |
| +// otherwise keep issuing purges indefinitely after Close returned. |
| +// Calling srv.Close() after p.ln is already closed is harmless: closing |
| +// an already-closed (or never-registered) listener a second time is a |
| +// no-op error internally, swallowed here, while every connection |
| +// srv independently tracks in its own connection map — unrelated to |
| +// listener registration — still gets closed as intended. |
| +// |
| +// The returned error is p.ln's — the unlink guarantee callers actually |
| +// depend on; srv.Close()'s error, if any, is dropped. Calling Close a |
| +// second time (nobody does today) returns "use of closed network |
| +// connection" from the second p.ln.Close(). |
| func (p *PurgeListener) Close() error { |
| - return p.ln.Close() |
| + err := p.ln.Close() |
| + p.srv.Close() |
| + return err |
| } |
diff --git a/internal/edge/purge_test.go b/internal/edge/purge_test.go| index 9393d5349..f3b39c6f3 100644 |
| --- a/internal/edge/purge_test.go |
| +++ b/internal/edge/purge_test.go |
| @@ -123,6 +123,54 @@ func TestListenPurgeCloseImmediatelyAfterOpenRemovesSocketFile(t *testing.T) { |
| } |
| } |
| +// Close must tear down connections already accepted before it runs, not |
| +// just stop new ones. Closing the raw listener alone (the fix for the |
| +// unlink race above) only stops future Accept calls — a connection the |
| +// server already handed off to its own handler goroutine keeps right on |
| +// serving requests, since nothing told that goroutine to stop. A client |
| +// whose transport pooled a keep-alive connection from a purge issued before |
| +// Close can therefore keep issuing purges after Close returns, defeating |
| +// the whole point of closing the listener. |
| +func TestListenPurgeCloseRefusesRequestsOnAlreadyEstablishedConnections(t *testing.T) { |
| + dir := t.TempDir() |
| + sock := filepath.Join(dir, "instance.sock") |
| + cache := NewCache(DefaultCacheMaxEntries, DefaultCacheMaxBodyBytes) |
| + cache.Put(CacheKey{Host: "a.example", Method: "GET", Path: "/event"}, "", &CachedResponse{ |
| + StatusCode: 200, Header: http.Header{}, Body: []byte("hi"), StoredAt: time.Now(), TTL: time.Minute, |
| + }) |
| + |
| + pl, err := ListenPurge(sock, "a.example", cache) |
| + if err != nil { |
| + t.Fatalf("ListenPurge: %v", err) |
| + } |
| + |
| + client := dialPurgeSocket(t, sock) |
| + body, _ := json.Marshal(map[string]string{"path": "/event", "query": ""}) |
| + |
| + // Establish (and, via keep-alive, pool) a connection with one successful |
| + // purge before Close. |
| + resp, err := client.Post("http://unix/purge", "application/json", bytes.NewReader(body)) |
| + if err != nil { |
| + t.Fatalf("first purge POST: %v", err) |
| + } |
| + resp.Body.Close() |
| + if resp.StatusCode != http.StatusOK { |
| + t.Fatalf("first purge status = %d, want 200", resp.StatusCode) |
| + } |
| + |
| + if err := pl.Close(); err != nil { |
| + t.Fatalf("Close: %v", err) |
| + } |
| + |
| + // The SAME client — free to reuse its pooled connection from the first |
| + // request — must not be able to complete a second purge after Close. |
| + resp2, err := client.Post("http://unix/purge", "application/json", bytes.NewReader(body)) |
| + if err == nil { |
| + resp2.Body.Close() |
| + t.Fatal("expected a purge over a connection established before Close to fail once Close has run, got success") |
| + } |
| +} |
| + |
| func TestListenPurgeRemovesStaleSocketFile(t *testing.T) { |
| dir := t.TempDir() |
| sock := filepath.Join(dir, "instance.sock") |