fix(chat): catch up resumed sessions by persisted cursor - #24
andrebrait wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (14)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds cursor-based history synchronization, indexed session reads, live stream snapshots with sequence metadata, SSE ordering changes, and client catch-up coordination. It also adds regression coverage for reconnects, pagination, live output, branches, retries, and stale events. ChangesSession synchronization
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SessionHook
participant SessionCatchUp
participant ContextRoute
participant SessionReader
participant AgentSessionWrapper
participant SSEStream
SessionHook->>SessionCatchUp: Request session catch-up
SessionCatchUp->>ContextRoute: Fetch history using cursor
ContextRoute->>SessionReader: Read indexed history page
SessionReader-->>ContextRoute: Return append or replace history
ContextRoute->>AgentSessionWrapper: Read live stream snapshot
AgentSessionWrapper-->>ContextRoute: Return live cursor and state
ContextRoute-->>SessionCatchUp: Return history page and snapshot
SSEStream-->>SessionCatchUp: Deliver ordered live event
SessionCatchUp-->>SessionHook: Apply confirmed and live state
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No concrete unresolved merge risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 20 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain around persistence confirmation, metadata and tool re-registration, and large-history pagination performance.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds cursor-based persisted-history catch-up for resumed sessions while preserving live SSE output and bounded pagination.
Changes:
- Adds paged history cursors and reset handling.
- Updates client, RPC, and SSE reconciliation with live snapshots.
- Expands recovery, race, pagination, and lifecycle test coverage.
File summaries
| File | Summary |
|---|---|
lib/sse-lifecycle.test.mjs |
Tests SSE subscription ordering and cleanup. |
lib/session-sync.ts |
Defines cursors and paged history selection. |
lib/session-sync.test.mjs |
Tests cursor and pagination behavior. |
lib/session-routes.test.mjs |
Tests synchronized context routes and races. |
lib/session-reader.ts |
Implements paged persisted-history loading. |
lib/session-reader.test.mjs |
Tests paging, branches, compaction, and recovery. |
lib/rpc-manager.ts |
Adds stream epochs and live snapshots. |
lib/rpc-manager.test.mjs |
Tests snapshot lifecycle and ordering. |
lib/message-update-coalescer.ts |
Adds snapshot-safe update flushing. |
lib/message-update-coalescer.test.mjs |
Tests coalescer flush behavior. |
hooks/useAgentSession.ts |
Integrates incremental catch-up and live hydration. |
hooks/useAgentSession.test.mjs |
Updates session hook test coverage. |
hooks/useAgentSession.rpc.test.mjs |
Adds catch-up and race regression coverage. |
hooks/useAgentSession-sync.ts |
Implements cursor reconciliation. |
hooks/useAgentSession-sync.test.mjs |
Tests selective live hydration and sync state. |
hooks/useAgentSession-stream.ts |
Adds stream cursor event typing. |
DESIGN.md |
Documents reconnect and catch-up behavior. |
CHANGELOG.md |
Records the feature. |
app/api/sessions/[id]/context/route.ts |
Serves paged history and live snapshots. |
app/api/agent/[id]/events/route.ts |
Prevents SSE subscription gaps. |
Review details
Suppressed comments (3)
hooks/useAgentSession.ts:416
- Because file-change notifications now call
catchUp.request()instead ofloadSession, this callback is the commit path for incremental reads, but it updates only messages, entry IDs, and (sometimes) todos.SessionContextalso carriesmodelandthinkingLevel; an idle/file-only session changed by another process will keep the initialdata.context.modeland thinking level, so the next prompt can use stale settings. Merge persisted metadata here while preserving live RPC state when it is authoritative.
setMessages(context.messages);
setEntryIds(context.entryIds);
if (!agentRunningRef.current) setTodoPhases(context.todoPhases ?? []);
setActiveLeafId(leafId);
hooks/useAgentSession.ts:2316
- When a sync response discovers a new wrapper epoch, this path replaces the SSE connection but does not re-register the per-wrapper host tools/URI schemes (or refresh the subagent roster). The existing fatal-stream reconnect does that immediately after
connectEventsat hooks/useAgentSession.ts:1084-1089; without the same call here, an active run can lose browser tools after a wrapper restart. Invoke the reconnect actions whenever this replacement is made while the agent is running.
void connectEvents(sid);
lib/session-reader.ts:563
- Every catch-up page rebuilds the entire selected transcript here before slicing it to at most 200 messages. A history with N entries therefore repeats the full path/message conversion about N/200 times; files above the 256 MiB parse-cache budget are also reparsed from disk on every page. This makes large-session recovery O(N²), blocks the Next server, and can exceed the client’s 30-second fetch timeout. Use a cursor-aware reader or retain a server-side parsed/path snapshot so the already-delivered prefix is not rebuilt per page.
const entries = loadEntriesOrThrowTooLarge(filePath);
const context = buildSessionContext(entries, leafId, options);
const history = selectSessionHistory(context, cursor, limit);
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@coderabbitai review Review-fix head 059fe34 is ready. Upstream and fork CI both pass Linux and Windows. Local tests: 763 passed, one existing platform skip; typecheck, lint and production build passed. The actual browser/HTTP/SSE offline-resume smoke passed again using a controlled native-process fixture: all three missed IDs recovered without prefix retransmission, quiet partial restored without another token, final answer once, zero page errors. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved critical and moderate issues remain in recovery correctness and catch-up scalability, requiring fixes and final human review.
Review details
Suppressed comments (4)
hooks/useAgentSession.ts:860
- The same race affects branch and pre-compaction navigation: a file notification can hydrate the first page before the full context GET resolves. When
catchUp.position()differs, this code leaves the UI on that partial paged context and never appliesd.context; a failed or interrupted later page can therefore permanently truncate the selected view. Keep the complete GET result until catch-up has completed successfully, or merge the two results.
if (catchUp.position() === position) {
catchUp.seed(d.context);
}
setShowPreCompactionHistory(includePreCompaction);
void catchUp.request();
hooks/useAgentSession.ts:1499
loadSession(..., includeState=true)returns a context withagentState: nullwhen the/stateRPC fails, buttranscriptLoadedis still set to true. A transient state failure with no answer persisted yet therefore makesstillBusyfalse and reaches the empty-response failure path, stopping recovery instead of leaving it retryable. Treat missing agent state as an incomplete reconciliation unless persisted content or an explicit error already proves completion.
if (!transcriptLoaded && !hadContent && !runError && !allowEmptyResponse) return;
hooks/useAgentSession.ts:2408
- This helper is called before every prompt in an existing session (including interrupt-and-reply), but it uses the unbounded session endpoint, which builds and returns the entire transcript rather than a bounded page. Large sessions therefore pay an O(history) parse/serialization/network cost on every prompt and can hit the session load ceiling, undermining the incremental-sync change. Reuse the confirmed entry IDs/cursor or a bounded sync/index read for this snapshot.
// Read immediately before dispatch, not from React's last rendered state:
lib/session-reader.ts:679
- This cache is invalidated whenever the file's size/mtime/ctime changes, but
buildHistoryIndex()callsloadSessionFile()and scans/parses the entire session before retaining only metadata. Consequently, a catch-up request after each persisted commit in a long active session performs a full-history read, so the bounded page does not provide bounded catch-up cost and can repeatedly block the Next.js event loop. Preserve an append-only index prefix or otherwise update the index incrementally when the file only grows, while retaining the full rebuild for truncation/rewrites.
for (const [cachedKey, cached] of cache) {
if (cached.pathKey === pathKey && cached.version !== version) cache.delete(cachedKey);
}
let index = cache.get(key);
if (!index) {
index = buildHistoryIndex(filePath, version, leafId, options.includePreCompaction);
- Files reviewed: 23/23 changed files
- Comments generated: 1
- Review effort level: Lite
| } else { | ||
| // A newer delta already committed while this full metadata read was in flight. | ||
| d.context = catchUp.history() ?? d.context; |
There was a problem hiding this comment.
Addressed in 3e5c24d. Catch-up pages now accumulate privately; only a complete selected history publishes its cursor and messages. Full loads cannot substitute an intermediate page, and a stale full response cannot resurrect entries truncated by a newer complete catch-up. Conflicting completed snapshots keep the newer catch-up and schedule a fresh read instead of inferring freshness from length. Coverage holds full responses and page two in both orders, rejects later pages, and exercises active/branch/pre-compaction views plus concurrent new entries. Also fixed the suppressed state-failure case: a failed /state read cannot prove an empty completion. Pre-prompt/interrupt boundary reads now use an ID-only indexed endpoint, not the full transcript. The unchanged-index boundary regression proves no message/body/blob reads. Local full suite passed 786 tests with one existing platform skip; the final stale-full/truncation correction was then verified by all 75 hook regressions, typecheck, and lint.
|
On the remaining suppressed append-index suggestion: retaining offsets based only on file growth is unsafe for native/imported session files. The same inode may rewrite its prefix and then grow; size/mtime/ctime do not certify append-only writes. We retain the correctness-first full metadata rebuild on any version change, and explicitly document that cold/mutated reads remain O(file size). Unchanged pages and pre-prompt ID-only boundaries reuse the bounded index; page bodies are not retained. A growing-prefix-rewrite regression verifies the new content and IDs are read, rather than trusting stale offsets. A truly incremental mutable-file index would require a trustworthy native append/version contract or prefix verification; this change intentionally does not invent either. |
|
Closing this review-only companion without merging. The upstream change has merged. Upstream: kahme247#95. Review history remains available here. Shared feature branches are preserved while upstream work remains open. |
Review-only companion for the upstream cursor catch-up follow-up. Base is pinned to upstream/main d52b0b3 so the diff matches; never merge this mirror. Supersedes the old recovery mirror #23 after upstream kahme247#94 merged.
Summary
Persisted-entry incremental catch-up while sessions are active or idle; bounded pages and branch/compaction reset; partial/tool snapshots with epoch and selective ordering. Confirmed history is fetch-since-only, while SSE continues streaming live output and signals new commits. Completion during a held fetch coalesces into a reread from the returned cursor. Native emit-before-persist is covered by file notifications. Upstream kahme247#61 idle retry/backoff behavior is preserved.
Verification
744 tests passed, one existing skip; typecheck/lint passed. Explicit before/after-persistence concurrent-completion regressions pass. Real Chromium/HTTP/SSE smoke with controlled native process recovers exactly entries 4/5/6 after cursor 3 and restores a quiet partial without token replay. Internal condensed review found no blocking issues.
No native upgrade or new dependencies. Matching frontend/API deployment required; not deployed.
Summary by CodeRabbit
Bug Fixes
Improvements