mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd/x/chatd): refresh chat status and bound subscriber reads on Subscribe (#24095)
Tightens the chat stream subscription path on a few related axes. None of these changes touch the steady-state event flow; they all concern the subscribe handshake. ## Motivation `Server.Subscribe` carries three responsibilities that were entangled: 1. Authorize the caller against the chat row. 2. Arm local + pubsub subscriptions before any DB reads (subscribe-first-then-query). 3. Build the initial snapshot from a fresh chat row, message history, and queue. When all three live in one function and share the request context, a few unfortunate behaviors fall out: - The HTTP handler's middleware already loaded and authorized the chat row, but `Subscribe(chatID)` discarded it and re-fetched on every WebSocket connection. - The chat row used to populate the initial `status` event was loaded *before* the pubsub subscription was armed, so a status transition that happened in that window was silently lost. - Control-path DB reads inherited whatever context the caller passed in. A caller without a deadline could wedge a subscriber goroutine indefinitely on a stalled DB. - A transient failure of the chat re-read collapsed the entire subscription instead of degrading gracefully. ## What changes **Split the auth boundary out into the type signature.** A new `SubscribeAuthorized(ctx, chat, ...)` takes the already-authorized row directly. The HTTP handler in `coderd/exp_chats.go` calls it with the chat row from `httpmw.ChatParam`, eliminating the redundant `GetChatByID`. `Subscribe(chatID)` is preserved as a thin wrapper for callers that don't have a chat row in hand (tests, internal callers); it does the auth lookup and delegates. **Re-read the chat after arming subscriptions.** Inside `SubscribeAuthorized`, after the local stream and pubsub subscriptions are active, we reload the chat row to populate the initial `status` event and any enterprise relay setup. Combined with the existing subscribe-first-then-query pattern, this closes the gap where a status transition between the middleware's load and the subscription arming would not appear in either the initial snapshot or a live notification. **Fall back to the middleware row on refresh failure.** If the post-subscription refresh fails (transient DB blip, brief pool exhaustion), we log a warning and reuse the row that proved authorization in the first place. Messages, queue, and pubsub are all independent of this row, so the stream still works; the initial `status` is just slightly stale and self-corrects via the next pubsub event. **Bound subscriber control-path DB reads.** A new `streamSubscriberControlFetchContext` helper applies a 5-second fallback timeout only when the caller has no deadline of their own. Used at the chat refresh, the initial queue load, and the queue-update goroutine following pubsub notifications. HTTP-driven callers pass through unchanged; background callers can no longer hang forever on a stalled DB and leak subscriber goroutines, pubsub subscriptions, and `chatStreams` entries.
This commit is contained in:
@@ -433,8 +433,13 @@ func TestSubscribeRelaySnapshotDelivered(t *testing.T) {
|
||||
user, org, model := seedChatDependencies(t, db)
|
||||
|
||||
chat := seedRemoteRunningChat(ctx, t, db, org.ID, user, model, workerID, "relay-snapshot")
|
||||
staleChat := chat
|
||||
staleChat.Status = database.ChatStatusWaiting
|
||||
staleChat.WorkerID = uuid.NullUUID{}
|
||||
staleChat.StartedAt = sql.NullTime{}
|
||||
staleChat.HeartbeatAt = sql.NullTime{}
|
||||
|
||||
initialSnapshot, events, cancel, ok := subscriber.Subscribe(ctx, chat.ID, nil, 0)
|
||||
initialSnapshot, events, cancel, ok := subscriber.SubscribeAuthorized(ctx, staleChat, nil, 0)
|
||||
require.True(t, ok)
|
||||
t.Cleanup(cancel)
|
||||
|
||||
@@ -458,15 +463,15 @@ func TestSubscribeRelaySnapshotDelivered(t *testing.T) {
|
||||
|
||||
require.Equal(t, []string{"snap-one", "snap-two", "live-part"}, receivedTexts)
|
||||
|
||||
// The initial snapshot should still contain the status event
|
||||
// from the OSS preamble.
|
||||
var hasStatus bool
|
||||
// The initial snapshot should contain the refreshed running status,
|
||||
// not the stale waiting status passed into SubscribeAuthorized.
|
||||
var snapshotStatus codersdk.ChatStatus
|
||||
for _, event := range initialSnapshot {
|
||||
if event.Type == codersdk.ChatStreamEventTypeStatus {
|
||||
hasStatus = true
|
||||
if event.Type == codersdk.ChatStreamEventTypeStatus && event.Status != nil {
|
||||
snapshotStatus = event.Status.Status
|
||||
}
|
||||
}
|
||||
require.True(t, hasStatus, "initial snapshot should contain status event")
|
||||
require.Equal(t, codersdk.ChatStatusRunning, snapshotStatus)
|
||||
}
|
||||
|
||||
func TestSubscribeRetryEventAcrossInstances(t *testing.T) {
|
||||
|
||||
Reference in New Issue
Block a user