Reduce lock contention during concurrent client disconnects

[sciascid]

Reduce server lock contention by introducing two additional locks:

• eventIdsMu to protect event id generation
• closedMu to protect the closed connections ring buffer

[sciascid]

@claude review

[synadia-claude-reviewer]

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.