From 41798d5811db486ee045274fbf86c7eeafe83d25 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 13 Sep 2026 19:35:09 -0700 Subject: [PATCH] fix(native-chat): read the wind-down a close owes from the live child MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An eviction recorded "nothing owed" whenever it ran over a session with no provider child of its own, and the retry then read that record in preference to the child in front of it. A session suspended to an agent terminal is exactly that shape, and the trip back to native re-acquires into the SAME session object rather than replacing it, so the next close skipped both the dead-generation settlement and the lease release — leaving the record claiming a live owner this host had just stopped, and a pending send unsettled. The obligation is now derived the way the quit sweep already derived it, from one shared predicate: a live child always owes a wind-down, and a remembered `false` only carries the obligation forward, never cancels it. Also drops a memoization in the history page that could never hit. Its key was the snapshot's items array, which the reducer rebuilds on every `snapshot()` call, so each backward page allocated a fresh key; the one reader that does share a snapshot across pages reads forward and never calls it. The comment claimed a multi-page read filtered once, which was not true of either path. Tests: the handoff round trip that strands the lease, and the quit sweep picking up an eviction whose close retry never came. --- .../agent-session-history-page.ts | 16 +- .../structured-agent-session-host-lifetime.ts | 11 +- ...red-agent-session-surface-lifetime.test.ts | 197 +++++++++++++++++- 3 files changed, 196 insertions(+), 28 deletions(-) diff --git a/src/main/native-chat/agent-session-wire/agent-session-history-page.ts b/src/main/native-chat/agent-session-wire/agent-session-history-page.ts index 200e8982502..1ca44bbc3ea 100644 --- a/src/main/native-chat/agent-session-wire/agent-session-history-page.ts +++ b/src/main/native-chat/agent-session-wire/agent-session-history-page.ts @@ -101,23 +101,9 @@ export function readAgentSessionHistory( * Retiring on the way out instead lets a window land entirely on retired rows and return an empty * page that still claims older history — a cursor that never advances, which is a reader spinning * on one request rather than a transcript that finished loading. - * - * Keyed on the snapshot's own items array, which the reducer rebuilds on every change, so a - * multi-page read over one snapshot filters once. */ -const renderableTimelines = new WeakMap< - readonly AgentJournalRenderItem[], - AgentJournalRenderItem[] ->() - function renderableTimeline(snapshot: AgentJournalSnapshot): AgentJournalRenderItem[] { - const cached = renderableTimelines.get(snapshot.items) - if (cached) { - return cached - } - const items = withoutRetiredProviderExitStatusItems(snapshot.items, snapshot.sessionId) - renderableTimelines.set(snapshot.items, items) - return items + return withoutRetiredProviderExitStatusItems(snapshot.items, snapshot.sessionId) } /** diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts index 4cb84fd42ca..0cff20c831b 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts @@ -50,6 +50,13 @@ function hasProviderChild( return context.sessions.get(sessionId)?.hasProviderChild === true } +/** The wind-down this host owes for the session's child. A live child always owes one, whatever a + * previous childless eviction recorded — the same session object is re-acquired in place on a + * handoff back to native, so a remembered `false` must never outrank the child in front of it. */ +function owesProviderChildWindDown(session: StructuredAgentSessionHostSession): boolean { + return session.hasProviderChild || session.owesProviderChildWindDown === true +} + /** Runs the eviction steps under a deadline. A step that fails — or runs out of time — aborts the * rest, which leaves the session indexed and the child loaded so the next close is a real retry. */ export async function evictHeldStructuredAgentSession( @@ -63,7 +70,7 @@ export async function evictHeldStructuredAgentSession( // The obligation OUTLIVES the child. `hasProviderChild` is retired the instant the adapter // proves the exit, so a step that aborts after that point would otherwise leave the retry // reading "no child here" and skipping the settlement and the lease release it still owes. - const owesWindDown = session.owesProviderChildWindDown ?? session.hasProviderChild + const owesWindDown = owesProviderChildWindDown(session) session.owesProviderChildWindDown = owesWindDown let settlementError: unknown const eviction: StructuredAgentSessionEvictionContext = { @@ -127,7 +134,7 @@ export async function evictOwnedStructuredAgentSessions( retainOnFailure: Set ): Promise { const ownedSessionIds = [...context.sessions] - .filter(([, session]) => session.hasProviderChild || session.owesProviderChildWindDown === true) + .filter(([, session]) => owesProviderChildWindDown(session)) .map(([sessionId]) => sessionId) // Retained up front and cleared only once an eviction settles: the quit phase is bounded, and a // timeout leaves these still running. Closing their journals underneath them is the one outcome diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.ts index 849a820c16b..2538b61c84b 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.ts @@ -3,7 +3,7 @@ // Two leaks meet here and each has to be tested against the real host, not a double: a chat that // closes without stopping its app-server, and a launch that starts one for every record on disk. -import { mkdtemp, rm } from 'node:fs/promises' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest' @@ -20,7 +20,11 @@ import { AgentSessionRecordStore } from '../../runtime/agent-session-record-stor import type { StructuredAgentSessionAdapter } from './structured-agent-session-adapter' import type { StructuredAgentSessionEventSink } from './structured-agent-session-event-sink' import { StructuredAgentSessionHost } from './structured-agent-session-host' -import type { StructuredAgentSessionHandoffTransport } from './structured-agent-session-handoff-types' +import type { + StructuredAgentSessionHandoffTransport, + StructuredTuiOwner +} from './structured-agent-session-handoff-types' +import { StructuredHandoffTestRequests } from './structured-agent-session-handoff-test-requests' import { unexpectedProviderExitOutcome } from './structured-agent-session-dead-generation-settlement' import type { StructuredAgentSessionStatusSink } from './structured-agent-session-status-feed' import { @@ -124,8 +128,107 @@ function waitOutSeveralGraceWindows(): Promise { return new Promise((resolve) => setTimeout(resolve, GRACE_MS * 20)) } +const handoffRequests = new StructuredHandoffTestRequests( + NOW, + SESSION, + () => store.getRecord(SESSION)?.lease.runtimeFence ?? 0 +) +/** Whether the terminal this host handed the session to can be reached again. */ +let tuiRecoverable: boolean + +/** One operation-id source with the rest of the suite, so the durable ledger sees no duplicate. */ +function handoffRequest(direction: 'to-tui' | 'to-native') { + return handoffRequests.request(direction, 'now', { operationId: hostTestOperationId() }) +} + +function tuiOwner(fence: number, spawnToken: string, transcriptPath: string): StructuredTuiOwner { + return { + terminal: { handle: 'term-tui', tabId: 'tab-tui', paneKey: 'pane-tui', ptyId: 'pty-tui' }, + process: { hostId: 'local', pid: 5200, processStartTimeMs: NOW, spawnToken }, + link: { + linkId: `tui-link-${fence}`, + handle: { provider: 'codex', threadId: THREAD }, + origin: 'resumed', + mintedAtFence: fence, + observedAt: NOW + }, + transcriptPath + } +} + +/** A codex rollout the return trip can import, so a real to-native handoff has history to read. */ +async function writeTuiTranscript(): Promise { + const sessionsDir = join(root, 'codex-home', 'sessions', '2026', '08', '12') + await mkdir(sessionsDir, { recursive: true }) + const transcriptPath = join(sessionsDir, `rollout-2026-08-12T10-00-00-${THREAD}.jsonl`) + await writeFile( + transcriptPath, + `${JSON.stringify({ + type: 'session_meta', + timestamp: '2026-08-12T10:00:00.000Z', + payload: { id: THREAD, session_id: THREAD } + })}\n`, + 'utf8' + ) + return transcriptPath +} + +/** Replaces the current host with one that can hand the session to a terminal and take it back. */ +function openHandoffHost(transcriptPath: string): void { + openHost(undefined, { + hostLabel: 'Test host', + launchTui: async ({ fence, spawnToken }) => tuiOwner(fence, spawnToken, transcriptPath), + reproveTuiOwner: async ({ owner }) => owner, + recoverTuiOwner: async (record) => { + if (!tuiRecoverable) { + throw new Error('the owning terminal could not be reached') + } + return tuiOwner( + record.lease.runtimeFence, + record.lease.reservedSpawnToken ?? 'recovered', + transcriptPath + ) + }, + stopRecoveredOwner: async () => undefined, + closeTuiOwner: async (owner) => ({ transcriptPath: owner.transcriptPath }), + waitForTuiExit: async (owner) => ({ transcriptPath: owner.transcriptPath }), + waitForTuiIdleOrExit: async () => 'idle', + tuiStatus: () => 'idle' + }) +} + +/** Fails the next eviction at `drain-published`, which leaves the session indexed for a retry. */ +function failNextDrain(): void { + vi.spyOn(host['runtimeState'].eventSinkFor(SESSION), 'drained').mockResolvedValueOnce({ + ok: false, + error: new Error('drain barrier lost') + }) +} + +/** The submissions as they stood when the session was forgotten; its journal is gone after that. */ +function captureSettledSubmissions(): { value: AgentJournalSubmission[] } { + const captured: { value: AgentJournalSubmission[] } = { value: [] } + const journal = host['sessions'].get(SESSION)!.journal + const closeJournal = journal.close.bind(journal) + vi.spyOn(journal, 'close').mockImplementation(async () => { + captured.value = journal.snapshot().submissions + await closeJournal() + }) + return captured +} + +async function sendPending(text: string): Promise { + dispatch.mockResolvedValueOnce({ state: 'admitted' }) + const body = hostTestMessage(text) + expect( + await host.send(CALLER, { envelope: envelope('agentSession.send', { body }), body }) + ).toMatchObject({ ok: true, value: { submission: { dispatchState: 'pending' } } }) +} + beforeEach(async () => { root = await mkdtemp(join(tmpdir(), 'orca-surface-lifetime-')) + handoffRequests.reset() + tuiRecoverable = true resetHostTestOperationIds() sink = null hostErrors = [] @@ -286,14 +389,7 @@ describe('a chat that closes', () => { ok: false, error: new Error('drain barrier lost') }) - // The journal is gone once the retry forgets the session, so read it on its way out. - let settledSubmissions: AgentJournalSubmission[] = [] - const journal = session!.journal - const closeJournal = journal.close.bind(journal) - vi.spyOn(journal, 'close').mockImplementation(async () => { - settledSubmissions = journal.snapshot().submissions - await closeJournal() - }) + const settled = captureSettledSubmissions() await expect(host.close(SESSION)).rejects.toMatchObject({ step: 'drain-published' }) // The child is proven gone, but the wind-down it owes is not done: nothing settled, no release. @@ -306,7 +402,7 @@ describe('a chat that closes', () => { claimStatus: 'released', ownerProcess: null }) - expect(hasUnansweredStructuredAgentSessionDispatch(settledSubmissions)).toBe(false) + expect(hasUnansweredStructuredAgentSessionDispatch(settled.value)).toBe(false) }) }) @@ -709,3 +805,82 @@ describe('an unexpected provider exit', () => { expect(acquire).toHaveBeenCalledTimes(2) }) }) + +describe('a chat handed to a terminal and taken back', () => { + // The wind-down a close owes belongs to the child in front of it, not to whatever the LAST + // eviction found. A session a terminal owns is indexed with no child of its own, so a close + // there records "nothing owed" — and the trip back re-acquires into that SAME session object. + it('settles and releases the child it was given back', async () => { + const transcriptPath = await writeTuiTranscript() + await host.flushAllStreamedEvents() + openHandoffHost(transcriptPath) + await attach() + expect(await host.requestHandoff(CALLER, handoffRequest('to-tui'))).toMatchObject({ ok: true }) + await vi.waitFor(async () => + expect(await host.handoffStatus(SESSION)).toMatchObject({ owner: 'tui' }) + ) + + // The app restarts and cannot reach the terminal, so this generation restores the session for + // reading and holds no handle to the owner it would otherwise stop on a close. + await host.flushAllStreamedEvents() + store = await AgentSessionRecordStore.open({ directory: join(root, 'store'), hostId: 'local' }) + tuiRecoverable = false + openHandoffHost(transcriptPath) + await host.restoreReadableSessions() + expect(store.getRecord(SESSION)?.lease).toMatchObject({ + runtimeKind: 'tui', + claimStatus: 'live' + }) + + failNextDrain() + await expect(host.close(SESSION)).rejects.toMatchObject({ step: 'drain-published' }) + expect(host.hasSession(SESSION)).toBe(true) + + // The terminal answers again, and the status read the reopened pane makes recovers the owner. + tuiRecoverable = true + expect(await host.handoffStatus(SESSION)).toMatchObject({ owner: 'tui' }) + expect(await host.requestHandoff(CALLER, handoffRequest('to-native'))).toMatchObject({ + ok: true + }) + await vi.waitFor(async () => + expect(await host.handoffStatus(SESSION)).toMatchObject({ owner: 'native' }) + ) + expect(host['sessions'].get(SESSION)?.hasProviderChild).toBe(true) + await sendPending('pending when the retaken chat closes') + const settled = captureSettledSubmissions() + + await expect(host.close(SESSION)).resolves.toBeUndefined() + + expect(store.getRecord(SESSION)?.lease).toMatchObject({ + claimStatus: 'released', + ownerProcess: null + }) + expect(hasUnansweredStructuredAgentSessionDispatch(settled.value)).toBe(false) + }) +}) + +describe('a quit over an eviction that never got its retry', () => { + // Nothing calls `close` a second time when the user quits instead of reopening the chat, so the + // quit sweep is the last thing that can hand the lease back — and it only reaches the session if + // it still counts a stopped child's unfinished wind-down as owed. + it('finishes the wind-down the aborted close left behind', async () => { + await attach() + await sendPending('pending across an abandoned eviction') + const settled = captureSettledSubmissions() + failNextDrain() + + await expect(host.close(SESSION)).rejects.toMatchObject({ step: 'drain-published' }) + expect(host['sessions'].get(SESSION)?.hasProviderChild).toBe(false) + expect(store.getRecord(SESSION)?.lease.claimStatus).not.toBe('released') + + await host.flushAllStreamedEvents() + + expect(closeSession).toHaveBeenCalledOnce() + expect(host.hasSession(SESSION)).toBe(false) + expect(store.getRecord(SESSION)?.lease).toMatchObject({ + claimStatus: 'released', + ownerProcess: null + }) + expect(hasUnansweredStructuredAgentSessionDispatch(settled.value)).toBe(false) + }) +})