Skip to content

Commit 6b1686e

Browse files
committed
Fix blank-session restarts after interrupt or failed run
Fixes #1054. Esc-aborting a session and sending a follow-up could start a brand new, blank conversation because the continuation state (previousRunStateRef) was only synced when run() settled, while the abort listener released the input lock immediately. - SetupStreamingContext gains an onAbort callback invoked synchronously at the top of the abort listener, before the lock is released, so the run owner can checkpoint its latest SDK snapshot. - useSendMessage passes syncRunState(latestRunStateSnapshot) as onAbort, and syncRunState now guards with a generation token so a superseded run settling late can never adopt state, persist a checkpoint, or touch shared queue state over the run that replaced it. - The catch path (failed/expired runs) now also syncs the ref, not just disk, so the next prompt resumes from the last snapshot instead of stale or null history. - loadMostRecentChatState stops adopting/persisting sessionState-less run states, which made the SDK build a blank session on restart. Adds hook-level regression tests through the real createRunConfig / client.run wiring: abort then follow-up carries full history, late superseded runs cannot clobber, the rejected-run path resumes from the last snapshot, and sessionState-less states are never adopted. Helper and storage suites updated.
1 parent 835ae0f commit 6b1686e

6 files changed

Lines changed: 330 additions & 51 deletions

File tree

cli/src/hooks/helpers/__tests__/send-message.test.ts

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1859,3 +1859,144 @@ describe('freebuff gate errors', () => {
18591859
expect(messages[0].userError).toBeUndefined()
18601860
})
18611861
})
1862+
1863+
describe('onAbort checkpoint callback (freebuff #1054)', () => {
1864+
test('runs synchronously at the top of the abort listener, before the input lock is released', () => {
1865+
let messages = createBaseMessages()
1866+
const streamRefs = createStreamController()
1867+
const timerController = createMockTimerController()
1868+
// Cast the initializers: TS narrows `let` vars from literal
1869+
// initializers and the abort listener's closure assignments never
1870+
// reset that narrowing, which would mis-type the assertions below.
1871+
let streamStatus = 'streaming' as StreamStatus
1872+
let statusSeenInOnAbort = null as StreamStatus | null
1873+
let chainInProgress = true
1874+
let onAbortCalls = 0
1875+
1876+
const { abortController } = setupStreamingContext({
1877+
aiMessageId: 'ai-1',
1878+
timerController,
1879+
setMessages: (fn: any) => {
1880+
messages = fn(messages)
1881+
},
1882+
streamRefs,
1883+
onAbort: () => {
1884+
onAbortCalls++
1885+
// The listener below swaps this to 'idle'; seeing 'streaming'
1886+
// proves onAbort ran BEFORE the lock release / UI reset.
1887+
statusSeenInOnAbort = streamStatus
1888+
},
1889+
setStreamStatus: (status: StreamStatus) => {
1890+
streamStatus = status
1891+
},
1892+
setCanProcessQueue: () => {
1893+
chainInProgress = false
1894+
},
1895+
updateChainInProgress: () => {},
1896+
setIsRetrying: () => {},
1897+
setStreamingAgents: () => {},
1898+
})
1899+
1900+
abortController.abort()
1901+
1902+
expect(onAbortCalls).toBe(1)
1903+
expect(statusSeenInOnAbort).toBe('streaming')
1904+
expect(streamStatus).toBe('idle')
1905+
})
1906+
1907+
test('a throwing onAbort still runs the interruption cleanup', () => {
1908+
let messages = createBaseMessages()
1909+
const streamRefs = createStreamController()
1910+
const timerController = createMockTimerController()
1911+
let chainInProgress = true
1912+
1913+
const { abortController } = setupStreamingContext({
1914+
aiMessageId: 'ai-1',
1915+
timerController,
1916+
setMessages: (fn: any) => {
1917+
messages = fn(messages)
1918+
},
1919+
streamRefs,
1920+
onAbort: () => {
1921+
throw new Error('checkpoint boom')
1922+
},
1923+
setStreamStatus: () => {},
1924+
setCanProcessQueue: () => {
1925+
chainInProgress = false
1926+
},
1927+
updateChainInProgress: () => {},
1928+
setIsRetrying: () => {},
1929+
setStreamingAgents: () => {},
1930+
})
1931+
1932+
expect(() => abortController.abort()).not.toThrow()
1933+
expect(chainInProgress).toBe(false)
1934+
expect(streamRefs.state.wasAbortedByUser).toBe(true)
1935+
})
1936+
1937+
test('abort leaves the full message history in the continuation snapshot', () => {
1938+
// Mirrors the hook wiring: onAbort stores the latest in-flight snapshot
1939+
// into the continuation ref used by the next sendMessage. This is the
1940+
// exact path that used to hand the SDK an empty previousRun after an
1941+
// interrupt, resetting the conversation to a blank session.
1942+
const previousRunStateRef = { current: null as RunState | null }
1943+
const snapshot = {
1944+
traceSessionId: 'trace-1',
1945+
sessionState: {
1946+
mainAgentState: {
1947+
messageHistory: [
1948+
{
1949+
role: 'user',
1950+
content: [{ type: 'text', text: 'implement authentication' }],
1951+
},
1952+
{
1953+
role: 'assistant',
1954+
content: [{ type: 'text', text: 'reading project files' }],
1955+
},
1956+
{
1957+
role: 'tool',
1958+
toolName: 'read_files',
1959+
content: [{ type: 'text', value: { files: ['src/auth.ts'] } }],
1960+
},
1961+
],
1962+
},
1963+
} as any,
1964+
output: {
1965+
type: 'error',
1966+
message: 'Session ended before this response completed.',
1967+
},
1968+
} as unknown as RunState
1969+
1970+
const { abortController } = setupStreamingContext({
1971+
aiMessageId: 'ai-1',
1972+
timerController: createMockTimerController(),
1973+
setMessages: (fn: any) => {},
1974+
streamRefs: createStreamController(),
1975+
onAbort: () => {
1976+
previousRunStateRef.current = snapshot
1977+
},
1978+
setStreamStatus: () => {},
1979+
setCanProcessQueue: () => {},
1980+
updateChainInProgress: () => {},
1981+
setIsRetrying: () => {},
1982+
setStreamingAgents: () => {},
1983+
})
1984+
1985+
abortController.abort()
1986+
1987+
// The continuation snapshot now carries the full history instead of
1988+
// null (which would make the SDK build a blank session and re-explore
1989+
// from scratch on the next prompt). The hook-level test in
1990+
// cli/src/hooks/__tests__/use-send-message.test.tsx asserts the same
1991+
// through the real createRunConfig/client.run path.
1992+
expect(previousRunStateRef.current).toBe(snapshot)
1993+
expect(
1994+
(previousRunStateRef.current!.sessionState as any).mainAgentState
1995+
.messageHistory,
1996+
).toHaveLength(3)
1997+
expect(
1998+
(previousRunStateRef.current!.sessionState as any).mainAgentState
1999+
.messageHistory[2].toolName,
2000+
).toBe('read_files')
2001+
})
2002+
})

cli/src/hooks/helpers/send-message.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,11 @@ export const setupStreamingContext = (params: {
272272
setMessages: (updater: (messages: ChatMessage[]) => ChatMessage[]) => void
273273
streamRefs: StreamController
274274
abortController?: AbortController
275+
/** Invoked synchronously at the top of the abort listener, before the
276+
* input lock is released. Lets the run owner checkpoint its latest SDK
277+
* snapshot so a follow-up message sent immediately after an interrupt
278+
* resumes from fresh state instead of stale or null continuation state. */
279+
onAbort?: () => void
275280
setStreamStatus: (status: StreamStatus) => void
276281
setCanProcessQueue: (can: boolean) => void
277282
isQueuePausedRef?: MutableRefObject<boolean>
@@ -303,9 +308,19 @@ export const setupStreamingContext = (params: {
303308
const abortController = params.abortController ?? new AbortController()
304309

305310
abortController.signal.addEventListener('abort', () => {
306-
// Abort means the user stopped streaming; update UI with an interruption notice.
307-
// Release the chain lock immediately so new messages can be sent directly instead
308-
// of being queued.
311+
// Let the run owner checkpoint its latest SDK snapshot synchronously
312+
// BEFORE the input lock below is released, so a follow-up message sent
313+
// the moment the user hits Esc inherits the latest state instead of
314+
// stale (or null) continuation state. Best-effort: an onAbort failure
315+
// must never block interruption cleanup.
316+
try {
317+
params.onAbort?.()
318+
} catch {
319+
// Checkpoint callbacks are best-effort; never skip abort cleanup.
320+
}
321+
// Abort means the user stopped streaming; update UI with an interruption
322+
// notice. Release the chain lock immediately so new messages can be sent
323+
// directly instead of being queued.
309324
streamRefs.setters.setWasAbortedByUser(true)
310325
setIsRetrying(false)
311326
timerController.stop('aborted')

cli/src/hooks/use-send-message.ts

Lines changed: 73 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,11 @@ export const useSendMessage = ({
172172
const previousRunStateRef = useRef<RunState | null>(
173173
useChatStore.getState().runState,
174174
)
175+
// Incremented for every run that is admitted as a real SDK run. Late
176+
// results from a superseded run (one replaced by a newer run after the
177+
// input lock was released) must never adopt state, persist a checkpoint,
178+
// or touch shared queue state over the run that replaced it.
179+
const runGenerationRef = useRef(0)
175180
// Memoize stream controller to maintain referential stability across renders
176181
const streamRefsRef = useRef<ReturnType<
177182
typeof createStreamController
@@ -310,6 +315,23 @@ export const useSendMessage = ({
310315
const abortController = new AbortController()
311316
const runChatDir = resolveCurrentChatDir()
312317
const runChatIsCurrent = () => resolveCurrentChatDir() === runChatDir
318+
// Bump only after the run-start guard admits the message: a
319+
// session-ended message that gets requeued must not supersede an
320+
// active run.
321+
const runGeneration = ++runGenerationRef.current
322+
const runIsCurrent = () =>
323+
runGenerationRef.current === runGeneration && runChatIsCurrent()
324+
// Adopt a snapshot as the continuation state for the next message, in
325+
// memory (ref) and React state. Skipped when the run has been
326+
// superseded or the chat switched away, and for snapshots that carry
327+
// no session state at all: adopting one would silently make the SDK
328+
// start a blank session on the next prompt.
329+
const syncRunState = (state: RunState) => {
330+
if (!runIsCurrent()) return
331+
if (!state.sessionState) return
332+
previousRunStateRef.current = state
333+
setRunState(state)
334+
}
313335
let latestRunStateSnapshot: RunState = previousRunStateRef.current ?? {
314336
traceSessionId: randomUUID(),
315337
output: {
@@ -520,6 +542,14 @@ export const useSendMessage = ({
520542
setMessages,
521543
streamRefs,
522544
abortController,
545+
onAbort: () => {
546+
// Sync before the abort listener releases the input lock, so a
547+
// message sent the moment the user hits Esc resumes from the
548+
// latest snapshot instead of stale (or null) state. The
549+
// generation/chat guards in syncRunState keep this a no-op for
550+
// superseded runs and context-changing stops.
551+
syncRunState(latestRunStateSnapshot)
552+
},
523553
setStreamStatus,
524554
setCanProcessQueue,
525555
isQueuePausedRef,
@@ -565,7 +595,7 @@ export const useSendMessage = ({
565595
)
566596

567597
const eventHandlerState = createEventHandlerState({
568-
isActive: () => !abortController.signal.aborted && runChatIsCurrent(),
598+
isActive: () => !abortController.signal.aborted && runIsCurrent(),
569599
streamRefs,
570600
setStreamingAgents,
571601
setStreamStatus,
@@ -625,7 +655,7 @@ export const useSendMessage = ({
625655
// conversation, and checkpointing them into this run's directory
626656
// would overwrite that chat's transcript with foreign (possibly
627657
// empty) state — the chat would then be hidden from /history.
628-
if (abortController.signal.aborted || !runChatIsCurrent()) {
658+
if (abortController.signal.aborted || !runIsCurrent()) {
629659
return
630660
}
631661
previousRunStateRef.current = snapshot
@@ -685,10 +715,9 @@ export const useSendMessage = ({
685715
// context, and previousRunStateRef/setRunState would leak this run's
686716
// agent state into the other chat. (A plain Esc interrupt keeps the
687717
// same chat, so the interrupted turn is still saved as before.)
688-
if (!abortController.signal.aborted && runChatIsCurrent()) {
718+
if (runIsCurrent()) {
689719
// Finalize: persist state and mark complete
690-
previousRunStateRef.current = runState
691-
setRunState(runState)
720+
syncRunState(runState)
692721
setIsRetrying(false)
693722

694723
// Drop any queued/in-flight async checkpoint first so a stale write
@@ -700,29 +729,31 @@ export const useSendMessage = ({
700729
// traps is several times slower.
701730
saveChatState(runState, useChatStore.getState().messages, runChatDir)
702731
}
703-
handleRunCompletion({
704-
runState,
705-
actualCredits,
706-
agentMode,
707-
timerController,
708-
updater,
709-
aiMessageId,
710-
wasAbortedByUser: abortController.signal.aborted,
711-
hasReceivedContent: hasReceivedContentRef.current,
712-
setStreamStatus,
713-
setCanProcessQueue,
714-
updateChainInProgress,
715-
setHasReceivedPlanResponse,
716-
resumeQueue,
717-
isProcessingQueueRef,
718-
isQueuePausedRef,
719-
})
732+
if (runIsCurrent()) {
733+
handleRunCompletion({
734+
runState,
735+
actualCredits,
736+
agentMode,
737+
timerController,
738+
updater,
739+
aiMessageId,
740+
wasAbortedByUser: abortController.signal.aborted,
741+
hasReceivedContent: hasReceivedContentRef.current,
742+
setStreamStatus,
743+
setCanProcessQueue,
744+
updateChainInProgress,
745+
setHasReceivedPlanResponse,
746+
resumeQueue,
747+
isProcessingQueueRef,
748+
isQueuePausedRef,
749+
})
750+
}
720751
} catch (error) {
721752
// If this run was aborted, the abort handler already handled cleanup.
722753
// Don't run error handling to avoid interfering with any new run that
723754
// may have started. Uses per-run abortController.signal (not shared
724755
// streamRefs) so a newer run's reset() can't clear this flag.
725-
if (!abortController.signal.aborted) {
756+
if (!abortController.signal.aborted && runIsCurrent()) {
726757
handleRunError({
727758
error,
728759
timerController,
@@ -735,20 +766,26 @@ export const useSendMessage = ({
735766
isQueuePausedRef,
736767
hasReceivedContent: hasReceivedContentRef.current,
737768
})
769+
// Keep the latest snapshot available to the next message in this
770+
// process, not only on disk: without this, a failed or expired
771+
// turn is followed by a run with stale (or null) history.
772+
syncRunState(latestRunStateSnapshot)
738773
// Persist the last checkpoint plus the error banner so a restart
739-
// after a failed run still shows this turn. Settle async checkpoints
740-
// first so a stale write can't clobber this one. Skipped after a
741-
// mid-run chat switch — the store's messages belong to the new chat.
742-
if (runChatIsCurrent()) {
743-
await settleCheckpointSave()
744-
saveChatState(
745-
latestRunStateSnapshot,
746-
useChatStore.getState().messages,
747-
runChatDir,
748-
)
749-
}
750-
} else {
774+
// after a failed run still shows this turn. Settle async
775+
// checkpoints first so a stale write can't clobber this one.
776+
await settleCheckpointSave()
777+
saveChatState(
778+
latestRunStateSnapshot,
779+
useChatStore.getState().messages,
780+
runChatDir,
781+
)
782+
} else if (abortController.signal.aborted) {
751783
logger.debug({ error }, '[send-message] Ignoring error after abort')
784+
} else {
785+
logger.debug(
786+
{ error },
787+
'[send-message] Ignoring error after run superseded',
788+
)
752789
}
753790
} finally {
754791
// Close the steering mailbox. Anything the run never drained was
@@ -792,7 +829,7 @@ export const useSendMessage = ({
792829
// interfering with any new run that may have started after the abort.
793830
// Uses per-run abortController.signal (not shared streamRefs) so a newer
794831
// run's reset() can't clear this flag.
795-
if (!abortController.signal.aborted) {
832+
if (!abortController.signal.aborted && runIsCurrent()) {
796833
if (isChainInProgressRef.current) {
797834
logger.warn(
798835
{},

0 commit comments

Comments
 (0)