mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(coderd/x/chatd): gate control subscriber to ignore stale pubsub notifications (#23865)
Fixes flaky `TestOpenAIReasoningWithWebSearchRoundTripStoreFalse` and
`TestOpenAIReasoningWithWebSearchRoundTrip`.
## Changes
- Gate the `processChat` control subscriber's cancel callback behind a
`chan struct{}` that is closed after publishing `"running"` status
- Add `TestGatedControlCancel` with 4 subtests exercising the gate logic
<details>
<summary>Root cause analysis</summary>
`SendMessage` publishes a `"pending"` notification on
`chat:stream:<chatID>` via PostgreSQL `NOTIFY`. `processChat` subscribes
to the same channel for control signals. Due to async NOTIFY delivery,
the `"pending"` notification can arrive at the control subscriber
**after** it registers its queue — even though it was published
**before**. `shouldCancelChatFromControlNotification("pending")` returns
`true`, immediately self-interrupting the processor before it does any
work.
The fix gates the cancel callback behind a closed channel. The channel
is closed after `processChat` publishes `"running"` status, so stale
notifications from before initialization are harmlessly ignored.
`close()` provides a happens-before guarantee in the Go memory model.
</details>
> 🤖 Written by a Coder Agent. Reviewed by a human.
This commit is contained in:
+25
-1
@@ -3487,7 +3487,25 @@ func (p *Server) processChat(ctx context.Context, chat database.Chat) {
|
||||
chatCtx, cancel := context.WithCancelCause(ctx)
|
||||
defer cancel(nil)
|
||||
|
||||
controlCancel := p.subscribeChatControl(chatCtx, chat.ID, cancel, logger)
|
||||
// Gate the control subscriber behind a channel that is closed
|
||||
// after we publish "running" status. This prevents stale
|
||||
// pubsub notifications (e.g. the "pending" notification from
|
||||
// SendMessage that triggered this processing) from
|
||||
// interrupting us before we start work. Due to async
|
||||
// PostgreSQL NOTIFY delivery, a notification published before
|
||||
// subscribeChatControl registers its queue can still arrive
|
||||
// after registration.
|
||||
controlArmed := make(chan struct{})
|
||||
gatedCancel := func(cause error) {
|
||||
select {
|
||||
case <-controlArmed:
|
||||
cancel(cause)
|
||||
default:
|
||||
logger.Debug(ctx, "ignoring control notification before armed")
|
||||
}
|
||||
}
|
||||
|
||||
controlCancel := p.subscribeChatControl(chatCtx, chat.ID, gatedCancel, logger)
|
||||
defer func() {
|
||||
if controlCancel != nil {
|
||||
controlCancel()
|
||||
@@ -3548,6 +3566,12 @@ func (p *Server) processChat(ctx context.Context, chat database.Chat) {
|
||||
Valid: true,
|
||||
})
|
||||
|
||||
// Arm the control subscriber. Closing the channel is a
|
||||
// happens-before guarantee in the Go memory model — any
|
||||
// notification dispatched after this point will correctly
|
||||
// interrupt processing.
|
||||
close(controlArmed)
|
||||
|
||||
// Determine the final status and last error to set when we're done.
|
||||
status := database.ChatStatusWaiting
|
||||
wasInterrupted := false
|
||||
|
||||
@@ -2018,3 +2018,95 @@ func chatMessageWithParts(parts []codersdk.ChatMessagePart) database.ChatMessage
|
||||
Content: pqtype.NullRawMessage{RawMessage: raw, Valid: true},
|
||||
}
|
||||
}
|
||||
|
||||
// TestProcessChat_IgnoresStaleControlNotification verifies that
|
||||
// processChat is not interrupted by a "pending" notification
|
||||
// published before processing begins. This is the race that caused
|
||||
// TestOpenAIReasoningWithWebSearchRoundTripStoreFalse to flake:
|
||||
// SendMessage publishes "pending" via PostgreSQL NOTIFY, and due
|
||||
// to async delivery the notification can arrive at the control
|
||||
// subscriber after it registers but before the processor publishes
|
||||
// "running".
|
||||
func TestProcessChat_IgnoresStaleControlNotification(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
|
||||
ctrl := gomock.NewController(t)
|
||||
db := dbmock.NewMockStore(ctrl)
|
||||
ps := dbpubsub.NewInMemory()
|
||||
clock := quartz.NewMock(t)
|
||||
|
||||
chatID := uuid.New()
|
||||
workerID := uuid.New()
|
||||
|
||||
server := &Server{
|
||||
db: db,
|
||||
logger: logger,
|
||||
pubsub: ps,
|
||||
clock: clock,
|
||||
workerID: workerID,
|
||||
chatHeartbeatInterval: time.Minute,
|
||||
configCache: newChatConfigCache(ctx, db, clock),
|
||||
}
|
||||
|
||||
// Publish a stale "pending" notification on the control channel
|
||||
// BEFORE processChat subscribes. In production this is the
|
||||
// notification from SendMessage that triggered the processing.
|
||||
staleNotify, err := json.Marshal(coderdpubsub.ChatStreamNotifyMessage{
|
||||
Status: string(database.ChatStatusPending),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
err = ps.Publish(coderdpubsub.ChatStreamNotifyChannel(chatID), staleNotify)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Track which status processChat writes during cleanup.
|
||||
var finalStatus database.ChatStatus
|
||||
cleanupDone := make(chan struct{})
|
||||
|
||||
// The deferred cleanup in processChat runs a transaction.
|
||||
db.EXPECT().InTx(gomock.Any(), gomock.Any()).DoAndReturn(
|
||||
func(fn func(database.Store) error, _ *database.TxOptions) error {
|
||||
return fn(db)
|
||||
},
|
||||
)
|
||||
db.EXPECT().GetChatByIDForUpdate(gomock.Any(), chatID).Return(
|
||||
database.Chat{ID: chatID, Status: database.ChatStatusRunning, WorkerID: uuid.NullUUID{UUID: workerID, Valid: true}}, nil,
|
||||
)
|
||||
db.EXPECT().UpdateChatStatus(gomock.Any(), gomock.Any()).DoAndReturn(
|
||||
func(_ context.Context, params database.UpdateChatStatusParams) (database.Chat, error) {
|
||||
finalStatus = params.Status
|
||||
close(cleanupDone)
|
||||
return database.Chat{ID: chatID, Status: params.Status}, nil
|
||||
},
|
||||
)
|
||||
|
||||
// resolveChatModel fails immediately — that's fine, we only
|
||||
// need processChat to get past initialization without being
|
||||
// interrupted by the stale notification.
|
||||
db.EXPECT().GetChatModelConfigByID(gomock.Any(), gomock.Any()).Return(
|
||||
database.ChatModelConfig{}, xerrors.New("no model configured"),
|
||||
).AnyTimes()
|
||||
db.EXPECT().GetEnabledChatProviders(gomock.Any()).Return(nil, nil).AnyTimes()
|
||||
db.EXPECT().GetEnabledChatModelConfigs(gomock.Any()).Return(nil, nil).AnyTimes()
|
||||
db.EXPECT().GetChatUsageLimitConfig(gomock.Any()).Return(
|
||||
database.ChatUsageLimitConfig{}, sql.ErrNoRows,
|
||||
).AnyTimes()
|
||||
db.EXPECT().GetChatMessagesForPromptByChatID(gomock.Any(), chatID).Return(nil, nil).AnyTimes()
|
||||
|
||||
chat := database.Chat{ID: chatID, LastModelConfigID: uuid.New()}
|
||||
go server.processChat(ctx, chat)
|
||||
|
||||
select {
|
||||
case <-cleanupDone:
|
||||
case <-ctx.Done():
|
||||
t.Fatal("processChat did not complete")
|
||||
}
|
||||
|
||||
// If the stale notification interrupted us, status would be
|
||||
// "waiting" (the ErrInterrupted path). Since the gate blocked
|
||||
// it, processChat reached runChat, which failed on model
|
||||
// resolution → status is "error".
|
||||
require.Equal(t, database.ChatStatusError, finalStatus,
|
||||
"processChat should have reached runChat (error), not been interrupted (waiting)")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user