48b6401 Tighten public TLS renewal cleanup · gordon

M internal/adapters/out/acmestore/store.go +2 -5


@@ -312,7 +312,7 @@ func (s *Store) Lock(ctx context.Context) (func() error, error) {
312	312		}
313	313
314	314		return func() error {
315		-	return releaseLockFile(path, f, fd)
	315	+	return releaseLockFile(f, fd)
316	316		}, nil
317	317		}
318	318

@@ -330,7 +330,7 @@ func writeLockMetadata(f *os.File) error {
330	330		return nil
331	331		}
332	332
333		-	func releaseLockFile(path string, f *os.File, fd int) error {
	333	+	func releaseLockFile(f *os.File, fd int) error {
334	334		var errs []error
335	335		if err := syscall.Flock(fd, syscall.LOCK_UN); err != nil {
336	336		errs = append(errs, fmt.Errorf("unlock: %w", err))

@@ -338,9 +338,6 @@ func releaseLockFile(path string, f *os.File, fd int) error {
338	338		if err := f.Close(); err != nil {
339	339		errs = append(errs, fmt.Errorf("close: %w", err))
340	340		}
341		-	if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
342		-	errs = append(errs, fmt.Errorf("remove: %w", err))
343		-	}
344	341		if err := errors.Join(errs...); err != nil {
345	342		return fmt.Errorf("acmestore: release lock: %w", err)
346	343		}

 	}
 	return func() error {
-		return releaseLockFile(path, f, fd)
+		return releaseLockFile(f, fd)
 	}, nil
 }
 	return nil
 }
-func releaseLockFile(path string, f *os.File, fd int) error {
+func releaseLockFile(f *os.File, fd int) error {
 	var errs []error
 	if err := syscall.Flock(fd, syscall.LOCK_UN); err != nil {
 		errs = append(errs, fmt.Errorf("unlock: %w", err))
 	if err := f.Close(); err != nil {
 		errs = append(errs, fmt.Errorf("close: %w", err))
 	}
-	if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
-		errs = append(errs, fmt.Errorf("remove: %w", err))
-	}
 	if err := errors.Join(errs...); err != nil {
 		return fmt.Errorf("acmestore: release lock: %w", err)
 	}

M internal/adapters/out/acmestore/store_test.go +1 -1


@@ -159,7 +159,7 @@ func TestStoreLockAcquireRelease(t *testing.T) {
159	159		assert.Contains(t, err.Error(), "lock already held")
160	160
161	161		require.NoError(t, unlock())
162		-	assert.NoFileExists(t, filepath.Join(root, lockFile))
	162	+	assert.FileExists(t, filepath.Join(root, lockFile))
163	163
164	164		unlock, err = store.Lock(ctx)
165	165		require.NoError(t, err)


@@ -159,7 +159,7 @@ func TestStoreLockAcquireRelease(t *testing.T) {
159		assert.Contains(t, err.Error(), "lock already held")	159		assert.Contains(t, err.Error(), "lock already held")
160			160
161		require.NoError(t, unlock())	161		require.NoError(t, unlock())
162	-	assert.NoFileExists(t, filepath.Join(root, lockFile))	162	+	assert.FileExists(t, filepath.Join(root, lockFile))
163			163
164		unlock, err = store.Lock(ctx)	164		unlock, err = store.Lock(ctx)
165		require.NoError(t, err)	165		require.NoError(t, err)

M internal/usecase/publictls/renewal.go +19 -3


@@ -5,6 +5,8 @@ import (
5	5		"fmt"
6	6		"time"
7	7
	8	+	"github.com/bnema/zerowrap"
	9	+
8	10		"github.com/bnema/gordon/internal/boundaries/out"
9	11		"github.com/bnema/gordon/internal/domain"
10	12		)

@@ -35,6 +37,10 @@ func (s *Service) StartRenewalLoop(ctx context.Context, interval time.Duration)
35	37
36	38		// Derive a cancellable context so Stop can terminate the loop.
37	39		loopCtx, cancel := context.WithCancel(ctx)
	40	+	loopCtx = zerowrap.CtxWithFields(loopCtx, map[string]any{
	41	+	zerowrap.FieldLayer: "usecase",
	42	+	zerowrap.FieldUseCase: "RenewDueCertificates",
	43	+	})
38	44
39	45		// Atomically swap loop state under lock before cancelling the old loop,
40	46		// preventing overlapping loop goroutines and race conditions.

@@ -59,10 +65,12 @@ func (s *Service) StartRenewalLoop(ctx context.Context, interval time.Duration)
59	65		ticker := time.NewTicker(interval)
60	66		defer ticker.Stop()
61	67
	68	+	log := zerowrap.FromCtx(loopCtx)
	69	+
62	70		// Startup and reload paths reconcile routes; the periodic loop only renews
63	71		// due certificates to avoid repeated zone lookups and ACME ordering work.
64	72		if err := s.renewDueCertificates(loopCtx, time.Now()); err != nil {
65		-	s.log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
	73	+	log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
66	74		}
67	75
68	76		for {

@@ -71,7 +79,7 @@ func (s *Service) StartRenewalLoop(ctx context.Context, interval time.Duration)
71	79		return
72	80		case <-ticker.C:
73	81		if err := s.renewDueCertificates(loopCtx, time.Now()); err != nil {
74		-	s.log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
	82	+	log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
75	83		}
76	84		}
77	85		}

@@ -86,6 +94,7 @@ func (s *Service) StartRenewalLoop(ctx context.Context, interval time.Duration)
86	94		// Per-certificate errors are recorded in lastErr and do not halt the loop.
87	95		// Returns nil unless the store or issuer is nil (systemic error).
88	96		func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error {
	97	+	log := zerowrap.FromCtx(ctx)
89	98		if !s.cfg.Enabled {
90	99		return nil
91	100		}

@@ -121,6 +130,13 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error
121	130		s.mu.Unlock()
122	131		continue
123	132		}
	133	+	if err := populateStoredCertificate(renewed); err != nil {
	134	+	s.mu.Lock()
	135	+	s.lastErr[cert.ID] = err.Error()
	136	+	s.mu.Unlock()
	137	+	continue
	138	+	}
	139	+	renewed.LastError = ""
124	140
125	141		// Acquire store lock only around Save to avoid holding it across
126	142		// the potentially long-running Issuer.Renew call.

@@ -134,7 +150,7 @@ func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error
134	150
135	151		saveErr := s.deps.Store.Save(ctx, *renewed)
136	152		if unlockErr := unlock(); unlockErr != nil {
137		-	s.log.Warn().Err(unlockErr).Msg("failed to release store lock")
	153	+	log.Warn().Err(unlockErr).Msg("failed to release store lock")
138	154		}
139	155
140	156		if saveErr != nil {

 	"fmt"
 	"time"
+	"github.com/bnema/zerowrap"
 	"github.com/bnema/gordon/internal/boundaries/out"
 	"github.com/bnema/gordon/internal/domain"
 )
 	// Derive a cancellable context so Stop can terminate the loop.
 	loopCtx, cancel := context.WithCancel(ctx)
+	loopCtx = zerowrap.CtxWithFields(loopCtx, map[string]any{
+		zerowrap.FieldLayer:   "usecase",
+		zerowrap.FieldUseCase: "RenewDueCertificates",
+	})
 	// Atomically swap loop state under lock before cancelling the old loop,
 	// preventing overlapping loop goroutines and race conditions.
 		ticker := time.NewTicker(interval)
 		defer ticker.Stop()
+		log := zerowrap.FromCtx(loopCtx)
 		// Startup and reload paths reconcile routes; the periodic loop only renews
 		// due certificates to avoid repeated zone lookups and ACME ordering work.
 		if err := s.renewDueCertificates(loopCtx, time.Now()); err != nil {
-			s.log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
+			log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
 		}
 		for {
 				return
 			case <-ticker.C:
 				if err := s.renewDueCertificates(loopCtx, time.Now()); err != nil {
-					s.log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
+					log.Warn().Err(err).Msg("failed to renew due public TLS certificates")
 				}
 			}
 		}
 // Per-certificate errors are recorded in lastErr and do not halt the loop.
 // Returns nil unless the store or issuer is nil (systemic error).
 func (s *Service) renewDueCertificates(ctx context.Context, now time.Time) error {
+	log := zerowrap.FromCtx(ctx)
 	if !s.cfg.Enabled {
 		return nil
 	}
 			s.mu.Unlock()
 			continue
 		}
+		if err := populateStoredCertificate(renewed); err != nil {
+			s.mu.Lock()
+			s.lastErr[cert.ID] = err.Error()
+			s.mu.Unlock()
+			continue
+		}
+		renewed.LastError = ""
 		// Acquire store lock only around Save to avoid holding it across
 		// the potentially long-running Issuer.Renew call.
 		saveErr := s.deps.Store.Save(ctx, *renewed)
 		if unlockErr := unlock(); unlockErr != nil {
-			s.log.Warn().Err(unlockErr).Msg("failed to release store lock")
+			log.Warn().Err(unlockErr).Msg("failed to release store lock")
 		}
 		if saveErr != nil {

M internal/usecase/publictls/renewal_test.go +4 -0


@@ -145,6 +145,8 @@ func TestRenewDueCertificatesSavesAndUpdatesCache(t *testing.T) {
145	145
146	146		issuer, _ := newMockPublicCertificateIssuer(t, nil, func(_ context.Context, cert out.StoredCertificate) (*out.StoredCertificate, error) {
147	147		renewed := cert
	148	+	renewed.Certificate = tls.Certificate{}
	149	+	renewed.LastError = "previous renewal error"
148	150		renewed.NotAfter = notAfterRenewed
149	151		return &renewed, nil
150	152		})

@@ -190,6 +192,8 @@ func TestRenewDueCertificatesSavesAndUpdatesCache(t *testing.T) {
190	192		all := storeState.All()
191	193		require.Len(t, all, 1)
192	194		assert.True(t, all[0].NotAfter.Equal(notAfterRenewed), "store should have renewed NotAfter")
	195	+	assert.NotEmpty(t, all[0].Certificate.Certificate, "store should have normalized tls.Certificate")
	196	+	assert.Empty(t, all[0].LastError, "store should not persist stale LastError")
193	197
194	198		// Verify the cache has the updated certificate via Status.
195	199		status = svc.Status(ctx)


@@ -145,6 +145,8 @@ func TestRenewDueCertificatesSavesAndUpdatesCache(t *testing.T) {
145		145
146	issuer, _ := newMockPublicCertificateIssuer(t, nil, func(_ context.Context, cert out.StoredCertificate) (*out.StoredCertificate, error) {	146		issuer, _ := newMockPublicCertificateIssuer(t, nil, func(_ context.Context, cert out.StoredCertificate) (*out.StoredCertificate, error) {
147	renewed := cert	147		renewed := cert
		148	+	renewed.Certificate = tls.Certificate{}
		149	+	renewed.LastError = "previous renewal error"
148	renewed.NotAfter = notAfterRenewed	150		renewed.NotAfter = notAfterRenewed
149	return &renewed, nil	151		return &renewed, nil
150	})	152		})

@@ -190,6 +192,8 @@ func TestRenewDueCertificatesSavesAndUpdatesCache(t *testing.T) {
190	all := storeState.All()	192		all := storeState.All()
191	require.Len(t, all, 1)	193		require.Len(t, all, 1)
192	assert.True(t, all[0].NotAfter.Equal(notAfterRenewed), "store should have renewed NotAfter")	194		assert.True(t, all[0].NotAfter.Equal(notAfterRenewed), "store should have renewed NotAfter")
		195	+	assert.NotEmpty(t, all[0].Certificate.Certificate, "store should have normalized tls.Certificate")
		196	+	assert.Empty(t, all[0].LastError, "store should not persist stale LastError")
193		197
194	// Verify the cache has the updated certificate via Status.	198		// Verify the cache has the updated certificate via Status.
195	status = svc.Status(ctx)	199		status = svc.Status(ctx)