fix(native-chat): let the reservation holder win an overlapping adoption

Two adoptions of the same Codex pane destroyed each other. The pane binding
was released by pty id alone, gated on a `priorBoundSessionId` sampled before
the reservation, and both attempts carry the same sessionId — so the attempt
whose reservation had been superseded unbound the pane the winner was proving
on, `admitProof` refused the winner's `/status` keystroke, and both failed.

The write gate now records which attempt holds a pane, keyed on the spawn
token the reservation already mints: a claim supersedes another attempt's but
never a settled owner's, the release is a compare-and-clear, and a proven
owner settles the binding so no later attempt can take it. No lock is needed;
the reservation release beside it was already fence-scoped and unchanged.

A failed `releaseAdoptedTuiReservation` stays swallowed — the proof error is
the one the user must act on — but is now logged with session, fence and spawn
token, because a release that never persisted leaves the lease latched and
refuses every later attempt with no record of why.

Adoption's no-record branch is deleted. The only caller reserves the lease
first and nothing deletes a record, so it minted an owner for a session the
store had never heard of in unit tests only; its test now drives the real
reserve-then-adopt sequence, and a new one pins the refusal.

Launch-at-chat discarded `waitForAgentReady`'s result, so a readiness timeout
still flipped the tab to chat and adoption then probed a shell. It stays in
terminal view instead: the terminal is usable and the view toggle re-runs
adoption whenever the user wants it.

Each fix was reverted against its new test to confirm the test reddens; all
four defects had been living under green suites.
This commit is contained in:
Brennan Benson
2026-08-19 20:43:00 -07:00
parent e473bc5f1a
commit ca409f0634
9 changed files with 291 additions and 65 deletions
@@ -79,15 +79,34 @@ afterEach(async () => {
await rm(root, { recursive: true, force: true })
})
/** What the runtime does before it may type `/status` into the pane it is adopting. */
async function reserveAdoption(attach: AgentSessionAttachParams, spawnToken: string) {
return host.reserveAdoptedTuiOwner({
caller: CALLER,
sessionId: attach.envelope.sessionId,
clientOperationId: attach.envelope.clientOperationId,
fingerprint: attach.envelope.payloadFingerprint,
location: attach.location,
provider: 'codex',
accountHome: attach.accountHome,
spawnToken,
claimKeyId: 'key-1'
})
}
describe('structured TUI adoption', () => {
it('mints a durable TUI owner without starting a native provider', async () => {
const adoptedOwner = owner()
const attach = params()
await expect(reserveAdoption(attach, adoptedOwner.process.spawnToken)).resolves.toMatchObject({
ok: true,
fence: 1
})
const result = await host.adoptTuiOwner({
caller: CALLER,
params: params(),
owner: adoptedOwner,
claimKeyId: 'key-1'
params: attach,
owner: adoptedOwner
})
expect(result).toMatchObject({ ok: true, replayed: false, fence: 1 })
@@ -107,12 +126,7 @@ describe('structured TUI adoption', () => {
const invalid = params()
invalid.envelope.payloadFingerprint = '0'.repeat(64)
const result = await host.adoptTuiOwner({
caller: CALLER,
params: invalid,
owner: owner(),
claimKeyId: 'key-1'
})
const result = await host.adoptTuiOwner({ caller: CALLER, params: invalid, owner: owner() })
expect(result).toMatchObject({
ok: false,
@@ -121,4 +135,14 @@ describe('structured TUI adoption', () => {
expect(store.getRecord(SESSION)).toBeNull()
expect(acquire).not.toHaveBeenCalled()
})
// The reservation is the only way in: adoption has no second mode that acquires the lease
// itself, so nothing here may invent an owner for a session the store has never heard of.
it('refuses to adopt a session that holds no reservation', async () => {
await expect(
host.adoptTuiOwner({ caller: CALLER, params: params(), owner: owner() })
).rejects.toThrow('agent_session_ownership_unknown')
expect(store.getRecord(SESSION)).toBeNull()
expect(acquire).not.toHaveBeenCalled()
})
})
@@ -5,11 +5,7 @@ import type {
AgentSessionMutationResult
} from '../../../shared/agent-session-wire'
import type { AgentSessionAttachParams } from './structured-agent-session-attach'
import {
admitAttachOrRefuse,
attachJournal,
reserveRequestFor
} from './structured-agent-session-attach'
import { admitAttachOrRefuse, attachJournal } from './structured-agent-session-attach'
import type {
StructuredAgentSessionCaller,
StructuredAgentSessionHostDeps,
@@ -22,8 +18,6 @@ export type StructuredTuiAdoptionRequest = {
caller: StructuredAgentSessionCaller
params: AgentSessionAttachParams
owner: StructuredTuiOwner
claimKeyId: string
launchEnv?: Record<string, string>
}
export function adoptStructuredTuiOwner(
@@ -48,32 +42,19 @@ async function adopt(
return admitted
}
let record = input.deps.store.getRecord(sessionId)
let replayed = record !== null
if (record && isStructuredTuiAdoptionReservation(record, owner.process.spawnToken)) {
if (!record) {
// Adoption reserves the lease before it may prove the pane, and nothing deletes a record, so
// this call always runs against one. Refusing beats reserving here: a second acquisition mode
// that no caller can enter is coverage that proves nothing.
throw new Error('agent_session_ownership_unknown')
}
let replayed = true
if (isStructuredTuiAdoptionReservation(record, owner.process.spawnToken)) {
// This adoption's own pre-proof reservation: turn it into an owner rather than reserve again.
replayed = false
record = await proveAdoptedTuiOwner(input, record)
} else if (record) {
assertMatchingTuiOwner(record, owner)
} else {
const reserved = await input.deps.store.reserveOwner(
reserveRequestFor({
sessionId,
params,
authority: {
spawnToken: owner.process.spawnToken,
claimKeyId: input.claimKeyId,
handoffOperationId: params.envelope.clientOperationId,
probe: { outcome: 'reservation-unused' },
...(input.launchEnv ? { launchEnv: input.launchEnv } : {})
},
callerKey: input.caller.callerKey,
fingerprint: admitted.fingerprint,
now: input.now()
})
)
replayed = reserved.disposition === 'replayed'
record = await proveAdoptedTuiOwner(input, reserved.record)
assertMatchingTuiOwner(record, owner)
}
const attached = await attachJournal({
record,
@@ -76,6 +76,48 @@ describe('binding lifecycle', () => {
})
})
describe('overlapping adoption attempts on one pane', () => {
beforeEach(() => {
gate.attachRecordLookup((sessionId) => records.get(sessionId) ?? null)
})
it('keeps the newer attempt bound when the one it superseded gives up', () => {
gate.bindPtyForAttempt(PTY_ID, SESSION_ID, 'spawn-a')
expect(gate.bindPtyForAttempt(PTY_ID, SESSION_ID, 'spawn-b')).toBe(true)
// Both attempts carry the same session, so only the spawn token can tell this release apart.
expect(gate.releasePtyAttempt(PTY_ID, 'spawn-a')).toBe(false)
expect(gate.boundSessionId(PTY_ID)).toBe(SESSION_ID)
expect(gate.releasePtyAttempt(PTY_ID, 'spawn-b')).toBe(true)
expect(gate.boundSessionId(PTY_ID)).toBeNull()
})
it('leaves a settled owner pane alone when a later attempt fails', () => {
gate.bindPty(PTY_ID, SESSION_ID)
expect(gate.bindPtyForAttempt(PTY_ID, SESSION_ID, 'spawn-late')).toBe(false)
expect(gate.releasePtyAttempt(PTY_ID, 'spawn-late')).toBe(false)
expect(gate.boundSessionId(PTY_ID)).toBe(SESSION_ID)
})
it('settles a proven attempt so no later attempt can release its pane', () => {
gate.bindPtyForAttempt(PTY_ID, SESSION_ID, 'spawn-a')
expect(gate.settlePtyAttempt(PTY_ID, 'spawn-a')).toBe(true)
expect(gate.settlePtyAttempt(PTY_ID, 'spawn-a')).toBe(false)
expect(gate.releasePtyAttempt(PTY_ID, 'spawn-a')).toBe(false)
expect(gate.boundSessionId(PTY_ID)).toBe(SESSION_ID)
})
it('settles nothing once the pane is gone', () => {
gate.bindPtyForAttempt(PTY_ID, SESSION_ID, 'spawn-a')
gate.unbindPty(PTY_ID)
expect(gate.settlePtyAttempt(PTY_ID, 'spawn-a')).toBe(false)
expect(gate.boundSessionId(PTY_ID)).toBeNull()
})
})
describe('admission through the store', () => {
beforeEach(() => {
gate.attachRecordLookup((sessionId) => records.get(sessionId) ?? null)
@@ -31,8 +31,17 @@ const ADMITTED_UNBOUND: AgentSessionPtyWriteAdmission = {
runtimeFence: null
}
/**
* A pane binding, plus the adoption attempt that is still proving it.
*
* `attemptToken` is null once an owner is proven — a settled binding no attempt may take away.
* While it is non-null the binding belongs to that one in-flight attempt, which is the only thing
* that tells two overlapping adoptions apart: both carry the same sessionId.
*/
type BoundPane = { sessionId: string; attemptToken: string | null }
export class AgentSessionPtyWriteGate {
private readonly sessionIdByPtyId = new Map<string, string>()
private readonly panesByPtyId = new Map<string, BoundPane>()
private lookup: AgentSessionRecordLookup | null = null
/** Point the gate at the durable store. Until this is called nothing can be enforced. */
@@ -42,24 +51,62 @@ export class AgentSessionPtyWriteGate {
detachRecordLookup(): void {
this.lookup = null
this.sessionIdByPtyId.clear()
this.panesByPtyId.clear()
}
bindPty(ptyId: string, sessionId: string): void {
this.sessionIdByPtyId.set(ptyId, sessionId)
this.panesByPtyId.set(ptyId, { sessionId, attemptToken: null })
}
unbindPty(ptyId: string): void {
this.sessionIdByPtyId.delete(ptyId)
this.panesByPtyId.delete(ptyId)
}
/**
* Claim the pane for one adoption attempt, identified by the spawn token its reservation minted.
* A settled owner keeps the pane; another attempt's claim is superseded, because the newest
* reservation is the one the record now names. Callers must already have refused a pane bound to
* a different session.
*/
bindPtyForAttempt(ptyId: string, sessionId: string, attemptToken: string): boolean {
const current = this.panesByPtyId.get(ptyId)
if (current && current.attemptToken === null) {
return false
}
this.panesByPtyId.set(ptyId, { sessionId, attemptToken })
return true
}
/** Promote this attempt's claim to a settled binding once its owner is proven. */
settlePtyAttempt(ptyId: string, attemptToken: string): boolean {
const current = this.panesByPtyId.get(ptyId)
if (current?.attemptToken !== attemptToken) {
return false
}
this.panesByPtyId.set(ptyId, { sessionId: current.sessionId, attemptToken: null })
return true
}
/**
* Compare-and-clear: hand the pane back only while this attempt still holds it. A losing attempt
* that unbinds by pty alone rips the pane out from under the attempt that superseded it, and then
* that one's proof is refused too — both fail where one should have won.
*/
releasePtyAttempt(ptyId: string, attemptToken: string): boolean {
if (this.panesByPtyId.get(ptyId)?.attemptToken !== attemptToken) {
return false
}
this.panesByPtyId.delete(ptyId)
return true
}
boundSessionId(ptyId: string): string | null {
return this.sessionIdByPtyId.get(ptyId) ?? null
return this.panesByPtyId.get(ptyId)?.sessionId ?? null
}
/** False while no PTY is bound, which is every write path in today's builds. */
get enforcing(): boolean {
return this.lookup !== null && this.sessionIdByPtyId.size > 0
return this.lookup !== null && this.panesByPtyId.size > 0
}
admit(ptyId: string): AgentSessionPtyWriteAdmission {
@@ -117,11 +164,11 @@ export class AgentSessionPtyWriteGate {
}
private binding(ptyId: string): AgentSessionPtyBinding | null {
const sessionId = this.sessionIdByPtyId.get(ptyId)
if (sessionId === undefined) {
const pane = this.panesByPtyId.get(ptyId)
if (pane === undefined) {
return null
}
return { sessionId, record: this.lookup?.(sessionId) ?? null }
return { sessionId: pane.sessionId, record: this.lookup?.(pane.sessionId) ?? null }
}
}
@@ -252,6 +252,52 @@ describe('structured Codex adoption against the real PTY write gate', () => {
})
}, 30_000)
it('lets the attempt holding the reservation win when two adoptions overlap', async () => {
// Both attempts park here: reservation taken and pane bound, proof not yet written.
const arrived: string[] = []
let releaseBoth: () => void = () => {}
const bothArrived = new Promise<void>((resolve) => {
releaseBoth = resolve
})
readStructuredTuiProcessIdentity.mockImplementation(
async (input: { hostId: string; spawnToken: string }) => {
if (!arrived.includes(input.spawnToken)) {
arrived.push(input.spawnToken)
if (arrived.length === 2) {
releaseBoth()
}
await bothArrived
}
return {
hostId: input.hostId,
pid: 4242,
processStartTimeMs: 1_700_000_000_000,
spawnToken: input.spawnToken
}
}
)
const superseded = rig.runtime.adoptStructuredAgentSessionTerminal(
{ ...adoptInput(), threadId: THREAD_ID },
{ callerKey: 'renderer-1' }
)
superseded.catch(() => undefined)
await vi.waitFor(() => expect(arrived).toHaveLength(1))
// The second attempt carries the same sessionId and supersedes the first one's reservation.
const winner = rig.runtime.adoptStructuredAgentSessionTerminal(
{ ...adoptInput(), threadId: THREAD_ID },
{ callerKey: 'renderer-1' }
)
await expect(winner).resolves.toMatchObject({ ok: true })
await expect(superseded).rejects.toThrow('could not verify its Codex session')
// The loser's cleanup ran against a pane and a reservation that were no longer its own.
expect(agentSessionPtyWriteGate.boundSessionId(PTY_ID)).toBe(SESSION_ID)
expect(rig.store.getRecord(SESSION_ID)).toMatchObject({
lease: { runtimeKind: 'tui', claimStatus: 'live', handoffStage: null }
})
}, 30_000)
it('refuses a pane another structured session already owns', async () => {
agentSessionPtyWriteGate.bindPty(PTY_ID, 'someone-elses-session')
@@ -215,6 +215,63 @@ describe('structured TUI launch tab binding', () => {
}
})
it('reports a reservation release that fails, without masking the proof error', async () => {
const runtime = terminalAdoptionHarness()
const host = structuredHostDouble({
releaseAdoptedTuiReservation: vi.fn(async () => {
throw new Error('store write failed')
})
})
setStructuredAgentSessionHost(host as never)
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
proveCodexTuiRollout.mockRejectedValueOnce(
new Error('The agent terminal did not prove the expected Codex rollout.')
)
readStructuredTuiProcessIdentity.mockResolvedValueOnce({
hostId: 'local',
pid: 4242,
processStartTimeMs: 1_700_000_000_000,
spawnToken: 'spawn-adopt'
})
try {
await expect(
runtime.adoptStructuredAgentSessionTerminal(
{
envelope: {
sessionId: 'session-release-failure',
clientOperationId: 'operation-release-failure',
expectedRuntimeFence: null,
payloadFingerprint: 'f'.repeat(64)
},
worktree: `id:${WORKTREE_ID}`,
tabId: 'tab-adopt',
paneKey: 'tab-adopt:leaf-adopt',
ptyId: 'pty-adopt',
threadId: 'thread-adopt'
},
{ callerKey: 'renderer-1' }
)
).rejects.toThrow('did not prove the expected Codex rollout')
const reserved = host.reserveAdoptedTuiOwner.mock.calls[0]?.[0] as { spawnToken: string }
const logged = warn.mock.calls.find(
([message]) => message === '[native-chat] adopted TUI reservation release failed'
)?.[1] as { error: Error } | undefined
// A latched lease refuses every later attempt, so the cause cannot vanish with the throw.
expect(logged).toMatchObject({
sessionId: 'session-release-failure',
fence: 1,
spawnToken: reserved.spawnToken
})
expect(logged?.error.message).toBe('store write failed')
} finally {
warn.mockRestore()
setStructuredAgentSessionHost(null)
agentSessionPtyWriteGate.unbindPty('pty-adopt')
}
})
it('refuses a thread proof when the Codex process changes during it', async () => {
const runtime = terminalAdoptionHarness()
const host = structuredHostDouble()
+21 -17
View File
@@ -11469,7 +11469,7 @@ export class OrcaRuntimeService {
return reserved
}
const { fence, spawnToken } = reserved
agentSessionPtyWriteGate.bindPty(input.ptyId, sessionId)
agentSessionPtyWriteGate.bindPtyForAttempt(input.ptyId, sessionId, spawnToken)
let owned = false
try {
return await this.attachAdoptedStructuredTui({
@@ -11477,7 +11477,6 @@ export class OrcaRuntimeService {
caller,
host,
intent,
launchEnv,
fence,
spawnToken,
rootProcessId,
@@ -11487,15 +11486,27 @@ export class OrcaRuntimeService {
}
})
} finally {
if (!owned) {
if (owned) {
agentSessionPtyWriteGate.settlePtyAttempt(input.ptyId, spawnToken)
} else {
// A failed proof must leave nothing behind: an unreleased reservation is exactly the
// latched lease that makes the next attempt refuse itself.
if (priorBoundSessionId === null) {
agentSessionPtyWriteGate.unbindPty(input.ptyId)
}
// latched lease that makes the next attempt refuse itself. Both releases are scoped to this
// attempt's own spawn token, because a second adoption of the same pane carries the same
// sessionId — clearing by pty alone unbinds the pane the attempt that superseded us is
// proving on, and then neither can finish.
agentSessionPtyWriteGate.releasePtyAttempt(input.ptyId, spawnToken)
await host
.releaseAdoptedTuiReservation({ sessionId, fence, spawnToken })
.catch(() => undefined)
.catch((error: unknown) => {
// Swallowed on purpose: the proof error below is the one the user must see. But a
// release that never persisted leaves the lease latched, so it cannot also be silent.
console.warn('[native-chat] adopted TUI reservation release failed', {
sessionId,
fence,
spawnToken,
error
})
})
}
}
}
@@ -11505,14 +11516,13 @@ export class OrcaRuntimeService {
caller: StructuredAgentSessionCaller
host: NonNullable<ReturnType<typeof getStructuredAgentSessionHost>>
intent: AgentSessionAttachParams
launchEnv: Record<string, string>
fence: number
spawnToken: string
rootProcessId: number
pty: RuntimePtyWorktreeRecord
onOwned: () => void
}): Promise<AgentSessionMutationResult<AgentSessionAttachResult>> {
const { input, caller, host, intent, launchEnv, fence, spawnToken, rootProcessId, pty } = args
const { input, caller, host, intent, fence, spawnToken, rootProcessId, pty } = args
const readPty = (): RuntimePtyWorktreeRecord => {
const current = this.ptysById.get(input.ptyId)
if (!current?.connected || current.paneKey !== input.paneKey) {
@@ -11605,13 +11615,7 @@ export class OrcaRuntimeService {
runtimeKind: 'tui',
providerHandle: { kind: 'codex', threadId }
}
const adopted = await host.adoptTuiOwner({
caller,
params,
owner,
claimKeyId: this.agentSessionClaimSigner.keyId,
launchEnv
})
const adopted = await host.adoptTuiOwner({ caller, params, owner })
if (adopted.ok) {
// The pane stays bound to the reservation this call already made; only the owner is new.
this.adoptedStructuredTuiOwners.set(input.envelope.sessionId, owner)
@@ -259,7 +259,17 @@ export function launchAgentInNewTab(args: LaunchAgentInNewTabArgs): LaunchAgentI
void (promptDeliveryCompletion ?? Promise.resolve())
.catch(() => undefined)
.then(async () => {
await waitForAgentReady(tab.id, TUI_AGENT_CONFIG.codex.expectedProcess)
const ready = await waitForAgentReady(tab.id, TUI_AGENT_CONFIG.codex.expectedProcess)
if (!ready.ready) {
// Why: chat adoption proves the pane's foreground process. Flipping a tab whose Codex is
// not up yet trades a working terminal for an adoption error; the view toggle re-runs
// adoption whenever the user wants it.
console.warn('Codex was not ready for structured chat; staying in terminal view', {
tabId: tab.id,
reason: ready.reason
})
return
}
useAppStore.getState().setTabViewMode(tab.id, 'chat')
})
}
@@ -81,6 +81,21 @@ describe('structured chat adoption guard on the launch path', () => {
await vi.waitFor(() => expect(mockSetTabViewMode).toHaveBeenCalledWith('tab-1', 'chat'))
})
it('stays in terminal view when Codex never became ready', async () => {
mockWaitForAgentReady.mockResolvedValue({ ready: false, reason: 'timeout' })
const { launchAgentInNewTab } = await import('./launch-agent-in-new-tab')
launchAgentInNewTab({ agent: 'codex', worktreeId: 'wt-1' })
await vi.waitFor(() => expect(mockWaitForAgentReady).toHaveBeenCalled())
// Let every continuation the ready wait could have queued run before asserting it did not flip.
await new Promise((resolve) => setTimeout(resolve, 0))
await new Promise((resolve) => setTimeout(resolve, 0))
// Chat adoption probes the pane's foreground process: flipping now hands the user an adoption
// error in place of the terminal Codex is still starting in.
expect(mockSetTabViewMode).not.toHaveBeenCalledWith('tab-1', 'chat')
})
it('keeps an SSH Codex tab on the bridge', async () => {
store.repos = [{ id: 'repo-1', connectionId: 'ssh-a', path: '/repo' }]
const { launchAgentInNewTab } = await import('./launch-agent-in-new-tab')