[RESP] Fix pub/sub: restrict commands in RESP2 subscription mode and fix reentrant lock crash

[badrishc]

Root Cause

Two related pub/sub bugs reported in #1615:

1. RESP2 command restriction missing: When a client entered subscription mode (via SUBSCRIBE/PSUBSCRIBE), the server continued to allow arbitrary commands like GET, SET, PUBLISH, etc. Per the Redis protocol, only (P|S)SUBSCRIBE, (P|S)UNSUBSCRIBE, PING, and QUIT are valid in RESP2 subscription mode — other commands cannot be reliably distinguished from out-of-band push messages.

2. Reentrant lock crash: When PUBLISH was executed from a subscribed session to a channel it was subscribed to (self-publish), SubscribeBroker.Broadcast() called the Publish() callback on the same session. This callback called networkSender.EnterAndGetResponseObject(), re-entering the SpinLock already held by TryConsumeMessages. When the callback's finally block released the lock, the outer TryConsumeMessages.finally tried to release it again, causing SynchronizationLockException: The calling thread does not hold the lock.

Description of Change

Key changes:

• RespCommand.IsAllowedInSubscriptionMode() — New AggressiveInlining extension method that returns true for SUBSCRIBE, UNSUBSCRIBE, PSUBSCRIBE, PUNSUBSCRIBE, SSUBSCRIBE, PING, and QUIT.

• Reentrant lock detection — Added commandProcessingThreadId field set around ProcessMessages() in TryConsumeMessages. In Publish() and PatternPublish() callbacks, compares Environment.CurrentManagedThreadId to detect reentrant calls and skips EnterAndGetResponseObject()/ExitAndReturnResponseObject() when the lock is already held, writing the push message directly to the existing output buffer.

• Removed stale assertion — Debug.Assert(isSubscriptionSession == false) in NetworkPUBLISH was removed since RESP3 legitimately allows PUBLISH in subscription mode.

Key Technical Details

Affected types/interfaces:

• RespServerSession (ProcessMessages, TryConsumeMessages, Publish, PatternPublish) — core command dispatch and pub/sub message delivery
• RespCommandExtensions — new IsAllowedInSubscriptionMode() method
• CmdStrings — new GenericPubSubCommandNotAllowed error format string

Performance Impact

None for normal (non-subscription) command processing. The guard check short-circuits immediately on isSubscriptionSession == false (a boolean field read). The commandProcessingThreadId write is a single int store per TryConsumeMessages call — effectively free.

What NOT to Do (for future agents)

• ❌ Don't use a simple boolean flag for reentrant detection — A flag like isProcessingCommands is not thread-safe: Thread A could set it on Session A, then Thread B reading Session A's flag would incorrectly think it's reentrant. Use thread ID comparison instead.
• ❌ Don't skip self-notification in Broadcast() — In RESP3, a subscriber should receive its own published messages. The reentrant lock detection preserves this behavior.

Tests Added

• PubSubModeRejectsDisallowedCommandsInResp2 — Verifies GET, SET, PUBLISH, MULTI are rejected with correct error in RESP2 subscription mode
• PubSubModeAllowsValidCommandsInResp2 — Verifies PING (subscription PONG), SUBSCRIBE, PSUBSCRIBE, UNSUBSCRIBE, PUNSUBSCRIBE work; also verifies commands work again after full unsubscribe
• PubSubSelfPublishResp3NoLockError — RESP3 self-publish (HELLO 3 → SUBSCRIBE → PUBLISH to same channel) succeeds with correct push message delivery and no crash

Issues Fixed

Fixes #1615

[Copilot]

Pull request overview

Fixes two pub/sub issues in the RESP server: (1) enforce Redis-compatible command restrictions while a RESP2 connection is in subscription mode, and (2) prevent a re-entrant network sender lock crash when a subscribed session publishes to its own channel (self-publish), especially relevant for RESP3 where self-notification is expected.

Changes:

• Add RESP2-only subscription-mode command gating (allow only subscribe/unsubscribe variants, PING, QUIT) with a dedicated error message.
• Add re-entrancy detection for pub/sub push callbacks to avoid re-entering the network sender lock during self-publish.
• Add regression tests covering RESP2 command rejection/allow-list and RESP3 self-publish stability.

Reviewed changes

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

Show a summary per file

File	Description
test/Garnet.test/RespPubSubTests.cs	Adds tests for RESP2 subscription-mode restrictions and RESP3 self-publish lock regression.
libs/server/Resp/RespServerSession.cs	Enforces RESP2 subscription-mode allow-list and tracks command-processing thread for re-entrancy detection.
libs/server/Resp/PubSubCommands.cs	Avoids re-entering the network sender lock for re-entrant publish callbacks; removes stale assertion.
libs/server/Resp/Parser/RespCommand.cs	Adds IsAllowedInSubscriptionMode() command allow-list helper.
libs/server/Resp/CmdStrings.cs	Adds standardized error string for disallowed commands in RESP2 subscription mode.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.