Fixes cubic review

Signed-off-by: Bryan Frimin <bryan@probo.com>

pkg/certmanager/acme.go

@@ -78,6 +78,8 @@ const (
 	OrderPollStatusInvalid  OrderPollStatus = "invalid"
 )
 
+const certificateURLPollInterval = 2 * time.Second
+
 type OrderPollResult struct {
 	Status OrderPollStatus
 	Order  *acme.Order
@@ -435,7 +437,14 @@ func (s *ACMEService) fetchOrderCertificateAfterFinalize(
 	}
 
 	if refreshed.Status != acme.StatusValid {
-		return nil, fmt.Errorf("%w: order status %q after finalize", ErrOrderNotReady, refreshed.Status)
+		// Keep finalizeErr in the chain so a rate-limited finalize still reaches
+		// handleError and triggers the ACME cooldown.
+		return nil, fmt.Errorf(
+			"%w: order status %q after finalize: %w",
+			ErrOrderNotReady,
+			refreshed.Status,
+			finalizeErr,
+		)
 	}
 
 	der, err := s.fetchOrderCertificate(ctx, orderURL, refreshed)
@@ -473,20 +482,29 @@ func (s *ACMEService) refreshOrderCertificateURL(
 		return order, nil
 	}
 
-	refreshed, err := s.client.GetOrder(ctx, orderURL)
-	if err != nil {
-		return nil, fmt.Errorf("cannot refresh order: %w", err)
-	}
+	for {
+		refreshed, err := s.client.GetOrder(ctx, orderURL)
+		if err != nil {
+			return nil, fmt.Errorf("cannot refresh order: %w", err)
+		}
 
-	if refreshed.CertURL != "" {
-		return refreshed, nil
-	}
+		if refreshed.CertURL != "" {
+			return refreshed, nil
+		}
 
-	if refreshed.Status != acme.StatusValid {
-		return nil, fmt.Errorf("%w: order left valid state while waiting for certificate URL", ErrOrderNotReady)
-	}
+		if refreshed.Status != acme.StatusValid {
+			return nil, fmt.Errorf("%w: order left valid state while waiting for certificate URL", ErrOrderNotReady)
+		}
 
-	return nil, fmt.Errorf("%w: certificate URL not yet available", ErrOrderNotReady)
+		// Order is VALID but CertURL not published yet; keep polling within this
+		// tick so we fetch the certificate while we still hold the key that
+		// finalized it, rather than abandoning the order next tick.
+		select {
+		case <-ctx.Done():
+			return nil, fmt.Errorf("%w: certificate URL not yet available: %w", ErrOrderNotReady, ctx.Err())
+		case <-time.After(certificateURLPollInterval):
+		}
+	}
 }
 
 func createCSR(domain string, key crypto.Signer) ([]byte, error) {

pkg/certmanager/provision_worker.go

@@ -335,6 +335,20 @@ func (h *provisionHandler) processPollOrder(
 
 	switch poll.Status {
 	case OrderPollStatusNotReady:
+		// Re-accept best-effort: if a prior tick committed the challenge but its
+		// Accept never reached the CA, the order would otherwise stay pending
+		// forever since this path only polls. Re-accepting a pending challenge
+		// starts validation; one already processing is a no-op at the CA.
+		if err := h.acmeService.AcceptHTTPChallenge(ctx, challenge); err != nil {
+			h.logger.WarnCtx(
+				ctx,
+				"re-accepting HTTP challenge for not-ready order failed, will poll again",
+				log.String("hostname", certificate.Hostname),
+				log.String("certificate_id", certificate.ID.String()),
+				log.Error(err),
+			)
+		}
+
 		h.logger.InfoCtx(
 			ctx,
 			"ACME order not ready yet, will poll again",
@@ -531,11 +545,19 @@ func (h *provisionHandler) persistFailure(
 			outcome := decideProvisioningOutcome(row, errorCode)
 
 			row.ProvisioningError = provisioningErrorCodePtr(outcome.errorCode)
-			now := time.Now()
-			row.SSLLastAttemptAt = &now
 			row.Status = outcome.status
 			row.SSLRetryCount = outcome.retryCount
 
+			if isImmediateRetryRateLimit(provisionErr) {
+				// Retry-After: 0 permits an immediate retry; clearing the
+				// timestamp exempts the row from the claim query's backoff gates
+				// that would otherwise hold it despite the zero cooldown.
+				row.SSLLastAttemptAt = nil
+			} else {
+				now := time.Now()
+				row.SSLLastAttemptAt = &now
+			}
+
 			if outcome.clearACMEState {
 				row.HTTPChallengeToken = nil
 				row.HTTPChallengeKeyAuth = nil
@@ -552,6 +574,17 @@ func (h *provisionHandler) persistFailure(
 	)
 }
 
+// isImmediateRetryRateLimit reports whether err is an ACME rate limit with an
+// explicit Retry-After of zero, i.e. the CA permits an immediate retry.
+func isImmediateRetryRateLimit(err error) bool {
+	acmeErr, ok := errors.AsType[*ACMEError](err)
+	if !ok {
+		return false
+	}
+
+	return errors.Is(acmeErr, ErrACMERateLimited) && acmeErr.RetryAfter() == 0
+}
+
 // decideProvisioningOutcome computes status/retry/clear policy for a failed step.
 //
 // Two backoff regimes:

pkg/certmanager/service.go

@@ -141,42 +141,6 @@ func (s *Service) EnsureCertificate(
 	return certificate, nil
 }
 
-// UpdateHostname renames a certificate and resets its lifecycle so a fresh
-// certificate is provisioned for the new hostname. It returns the certificate
-// unchanged when the hostname already matches.
-func (s *Service) UpdateHostname(
-	ctx context.Context,
-	tx pg.Tx,
-	scope coredata.Scoper,
-	certificateID gid.GID,
-	newHostname string,
-) (*coredata.Certificate, error) {
-	certificate := &coredata.Certificate{}
-	if err := certificate.LoadByID(ctx, tx, scope, certificateID); err != nil {
-		return nil, fmt.Errorf("cannot load certificate: %w", err)
-	}
-
-	if certificate.Hostname == newHostname {
-		return certificate, nil
-	}
-
-	certificate.Hostname = newHostname
-	certificate.Status = coredata.CertificateStatusPending
-	certificate.SSLRetryCount = 0
-	certificate.SSLLastAttemptAt = nil
-	certificate.ProvisioningError = nil
-	certificate.HTTPChallengeToken = nil
-	certificate.HTTPChallengeKeyAuth = nil
-	certificate.HTTPChallengeURL = nil
-	certificate.HTTPOrderURL = nil
-
-	if err := certificate.Update(ctx, tx, scope); err != nil {
-		return nil, fmt.Errorf("cannot update certificate: %w", err)
-	}
-
-	return certificate, nil
-}
-
 // Get returns a certificate by ID.
 func (s *Service) Get(
 	ctx context.Context,