fix(eventhubs): remove the management-client recovery deadlock - #4806
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Fixes Event Hubs management-client recovery deadlocks by avoiding lock-held AMQP initialization.
Changes:
- Replaces the management cache mutex with a swappable
OnceCell. - Adds recovery regression tests.
- Documents the fix in the changelog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
connection.rs |
Refactors management caching and adds tests. |
CHANGELOG.md |
Records the deadlock fix. |
|
I reproduced this deadlock live against a real namespace, on To get a real CBS failure inside the management-client build I corrupted the SAS key, then fired 20 concurrent On this branch the same probe finishes. All 20 callers error with I also checked the single-flight claim, since that is why this uses No regressions. Warm p50 is 279ms on this branch against 277ms on One caveat: the |
33ff7ec to
76621e6
Compare
76621e6 to
b60cfa6
Compare
Eldert Grootenboer (EldertGrootenboer)
left a comment
Eldert Grootenboer (EldertGrootenboer)
left a comment
There was a problem hiding this comment.
Approving. The lock-free OnceCell is the right fix here.
On the CHANGELOG thread: agree with keeping the entry. A hang that no longer happens is user-visible, and the sibling load-balancer entry in the same section is the same shape.
A few nits, none blocking:
- The cache primitive is
async_lock::OnceCell, not tokio. Worth confirming itsget_or_try_initcancel and drop-mid-init semantics match what you expect. - The
close_connectionArc::try_unwrapfallback looks unreachable, sinceclose_connectiontakesselfby value so no concurrent build can hold a cell-Arc clone. A one-line comment or adebug_assert!on the strong count would keep the next reader from reading it as a cleanup gap. - A single-task test injecting a CBS-authorize failure during the build would cover the literal self-deadlock path directly.
- The two tests duplicate the stalled-peer listener setup and never tear down the
while let Ok(accept())thread. Fine for a test binary, worth a helper if it spreads.
Creating the management client could hang the client instead of reporting an error. `ensure_amqp_management` held the `mgmt_client` mutex guard across the whole build: connect, begin session, authorize `$management` over CBS, attach the link. A link or connection failure during that CBS authorization runs the recovery hook, and both `ReconnectLink` and `ReconnectConnection` plans set `drop_mgmt_client`, so `apply_recovery_plan` takes the same guard on the same task. `async_lock::Mutex` is not reentrant, so the task waited on itself. This is the ordinary transient-failure path the recovery code exists to handle, so the hang is reachable in normal operation. The management client now uses the lock-free `OnceCell` cache that the sender, session, and receiver paths already use. `mgmt_client: AsyncMutex<Option<Arc<AmqpManagement>>>` becomes `RwLock<Arc<OnceCell<Arc<AmqpManagement>>>>`. The build clones the cell pointer under a brief read lock and then runs `get_or_try_init` with no lock held. Recovery swaps in a fresh cell under a momentary write lock, so it never waits for a build in flight. Notes for review. A failed build leaves the cell uninitialized, so the next call retries, matching the old `is_none()` behavior. Concurrent callers still wait on the single in-flight build, so this does not create a burst of `$management` attaches. It adds the same staleness window the per-path caches already document for Azure#4454: a build in flight when recovery swaps the cell initializes the orphaned cell and its caller gets one pre-recovery client, then the retry loop picks up the fresh cell. Adds two tests that use production entry points only. They point the connection at a loopback peer that accepts the socket and never sends the AMQP protocol header, so the build stalls inside the region that used to be locked. `management_build_does_not_hold_mgmt_lock` asserts the cache lock is free while the build is in flight. `recovery_does_not_wait_for_in_flight_management_build` runs real recovery under a 10 second timeout. Both fail on unmodified main. Fixes Azure#4728
Both tests waited a fixed two seconds and then asserted. On a loaded runner the build task can still be pending after that wait, so the lock looks free and recovery completes for reasons that have nothing to do with the fix. Both tests could therefore pass against the old locking. The listener now signals the test when it accepts the first socket. The old code took the `mgmt_client` guard before it opened the connection, so a completed accept proves the build is inside the region that used to be locked. Each test waits for that signal instead of sleeping. Both still fail when the lock returns across the build. They now finish in about 10 ms rather than 2 seconds each.
The write guard was an inline expression inside `std::mem::replace`. A separate binding makes the lock scope visible and lets a debugger read the guard. An explicit drop marks where the lock is released, before the detach. Also shortens the changelog entry and leaves the detail to the issue.
heaths asked to keep the entry brief and to link to the issue for the detail. The mechanism sentence moves to Azure#4728.
49d9f6a to
2a89aab
Compare
Josue Gomez (SwayGom)
left a comment
Josue Gomez (SwayGom)
left a comment
There was a problem hiding this comment.
Approving. The deadlock fix is correct and the tests are unusually good.
Verified the core fix. let cell = self.mgmt_client.read().await.clone(); drops the read guard at the semicolon, so get_or_try_init runs with no lock held. apply_recovery_plan swaps the cell pointer under a brief write lock that is never held across a build. That genuinely breaks the five-hop self-deadlock chain — the recovery hook can now take the write lock while a build is in flight on the same task.
The tests are the right kind. Using a real TCP peer that accepts and never sends the AMQP header, with a oneshot accept signal as the synchronization point rather than a sleep, means these actually fail against the old implementation and won't flake on a loaded runner. recovery_does_not_wait_for_in_flight_management_build exercises the production entry points instead of poking internals, which is what makes it a real regression guard. Nice.
Two non-blocking notes.
1. close_connection can silently skip the detach.
if let Some(Some(management_client)) = Arc::try_unwrap(management_cell)
.ok()
.map(OnceCell::into_inner)Arc::try_unwrap fails if any concurrent ensure_amqp_management still holds a clone of the cell pointer, and the .ok() discards that case silently. The management client is then never detached. The previous Option::take() always retrieved it when present.
In the common case the refcount is 1 by the time close runs, so this is narrow — but "close raced a build in flight" is exactly the situation this PR makes reachable, since builds now run without the lock. Worth either handling the Err arm (detach through the shared Arc rather than requiring exclusive ownership) or leaving a comment that the detach is best-effort under concurrent build.
2. Coordinate with #4571 — the new mgmt cell won't be generation-protected.
The description notes this change "adds the same staleness window the per-path caches already document for #4454." #4571 exists specifically to close #4454, and it converts session_instances / sender_instances / receiver_instances to a GenerationalCell wrapper. It does not touch mgmt_client, because on its base that field is still an AsyncMutex.
So whichever of these lands second, the mgmt cell ends up as the only cache without the generation check — a build in flight when recovery swaps the cell would still hand back one pre-recovery management client with nothing to catch it.
Not a defect in this PR, but it needs an owner. Both this and #4571 also modify the RecoverableConnection field list, the constructor, apply_recovery_plan, and the mod tests block in connection.rs, and #4895 adds a field too — so all three will conflict textually regardless. Suggest merging #4571 first and rebasing this onto it so the mgmt cell can be made generational in the same pass.
Summary
Creating the management client could hang the client instead of reporting an error.
Motivation
ensure_amqp_managementheld themgmt_clientmutex guard across the whole build. The build connects, begins a session, authorizes$managementover CBS, and attaches the link.A link or connection failure during that CBS authorization runs the recovery hook. Both the
ReconnectLinkandReconnectConnectionplans setdrop_mgmt_client, soapply_recovery_plantakes the same guard on the same task.async_lock::Mutexis not reentrant, so the task waited on itself.The chain has five hops.
ensure_amqp_managementtakes the guard and awaits the build under it.authorize_path.RecoverableClaimsBasedSecurity, whose retry loop passesrecover_from_erroras its recovery hook.recover_with_backoffawaits that hook inline, with no spawn, no timeout, and notry_lock.apply_recovery_plantakes the guard again.This is a common transient-failure path. The recovery code exists to handle it. A CBS link detach or a dropped connection during
$managementauthorization is what triggers it. The existingforce_errors_*tests never reach it, because they inject atRecoverableManagementClient::call, which runs before the guard is taken.Changes
The management client now uses the lock-free
OnceCellcache that the sender, session and receiver paths already use.mgmt_client: AsyncMutex<Option<Arc<AmqpManagement>>>becomesRwLock<Arc<OnceCell<Arc<AmqpManagement>>>>. The build clones the cell pointer under a brief read lock, then runsget_or_try_initwith no lock held.apply_recovery_planswaps in a fresh cell under a brief write lock, so recovery never waits for a build in flight.close_connectionswaps the cell out and detaches outside the lock.Design notes.
is_none()behavior.$managementattaches. The cell is necessary for that reason. The simpler pattern of dropping the guard, building, then re-locking and inserting permits concurrent builds.Err("Missing Management Client")branch is gone. The function now returns the build's own error.Related
Fixes #4728
#4810 covers the same family of defect on the
connectionsmutex, which this change does not touch.