Fix ABBA deadlock between allocation cleanup and ICE-ufrag registration - #9
Merged
Merged
Conversation
…anup AllocationTable keeps a primary `allocations` map plus secondary index DashMaps. The cleanup paths (cleanup_expired/cleanup_inactive/ cleanup_orphaned_senders) hold an `allocations` shard write lock via `retain` and then mutate the secondary indices, establishing the order `allocations -> secondary`. register_ice_ufrags did the reverse: it held a `by_ice_ufrag` write lock (entry API) and then acquired an `allocations` read lock via get(). On the multi-thread runtime a concurrent cleanup and register on colliding shards deadlock (ABBA): neither thread crashes or logs, the inline packet loop blocks on the lock, and the whole server goes silent until restarted. Reorder register_ice_ufrags to acquire `allocations` first, matching the cleanup paths, preserving the atomic by_ice_ufrag dedup and exact return semantics. Document the lock-order invariant on the AllocationTable struct. Add a contention regression test that drives register_ice_ufrags and cleanup_inactive from multiple threads with a watchdog: if the inverse order is reintroduced the workers deadlock and the watchdog fails the test instead of hanging the suite. Verified it fails on the pre-fix order and passes after. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The TURN handlers bound a DashMap `Ref`/`get_by_client` guard and then awaited `socket.send_to(...)` while it was still alive (handle_send held both the sender's and each target's guard across every relay send). A DashMap guard is a shard RwLock guard; holding it across network I/O blocks every writer on that shard -- including the periodic allocation cleanup -- for the full duration of the send, and amplifies lock contention under load. Rework handle_binding_request, handle_client_response, handle_refresh, handle_create_permission, handle_channel_bind and handle_send to snapshot the owned data they need (client addresses, ids, permission/ufrag state, auth username), drop the guard, and only then await the sends. Relay fan-out now collects target client addresses first and sends guard-free; activity timers are updated under short-lived re-acquired guards with no await held. Behavior is preserved; the rare allocation-reaped-mid-request races resolve to the same error/skip paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
srperens
added a commit
that referenced
this pull request
May 27, 2026
Includes the AllocationTable ABBA-deadlock fix (#9) and the handler guard-across-await refactor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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.
Why
On 2026-05-27 the production server hung silently for ~1h40m (no crash, no panic, no log output) until a manual
docker restart. Root cause: an ABBA lock-order deadlock inAllocationTable.AllocationTablekeeps a primaryallocationsDashMap plus secondary index maps. Thecleanup_*paths hold anallocationsshard write lock (viaretain) and then mutate the secondary indices — orderallocations → secondary.register_ice_ufragsdid the reverse: it held aby_ice_ufragwrite lock (entry API) and then acquired anallocationsread lock viaget().On the multi-thread tokio runtime, a concurrent cleanup and register on colliding shards park both threads forever. Because packets are processed inline on the recv loop, the whole server then goes silent. It ran for ~5.5 weeks before the rare shard collision finally hit; the last log line before the freeze was
Removing inactive allocation— emitted from insidecleanup_inactive'sretainclosure, exactly where the cleanup side parks.What
Commit 1 —
fix(table)register_ice_ufragsto acquireallocationsfirst, matching the cleanup paths. The atomicby_ice_ufragdedup and exact return semantics are preserved.AllocationTablestruct (allocationsbefore any secondary index, never the reverse).register_ice_ufragsandcleanup_inactivefrom multiple threads. Validated: it deadlocks (and fails via the watchdog) on the pre-fix order, and passes after.Commit 2 —
refactor(handler)socket.send_to(...)while it was alive (handle_sendheld both the sender's and each target's guard across every relay send). A guard is a shard RwLock guard; holding it across network I/O blocks every writer on that shard — including the cleanup task — for the full send duration.handle_binding_request,handle_client_response,handle_refresh,handle_create_permission,handle_channel_bind, andhandle_sendto snapshot the owned data they need, drop the guard, then await. Behavior is preserved; the rare allocation-reaped-mid-request races resolve to the same error/skip paths.Verification
cargo clippy --all-targetsclean.cargo test— 36 passed (35 existing + new regression test).🤖 Generated with Claude Code