Keep the per-thread realtime channel alive across thread-row echoes - #548
Conversation
sysread
left a comment
sysread
left a comment
There was a problem hiding this comment.
Landru is thinking on behalf of Jeff:
Full review of the channel-churn fix. Both mechanisms check out as necessary and correctly shaped, and the stop-then-edit chain holds at every link (draft -> materialize -> stop -> edit -> echo -> transcript). Five findings inline and one PR-body item below; the good parts first.
Highlights:
- The boolean projection is the load-bearing shape (src/screens/Chat.svelte:1881). The obvious fix, reading
currentThread.isDraftin the effect, silently fails:currentThreadis object-valued, deriveds compare with strict===, and row patches produce new references, so the effect would churn again in the same failure window. Projecting to a boolean is the non-obvious correct move and the comment names the mechanism. - The topic comment preempts the predictable objection (src/lib/supabase/realtime.ts:91).
unique topics might fork or leak the server-side subscriptionis exactly what a skimming reviewer would object to; the comment answers it with the full collision mechanics. - QA step 8 teaches the failure mode, not just the symptom (docs/qa/use-cases/chat-edit-user-message.md:52). The busiest-echo-window rationale means a future failure points at the realtime channel instead of the edit RPC, and the NOT RUN results row is honest about the pending manual pass.
- The test owns exactly our half of the contract (tests/realtime-subscribe.test.ts). Topic uniqueness is what our code controls; realtime-js registry behavior is delegated to the QA walkthrough as a documented coverage split, and the docblock names the hazard and the user-visible stake.
PR body: the trailing _Generated by Claude Code_ footer violates the repo rule of no AI attribution in commits or PRs (PRs 543 and 546 carry none). Drop it from the body.
sysread
left a comment
sysread
left a comment
There was a problem hiding this comment.
Landru is thinking on behalf of Jeff:
Two pre-existing items from the same review, added at Jeff's direction: fix both in this PR rather than deferring them. They share one mechanism - a post-edit-commit re-fetch of the thread's messages, merged through the position sort, backstops the missing replacement row and makes the live ordering deterministic in one move.
Editing the first message of a new conversation after a stop made the old message fade out and the edited replacement never appear until a reload or a thread switch. The replacement row is inserted server-side by the commit RPC and reaches the open transcript only through the realtime INSERT echo, so a dead per-thread channel shows up exactly there - the normal send path appends the user row locally and takes the reply off the stream, which is why it never noticed. The channel died from churn. The messages-subscription effect read the thread list to check for a draft, which made it re-run on every thread-row echo (the commit's updated_at bump, auto-title, topics, priming-payload writes), tearing the channel down and re-creating it each time. realtime-js returns the still-leaving channel when a new one is requested under the same topic, and subscribe() on a channel that is not closed returns without joining, so the re-created subscription was a no-op and the thread lost its echo stream. A fresh conversation's first turn is the busiest window for those echoes, which is why this surfaced there first. The effect now keys on a memoized draft boolean so thread-list churn no longer re-runs it, and subscribeToMessages suffixes its topic per subscription so a same-thread resubscribe can never collide with a leaving channel. A unit test pins the unique-topic contract; the QA walkthrough gains the stop-then-edit-first-message step.
Review follow-ups on the channel-churn fix. The replacement user row a destructive edit inserts had no path into the live view except its realtime INSERT echo, and that echo raced the reply's END hydration to the tail of the transcript, so even with a live channel the edited message could land below its reply until a reload sorted it. runExchange now re-fetches the transcript after the old range fades out and merges it through the position sort, the same shape as the second-thoughts verdict backfill. That lands the row when the echo drops and makes the live order deterministic. The re-fetch is a named helper shared with the claim-release reconcile, which did the same thing inline. The per-user realtime effects keyed on the Session object, which is reassigned on every auth event (initial session, token refresh, the mount-time getSession resolve), so each auth event re-created every per-user channel inside the same leave-ack window that killed the messages channel. They now key on a memoized user-id string. Every postgres_changes helper also opens a unique topic per subscription; the Broadcast helpers cannot, because their topic is the address the edge functions publish to, so they rely on the memoized key alone. Doc fixes: the verdict has a re-fetch backstop and is no longer cited as echo-only; the topic counter sits above the helper docblock; the effect comment names currentThread as an unsafe read alongside the raw list; the editing gotcha links chat.md instead of naming it.
69f6ffe to
9e478a4
Compare
sysread
left a comment
sysread
left a comment
There was a problem hiding this comment.
Landru is thinking on behalf of Jeff:
Round-2 review of 9e478a4. The fix commit's mechanics all check out - the two-part channel fix, the sessionUserId conversion, and the reconcileTranscript backstop verify end to end, including the auth-js and realtime-js internals they rest on. What follows is text-level drift the fix left behind, plus two pre-existing holes in the same families, pulled into scope like last round. Nine findings inline; one here that has no diff line to anchor to.
Highlights:
- The
uniqueTopicdocblock names the invariant that makes the suffix safe for postgres_changes and silently destructive for Broadcast, with the publisher topics enumerated (src/lib/supabase/realtime.ts:49). - The test is two-sided: fresh topics asserted for the postgres helpers AND the stable topic pinned for the Broadcast helper. Asserting the deliberate non-change as a contract is what stops a future generalization from detaching the publishers.
- Dedup over addition: the claim-release inline copy collapsed into the shared
reconcileTranscriptinstead of a second copy appearing next to it. - The
sessionUserIdcomment pre-refutes thejust use sessionfalse read at the site (src/screens/Chat.svelte:1970). - The module preamble's now-false
no stateclaim was corrected rather than left to rot (src/lib/supabase/realtime.ts:20).
Mentorship notes (informational, not fix requests):
- The Broadcast-vs-postgres topic rule - a Broadcast topic is the publisher's address; a postgres_changes topic is arbitrary because the server-side filter scopes the stream - lives only in
uniqueTopic's docblock. One sentence in the module preamble'sTwo channel flavorsparagraph puts it where a future Broadcast helper author will actually look. - The value-projection-for-effect-keys pattern now has two instances (
activeThreadIsDraft,sessionUserId). Naming the rule once in docs/dev/frontend-organization.md - effects that own a teardown-able resource key on primitive projections, because deriveds compare with strict equality - shortens the next ten-line comment to one. scheduleVerdictBackfill's 20s delay next toreconcileTranscript's immediate fetch is deliberate: the verdict is written seconds after the turn commits, so an immediate re-fetch would miss it. A one-clause cross-reference at either site records the timing analysis where the next reader will need it.
No-anchor finding: LOW - doc ledger. mergeMessagesById now has a second non-selectThread caller from the chat side (the post-edit-commit reconcileTranscript in runExchange), but the Interactions entry in docs/dev/exchange.md:398-401 still names only the post-claim-release effect. The repo rule is that a new cross-feature call site updates the affected doc's Interactions section in the same PR. One line closes it.
Round-2 review follow-ups. The agent-runs Broadcast helper opened a channel per consumer on the one topic that cannot be made unique (it is the address the edge functions publish to). Two overlapping manual runs in one tab - the wiki librarian strip and a memory strip have independent busy guards - therefore got the same channel back from realtime-js, and the first run's teardown removed it out from under the survivor, which went deaf and timed out on its inactivity backstop while the server-side run completed. The helper now keeps one refcounted channel per user: events fan out to every live consumer, the channel closes when the last one leaves, and a consumer arriving during the leave waits for it to settle so it never joins a leaving channel. Two more members of the churn family: the offline-cache effect now keys on the memoized user id like the relays, and the reconnect settle path re-checks the active thread after its fetch so a mid-fetch thread switch cannot paint the old transcript under the new one. Text drift from the previous commit: three sites still described the replacement row as echo-only, which the post-commit re-fetch made false; the reconcile helper's docblock now names the snapshot window a row can fall into on the claim-release path and why merging the live array in is not the fix; the verdict backfill and the reconcile cross-reference each other's timing. The test pins every postgres_changes helper's fresh-topic contract, not two representatives, plus the shared agent-runs channel. Docs: the exchange ledger names the second mergeMessagesById caller, frontend-organization.md states the primitive-projection rule for resource-owning effects, the realtime preamble states the topic rule per channel flavor, the wiki and memory docs note the shared channel, and the QA log records the step-8 expansion.
The destructive-edit use-case's step 8 ran in a browser for the first time, against the local dev stack at 405e9fb: fresh conversation, first message, Stop mid-stream, destructive edit, resend. The edited row rendered directly above the new reply without a reload (the reported symptom did not reproduce), a reload held the same two rows, and a follow-up send kept the full order live. Steps 1-2 expectations validated along the way; steps 3-6 not re-run (PASS at 59ea99c and 743ed1d).
|
Landru is thinking on behalf of Jeff: Step 8 of the destructive-edit use-case ran in a browser for the first time, against the local dev stack at 405e9fb: fresh conversation, first message sent, Stop clicked mid-stream, destructive edit, resend. PASS on the reported symptom and both mechanisms under test:
Steps 1-2 expectations validated along the way (dropdown items, composer pre-population, regen-target outline). Steps 3-6 not re-run this pass (PASS at 59ea99c / 743ed1d). The results row is appended to the use-case log and pushed to this branch (53f3351). |
|
Landru is thinking on behalf of Jeff: Approving on Jeff's behalf - GitHub will not let the PR author file a formal APPROVE review on their own pull request, so this comment is the approval of record. All 16 review-thread findings across the three review rounds are fixed and verified in the code, every thread is resolved with a reply, and step 8 of the destructive-edit use-case passed in a browser for the first time (QA comment and results row on this PR). Merge-ready. |
2 participants
SYNOPSIS
Fix the edited replacement message never appearing after a stop-then-edit on a new conversation's first message. The per-thread realtime channel was dying from subscription churn, and the replacement row had no backstop behind its echo. Two siblings in the same family ride along: the per-user relays churned on auth events, and the agent-runs Broadcast channel was torn down under a concurrent consumer.
PURPOSE
Send the first message of a new conversation, hit Stop, choose Edit, resend. Currently the old message fades out and the edited one never shows up until a reload or a thread switch. Bad because the transcript lies: the DB has the edited row and the view has nothing above the reply.
DESCRIPTION
How it behaves today. The destructive edit does not insert the new user row from the browser. The commit RPC inserts it server-side and returns only the assistant row, so nothing on the stream or the send path appends it: its live delivery is the realtime INSERT echo on the per-thread messages channel. Nothing stands behind that echo, and it races the reply's END hydration to the tail of the transcript, so even a delivered echo can land the edited row below its reply.
That channel is opened by an effect keyed on the active thread. To skip drafts, the effect read the thread list directly (
findThread), which made it a dependent of every thread-row change: the commit'supdated_atbump, auto-title, topics tagging, priming-payload writes. Each change re-ran the effect, which tore the channel down and re-created it under the same topic. realtime-js hands back the still-leaving channel for a repeated topic, andsubscribe()on a channel that is not closed returns without joining. Result: the re-created subscription was a no-op and the thread had no live echo stream.A fresh conversation's first turn is the busiest window for those echoes, which is why this surfaced there first. The normal send never notices: the user row is appended locally and the reply arrives on the stream. The per-user relays (threads, wiki, memories, recipes, grocery, logs, mints) have the same exposure from a different key: their effects read the Session object, which is reassigned on every auth event, so each auth event re-created every per-user channel inside the same leave-ack window. And the agent-runs Broadcast helper opened a channel per consumer on a topic that cannot vary, so two overlapping manual runs (wiki librarian + a memory strip) shared one channel and the first run's teardown deafened the survivor until its inactivity timeout.
What this PR changes.
activeThreadIsDraft, a$derived), so thread-list churn recomputes a boolean that does not change and the effect stays putsessionUserId) instead of the Session object, so each channel is created once per signed-in useruniqueTopic), so a fast resubscribe can never collide w/ a leaving channel (the server-side filter, not the topic, scopes the stream)runExchangere-fetches the transcript (reconcileTranscript, shared w/ the claim-release reconcile) once the old range has faded out, merging through the position sort - lands the row if the echo drops and makes the live order deterministicchat.mdanduser-message-editing.md, the exchange ledger's secondmergeMessagesByIdcaller, a primitive-projection rule for resource-owning effects infrontend-organization.md, the topic rule per channel flavor in the realtime preamble, the shared channel inwiki.md/memory.md; QA walkthrough gains step 8 (stop-then-edit the first message of a new conversation)Why that fixes it. The channel is only re-created on an actual thread switch, draft materialization, or user change, and even a same-key re-create gets a fresh channel. The edited row's INSERT echo lands on a live subscription, and the post-commit re-fetch puts it above its reply whether or not the echo arrived.
Notes:
scheduleVerdictBackfill); the delay is deliberate, since the verdict does not exist yet when the turn commitsreconcileTranscripthas a known snapshot window on the claim-release path (a row committed during the fetch round-trip drops out until the next reconcile); it is named in the docblock, and merging the live array in is not the fix because the messages channel has no DELETE subscription