Reduce lock contention during concurrent client disconnects - #8533

@claude review

Claude finished @sciascid's task in 3m 54s —— View job

Code Review

• Gather context
• Read changed files
• Analyze lock ordering and correctness
• Review test changes
• Post review feedback

Overall this is a clean, well-reasoned change that correctly reduces server lock contention during high-frequency client disconnect events. The lock ordering is documented and respected throughout.

Summary

The PR introduces two new fine-grained mutexes to reduce contention on the server's main sync.RWMutex:

• eventIdsMu — serializes access to the nuid.NUID event ID generator
• closedMu — serializes access to the closed connection ring buffer

This is consistent with how Account.eventIdsMu already worked (accounts.go:104), so the pattern is proven in the codebase.

Correctness

eventsEnabled() under RLock (events.go:2574, 2621, etc.)

The switch from s.mu.Lock() to s.mu.RLock() for the eventsEnabled() check is correct. eventsEnabled() only reads s.sys, s.sys.client, and s.sys.account, all of which are protected by the server lock. A read lock is sufficient.

TOCTOU window between check and send

After s.mu.RUnlock(), there's a brief window before s.nextEventID() and sendInternalMsgLocked() where events could be disabled. This is benign: sendInternalMsg re-checks s.sys != nil && s.sys.sendq != nil before enqueuing (events.go:764). At worst, a spurious event ID is generated but the message is dropped — no correctness issue.

sendAccConnsUpdate holding s.mu.Lock() then calling s.nextEventID() (events.go:2465)

This acquires s.mu.Lock() then s.eventIdsMu.Lock(), which matches the documented ordering Server -> eventIdsMu in locksordering.txt. ✓

sendInternalMsgLocked usage (events.go:755-758)

The OCSP event functions correctly capture serverID := s.info.ID under the read lock, use it to construct the subject, then call sendInternalMsgLocked (which takes its own s.mu.RLock()). The m.Server (ServerInfo) fields are populated later by the send loop (events.go:532-543), not at call time, so there's no issue with the struct escaping the function.

closedMu is sync.Mutex not sync.RWMutex (server.go:214)

Read-only accessors (closedClients, numClosedConns, totalClosedConns) previously ran under s.mu.RLock() and could proceed concurrently. Now they use s.closedMu.Lock() (exclusive), so concurrent monitor reads will serialize. This is a minor regression in read-side concurrency for /connz, but given that monitor access is relatively infrequent compared to disconnect events, the trade-off is reasonable. A sync.RWMutex would allow concurrent reads, but the current approach is correct.

Lock ordering documentation (locksordering.txt:63-73)

Both new orderings are correctly described: Server -> eventIdsMu and Server -> closedMu. No code path in this PR acquires closedMu or eventIdsMu while holding the server lock (or vice versa), so these are effectively independent mutexes right now. The documentation just establishes the permitted order for future code. ✓

Tests

TestMonitorConnzSortedByStopTimeClosedConn / TestMonitorConnzSortedByReason (monitor_test.go:1205, 1248)

The tests now call s.closedClients() (which takes s.closedMu.Lock() internally) and then modify cc.Stop / cc.Reason on the returned pointers outside any lock. Since closedRingBuffer.append replaces slot pointers rather than modifying existing closedClient structs (ring.go:40), the returned pointers are safe to modify after the lock is released. Correct.

TestConnzClosedSubsDetailNoSharedMutation (monitor_test.go:7007)

Correctly updated to use s.closedMu.Lock() directly for test setup. ✓

Nits / suggestions

None that would block merging. The change is straightforward and well-scoped.

@claude review the two additional commits

Claude finished @sciascid's task in 4m 13s —— View job

Review of Two Additional Commits

• Identify the two additional commits
• Read changed files and diff
• Analyze correctness and lock ordering
• Post review feedback

The two additional commits are:

• f461e3d — Skip accConnsUpdate for the global account (server/accounts.go)
• 3d89a0d — Reduce lock contention in Connz handler (server/monitor.go)

Both are correct and well-targeted. Details below.

f461e3d — Skip accConnsUpdate for the global account

accConnsUpdate (events.go:2558) already contains an early-return guard:

if !s.eventsEnabled() || a == nil || a == s.gacc {
    return
}

Before this commit, addClient/removeClient were calling accConnsUpdate for global-account clients anyway, pointlessly acquiring s.mu.Lock() only to immediately release it. This commit short-circuits that by capturing isGlobal while already holding a.mu.Lock().

Correctness observations:

1. Name vs. pointer check — The new guard uses a.Name == globalAccountName (string comparison) while accConnsUpdate uses a == s.gacc (pointer comparison). These are semantically equivalent because s.gacc is always initialized with globalAccountName and never changes. This name-based pattern is already established elsewhere in the codebase (e.g., opts.go:3438).

2. a.Name read under a.mu.Lock() — Correct, though a.Name is immutable after account creation, so even reading it without the lock would be safe. Capturing it inside the lock (before unlock) is fine.

3. c != nil check — The c != nil check on line 1081/1173 is always true since c is the receiver being actively used, but this pre-existed and is harmless.

Verdict: Clean optimization, no issues.

3d89a0d — Reduce lock contention in Connz handler

This moves s.mu.RUnlock() from after the per-client iteration loop (old line 419) to immediately after the connection map snapshot (new line 318). The server lock is no longer held during the slow per-client scan that acquires client.mu.RLock() per connection.

Correctness observations:

1. Snapshot consistency — clist (copy of s.clients+s.leafs) and cidClient (s.clients[cid]) are both captured under the same s.mu.RLock(), so they represent a consistent point-in-time view. ✓

2. cidClient lifetime after unlock — After s.mu.RUnlock(), a client referenced by cidClient could disconnect and be removed from s.clients. However, the Go GC keeps the *client struct alive as long as cidClient holds a reference, so accessing client.mu and calling fill() under the client lock remains safe. This is the standard "snapshot then release" pattern.

3. CID-only lookup uses s.clients, not s.leafs — cidClient = s.clients[cid] only searches open clients, not leafs. This matches the original behavior (if client := s.clients[cid]; client != nil) at the old line 376, so there is no regression.

4. Lock ordering — Previously the code held s.mu.RLock() while acquiring client.mu.RLock() (Server → Client ordering, which is correct per lock ordering rules). Now the server lock is released before any client locks are taken, which is also valid (no lock held → acquiring a lock is always safe). ✓

5. Filter loop (pre-existing) — Lines 424–436 have a pre-existing quirk where openClients = oc is assigned inside the range loop body over openClients. This is unchanged by this commit, not introduced here.

Verdict: Correct and a meaningful improvement — the server lock is now held for a minimal snapshot window instead of across the entire per-client inspection loop.

No blocking issues in either commit. Both are good additions to the PR.