From d58d340fe22ccf32fcc40642cbf49acac6304cd1 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:44:09 -0400 Subject: [PATCH] fix(native-chat): stop a teardown answering for an offer it could not read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the write-back deleted a durable recovery offer nobody had seen. A take that FAILED left the claim holding an empty list and reporting that this launch had answered for the offer. The markers were still on disk, unread and unknowable, and teardown then overwrote them with its own empty list. It now writes nothing at all unless it has a witness of its own. `owed()` read "has the capsule been touched" where it meant "did anything here LOOK at the offer" — and its own write-back read counted. Teardown is retried when a phase fails, so the second attempt re-derived carried markers against a session map eviction had already emptied, refused every one, and wiped what the first attempt had just carried forward. The flag is now set only by the paths that actually read or act on the offer. The mock guard for the carry could not fail: it indexed the session it claimed nothing had revealed, so re-deriving passed and the verbatim carry was never the reason it went green. It now runs against no indexed session, which is what an unread offer looks like. Also drops the `Not now` row from the preference table, where it was paired with a dismiss method it no longer calls, and asserts the same thing where the snooze is already covered. Splits the marker predicate's journal reader out of the resume host, which was at its line ceiling. --- ...ctured-agent-session-restart-candidates.ts | 82 +++++++++++++++++++ ...ctured-agent-session-restart-claim.test.ts | 64 ++++++++++++++- .../structured-agent-session-restart-claim.ts | 27 ++++-- ...tured-agent-session-restart-resume-host.ts | 76 +++++------------ .../NativeChatResumeOnRestartModal.test.tsx | 23 +++--- 5 files changed, 195 insertions(+), 77 deletions(-) create mode 100644 src/main/native-chat/agent-session-wire/structured-agent-session-restart-candidates.ts diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-restart-candidates.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-candidates.ts new file mode 100644 index 00000000000..4a1b348b336 --- /dev/null +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-candidates.ts @@ -0,0 +1,82 @@ +// Which of a set of markers the predicate would still act on, read off the host's own live journals. +// +// A different question from the claim's: the claim decides which markers are still OWED, this +// decides which of those describe work a resume may touch. It lives apart because both the offer, +// the click and the teardown write-back ask it, each with a different lease expectation. +// +// The per-session journal snapshot is cached for the length of one call: the predicate asks the +// same session for its items four times, and a snapshot that moved between those reads would let +// two clauses judge two different conversations. + +import { agentJournalSubmissionKey } from '../../../shared/agent-session-journal-item-key' +import type { AgentJournalRenderItem } from '../../../shared/agent-session-journal-types' +import type { AgentSessionRecord } from '../../../shared/agent-session-record' +import type { AgentSessionResumeMarker } from '../../../shared/agent-session-resume-marker' +import { + latestStructuredAgentSessionPrompt, + latestStructuredAgentSessionUserItem, + newestStructuredAgentSessionTurn, + projectStructuredAgentSessionStatus +} from '../../../shared/structured-agent-session-projection' +import type { AgentSessionJournal } from '../agent-session-journal/journal-store' +import type { StructuredAgentSessionAdapter } from './structured-agent-session-adapter' +import { adapterSupportsRecord } from './structured-agent-session-provider-support' +import { + structuredAgentSessionResumableSet, + type StructuredAgentSessionResumeCandidate +} from './structured-agent-session-restart-resume-set' + +/** The only part of a live session this reads. */ +export type StructuredAgentSessionRestartJournalSource = { journal: AgentSessionJournal } + +export type StructuredAgentSessionRestartCandidateReader = ( + markers: readonly AgentSessionResumeMarker[], + leaseState: 'must-be-released' | 'may-be-held', + /** Only teardown may judge before it rewrites the stopped child's running turn. */ + providerStopped?: boolean, + /** A continuation already in flight; its own submission is not newer user work. */ + pendingContinuationId?: string +) => StructuredAgentSessionResumeCandidate[] + +export function createStructuredAgentSessionRestartCandidateReader(deps: { + /** The host's LIVE session map — the only honest answer to "was this actually working". */ + sessions: ReadonlyMap + getRecord: (sessionId: string) => AgentSessionRecord | null + adapter: StructuredAgentSessionAdapter + now: () => number +}): StructuredAgentSessionRestartCandidateReader { + return (markers, leaseState, providerStopped = false, pendingContinuationId) => { + const items = new Map() + const itemsFor = (sessionId: string): AgentJournalRenderItem[] => { + let snapshot = items.get(sessionId) + if (!snapshot) { + snapshot = deps.sessions.get(sessionId)?.journal.snapshot().items ?? [] + if (pendingContinuationId) { + const ownItemId = agentJournalSubmissionKey(pendingContinuationId) + snapshot = snapshot.filter((item) => item.itemId !== ownItemId) + } + items.set(sessionId, snapshot) + } + return snapshot + } + return structuredAgentSessionResumableSet({ + markers, + getRecord: deps.getRecord, + supportsRecord: (record) => adapterSupportsRecord(deps.adapter, record), + waitingOnUser: (sessionId) => + projectStructuredAgentSessionStatus(itemsFor(sessionId)) === 'attention', + providerStopped, + journalTurn: (sessionId) => newestStructuredAgentSessionTurn(itemsFor(sessionId)), + journalSubmission: (sessionId, clientMessageId) => + deps.sessions + .get(sessionId) + ?.journal.submissions() + .find((submission) => submission.clientMessageId === clientMessageId) ?? null, + latestPrompt: (sessionId) => latestStructuredAgentSessionPrompt(itemsFor(sessionId)), + latestUserItemId: (sessionId) => + latestStructuredAgentSessionUserItem(itemsFor(sessionId))?.itemId ?? null, + now: deps.now(), + leaseState + }) + } +} diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts index 11dfdebcce2..b1a3d8815fe 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.test.ts @@ -335,11 +335,11 @@ describe('the restart-resume surface', () => { }) // A launch that never read the offer must not answer for it — the flag was off, the first read - // failed, the window never mounted. Nothing revealed those sessions, so nothing here can judge - // them, and replacing the capsule with this launch's empty list deletes a recovery the user was - // never shown. + // failed, the window never mounted. NO SESSION IS INDEXED here, which is the whole point: that is + // what an unread offer looks like, and re-deriving against it refuses every marker, which is + // indistinguishable from deleting a recovery the user was never shown. it('leaves a durable offer this launch never claimed intact at teardown', async () => { - const { restartResume, live, recorded } = surface({}) + const { restartResume, live, recorded } = surface({ sessions: new Map() }) restartResume.captureMarkers('quit') await restartResume.recordMarkers() @@ -348,6 +348,62 @@ describe('the restart-resume surface', () => { expect([...live.keys()]).toEqual([SESSION]) }) + // Teardown is RETRIED when a phase fails, and its own write-back read must not make the second + // attempt look like a launch that had read the offer — that would re-derive against a session map + // eviction has already emptied, and answer "nothing is resumable" for every carried marker. + it('carries the same unread offer again when teardown is repeated', async () => { + const { restartResume, live, recorded } = surface({ sessions: new Map() }) + + await restartResume.recordMarkers() + await restartResume.recordMarkers() + + expect(recorded.map((entry) => entry.map((marker) => marker.sessionId))).toEqual([ + [SESSION], + [SESSION] + ]) + expect([...live.keys()]).toEqual([SESSION]) + }) + + // A take that failed is not an empty offer. The markers are still on disk, unread and unknowable, + // and an empty write over them deletes exactly the recovery nobody was shown. + it('writes nothing over a durable offer it could not read', async () => { + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const { restartResume, live, recorded } = surface({ clearFails: true, sessions: new Map() }) + + restartResume.captureMarkers('quit') + await restartResume.recordMarkers() + + expect(recorded).toEqual([]) + expect([...live.keys()]).toEqual([SESSION]) + } finally { + warning.mockRestore() + } + }) + + // ...but a witness from THIS teardown still has to reach disk, whatever the take did. + it('still records this teardown witness after a take it could not read', async () => { + const warning = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const { restartResume, recorded } = surface({ + clearFails: true, + sessions: new Map([ + [SESSION, { journal: journal([turnItem('turn-2', 'running')]), hasProviderChild: true }] + ]) + }) + + restartResume.captureMarkers('update') + restartResume.confirmStoppedMarker(SESSION) + await restartResume.recordMarkers() + + expect(recorded[0]).toMatchObject([ + { sessionId: SESSION, work: { kind: 'turn', id: 'turn-2' } } + ]) + } finally { + warning.mockRestore() + } + }) + it('persists a snoozed claimed offer across the next teardown', async () => { const { restartResume, recorded } = surface({}) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.ts index 0faaba20434..1a102ae184e 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-claim.ts @@ -23,14 +23,24 @@ export type StructuredAgentSessionRestartClaim = { /** What the host still advertises: evidence minus recovery. */ offered: () => AgentSessionResumeMarker[] /** - * What teardown must write back, and whether this launch ever claimed it. + * What teardown must write back, whether this launch ever READ the offer, and whether the durable + * copy could be read at all. * * `claimed: false` is not "nothing is owed" — the durable copy is still intact and nothing here * revealed it, so it is taken now and must be carried forward VERBATIM. Re-deriving it would * refuse every marker for want of a journal nobody opened, which is indistinguishable from - * deleting an offer the user was never shown. + * deleting an offer the user was never shown. Reading it HERE is not reading it: the flag stays + * false, so a teardown repeated after a failed one carries the same offer forward again instead + * of answering for a session map that has since been emptied. + * + * `unreadable: true` means the take itself failed. The durable copy is then intact AND unknowable, + * so nothing here may answer for it. */ - owed: () => Promise<{ markers: AgentSessionResumeMarker[]; claimed: boolean }> + owed: () => Promise<{ + markers: AgentSessionResumeMarker[] + claimed: boolean + unreadable: boolean + }> /** Acted on. In memory, because the durable copy is already gone. */ spend: (sessionId: string) => boolean /** This launch handed the provider back; see the header. */ @@ -47,6 +57,10 @@ export function createStructuredAgentSessionRestartClaim(deps: { }): StructuredAgentSessionRestartClaim { let claimed: AgentSessionResumeMarker[] | null = null let claiming: Promise | undefined + /** Anything in this launch LOOKED at the offer. Teardown's own write-back read is not a reader. */ + let read = false + /** The take failed, so an empty `claimed` says nothing about what is still on disk. */ + let unreadable = false const recovered = new Set() const claim = async (): Promise => { @@ -56,6 +70,7 @@ export function createStructuredAgentSessionRestartClaim(deps: { } catch { console.warn('[structured-agent-session] taking recovery capsule failed') claimed = [] + unreadable = true } })() await claiming @@ -67,6 +82,7 @@ export function createStructuredAgentSessionRestartClaim(deps: { return { evidence: async () => { + read = true for (const marker of await claim()) { if (!deps.isOpen(marker.sessionId)) { await deps.open(marker.sessionId).catch(() => null) @@ -76,9 +92,9 @@ export function createStructuredAgentSessionRestartClaim(deps: { }, offered, owed: async () => { - const claimed = claiming !== undefined + const readBeforeThisTake = read await claim() - return { markers: offered(), claimed } + return { markers: offered(), claimed: readBeforeThisTake, unreadable } }, spend: (sessionId) => { const before = claimed?.length ?? 0 @@ -93,6 +109,7 @@ export function createStructuredAgentSessionRestartClaim(deps: { } }, abandon: async () => { + read = true await claim() const spent = offered().length claimed = [] diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts index b1b58643121..c8034b0af17 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts @@ -2,23 +2,13 @@ // still owes is written back during orderly teardown; a crash after take can still lose it. import { randomUUID } from 'node:crypto' -import { agentJournalSubmissionKey } from '../../../shared/agent-session-journal-item-key' import type { AgentSessionRecordStore } from '../../runtime/agent-session-record-store' import type { AgentSessionRecoveryCapsule } from '../../runtime/agent-session-recovery-capsule' import type { AgentSessionResumeMarker, AgentSessionResumeTrigger } from '../../../shared/agent-session-resume-marker' -import { - latestStructuredAgentSessionPrompt, - latestStructuredAgentSessionUserItem, - newestStructuredAgentSessionTurn, - projectStructuredAgentSessionStatus -} from '../../../shared/structured-agent-session-projection' -import type { - AgentJournalMessageItem, - AgentJournalRenderItem -} from '../../../shared/agent-session-journal-types' +import type { AgentJournalMessageItem } from '../../../shared/agent-session-journal-types' import type { AgentSessionMutationEnvelope, AgentSessionMutationResult, @@ -26,12 +16,9 @@ import type { } from '../../../shared/agent-session-wire' import type { AgentSessionJournal } from '../agent-session-journal/journal-store' import type { StructuredAgentSessionAdapter } from './structured-agent-session-adapter' -import { adapterSupportsRecord } from './structured-agent-session-provider-support' +import { createStructuredAgentSessionRestartCandidateReader } from './structured-agent-session-restart-candidates' import { createStructuredAgentSessionRestartClaim } from './structured-agent-session-restart-claim' -import { - structuredAgentSessionResumableSet, - type StructuredAgentSessionResumeCandidate -} from './structured-agent-session-restart-resume-set' +import type { StructuredAgentSessionResumeCandidate } from './structured-agent-session-restart-resume-set' import { resumeStructuredAgentSessionsFromRestart, StructuredAgentSessionResumeAdmission, @@ -117,45 +104,14 @@ export function createStructuredAgentSessionRestartResume( open: (sessionId) => surfaces.revealSession(sessionId) }) - const derive = ( - markers: readonly AgentSessionResumeMarker[], - leaseState: 'must-be-released' | 'may-be-held', - providerStopped = false, - pendingContinuationId?: string - ): StructuredAgentSessionResumeCandidate[] => { - const items = new Map() - const itemsFor = (sessionId: string): AgentJournalRenderItem[] => { - let snapshot = items.get(sessionId) - if (!snapshot) { - snapshot = sessions.get(sessionId)?.journal.snapshot().items ?? [] - if (pendingContinuationId) { - const ownItemId = agentJournalSubmissionKey(pendingContinuationId) - snapshot = snapshot.filter((item) => item.itemId !== ownItemId) - } - items.set(sessionId, snapshot) - } - return snapshot - } - return structuredAgentSessionResumableSet({ - markers, - getRecord: deps.store.getRecord, - supportsRecord: (record) => adapterSupportsRecord(deps.adapter, record), - waitingOnUser: (sessionId) => - projectStructuredAgentSessionStatus(itemsFor(sessionId)) === 'attention', - providerStopped, - journalTurn: (sessionId) => newestStructuredAgentSessionTurn(itemsFor(sessionId)), - journalSubmission: (sessionId, clientMessageId) => - sessions - .get(sessionId) - ?.journal.submissions() - .find((submission) => submission.clientMessageId === clientMessageId) ?? null, - latestPrompt: (sessionId) => latestStructuredAgentSessionPrompt(itemsFor(sessionId)), - latestUserItemId: (sessionId) => - latestStructuredAgentSessionUserItem(itemsFor(sessionId))?.itemId ?? null, - now: surfaces.now(), - leaseState - }) - } + // The predicate's reader, built once: the offer, the click and the write-back all ask it, and a + // second copy is how two of them come to disagree about what is resumable. + const derive = createStructuredAgentSessionRestartCandidateReader({ + sessions, + getRecord: deps.store.getRecord, + adapter: deps.adapter, + now: surfaces.now + }) const list = async (): Promise => { await claim.evidence() @@ -307,10 +263,18 @@ export function createStructuredAgentSessionRestartResume( // capsule instead of just replacing it: those sessions were never revealed, so the predicate // has no journal to judge them by and would refuse every one. Answering for an offer nobody // read is how it gets deleted unseen, so it carries forward untouched. + // + // A take that FAILED is the third case. The durable copy is then intact and unknowable, so + // only this teardown's own witnesses may go out — and when it has none, writing at all would + // delete an offer no one here could even see. let carried: AgentSessionResumeMarker[] = [] try { const owed = await claim.owed() - if (!owed.claimed) { + if (owed.unreadable) { + if (confirmedMarkers.size === 0) { + return + } + } else if (!owed.claimed) { carried = owed.markers } else { const stillResumable = new Set( diff --git a/src/renderer/src/components/NativeChatResumeOnRestartModal.test.tsx b/src/renderer/src/components/NativeChatResumeOnRestartModal.test.tsx index aae036bc758..52ccc901e40 100644 --- a/src/renderer/src/components/NativeChatResumeOnRestartModal.test.tsx +++ b/src/renderer/src/components/NativeChatResumeOnRestartModal.test.tsx @@ -87,8 +87,7 @@ afterEach(() => { it.each([ ['Reconnect 1', 'agentSession.restartResume'], - ['Reconnect and continue', 'agentSession.restartContinue'], - ['Not now', 'agentSession.restartResumableDismiss'] + ['Reconnect and continue', 'agentSession.restartContinue'] ])('keeps next-launch preference out of the current %s action', async (label, method) => { const action = Promise.withResolvers() rpc.mockImplementation(async (_target, calledMethod) => { @@ -102,30 +101,30 @@ it.each([ await act(async () => checkbox(2).click()) await act(async () => button(label).click()) expect(useAppStore.getState().settings?.nativeChatResumeWorkOnRestart).toBe(true) - expect(rpc.mock.calls.map((call) => [call[1], call[2]])).toEqual( - method === 'agentSession.restartResumableDismiss' - ? [['agentSession.restartResumable', undefined]] - : [ - ['agentSession.restartResumable', undefined], - [method, { sessionIds: ['a'] }] - ] - ) + expect(rpc.mock.calls.map((call) => [call[1], call[2]])).toEqual([ + ['agentSession.restartResumable', undefined], + [method, { sessionIds: ['a'] }] + ]) await act(async () => action.resolve({ results: [{ sessionId: 'a', outcome: 'resumed' }], continued: [{ sessionId: 'a', outcome: 'continued' }] }) ) - expect(rpc).toHaveBeenCalledTimes(method === 'agentSession.restartResumableDismiss' ? 1 : 2) + expect(rpc).toHaveBeenCalledTimes(2) }) +// Snoozing saves the preference like every other way out of the dialog, and calls NOTHING: the +// offer is the host's and stays exactly where it was. it('keeps Not now available through the status-bar offer', async () => { rpc.mockImplementation(async (_target, method) => method === 'agentSession.restartResumable' ? { sessions: offered } : { results: [] } ) await act(async () => root.render()) + await act(async () => checkbox(2).click()) await act(async () => button('Not now').click()) - expect(rpc).toHaveBeenCalledTimes(1) + expect(useAppStore.getState().settings?.nativeChatResumeWorkOnRestart).toBe(true) + expect(rpc.mock.calls.map((call) => call[1])).toEqual(['agentSession.restartResumable']) expect(offerIds()).toEqual(['a', 'b']) expect(document.querySelector('[role="dialog"]')).toBeNull() })