Pace certificate renewal retries instead of blocking on one long job#394
Open
Socialpranker wants to merge 1 commit into
Open
Pace certificate renewal retries instead of blocking on one long job#394Socialpranker wants to merge 1 commit into
Socialpranker wants to merge 1 commit into
Conversation
… job A certificate that fails to renew (e.g. because DNS is temporarily broken) used to get stuck: queueRenewalTask submitted a single job to the jobManager that internally retried for up to 30 days with a growing backoff. While that job was in flight, every subsequent maintenance tick tried to queue the same job again, but jobManager's dedup-by-name silently dropped it. So once the backoff grew to hours, a fixed DNS record wouldn't get picked up until the sleeping job woke up on its own schedule, sometimes many hours later, even though the logs made it look like a new attempt was queued every 10 minutes. This moves the backoff schedule out of the job and into queueRenewalTask itself. Each job now makes a single renewal attempt and returns right away, so it never occupies the dedup slot for more than one attempt. The periodic maintenance tick is now what actually paces retries, gated by the same retryIntervals schedule doWithRetry already used. RenewCertSync/RenewCertAsync are unchanged and still used by manageOne, on-demand handshakes, and forceRenew.
|
Something like this is really needed for anyone who handles a large number of domains on a single Caddy instance. Internally we use a thin cache service that we use as Caddy's ask endpoint that basically does exactly this. Otherwise the server effectively deadlocks itself attempting to provision SSL certificates with ACME providers. |
Member
|
Thanks for the contribution. Been out with some family things for a bit, I will try to get to this soon. |
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
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.
Fixes caddyserver/caddy#7843.
What's going on
queueRenewalTasksubmits a single job tojmthat internally callsdoWithRetry, which keeps retrying for up to 30 days with a growing backoff (up to 6 hours between attempts). While that job is still running,jm.Submit's dedup-by-name silently drops every subsequent attempt to queue the same renewal, because the name is only removed once the job returns.The periodic maintenance tick (
RenewCheckInterval, 10 minutes by default) keeps callingqueueRenewalTaskand logging "certificate expires soon; queuing for renewal" every time, which looks like it's doing something every 10 minutes — but it isn't. The actual next attempt only happens whenever the already-running job's internal sleep wakes up, which by that point in the backoff schedule can be hours away.That's what the linked issue is describing: a cert failed to renew because of a temporary DNS problem, the DNS got fixed, and the cert still sat there unrenewed for a long time because the collector was asleep inside a 6-hour backoff step from before the DNS was fixed.
What I changed
Moved the backoff schedule out of the job and into
queueRenewalTaskitself:queueRenewalTasknow checks a small per-domain backoff tracker (renewalBackoffin async.go) before submitting anything. If the domain failed recently and isn't due for a retry yet, it just returns without touchingjmat all.renewCertOnce(one attempt, no internal loop) instead ofRenewCertAsync(which loops viadoWithRetry). So the job returns right after a single attempt, and the dedup slot injmis only held for as long as that one attempt takes, not for the whole multi-day retry campaign.retryIntervalsthatdoWithRetryalready uses, so the pacing behavior (fast retries at first, growing to 6h) is preserved — it's just driven by the maintenance tick now instead of a sleeping goroutine.maxRetryDurationruns out), the tracker forgets the domain.RenewCertSyncandRenewCertAsyncthemselves are untouched. Nothing else that calls them (on-demand handshakes,manageOne, forced renewals) changes behavior.One side effect worth flagging: the "certificate expires soon; queuing for renewal" info log no longer fires on every maintenance tick while a domain is in its backoff window — it only fires right before an actual attempt. I think that's more honest than the old behavior (which logged that line every 10 minutes regardless of whether anything was actually attempted), but wanted to call it out since someone might be grepping for that line.
Testing
TestRenewalBackoffSchedule— unit test on the new backoff tracker directly, using synthetic timestamps (no real sleeping).TestQueueRenewalTaskPacesRetriesItself— exercisesqueueRenewalTaskagainst a fake issuer that always fails, calling it twice back-to-back. First call attempts and fails; the second call (still within the backoff window) makes no attempt at all, confirmed via the fake issuer's call count and the log output. I also temporarily removed the backoff check to confirm this test actually fails without the fix, then put it back.go build ./...,go vet ./...,gofmt -l .,go test -race ./...all clean, no regressions in the existing suite.Assistance Disclosure: I used Claude Code to help investigate and implement this — it dug up the root cause (the job-manager dedup + doWithRetry interaction) and drafted the fix, I reviewed the diff and ran the tests myself before opening this.