Fix SynchronizationLockException on pub/sub self-publish

[hexonal (hexonal)]

Fixes #1615 (the network-lock-error part of that issue; the RESP2 command-restriction part — Garnet not yet rejecting non-(P)SUBSCRIBE/PING/QUIT/RESET commands while in subscribe mode — is a separate, larger concern and out of scope for this PR).

Root cause

GarnetTcpNetworkSender's spinLock field is a SpinLock struct initialized with new() and no user-declared parameterless constructor. That zero-inits _owner, which per SpinLock semantics enables thread-owner tracking (equivalent to new SpinLock(true)), so a same-thread reentrant Enter() throws.

A session can trigger a reentrant call into its own network sender: PUBLISH on a connection that is itself subscribed to the target channel causes SubscribeBroker.PublishNow/Broadcast to call back into that same connection's Publish()/PatternPublish() synchronously, while TryConsumeMessages for the PUBLISH command is still on the stack and still holding the sender's SpinLock. Garnet does not currently enforce RESP2's "only a few commands allowed while subscribed" restriction, so this is reachable without RESP3.

EnterAndGetResponseObject on that reentrant call throws before touching lockTaken/responseObject, but the old code's finally block unconditionally called ExitAndReturnResponseObject(). Since SpinLock.Exit()'s ownership check is by thread ID (not call-frame/recursion depth), the erroneous reentrant Exit() succeeded and silently released the outer frame's lock early. The outer TryConsumeMessages finally then called Exit() again on an already-released lock, throwing an uncaught SynchronizationLockException and tearing down the connection.

As a secondary effect, the same unconditional finally also returned responseObject to the free buffer pool on the reentrant frame — but responseObject there is still the outer frame's actively-in-use buffer (never reset, since the failed Enter never got that far), so the old code was also returning a buffer to the pool while it was still being written to.

Fix

In PubSubCommands.cs, Publish() and PatternPublish() now track whether EnterAndGetResponseObject actually succeeded (entered, following the same lockTaken-guard pattern already used in EnterAndGetResponseObject/GarnetTcpNetworkSender.cs) and only call ExitAndReturnResponseObject() in finally if it did. This also incidentally fixes the buffer-pool double-use described above, since the erroneous Exit/return no longer happens.

The Debug.Assert(isSubscriptionSession == false) in NetworkPUBLISH is removed: in Debug builds without a debugger attached, Debug.Assert failure calls Environment.FailFast and kills the whole process, so this exact scenario would crash the test process outright regardless of the lock fix. The assert was asserting a false invariant anyway — self-publish-while-subscribed is a real, currently-unrestricted scenario.

Behavior note / known tradeoff

With this fix, a message a session publishes to a channel it is itself (pattern-)subscribed to is not delivered back to that same connection — the reentrant delivery attempt is now a no-op instead of corrupting the connection, so the self-message is silently dropped rather than crashing. PUBLISH itself still completes normally and still reports the correct subscriber count. This is a deviation from full Redis pub/sub semantics (a subscriber normally does receive its own publish) and is a deliberate, minimal tradeoff to fix the crash without restructuring the network-sender locking or deferring self-delivery to after the outer lock is released. Delivering the self-message correctly (e.g. by queuing it for delivery once the outer TryConsumeMessages lock is released) would be a larger change and is left as potential follow-up; happy to discuss if maintainers would prefer that approach instead.

Testing performed

• dotnet build test/standalone/Garnet.test/Garnet.test.csproj in both Debug and Release: both succeed, 0 warnings, 0 errors.
• dotnet test ... --filter "FullyQualifiedName~RespPubSubTests" on net10.0, Debug and Release: 8/8 pass (7 pre-existing + 2 new regression tests, one for SUBSCRIBE/Publish and one for PSUBSCRIBE/PatternPublish).
• Reverted the entered-guard fix in Publish() only (kept the Debug.Assert removal) and reran the new SelfPublishOnSubscribedChannelDoesNotCorruptConnection test alone: it fails, with the connection dying mid-PUBLISH as expected. Restored the fix: passes again, confirmed stable across repeated runs.
• dotnet format --verify-no-changes on the changed files: clean.
• git diff --check: no whitespace/line-ending issues.
• Manual review of the diff against existing lock-guard conventions elsewhere in the same files (GarnetTcpNetworkSender.cs, EnterAndGetResponseObject).
• Not run: the full Garnet test suite (only the pub/sub-focused subset above was targeted) and any cluster/RESP3-specific pub/sub paths beyond what these tests cover.

Diff touches libs/server/Resp/PubSubCommands.cs and test/standalone/Garnet.test/RespPubSubTests.cs only.

[Copilot]

Pull request overview

This PR addresses a crash scenario in Garnet’s pub/sub path where a connection can re-enter its own network-sender SpinLock during self-publish delivery, leading to an eventual SynchronizationLockException and connection teardown. The fix ensures the sender lock/buffer are only released when they were actually acquired, and adds regression coverage for both SUBSCRIBE and PSUBSCRIBE self-publish scenarios.

Changes:

• Guard ExitAndReturnResponseObject() in Publish() and PatternPublish() behind an entered flag so re-entrant failures do not unlock/return buffers they never acquired.
• Remove the Debug.Assert(isSubscriptionSession == false) in NetworkPUBLISH and replace it with an explanatory comment (avoids FailFast in Debug and reflects real reachable behavior).
• Add two socket-level regression tests that reproduce the self-publish-on-subscribed-connection scenario and verify the connection remains usable afterward.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
libs/server/Resp/PubSubCommands.cs	Adds an entered guard to prevent incorrect lock exit/buffer return on re-entrant pub/sub delivery; removes unsafe Debug assert in PUBLISH.
test/standalone/Garnet.test/RespPubSubTests.cs	Adds regression tests for self-publish on subscribed and pattern-subscribed connections, plus helper routines to send/receive raw RESP.

[Badrish Chandramouli (badrishc)]

Thank you hexonal (@hexonal) for your contributions! For this particular PR, note that there was already an open PR #1669 that handles the lock issue as well as RESP command restrictions. We have incorporated aspects such as the test and RCA from here into that one.