From 967669957c616051d14c5a0d2ffe14f447342a57 Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Tue, 18 Aug 2026 11:59:54 -0400 Subject: [PATCH 01/11] fix(EVBL-47): disable pending-cert retries and always log ACME rate-limit 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 --- lemur/common/celery.py | 15 ++- lemur/constants.py | 6 +- lemur/tests/test_pending_cert_no_retry.py | 125 ++++++++++++++++++++++ 3 files changed, 142 insertions(+), 4 deletions(-) create mode 100644 lemur/tests/test_pending_cert_no_retry.py diff --git a/lemur/common/celery.py b/lemur/common/celery.py index 195f8355f8..bc7e2ca2b8 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -357,10 +357,19 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): error_log = copy.deepcopy(log_data) error_log["message"] = "Pending certificate creation failure" error_log["pending_cert_id"] = pending_cert.id - error_log["last_error"] = cert.get("last_error") error_log["cn"] = pending_cert.cn - - if pending_cert.number_attempts > ACME_ADDITIONAL_ATTEMPTS: + error_log["authority"] = cert_authority.name + error_log["authority_id"] = pending_cert.authority_id + error_log["number_attempts"] = pending_cert.number_attempts + error_log["dns_provider_id"] = pending_cert.dns_provider_id + error_log["last_error"] = str(cert.get("last_error")) + # Every failed issuance consumes the CA's ACME rate limit (e.g. Let's + # Encrypt: 5 duplicate certs / failed validations per week per domain). + # Always emit the full context so rate-limit burn is attributable to a + # specific cert / domain / authority for triage. + error_log["rate_limit_relevant"] = True + + if pending_cert.number_attempts >= ACME_ADDITIONAL_ATTEMPTS: error_log["message"] = "Deleting pending certificate" send_pending_failure_notification( pending_cert, notify_owner=pending_cert.notify diff --git a/lemur/constants.py b/lemur/constants.py index 8b25e2f4a1..7857ef359c 100644 --- a/lemur/constants.py +++ b/lemur/constants.py @@ -16,7 +16,11 @@ # when ACME attempts to resolve a certificate try in total 3 times -ACME_ADDITIONAL_ATTEMPTS = 2 +# Retries are disabled: a pending cert is attempted exactly once and, on failure, +# 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 CERTIFICATE_KEY_TYPES = [ "RSA2048", diff --git a/lemur/tests/test_pending_cert_no_retry.py b/lemur/tests/test_pending_cert_no_retry.py new file mode 100644 index 0000000000..515164cf75 --- /dev/null +++ b/lemur/tests/test_pending_cert_no_retry.py @@ -0,0 +1,125 @@ +"""Tests for fetch_acme_cert no-retry + ACME rate-limit logging (EVBL-47). + +Verifies that, with retries disabled (ACME_ADDITIONAL_ATTEMPTS = 0), a pending +certificate that fails issuance is resolved immediately on the first attempt (no +re-queue), and that the failure log always carries the full ACME rate-limit +context (cn, authority, number of attempts, DNS provider, error) so rate-limit +burn is attributable for triage. +""" + +import sys +from unittest.mock import MagicMock, patch + +import pytest + +# celery.py connects to Redis at module level; pre-import it with Redis mocked so +# the @patch decorators below don't trigger a real Redis connection on first import. +if "lemur.common.celery" not in sys.modules: + with patch("redis.StrictRedis") as _mock_redis: + _mock_redis.return_value.set.return_value = True + import lemur.common.celery # noqa: F401 + +import lemur.common.celery as _celery_module # noqa: E402 + +from lemur.common.celery import fetch_acme_cert # noqa: E402 + + +@pytest.fixture(autouse=True) +def _mock_celery_current_app(monkeypatch): + """Scope the current_app mock to each test and restore the original on teardown.""" + monkeypatch.setattr(_celery_module, "current_app", MagicMock()) + + +def _pending_cert(id, number_attempts=0): + pc = MagicMock() + pc.id = id + pc.number_attempts = number_attempts + pc.resolved = False + pc.cn = "*.us3.ddbuild.io" + pc.notify = True + pc.owner = "joe@example.com" + pc.authority_id = 14 + pc.dns_provider_id = 6 + return pc + + +def _run_fetch_acme_cert(pc, last_error): + """Run fetch_acme_cert(id) with get_ordered_certificates returning a single failure. + + Returns the started mocks keyed by name so tests can assert on call behavior. + """ + plugin = MagicMock() + plugin.get_ordered_certificates.return_value = [ + {"cert": False, "pending_cert": pc, "last_error": last_error} + ] + + authority = MagicMock() + authority.name = "LetsEncryptStaging2" + authority.plugin_name = "acme-issuer" + + patchers = [ + patch( + "lemur.common.celery.pending_certificate_service.get_pending_certs", + return_value=[pc], + ), + patch("lemur.common.celery.get_authority", return_value=authority), + patch("lemur.common.celery.plugins.get", return_value=plugin), + patch("lemur.common.celery.pending_certificate_service.get", return_value=pc), + patch("lemur.common.celery.send_pending_failure_notification"), + patch("lemur.common.celery.pending_certificate_service.update"), + patch("lemur.common.celery.pending_certificate_service.increment_attempt"), + patch("lemur.common.celery.fetch_acme_cert.delay"), + ] + started = [p.start() for p in patchers] + try: + fetch_acme_cert(pc.id) + finally: + for p in patchers: + p.stop() + + return { + "get_authority": started[1], + "update": started[5], + "increment_attempt": started[6], + "delay": started[7], + "logger": _celery_module.current_app.logger, + } + + +def _rate_limit_error_log(mocks): + """Return the failure error_log emitted via logger.error.""" + for call in mocks["logger"].error.call_args_list: + if call.args and isinstance(call.args[0], dict): + log = call.args[0] + if log.get("rate_limit_relevant") is True: + return log + raise AssertionError("no rate-limit-relevant error log emitted") + + +def test_fetch_acme_cert_failure_resolves_immediately_no_requeue(): + """With retries disabled, a failed pending cert is resolved on the first attempt.""" + pc = _pending_cert(1, number_attempts=0) + mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) + + # Marked resolved + assert any( + call.kwargs.get("resolved") is True for call in mocks["update"].call_args_list + ) + # Not re-queued, not incremented + mocks["delay"].assert_not_called() + mocks["increment_attempt"].assert_not_called() + + +def test_fetch_acme_cert_failure_logs_rate_limit_context(): + """The failure log always carries the ACME rate-limit-relevant context.""" + pc = _pending_cert(1, number_attempts=0) + mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) + + log = _rate_limit_error_log(mocks) + assert log["cn"] == pc.cn + assert log["authority"] == "LetsEncryptStaging2" + assert log["authority_id"] == pc.authority_id + assert log["number_attempts"] == pc.number_attempts + assert log["dns_provider_id"] == pc.dns_provider_id + assert log["rate_limit_relevant"] is True + assert "Failed verification" in log["last_error"] From 166aefb966a12e84eefdb27d648fafc17209163a Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Tue, 18 Aug 2026 12:23:09 -0400 Subject: [PATCH 02/11] test: clear g.current_user between tests to fix DetachedInstanceError 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 --- lemur/tests/conftest.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lemur/tests/conftest.py b/lemur/tests/conftest.py index 899b91fb2b..419ed91ee2 100644 --- a/lemur/tests/conftest.py +++ b/lemur/tests/conftest.py @@ -5,7 +5,7 @@ from cryptography import x509 from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import hashes -from flask import current_app +from flask import current_app, g from flask_principal import identity_changed, Identity from sqlalchemy.sql import text @@ -104,6 +104,11 @@ def session(db, request): db.session.begin_nested() yield db.session db.session.rollback() + # g is app-context scoped and persists across tests. Clear request-scoped + # state so a stale, detached User from an earlier test isn't reused by a + # later test (which raises DetachedInstanceError when its attributes are + # expired). + g.pop("current_user", None) @pytest.fixture(scope="function") From 1dd7021731b5280f5a2998f4a23760e31cf90a80 Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Tue, 18 Aug 2026 15:23:15 -0400 Subject: [PATCH 03/11] fix(EVBL-47): emit ACME rate-limit error log via module logger 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 --- lemur/common/celery.py | 11 +++++++++-- lemur/tests/test_pending_cert_no_retry.py | 3 ++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lemur/common/celery.py b/lemur/common/celery.py index bc7e2ca2b8..4996878db9 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -9,6 +9,7 @@ """ import copy +import logging import sys import time from celery import Celery @@ -36,6 +37,8 @@ from lemur.factory import create_app, json_log_formatter from lemur import fips from lemur.notifications import cli as cli_notification + +logger = logging.getLogger(__name__) from lemur.notifications.messaging import ( send_pending_failure_notification, send_reissue_no_endpoints_notification, @@ -366,7 +369,11 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): # Every failed issuance consumes the CA's ACME rate limit (e.g. Let's # Encrypt: 5 duplicate certs / failed validations per week per domain). # Always emit the full context so rate-limit burn is attributable to a - # specific cert / domain / authority for triage. + # specific cert / domain / authority for triage. Log via the module + # logger (not current_app.logger): in the celery worker, app.logger's + # handlers are cleared and it propagates to an unhandled root logger, + # so current_app.logger.error() is dropped. The module logger emits + # through Celery's configured handlers. error_log["rate_limit_relevant"] = True if pending_cert.number_attempts >= ACME_ADDITIONAL_ATTEMPTS: @@ -387,7 +394,7 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): ) # Add failed pending cert task back to queue fetch_acme_cert.delay(id, notify_reissue_cert_id) - current_app.logger.error(error_log) + logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new log_data["failed"] = failed diff --git a/lemur/tests/test_pending_cert_no_retry.py b/lemur/tests/test_pending_cert_no_retry.py index 515164cf75..32f081480e 100644 --- a/lemur/tests/test_pending_cert_no_retry.py +++ b/lemur/tests/test_pending_cert_no_retry.py @@ -69,6 +69,7 @@ def _run_fetch_acme_cert(pc, last_error): patch("lemur.common.celery.pending_certificate_service.update"), patch("lemur.common.celery.pending_certificate_service.increment_attempt"), patch("lemur.common.celery.fetch_acme_cert.delay"), + patch("lemur.common.celery.logger"), ] started = [p.start() for p in patchers] try: @@ -82,7 +83,7 @@ def _run_fetch_acme_cert(pc, last_error): "update": started[5], "increment_attempt": started[6], "delay": started[7], - "logger": _celery_module.current_app.logger, + "logger": started[8], } From a35666e75e8de80ecd5d2b10efe356c3df85a24b Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Tue, 18 Aug 2026 16:04:02 -0400 Subject: [PATCH 04/11] fix(EVBL-47): harden last_error logging + document zero-retry dead branch 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 --- lemur/common/celery.py | 10 +++++++++- lemur/tests/test_pending_cert_no_retry.py | 9 +++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/lemur/common/celery.py b/lemur/common/celery.py index 4996878db9..98bf947320 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -365,7 +365,10 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): error_log["authority_id"] = pending_cert.authority_id error_log["number_attempts"] = pending_cert.number_attempts error_log["dns_provider_id"] = pending_cert.dns_provider_id - error_log["last_error"] = str(cert.get("last_error")) + last_error = cert.get("last_error") + error_log["last_error"] = ( + str(last_error) if last_error is not None else "No error message provided by CA" + ) # Every failed issuance consumes the CA's ACME rate limit (e.g. Let's # Encrypt: 5 duplicate certs / failed validations per week per domain). # Always emit the full context so rate-limit burn is attributable to a @@ -376,6 +379,11 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): # through Celery's configured handlers. error_log["rate_limit_relevant"] = True + # Give up when number_attempts reaches ACME_ADDITIONAL_ATTEMPTS. With + # retries disabled (ACME_ADDITIONAL_ATTEMPTS = 0) this is always true on + # the first failure, so the else (re-queue) branch below is effectively + # dead code — retained only so retries can be re-enabled later by bumping + # the constant without restructuring this logic. if pending_cert.number_attempts >= ACME_ADDITIONAL_ATTEMPTS: error_log["message"] = "Deleting pending certificate" send_pending_failure_notification( diff --git a/lemur/tests/test_pending_cert_no_retry.py b/lemur/tests/test_pending_cert_no_retry.py index 32f081480e..a87717f02e 100644 --- a/lemur/tests/test_pending_cert_no_retry.py +++ b/lemur/tests/test_pending_cert_no_retry.py @@ -124,3 +124,12 @@ def test_fetch_acme_cert_failure_logs_rate_limit_context(): assert log["dns_provider_id"] == pc.dns_provider_id assert log["rate_limit_relevant"] is True assert "Failed verification" in log["last_error"] + + +def test_fetch_acme_cert_failure_logs_default_last_error_when_missing(): + """A missing last_error is logged as a meaningful default, not 'None'.""" + pc = _pending_cert(1, number_attempts=0) + mocks = _run_fetch_acme_cert(pc, None) + + log = _rate_limit_error_log(mocks) + assert log["last_error"] == "No error message provided by CA" From ebaaaf555957fbe7b93330058ae8f800db3a3573 Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Wed, 19 Aug 2026 12:45:33 -0400 Subject: [PATCH 05/11] refactor: move module-logger comment to declaration site Workspace: local --- lemur/common/celery.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lemur/common/celery.py b/lemur/common/celery.py index 98bf947320..b8bb3d4c97 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -38,6 +38,10 @@ from lemur import fips from lemur.notifications import cli as cli_notification +# Use a module-level logger rather than current_app.logger: in the Celery worker, +# app.logger's handlers are cleared by _configure_worker_logging below, so +# current_app.logger calls propagate to an unhandled root logger and are dropped. +# The module logger emits through Celery's configured handlers. logger = logging.getLogger(__name__) from lemur.notifications.messaging import ( send_pending_failure_notification, @@ -372,11 +376,7 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): # Every failed issuance consumes the CA's ACME rate limit (e.g. Let's # Encrypt: 5 duplicate certs / failed validations per week per domain). # Always emit the full context so rate-limit burn is attributable to a - # specific cert / domain / authority for triage. Log via the module - # logger (not current_app.logger): in the celery worker, app.logger's - # handlers are cleared and it propagates to an unhandled root logger, - # so current_app.logger.error() is dropped. The module logger emits - # through Celery's configured handlers. + # specific cert / domain / authority for triage. error_log["rate_limit_relevant"] = True # Give up when number_attempts reaches ACME_ADDITIONAL_ATTEMPTS. With From 2879c9f093885480c2013b18fabb83e64380a6ef Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Wed, 19 Aug 2026 13:14:14 -0400 Subject: [PATCH 06/11] revert: use current_app.logger.error (module logger was unnecessary) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lemur/common/celery.py | 8 +------- lemur/tests/test_pending_cert_no_retry.py | 6 ++---- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/lemur/common/celery.py b/lemur/common/celery.py index b8bb3d4c97..084d5921eb 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -9,7 +9,6 @@ """ import copy -import logging import sys import time from celery import Celery @@ -38,11 +37,6 @@ from lemur import fips from lemur.notifications import cli as cli_notification -# Use a module-level logger rather than current_app.logger: in the Celery worker, -# app.logger's handlers are cleared by _configure_worker_logging below, so -# current_app.logger calls propagate to an unhandled root logger and are dropped. -# The module logger emits through Celery's configured handlers. -logger = logging.getLogger(__name__) from lemur.notifications.messaging import ( send_pending_failure_notification, send_reissue_no_endpoints_notification, @@ -402,7 +396,7 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): ) # Add failed pending cert task back to queue fetch_acme_cert.delay(id, notify_reissue_cert_id) - logger.error(error_log) + current_app.logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new log_data["failed"] = failed diff --git a/lemur/tests/test_pending_cert_no_retry.py b/lemur/tests/test_pending_cert_no_retry.py index a87717f02e..621af64ff4 100644 --- a/lemur/tests/test_pending_cert_no_retry.py +++ b/lemur/tests/test_pending_cert_no_retry.py @@ -69,7 +69,6 @@ def _run_fetch_acme_cert(pc, last_error): patch("lemur.common.celery.pending_certificate_service.update"), patch("lemur.common.celery.pending_certificate_service.increment_attempt"), patch("lemur.common.celery.fetch_acme_cert.delay"), - patch("lemur.common.celery.logger"), ] started = [p.start() for p in patchers] try: @@ -83,13 +82,12 @@ def _run_fetch_acme_cert(pc, last_error): "update": started[5], "increment_attempt": started[6], "delay": started[7], - "logger": started[8], } def _rate_limit_error_log(mocks): - """Return the failure error_log emitted via logger.error.""" - for call in mocks["logger"].error.call_args_list: + """Return the failure error_log emitted via current_app.logger.error.""" + for call in _celery_module.current_app.logger.error.call_args_list: if call.args and isinstance(call.args[0], dict): log = call.args[0] if log.get("rate_limit_relevant") is True: From 2cbbe970ee25c49aeff0c4a7e72397baf9b04abe Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Wed, 19 Aug 2026 14:17:36 -0400 Subject: [PATCH 07/11] refactor: remove ACME_ADDITIONAL_ATTEMPTS retry branch entirely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- lemur/common/celery.py | 33 ++++++--------------- lemur/constants.py | 1 - lemur/pending_certificates/cli.py | 18 ++++------- lemur/plugins/lemur_acme/challenge_types.py | 3 -- 4 files changed, 14 insertions(+), 41 deletions(-) diff --git a/lemur/common/celery.py b/lemur/common/celery.py index 084d5921eb..aaaf1d187c 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -30,7 +30,6 @@ from lemur.certificates import cli as cli_certificate from lemur.certificates import service as certificate_service from lemur.common.redis import RedisHandler -from lemur.constants import ACME_ADDITIONAL_ATTEMPTS from lemur.dns_providers import cli as cli_dns_providers from lemur.extensions import metrics from lemur.factory import create_app, json_log_formatter @@ -373,29 +372,15 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): # specific cert / domain / authority for triage. error_log["rate_limit_relevant"] = True - # Give up when number_attempts reaches ACME_ADDITIONAL_ATTEMPTS. With - # retries disabled (ACME_ADDITIONAL_ATTEMPTS = 0) this is always true on - # the first failure, so the else (re-queue) branch below is effectively - # dead code — retained only so retries can be re-enabled later by bumping - # the constant without restructuring this logic. - if pending_cert.number_attempts >= ACME_ADDITIONAL_ATTEMPTS: - error_log["message"] = "Deleting pending certificate" - send_pending_failure_notification( - pending_cert, notify_owner=pending_cert.notify - ) - if notify_reissue_cert_id is not None: - send_reissue_failed_notification(pending_cert) - # Mark the pending cert as resolved - pending_certificate_service.update( - cert.get("pending_cert").id, resolved=True - ) - else: - pending_certificate_service.increment_attempt(pending_cert) - pending_certificate_service.update( - cert.get("pending_cert").id, status=str(cert.get("last_error")) - ) - # Add failed pending cert task back to queue - fetch_acme_cert.delay(id, notify_reissue_cert_id) + error_log["message"] = "Deleting pending certificate" + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + if notify_reissue_cert_id is not None: + send_reissue_failed_notification(pending_cert) + pending_certificate_service.update( + cert.get("pending_cert").id, resolved=True + ) current_app.logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new diff --git a/lemur/constants.py b/lemur/constants.py index 7857ef359c..e0632c4e15 100644 --- a/lemur/constants.py +++ b/lemur/constants.py @@ -20,7 +20,6 @@ # 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 CERTIFICATE_KEY_TYPES = [ "RSA2048", diff --git a/lemur/pending_certificates/cli.py b/lemur/pending_certificates/cli.py index 73b0ce2b50..9b13431327 100644 --- a/lemur/pending_certificates/cli.py +++ b/lemur/pending_certificates/cli.py @@ -12,7 +12,6 @@ from flask_script import Manager from lemur.authorities.service import get as get_authority -from lemur.constants import ACME_ADDITIONAL_ATTEMPTS from lemur.notifications.messaging import send_pending_failure_notification from lemur.pending_certificates import service as pending_certificate_service from lemur.plugins.base import plugins @@ -109,18 +108,11 @@ def fetch_all_acme(): error_log["last_error"] = cert.get("last_error") error_log["cn"] = pending_cert.cn - if pending_cert.number_attempts > ACME_ADDITIONAL_ATTEMPTS: - error_log["message"] = "Marking pending certificate as resolved" - send_pending_failure_notification( - pending_cert, notify_owner=pending_cert.notify - ) - # Mark "resolved" as True - pending_certificate_service.update(cert.id, resolved=True) - else: - pending_certificate_service.increment_attempt(pending_cert) - pending_certificate_service.update( - cert.get("pending_cert").id, status=str(cert.get("last_error")) - ) + error_log["message"] = "Marking pending certificate as resolved" + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + pending_certificate_service.update(cert.id, resolved=True) current_app.logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new diff --git a/lemur/plugins/lemur_acme/challenge_types.py b/lemur/plugins/lemur_acme/challenge_types.py index 1409a6f128..ed58040735 100644 --- a/lemur/plugins/lemur_acme/challenge_types.py +++ b/lemur/plugins/lemur_acme/challenge_types.py @@ -19,7 +19,6 @@ from sentry_sdk import capture_exception from lemur.authorizations import service as authorization_service -from lemur.constants import ACME_ADDITIONAL_ATTEMPTS from lemur.common.utils import drop_last_cert_from_chain, csr_to_string from lemur.exceptions import LemurException, InvalidConfiguration from lemur.extensions import metrics @@ -27,7 +26,6 @@ from lemur.destinations import service as destination_service from lemur.plugins.lemur_acme.acme_handlers import AcmeHandler, AcmeDnsHandler -from retrying import retry class AcmeChallengeMissmatchError(LemurException): @@ -299,7 +297,6 @@ def create_certificate(self, csr, issuer_options): # TODO add external ID (if possible) return pem_certificate, pem_certificate_chain, None - @retry(stop_max_attempt_number=ACME_ADDITIONAL_ATTEMPTS, wait_fixed=5000) def create_certificate_immediately(self, acme_client, order_info, csr): try: order = acme_client.new_order(csr_to_string(csr)) From c010a3fd3cee155c6493e1fa8de5e20174858fc2 Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Wed, 19 Aug 2026 14:21:29 -0400 Subject: [PATCH 08/11] feat: retry pending cert issuance up to 3 times total 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 --- lemur/common/celery.py | 25 +++++++----- lemur/pending_certificates/cli.py | 16 +++++--- lemur/tests/test_pending_cert_no_retry.py | 46 +++++++++++++++++------ 3 files changed, 61 insertions(+), 26 deletions(-) diff --git a/lemur/common/celery.py b/lemur/common/celery.py index aaaf1d187c..b992f165bb 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -372,15 +372,22 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): # specific cert / domain / authority for triage. error_log["rate_limit_relevant"] = True - error_log["message"] = "Deleting pending certificate" - send_pending_failure_notification( - pending_cert, notify_owner=pending_cert.notify - ) - if notify_reissue_cert_id is not None: - send_reissue_failed_notification(pending_cert) - pending_certificate_service.update( - cert.get("pending_cert").id, resolved=True - ) + if pending_cert.number_attempts >= 2: + error_log["message"] = "Deleting pending certificate" + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + if notify_reissue_cert_id is not None: + send_reissue_failed_notification(pending_cert) + pending_certificate_service.update( + cert.get("pending_cert").id, resolved=True + ) + else: + pending_certificate_service.increment_attempt(pending_cert) + pending_certificate_service.update( + cert.get("pending_cert").id, status=str(cert.get("last_error")) + ) + fetch_acme_cert.delay(id, notify_reissue_cert_id) current_app.logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new diff --git a/lemur/pending_certificates/cli.py b/lemur/pending_certificates/cli.py index 9b13431327..922c6a1292 100644 --- a/lemur/pending_certificates/cli.py +++ b/lemur/pending_certificates/cli.py @@ -108,11 +108,17 @@ def fetch_all_acme(): error_log["last_error"] = cert.get("last_error") error_log["cn"] = pending_cert.cn - error_log["message"] = "Marking pending certificate as resolved" - send_pending_failure_notification( - pending_cert, notify_owner=pending_cert.notify - ) - pending_certificate_service.update(cert.id, resolved=True) + if pending_cert.number_attempts >= 2: + error_log["message"] = "Marking pending certificate as resolved" + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + pending_certificate_service.update(cert.id, resolved=True) + else: + pending_certificate_service.increment_attempt(pending_cert) + pending_certificate_service.update( + cert.get("pending_cert").id, status=str(cert.get("last_error")) + ) current_app.logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new diff --git a/lemur/tests/test_pending_cert_no_retry.py b/lemur/tests/test_pending_cert_no_retry.py index 621af64ff4..1690d0f113 100644 --- a/lemur/tests/test_pending_cert_no_retry.py +++ b/lemur/tests/test_pending_cert_no_retry.py @@ -1,10 +1,10 @@ -"""Tests for fetch_acme_cert no-retry + ACME rate-limit logging (EVBL-47). +"""Tests for fetch_acme_cert 3-attempt retry + ACME rate-limit logging (EVBL-47). -Verifies that, with retries disabled (ACME_ADDITIONAL_ATTEMPTS = 0), a pending -certificate that fails issuance is resolved immediately on the first attempt (no -re-queue), and that the failure log always carries the full ACME rate-limit -context (cn, authority, number of attempts, DNS provider, error) so rate-limit -burn is attributable for triage. +Verifies that a pending certificate that fails issuance is retried up to 3 times +in total (number_attempts 0, 1, 2), resolved and the owner notified on the third +failure, and that the failure log always carries the full ACME rate-limit context +(cn, authority, number of attempts, DNS provider, error) so rate-limit burn is +attributable for triage. """ import sys @@ -95,23 +95,45 @@ def _rate_limit_error_log(mocks): raise AssertionError("no rate-limit-relevant error log emitted") -def test_fetch_acme_cert_failure_resolves_immediately_no_requeue(): - """With retries disabled, a failed pending cert is resolved on the first attempt.""" +def test_fetch_acme_cert_first_failure_requeues(): + """On the first failure (number_attempts=0) the cert is re-queued, not resolved.""" pc = _pending_cert(1, number_attempts=0) + mocks = _run_fetch_acme_cert(pc, ValueError("dns timeout")) + + mocks["increment_attempt"].assert_called_once() + mocks["delay"].assert_called_once() + assert not any( + call.kwargs.get("resolved") is True for call in mocks["update"].call_args_list + ) + + +def test_fetch_acme_cert_second_failure_requeues(): + """On the second failure (number_attempts=1) the cert is still re-queued.""" + pc = _pending_cert(1, number_attempts=1) + mocks = _run_fetch_acme_cert(pc, ValueError("dns timeout")) + + mocks["increment_attempt"].assert_called_once() + mocks["delay"].assert_called_once() + assert not any( + call.kwargs.get("resolved") is True for call in mocks["update"].call_args_list + ) + + +def test_fetch_acme_cert_third_failure_resolves(): + """On the third failure (number_attempts=2) the cert is resolved and owner notified.""" + pc = _pending_cert(1, number_attempts=2) mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) - # Marked resolved assert any( call.kwargs.get("resolved") is True for call in mocks["update"].call_args_list ) - # Not re-queued, not incremented mocks["delay"].assert_not_called() mocks["increment_attempt"].assert_not_called() def test_fetch_acme_cert_failure_logs_rate_limit_context(): """The failure log always carries the ACME rate-limit-relevant context.""" - pc = _pending_cert(1, number_attempts=0) + pc = _pending_cert(1, number_attempts=2) mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) log = _rate_limit_error_log(mocks) @@ -126,7 +148,7 @@ def test_fetch_acme_cert_failure_logs_rate_limit_context(): def test_fetch_acme_cert_failure_logs_default_last_error_when_missing(): """A missing last_error is logged as a meaningful default, not 'None'.""" - pc = _pending_cert(1, number_attempts=0) + pc = _pending_cert(1, number_attempts=2) mocks = _run_fetch_acme_cert(pc, None) log = _rate_limit_error_log(mocks) From c27b8004458ac1b33360933097934e07b5d66ba0 Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Wed, 19 Aug 2026 14:28:35 -0400 Subject: [PATCH 09/11] Revert "feat: retry pending cert issuance up to 3 times total" This reverts commit c010a3fd3cee155c6493e1fa8de5e20174858fc2. Workspace: local --- lemur/common/celery.py | 25 +++++------- lemur/pending_certificates/cli.py | 16 +++----- lemur/tests/test_pending_cert_no_retry.py | 46 ++++++----------------- 3 files changed, 26 insertions(+), 61 deletions(-) diff --git a/lemur/common/celery.py b/lemur/common/celery.py index b992f165bb..aaaf1d187c 100644 --- a/lemur/common/celery.py +++ b/lemur/common/celery.py @@ -372,22 +372,15 @@ def fetch_acme_cert(id, notify_reissue_cert_id=None): # specific cert / domain / authority for triage. error_log["rate_limit_relevant"] = True - if pending_cert.number_attempts >= 2: - error_log["message"] = "Deleting pending certificate" - send_pending_failure_notification( - pending_cert, notify_owner=pending_cert.notify - ) - if notify_reissue_cert_id is not None: - send_reissue_failed_notification(pending_cert) - pending_certificate_service.update( - cert.get("pending_cert").id, resolved=True - ) - else: - pending_certificate_service.increment_attempt(pending_cert) - pending_certificate_service.update( - cert.get("pending_cert").id, status=str(cert.get("last_error")) - ) - fetch_acme_cert.delay(id, notify_reissue_cert_id) + error_log["message"] = "Deleting pending certificate" + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + if notify_reissue_cert_id is not None: + send_reissue_failed_notification(pending_cert) + pending_certificate_service.update( + cert.get("pending_cert").id, resolved=True + ) current_app.logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new diff --git a/lemur/pending_certificates/cli.py b/lemur/pending_certificates/cli.py index 922c6a1292..9b13431327 100644 --- a/lemur/pending_certificates/cli.py +++ b/lemur/pending_certificates/cli.py @@ -108,17 +108,11 @@ def fetch_all_acme(): error_log["last_error"] = cert.get("last_error") error_log["cn"] = pending_cert.cn - if pending_cert.number_attempts >= 2: - error_log["message"] = "Marking pending certificate as resolved" - send_pending_failure_notification( - pending_cert, notify_owner=pending_cert.notify - ) - pending_certificate_service.update(cert.id, resolved=True) - else: - pending_certificate_service.increment_attempt(pending_cert) - pending_certificate_service.update( - cert.get("pending_cert").id, status=str(cert.get("last_error")) - ) + error_log["message"] = "Marking pending certificate as resolved" + send_pending_failure_notification( + pending_cert, notify_owner=pending_cert.notify + ) + pending_certificate_service.update(cert.id, resolved=True) current_app.logger.error(error_log) log_data["message"] = "Complete" log_data["new"] = new diff --git a/lemur/tests/test_pending_cert_no_retry.py b/lemur/tests/test_pending_cert_no_retry.py index 1690d0f113..621af64ff4 100644 --- a/lemur/tests/test_pending_cert_no_retry.py +++ b/lemur/tests/test_pending_cert_no_retry.py @@ -1,10 +1,10 @@ -"""Tests for fetch_acme_cert 3-attempt retry + ACME rate-limit logging (EVBL-47). +"""Tests for fetch_acme_cert no-retry + ACME rate-limit logging (EVBL-47). -Verifies that a pending certificate that fails issuance is retried up to 3 times -in total (number_attempts 0, 1, 2), resolved and the owner notified on the third -failure, and that the failure log always carries the full ACME rate-limit context -(cn, authority, number of attempts, DNS provider, error) so rate-limit burn is -attributable for triage. +Verifies that, with retries disabled (ACME_ADDITIONAL_ATTEMPTS = 0), a pending +certificate that fails issuance is resolved immediately on the first attempt (no +re-queue), and that the failure log always carries the full ACME rate-limit +context (cn, authority, number of attempts, DNS provider, error) so rate-limit +burn is attributable for triage. """ import sys @@ -95,45 +95,23 @@ def _rate_limit_error_log(mocks): raise AssertionError("no rate-limit-relevant error log emitted") -def test_fetch_acme_cert_first_failure_requeues(): - """On the first failure (number_attempts=0) the cert is re-queued, not resolved.""" +def test_fetch_acme_cert_failure_resolves_immediately_no_requeue(): + """With retries disabled, a failed pending cert is resolved on the first attempt.""" pc = _pending_cert(1, number_attempts=0) - mocks = _run_fetch_acme_cert(pc, ValueError("dns timeout")) - - mocks["increment_attempt"].assert_called_once() - mocks["delay"].assert_called_once() - assert not any( - call.kwargs.get("resolved") is True for call in mocks["update"].call_args_list - ) - - -def test_fetch_acme_cert_second_failure_requeues(): - """On the second failure (number_attempts=1) the cert is still re-queued.""" - pc = _pending_cert(1, number_attempts=1) - mocks = _run_fetch_acme_cert(pc, ValueError("dns timeout")) - - mocks["increment_attempt"].assert_called_once() - mocks["delay"].assert_called_once() - assert not any( - call.kwargs.get("resolved") is True for call in mocks["update"].call_args_list - ) - - -def test_fetch_acme_cert_third_failure_resolves(): - """On the third failure (number_attempts=2) the cert is resolved and owner notified.""" - pc = _pending_cert(1, number_attempts=2) mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) + # Marked resolved assert any( call.kwargs.get("resolved") is True for call in mocks["update"].call_args_list ) + # Not re-queued, not incremented mocks["delay"].assert_not_called() mocks["increment_attempt"].assert_not_called() def test_fetch_acme_cert_failure_logs_rate_limit_context(): """The failure log always carries the ACME rate-limit-relevant context.""" - pc = _pending_cert(1, number_attempts=2) + pc = _pending_cert(1, number_attempts=0) mocks = _run_fetch_acme_cert(pc, ValueError("Failed verification")) log = _rate_limit_error_log(mocks) @@ -148,7 +126,7 @@ def test_fetch_acme_cert_failure_logs_rate_limit_context(): def test_fetch_acme_cert_failure_logs_default_last_error_when_missing(): """A missing last_error is logged as a meaningful default, not 'None'.""" - pc = _pending_cert(1, number_attempts=2) + pc = _pending_cert(1, number_attempts=0) mocks = _run_fetch_acme_cert(pc, None) log = _rate_limit_error_log(mocks) From ac7ca05ce243f3e01e5487af334c7ab7b84f4f66 Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Wed, 19 Aug 2026 14:29:54 -0400 Subject: [PATCH 10/11] chore: remove stale ACME retry comment from constants.py Workspace: local --- lemur/constants.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lemur/constants.py b/lemur/constants.py index e0632c4e15..933476a50d 100644 --- a/lemur/constants.py +++ b/lemur/constants.py @@ -15,11 +15,6 @@ FAILURE_METRIC_STATUS = "failure" -# when ACME attempts to resolve a certificate try in total 3 times -# Retries are disabled: a pending cert is attempted exactly once and, on failure, -# 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. CERTIFICATE_KEY_TYPES = [ "RSA2048", From 188f279bb36ceb29b1b2269535840b13195912d1 Mon Sep 17 00:00:00 2001 From: "evan.mcelheny" Date: Wed, 19 Aug 2026 14:39:40 -0400 Subject: [PATCH 11/11] fix: remove extra blank lines to satisfy flake8 E303 Workspace: local --- lemur/constants.py | 1 - lemur/plugins/lemur_acme/challenge_types.py | 1 - 2 files changed, 2 deletions(-) diff --git a/lemur/constants.py b/lemur/constants.py index 933476a50d..07f86cbd5e 100644 --- a/lemur/constants.py +++ b/lemur/constants.py @@ -15,7 +15,6 @@ FAILURE_METRIC_STATUS = "failure" - CERTIFICATE_KEY_TYPES = [ "RSA2048", "RSA4096", diff --git a/lemur/plugins/lemur_acme/challenge_types.py b/lemur/plugins/lemur_acme/challenge_types.py index ed58040735..6e69a4a974 100644 --- a/lemur/plugins/lemur_acme/challenge_types.py +++ b/lemur/plugins/lemur_acme/challenge_types.py @@ -27,7 +27,6 @@ from lemur.plugins.lemur_acme.acme_handlers import AcmeHandler, AcmeDnsHandler - class AcmeChallengeMissmatchError(LemurException): pass