feat: add LeafLock to detect leaf-lock → tree-lock ordering violations (prototype) - #25171
Draft
totally-not-ai[bot] wants to merge 2 commits into
Draft
feat: add LeafLock to detect leaf-lock → tree-lock ordering violations (prototype)#25171totally-not-ai[bot] wants to merge 2 commits into
totally-not-ai[bot] wants to merge 2 commits into
Conversation
Contributor
totally-not-ai
Bot
force-pushed
the
prototype-leaf-lock-ordering-assertion
branch
from
August 11, 2026 13:17
6dd1ba6 to
1a7a33d
Compare
totally-not-ai
Bot
changed the base branch from
main
to
fix/combinedusage-lock-order-inversion
August 11, 2026 13:17
Base automatically changed from
fix/combinedusage-lock-order-inversion
to
main
August 12, 2026 08:42
…locks Rebased on top of the CombinedUsage fix and reduced to just the abstraction: the ordering fix and its deadlock regression test now live in that fix; this commit only adds the enforcement mechanism and migrates the already-fixed call site to it. All the shared-signal deadlocks share one shape: a component holds its own monitor while calling into a SignalTree (which takes the tree lock), while another thread holds the tree lock (notifyObservers invokes listeners under it) and then needs the component monitor. LeafLock is a component-level lock that records itself in a thread-local while held. SignalTree asserts (under -ea) that no LeafLock is held when it acquires its own lock, ignoring reentrant tree re-locks. The tree cannot enumerate arbitrary intrinsic monitors held by callers, so the leaf locks make themselves known instead. This turns a latent, timing-dependent inversion into a deterministic unit-test failure on any thread that violates the ordering. CombinedUsage (already reordered by the fix) is migrated from its Object monitor to a LeafLock as the first adopter, so the assertion guards it. Effect and CachedSignal are not migrated yet; that is the follow-up if this direction is accepted. LeafLockPrototypeTest demonstrates the assertion firing on a leaf-lock -> tree-lock violation.
totally-not-ai
Bot
force-pushed
the
prototype-leaf-lock-ordering-assertion
branch
from
August 12, 2026 10:13
1a7a33d to
f6d9424
Compare
|
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.



Prototype for discussion — see #25166.
Why
Every one of the recurring shared-signal deadlocks is the same ABBA pair:
SignalTree, which then takes the tree lock.notifyObserversinvokes listeners under it) and then needs that component monitor.These inversions are latent and timing-dependent, so they surface as rare CI flakes or production hangs rather than as test failures. A
SignalTreecannot enumerate the arbitrary intrinsic monitors its callers happen to hold, so it can't defend itself.What changed
LeafLock(new) — a component-level lock that records itself in a thread-local while held. Leaf locks make themselves known so the tree doesn't have to discover them.SignalTree— asserts (under-ea) that noLeafLockis held when it acquires its own lock, ignoring reentrant tree re-locks. A leaf-lock → tree-lock violation now fails deterministically on the offending thread instead of deadlocking later under load.UsageTracker.CombinedUsage— migrated toLeafLock, and its ordering fixed: it now registers the per-usage listeners and invokes the downstream listener without holding the leaf lock. This closes a real, reproduced ABBA (a registrar holding theCombinedUsagemonitor and wanting the tree lock, versus a committer holding the tree lock and wanting the monitor) that survives the #25166CachedSignalfix.The thread-local is cleaned up properly (Sonar S5164): no
withInitial(), so the read paths hit on every tree-lock acquisition never create a lingering entry; the deque is created lazily on the firstlock()and the entry is removed when the last leaf lock on the thread is released.Review notes
CombinedUsage.onChangenow invokes the downstream listener outside the leaf lock, so concurrent fires are no longer serialized by it. This matches how the downstream listeners (Effect,CachedSignal, nestedCombinedUsage) already tolerate concurrent/reentrant invocation — but it is the part worth a close look.EffectandCachedSignalare not migrated toLeafLockyet; that's the follow-up if this direction is accepted.Tests
CombinedUsagesurvives the register-vs-commit race.