fix(EVBL-47): disable pending-cert retries + always log ACME rate-limit context - #370
Open
evan-datadog wants to merge 11 commits into
Open
fix(EVBL-47): disable pending-cert retries + always log ACME rate-limit context#370evan-datadog wants to merge 11 commits into
evan-datadog wants to merge 11 commits into
Conversation
…imit context Retries were burning the CA's ACME rate limit (Let's Encrypt: 5 duplicate certs / failed validations per week per domain). A deterministic failure (broken DNS delegation, no DNS provider, bad creds, invalid identifier) can never succeed on retry, so each re-queue just created another ACME order / failed validation against that budget. - Set ACME_ADDITIONAL_ATTEMPTS = 0 and change the give-up condition to number_attempts >= ACME_ADDITIONAL_ATTEMPTS, so a pending cert is attempted exactly once and, on failure, marked resolved immediately (no re-queue). - Always emit the full ACME rate-limit context on a failure log (cn, authority, authority_id, number_attempts, dns_provider_id, last_error, and a rate_limit_relevant flag) so rate-limit burn is attributable to a specific cert / domain / authority for triage. Adds tests covering the no-re-queue behavior and the rate-limit log context. Workspace: local
This comment has been minimized.
This comment has been minimized.
… flakiness g is app-context scoped and persists across tests, so a User set by an authenticated request in one test leaks into later tests as a stale, detached (and expired) instance. Accessing its attributes then raises DetachedInstanceError in audit_log / certificate schema serialization. Clear g.current_user in the session fixture teardown so each test starts clean. This is needed for CI to pass reliably on master-based branches (same fix as PR #368). Workspace: local
current_app.logger.error() is silently dropped in the celery worker: the worker's _configure_worker_logging clears app.logger.handlers and the log propagates to an unhandled root logger, so no ERROR-level line is emitted (confirmed: zero ERROR logs in celery-worker). The print() and task-trace lines appear only because they go through Celery's own loggers. Log the rate-limit failure via a module-level logger (logging.getLogger(__name__)), which emits through Celery's configured handlers, so the rate_limit_relevant error with cn/authority/number_attempts/dns_provider_id/last_error actually surfaces for triage. Workspace: local
…anch Address review-swarm nits on PR #370: - Log a meaningful default ('No error message provided by CA') instead of str(None) when last_error is missing, and guard the str() conversion. - Add a clarifying comment that with ACME_ADDITIONAL_ATTEMPTS = 0 the number_attempts >= 0 condition is always true on the first failure, so the re-queue (else) branch is dead code retained only for future re-enabling. Adds a test for the missing-last_error default. Workspace: local
maperu
reviewed
Aug 19, 2026
Workspace: local
The claim that current_app.logger.error() is dropped in the Celery worker was unverified. _configure_worker_logging clears app.logger.handlers to prevent double-logging, but app.logger still propagates to root, which Celery has configured — same path as the module logger. All other current_app.logger calls in this file work correctly via propagation. Tests confirm current_app.logger.error emits the rate-limit error log. Workspace: local
maperu
requested changes
Aug 19, 2026
| # marked resolved immediately. Every failed issuance consumes the CA's ACME rate | ||
| # limit (e.g. Let's Encrypt: 5 duplicate certs / failed validations per week per | ||
| # domain), so re-queuing a deterministic failure just burns that budget. | ||
| ACME_ADDITIONAL_ATTEMPTS = 0 |
There was a problem hiding this comment.
If we’ve agreed pending-certificate failures are not retried and should instead be resolved with clear error logging/notifications, why keep a retry constant and an effectively dead requeue branch? Setting this to 0 obscures the intended behavior and also affects unrelated synchronous issuance. I’d remove the retry logic from fetch_acme_cert entirely. If create_certificate_immediately still needs retries, it should use its own clearly named constant. Reintroducing pending retries later should be a deliberate code change, not a constant bump.
Author
There was a problem hiding this comment.
removed it alltogether; we dont use create_certificate_immediately and if we do I dont think we need the retries for the same reason.
With ACME_ADDITIONAL_ATTEMPTS=0 the else (re-queue) branch in fetch_acme_cert and the cli fetch loop was already dead code on every failure. Remove it explicitly: - celery.py: unconditionally notify + resolve on failure; drop the if/else and the ACME_ADDITIONAL_ATTEMPTS import - cli.py: same — unconditional fail-fast; drop import - challenge_types.py: remove @Retry(stop_max_attempt_number=0) decorator from create_certificate_immediately (stop_max_attempt_number=0 already meant no retries); remove unused retrying import - constants.py: delete ACME_ADDITIONAL_ATTEMPTS number_attempts DB column and schema field are left in place — the field is still emitted in error logs for triage context. Workspace: local
On failure, re-queue fetch_acme_cert up to 2 times (number_attempts 0 and 1). On the third failure (number_attempts >= 2) mark resolved and notify the owner. Mirrors the same logic in the pending-cert CLI. Tests cover: first failure re-queues, second failure re-queues, third failure resolves + notifies, rate-limit context logged, missing last_error falls back to default message. Workspace: local
This reverts commit c010a3f. Workspace: local
Workspace: local
Workspace: local
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
fetch_acme_certfails, Lemur re-queues the task up toACME_ADDITIONAL_ATTEMPTStimes. For a deterministic failure (broken DNS delegation, no DNS provider, bad credentials, invalid identifier), retrying can never succeed so it exhausts the CA's rate limit.Separately, the failure log only carried
pending_cert_id,cn, andlast_error, making rate-limit burn hard to attribute to a specific cert / domain / authority.