fix: prevent zombie IPC subscriptions in multi-worker memstore - #718
Open
gunjanjp wants to merge 4 commits into
Open
fix: prevent zombie IPC subscriptions in multi-worker memstore#718gunjanjp wants to merge 4 commits into
gunjanjp wants to merge 4 commits into
Conversation
Three bugs cause "zombie" subscriptions where WebSocket clients remain
connected (heartbeat works) but stop receiving published messages:
1. KA_REPLY_UNHOOK_NORENEW silently drops messages without notifying
the non-owner worker. When receive_subscriber_keepalive detects a
stale chanhead pointer ("keepalive for expired channel"), the owner
calls memstore_ipc_subscriber_unhook() which sets unhooked=1 but
never dequeues the IPC subscriber and never sends 'unsubscribed'
to the non-owner. The IPC subscriber remains in the spooler,
silently dropping all future messages. WebSocket subscribers on the
non-owner become permanent zombies.
Fix: call sub->fn->dequeue() instead of memstore_ipc_subscriber_unhook().
This properly removes the IPC subscriber from the spooler and sends
'unsubscribed' to the non-owner, matching KA_REPLY_NORENEW behavior.
2. receive_unsubscribed does not re-establish the IPC link when
sub_count > 0. When a non-owner receives 'unsubscribed', it sets
foreign_owner_ipc_sub = NULL but only acts if sub_count == 0
(adding to GC). If subscribers are still connected, the IPC link
is dead but no re-subscription is triggered.
Fix: when sub_count > 0, call memstore_ensure_chanhead_is_ready()
to re-establish the IPC subscription to the owner.
3. nchan_memstore_publish_generic GC's the owner chanhead after every
publish regardless of subscriber count. The chanhead bounces between
READY and INACTIVE on every message, creating unnecessary state
churn that triggers the keepalive stale-pointer race condition.
Fix: only GC the owner chanhead after publish when sub_count == 0.
The reaper already checks memstore_chanhead_reserved_or_in_use()
before reaping, so this avoids the pointless READY->INACTIVE->READY
transitions while subscribers are active.
Signed-off-by: Gunj Patel <malav@aditadv.com>
The receive_subscriber_keepalive handler (non-owner side) called nchan_memstore_force_delete_channel when the chanhead had unexpected status. This crashes because force_delete asserts memstore_channel_owner == memstore_slot(), which fails on non-owners. The d->shm_chid pointer was also already freed (line 912) before being passed to force_delete — a use-after-free. The WAITING status with sub_count > 0 is now a valid transitional state during IPC re-subscription. Renew the keepalive to allow the subscribe reply to complete. For other unexpected states, just unhook without the destructive force_delete — the chanhead will be cleaned up by normal GC. Signed-off-by: Gunj Patel <malav@aditadv.com>
When an old IPC subscriber is dequeued (e.g. via keepalive mismatch), the unsubscribed message was unconditionally NULLing foreign_owner_ipc_sub, even when a newer subscription had already replaced it. This triggered a re-subscribe cycle: new sub created -> old keepalive mismatches -> dequeue old -> unsubscribed NULLs the new sub -> re-subscribe again -> repeat. Fix: pass the subscriber pointer through the unsubscribed message so receive_unsubscribed can check if it matches the current subscription. If foreign_owner_ipc_sub is already set to a different (newer) subscriber, the unsubscribed message is stale and should be ignored.
…ED handler When a message expires, subscribers transfer to the oldest-message spool via get_spool(). If that spool already has a fetch in progress (msg_status == MSG_PENDING), calling spool_fetch_msg() without checking hits the assertion at spool.c:479 (msg_status == MSG_INVALID). Add a status guard: only call spool_fetch_msg() when the target spool is in MSG_INVALID or MSG_CHANNEL_NOTREADY state. If a fetch is already pending, the transferred subscribers will receive the result when it completes.
gunjanjp
force-pushed
the
fix/zombie-ipc-subscription-recovery
branch
2 times, most recently
from
September 3, 2026 21:40
7849066 to
effef71
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes zombie WebSocket subscriptions and two related crash bugs in multi-worker memstore configurations (no Redis). WebSocket clients remain connected (heartbeat works) but stop receiving published messages due to broken IPC pipeline between nginx workers.
Addresses #719 (zombie subscriptions) and #720 (spool assertion crash).
Changes
Fix 1: Dequeue instead of unhook on
KA_REPLY_UNHOOK_NORENEW(ipc-handlers.c)Before:
memstore_ipc_subscriber_unhook(sub)setsunhooked=1but doesn't dequeue — IPC subscriber remains in the spooler silently dropping all messages.After:
sub->fn->dequeue(sub)properly dequeues and sendsunsubscribedto the non-owner, enabling recovery.Fix 2: Re-subscribe in
receive_unsubscribedwhen subscribers exist (ipc-handlers.c)Before: When
foreign_owner_ipc_subis NULLed andsub_count > 0, no action taken — IPC link stays dead.After: Calls
memstore_ensure_chanhead_is_ready(head, 1)to re-establish the IPC subscription when local subscribers still exist.Fix 3: Skip GC after publish when subscribers exist (memstore.c)
Before: Owner chanhead added to GC after every publish regardless of subscriber count, causing unnecessary READY→INACTIVE→READY churn that widens the race window for Fix 1.
After: Only add to GC when
sub_count == 0.Fix 4: Safe keepalive handler for non-READY chanheads (ipc-handlers.c)
Before: Keepalive handler calls
nchan_memstore_force_delete_channel()on non-owner workers (assertion violation) and has use-after-free ond->shm_chid(freed at line 912, used at lines 927/934).After: WAITING + subs →
KA_REPLY_RENEW; other non-READY states →KA_REPLY_UNHOOK_NORENEW. No force_delete, no use-after-free.Fix 5: Prevent IPC re-subscribe thrashing loop (ipc-handlers.c, memstore_ipc.c)
Before:
receive_unsubscribedunconditionally NULLsforeign_owner_ipc_sub, even when a newer subscription has replaced the old one. This creates an infinite re-subscribe loop (~3,900 log messages/minute).After: Pass the subscriber pointer through the unsubscribed message. If
foreign_owner_ipc_subis already set to a different (newer) subscriber, the stale unsubscribed message is ignored.Fix 6: Guard spool_fetch_msg against concurrent fetch race (spool.c)
Before:
spool_fetch_msg_callback()callsspool_fetch_msg(nuspool)unconditionally on MSG_EXPIRED. Ifnuspoolalready has a fetch in progress (MSG_PENDING), assertion fails at line 479.After: Check
nuspool->msg_statusbefore calling — only fetch when MSG_INVALID or MSG_CHANNEL_NOTREADY, consistent with all other call sites.Files Changed
src/store/memory/ipc-handlers.csrc/store/memory/memstore.csrc/subscribers/memstore_ipc.csrc/store/spool.cTesting
Tested on production-equivalent beta server (16 workers, nginx 1.30.4):
Deployed to production (8 workers, 300-500 concurrent subscribers) — stable with zero errors.