fix(coderd/x/chatd): wake after auto-promoting queued message#24714
Conversation
|
/coder-agents-review |
There was a problem hiding this comment.
Clean, well-scoped fix. The production change completes the pending → signalWake contract that all five other entry points already follow. The test redesign is sharp: disabling the ticker and synchronizing on a channel proves the wake is both necessary and sufficient. Netero verified empirically that reverting the fix causes the test to fail with context timeout.
1 P3, 3 Nits. All minor.
"I tried to build a case against this PR and couldn't. The problem is correctly diagnosed, the solution is proportional, the fix is at the right causal level, and the test proves the fix is both necessary and sufficient." — Pariston
coderd/x/chatd/chatd.go:190
Nit [DEREM-1] This comment enumerates four callers (SendMessage, EditMessage, CreateChat, PromoteQueued), but signalWake is now called from six sites: the four listed, SubmitToolResults (line 2178, pre-existing omission), and the auto-promote path added by this PR (line 5213).
Since this PR adds a new caller, the comment is directly in scope to update. Consider replacing the enumeration with intent:
// wakeCh is signaled whenever a chat transitions to
// pending so the run loop calls processOnce immediately
// instead of waiting for the next ticker.This avoids the comment going stale each time a new call site appears.
(Hisoka, Mafuuu, Ryosuke, Meruem, Chopper, Knov)
🤖
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All four R1 findings addressed in b2d9c63. The DEREM-1 fix is notably good: rather than updating the caller count from 4 to 6, the comment was generalized to describe the invariant ("signaled whenever a chat transitions to pending"), eliminating the staleness class entirely.
12-reviewer panel, zero new findings. The production fix completes the pending → signalWake contract. The test proves the fix is necessary by construction. Clean.
"I tried to build a case against this and couldn't. The problem is correctly understood: a missing wake signal in the one path that transitions to pending without waking." — Pariston
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
R3 moved signalWake after shouldPublishFinishedChatState, aligning with PromoteQueued's publish-then-wake ordering. Applied the deterministic test pattern to the two remaining sibling tests (TestAutoPromoteQueuedMessagesPreservesPerTurnModelOrder and TestInterruptAutoPromotionIgnoresLaterUsageLimitIncrease), replacing clock.Advance and require.Eventually with channel-gated synchronization. All three auto-promote chain tests now prove signalWake works by construction.
14-reviewer panel, zero code findings across 3 rounds. The production fix is 6 lines, pattern-consistent, and at the right causal level. Tests are deterministic.
"I traced all six signalWake sites. Each is preceded by a transition to ChatStatusPending. No signalWake call exists without a preceding pending transition." — Razor
🤖 This review was automatically generated with Coder Agents.
775a5e7 to
6eca6cc
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Rebase-only round (diff unchanged from R3, new base 056203f). 5-reviewer panel confirmed all prior fixes intact, zero new findings. CI pending.
4 rounds, 0 open findings. The production fix completes the pending-to-signalWake contract. Tests are deterministic by construction.
🤖 This review was automatically generated with Coder Agents.
tryAutoPromoteQueuedMessageinprocessChat's deferred cleanup could set a chat back topendingwithout waking the processor. The processor only noticed on the next 10ms poll, so under load tests likeTestAutoPromoteQueuedMessageFallsBackForInvalidQueuedModelConfigIDcould time out waiting for the second streaming request (#1500).Call
p.signalWake()after the promoted-message publishes whenpromotedMessage != nil, matching the pattern used byCreateChat,SendMessage,EditMessage,PromoteQueued, andInterruptChat. Make the regression helpertestAutoPromoteQueuedMessageFallbackdeterministic by settingPendingChatAcquireInterval = time.Hourand synchronizing on asecondRunStartedchannel instead of pollingrequestCount, so the test fails without the wake instead of relying on the 10ms ticker.Closes coder/internal#1500