Skip to content

fix(coderd): make chatd stream sync poller a callback fanout to fix send-on-closed-channel panic - #28746

Draft
ethanndickson wants to merge 1 commit into
mainfrom
fix/chatd-stream-sync-poller-panic
Draft

fix(coderd): make chatd stream sync poller a callback fanout to fix send-on-closed-channel panic#28746
ethanndickson wants to merge 1 commit into
mainfrom
fix/chatd-stream-sync-poller-panic

Conversation

@ethanndickson

Copy link
Copy Markdown
Member

Closes SEC-515

Problem

streamSyncPoller delivered hints over per-subscriber channels. unregister closed a subscriber's channel under the poller mutex, while pollOnce sent on it from a lock-free snapshot taken before the DB query. A subscriber unregistering mid-poll made pollOnce panic with send on closed channel, taking down the whole coderd process. Any authenticated user could trigger this by opening and closing chat streams (GHSA-7x3x-59xg-4hrc). On dev.coder.com this crashed 7 pods within 12 hours on 2026-08-27:

panic: send on closed channel
  coderd/x/chatd.(*streamSyncPoller).pollOnce  stream_sync_poller.go:136
  coderd/x/chatd.(*streamSyncPoller).loop      stream_sync_poller.go:115
  created by coderd/x/chatd.(*streamSyncPoller).Start  stream_sync_poller.go:61

Fix

Convert the poller to a callback fanout, the same shape as pubsub.Subscribe / MemoryPubsub:

  • Register(chatID, deliver func(streamSyncHint)) returns only an unregister func().
  • unregister is delete-only; nothing is ever closed, so the panic is unrepresentable rather than guarded.
  • The consumer registers a callback that funnels poller hints into its existing consumer-owned updateCh, using the identical streamCtx.Done() guard already used for the pubsub producer ten lines above. The pollerCh select arm is deleted.
  • The nil-poller path returns a no-op unregister instead of an already-closed channel, removing a latent bug where the consumer's !ok check would instantly terminate every stream.

Delivery stays outside the poller mutex (existing snapshot design), so a slow consumer cannot stall polling for other chats; the callback-may-race-unregister contract matches what the consumer already assumes for async PGPubsub delivery and is documented on Register.

Testing

  • New regression race test (stream_sync_poller_internal_test.go) churns register/unregister across 8 goroutines against 1000 pollOnce deliveries; the previous implementation panics under it.
  • go test -race ./coderd/x/chatd/... passes.

…end-on-closed-channel panic

The stream sync poller delivered hints over per-subscriber channels that
unregister closed under the poller mutex, while pollOnce sent on them
from a lock-free snapshot. A subscriber unregistering mid-poll made
pollOnce panic with "send on closed channel", crashing coderd. Any
authenticated user could trigger this by opening and closing chat
streams (GHSA-7x3x-59xg-4hrc).

Convert the poller to a callback fanout, mirroring pubsub.Subscribe:
Register now takes a deliver callback and returns only an unregister
func, unregister is delete-only (nothing is ever closed), and the
consumer funnels poller hints into its existing consumer-owned updateCh
with the same streamCtx guard already used for pubsub hints. This makes
the panic unrepresentable and also removes the nil-poller closed-channel
return that instantly terminated streams via the ok-check.

Adds a regression race test churning register/unregister against
pollOnce; the previous implementation panics under it.
@linear-code

linear-code Bot commented Aug 28, 2026

Copy link
Copy Markdown

SEC-515

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant