From ac14634a375d312e8652716e5f2bdd92a14d6731 Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Tue, 4 Aug 2026 19:53:13 -0700 Subject: [PATCH 1/8] Resolve current stack of stuck lock holders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lock tracker records where a stuck lock was first acquired, but not what its holder is doing now — during an incident the waiters are easy to find in a goroutine dump while the blocked holder is anonymous. Record the holder's goroutine id on acquisition (~0.3ns via goid) and add PopulateHolderStacks to resolve holders' current stacks from a single runtime.Stack snapshot at scan time. Co-Authored-By: Claude Fable 5 --- .changeset/tough-locks-talk.md | 5 +++ go.mod | 1 + go.sum | 2 + utils/lock_tracker.go | 75 ++++++++++++++++++++++++++++++++-- utils/lock_tracker_test.go | 41 +++++++++++++++++++ 5 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 .changeset/tough-locks-talk.md diff --git a/.changeset/tough-locks-talk.md b/.changeset/tough-locks-talk.md new file mode 100644 index 000000000..65921b596 --- /dev/null +++ b/.changeset/tough-locks-talk.md @@ -0,0 +1,5 @@ +--- +"github.com/livekit/protocol": patch +--- + +Track the holder goroutine of stuck locks and resolve its current stack diff --git a/go.mod b/go.mod index 356244140..3d26c67aa 100644 --- a/go.mod +++ b/go.mod @@ -20,6 +20,7 @@ require ( github.com/mackerelio/go-osstat v0.2.8 github.com/maxbrunsfeld/counterfeiter/v6 v6.12.2 github.com/nyaruka/phonenumbers v1.8.1 + github.com/petermattis/goid v0.0.0-20260725062400-500c67a39b75 github.com/pion/logging v0.2.4 github.com/pion/sdp/v3 v3.0.19 github.com/pion/webrtc/v4 v4.2.17 diff --git a/go.sum b/go.sum index ee809f649..1597a5af8 100644 --- a/go.sum +++ b/go.sum @@ -121,6 +121,8 @@ github.com/opencontainers/image-spec v1.1.1 h1:y0fUlFfIZhPF1W537XOLg0/fcx6zcHCJw github.com/opencontainers/image-spec v1.1.1/go.mod h1:qpqAh3Dmcf36wStyyWU+kCeDgrGnAve2nCC8+7h8Q0M= github.com/ory/dockertest/v4 v4.0.0 h1:i19aFsO/VXE0VrMk4ifnKW4G/KIJ93PCjLOslxXoPME= github.com/ory/dockertest/v4 v4.0.0/go.mod h1:b5Ofu8VIxWNhXFvQcLu17pRNQdoUBKtXBW74G4Ygzx8= +github.com/petermattis/goid v0.0.0-20260725062400-500c67a39b75 h1:VmZ6mKVkxavKEhEy4ZYyV7BwBYBFBP0TwIqmLk84fpU= +github.com/petermattis/goid v0.0.0-20260725062400-500c67a39b75/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4= github.com/pion/datachannel v1.6.2 h1:7EXQ8TH3vTouBUdRWYbcX2edSx9Yj6k5zl5P+qyxEPc= github.com/pion/datachannel v1.6.2/go.mod h1:pzbdAZvyGtXbcHM1hBbsFaOTf40lZizU/dNlvVOak6E= github.com/pion/dtls/v3 v3.1.5 h1:9xJtVsHwMYeSjPp5Hh1FTis4DchnQWtnOa5o+6ygqfc= diff --git a/utils/lock_tracker.go b/utils/lock_tracker.go index 61b6b3c30..7fc1b23a5 100644 --- a/utils/lock_tracker.go +++ b/utils/lock_tracker.go @@ -24,6 +24,7 @@ import ( "time" "unsafe" + "github.com/petermattis/goid" "golang.org/x/exp/slices" ) @@ -116,6 +117,7 @@ func scanTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { ts: ts, waiting: waiting, held: atomic.LoadInt32(&t.held), + gid: atomic.LoadInt64(&t.gid), }) } } @@ -124,10 +126,12 @@ func scanTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { } type StuckLock struct { - stack []uintptr - ts uint32 - waiting int32 - held int32 + stack []uintptr + ts uint32 + waiting int32 + held int32 + gid int64 + holderStack string } func (d *StuckLock) FirstLockedAtStack() string { @@ -165,11 +169,73 @@ func (d *StuckLock) NumGoroutineWaiting() int { return int(d.waiting) } +// HolderGoroutineID returns the id of the goroutine that took the lock while +// it was free. For RWMutex this is the first reader or the writer. +func (d *StuckLock) HolderGoroutineID() int64 { + return d.gid +} + +// HolderStack returns the holder goroutine's stack as resolved by +// PopulateHolderStacks, or "" if it has not been resolved. +func (d *StuckLock) HolderStack() string { + return d.holderStack +} + +// PopulateHolderStacks resolves the current stack of each stuck lock's holder +// goroutine from a single snapshot of all goroutine stacks. The snapshot stops +// the world; call it once per detection, not per lock. +func PopulateHolderStacks(stuck []*StuckLock) { + byGID := make(map[int64][]*StuckLock, len(stuck)) + for _, d := range stuck { + if d.gid != 0 { + byGID[d.gid] = append(byGID[d.gid], d) + } + } + if len(byGID) == 0 { + return + } + + buf := make([]byte, 1<<20) + for { + n := runtime.Stack(buf, true) + if n < len(buf) || len(buf) >= 1<<26 { + buf = buf[:n] + break + } + buf = make([]byte, len(buf)*2) + } + + for _, g := range strings.Split(string(buf), "\n\n") { + if gid, ok := parseGoroutineHeader(g); ok { + for _, d := range byGID[gid] { + d.holderStack = g + } + } + } +} + +// parseGoroutineHeader extracts the goroutine id from a stack block formatted +// like "goroutine 123 [chan receive]:\n..." +func parseGoroutineHeader(g string) (int64, bool) { + const prefix = "goroutine " + if !strings.HasPrefix(g, prefix) { + return 0, false + } + rest := g[len(prefix):] + sp := strings.IndexByte(rest, ' ') + if sp <= 0 { + return 0, false + } + gid, err := strconv.ParseInt(rest[:sp], 10, 64) + return gid, err == nil +} + type lockTracker struct { stack []uintptr ts uint32 waiting int32 held int32 + gid int64 ref int } @@ -184,6 +250,7 @@ func (t *lockTracker) trackLock() { atomic.AddInt32(&t.waiting, -1) if atomic.AddInt32(&t.held, 1) == 1 { atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) + atomic.StoreInt64(&t.gid, goid.Get()) if atomic.LoadUint32(&enableLockTrackerStackTrace) == 1 { n := runtime.Callers(2, t.stack[:lockTrackerMaxStackDepth]) diff --git a/utils/lock_tracker_test.go b/utils/lock_tracker_test.go index 383c41edc..aec55e7a9 100644 --- a/utils/lock_tracker_test.go +++ b/utils/lock_tracker_test.go @@ -92,6 +92,47 @@ func TestFirstLockStackTrace(t *testing.T) { m.Unlock() } +func parkHoldingLock(release chan struct{}) { + <-release +} + +func TestHolderStack(t *testing.T) { + t.Cleanup(cleanupTest) + require.Nil(t, utils.ScanTrackedLocks(time.Millisecond)) + + m := &utils.Mutex{} + release := make(chan struct{}) + held := make(chan struct{}) + done := make(chan struct{}) + + go func() { + m.Lock() + close(held) + parkHoldingLock(release) + m.Unlock() + }() + + <-held + go func() { + m.Lock() + m.Unlock() + close(done) + }() + + time.Sleep(100 * time.Millisecond) + locks := utils.ScanTrackedLocks(time.Millisecond) + require.NotNil(t, locks) + require.NotZero(t, locks[0].HolderGoroutineID()) + require.Equal(t, "", locks[0].HolderStack()) + + utils.PopulateHolderStacks(locks) + require.Contains(t, locks[0].HolderStack(), "parkHoldingLock") + require.Contains(t, locks[0].HolderStack(), fmt.Sprintf("goroutine %d ", locks[0].HolderGoroutineID())) + + close(release) + <-done +} + func TestMutexFinalizer(t *testing.T) { cleanupTest() require.Equal(t, 0, utils.NumMutexes()) From 8b5583ae1dc8768a1ab5c2d6223dc27c9a06a6e7 Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Tue, 4 Aug 2026 19:56:15 -0700 Subject: [PATCH 2/8] avoid empty critical section in test (SA2001) Co-Authored-By: Claude Fable 5 --- utils/lock_tracker_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/utils/lock_tracker_test.go b/utils/lock_tracker_test.go index aec55e7a9..c1510b851 100644 --- a/utils/lock_tracker_test.go +++ b/utils/lock_tracker_test.go @@ -115,6 +115,7 @@ func TestHolderStack(t *testing.T) { <-held go func() { m.Lock() + noop() m.Unlock() close(done) }() From c53ceae1f4b05d6756b9e8d8cdcec7162a083c3c Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Tue, 4 Aug 2026 20:46:12 -0700 Subject: [PATCH 3/8] track all holder goroutines of stuck locks Track every holder gid (up to 8 concurrent RWMutex readers) in atomic slots instead of only the first, so PopulateHolderStacks resolves all holders' current stacks. Co-Authored-By: Claude Fable 5 --- .changeset/tough-locks-talk.md | 2 +- utils/lock_tracker.go | 74 +++++++++++++++++++++++----------- utils/lock_tracker_test.go | 34 +++++++++------- 3 files changed, 72 insertions(+), 38 deletions(-) diff --git a/.changeset/tough-locks-talk.md b/.changeset/tough-locks-talk.md index 65921b596..8ebacbba0 100644 --- a/.changeset/tough-locks-talk.md +++ b/.changeset/tough-locks-talk.md @@ -2,4 +2,4 @@ "github.com/livekit/protocol": patch --- -Track the holder goroutine of stuck locks and resolve its current stack +Track holder goroutines of stuck locks (all RWMutex readers up to 8) and resolve their current stacks diff --git a/utils/lock_tracker.go b/utils/lock_tracker.go index 7fc1b23a5..ba96eb680 100644 --- a/utils/lock_tracker.go +++ b/utils/lock_tracker.go @@ -28,7 +28,10 @@ import ( "golang.org/x/exp/slices" ) -const lockTrackerMaxStackDepth = 16 +const ( + lockTrackerMaxStackDepth = 16 + lockTrackerMaxHolders = 8 +) var lockTrackerEnabled = false var enableLockTrackerOnce sync.Once @@ -112,12 +115,18 @@ func scanTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { ts := atomic.LoadUint32(&t.ts) waiting := atomic.LoadInt32(&t.waiting) if ts <= minTS && waiting > 0 { + var gids []int64 + for i := range t.gids { + if gid := atomic.LoadInt64(&t.gids[i]); gid != 0 { + gids = append(gids, gid) + } + } stuck = append(stuck, &StuckLock{ stack: slices.Clone(t.stack), ts: ts, waiting: waiting, held: atomic.LoadInt32(&t.held), - gid: atomic.LoadInt64(&t.gid), + gids: gids, }) } } @@ -126,12 +135,12 @@ func scanTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { } type StuckLock struct { - stack []uintptr - ts uint32 - waiting int32 - held int32 - gid int64 - holderStack string + stack []uintptr + ts uint32 + waiting int32 + held int32 + gids []int64 + holderStacks []string } func (d *StuckLock) FirstLockedAtStack() string { @@ -169,26 +178,26 @@ func (d *StuckLock) NumGoroutineWaiting() int { return int(d.waiting) } -// HolderGoroutineID returns the id of the goroutine that took the lock while -// it was free. For RWMutex this is the first reader or the writer. -func (d *StuckLock) HolderGoroutineID() int64 { - return d.gid +// HolderGoroutineIDs returns the ids of the goroutines holding the lock, up +// to lockTrackerMaxHolders concurrent RWMutex readers. +func (d *StuckLock) HolderGoroutineIDs() []int64 { + return d.gids } -// HolderStack returns the holder goroutine's stack as resolved by -// PopulateHolderStacks, or "" if it has not been resolved. -func (d *StuckLock) HolderStack() string { - return d.holderStack +// HolderStacks returns the holder goroutines' stacks as resolved by +// PopulateHolderStacks, or "" if not resolved. +func (d *StuckLock) HolderStacks() string { + return strings.Join(d.holderStacks, "\n\n") } // PopulateHolderStacks resolves the current stack of each stuck lock's holder -// goroutine from a single snapshot of all goroutine stacks. The snapshot stops -// the world; call it once per detection, not per lock. +// goroutines from a single snapshot of all goroutine stacks. The snapshot +// stops the world; call it once per detection, not per lock. func PopulateHolderStacks(stuck []*StuckLock) { byGID := make(map[int64][]*StuckLock, len(stuck)) for _, d := range stuck { - if d.gid != 0 { - byGID[d.gid] = append(byGID[d.gid], d) + for _, gid := range d.gids { + byGID[gid] = append(byGID[gid], d) } } if len(byGID) == 0 { @@ -208,7 +217,7 @@ func PopulateHolderStacks(stuck []*StuckLock) { for _, g := range strings.Split(string(buf), "\n\n") { if gid, ok := parseGoroutineHeader(g); ok { for _, d := range byGID[gid] { - d.holderStack = g + d.holderStacks = append(d.holderStacks, g) } } } @@ -235,7 +244,7 @@ type lockTracker struct { ts uint32 waiting int32 held int32 - gid int64 + gids [lockTrackerMaxHolders]int64 ref int } @@ -248,9 +257,16 @@ func (t *lockTracker) trackWait() { func (t *lockTracker) trackLock() { if t != nil { atomic.AddInt32(&t.waiting, -1) + + gid := goid.Get() + for i := range t.gids { + if atomic.CompareAndSwapInt64(&t.gids[i], 0, gid) { + break + } + } + if atomic.AddInt32(&t.held, 1) == 1 { atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) - atomic.StoreInt64(&t.gid, goid.Get()) if atomic.LoadUint32(&enableLockTrackerStackTrace) == 1 { n := runtime.Callers(2, t.stack[:lockTrackerMaxStackDepth]) @@ -262,8 +278,20 @@ func (t *lockTracker) trackLock() { func (t *lockTracker) trackUnlock() { if t != nil { + gid := goid.Get() + for i := range t.gids { + if atomic.CompareAndSwapInt64(&t.gids[i], gid, 0) { + break + } + } + if atomic.AddInt32(&t.held, -1) == 0 { atomic.StoreUint32(&t.ts, math.MaxUint32) + // reclaim slots leaked by goroutines that unlock a mutex locked + // elsewhere; may briefly drop a concurrent new holder's slot + for i := range t.gids { + atomic.StoreInt64(&t.gids[i], 0) + } } } } diff --git a/utils/lock_tracker_test.go b/utils/lock_tracker_test.go index c1510b851..769311894 100644 --- a/utils/lock_tracker_test.go +++ b/utils/lock_tracker_test.go @@ -17,6 +17,7 @@ package utils_test import ( "fmt" "runtime" + "strings" "sync" "testing" "time" @@ -96,23 +97,26 @@ func parkHoldingLock(release chan struct{}) { <-release } -func TestHolderStack(t *testing.T) { +func TestHolderStacks(t *testing.T) { t.Cleanup(cleanupTest) require.Nil(t, utils.ScanTrackedLocks(time.Millisecond)) - m := &utils.Mutex{} + m := &utils.RWMutex{} release := make(chan struct{}) - held := make(chan struct{}) done := make(chan struct{}) - go func() { - m.Lock() - close(held) - parkHoldingLock(release) - m.Unlock() - }() + var held sync.WaitGroup + held.Add(2) + for range 2 { + go func() { + m.RLock() + held.Done() + parkHoldingLock(release) + m.RUnlock() + }() + } - <-held + held.Wait() go func() { m.Lock() noop() @@ -123,12 +127,14 @@ func TestHolderStack(t *testing.T) { time.Sleep(100 * time.Millisecond) locks := utils.ScanTrackedLocks(time.Millisecond) require.NotNil(t, locks) - require.NotZero(t, locks[0].HolderGoroutineID()) - require.Equal(t, "", locks[0].HolderStack()) + require.Len(t, locks[0].HolderGoroutineIDs(), 2) + require.Equal(t, "", locks[0].HolderStacks()) utils.PopulateHolderStacks(locks) - require.Contains(t, locks[0].HolderStack(), "parkHoldingLock") - require.Contains(t, locks[0].HolderStack(), fmt.Sprintf("goroutine %d ", locks[0].HolderGoroutineID())) + for _, gid := range locks[0].HolderGoroutineIDs() { + require.Contains(t, locks[0].HolderStacks(), fmt.Sprintf("goroutine %d ", gid)) + } + require.Equal(t, 2, strings.Count(locks[0].HolderStacks(), "parkHoldingLock(")) close(release) <-done From dc6c60a10f3ba5efe6932f28841b3e1222512009 Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Tue, 4 Aug 2026 21:39:48 -0700 Subject: [PATCH 4/8] harden holder-slot accounting for cross-goroutine unlock and overflow Every unlock now removes exactly one holder entry: its own gid if present, else an overflow credit, else an arbitrary slot (same policy go-deadlock uses for cross-goroutine unlocks). Replaces the racy wipe-all-slots-at-zero, making occupied slots + overflow == holders an exact invariant. Co-Authored-By: Claude Fable 5 --- utils/lock_tracker.go | 48 ++++++++++++---- utils/lock_tracker_test.go | 109 +++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 12 deletions(-) diff --git a/utils/lock_tracker.go b/utils/lock_tracker.go index ba96eb680..9bfc7e347 100644 --- a/utils/lock_tracker.go +++ b/utils/lock_tracker.go @@ -179,7 +179,9 @@ func (d *StuckLock) NumGoroutineWaiting() int { } // HolderGoroutineIDs returns the ids of the goroutines holding the lock, up -// to lockTrackerMaxHolders concurrent RWMutex readers. +// to lockTrackerMaxHolders concurrent RWMutex readers; compare with +// NumGoroutineHeld to detect overflow. A lock unlocked from a goroutine other +// than its locker may transiently misattribute one entry. func (d *StuckLock) HolderGoroutineIDs() []int64 { return d.gids } @@ -240,12 +242,13 @@ func parseGoroutineHeader(g string) (int64, bool) { } type lockTracker struct { - stack []uintptr - ts uint32 - waiting int32 - held int32 - gids [lockTrackerMaxHolders]int64 - ref int + stack []uintptr + ts uint32 + waiting int32 + held int32 + overflow int32 + gids [lockTrackerMaxHolders]int64 + ref int } func (t *lockTracker) trackWait() { @@ -254,16 +257,28 @@ func (t *lockTracker) trackWait() { } } +// Holder accounting: trackLock records the holder's gid in a free slot, or +// increments overflow when all slots are taken. trackUnlock removes exactly +// one entry: the caller's own gid if present, else an overflow credit, else +// an arbitrary slot — Go permits unlocking from a goroutine other than the +// locker, so like go-deadlock we keep the count consistent at the price of a +// possibly misattributed entry in that rare case. Invariant: occupied slots +// plus overflow equals the number of current holders. func (t *lockTracker) trackLock() { if t != nil { atomic.AddInt32(&t.waiting, -1) gid := goid.Get() + claimed := false for i := range t.gids { if atomic.CompareAndSwapInt64(&t.gids[i], 0, gid) { + claimed = true break } } + if !claimed { + atomic.AddInt32(&t.overflow, 1) + } if atomic.AddInt32(&t.held, 1) == 1 { atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) @@ -279,19 +294,28 @@ func (t *lockTracker) trackLock() { func (t *lockTracker) trackUnlock() { if t != nil { gid := goid.Get() + removed := false for i := range t.gids { if atomic.CompareAndSwapInt64(&t.gids[i], gid, 0) { + removed = true break } } + for !removed { + if o := atomic.LoadInt32(&t.overflow); o > 0 { + removed = atomic.CompareAndSwapInt32(&t.overflow, o, o-1) + continue + } + removed = true + for i := range t.gids { + if g := atomic.LoadInt64(&t.gids[i]); g != 0 && atomic.CompareAndSwapInt64(&t.gids[i], g, 0) { + break + } + } + } if atomic.AddInt32(&t.held, -1) == 0 { atomic.StoreUint32(&t.ts, math.MaxUint32) - // reclaim slots leaked by goroutines that unlock a mutex locked - // elsewhere; may briefly drop a concurrent new holder's slot - for i := range t.gids { - atomic.StoreInt64(&t.gids[i], 0) - } } } } diff --git a/utils/lock_tracker_test.go b/utils/lock_tracker_test.go index 769311894..b35de859f 100644 --- a/utils/lock_tracker_test.go +++ b/utils/lock_tracker_test.go @@ -140,6 +140,115 @@ func TestHolderStacks(t *testing.T) { <-done } +func TestHolderCrossGoroutineUnlock(t *testing.T) { + t.Cleanup(cleanupTest) + require.Nil(t, utils.ScanTrackedLocks(time.Millisecond)) + + m := &utils.Mutex{} + locked := make(chan struct{}) + go func() { + m.Lock() + close(locked) + }() + <-locked + m.Unlock() // legal: different goroutine than the locker + + release := make(chan struct{}) + held := make(chan struct{}) + done := make(chan struct{}) + go func() { + m.Lock() + close(held) + parkHoldingLock(release) + m.Unlock() + }() + <-held + go func() { + m.Lock() + noop() + m.Unlock() + close(done) + }() + + time.Sleep(100 * time.Millisecond) + locks := utils.ScanTrackedLocks(time.Millisecond) + require.NotNil(t, locks) + // the cross-goroutine unlock must not leak the original locker's slot + require.Len(t, locks[0].HolderGoroutineIDs(), 1) + utils.PopulateHolderStacks(locks) + require.Contains(t, locks[0].HolderStacks(), "parkHoldingLock(") + + close(release) + <-done +} + +func TestHolderOverflow(t *testing.T) { + t.Cleanup(cleanupTest) + require.Nil(t, utils.ScanTrackedLocks(time.Millisecond)) + + const readers = 10 // more than the 8 holder slots + + m := &utils.RWMutex{} + release := make(chan struct{}) + done := make(chan struct{}) + + var held, finished sync.WaitGroup + held.Add(readers) + finished.Add(readers) + for range readers { + go func() { + m.RLock() + held.Done() + parkHoldingLock(release) + m.RUnlock() + finished.Done() + }() + } + + held.Wait() + go func() { + m.Lock() + noop() + m.Unlock() + close(done) + }() + + time.Sleep(100 * time.Millisecond) + locks := utils.ScanTrackedLocks(time.Millisecond) + require.NotNil(t, locks) + require.Len(t, locks[0].HolderGoroutineIDs(), 8) + require.Equal(t, readers, locks[0].NumGoroutineHeld()) + + close(release) + finished.Wait() + <-done + + // slot and overflow accounting must drain fully for the next episode + release2 := make(chan struct{}) + held2 := make(chan struct{}) + done2 := make(chan struct{}) + go func() { + m.RLock() + close(held2) + parkHoldingLock(release2) + m.RUnlock() + }() + <-held2 + go func() { + m.Lock() + noop() + m.Unlock() + close(done2) + }() + time.Sleep(100 * time.Millisecond) + locks = utils.ScanTrackedLocks(time.Millisecond) + require.NotNil(t, locks) + require.Len(t, locks[0].HolderGoroutineIDs(), 1) + + close(release2) + <-done2 +} + func TestMutexFinalizer(t *testing.T) { cleanupTest() require.Equal(t, 0, utils.NumMutexes()) From fb4c9ab696a682edb117c035f0384eb68683262e Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Wed, 5 Aug 2026 13:06:19 -0700 Subject: [PATCH 5/8] split single-holder tracking from the RW reader extension MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mutex has exactly one holder and trackLock/trackUnlock run while it is held, so its bookkeeping needs no atomics — a plain gid store replaces the slot claim, and cross-goroutine unlock just clears it. The slot array, overflow accounting, and eviction heuristics now exist only for RWMutex readers in lock_tracker_rw.go, where holders are genuinely concurrent. Wrapped Mutex Lock/Unlock: 8.7ns -> 6.1ns (pre-tracking baseline 6.2ns, native 4.0ns). Co-Authored-By: Claude Fable 5 --- utils/lock_tracker.go | 234 ++++++++++++++----------------------- utils/lock_tracker_rw.go | 192 ++++++++++++++++++++++++++++++ utils/lock_tracker_test.go | 23 ++++ 3 files changed, 304 insertions(+), 145 deletions(-) create mode 100644 utils/lock_tracker_rw.go diff --git a/utils/lock_tracker.go b/utils/lock_tracker.go index 9bfc7e347..00475f4c6 100644 --- a/utils/lock_tracker.go +++ b/utils/lock_tracker.go @@ -28,10 +28,7 @@ import ( "golang.org/x/exp/slices" ) -const ( - lockTrackerMaxStackDepth = 16 - lockTrackerMaxHolders = 8 -) +const lockTrackerMaxStackDepth = 16 var lockTrackerEnabled = false var enableLockTrackerOnce sync.Once @@ -62,14 +59,45 @@ func updateLowResTime() { } } -var weakRefs []uintptr -var weakRefFree []int +// weakRefList is a registry of tracker pointers held as uintptrs so they +// don't keep their owners alive; finalizers clear the slots. All lists share +// weakRefLock. +type weakRefList struct { + refs []uintptr + free []int +} + var weakRefLock sync.Mutex +var mutexRefs, rwRefs weakRefList + +func (l *weakRefList) add(p unsafe.Pointer) int { + weakRefLock.Lock() + defer weakRefLock.Unlock() + if fi := len(l.free) - 1; fi >= 0 { + ref := l.free[fi] + l.refs[ref] = uintptr(p) + l.free = l.free[:fi] + return ref + } + l.refs = append(l.refs, uintptr(p)) + return len(l.refs) - 1 +} + +func (l *weakRefList) remove(ref int) { + weakRefLock.Lock() + defer weakRefLock.Unlock() + l.refs[ref] = 0 + l.free = append(l.free, ref) +} + +func (l *weakRefList) count() int { + return len(l.refs) - len(l.free) +} func NumMutexes() int { weakRefLock.Lock() defer weakRefLock.Unlock() - return len(weakRefs) - len(weakRefFree) + return mutexRefs.count() + rwRefs.count() } // ScanTrackedLocks check all lock trackers @@ -78,10 +106,11 @@ func ScanTrackedLocks(threshold time.Duration) []*StuckLock { weakRefLock.Lock() defer weakRefLock.Unlock() - return scanTrackedLocks(weakRefs, minTS) + return append(scanTrackedLocks(mutexRefs.refs, minTS), scanRWTrackedLocks(rwRefs.refs, minTS)...) } var nextScanMin int +var nextScanMinRW int // ScanTrackedLocksI check lock trackers incrementally n at a time func ScanTrackedLocksI(threshold time.Duration, n int) []*StuckLock { @@ -93,16 +122,22 @@ func ScanTrackedLocksI(threshold time.Duration, n int) []*StuckLock { weakRefLock.Lock() defer weakRefLock.Unlock() - min := nextScanMin - max := nextScanMin + n - if rl := len(weakRefs); rl <= max { - max = rl - nextScanMin = 0 - } else { - nextScanMin = max + window := func(size int, next *int) (int, int) { + min := *next + max := min + n + if size <= max { + max = size + *next = 0 + } else { + *next = max + } + return min, max } - return scanTrackedLocks(weakRefs[min:max], minTS) + min, max := window(len(mutexRefs.refs), &nextScanMin) + stuck := scanTrackedLocks(mutexRefs.refs[min:max], minTS) + min, max = window(len(rwRefs.refs), &nextScanMinRW) + return append(stuck, scanRWTrackedLocks(rwRefs.refs[min:max], minTS)...) } //go:norace @@ -116,16 +151,16 @@ func scanTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { waiting := atomic.LoadInt32(&t.waiting) if ts <= minTS && waiting > 0 { var gids []int64 - for i := range t.gids { - if gid := atomic.LoadInt64(&t.gids[i]); gid != 0 { - gids = append(gids, gid) - } + var held int32 + if gid := t.gid; gid != 0 { + gids = []int64{gid} + held = 1 } stuck = append(stuck, &StuckLock{ stack: slices.Clone(t.stack), ts: ts, waiting: waiting, - held: atomic.LoadInt32(&t.held), + held: held, gids: gids, }) } @@ -178,10 +213,9 @@ func (d *StuckLock) NumGoroutineWaiting() int { return int(d.waiting) } -// HolderGoroutineIDs returns the ids of the goroutines holding the lock, up -// to lockTrackerMaxHolders concurrent RWMutex readers; compare with -// NumGoroutineHeld to detect overflow. A lock unlocked from a goroutine other -// than its locker may transiently misattribute one entry. +// HolderGoroutineIDs returns the ids of the goroutines holding the lock: the +// single holder for a Mutex, the writer or up to 8 concurrent readers for an +// RWMutex (compare with NumGoroutineHeld to detect overflow). func (d *StuckLock) HolderGoroutineIDs() []int64 { return d.gids } @@ -241,14 +275,16 @@ func parseGoroutineHeader(g string) (int64, bool) { return gid, err == nil } +// lockTracker tracks an exclusive lock. trackLock and trackUnlock run while +// the lock is held, so the single holder's bookkeeping needs no atomics; +// only the waiter count, maintained outside the lock, does. The scanner +// reads holder state without the lock and tolerates racy reads (go:norace). type lockTracker struct { - stack []uintptr - ts uint32 - waiting int32 - held int32 - overflow int32 - gids [lockTrackerMaxHolders]int64 - ref int + stack []uintptr + ts uint32 + waiting int32 + gid int64 + ref int } func (t *lockTracker) trackWait() { @@ -257,66 +293,23 @@ func (t *lockTracker) trackWait() { } } -// Holder accounting: trackLock records the holder's gid in a free slot, or -// increments overflow when all slots are taken. trackUnlock removes exactly -// one entry: the caller's own gid if present, else an overflow credit, else -// an arbitrary slot — Go permits unlocking from a goroutine other than the -// locker, so like go-deadlock we keep the count consistent at the price of a -// possibly misattributed entry in that rare case. Invariant: occupied slots -// plus overflow equals the number of current holders. func (t *lockTracker) trackLock() { if t != nil { atomic.AddInt32(&t.waiting, -1) + t.gid = goid.Get() + atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) - gid := goid.Get() - claimed := false - for i := range t.gids { - if atomic.CompareAndSwapInt64(&t.gids[i], 0, gid) { - claimed = true - break - } - } - if !claimed { - atomic.AddInt32(&t.overflow, 1) - } - - if atomic.AddInt32(&t.held, 1) == 1 { - atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) - - if atomic.LoadUint32(&enableLockTrackerStackTrace) == 1 { - n := runtime.Callers(2, t.stack[:lockTrackerMaxStackDepth]) - t.stack = t.stack[:n] - } + if atomic.LoadUint32(&enableLockTrackerStackTrace) == 1 { + n := runtime.Callers(2, t.stack[:lockTrackerMaxStackDepth]) + t.stack = t.stack[:n] } } } func (t *lockTracker) trackUnlock() { if t != nil { - gid := goid.Get() - removed := false - for i := range t.gids { - if atomic.CompareAndSwapInt64(&t.gids[i], gid, 0) { - removed = true - break - } - } - for !removed { - if o := atomic.LoadInt32(&t.overflow); o > 0 { - removed = atomic.CompareAndSwapInt32(&t.overflow, o, o-1) - continue - } - removed = true - for i := range t.gids { - if g := atomic.LoadInt64(&t.gids[i]); g != 0 && atomic.CompareAndSwapInt64(&t.gids[i], g, 0) { - break - } - } - } - - if atomic.AddInt32(&t.held, -1) == 0 { - atomic.StoreUint32(&t.ts, math.MaxUint32) - } + t.gid = 0 + atomic.StoreUint32(&t.ts, math.MaxUint32) } } @@ -325,41 +318,23 @@ func newLockTracker() *lockTracker { stack: make([]uintptr, lockTrackerMaxStackDepth), ts: math.MaxUint32, } - - runtime.SetFinalizer(t, finalizeLockTracker) - - weakRefLock.Lock() - defer weakRefLock.Unlock() - - if fi := len(weakRefFree) - 1; fi >= 0 { - t.ref = weakRefFree[fi] - weakRefs[t.ref] = uintptr((unsafe.Pointer)(t)) - weakRefFree = weakRefFree[:fi] - } else { - t.ref = len(weakRefs) - weakRefs = append(weakRefs, uintptr((unsafe.Pointer)(t))) - } - + t.ref = mutexRefs.add(unsafe.Pointer(t)) + runtime.SetFinalizer(t, func(t *lockTracker) { + mutexRefs.remove(t.ref) + }) return t } -func finalizeLockTracker(t *lockTracker) { - weakRefLock.Lock() - defer weakRefLock.Unlock() - - weakRefs[t.ref] = 0 - weakRefFree = append(weakRefFree, t.ref) -} - -var waiting lockTracker - //go:linkname sync_runtime_canSpin sync.runtime_canSpin func sync_runtime_canSpin(int) bool //go:linkname sync_runtime_doSpin sync.runtime_doSpin func sync_runtime_doSpin() -func lazyInitLockTracker(p **lockTracker) *lockTracker { +// trackerInitSentinel marks a tracker pointer field as mid-initialization. +var trackerInitSentinel = unsafe.Pointer(new(int)) + +func lazyInitTracker[T any](p **T, construct func() *T) *T { if !lockTrackerEnabled { return nil } @@ -367,10 +342,10 @@ func lazyInitLockTracker(p **lockTracker) *lockTracker { iter := 0 for { if t := atomic.LoadPointer(up); t == nil { - if atomic.CompareAndSwapPointer(up, nil, (unsafe.Pointer)(&waiting)) { - atomic.StorePointer(up, (unsafe.Pointer)(newLockTracker())) + if atomic.CompareAndSwapPointer(up, nil, trackerInitSentinel) { + atomic.StorePointer(up, unsafe.Pointer(construct())) } - } else if t == (unsafe.Pointer)(&waiting) { + } else if t == trackerInitSentinel { if sync_runtime_canSpin(iter) { sync_runtime_doSpin() iter++ @@ -378,7 +353,7 @@ func lazyInitLockTracker(p **lockTracker) *lockTracker { runtime.Gosched() } } else { - return (*lockTracker)(t) + return (*T)(t) } } } @@ -389,45 +364,14 @@ type Mutex struct { } func (m *Mutex) Lock() { - t := lazyInitLockTracker(&m.t) + t := lazyInitTracker(&m.t, newLockTracker) t.trackWait() m.Mutex.Lock() t.trackLock() } func (m *Mutex) Unlock() { - t := lazyInitLockTracker(&m.t) + t := lazyInitTracker(&m.t, newLockTracker) t.trackUnlock() m.Mutex.Unlock() } - -type RWMutex struct { - sync.RWMutex - t *lockTracker -} - -func (m *RWMutex) Lock() { - t := lazyInitLockTracker(&m.t) - t.trackWait() - m.RWMutex.Lock() - t.trackLock() -} - -func (m *RWMutex) Unlock() { - t := lazyInitLockTracker(&m.t) - t.trackUnlock() - m.RWMutex.Unlock() -} - -func (m *RWMutex) RLock() { - t := lazyInitLockTracker(&m.t) - t.trackWait() - m.RWMutex.RLock() - t.trackLock() -} - -func (m *RWMutex) RUnlock() { - t := lazyInitLockTracker(&m.t) - t.trackUnlock() - m.RWMutex.RUnlock() -} diff --git a/utils/lock_tracker_rw.go b/utils/lock_tracker_rw.go new file mode 100644 index 000000000..8d4dce29e --- /dev/null +++ b/utils/lock_tracker_rw.go @@ -0,0 +1,192 @@ +// Copyright 2026 LiveKit, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package utils + +import ( + "math" + "runtime" + "sync" + "sync/atomic" + "unsafe" + + "github.com/petermattis/goid" + "golang.org/x/exp/slices" +) + +const lockTrackerMaxReadHolders = 8 + +// rwLockTracker extends lockTracker with read-holder tracking. The write +// holder uses the embedded single-holder bookkeeping (writes are exclusive); +// readers are concurrent, so their gids live in atomic slots. +// +// Read-holder accounting: trackRLock records the reader's gid in a free +// slot, or increments roverflow when all slots are taken. trackRUnlock +// removes exactly one entry: the caller's own gid if present, else an +// overflow credit, else an arbitrary slot — Go permits unlocking from a +// goroutine other than the locker, so like go-deadlock we keep the count +// consistent at the price of a possibly misattributed entry in that rare +// case. Invariant: occupied slots plus roverflow equals the reader count. +type rwLockTracker struct { + lockTracker + rheld int32 + roverflow int32 + rgids [lockTrackerMaxReadHolders]int64 +} + +// base returns the embedded exclusive tracker, tolerating nil like the +// lockTracker methods do. +func (t *rwLockTracker) base() *lockTracker { + if t == nil { + return nil + } + return &t.lockTracker +} + +func (t *rwLockTracker) trackRLock() { + if t == nil { + return + } + atomic.AddInt32(&t.waiting, -1) + + gid := goid.Get() + claimed := false + for i := range t.rgids { + if atomic.CompareAndSwapInt64(&t.rgids[i], 0, gid) { + claimed = true + break + } + } + if !claimed { + atomic.AddInt32(&t.roverflow, 1) + } + + if atomic.AddInt32(&t.rheld, 1) == 1 { + atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) + + if atomic.LoadUint32(&enableLockTrackerStackTrace) == 1 { + n := runtime.Callers(2, t.stack[:lockTrackerMaxStackDepth]) + t.stack = t.stack[:n] + } + } +} + +func (t *rwLockTracker) trackRUnlock() { + if t == nil { + return + } + gid := goid.Get() + removed := false + for i := range t.rgids { + if atomic.CompareAndSwapInt64(&t.rgids[i], gid, 0) { + removed = true + break + } + } + for !removed { + if o := atomic.LoadInt32(&t.roverflow); o > 0 { + removed = atomic.CompareAndSwapInt32(&t.roverflow, o, o-1) + continue + } + removed = true + for i := range t.rgids { + if g := atomic.LoadInt64(&t.rgids[i]); g != 0 && atomic.CompareAndSwapInt64(&t.rgids[i], g, 0) { + break + } + } + } + + if atomic.AddInt32(&t.rheld, -1) == 0 { + atomic.StoreUint32(&t.ts, math.MaxUint32) + } +} + +func newRWLockTracker() *rwLockTracker { + t := &rwLockTracker{ + lockTracker: lockTracker{ + stack: make([]uintptr, lockTrackerMaxStackDepth), + ts: math.MaxUint32, + }, + } + t.ref = rwRefs.add(unsafe.Pointer(t)) + runtime.SetFinalizer(t, func(t *rwLockTracker) { + rwRefs.remove(t.ref) + }) + return t +} + +//go:norace +//go:nosplit +func scanRWTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { + var stuck []*StuckLock + for _, ref := range refs { + if ref != 0 { + t := (*rwLockTracker)(unsafe.Pointer(ref)) + ts := atomic.LoadUint32(&t.ts) + waiting := atomic.LoadInt32(&t.waiting) + if ts <= minTS && waiting > 0 { + var gids []int64 + held := atomic.LoadInt32(&t.rheld) + if gid := t.gid; gid != 0 { + gids = append(gids, gid) + held++ + } + for i := range t.rgids { + if gid := atomic.LoadInt64(&t.rgids[i]); gid != 0 { + gids = append(gids, gid) + } + } + stuck = append(stuck, &StuckLock{ + stack: slices.Clone(t.stack), + ts: ts, + waiting: waiting, + held: held, + gids: gids, + }) + } + } + } + return stuck +} + +type RWMutex struct { + sync.RWMutex + t *rwLockTracker +} + +func (m *RWMutex) Lock() { + t := lazyInitTracker(&m.t, newRWLockTracker) + t.base().trackWait() + m.RWMutex.Lock() + t.base().trackLock() +} + +func (m *RWMutex) Unlock() { + t := lazyInitTracker(&m.t, newRWLockTracker) + t.base().trackUnlock() + m.RWMutex.Unlock() +} + +func (m *RWMutex) RLock() { + t := lazyInitTracker(&m.t, newRWLockTracker) + t.base().trackWait() + m.RWMutex.RLock() + t.trackRLock() +} + +func (m *RWMutex) RUnlock() { + t := lazyInitTracker(&m.t, newRWLockTracker) + t.trackRUnlock() + m.RWMutex.RUnlock() +} diff --git a/utils/lock_tracker_test.go b/utils/lock_tracker_test.go index b35de859f..d5b153031 100644 --- a/utils/lock_tracker_test.go +++ b/utils/lock_tracker_test.go @@ -458,6 +458,29 @@ func BenchmarkLockTracker(b *testing.B) { }) } +func BenchmarkLockTrackerParallel(b *testing.B) { + b.Run("wrapped rwmutex rlock", func(b *testing.B) { + var m utils.RWMutex + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + m.RLock() + noop() + m.RUnlock() + } + }) + }) + b.Run("native rwmutex rlock", func(b *testing.B) { + var m sync.RWMutex + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + m.RLock() + noop() + m.RUnlock() + } + }) + }) +} + func BenchmarkGetBlocked(b *testing.B) { for n := 100; n <= 1000000; n *= 100 { n := n From c00de3efdf94962416eee089c07efcad26d25aac Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Wed, 5 Aug 2026 13:11:23 -0700 Subject: [PATCH 6/8] hoist tracker nil checks to call sites and report holder strength Lock paths guard tracking behind one nil check with the post-acquire bookkeeping deferred; unlock paths use a bare atomic load (loadTracker) instead of the full lazy-init, since a lock being unlocked was locked first. StuckLock gains HolderStrength (exclusive vs shared readers). Co-Authored-By: Claude Fable 5 --- utils/lock_tracker.go | 81 ++++++++++++++++++++++++++------------ utils/lock_tracker_rw.go | 50 +++++++++++------------ utils/lock_tracker_test.go | 2 + 3 files changed, 80 insertions(+), 53 deletions(-) diff --git a/utils/lock_tracker.go b/utils/lock_tracker.go index 00475f4c6..31c9b8dfb 100644 --- a/utils/lock_tracker.go +++ b/utils/lock_tracker.go @@ -169,13 +169,29 @@ func scanTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { return stuck } +// HolderStrength describes how a stuck lock is held. +type HolderStrength int32 + +const ( + HolderExclusive HolderStrength = iota // Mutex, RWMutex write lock, Synchronized + HolderShared // RWMutex read locks +) + +func (s HolderStrength) String() string { + if s == HolderShared { + return "shared" + } + return "exclusive" +} + type StuckLock struct { - stack []uintptr - ts uint32 - waiting int32 - held int32 - gids []int64 - holderStacks []string + stack []uintptr + ts uint32 + waiting int32 + held int32 + holderStrength HolderStrength + gids []int64 + holderStacks []string } func (d *StuckLock) FirstLockedAtStack() string { @@ -220,6 +236,12 @@ func (d *StuckLock) HolderGoroutineIDs() []int64 { return d.gids } +// HolderStrength reports whether the lock is held exclusively or shared by +// readers. +func (d *StuckLock) HolderStrength() HolderStrength { + return d.holderStrength +} + // HolderStacks returns the holder goroutines' stacks as resolved by // PopulateHolderStacks, or "" if not resolved. func (d *StuckLock) HolderStacks() string { @@ -288,29 +310,23 @@ type lockTracker struct { } func (t *lockTracker) trackWait() { - if t != nil { - atomic.AddInt32(&t.waiting, 1) - } + atomic.AddInt32(&t.waiting, 1) } func (t *lockTracker) trackLock() { - if t != nil { - atomic.AddInt32(&t.waiting, -1) - t.gid = goid.Get() - atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) + atomic.AddInt32(&t.waiting, -1) + t.gid = goid.Get() + atomic.StoreUint32(&t.ts, atomic.LoadUint32(&lowResTime)) - if atomic.LoadUint32(&enableLockTrackerStackTrace) == 1 { - n := runtime.Callers(2, t.stack[:lockTrackerMaxStackDepth]) - t.stack = t.stack[:n] - } + if atomic.LoadUint32(&enableLockTrackerStackTrace) == 1 { + n := runtime.Callers(2, t.stack[:lockTrackerMaxStackDepth]) + t.stack = t.stack[:n] } } func (t *lockTracker) trackUnlock() { - if t != nil { - t.gid = 0 - atomic.StoreUint32(&t.ts, math.MaxUint32) - } + t.gid = 0 + atomic.StoreUint32(&t.ts, math.MaxUint32) } func newLockTracker() *lockTracker { @@ -334,6 +350,18 @@ func sync_runtime_doSpin() // trackerInitSentinel marks a tracker pointer field as mid-initialization. var trackerInitSentinel = unsafe.Pointer(new(int)) +// loadTracker returns the tracker if its initialization completed, else nil. +// Unlock paths use it instead of lazyInitTracker: a lock being unlocked was +// necessarily locked first, so the tracker either exists or tracking was +// disabled at Lock time. +func loadTracker[T any](p **T) *T { + t := atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(p))) + if t == trackerInitSentinel { + return nil + } + return (*T)(t) +} + func lazyInitTracker[T any](p **T, construct func() *T) *T { if !lockTrackerEnabled { return nil @@ -365,13 +393,16 @@ type Mutex struct { func (m *Mutex) Lock() { t := lazyInitTracker(&m.t, newLockTracker) - t.trackWait() + if t != nil { + t.trackWait() + defer t.trackLock() + } m.Mutex.Lock() - t.trackLock() } func (m *Mutex) Unlock() { - t := lazyInitTracker(&m.t, newLockTracker) - t.trackUnlock() + if t := loadTracker(&m.t); t != nil { + t.trackUnlock() + } m.Mutex.Unlock() } diff --git a/utils/lock_tracker_rw.go b/utils/lock_tracker_rw.go index 8d4dce29e..6d831e668 100644 --- a/utils/lock_tracker_rw.go +++ b/utils/lock_tracker_rw.go @@ -45,19 +45,7 @@ type rwLockTracker struct { rgids [lockTrackerMaxReadHolders]int64 } -// base returns the embedded exclusive tracker, tolerating nil like the -// lockTracker methods do. -func (t *rwLockTracker) base() *lockTracker { - if t == nil { - return nil - } - return &t.lockTracker -} - func (t *rwLockTracker) trackRLock() { - if t == nil { - return - } atomic.AddInt32(&t.waiting, -1) gid := goid.Get() @@ -83,9 +71,6 @@ func (t *rwLockTracker) trackRLock() { } func (t *rwLockTracker) trackRUnlock() { - if t == nil { - return - } gid := goid.Get() removed := false for i := range t.rgids { @@ -137,10 +122,12 @@ func scanRWTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { waiting := atomic.LoadInt32(&t.waiting) if ts <= minTS && waiting > 0 { var gids []int64 + strength := HolderShared held := atomic.LoadInt32(&t.rheld) if gid := t.gid; gid != 0 { gids = append(gids, gid) held++ + strength = HolderExclusive } for i := range t.rgids { if gid := atomic.LoadInt64(&t.rgids[i]); gid != 0 { @@ -148,11 +135,12 @@ func scanRWTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { } } stuck = append(stuck, &StuckLock{ - stack: slices.Clone(t.stack), - ts: ts, - waiting: waiting, - held: held, - gids: gids, + stack: slices.Clone(t.stack), + ts: ts, + waiting: waiting, + held: held, + holderStrength: strength, + gids: gids, }) } } @@ -167,26 +155,32 @@ type RWMutex struct { func (m *RWMutex) Lock() { t := lazyInitTracker(&m.t, newRWLockTracker) - t.base().trackWait() + if t != nil { + t.trackWait() + defer t.trackLock() + } m.RWMutex.Lock() - t.base().trackLock() } func (m *RWMutex) Unlock() { - t := lazyInitTracker(&m.t, newRWLockTracker) - t.base().trackUnlock() + if t := loadTracker(&m.t); t != nil { + t.trackUnlock() + } m.RWMutex.Unlock() } func (m *RWMutex) RLock() { t := lazyInitTracker(&m.t, newRWLockTracker) - t.base().trackWait() + if t != nil { + t.trackWait() + defer t.trackRLock() + } m.RWMutex.RLock() - t.trackRLock() } func (m *RWMutex) RUnlock() { - t := lazyInitTracker(&m.t, newRWLockTracker) - t.trackRUnlock() + if t := loadTracker(&m.t); t != nil { + t.trackRUnlock() + } m.RWMutex.RUnlock() } diff --git a/utils/lock_tracker_test.go b/utils/lock_tracker_test.go index d5b153031..79a2f5089 100644 --- a/utils/lock_tracker_test.go +++ b/utils/lock_tracker_test.go @@ -128,6 +128,7 @@ func TestHolderStacks(t *testing.T) { locks := utils.ScanTrackedLocks(time.Millisecond) require.NotNil(t, locks) require.Len(t, locks[0].HolderGoroutineIDs(), 2) + require.Equal(t, utils.HolderShared, locks[0].HolderStrength()) require.Equal(t, "", locks[0].HolderStacks()) utils.PopulateHolderStacks(locks) @@ -175,6 +176,7 @@ func TestHolderCrossGoroutineUnlock(t *testing.T) { require.NotNil(t, locks) // the cross-goroutine unlock must not leak the original locker's slot require.Len(t, locks[0].HolderGoroutineIDs(), 1) + require.Equal(t, utils.HolderExclusive, locks[0].HolderStrength()) utils.PopulateHolderStacks(locks) require.Contains(t, locks[0].HolderStacks(), "parkHoldingLock(") From 7989e0ae61ffb922068650d73e79f16b7dc8e060 Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Wed, 5 Aug 2026 13:30:38 -0700 Subject: [PATCH 7/8] extract window helper for incremental scans Co-Authored-By: Claude Fable 5 --- utils/lock_tracker.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/utils/lock_tracker.go b/utils/lock_tracker.go index 31c9b8dfb..d33fd3af4 100644 --- a/utils/lock_tracker.go +++ b/utils/lock_tracker.go @@ -122,22 +122,22 @@ func ScanTrackedLocksI(threshold time.Duration, n int) []*StuckLock { weakRefLock.Lock() defer weakRefLock.Unlock() - window := func(size int, next *int) (int, int) { - min := *next - max := min + n - if size <= max { - max = size - *next = 0 - } else { - *next = max - } - return min, max + stuck := scanTrackedLocks(window(mutexRefs.refs, &nextScanMin, n), minTS) + return append(stuck, scanRWTrackedLocks(window(rwRefs.refs, &nextScanMinRW, n), minTS)...) +} + +// window returns the next n-element window of refs, advancing next and +// wrapping to the start when the end is reached. +func window(refs []uintptr, next *int, n int) []uintptr { + min := *next + max := min + n + if len(refs) <= max { + max = len(refs) + *next = 0 + } else { + *next = max } - - min, max := window(len(mutexRefs.refs), &nextScanMin) - stuck := scanTrackedLocks(mutexRefs.refs[min:max], minTS) - min, max = window(len(rwRefs.refs), &nextScanMinRW) - return append(stuck, scanRWTrackedLocks(rwRefs.refs[min:max], minTS)...) + return refs[min:max] } //go:norace From 1414a20a59cc8ed46da2fc7653112c1b46275ed1 Mon Sep 17 00:00:00 2001 From: Erik Hortsch Date: Wed, 5 Aug 2026 13:46:17 -0700 Subject: [PATCH 8/8] single tracker registry with side-effect-free construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lazyInitTracker now CASes the constructed tracker directly: construct is pure allocation, the CAS winner registers, losers are left to the GC. This removes the mid-init sentinel state, the runtime spin linknames, and the per-type registration/finalizer duplication. Both tracker types live in one registry — rwLockTracker extends lockTracker, so the scan reads the base through every ref and toStuckLock gathers reader slots when the rw flag is set. window is a weakRefList method with the cursor as a field. Co-Authored-By: Claude Fable 5 --- utils/lock_tracker.go | 151 +++++++++++++++++++-------------------- utils/lock_tracker_rw.go | 49 ++----------- 2 files changed, 77 insertions(+), 123 deletions(-) diff --git a/utils/lock_tracker.go b/utils/lock_tracker.go index d33fd3af4..1eac35ce3 100644 --- a/utils/lock_tracker.go +++ b/utils/lock_tracker.go @@ -60,15 +60,15 @@ func updateLowResTime() { } // weakRefList is a registry of tracker pointers held as uintptrs so they -// don't keep their owners alive; finalizers clear the slots. All lists share -// weakRefLock. +// don't keep their owners alive; finalizers clear the slots. type weakRefList struct { refs []uintptr free []int + next int } var weakRefLock sync.Mutex -var mutexRefs, rwRefs weakRefList +var weakRefs weakRefList func (l *weakRefList) add(p unsafe.Pointer) int { weakRefLock.Lock() @@ -94,10 +94,24 @@ func (l *weakRefList) count() int { return len(l.refs) - len(l.free) } +// window returns the next n-element window of refs, wrapping to the start +// when the end is reached. +func (l *weakRefList) window(n int) []uintptr { + min := l.next + max := min + n + if len(l.refs) <= max { + max = len(l.refs) + l.next = 0 + } else { + l.next = max + } + return l.refs[min:max] +} + func NumMutexes() int { weakRefLock.Lock() defer weakRefLock.Unlock() - return mutexRefs.count() + rwRefs.count() + return weakRefs.count() } // ScanTrackedLocks check all lock trackers @@ -106,12 +120,9 @@ func ScanTrackedLocks(threshold time.Duration) []*StuckLock { weakRefLock.Lock() defer weakRefLock.Unlock() - return append(scanTrackedLocks(mutexRefs.refs, minTS), scanRWTrackedLocks(rwRefs.refs, minTS)...) + return scanTrackedLocks(weakRefs.refs, minTS) } -var nextScanMin int -var nextScanMinRW int - // ScanTrackedLocksI check lock trackers incrementally n at a time func ScanTrackedLocksI(threshold time.Duration, n int) []*StuckLock { minTS := uint32(time.Now().Add(-threshold).Unix()) @@ -122,22 +133,7 @@ func ScanTrackedLocksI(threshold time.Duration, n int) []*StuckLock { weakRefLock.Lock() defer weakRefLock.Unlock() - stuck := scanTrackedLocks(window(mutexRefs.refs, &nextScanMin, n), minTS) - return append(stuck, scanRWTrackedLocks(window(rwRefs.refs, &nextScanMinRW, n), minTS)...) -} - -// window returns the next n-element window of refs, advancing next and -// wrapping to the start when the end is reached. -func window(refs []uintptr, next *int, n int) []uintptr { - min := *next - max := min + n - if len(refs) <= max { - max = len(refs) - *next = 0 - } else { - *next = max - } - return refs[min:max] + return scanTrackedLocks(weakRefs.window(n), minTS) } //go:norace @@ -150,25 +146,39 @@ func scanTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { ts := atomic.LoadUint32(&t.ts) waiting := atomic.LoadInt32(&t.waiting) if ts <= minTS && waiting > 0 { - var gids []int64 - var held int32 - if gid := t.gid; gid != 0 { - gids = []int64{gid} - held = 1 - } - stuck = append(stuck, &StuckLock{ - stack: slices.Clone(t.stack), - ts: ts, - waiting: waiting, - held: held, - gids: gids, - }) + stuck = append(stuck, t.toStuckLock(ts, waiting)) } } } return stuck } +//go:norace +func (t *lockTracker) toStuckLock(ts uint32, waiting int32) *StuckLock { + d := &StuckLock{ + stack: slices.Clone(t.stack), + ts: ts, + waiting: waiting, + } + if gid := t.gid; gid != 0 { + d.gids = append(d.gids, gid) + d.held = 1 + } + if t.rw { + r := (*rwLockTracker)(unsafe.Pointer(t)) + d.held += atomic.LoadInt32(&r.rheld) + if len(d.gids) == 0 && d.held > 0 { + d.holderStrength = HolderShared + } + for i := range r.rgids { + if gid := atomic.LoadInt64(&r.rgids[i]); gid != 0 { + d.gids = append(d.gids, gid) + } + } + } + return d +} + // HolderStrength describes how a stuck lock is held. type HolderStrength int32 @@ -301,12 +311,14 @@ func parseGoroutineHeader(g string) (int64, bool) { // the lock is held, so the single holder's bookkeeping needs no atomics; // only the waiter count, maintained outside the lock, does. The scanner // reads holder state without the lock and tolerates racy reads (go:norace). +// rw marks trackers that are really rwLockTrackers, which extend this with +// read-holder slots. type lockTracker struct { stack []uintptr ts uint32 waiting int32 gid int64 - ref int + rw bool } func (t *lockTracker) trackWait() { @@ -329,61 +341,44 @@ func (t *lockTracker) trackUnlock() { atomic.StoreUint32(&t.ts, math.MaxUint32) } +// newLockTracker allocates a tracker without side effects; lazyInitTracker +// registers whichever allocation wins publication. func newLockTracker() *lockTracker { - t := &lockTracker{ + return &lockTracker{ stack: make([]uintptr, lockTrackerMaxStackDepth), ts: math.MaxUint32, } - t.ref = mutexRefs.add(unsafe.Pointer(t)) - runtime.SetFinalizer(t, func(t *lockTracker) { - mutexRefs.remove(t.ref) - }) - return t } -//go:linkname sync_runtime_canSpin sync.runtime_canSpin -func sync_runtime_canSpin(int) bool - -//go:linkname sync_runtime_doSpin sync.runtime_doSpin -func sync_runtime_doSpin() - -// trackerInitSentinel marks a tracker pointer field as mid-initialization. -var trackerInitSentinel = unsafe.Pointer(new(int)) - -// loadTracker returns the tracker if its initialization completed, else nil. -// Unlock paths use it instead of lazyInitTracker: a lock being unlocked was -// necessarily locked first, so the tracker either exists or tracking was -// disabled at Lock time. +// loadTracker returns the tracker, or nil if there is none. Unlock paths use +// it instead of lazyInitTracker: a lock being unlocked was necessarily +// locked first, so the tracker either exists or tracking was disabled at +// Lock time. func loadTracker[T any](p **T) *T { - t := atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(p))) - if t == trackerInitSentinel { - return nil - } - return (*T)(t) + return (*T)(atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(p)))) } +// lazyInitTracker returns the field's tracker, constructing and registering +// it on first use. construct must be free of side effects: racing +// initializers may each allocate, but only the publishing CAS winner +// registers its tracker; losers' allocations are left to the GC. func lazyInitTracker[T any](p **T, construct func() *T) *T { if !lockTrackerEnabled { return nil } up := (*unsafe.Pointer)(unsafe.Pointer(p)) - iter := 0 - for { - if t := atomic.LoadPointer(up); t == nil { - if atomic.CompareAndSwapPointer(up, nil, trackerInitSentinel) { - atomic.StorePointer(up, unsafe.Pointer(construct())) - } - } else if t == trackerInitSentinel { - if sync_runtime_canSpin(iter) { - sync_runtime_doSpin() - iter++ - } else { - runtime.Gosched() - } - } else { - return (*T)(t) - } + if t := atomic.LoadPointer(up); t != nil { + return (*T)(t) + } + t := construct() + if !atomic.CompareAndSwapPointer(up, nil, unsafe.Pointer(t)) { + return (*T)(atomic.LoadPointer(up)) } + ref := weakRefs.add(unsafe.Pointer(t)) + runtime.SetFinalizer(t, func(*T) { + weakRefs.remove(ref) + }) + return t } type Mutex struct { diff --git a/utils/lock_tracker_rw.go b/utils/lock_tracker_rw.go index 6d831e668..f25154dcd 100644 --- a/utils/lock_tracker_rw.go +++ b/utils/lock_tracker_rw.go @@ -19,10 +19,8 @@ import ( "runtime" "sync" "sync/atomic" - "unsafe" "github.com/petermattis/goid" - "golang.org/x/exp/slices" ) const lockTrackerMaxReadHolders = 8 @@ -97,55 +95,16 @@ func (t *rwLockTracker) trackRUnlock() { } } +// newRWLockTracker allocates a tracker without side effects; lazyInitTracker +// registers whichever allocation wins publication. func newRWLockTracker() *rwLockTracker { - t := &rwLockTracker{ + return &rwLockTracker{ lockTracker: lockTracker{ stack: make([]uintptr, lockTrackerMaxStackDepth), ts: math.MaxUint32, + rw: true, }, } - t.ref = rwRefs.add(unsafe.Pointer(t)) - runtime.SetFinalizer(t, func(t *rwLockTracker) { - rwRefs.remove(t.ref) - }) - return t -} - -//go:norace -//go:nosplit -func scanRWTrackedLocks(refs []uintptr, minTS uint32) []*StuckLock { - var stuck []*StuckLock - for _, ref := range refs { - if ref != 0 { - t := (*rwLockTracker)(unsafe.Pointer(ref)) - ts := atomic.LoadUint32(&t.ts) - waiting := atomic.LoadInt32(&t.waiting) - if ts <= minTS && waiting > 0 { - var gids []int64 - strength := HolderShared - held := atomic.LoadInt32(&t.rheld) - if gid := t.gid; gid != 0 { - gids = append(gids, gid) - held++ - strength = HolderExclusive - } - for i := range t.rgids { - if gid := atomic.LoadInt64(&t.rgids[i]); gid != 0 { - gids = append(gids, gid) - } - } - stuck = append(stuck, &StuckLock{ - stack: slices.Clone(t.stack), - ts: ts, - waiting: waiting, - held: held, - holderStrength: strength, - gids: gids, - }) - } - } - } - return stuck } type RWMutex struct {