Repository navigation
Fix deadlock from taking db lock while holding authz lock in Authz() - #564
Open
wallrj-cyberark wants to merge 1 commit into
Open
wallrj-cyberark wants to merge 1 commit into
wallrj-cyberark wants to merge 1 commit into
Conversation
wallrj-cyberark
commented
Oct 1, 2026
wallrj-cyberark
left a comment
Author
There was a problem hiding this comment.
Self-review: one note on why the order is read without the authz lock, and one place I left alone.
wallrj-cyberark
added a commit
to wallrj-cyberark/cert-manager
that referenced
this pull request
Oct 1, 2026
Pebble can deadlock when a new-order, an authorization and a new-account request arrive at the same time. After that it accepts connections but never answers, so every ACME e2e test in the run times out. See letsencrypt/pebble#563. The new commit is current upstream Pebble main, plus the existing Ed25519 patch, plus the fix from letsencrypt/pebble#564. Also correct the comment: we build from cert-manager's fork of Pebble, not @inteon's, and it now carries two patches. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Richard Wall <richard.wall@cyberark.com>
aarongable
requested changes
Oct 1, 2026
Authz() held the authorization's write lock while it looked up the account, which takes the MemoryStore read lock. FindValidAuthorization takes the same two locks in the opposite order: it holds the MemoryStore read lock while it read-locks every authorization. Once a writer such as AddAccount queues on the MemoryStore lock, the three goroutines wait on each other and every later request that touches the db hangs. Authz() also read-locked the order while holding the authz lock, which is the reverse of the order used by Order.GetStatus. Do the order and account lookups before taking the authz lock. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Richard Wall <richard.wall@cyberark.com>
wallrj-cyberark
force-pushed
the
fix-authz-lock-order-deadlock
branch
from
October 2, 2026 05:46
f81564c to
2eced24
Compare
cert-manager-prow Bot
pushed a commit
to cert-manager/cert-manager
that referenced
this pull request
Oct 2, 2026
Pebble can deadlock when a new-order, an authorization and a new-account request arrive at the same time. After that it accepts connections but never answers, so every ACME e2e test in the run times out. See letsencrypt/pebble#563. The new commit is current upstream Pebble main, plus the existing Ed25519 patch, plus the fix from letsencrypt/pebble#564. Also correct the comment: we build from cert-manager's fork of Pebble, not @inteon's, and it now carries two patches. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Richard Wall <richard.wall@cyberark.com>
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 #563.
Authz()held the authorization's write lock while it looked up the account, which takes theMemoryStoreread lock.FindValidAuthorizationtakes the same locks in the opposite order. Once a writer such asAddAccountqueues on the store lock, the whole store wedges. #563 has the details and a goroutine dump.This change does both lookups before
authz.Lock():authz.Orderis set when the authorization is created and never changes, so it is safe to read before taking the authz lock. Reading it first also removes the authz-then-order lock order, which is the reverse ofOrder.GetStatus().validPOSTAsGETorgetAcctByKey. Both branches did this lookup first anyway, so responses and error order do not change.The authz lock now only covers reading and updating the authorization itself.
Testing
mainhung within 1 second in each of 3 runs. With this change it ran for 10 minutes and 264,962 operations without hanging.go test -race ./...passes. Thewfepackage has no tests, so the stress client is the only test of this path.[with Claude]