Repository navigation
Conversation
ad7a0f3 to
f9b402f
Compare
a82cfd8 to
381fb14
Compare
381fb14 to
cb8ddd6
Compare
|
@codex review |
|
Chat: Review posted | View chat Review history
deep-review v0.13.0 | Round 7 | Last posted: Round 7, 32 findings (9 P3, 14 P4, 9 Nit), COMMENT. Review Finding inventoryFinding inventory: PR #29662Findings
Contested and acknowledgedCRF-5 (P3, useChatStore.ts:664) - Reconnect cursor never lowered when a stale REST page rolls the store back
CRF-10 (P3, AgentChatPage.tsx:296) - No test covers the page passing history_version to useChatStore
CRF-30 (P4, useChatStore.ts:580) - Full reset upserts earlier-frame messages then replaces them
CRF-33 (Nit, exp_chats_test.go:10270) - Comment repeats the assertion message
CRF-35 (Nit, stream_loop_internal_test.go:503) - reset(messages int) parameter name
CRF-10 R3 update
R3 panel closures
CRF-44 (P4, codersdk/chats.go:3163) - StreamChatOptions.HistoryVersion pointer adds nil state equal to zero
Law analysisR1: effective +1163 (head 6787013). Verdict: Don't split. Enforcement: Advisory. Round logRound 1Full panel (round 1). Netero + Law first (Law: don't split, advisory). Panel: ging-go, ging-ts, ging-react, nami, melody, ryosuke, killua (specialists), pariston, mafuuu (core), bisky, hisoka, leorio (pool), razor (wildcard). Gon deferred to next panel round to hold the 12-reviewer target; Knuckle and Takumi not triggered (no query/schema or goroutine changes). CRF-5 is convergent (Hisoka, Melody, Mafuuu; Nami listed the root race as OOS); kept at P3 because on main the same stale state also persisted until a reconnect or refetch, so the PR's delta is the lost repair on reconnect. Reviewed against 5552152..6787013. Round 2Restructure round (exported SDK types changed: ChatStreamStatus.HistoryVersion removed, StreamChatOptions.AfterRevision renamed to HistoryVersion, ChatStreamPreviewReset added; wire param renamed). Churn guard PROCEED: 12 addressed, CRF-10 contested, CRF-5 deferred to #30478. Law skipped (+37 effective). Netero: no new findings, CRF-1/CRF-2 verified. Full panel: ging-go, ging-ts, ging-react, nami, melody, ryosuke, killua, pariston, mafuuu, gon (once-per-PR floor), leorio, hisoka, kite (wildcard), plus bisky (dispute check-in, R1 agent reused). CRF-10 re-raised by Bisky (0/1 accept). CRF-12 re-raised by Leorio (event field docs still missing; author declined that part). Gon P2 (redundant comment) downgraded to Nit: keep-argument is noise in test, but no behavior or reader trap. Gon P3 (hasHistoryVersion naming) downgraded to P4: keep-argument is a plausible refactor breaking old clients, but the field comment states the meaning and no current behavior is wrong. Reviewed against c9680e4..0e0a8ec. Round 3Fix round (no exported signature changes; SDK edits are comments; one behavior commit 8c68ff3 in useChatStore). Churn guard PROCEED: 6 addressed, 4 contested, CRF-5 deferred. Law skipped (+76 effective since R2). Netero: no findings. Panel: bisky, nami, gon (dispute check-ins, prior agents messaged), leorio, mafuuu, ging-go (fix verification), ging-ts, ging-react, ryosuke, hisoka (files/behavior changed since R2), chopper (wildcard). Workspace agent disconnected mid-round; after restart the 10 reviewers without output files were resumed via message_agent with the same prompt (Gon had already written). All four contested findings closed 1/1. New: 1 P3, 1 P4, 1 Nit. Reviewed against a4b68ac..f19806a. Round 4Fix round (no new files or exported signature changes; one behavior commit bfdc902 writes the preview_reset version into cached page 0, resolving CRF-4 and a Codex thread). Churn guard PROCEED: CRF-39, CRF-40, CRF-41 addressed; CRF-5 deferred. Law skipped (+137 effective since R3). Netero: no findings, both new tests mutation-verified. Panel: bisky, leorio, chopper (fix verification), nami, ging-ts, ging-react, ging-go (changed files), hisoka (new cache state), zoro (wildcard). All three fixes verified. New: 2 P4, 1 Nit (convergent). Zoro P4 on the HistoryVersion pointer kept at P4: keep-argument is the generated number|null and caller zero-checks on an experimental API that is cheap to change now; counter-argument is the sibling AfterID *int64, which does not remove the meaningless state. Reviewed against 6bae268..bfdc902. Round 5Fix round (refactor and doc comments only since R4). Churn guard PROCEED: CRF-43, CRF-45 addressed; CRF-44 contested; CRF-5 deferred. Netero: no findings. Panel: zoro (dispute check-in, R4 agent messaged), nami, hisoka, leorio, ging-ts, ging-react, knov (wildcard). CRF-44 closed 1/1. Codex thread PRRT_kwDOGkVX1s6q1d9h (empty chat at version 0) refuted by Nami, Hisoka, Knov; orchestrator confirmed insertChat rejects empty InitialMessages (transitions.go:126). Knov raised CRF-3 at P4 claiming revisions between chat version and the client value are permanently lost; orchestrator disproved that mechanism (applyDBSnapshot sets l.state.historyVersion = chat.HistoryVersion unconditionally at stream_loop.go:361, so later syncs use the real version). Kept at P4 as CRF-47 with the residual consequence (rows at or below the chat's version the client never held are silently never sent; public parameter, one-comparison fix). Keep-argument for P3: silent incomplete history on a public API; settled P4 because only a caller sending a version it never read hits it. Reviewed against 782fb30..a73dc4e. Round 6Fix round (one validation check, its test, doc text). Churn guard PROCEED: CRF-47, CRF-48 addressed; CRF-5 deferred. Netero: no findings. Panel: knov, leorio (fix verification), bisky, chopper, hisoka, ging-go, zoro (wildcard). Both fixes verified; > vs >= boundary pinned (Bisky, Chopper mutation). New: 1 P4 (error detail), 1 Note (DB rollback retry loop, no change requested). Reviewed against 782fb30..77ab16b. Round 7Fix round (CRF-51 detail text, two comments). Churn guard PROCEED: CRF-51 addressed; CRF-5 deferred. Netero: no findings. Panel: leorio, hisoka (raised CRF-51), gon (comments), chopper, kite (wildcard). Gon first reported its prompt missing (read during select); re-sent the same prompt and it completed. CRF-51 verified. Gon P2 on the patchNewestPage comment downgraded to Nit: keep-argument is the misleading reference-vs-equality contract for a future caller; downgrade because the cost of misreading is one redundant setQueryData, and Leorio R5 judged the comment accurate. Gon AfterID doc item is pre-existing text (OOS). Reviewed against 782fb30..698f065. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
This PR adds a history-version cursor to the chat stream and a partial history_reset that starts at the lowest changed message. Findings: 7 P3, 5 P4, 2 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:298: a messages refetch orfetchNextPagethat resolves after stream events writes pages read before those events into the cache and store, so durable messages disappear or deleted messages reappear until the next refetch.coderd/x/chatd/stream_loop.go:233: a chat inrequires_actionreads its whole visible history on every sync while that status persists.coderd/database/queries/chats.sql:451:GetChatMessagesByRevisionForStreamhas no(chat_id, revision)index and scans every row of the chat on each sync with a history change.
Notes:
coderd/exp_chats.go:3550: anafter_revisionabove the chat'shistory_versionis accepted; that stream gets no message events until the chat's version passes it. The web client only sends versions it read.site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:183: on remount with cached messages, the stream opens at the cached page's originalhistory_version, which resends more than needed but loses nothing.coderd/x/chatd/stream_loop.go:100: advancing the cursor in thestatushandler is safe only because every non-messageevent callscommitHistoryReplacement()first; nothing at either site states that dependency.
🤖 This review was automatically generated with Coder Agents.
| if (!nextStatus) { | ||
| continue; | ||
| } | ||
| const syncedVersion = streamEvent.status?.history_version; |
There was a problem hiding this comment.
P3 [CRF-5] historyVersionRef only moves forward from stream status events, but REST page responses can roll the store back to older state, and a reconnect no longer repairs it. (Hisoka, Melody, Mafuuu)
Example: deleteChatQueuedMessage awaits invalidateChatMessages during a running turn; the assistant message and status: waiting arrive while the refetch is in flight, the ref advances, and the stale response then drops the message from cache and store (the hydration effect at line 298 sees an ID the previous sync held). A page read before another tab's edit can likewise put deleted rows back. On main the next reconnect sent after_id from the stale tail or a full reset and restored the state; now it sends the newer after_revision and the server sends nothing. Fix: return history_version on every page and lower the ref to min(ref, page.history_version) when a page is written into the cache.
🤖
There was a problem hiding this comment.
The race is on main, so the fix is its own PR: #30478. Cache writes now cancel a background refetch first, as watch events already do for the chat list. A pagination fetch in flight during a stream write can still overwrite it; #30478 explains why that case is left alone.
🤖 Posted using
/amend-reviewskill via Coder Agents.
6787013 to
0e0a8ec
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Round 2. The reconnect version now rides on preview_reset, the stream parameter is renamed to history_version, and most round 1 findings are fixed and verified. New and re-raised findings: 2 P3, 4 P4, 4 Nit.
Verified fixed: the requires_action read, the first-row reset start, the mid-turn resend, the version on after_id streams, the "non-zero" SDK and swagger text, the live-edit test wait, the removed commitHistoryReplacement() call, the from_message_id doc, the connect() comment, the rename, and both Nits. The stale-page rollback is deferred to #30478. The missing page-level test for history_version and the ChatStreamEvent field docs are re-raised on their threads.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:298: a messages refetch orfetchNextPagethat resolves after stream events writes older pages into the cache and store (partly addressed by #30478).coderd/x/chatd/stream_loop.go:223: a chat inrequires_actionreads its whole visible history on every sync while that status persists.coderd/database/queries/chats.sql:451:GetChatMessagesByRevisionForStreamhas no(chat_id, revision)index and scans every row of the chat on each sync with a history change.site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:239:replaceCacheMessagesdoes not invalidate chat prompts, so after another tab edits a prompt, this tab's prompt history keeps the deleted prompt untilstaleTimeexpires.site/src/api/queries/chats.ts:2350: theeditChatMessageonSettledcomment says the WebSocket delivers post-edit messages "via FullRefresh", a mechanism that does not exist.
Notes:
coderd/exp_chats.go:3551: ahistory_versionabove the chat's version is accepted; that stream gets no message events until the chat's version passes it.site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:183: on remount with cached messages, the stream opens at the cached page's original version, which resends more than needed but loses nothing.
🤖 This review was automatically generated with Coder Agents.
0e0a8ec to
f19806a
Compare
|
@codex review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f19806a14c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/coder-agents-review
|
There was a problem hiding this comment.
Round 3. The fixes for the round 2 findings are verified, and the four contested findings are closed. New findings: 1 P3, 1 P4, 1 Nit.
Verified fixed: the ChatStreamEvent field docs, the vacuous page-refetch test step, waitForChatStatus bounded by its context, the ARCHITECTURE.md "non-zero" definition, the openedWithHistoryVersion rename, and the restored reset comment. 8c68ff3 (a chat opened without a version stays on after_id) is correct, and its test fails against the previous code. The stale-page rollback stays deferred to #30478.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:298: a messages refetch orfetchNextPagethat resolves after stream events writes older pages into the cache and store (partly addressed by #30478).coderd/x/chatd/stream_loop.go:223: a chat inrequires_actionreads its whole visible history on every sync while that status persists.coderd/database/queries/chats.sql:451:GetChatMessagesByRevisionForStreamhas no(chat_id, revision)index and scans every row of the chat on each sync with a history change.site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:239:replaceCacheMessagesdoes not invalidate chat prompts, so after another tab edits a prompt, this tab's prompt history keeps the deleted prompt untilstaleTimeexpires. The upsert of earlier-frame messages refreshes prompts only when one of them is a user message.site/src/api/queries/chats.ts:2350: theeditChatMessageonSettledcomment says the WebSocket delivers post-edit messages "via FullRefresh", a mechanism that does not exist.
Notes:
coderd/exp_chats.go:3551: ahistory_versionabove the chat's version is accepted; that stream gets no message events until the chat's version passes it.site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:183: on remount with cached messages, the stream opens at the cached page's original version, which resends more than needed but loses nothing.
🤖 This review was automatically generated with Coder Agents.
f19806a to
bfdc902
Compare
|
@codex review
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Round 4. The three round 3 fixes are verified, and bfdc902 now reopens a remounted chat at its last synchronized version; its test fails when the cache write is removed. New findings: 2 P4, 1 Nit.
Verified fixed: the !pageLoaded test, the openedWithHistoryVersion comment, and the applyUntil failure message. The stale-page rollback stays deferred to #30478.
Out of scope (needs a ticket or explicit acceptance by a human):
site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:298: a messages refetch orfetchNextPagethat resolves after stream events writes older pages into the cache and store (partly addressed by #30478).coderd/x/chatd/stream_loop.go:223: a chat inrequires_actionreads its whole visible history on every sync while that status persists.coderd/database/queries/chats.sql:451:GetChatMessagesByRevisionForStreamhas no(chat_id, revision)index and scans every row of the chat on each sync with a history change.site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts:239:replaceCacheMessagesdoes not invalidate chat prompts, so after another tab edits a prompt, this tab's prompt history keeps the deleted prompt untilstaleTimeexpires.site/src/api/queries/chats.ts:2350: theeditChatMessageonSettledcomment says the WebSocket delivers post-edit messages "via FullRefresh", a mechanism that does not exist.
Notes:
coderd/exp_chats.go:3551: ahistory_versionabove the chat's version is accepted; that stream gets no message events until the chat's version passes it.
🤖 This review was automatically generated with Coder Agents.
A chat's snapshot version only grows, so a cursor above it was never issued for the chat; a stream from it would skip every change until the chat passed it. Reject it like any other invalid parameter.
A client showed the turn from four events with separate rules: status on a change, error only when the history also advanced, retry until the next part, message, status or error, and action_required. Every checkpoint now carries the turn (status, and the error or pending retry that status shows), and the newest messages page returns it at its snapshot version, so a client replaces its turn and needs no rules. Streams opened with snapshot_version no longer get the status, error, retry and action_required events, and their syncs skip the full-history read action_required needed; no client in the repository reads that event. Streams opened without it keep the events.
Clients cleared their live output on the retry and error events, and the web client dropped parts after a waiting status. Streams opened with snapshot_version get neither event, and a client should not have to infer when an attempt ends. The stream now sends preview_reset when it retires the current attempt and on the first sync of every connection, whose parts it replays from the start. It also retires the attempt when the chat stops running. A turn that finishes without committing a message keeps its history version and attempt, so its buffered parts still matched the stream's episode and reached clients after the turn ended.
When the pubsub subscription or the first read failed, the handler accepted the socket and sent one error event, the event that carries a chat's turn error, and the web client showed the chat as errored. The handler's comment already meant to report setup failures as HTTP errors, but SubscribeAuthorized never returned its failure branch. SubscribeAuthorized now returns the error. The handler answers 404 when the chat was deleted after the route loaded it, writes nothing when the client went away, and answers 500 otherwise.
Chat actions now set Coder-Chat-Snapshot-Version instead of carrying snapshot_version in their bodies. Delete-queued and tool-results return 204 again and promote returns 202, as on main, so existing clients, including the Go SDK SubmitToolResults that requires 204, keep working. Chat no longer exposes snapshot_version; the newest messages page and the checkpoint are where a client reads it.
The stream section still described the history_version cursor, which the stream no longer accepts. It now covers the snapshot_version parameter, the turn on the checkpoint and the newest page, the action response header, preview_reset on the first sync and on retirement, and setup failures as HTTP errors.
applyDBSnapshot treated a history reset as a history change. A reset is found only among rows read because the chat's history version is above the stream's, so the check never adds a case since the full reset for a cursor above the chat was removed.
NonPositiveSnapshotVersionReturns400 only sent 0.
The chatd tests show that SubscribeAuthorized returns setup failures, but nothing checked what the handler answers. A chat that is gone when the stream first reads it must get a 404 and any other read failure a 500, both before the upgrade.
The header let a client end an eager update exactly at the commit it stood for. A client can instead let the next stream update of the same kind replace the eager item, which needs nothing from the response. The action handlers and SubmitToolResults, DeleteQueued and PromoteQueued are as on main again.
A commit that changed no field the stream tracks, such as an archive, sent a checkpoint built from the cached turn, so every committed version reached every stream for the action header to match. Without the header nothing reads that checkpoint, and the cached turn was only correct while last_error changed together with a tracked field. Such a hint is a no-op again, as on main.
Only ARCHITECTURE.md said that such a stream sends no status, error, retry or action_required events. A Go SDK or API client that sets the version for the smaller first sync would lose them without warning.
The SDK doc said pending dynamic tool calls are read from the chat watch stream, and the parameter doc said the checkpoint replaces action_required. Neither carries them: the watch event sets only its kind and the chat, and the turn has no tool calls.
The comment named a web client behavior that no longer exists: the client no longer stores send responses.
A stream opened with snapshot_version sends no action_required events, and its checkpoint turn left out the tool calls, so such a client could not learn which calls wait for results. The turn now carries them while the status is requires_action, on the checkpoint and on the newest messages page, read from the history the same way action_required reads them.
The added sentences restated the error result.
Clients resume a stream from the version of the last change they applied, so each message, queue_update and history_reset event needs the version of the commit that made it.
The checkpoint made clients buffer a sync's events until its end, though each change already has the version of the commit that made it. Streams opened with snapshot_version now stamp message, queue_update and history_reset with that version, send them in version order, and resume at or after it, since a connection can drop between the events of one commit. They get main's status, error, retry, action_required and preview_reset events in place of the checkpoint's turn. An edit soft-deletes every message from the edited one on, so each edit becomes one history_reset from the lowest ID it removed, and every live message at or above that ID was written by the edit or a later commit. The stream no longer reads the history after a reset.
The stream section still described the checkpoint, its turn, and the extra preview_reset events, which the stream no longer sends.
b60952b to
9a3d397
Compare
Nothing calls it, and it soft-deletes rows outside a state machine transition and from the middle of the history. Streams opened with snapshot_version rely on every soft-delete removing a message and every later one, which only EditMessage does.
DeleteChatQueuedMessage, DeleteAllChatQueuedMessages, PopNextQueuedMessage and ReorderChatQueuedMessageToFront have no callers. A write through them would change the queue outside a state machine transition, without a new snapshot version for streams to resume from.
The setup-failure fix needs only subscribeWithInitialError gone: its three callers now return false, and the handler already answers false with 500 before the upgrade. Subscribe keeps main's bool result, and the snapshot version is a second parameter instead of a cursor struct. A chat deleted between the route and the subscribe now gets 500, not 404. The invalid snapshot_version test also covers a non-integer value.
The stream reference listed snapshot_version and the new event fields but not the rules a client needs to resume from them: where the value comes from, what the first read sends, the event order, the 400 and 500 cases, and why after_id resends the whole history after an edit. The parameter text moves into the endpoint description, which the generator renders above the parameter table.
The migration guide told clients to reconnect with after_id. Once any message in a chat was edited, a stream opened with after_id sends the whole history on every connection, so the guide now walks through resuming with snapshot_version and the client rules that go with it. The event table gains history_reset, preview_reset and action_required, and the frame description matches what the server sends: a JSON array of events per WebSocket message, with no envelope.
The migration test step described JSON envelopes with a data type. The server writes each batch of events as a bare JSON array.
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
…rsor (#29761) Opening a chat on the Agents page loads its transcript over REST and then connects the chat stream with `after_id` set to the newest loaded message. The first sync of the stream ignores that cursor when it queries the database: it fetches every row of the chat on each connection, and then sends only the rows above `after_id`. The first sync now resolves `after_id` to that message's revision and fetches only rows changed at or after it. The history reset rule is the one on `main` (#29808): the stream resets only when message `after_id` is deleted or missing. A deleted cursor still produces one `history_reset` with the surviving history, because the bounded query includes the cursor row. An unknown ID keeps the full query. A message ID from another chat, live or deleted, is ignored, so the client receives the full history. Live updates after the first sync are unchanged. ## Performance Measured on dogfood's database, which runs `main`, with `EXPLAIN (ANALYZE, BUFFERS, SERIALIZE BINARY)` of the first-sync query. The chats are the 554 chats updated in the past day, picked by row count. `main` starts at revision 0. This PR starts at the revision of the newest live message, which is the `after_id` the web client sends. | Chat (rows) | `main` output | `main` first / repeat run | This PR output | This PR time | |---|---|---|---|---| | p50 (41) | 189 kB | 21.5 ms / 0.6 ms | 1 kB | 0.1 ms | | p90 (351) | 2.0 MB | 163 ms / 7.9 ms | 10 kB | 0.5 ms | | p99 (2,200) | 7.1 MB | 425 ms / 25 ms | 14 kB | 3.0 ms | | Largest (14,746) | 64.9 MB | 4.8 s / 193 ms | 6 kB | 17.6 ms | - Across the 554 chats, the bounded query returns 1 row up to p99 and 5 rows at most. - Both queries visit the same heap pages, because no index covers `revision`. The saving is the TOAST reads, serialization, transfer, and decoding of rows that `main` then discards. Most of `main`'s time is serialization: 153 ms of 193 ms for the largest chat. - The cursor lookup is one primary-key read of about 0.4 ms, so a median chat gains almost no latency. The gain grows with chat size. - Deleted rows are about 0.1% of rows, so they do not change the cost. ## Not changed A tab whose stream was down can miss an edit, then send a message and store the response, so its newest ID is above the edit. On reconnect, message `after_id` is live and the stream sends no reset, so the tab keeps the deleted messages. `main` has done the same since #29808, and this PR does not change it (mafredri's reproduction gives `expected [5 6 7 8], actual [3 4 7 8]` on both). An ID cannot show which edits the client applied. #29662 adds a client-reported version for that. <details> <summary>Delivery record</summary> - Rebased on `main` at `6ebefb83254`, keeping the #29808 reset rule and #30266 read-only snapshot reads. The cursor-bound mock test is part of `TestStreamLoopInitialSyncTombstones`. - Assumption checked on dogfood: no live message has a higher revision than a later message. Messages are only updated to soft-delete them or to set search columns, which the revision trigger ignores. 2,861 rows in 29 recently active chats break it, all created between 2026-06-26 and 2026-07-16 and none since. A client cursor cannot sit before them, because the client sends its newest message. - Local: `go test ./coderd/x/chatd/...`, `go test ./coderd -run '^TestStreamChat$'`, `go test ./coderd/database/dbauthz -run '^TestMethodTestSuite$'`. Mutants of the cursor bound (no `- 1`, no bound, cursor from another chat kept) each fail `TestStreamLoop*`. - Remote UAT before the rebase covered deleted, foreign, and unknown cursors, edits, grouped commits, and reconnects. On a synthetic 5,000-message chat, dial to first snapshot went from 129.6 ms to 15.0 ms. - Codex: [round 1](#29761 (review)) and [round 2](#29761 (review)) found cross-chat cursor bugs, fixed. Round 3 and the round on `7bed7855e41` found nothing. </details> > Xum opened this PR on behalf of @ibetitsmike.
On
main, an edit soft-deletes the edited message and every later one, and the deleted rows stay in the chat. After that, every connection to the chat stream (GET /api/v2/chats/{chat}/stream) reads a deleted row and sendshistory_resetfollowed by every visible message, whateverafter_idthe client sent. No messages page or stream event includes the chat'ssnapshot_version, so a client cannot order a page relative to stream events or resume a stream where it stopped.This PR adds
snapshot_versionto the newest messages page and to the stream'smessage,queue_updateandhistory_resetevents, and lets a client open the stream at the version it holds. It implements H1 to H5 of the RFC Chat stream contract: one copy of each chat in the web client, which describes the design and the alternatives. The RFC is in review, and this PR will change to match its outcome. #30478 changes the web client to use the new stream behavior.Events the stream sends on connect, up to its first
statusevent, for the three edited chats of the chat bench from #29596. The client loads the newest page, then opens the stream with the cursor from that page:mainatad85646164,after_id= newest message IDsnapshot_versionfrom the pagehistory_resetand 306 messages, 23.9 MBstatus, 99 Bhistory_resetand 306 messages, 23.9 MBstatus, 99 Bhistory_resetand 584 messages, 3.1 MBstatus, 99 BThe latest commit of each chat changed no message and no queue, so the stream had nothing to send again. A client that does not send
snapshot_versiongets the same full history from this PR as frommain. The open PRs #29808 and #29761 change that behavior, and this PR does not depend on either.API changes
All changes are additive. A stream opened without
snapshot_versionsends the same events as onmain; only its setup failures change.GET /chats/{chat}/messageswithoutbefore_idorafter_idsnapshot_version. The handler reads it with the chat row, before the messages and the queue, so a change committed between the reads is in the page and the stream sends it again.GET /chats/{chat}/stream?snapshot_version=Safter_id. It returns 400 for an S that is not an integer, is below 1, or is above the chat's version.message,queue_updateandhistory_reseton that streamsnapshot_version: the message'srevision, the chat'squeue_version, or the revision of the rows the edit deleted. Each read sends them in version order. Within one version it sends the reset, then the messages by ID, then the queue.history_reseton that streamfrom_message_id, the lowest ID the edit deleted. Only messages written by that edit or a later commit follow it.main, the server upgrades and sends oneerrorevent, which the web client shows as a chat error.StreamChatOptions.SnapshotVersion.The PR also deletes five queries that write messages or the queue outside a chat state machine transition and that no code calls:
SoftDeleteContextFileMessages,DeleteChatQueuedMessage,DeleteAllChatQueuedMessages,PopNextQueuedMessageandReorderChatQueuedMessageToFront. A write outside a transition does not incrementsnapshot_version, so a stream can miss it.The API reference of the stream endpoint now describes the order of events, the resend rules and the errors, from a new
@DescriptiononstreamChat. Step 4 of the Tasks-to-Chats migration guide now lists the client rules. Before this PR, it told clients to reconnect withafter_id, which sends the whole history after an edit, and it described the frames asping,dataanderrorenvelopes. The server sends each frame as a JSON array of events, onmaintoo.Tests
TestStreamChat/EditedChatSyncsOnlyChangedHistorysnapshot_versiongets the full history, and no event includes a version.TestStreamChat/ReconnectReceivesMessagesSentElsewhereTestStreamChat/InvalidSnapshotVersionReturns400,TestStreamChat/SnapshotVersionAboveChatReturns4000,-1,abc, and a version above the chat's.TestStreamChat/SetupFailureReturnsHTTPErrorTestGetChatMessages_Pagination/SnapshotVersionOnlyOnUncursoredPagesnapshot_version.TestStreamLoopSnapshotVersionTestStreamLoopSnapshotVersionOrder,TestStreamLoopSnapshotVersionTurnEventssnapshot_version.Lines against
main: Go source +204/−35 (+40 of it is the swagger description), the deleted queries and their authorization wrappers −88, tests +769/−67,ARCHITECTURE.md+17/−2, docs +44/−17, generated +201/−229.Not covered
main.snapshot_version, the stream returns 400 to a client that holds a higher version. The user must reload the page.Decision log
Implementation choices that the RFC does not state:
queue_updatecontains the whole queue.snapshot_version, the server ignoresafter_id, because a client can hold a message ID without holding every lower one.ReconnectReceivesMessagesSentElsewherecovers that case.snapshot_version, the loop does not read the full history for a reset.main, then sorts them with a stable sort by version. The stable sort keeps the reset, message and queue order within one version.subscribeWithInitialErroris deleted.Subscribereturnsok == falsefor a setup failure, and the handler returns its existing 500 response.Related: #29596 (rendering performance, where this problem was found), #29661 (interrupted turns stored one part per token, which is why two of the chats above are 24 MB).