From 6e9de5fa58d2bacbb4d35bc754fbd4cdb8744c26 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:19:20 -0700 Subject: [PATCH] fix(orchestration): revalidate an attempted Enter instead of resending it (#19911) When a PTY retires mid-delivery, every staged message was marked undelivered, which made all of them redeliverable. That is right for a pointer whose Enter never fired, but an Enter that was already written may have landed: redelivering it types the same mail into the pane a second time. The Enter timer is cleared at the top of retirement, so a RESERVED or WRITE_ATTEMPTED pointer provably never submitted and is released. An ENTER_ATTEMPTED pointer is ambiguous and now stays at its phase for the resume path to revalidate, matching the policy mailbox-pointer-submit.ts already documents for an unverifiable settlement. Co-authored-by: Merge Sim --- .../orchestration/mailbox-pointer-delivery.ts | 18 ++++- .../mailbox-pointer-stage.test.ts | 69 +++++++++++++++++++ .../orchestration/mailbox-pointer-stage.ts | 1 + .../orchestration/mailbox-pointer-state.ts | 2 + .../terminal-send-stale-leaf-liveness.test.ts | 7 +- 5 files changed, 93 insertions(+), 4 deletions(-) diff --git a/src/main/runtime/orchestration/mailbox-pointer-delivery.ts b/src/main/runtime/orchestration/mailbox-pointer-delivery.ts index 11fbc08229d..b16e5c474b3 100644 --- a/src/main/runtime/orchestration/mailbox-pointer-delivery.ts +++ b/src/main/runtime/orchestration/mailbox-pointer-delivery.ts @@ -9,6 +9,10 @@ import { OrchestrationMailboxPointerState, type OrchestrationMailboxDeliveryFlight } from './mailbox-pointer-state' +import { + MAILBOX_POINTER_RESERVED, + MAILBOX_POINTER_WRITE_ATTEMPTED +} from './db/messages/mailbox-pointer-enter-state' import { resumePendingOrchestrationMailboxPointer } from './mailbox-pointer-resume' import { stageOrchestrationMailboxPointer } from './mailbox-pointer-stage' @@ -163,7 +167,19 @@ export class OrchestrationMailboxPointerDelivery { db.close() }) }) + +describe('retiring a pty mid-delivery', () => { + // Why: an Enter that was already written may have landed. Releasing it would send the same + // mail a second time, so only phases that provably never submitted become redeliverable. + it('leaves an attempted Enter at its phase instead of making it redeliverable', async () => { + vi.useFakeTimers() + const db = new OrchestrationDb(':memory:') + const settlements: ((settlement: WriteSettlement) => void)[] = [] + const writePty = vi.fn( + () => + new Promise((resolve) => { + settlements.push(resolve) + }) as unknown as WriteSettlement + ) + try { + const message = db.insertMessage({ from: 'a', to: 'run:run-1', subject: 'mail' }) + const delivery = new OrchestrationMailboxPointerDelivery(pointerDeps(db, writePty) as never) + delivery.deliver(LEAF, { mailboxHandle: 'run:run-1' }) + + // Settle the pointer write, so the pane reaches WRITE_ATTEMPTED and arms the Enter. + settlements[0]?.(WRITE_ACCEPTED) + await vi.advanceTimersByTimeAsync(0) + expect(db.getMessageById(message.id)?.pointer_enter_pending).toBe(2) + + // Fire the Enter but never settle it: this is the ambiguous state. + await vi.advanceTimersByTimeAsync(600) + expect(db.getMessageById(message.id)?.pointer_enter_pending).toBe(3) + + delivery.retirePty('pty-1') + expect(db.getMessageById(message.id)).toMatchObject({ + pointer_enter_pending: 3, + read: 0 + }) + } finally { + db.close() + vi.useRealTimers() + } + }) + + it('releases a pointer whose Enter never fired', async () => { + vi.useFakeTimers() + const db = new OrchestrationDb(':memory:') + const settlements: ((settlement: WriteSettlement) => void)[] = [] + const writePty = vi.fn( + () => + new Promise((resolve) => { + settlements.push(resolve) + }) as unknown as WriteSettlement + ) + try { + const message = db.insertMessage({ from: 'a', to: 'run:run-1', subject: 'mail' }) + const delivery = new OrchestrationMailboxPointerDelivery(pointerDeps(db, writePty) as never) + delivery.deliver(LEAF, { mailboxHandle: 'run:run-1' }) + settlements[0]?.(WRITE_ACCEPTED) + await vi.advanceTimersByTimeAsync(0) + expect(db.getMessageById(message.id)?.pointer_enter_pending).toBe(2) + + delivery.retirePty('pty-1') + expect(db.getMessageById(message.id)).toMatchObject({ + pointer_enter_pending: 0, + read: 0, + delivered_at: null + }) + } finally { + db.close() + vi.useRealTimers() + } + }) +}) diff --git a/src/main/runtime/orchestration/mailbox-pointer-stage.ts b/src/main/runtime/orchestration/mailbox-pointer-stage.ts index aed3b8e06b4..3c0bea2baad 100644 --- a/src/main/runtime/orchestration/mailbox-pointer-stage.ts +++ b/src/main/runtime/orchestration/mailbox-pointer-stage.ts @@ -58,6 +58,7 @@ export function stageOrchestrationMailboxPointer message.id) try { if ( diff --git a/src/main/runtime/orchestration/mailbox-pointer-state.ts b/src/main/runtime/orchestration/mailbox-pointer-state.ts index 149d25057b5..d52374a4888 100644 --- a/src/main/runtime/orchestration/mailbox-pointer-state.ts +++ b/src/main/runtime/orchestration/mailbox-pointer-state.ts @@ -6,6 +6,8 @@ export type OrchestrationMailboxDeliveryFlight = { submitEnter: (() => void) | null deferredUntilIdle: boolean idleObservedWhileDeferred: boolean + /** The incarnation that staged this flight, so retirement can name the rows it owns. */ + processIncarnation?: string } export type ParkedOrchestrationMailboxDelivery = { diff --git a/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts b/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts index 7d173055b3a..b7c5ed95068 100644 --- a/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts +++ b/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts @@ -386,6 +386,7 @@ function makeOrchestrationDbStub(toHandle: () => string) { runMailbox, markAsDelivered, markAsUndelivered, + releaseMailboxPointerEnter, stageMailboxPointerEnter, insert(subject: string, type: StoredMessageRow['type'] = 'status'): void { rows.push({ @@ -759,7 +760,7 @@ describe('push-on-idle orchestration delivery absence gate', () => { await vi.advanceTimersByTimeAsync(500) expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(0) expect(stub.stageMailboxPointerEnter).toHaveBeenCalledOnce() - expect(stub.markAsUndelivered).toHaveBeenCalledOnce() + expect(stub.releaseMailboxPointerEnter).toHaveBeenCalledOnce() expect(stub.rows[0].delivered_at).toBeNull() // The replacement's own delivery starts a fresh flight and completes — @@ -804,7 +805,7 @@ describe('push-on-idle orchestration delivery absence gate', () => { await vi.advanceTimersByTimeAsync(500) expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(0) expect(stub.stageMailboxPointerEnter).toHaveBeenCalledOnce() - expect(stub.markAsUndelivered).toHaveBeenCalledOnce() + expect(stub.releaseMailboxPointerEnter).toHaveBeenCalledOnce() // No stray settle flushed the parked trigger into the dead pty. expect(write).toHaveBeenCalledTimes(1) expect(stub.rows.every((row) => row.delivered_at === null)).toBe(true) @@ -835,7 +836,7 @@ describe('push-on-idle orchestration delivery absence gate', () => { await vi.advanceTimersByTimeAsync(500) expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(0) expect(stub.stageMailboxPointerEnter).toHaveBeenCalledOnce() - expect(stub.markAsUndelivered).toHaveBeenCalledOnce() + expect(stub.releaseMailboxPointerEnter).toHaveBeenCalledOnce() expect(stub.rows[0].delivered_at).toBeNull() } finally { vi.useRealTimers()