Avoid lock inversion in raft mock transport

[sciascid]

Deliver mock transport callbacks after releasing the hub lock to prevent deadlocks with Raft node locks. This deflakes TestNRGLeaderWithoutQuorumAfterPeerAdd.

[sciascid]

@claude review

[synadia-claude-reviewer]

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

---

Code Review

• Gather context (read diff and relevant files)
• Analyze lock ordering and correctness
• Review behavioral changes
• Post review

---

Summary: The fix is correct and well-targeted. It resolves a classic lock-inversion deadlock where hub.mu was held while invoking raft message callbacks that could themselves need to acquire hub.mu (e.g. via partition, heal, or publish on another node).

What the fix does

The original publish loop called sub.icb(...) while holding h.mu:

// Before — deadlock-prone
h.mu.Lock()
for id, transport := range h.transports {
    res := transport.sub.Match(subject)
    for _, sub := range res.psubs {
        sub.icb(sub, nil, transport.acc, subject, reply, msg) // under hub lock!
    }
}
h.mu.Unlock()

The fix splits this into two phases — collect while locked, deliver after unlocking — which is the standard remedy for this pattern:

// After — lock-inversion free
h.mu.Lock()
for id, transport := range h.transports {
    result := transport.sub.Match(subject)
    subscriptions = append(subscriptions, result.psubs...)
}
h.mu.Unlock()

for _, sub := range subscriptions {
    sub.icb(sub, nil, nil, subject, reply, msg)
}

Correctness

transport.acc → nil (server/raft_transport_helpers_test.go:123): All four raft handler callbacks discard the account argument (using _ *Account):

• handleAppendEntry (raft.go:4237)
• handleAppendEntryResponse (raft.go:5032)
• handleVoteResponse (raft.go:5581)
• handleVoteRequest (raft.go:5673)

Passing nil here is safe.

Subscription lifetime: *subscription pointers are collected under the hub lock. After unlocking, a transport could be closed (t.sub = nil; hub.unregister(t)), but the collected *subscription pointers remain valid — they're not freed. Calling icb on an orphaned subscription is harmless in this test-only context.

Sublist.Match thread-safety: Match takes its own internal lock (doLock=true), so it is safe regardless of hub lock state.

Concurrent publishes: The original code serialized all icb calls across concurrent publish invocations (hub lock held throughout). The new code allows interleaving between concurrent publishes at delivery time. For raft testing, where each node runs its own goroutine and processes messages from its own queue, this is fine.

afterMsgHook ordering: The hook is still called after all callbacks complete, preserving the documented contract that the hub lock is released before the hook runs.

One minor observation

The comment above setAfterMsgHook (raft_transport_helpers_test.go:87-96) says:

"the underlying RaftNode that has sent the message remains locked throughout the execution of the hook"

This remains true since the call chain is: raft.mu held → n.t.Publish(...) → hub.publish(...) → callbacks → hook. The raft sender's lock is still held during both callbacks and the hook, so no update needed to the comment.

Verdict

The fix is correct, minimal, and addresses the root cause cleanly. No concerns.

---

[MauriceVanVeen]

LGTM