amadan
edge: an unchanged cert poll retries a failed local copy from the snapshot it holds (#336)
Final review finding 7. syncLocal reports whether any entry failed to reconcile into the local cert cache; publish records it, and an armed poll that finds control/certs unmoved retries syncLocal against the existing snapshot, with no bucket read. A settled cache costs no local write.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+67
−4
internal/edge/certsync.go+15 −4internal/edge/certsync_test.go+52 −0
diff --git a/internal/edge/certsync.go b/internal/edge/certsync.go| index 106085435..ba54d8564 100644 |
| --- a/internal/edge/certsync.go |
| +++ b/internal/edge/certsync.go |
| @@ -545,6 +545,9 @@ type certMirror struct { |
| fullAge time.Duration |
| actedGen uint64 |
| actedOK bool |
| + // localPending: the last copy into the local cache left entries it |
| + // could not write; an unchanged poll retries it from the snapshot. |
| + localPending bool |
| // synced remembers, per name, the mirrored bytes last reconciled into the |
| // local cache, so a steady state costs no local I/O at all. |
| @@ -754,6 +757,12 @@ func (m *certMirror) poll(ctx context.Context) error { |
| case perr != nil, !ok, m.lastFull.IsZero(), !m.clock().Before(m.lastFull.Add(m.fullAge)): |
| return m.refreshFull(ctx) |
| case m.actedOK && gen == m.actedGen: |
| + // Nothing moved in the bucket, but a copy into the local cache |
| + // that failed is retried from the snapshot already held: no |
| + // bucket read (final review R1, finding 7). |
| + if m.localPending { |
| + m.localPending = m.syncLocal(ctx, m.snapshot()) |
| + } |
| return nil |
| } |
| return m.refreshChanged(ctx, gen) |
| @@ -920,7 +929,7 @@ func (m *certMirror) publish(ctx context.Context, prev *certSnapshot, next map[s |
| m.snap = snap |
| m.mu.Unlock() |
| m.etags = etags |
| - m.syncLocal(ctx, snap) |
| + m.localPending = m.syncLocal(ctx, snap) |
| // The local cache the handshake reads now holds what the bucket does: |
| // the certificate condition is met for good (§6). A later failed |
| // refresh keeps serving this copy and leaves the latch set. |
| @@ -1417,10 +1426,11 @@ func plural(n int, one, many string) string { |
| // litter nothing sweeps. |
| // |
| // Writes are best-effort and loud: a full disk must not stop the mirror from |
| -// SERVING what it already holds in memory. |
| -func (m *certMirror) syncLocal(ctx context.Context, snap *certSnapshot) { |
| +// SERVING what it already holds in memory. It reports whether any entry could |
| +// not be reconciled, so the armed poll can retry an unchanged snapshot. |
| +func (m *certMirror) syncLocal(ctx context.Context, snap *certSnapshot) (failedAny bool) { |
| if m.local == nil { |
| - return |
| + return false |
| } |
| m.syncMu.Lock() |
| defer m.syncMu.Unlock() |
| @@ -1460,6 +1470,7 @@ func (m *certMirror) syncLocal(ctx context.Context, snap *certSnapshot) { |
| delete(m.synced, name) |
| } |
| } |
| + return failed > 0 |
| } |
| // errUnparseableEntry marks the one reconcile failure that is the ENTRY's |
diff --git a/internal/edge/certsync_test.go b/internal/edge/certsync_test.go| index fa3651297..1c7c258b6 100644 |
| --- a/internal/edge/certsync_test.go |
| +++ b/internal/edge/certsync_test.go |
| @@ -2078,6 +2078,58 @@ func mirrorPoll(t *testing.T, m *certMirror) { |
| } |
| } |
| +// failingPutCache refuses every Put while fail is set, and counts the Puts |
| +// it lets through. |
| +type failingPutCache struct { |
| + autocert.Cache |
| + fail atomic.Bool |
| + puts atomic.Int32 |
| +} |
| + |
| +func (c *failingPutCache) Put(ctx context.Context, key string, data []byte) error { |
| + if c.fail.Load() { |
| + return errors.New("disk full") |
| + } |
| + c.puts.Add(1) |
| + return c.Cache.Put(ctx, key, data) |
| +} |
| + |
| +// Final review R1 finding 7 (amadan #336): a copy into the local cert cache |
| +// that failed is retried on the next poll even when the index says nothing |
| +// moved, from the snapshot the mirror already holds, with no bucket read. |
| +// Once it lands, an unchanged poll writes nothing. |
| +func TestCertMirrorRetriesAFailedLocalCopyOnAnUnchangedPoll(t *testing.T) { |
| + t.Parallel() |
| + s := newChangeIndexStore(t) |
| + pem := hostCertPEM(t, "shop.example.com", time.Now().Add(80*24*time.Hour)) |
| + putBucket(t, s, "shop.example.com", pem) |
| + m, clk := armedMirror(t, s) |
| + local := &failingPutCache{Cache: autocert.DirCache(t.TempDir())} |
| + m.local = local |
| + local.fail.Store(true) |
| + mirrorPoll(t, m) |
| + if _, err := local.Get(context.Background(), "shop.example.com"); err == nil { |
| + t.Fatal("fixture: the local copy landed while Puts fail") |
| + } |
| + |
| + local.fail.Store(false) |
| + s.reset() |
| + clk.advance(time.Second) |
| + mirrorPoll(t, m) |
| + if body, err := local.Get(context.Background(), "shop.example.com"); err != nil || !bytes.Equal(body, pem) { |
| + t.Fatalf("an unchanged poll after a failed local copy left the local cache without it: %v", err) |
| + } |
| + if list, listETags, get := certReads(s); list+listETags+get != 0 { |
| + t.Fatalf("the retry read the bucket: list %d listETags %d get %d", list, listETags, get) |
| + } |
| + n := local.puts.Load() |
| + clk.advance(time.Second) |
| + mirrorPoll(t, m) |
| + if got := local.puts.Load(); got != n { |
| + t.Fatalf("a settled unchanged poll wrote the local cache again (%d puts, want %d)", got, n) |
| + } |
| +} |
| + |
| func TestCertMirrorSkipsWhenUnmoved(t *testing.T) { |
| t.Parallel() |
| s := newChangeIndexStore(t) |