amadan
host+edge: final review fix wave — close stop's dead-ctx window, serialize Refresh, soften the issuer-gate error body
Three small, surgical fixes from the whole-branch final review: - housekeeper.go: stop() now calls hk.endHolding() BEFORE cancel(), not after. cancel() only reaches the holding's context by propagation (it's a child of the outer ctx via beginHolding), so there was a window between cancel() firing and the run loop noticing and calling endHolding() itself where hk.cur was still non-nil over an already-cancelled context — held() read true against dead work. Closing the gate first removes the window synchronously. The run path's own endHolding() call inside hold() is unchanged and stays idempotent. - certsync.go: certMirror gained refreshMu, locked for Refresh's whole body. Production has one caller today (the 30s loop), so this is insurance, not a live fix — two overlapping polls could otherwise both read the same prev snapshot, build independently, and publish out of order under m.mu.Lock(), with the slower one clobbering a fresher result regardless of which list was actually more recent. - certsync.go: errNotIssuer's message changed from "acme: this box is not the deployment's certificate issuer" to the neutral "acme: not available here". autocert's own HTTPHandler writes err.Error() verbatim as an unauthenticated :80 response body (http.Error at autocert.go's HTTPHandler) — our code never gets a chance to reword it — so the old text disclosed multi-edge topology to anonymous probers. Every internal check uses errors.Is(err, errNotIssuer), never the string, so this costs nothing internal. Gate: go build/vet/test ./... (all 27 packages), GOOS=darwin go build ./..., gofmt -l clean, go test -race ./internal/edge/... and ./internal/host/... — all green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+35
−1
internal/edge/certsync.go+24 −1internal/host/housekeeper.go+11 −0
diff --git a/internal/edge/certsync.go b/internal/edge/certsync.go| index a802607e9..a1c0c23d7 100644 |
| --- a/internal/edge/certsync.go |
| +++ b/internal/edge/certsync.go |
| @@ -87,7 +87,17 @@ func (e *Edge) issuerGate() (context.Context, bool) { |
| // deployment's certificate issuer. Sentinel (errors.Is-able) so the gate is |
| // distinguishable from "that host isn't mine", which is a different fault |
| // with a different fix. |
| -var errNotIssuer = errors.New("acme: this box is not the deployment's certificate issuer") |
| +// |
| +// The message text is deliberately generic, not diagnostic: autocert's own |
| +// HTTPHandler writes this exact string, verbatim, as the body of an |
| +// unauthenticated :80 response (http.Error(w, err.Error(), 403) at |
| +// autocert.go's HTTPHandler) — OUR code never gets a chance to intercept or |
| +// reword it before it reaches the wire. "acme: this box is not the |
| +// deployment's certificate issuer" told any anonymous prober both that |
| +// multi-edge issuance exists and which box is not the issuer; the identity |
| +// that matters everywhere else in this file is errors.Is(err, errNotIssuer), |
| +// never the string, so softening it costs nothing internal. |
| +var errNotIssuer = errors.New("acme: not available here") |
| // armCertIssuance wires cert DISTRIBUTION and the issuance GATE together, |
| // because they are one decision and not two: a box may only be told "you are |
| @@ -424,6 +434,17 @@ type certMirror struct { |
| // issuance to arm. |
| gated func() bool |
| + // refreshMu serializes Refresh's whole body. Production has exactly one |
| + // caller today (the 30s loop, run()), so this is insurance rather than a |
| + // live fix: two overlapping polls (a second caller added later, or a |
| + // boot-time refresh racing the loop's first tick) could otherwise both |
| + // read m.snapshot() as prev, build independently, and publish out of |
| + // order — the SECOND Store to finish under m.mu.Lock() wins regardless |
| + // of which list was actually more recent, so a slow poll could clobber a |
| + // fresher one. Locking the body makes Refresh calls queue and apply in |
| + // the order they were made. |
| + refreshMu sync.Mutex |
| + |
| mu sync.RWMutex |
| snap *certSnapshot |
| // partialPolls counts polls that could not read every object, for the |
| @@ -494,6 +515,8 @@ func newCertMirror(s release.Store, local autocert.Cache) *certMirror { |
| // matters more here than usual: an empty map on a bucket blip would be a TLS |
| // outage on every non-issuer in the deployment. |
| func (m *certMirror) Refresh(ctx context.Context) error { |
| + m.refreshMu.Lock() |
| + defer m.refreshMu.Unlock() |
| keys, err := m.s.List(ctx, certPrefix) |
| if err != nil { |
| return fmt.Errorf("list %s: %w", certPrefix, err) |
diff --git a/internal/host/housekeeper.go b/internal/host/housekeeper.go| index aea344a68..ab7386fa0 100644 |
| --- a/internal/host/housekeeper.go |
| +++ b/internal/host/housekeeper.go |
| @@ -252,6 +252,17 @@ func (hk *housekeeper) start(parent context.Context) (stop func()) { |
| hk.run(ctx) |
| }() |
| return func() { |
| + // Close the gate BEFORE cancelling: cancel() only reaches the |
| + // holding's context by propagation (it is a child of this ctx via |
| + // beginHolding), so between cancel() and the run loop noticing and |
| + // calling endHolding() itself there is a window where hk.cur is |
| + // still non-nil over an already-cancelled context — held() reads |
| + // true against dead work. Calling endHolding() first closes hk.cur |
| + // synchronously, before ctx is even touched, so that window never |
| + // opens. The run path's own endHolding() call (in hold()) stays and |
| + // is idempotent — this is belt and braces for a path that doesn't |
| + // go through hold() (housekeeperSole) or that races it. |
| + hk.endHolding() |
| cancel() |
| select { |
| case <-done: |