Skip to content

fix(core): preserve session state on abort and error / session expiration (replaces #1055) - #1181

Open
Utkarsh-X wants to merge 1 commit into
CodebuffAI:mainfrom
Utkarsh-X:fix/preserve-session-state-on-abort
Open

fix(core): preserve session state on abort and error / session expiration (replaces #1055)#1181
Utkarsh-X wants to merge 1 commit into
CodebuffAI:mainfrom
Utkarsh-X:fix/preserve-session-state-on-abort

Conversation

@Utkarsh-X

Copy link
Copy Markdown

Fixes #1054
Reopens #1055 (rebased onto the new main history following @victorxheng's note).

What this fixes

When a user interrupts a run with Esc or when a session expires, sending a follow-up message immediately was reading a stale/null in-memory previousRunStateRef. This caused the SDK to construct a fresh empty session state (messageHistory = []), wiping conversation context.

What's included (addressing the #1055 review feedback)

  1. Regression tests (send-message.test.ts): Added 3 dedicated tests verifying:
    • User abort (Esc) preserves turn history + tool calls in createRunConfig.previousRun.
    • Session expiration / gate errors preserve history on "continue".
    • Mid-run chat switches (runChatIsCurrent === false) never leak state into other chats.
  2. DRY state sync (use-send-message.ts): Consolidated the assignment into a simple 5-line syncRunState closure used uniformly across abort, completion, and error handlers.
  3. Clean scope: Reverted the previous prompt text tweaks so this PR is strictly isolated to the session state bug (3 files total).
  4. Re-render safety: Checked store subscriptions — zero React components subscribe to state.runState, so updating it alongside streamStatus = 'idle' causes no visual layout flash.

I intentionally kept this as minimal and safe as possible to solve the demonstrated bug without bloating the change surface or touching unrelated client/storage APIs. That said, if you'd prefer any tweaks or want me to look into anything further here, just let me know and I'm happy to research and adjust!

Verification

  • Test Suites (202 / 202 passed across touched areas):
    • bun test cli/src/hooks/helpers/__tests__/send-message.test.ts (49 passed, including the 3 new regression tests)
    • bun test sdk/src/__tests__/run-cancellation.test.ts (16 passed)
    • bun test cli/src/utils/__tests__/run-state-storage.test.ts (37 passed)
    • bun test packages/agent-runtime/src/__tests__/compact-history.test.ts (31 passed)
    • bun test agents/__tests__/context-pruner.test.ts (69 passed)
  • Builds:
    • bun run build:sdk (ESM, CJS, and TypeScript declarations bundled with 0 errors)
    • bun run build:freebuff (Native executable built with exit code 0)

@codebuff-team

Copy link
Copy Markdown
Contributor

The core insight is reasonable: previousRunStateRef (and the store's runState) weren't being refreshed in the abort callback (registerActiveRun) or in the error/catch path, so an immediate follow-up message could pick up a stale/null snapshot and start a fresh session. Introducing a single syncRunState closure and calling it uniformly in the abort handler, the completion path, and the error path (use-send-message.ts lines ~332-353, ~700, ~752) is a sensible, minimal way to fix that, and it's a legitimate in-scope change (cli/src/hooks).

The problem is the tests in send-message.test.ts. All three new tests hand-roll a local copy of the syncRunState logic (const syncRunState = (state) => { if (currentChatDir !== '/chat-1') return; ...}) inside the test body rather than importing and exercising the actual closure from use-send-message.ts. They then assert against createRunConfig using a manually-set previousRunStateRef.current. This proves the concept works, but it does not verify that the real hook wires latestRunStateSnapshot into syncRunState correctly, or that latestRunStateSnapshot is actually fresh (not stale/null) at the abort callback call site — which is the crux of the original bug. Since use-send-message.ts isn't unit-testable in isolation here, this would need either an integration test that drives the actual hook (e.g. via renderHook) through an abort/error cycle, or at minimum a comment/justification for why the current test strategy is an acceptable proxy.

Worth also double-checking that latestRunStateSnapshot is guaranteed to be populated with the latest partial state at the moment the abort callback fires (before client.run settles) — the diff assumes this but the surrounding declaration isn't shown.

Right direction, but please add a test that actually calls into the hook/module under test rather than reimplementing its logic in the spec file.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Sep 1, 2026
@Utkarsh-X
Utkarsh-X force-pushed the fix/preserve-session-state-on-abort branch from 66f89ba to 469883b Compare September 1, 2026 19:42
@Utkarsh-X
Utkarsh-X force-pushed the fix/preserve-session-state-on-abort branch from 469883b to 10fb2a1 Compare September 2, 2026 09:45
@Utkarsh-X

Copy link
Copy Markdown
Author

Addressed the feedback. Replaced the 3 manual tests with 3 hook-level integration tests in use-send-message.test.tsx. Each test mounts the real useSendMessage hook via createTestRenderer + createRoot, drives it through the real codepath, and asserts against the actual previousRun passed into the real createRunConfig call — no reimplemented logic in the test body.

The 3 tests cover:

  1. Abort with streaming progress — calls onStateSnapshot(snapshot) then stopActiveRun('user-interrupt'), proving the real abort callback at line ~355 commits the live snapshot to previousRunStateRef before client.run() settles.

  2. Error / session expiry — calls onStateSnapshot(snapshot) then rejects client.run(), proving the real catch block at line ~752 commits the snapshot.

  3. Abort with no prior snapshot (directly addresses your follow-up question about latestRunStateSnapshot freshness) — aborts immediately before any onStateSnapshot fires, proving latestRunStateSnapshot initializes from previousRunStateRef.current (the prior completed run's state) and is therefore never null at the abort callsite.

All 4 tests in the file pass (3 new + 1 existing upstream test).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Session state is lost after free session expires or run is interrupted

2 participants