[fix] Stop agent sessions breaking when open in two places [AGE-4004] - #5589
Merged
mmabrouk merged 8 commits intoJul 31, 2026
Merged
Conversation
The agent chat's session history, open-tab list, active tab and cached transcripts are all `atomWithStorage` atoms. jotai's default storage subscribes to the window `storage` event, so every write in one browser tab replaced those records live in every other tab of the same origin. The open-tab list drives the antd `Tabs` items, so an incoming replacement could remove a tab and unmount its `AgentConversation`. That orphans the `useChat` stream mid-turn, and since a transcript is only persisted once its stream settles, the in-flight turn was lost. Switching browser tabs made it worse: both session queries revalidate on window focus, and the reconcile that follows writes to all three keys. These four stores now use localStorage with `subscribe` removed, so a tab owns its own view. Cross-tab awareness already has a correct channel: the server reconcile on focus, plus renames pushed to the durable stream header. The localStorage push was a redundant second channel that replaced whole records instead of merging them. Storage stays shared, so a reload still picks up the last write.
…004] A browser that opened a session while another browser was mid-turn cached a partial transcript, and no amount of refreshing ever fixed it. The durable record log had the complete turn the whole time. The adoption guard asked "does the server have more MESSAGES than I do?". `transcriptToMessages` only closes a message on a `done` record and folds a paused turn into its resume, so a turn that grows in place (tool results landing, an approval round-trip completing) keeps the same message count from start to finish. The guard concluded the server was not ahead and kept the partial copy, on every subsequent reload. The record log is append-only and ordered, so record count is an exact test. `loadSessionMessages` now returns the count it built the transcript from, and that watermark is persisted next to the cached messages. The guard adopts when the log grew past the watermark, with a message-count floor so ingest lag can never trade a longer local transcript for a shorter server one. Both guards now share one rule (`shouldAdoptServerTranscript`): the cache-miss hydration path carried the same blind spot and would have stayed broken if only the revalidate path were fixed. The old "local tail paused, server tail complete" special case is deleted rather than extended, since a watermark sees that growth directly. The watermark is cleared when a turn goes live locally (we cannot know what the runner logged for it), so the next open re-syncs from the log. That clearing effect must stay declared above the persist effect; effects fire in declaration order, and that is what stops a locally-extended transcript being filed under a server watermark. Messages and watermarks can only move together: one writer sets both, and a single `dropSessionMessages` helper is now the only deletion path, covering the quota-eviction loop that previously shed transcripts with no way to shed their watermarks. Caches written before this change have no watermark, which reads as 0, so they adopt the server copy once and are correct from then on.
A session being driven from another browser or tab gave no sign anything was happening. It looked identical to an idle one, so the only feedback was the transcript silently not changing. There is no push channel to browsers today: the runner publishes every event to Redis, but the only consumer is the ingest worker that writes them to the DB. True live following would need an SSE endpoint with per-session auth, which is out of scope here. Instead the conversation now polls the durable record log while the backend reports a live run that this browser is not driving, and the adoption guard decides whether anything changed. A strip above the composer says so, reusing the session bar's `running` dot so the two read as one signal. The poll chains its timeouts rather than using setInterval, since the log is large and backend-slow and a slow fetch must not stack requests on itself. It backs off from 15s to 60s while the log is quiet and resets on real growth: `is_running` is Redis-TTL'd at an hour and the runner only clears it at turn end, so a runner that dies mid-turn would otherwise leave a browser refetching the full log every 15s for that hour. Backing off rather than stopping keeps a genuinely long turn followed, since a slow tool call emits no records until it returns. Background adoptions do not jump to the live edge unless the reader is already there, so a catch-up cannot yank someone who scrolled up to read.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughSession transcript hydration now tracks durable record-count watermarks, applies a centralized server-adoption policy, polls sessions running elsewhere, and displays that state in the composer dock. Persisted transcript cleanup also removes associated watermarks. ChangesTranscript adoption and validation
Session persistence
Hydration and remote following
Composer status
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserB
participant useSessionHydration
participant sessionLivenessAtomFamily
participant loadSessionMessages
participant shouldAdoptServerTranscript
BrowserB->>useSessionHydration: open session
useSessionHydration->>sessionLivenessAtomFamily: read session liveness
sessionLivenessAtomFamily-->>useSessionHydration: running elsewhere
useSessionHydration->>loadSessionMessages: poll durable transcript
loadSessionMessages->>shouldAdoptServerTranscript: compare record watermark
shouldAdoptServerTranscript-->>useSessionHydration: adopt updated transcript
useSessionHydration-->>BrowserB: render current transcript and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…session-multi-tab
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
ardaerzin
marked this pull request as ready for review
July 30, 2026 17:16
dosubot
Bot
added
size:L
Contributor
Railway Preview Environment
|
ashrafchowdury
approved these changes
Jul 31, 2026
…session-multi-tab Resolves the useSessionHydration.ts conflict between the watermark-based adoption rework (#5589) and the latent-defect review pass (01f11b7 on #5569) so both intents survive: - Keep #5589's single `adoptServerTranscript` adopter (record-count watermark via `shouldAdoptServerTranscript`) for the hydration path, the SWR revalidation path, and the remote-run poll. - Port 01f11b7's hydration fix into that structure: a local `adopted` flag so a stale empty first result cannot set the "history unavailable" notice after the background refetch already adopted real history. - The rest of 01f11b7's commit-timing guard (local high-water mark / adopted snapshot) is subsumed by the watermark ref, which the adopter updates synchronously before any React commit; documented that invariant on the write so it is not lost. - The count-based guard and paused-tail special case from the old SWR effect are intentionally gone — #5589 replaced them with the watermark ("the special case is gone rather than extended").
mmabrouk
reviewed
Jul 31, 2026
mmabrouk
left a comment
mmabrouk
left a comment
Member
There was a problem hiding this comment.
🤖 The AI agent says:
The main remote-session flow can fail to discover or poll a newly started run, and the adoption rule can overwrite complete local history with an ingest-lagging snapshot. Quota failures can also persist mismatched transcripts and watermarks.
| // decides whether anything actually changed. `isRunning` also covers OUR stream, so exclude the | ||
| // case where this browser is the one driving. | ||
| const {nest} = useAtomValue(sessionLivenessAtomFamily(sessionId)) | ||
| const runningElsewhere = nest.isRunning && !busy |
Member
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Poll for remote runs from an initially idle state
When the last liveness result is empty, aliveStreamsQueryAtom disables its interval (state/liveness.ts:42) and only refetches on window focus. If another browser starts this session while this tab remains open, nest.isRunning never changes, so neither the status strip nor record-log polling starts. Keep a discovery poll or another invalidation channel while the alive set is empty.
| recordWatermarkRef, | ||
| setMessages, | ||
| persistMessages, | ||
| intent, |
Member
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Keep the remote polling effect stable across renders
useScrollIntent returns a new object each render, so this intent dependency recreates adoptServerTranscript; the polling effect then clears and restarts its 15-second timer. During a remote run, the liveness query itself rerenders on its 15-second refetches, which can continually cancel record polling before adoption. Depend on stable members or use the prescribed query polling pattern (web/AGENTS.md:284).
| // Floor, never a trigger: ingest lag can serve a snapshot shorter than what we render, and | ||
| // trading down would drop the tail. Equal length is fine — that is the #5530 case, where the | ||
| // same messages carry far more content. | ||
| if (serverMessageCount < localMessageCount) return false |
Member
There was a problem hiding this comment.
🤖 The AI agent says:
[P1] Preserve local history against equal-length lagging snapshots
When a locally streamed turn settles, its watermark is intentionally cleared. On a quick reload during ingest lag, the server can already map to the same message count while still missing later tool results or text, which is the same-count growth this change documents. Treating undefined as zero and allowing equal counts therefore overwrites the complete local transcript with a partial server copy, which may remain cached after liveness ends. Missing-watermark adoption needs a completion or content guard.
| // Even the active session alone won't fit — keep it in memory, skip persistence. | ||
| console.warn("[agent-chat] message store over quota; skipping persistence") | ||
| return | ||
| return evicted |
Member
There was a problem hiding this comment.
🤖 The AI agent says:
[P2] Avoid persisting a watermark after transcript write failure
When one session is too large for localStorage, this branch returns after the transcript write throws, but persistSessionMessagesAtom still writes recordCount. After reload, the old or missing messages are paired with the newer watermark, so shouldAdoptServerTranscript rejects the complete server log as not newer and leaves the cache stale. Return a failure indicator and clear or skip the watermark in this path.
`useScrollIntent` returns a fresh object every render, and `intent` sat in `adoptServerTranscript`'s dependency list — so the callback was recreated on every render, and the remote-run poll effect (keyed on it) tore down and re-armed its timer each time. The liveness query alone re-renders the hook ~every 15s while a run is live elsewhere, i.e. on the same cadence as the poll it was cancelling: the record-log catch-up could be starved for the whole run and its backoff never accumulated. Two layers, so the timer's stability is true by construction, not by audit: depend on `intent.armJump` / `intent.stickRef` (both stable) instead of the per-render `intent` object, and have the poll effect read the current adopter through a ref, keyed only on `runningElsewhere` + `sessionId`.
… skipped When even the active session alone overflows the localStorage quota, `writeMessagesWithQuotaGuard` skips the transcript write — but `persistSessionMessagesAtom` still wrote the NEW `recordCount`. After a reload that pairs the OLD (or missing) cached messages with the newer watermark, so `shouldAdoptServerTranscript` rejects the complete server log as "not newer" on every open and the stale cache is frozen until the log happens to outgrow the bogus number. Surface `persisted` from the quota guard and drop the session's watermark when the write was skipped, enforcing the invariant already documented on `dropSessionMessages`: the transcript store and the watermark store must never diverge.
dosubot
Bot
added
size:XL
mmabrouk
merged commit Jul 31, 2026
18367fd
into
fe-chore/agent-conversation-split
24 of 25 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5530.
Context
Two separate bugs made an agent session unreliable whenever it was open in more than one place.
A second browser got permanently stuck (#5530). Open a session in browser B, drive it from browser A through a tool call and an approval, then refresh B mid-turn. B cached a partial transcript and no amount of refreshing ever fixed it. The durable record log had the complete turn the whole time.
The adoption guard asked "does the server have more MESSAGES than I do?".
transcriptToMessagesonly closes a message on adonerecord, and it deliberately folds a paused turn into its resume. So a turn that grows in place (tool results landing, an approval round-trip completing) keeps the same message count from start to finish. The guard concluded the server was not ahead, kept the partial copy, and repeated that decision on every reload.One browser tab could kill another's live stream. The session history, open-tab list, active tab and cached transcripts are all
atomWithStorageatoms, and jotai's default storage subscribes to the windowstorageevent. Every write in one tab replaced those records live in every other tab of the same origin. The open-tab list drives the antdTabsitems, so an incoming replacement could remove a tab and unmount its conversation, orphaning theuseChatstream mid-turn. A transcript is only persisted once its stream settles, so the in-flight turn was lost. Switching browser tabs made it worse, because both session queries revalidate on window focus and the reconcile that follows writes to all three keys.Changes
Record count instead of message count. The log is append-only and ordered, so "the server has more records than my transcript was built from" is exact.
loadSessionMessagesnow returns the count it built from, and that watermark is persisted next to the cached messages.Before, for a turn that paused for approval and then completed:
After, the guard compares 40 against the stored 12 and adopts. A message-count check stays, but only as a floor: ingest lag can serve a snapshot shorter than what we render, and that must never be traded down.
Both paths in
useSessionHydrationnow share one rule,shouldAdoptServerTranscript. The cache-miss hydration path carried the same blind spot, so fixing only the revalidate path would have left it broken. The old "local tail paused, server tail complete" special case is deleted rather than extended, since a watermark sees that growth directly.The watermark is cleared when a turn goes live locally, because we cannot know what the runner logged for it, so the next open re-syncs from the log. Messages and watermarks can only move together: one writer sets both, and a single
dropSessionMessageshelper is the only deletion path. That covers the quota-eviction loop, which previously shed transcripts with no way to shed their watermarks.Per-tab storage. The four session stores now use localStorage with
subscriberemoved, so a tab owns its own view. Cross-tab awareness already has a correct channel: the server reconcile on focus, plus renames pushed to the durable stream header. The localStorage push was a redundant second channel that replaced whole records instead of merging them. Storage stays shared, so a reload still picks up the last write.A signal that something is running. A session driven from elsewhere now shows a strip above the composer, reusing the session bar's
runningdot so the two read as one signal. Behind it,useSessionHydrationpolls the record log while the backend reports a live run this browser is not driving, and the guard decides whether anything changed.The poll chains its timeouts rather than using
setInterval, since the log is large and backend-slow and a slow fetch must not stack on itself. It backs off from 15s to 60s while the log is quiet and resets on real growth.is_runningis Redis-TTL'd at an hour and the runner only clears it at turn end, so a runner that dies mid-turn would otherwise leave a browser refetching the full log every 15s for that hour. Backing off rather than stopping keeps a genuinely long turn followed, since a slow tool call emits no records until it returns.Notes
No backend changes. I checked the two behaviours this depends on.
get_recordsreturns the complete ordered log with noLIMITand no windowing, so the count is a genuine monotonic watermark. Liveness is Redis-backed with runner heartbeats and is explicitly dropped at turn end, so the signal cannot be permanently wrong.Caches written before this change have no watermark, which reads as 0, so they adopt the server copy once and are correct from then on. No migration needed.
Six unit tests cover the adoption rule in
@agenta/entities, where the package vitest suite actually runs in CI. A regression test intranscriptToMessages.test.tspins the property that made message counts wrong, so nobody simplifies the guard back to counting.Worth flagging for review:
web/osshas no test runner yet, so the mapper test here does not run in CI. CI runspnpm -r --if-present run test:unitand only packages define that script, so the pre-existingtranscriptToMessages.test.tshas never executed either. [5528] test(frontend): run web/oss unit tests in CI #5567 adds exactly that wiring (avitest.config.ts, thetest:unitscript and the devDependency), so nothing is needed here. I ran this file against [5528] test(frontend): run web/oss unit tests in CI #5567's config locally and all 11 tests pass, so it will go green once that wiring reaches this branch's release line. This is also why the adoption rule itself went into@agenta/entities, which already hostscore/liveness.tsand whose suite runs in CI today.runningTTL is long for a signal the UI treats as "is something happening right now". Nothing here depends on shortening it, and the default is shared with the TS runner viaservices/runner/tests/fixtures/sessions/redis_contract.json, so it is a question for the coordination-plane owner rather than a change to make in this PR.What to QA
Needs the session flags active (
AGENTA_SESSIONS_RECONSTRUCT,AGENTA_RECORDS_DURABLE,NEXT_PUBLIC_SESSIONS_LAST_MESSAGE_ONLY).+. Tab A keeps streaming and does not lose its turn.