amadan
cert sweep: a capped-drain pass does not sweep
A first-time backlog trips warmOrdersPerPass, and pass() then comes back every minute until it clears. Housekeeping was riding along: a listing of the whole cert prefix, sixty times an hour, to enforce a one-hour token TTL and a daily expiry pass. Floor the whole sweep at the warm's own quiet-path cadence, so an ordinary pass sweeps and a drain pass does not. The two pacing fields are deliberately not mutex-guarded, unlike everything else on the struct: they are touched only from sweep(), reached only from pass(), which only ever runs on run()'s single goroutine. A mutex there would suggest a concurrency that does not exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 files changed,
+57
−5
internal/edge/certsweep.go+16 −2internal/edge/certsweep_test.go+35 −0internal/edge/certwarm.go+6 −3
CI passed — run details
diff --git a/internal/edge/certsweep.go b/internal/edge/certsweep.go| index 858af2883..c4a710823 100644 |
| --- a/internal/edge/certsweep.go |
| +++ b/internal/edge/certsweep.go |
| @@ -65,9 +65,14 @@ const ( |
| // cost of keeping a dead certificate is bytes, and the cost of removing |
| // a live one is an outage. |
| certSweepGrace = 30 * 24 * time.Hour |
| - // certSweepInterval is how often the expiry half runs. The token half |
| - // runs every pass, because it needs no reads at all. |
| + // certSweepInterval is how often the expiry half runs — the half that |
| + // costs reads. A certificate dead for a month can wait a day. |
| certSweepInterval = 24 * time.Hour |
| + // certSweepMinInterval is the floor for the whole sweep, token half |
| + // included. It matches the warm's own quiet-path cadence, so an ordinary |
| + // pass sweeps and a capped-drain pass (which comes back every minute |
| + // until a backlog clears) does not. |
| + certSweepMinInterval = warmInterval |
| // certSweepDeletesPerPass bounds the damage any single pass can do, in |
| // either direction: a bug here deletes at most this many objects before |
| // somebody sees the log, and a genuine backlog drains over days rather |
| @@ -85,6 +90,14 @@ func (c *certWarm) sweep(ctx context.Context, now time.Time) { |
| if c.store == nil { |
| return |
| } |
| + // Not on a capped-drain pass. When a first-time backlog trips |
| + // warmOrdersPerPass, pass() comes back every minute until it clears, and |
| + // housekeeping does not need doing sixty times an hour — the token TTL |
| + // is an hour and the expiry half is daily. Without this the drain would |
| + // spend a listing of the whole cert prefix per minute for nothing. |
| + if !c.lastSweep.IsZero() && now.Sub(c.lastSweep) < certSweepMinInterval { |
| + return |
| + } |
| lister, ok := c.store.(release.ModLister) |
| if !ok { |
| // No stamps, no age, no sweep. Guessing at an object's age from its |
| @@ -98,6 +111,7 @@ func (c *certWarm) sweep(ctx context.Context, now time.Time) { |
| log.Printf("edge: cert sweep: could not list %s: %v", certPrefix, err) |
| return |
| } |
| + c.lastSweep = now |
| expiryDue := now.Sub(c.lastExpirySweep) >= certSweepInterval |
| deletes := 0 |
| for _, e := range entries { |
diff --git a/internal/edge/certsweep_test.go b/internal/edge/certsweep_test.go| index 217487d94..406cff142 100644 |
| --- a/internal/edge/certsweep_test.go |
| +++ b/internal/edge/certsweep_test.go |
| @@ -284,3 +284,38 @@ func TestSweepNeverRunsOnANonIssuer(t *testing.T) { |
| t.Error("a stood-down box deleted cert material") |
| } |
| } |
| + |
| +// A first-time backlog trips warmOrdersPerPass and pass() comes back every |
| +// minute until it clears. Housekeeping must not ride along sixty times an |
| +// hour: the token TTL is an hour and the expiry half is daily, so a listing |
| +// of the whole cert prefix per minute would be spent on nothing. |
| +func TestSweepDoesNotRunOnEveryCappedDrainPass(t *testing.T) { |
| + dir := t.TempDir() |
| + s := &listCounter{Store: release.NewDirStore(dir)} |
| + w := sweepWarm(t, s) |
| + now := time.Now() |
| + |
| + w.sweep(context.Background(), now) |
| + if s.lists != 1 { |
| + t.Fatalf("first sweep listed %d time(s), want 1", s.lists) |
| + } |
| + w.sweep(context.Background(), now.Add(time.Minute)) |
| + w.sweep(context.Background(), now.Add(2*time.Minute)) |
| + if s.lists != 1 { |
| + t.Errorf("sweep listed %d times across three passes a minute apart, want 1", s.lists) |
| + } |
| + w.sweep(context.Background(), now.Add(certSweepMinInterval+time.Second)) |
| + if s.lists != 2 { |
| + t.Errorf("sweep listed %d times, want 2 — an ordinary pass an hour later must sweep", s.lists) |
| + } |
| +} |
| + |
| +type listCounter struct { |
| + release.Store |
| + lists int |
| +} |
| + |
| +func (l *listCounter) ListMod(ctx context.Context, prefix string) ([]release.KeyMod, error) { |
| + l.lists++ |
| + return l.Store.(release.ModLister).ListMod(ctx, prefix) |
| +} |
diff --git a/internal/edge/certwarm.go b/internal/edge/certwarm.go| index 28b4d6c34..3e4700a74 100644 |
| --- a/internal/edge/certwarm.go |
| +++ b/internal/edge/certwarm.go |
| @@ -115,9 +115,12 @@ type certWarm struct { |
| warmed map[string]bool |
| lastStandDownLog time.Time |
| - // lastExpirySweep is when the expiry half of the cert sweep last ran |
| - // (certsweep.go). Guarded by mu like everything else here, though the |
| - // sweep is only ever reached from pass(). |
| + // lastSweep and lastExpirySweep pace the cert sweep (certsweep.go). |
| + // Deliberately NOT guarded by mu, unlike everything else on this struct: |
| + // they are touched only from sweep(), which is only ever reached from |
| + // pass(), which only ever runs on run()'s single goroutine. A mutex here |
| + // would suggest a concurrency that does not exist. |
| + lastSweep time.Time |
| lastExpirySweep time.Time |
| } |