diff --git a/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts b/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts index bb9d55942c..24f847f53c 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-command-recovery.test.ts @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { RunnerCommandAccounting } from '../runner-session-types.ts'; import { afterEach, test, vi } from 'vitest'; import { AppError } from '@agent-device/kernel/errors'; import { IOS_SIMULATOR } from './device-fixtures.ts'; @@ -35,8 +36,7 @@ function makeRunnerSession(port: number): RunnerSession { testPromise: new Promise(() => {}), child: { pid: process.pid, exitCode: null }, state: 'ready', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), }; } diff --git a/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts b/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts index e1eb003d7e..fc3b353a9c 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-command-retry.test.ts @@ -1,11 +1,14 @@ import { beforeEach, test, vi } from 'vitest'; import assert from 'node:assert/strict'; import { IOS_SIMULATOR } from './device-fixtures.ts'; -import { createTestRequestCancellation, runnerConnectFailure } from './runner-session-fixtures.ts'; +import { + createTestRequestCancellation, + makeRunnerSession, + runnerConnectFailure, +} from './runner-session-fixtures.ts'; import { AppError } from '@agent-device/kernel/errors'; import { Deadline } from '../host.ts'; import { appleRunnerTestHost } from '../test-host.ts'; -import type { RunnerSession } from '../runner-session-types.ts'; const { mockEnsureRunnerSession, @@ -1090,21 +1093,6 @@ function assertDiagnosticDecision(expected: { ); } -function makeRunnerSession(overrides: Partial = {}): RunnerSession { - return { - sessionId: `session-${overrides.port ?? 8100}`, - device: IOS_SIMULATOR, - deviceId: IOS_SIMULATOR.id, - port: 8100, - xctestrunPath: '/tmp/runner.xctestrun', - jsonPath: '/tmp/runner.json', - testPromise: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }), - child: { pid: 1234, exitCode: null }, - state: 'ready', - ...overrides, - } as RunnerSession; -} - function makeRunnerArtifact( overrides: Partial = {}, ): RunnerXctestrunArtifact { diff --git a/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts b/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts index b658163c14..f18af7075b 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-disposal.test.ts @@ -1,7 +1,7 @@ import { afterEach, beforeEach, expect, test, vi } from 'vitest'; import { IOS_SIMULATOR, MACOS_DEVICE, TVOS_SIMULATOR } from './device-fixtures.ts'; import type { ExecResult } from '@agent-device/host-kit/command'; -import type { RunnerSession } from '../runner-session-types.ts'; +import { RunnerCommandAccounting, type RunnerSession } from '../runner-session-types.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import { makeRunnerLease } from './runner-session-fixtures.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; @@ -257,8 +257,7 @@ function makeRunnerSession( testPromise, child: { pid: 42, exitCode: null }, state: 'ready', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), ...overrides, }; } diff --git a/packages/platform-apple/src/runner/__tests__/runner-early-exit-diagnosis.test.ts b/packages/platform-apple/src/runner/__tests__/runner-early-exit-diagnosis.test.ts index 298473855b..23af649d68 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-early-exit-diagnosis.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-early-exit-diagnosis.test.ts @@ -7,7 +7,7 @@ import { resetAllProcessMemosForTests } from '@agent-device/kernel/ttl-memo'; import type { ExecBackgroundResult } from '@agent-device/host-kit/command'; import { buildRunnerEarlyExitError } from '../runner-startup-transport.ts'; import { readRunnerLogTail } from '../runner-io.ts'; -import type { RunnerSession } from '../runner-session-types.ts'; +import { RunnerCommandAccounting, type RunnerSession } from '../runner-session-types.ts'; import { mkdtempForTestSync } from './tmp-dir.ts'; import { STUBBED_APPLE_TOOLCHAIN, stubAppleToolchainProbes } from './apple-toolchain-fixtures.ts'; import { @@ -52,8 +52,7 @@ function sessionFailingWith( child: { pid: 4242, exitCode: 1 } as ExecBackgroundResult['child'], state: 'starting', startupDeviceStates, - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), }; } diff --git a/packages/platform-apple/src/runner/__tests__/runner-readiness-routing.test.ts b/packages/platform-apple/src/runner/__tests__/runner-readiness-routing.test.ts new file mode 100644 index 0000000000..2522d30473 --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-readiness-routing.test.ts @@ -0,0 +1,43 @@ +import { test } from 'vitest'; +import type { RunnerCommand } from '../runner-contract.ts'; +import { isRunnerReadinessProbeCommand, RUNNER_COMMAND_TRAITS } from '../runner-command-traits.ts'; +import { readSwiftInlineCommands } from './runner-swift-settlement-fixtures.ts'; + +/** + * The runner serves `status` and `uptime` inline — outside its journal and off the serial command + * queue — which is why the daemon charges no exchange for them: an inline reply proves the runner is + * reachable and nothing about queued work, so a charge for one could only ever be paid by the wrong + * exchange (#2965). The `readinessProbe` trait is how the daemon names that set, and this tie is what + * keeps the two sides one claim: add an inline arm in Swift that the trait missed and that reply would + * discharge an unrelated command's charge; drop one and a genuinely queued command would go uncharged, + * so a shutdown could hand off a runner with work on its queue. + * + * The Swift half is read from the runner's own switch rather than restated here, so the check cannot + * agree with a stale copy of the answer. + */ + +function readinessProbeCommands(): string[] { + return (Object.keys(RUNNER_COMMAND_TRAITS) as RunnerCommand['command'][]) + .filter((command) => isRunnerReadinessProbeCommand({ command })) + .sort(); +} + +test('the readinessProbe trait names exactly the commands the runner serves inline', () => { + const inline = readSwiftInlineCommands(); + + if (inline.length === 0) { + throw new Error( + 'the runner serves no command inline, so the daemon must stop routing any reply as an inline answer', + ); + } + for (const command of inline) { + if (!readinessProbeCommands().includes(command)) { + throw new Error(`the runner answers "${command}" inline but the trait does not name it`); + } + } + for (const command of readinessProbeCommands()) { + if (!inline.includes(command)) { + throw new Error(`the trait calls "${command}" a probe but the runner queues it`); + } + } +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts b/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts index ca7ce87fa5..5e1c7a4294 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-recovery-wiring.test.ts @@ -15,7 +15,10 @@ import { requireRunnerPhaseRemainingMs, resolveExpectedRunnerCacheMetadata, } from '../runner-cache-metadata.ts'; +import { captureDiagnostics } from './runner-session-fixtures.ts'; import { startFakeRunnerServer, type FakeRunnerServer } from './fake-runner-server.ts'; +import { resolveRunnerDetachDecision, RunnerCommandAccounting } from '../runner-session-types.ts'; +import { requireLifecycleSettlementRows } from './runner-swift-settlement-fixtures.ts'; /** * The wiring regression the recovery suite cannot provide (#1644 review P1): @@ -104,8 +107,7 @@ function makeRunnerSession(port: number, sessionId = `wiring:${port}`): RunnerSe testPromise: new Promise(() => {}), child: { pid: process.pid, exitCode: null }, state: 'ready', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), }; return session; } @@ -158,6 +160,186 @@ test.each(Object.values(LOST_RESPONSE_MUTATION_ROWS))( }, ); +// #2965: an inline `status` probe answers while the command it probes may still be executing, so its +// own reply must not clear the mutation's outstanding charge. The handoff verdict is asserted through +// `resolveRunnerDetachDecision` — the exact gate `detachRunnerSessionForShutdown` consults. Rows come +// from the runner journal's own state list, so a state the runner gains is a missing row rather than an +// unruled verdict. +const LOST_RESPONSE_HANDOFF_ROWS = requireLifecycleSettlementRows({ + completed: true, + failed: true, + accepted: false, + started: false, + notAccepted: false, +}); + +test.each(LOST_RESPONSE_HANDOFF_ROWS)( + 'a lost mutation and status $lifecycleState leaves handoff $settlesCharge', + async ({ lifecycleState, settlesCharge }) => { + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [{ kind: 'ok', data: { lifecycleState } }], + }); + const session = seedSession(server.port); + + // The mutation is refused, never replayed, whatever the terminal verdict — recovery keeps the + // session and reports the command's state to the caller. + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + assert.equal( + server.requests.filter((request) => request.command === 'tap').length, + 1, + 'the mutation is sent once', + ); + // `notAccepted` is the journal's "never seen this id", which this daemon cannot read as a safe + // terminal state: it keeps the invalidation path and its charge, while every state the journal + // actually reports for the command keeps the session. + assert.equal( + invalidateRunnerSessionMock.mock.calls.length, + lifecycleState === 'notAccepted' ? 1 : 0, + `${lifecycleState} must ${lifecycleState === 'notAccepted' ? '' : 'not '}invalidate the session`, + ); + assert.equal(resolveRunnerDetachDecision(session).detach, settlesCharge); + }, +); + +test('a lost mutation answered by an inline status probe keeps handoff refused', async () => { + // `started` with the runner reporting itself not busy is the legitimate state this issue is about: + // the probe is served off the XCTest channel while the abandoned mutation keeps running. + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [{ kind: 'ok', data: { lifecycleState: 'started', runnerMainThreadBusy: false } }], + }); + const session = seedSession(server.port); + + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + assert.equal(session.runnerMainThreadBusy, false); + assert.deepEqual(resolveRunnerDetachDecision(session), { + detach: false, + reason: 'command_in_flight', + }); +}); + +test('an answered uptime health probe keeps handoff refused over an abandoned mutation', async () => { + // The other inline probe, and the realistic #2965 producer: `prepareIosRunner` and prewarm send + // `uptime` as background health traffic while a mutation is still owed. `status` only ever comes + // from recovery, so pinning it alone would leave `uptime` free to start carrying a charge again — + // whose answer would then forgive the mutation's residue on the serial-queue premise and hand a + // runner off mid-mutation. + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [{ kind: 'ok', data: { lifecycleState: 'started' } }], + uptime: [ + { kind: 'ok', data: { uptime: 12 } }, + { kind: 'ok', data: { uptime: 13 } }, + ], + }); + const session = seedSession(server.port); + + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + // One charge: the mutation. The readiness preflight's probe is inline and carries no charge, so it + // cannot arrive here as a second debt that some later answer could pay off (#2965). + assert.equal(session.commandCharges.outstandingChargeCount, 1); + + await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'uptime' }); + assert.equal( + server.requests.filter((request) => request.command === 'uptime').length > 1, + true, + 'this command is a second probe, answered by the same session', + ); + // The probe answered and owed nothing, so the mutation is still the only thing owed and the runner + // stays on the kill path rather than being handed off mid-mutation. + assert.equal(session.commandCharges.outstandingChargeCount, 1); + assert.equal(session.commandCharges.hasAbandonedCharges, true); + assert.deepEqual(resolveRunnerDetachDecision(session), { + detach: false, + reason: 'command_in_flight', + }); +}); + +test('terminal status for a lost mutation charges the next lost mutation afresh', async () => { + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }, { kind: 'hangUp' }], + status: [ + { kind: 'ok', data: { lifecycleState: 'completed' } }, + { kind: 'ok', data: { lifecycleState: 'started' } }, + ], + }); + const session = seedSession(server.port); + + const first = await captureDiagnostics(async () => { + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + }); + // The daemon log has to say whether terminal evidence actually paid the debt, because a refused + // settlement and a settled one both leave the runner answering (#2965). + assert.match(first, /"abandonedChargeSettled":true/); + assert.equal(resolveRunnerDetachDecision(session).detach, true); + + // The settlement is this command's, not a latch: a second mutation that loses its response is + // charged again and only its own terminal verdict frees the runner. + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + assert.equal(resolveRunnerDetachDecision(session).detach, false); +}); + +test('terminal status with the runner reporting busy still refuses handoff', async () => { + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [{ kind: 'ok', data: { lifecycleState: 'completed', runnerMainThreadBusy: true } }], + }); + const session = seedSession(server.port); + + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + + // The journal verdict discharged the charge, yet the runner's own occupancy report keeps it on the + // kill path: a terminal state never bypasses the busy gate (#2552). + assert.equal(session.runnerMainThreadBusy, true); + assert.deepEqual(resolveRunnerDetachDecision(session), { + detach: false, + reason: 'main_thread_occupied', + }); +}); + +test('a lost mutation whose status probe fails keeps the runner on the kill path', async () => { + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [{ kind: 'hangUp' }], + }); + const session = seedSession(server.port); + + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + assert.equal(resolveRunnerDetachDecision(session).detach, false); +}); + +test('a healthy command after a settled abandoned charge returns the runner to handoff-eligible', async () => { + server = await startFakeRunnerServer({ + tap: [{ kind: 'hangUp' }], + status: [{ kind: 'ok', data: { lifecycleState: 'completed' } }], + snapshot: [{ kind: 'ok', data: { nodes: [] } }], + }); + const session = seedSession(server.port); + + await expect( + runAppleRunnerCommand(IOS_SIMULATOR, { command: 'tap', x: 5, y: 5 }), + ).rejects.toThrow(AppError); + assert.equal(resolveRunnerDetachDecision(session).detach, true); + + await runAppleRunnerCommand(IOS_SIMULATOR, { command: 'snapshot' }); + assert.equal(resolveRunnerDetachDecision(session).detach, true); +}); + test('a runner that reports the command failed surfaces that failure, not the transport error', async () => { server = await startFakeRunnerServer({ tap: [{ kind: 'hangUp' }], diff --git a/packages/platform-apple/src/runner/__tests__/runner-response.test.ts b/packages/platform-apple/src/runner/__tests__/runner-response.test.ts index 59b5c25276..89300e2bfc 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-response.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-response.test.ts @@ -7,6 +7,7 @@ import { isRunnerResponseOk, readRunnerResponseData, } from '../runner-contract.ts'; +import { isStructuredRunnerFailure } from '../runner-error-classification.ts'; import { parseRunnerResponse } from '../runner-session.ts'; import type { RunnerSessionState } from '../runner-session-types.ts'; @@ -34,16 +35,24 @@ describe('decodeRunnerResponseBody', () => { ); }); - test('never reads a JSON body that is not an envelope as an answer', () => { + test('refuses a JSON body that is not an envelope instead of inventing one', () => { + // A JSON scalar is not something the runner's encoder emits. Reading it as an empty reply would + // count as an answer and discharge a command the runner may still be executing. for (const body of ['null', '42', '"ok"']) { - assert.equal(isRunnerResponseOk(decodeRunnerResponseBody(body)), false, body); + assert.throws( + () => decodeRunnerResponseBody(body), + (error: unknown) => + error instanceof AppError && error.message === 'Invalid runner response', + body, + ); } }); - test('carries a bare-array body through unread rather than inventing an envelope', () => { - const payload = decodeRunnerResponseBody('[]'); - assert.equal(isRunnerResponseOk(payload), false); - assert.deepEqual(readRunnerResponseData(payload), {}); + test('refuses a bare-array body for the same reason', () => { + assert.throws( + () => decodeRunnerResponseBody('[]'), + (error: unknown) => error instanceof AppError && error.message === 'Invalid runner response', + ); }); }); @@ -137,4 +146,24 @@ describe('parseRunnerResponse', () => { assert.equal(session.state, 'starting'); }); + + test('reads a JSON scalar body as transport-shaped, not as an empty reply', async () => { + // The chain #2965 turns on: an error carrying a `runner` detail counts as an answer and discharges + // the command's charge. A bare `null` is not an envelope the runner's encoder emits, so decoding it + // into `{}` and erroring from that would have answered a command the runner may still be running. + const session: { state: RunnerSessionState } = { state: 'starting' }; + + await assert.rejects( + () => parseRunnerResponse(new Response('null'), session), + (error: unknown) => { + assert.ok(error instanceof AppError); + assert.equal(error.message, 'Invalid runner response'); + assert.equal(error.details?.runner, undefined); + assert.equal(isStructuredRunnerFailure(error), false); + return true; + }, + ); + + assert.equal(session.state, 'starting'); + }); }); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts b/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts index bbe2a4c1ab..d569b32090 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-fixtures.ts @@ -5,7 +5,7 @@ import { AppError } from '@agent-device/kernel/errors'; import { IOS_SIMULATOR } from './device-fixtures.ts'; import { appleRunnerTestHost } from '../test-host.ts'; import { runnerOwnerStartTime, type RunnerLease } from '../runner-lease.ts'; -import type { RunnerSession } from '../runner-session-types.ts'; +import { RunnerCommandAccounting, type RunnerSession } from '../runner-session-types.ts'; import { runnerConnectFailureDetails, type RunnerConnectFailureReason, @@ -28,8 +28,7 @@ export function makeRunnerSession(overrides: Partial = {}): Runne testPromise: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }), child: { pid: 1234, exitCode: null }, state: 'ready', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), ...overrides, } as RunnerSession; } diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-lifecycle.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session-lifecycle.test.ts index 2bba59df4b..681b2486d7 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-lifecycle.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-lifecycle.test.ts @@ -480,16 +480,20 @@ test('a session that still owes a response is not handed off', async () => { // The charge lands behind the preflight and deadline awaits, so wait for it instead of betting on a // single macrotask: a loaded runner can sit anywhere on that path, and a session that had not been // charged yet would look identical to one whose charge was wrongly dropped (#2681). - for (let tick = 0; tick < 500 && session.inFlightCommands === 0; tick += 1) { + for (let tick = 0; tick < 500 && session.commandCharges.outstandingChargeCount === 0; tick += 1) { await new Promise((resolve) => setTimeout(resolve, 1)); } - assert.equal(session.inFlightCommands, 1); + assert.equal(session.commandCharges.outstandingChargeCount, 1); const diagnostics = await captureDiagnostics(async () => { assert.equal(await detachIosRunnerSessionsForShutdown(), 0); }); assert.match(diagnostics, /"reason":"command_in_flight"/); + // The awaited exchange is charged but not abandoned: what waits here is its own answer, not terminal + // evidence for a residue (#2965). + assert.match(diagnostics, /"outstandingCharges":1/); + assert.match(diagnostics, /"hasAbandonedCharges":false/); assert.ok(readRunnerSessionLiveness(device.id)); }); @@ -516,18 +520,23 @@ test('a command abandoned by a cancelled transport keeps the runner occupied', a controller.signal, ), ); - assert.equal(session.hasAbandonedCommands, true); + assert.equal(session.commandCharges.hasAbandonedCharges, true); const refused = await captureDiagnostics(async () => { assert.equal(await detachIosRunnerSessionsForShutdown(), 0); }); assert.match(refused, /"reason":"command_in_flight"/); - - // Any answered exchange forgives the abandoned charge: the runner is serving again, and stamps - // whatever is still draining onto that very reply. + // The refusal this issue is about: nothing is awaited any more, and only terminal evidence for that + // command's `commandId` — never a later reply — may discharge it (#2965). + assert.match(refused, /"outstandingCharges":1/); + assert.match(refused, /"hasAbandonedCharges":true/); + + // An answered queued exchange forgives the abandoned charge: the serial queue makes it evidence that + // the abandoned handling ahead of it finished, and whatever is still draining is stamped onto that + // very reply. An answer the runner serves inline forgives nothing (#2965); that case is pinned in + // `runner-recovery-wiring.test.ts`, where the probe reaches a real session. await serveOneCommand(device, session); - assert.equal(session.inFlightCommands, 0); - assert.equal(session.hasAbandonedCommands, false); + assert.equal(session.commandCharges.hasOutstandingCharges, false); assert.equal(await detachIosRunnerSessionsForShutdown(), 1); }); diff --git a/packages/platform-apple/src/runner/__tests__/runner-session-types.test.ts b/packages/platform-apple/src/runner/__tests__/runner-session-types.test.ts index 182a58c11b..886db704f3 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-session-types.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-session-types.test.ts @@ -3,6 +3,7 @@ import { advanceRunnerSessionState, canWorkWithRunnerSession, resolveRunnerSessionLiveness, + RunnerCommandAccounting, type RunnerSessionState, } from '../runner-session-types.ts'; @@ -118,3 +119,197 @@ describe('resolveRunnerSessionLiveness', () => { } }); }); + +// The charge ledger decides whether a runner may be handed to the next daemon, so each row records +// what is still owed after one settlement step (#2681, #2965). + +describe('RunnerCommandAccounting charging', () => { + test('an unanswered charge refuses handoff', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + + expect(charges.hasOutstandingCharges).toBe(true); + expect(charges.hasAbandonedCharges).toBe(false); + }); + + test('a charge with no id stays owed forever', () => { + // Nothing can name it later: the runner journals only commands that carried an id, so an + // exchange sent without one can never be proven landed. Unreachable on the command path, where + // every queued command is given an id; kept as the conservative outcome rather than a guess. + const charges = new RunnerCommandAccounting(); + charges.charge(undefined); + charges.settleAnswered(undefined); + charges.markAbandoned(undefined); + + expect(charges.hasOutstandingCharges).toBe(true); + expect(charges.settleTerminalEvidence(undefined)).toBe(false); + }); +}); + +describe('RunnerCommandAccounting answering', () => { + test('settles its own charge and leaves a clean runner eligible', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.settleAnswered('cmd-a'); + + expect(charges.hasOutstandingCharges).toBe(false); + }); + + test('forgives one abandoned charge sent before it', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.charge('cmd-b'); + charges.settleAnswered('cmd-b'); + + // The serial queue makes B's answer evidence that the queued handling ahead of it finished. + expect(charges.hasOutstandingCharges).toBe(false); + }); + + test('does not forgive an abandoned charge sent after it', () => { + // A's answer says nothing about work queued behind it, so B keeps the runner on the kill path + // even though both charges are for the same session. + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.charge('cmd-b'); + charges.markAbandoned('cmd-b'); + charges.settleAnswered('cmd-a'); + + expect(charges.outstandingChargeCount).toBe(1); + expect(charges.hasAbandonedCharges).toBe(true); + }); + + test('leaves one residue per extra abandoned exchange', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.charge('cmd-b'); + charges.markAbandoned('cmd-b'); + charges.charge('cmd-c'); + charges.settleAnswered('cmd-c'); + + expect(charges.outstandingChargeCount).toBe(1); + expect(charges.hasAbandonedCharges).toBe(true); + }); + + test("treats a late answer on an abandoned charge as that exchange's own proof", () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.charge('cmd-b'); + charges.markAbandoned('cmd-b'); + charges.settleAnswered('cmd-a'); + + expect(charges.outstandingChargeCount).toBe(1); + expect(charges.hasAbandonedCharges).toBe(true); + }); + + test('settles nothing when the id names no charge', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.settleAnswered('cmd-unknown'); + + expect(charges.outstandingChargeCount).toBe(1); + }); +}); + +describe('RunnerCommandAccounting abandonment', () => { + test('marks the named charge and keeps it owed', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + + expect(charges.hasOutstandingCharges).toBe(true); + expect(charges.hasAbandonedCharges).toBe(true); + }); + + test('marks no other command when the id holds no charge', () => { + // A send that failed before the request carried an id reports a failure for nothing. Marking the + // oldest charge abandoned instead would hand terminal evidence for that other command a debt to + // discharge and put a runner whose queue is empty back on the happy path. + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-gone'); + + expect(charges.hasAbandonedCharges).toBe(false); + expect(charges.settleTerminalEvidence('cmd-a')).toBe(false); + expect(charges.hasOutstandingCharges).toBe(true); + }); +}); + +describe('RunnerCommandAccounting terminal evidence', () => { + test('discharges the abandoned charge it names', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + + expect(charges.settleTerminalEvidence('cmd-a')).toBe(true); + expect(charges.hasOutstandingCharges).toBe(false); + }); + + test('discharges nothing a second time', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.settleTerminalEvidence('cmd-a'); + + expect(charges.settleTerminalEvidence('cmd-a')).toBe(false); + }); + + test("cannot consume another command's charge", () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.charge('cmd-b'); + charges.markAbandoned('cmd-b'); + + charges.settleTerminalEvidence('cmd-a'); + + expect(charges.outstandingChargeCount).toBe(1); + expect(charges.hasAbandonedCharges).toBe(true); + }); + + test('leaves an exchange still being awaited charged', () => { + // A read-only resend re-sends the same id, so both attempts share one logical command. Terminal + // evidence discharges the abandoned attempt; the live wait keeps its own charge. + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.charge('cmd-a'); + + expect(charges.settleTerminalEvidence('cmd-a')).toBe(true); + expect(charges.outstandingChargeCount).toBe(1); + expect(charges.hasAbandonedCharges).toBe(false); + + charges.settleAnswered('cmd-a'); + expect(charges.hasOutstandingCharges).toBe(false); + }); + + test('settles every abandoned attempt of a coalesced execution', () => { + // The runner journals one execution per id, so one verdict proves every send waiting on it. + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + + expect(charges.settleTerminalEvidence('cmd-a')).toBe(true); + expect(charges.hasOutstandingCharges).toBe(false); + }); + + test('names the surviving charge when a queued answer forgave one of two', () => { + const charges = new RunnerCommandAccounting(); + charges.charge('cmd-a'); + charges.markAbandoned('cmd-a'); + charges.charge('cmd-b'); + charges.markAbandoned('cmd-b'); + charges.charge('cmd-c'); + charges.settleAnswered('cmd-c'); + + // C's answer forgave the oldest residue, so B survives and only B's own verdict discharges it. + expect(charges.hasAbandonedCharges).toBe(true); + expect(charges.settleTerminalEvidence('cmd-a')).toBe(false); + expect(charges.settleTerminalEvidence('cmd-b')).toBe(true); + expect(charges.hasOutstandingCharges).toBe(false); + }); +}); diff --git a/packages/platform-apple/src/runner/__tests__/runner-startup-transport.test.ts b/packages/platform-apple/src/runner/__tests__/runner-startup-transport.test.ts index a335e95414..319981ba76 100644 --- a/packages/platform-apple/src/runner/__tests__/runner-startup-transport.test.ts +++ b/packages/platform-apple/src/runner/__tests__/runner-startup-transport.test.ts @@ -3,7 +3,7 @@ import assert from 'node:assert/strict'; import type { ExecBackgroundResult } from '@agent-device/host-kit/command'; import { appleRunnerTestHost } from '../test-host.ts'; import { AppError } from '@agent-device/kernel/errors'; -import type { RunnerSession } from '../runner-session-types.ts'; +import { RunnerCommandAccounting, type RunnerSession } from '../runner-session-types.ts'; import { iosDevice, iosSimulator, @@ -251,8 +251,7 @@ test('waitForRunner preserves xcodebuild diagnostics when the runner exits durin testPromise: Promise.resolve({ exitCode: 65, stdout: '', stderr: '' }), child: { pid: 1234, exitCode: null } as ExecBackgroundResult['child'], state: 'starting', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), }; mockUsbmuxPostCommand.mockImplementation(async () => { (session.child as { exitCode: number | null }).exitCode = 65; @@ -294,8 +293,7 @@ test('waitForRunner carries the disk-image state when the runner is still alive testPromise: new Promise(() => {}), child: { pid: 1234, exitCode: null } as ExecBackgroundResult['child'], state: 'starting', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), startupDeviceStates: { developerMode: 'enabled', developerDiskImage: 'unavailable', @@ -379,8 +377,7 @@ function makeReadyRunnerSession(): RunnerSession { testPromise: Promise.resolve({ exitCode: 0, stdout: '', stderr: '' }), child: { pid: 1234, exitCode: null } as ExecBackgroundResult['child'], state: 'ready', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), }; } diff --git a/packages/platform-apple/src/runner/__tests__/runner-swift-settlement-fixtures.ts b/packages/platform-apple/src/runner/__tests__/runner-swift-settlement-fixtures.ts new file mode 100644 index 0000000000..41e75cb065 --- /dev/null +++ b/packages/platform-apple/src/runner/__tests__/runner-swift-settlement-fixtures.ts @@ -0,0 +1,127 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** + * Reads the two Swift declarations that decide how a command charge settles (#2965), so the daemon's + * rules and the runner's cannot drift apart silently. These are source inspections on purpose: the + * Swift answers they name run on a device lane, so the tie that keeps one claim has to read the + * declaration rather than a second copy of its answer. + */ + +const here = path.dirname(fileURLToPath(import.meta.url)); +const SWIFT_TRANSPORT = path.resolve( + here, + '../../../../../apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+Transport.swift', +); +const SWIFT_JOURNAL = path.resolve( + here, + '../../../../../apple/runner/AgentDeviceRunner/AgentDeviceRunnerUITests/RunnerTests+CommandJournal.swift', +); + +/** `inlineResponse(for:)`'s switch arms, up to the `default:` where queued commands begin. */ +const INLINE_SWITCH = + /func inlineResponse\(for command: Command\)[\s\S]*?switch command\.command \{([\s\S]*?)\n\s*default:/; +/** + * One unguarded `case .a, .b:` header, bindings continuing across lines if needed. Each repetition + * must carry its own comma and `.name`, so no two splits of the same text both match — an ambiguous + * separator like `\s*,?\s*` backtracks exponentially on a long arm list. Requiring the closing colon + * is what makes a guarded arm (`case .uptime where flag:`) fail the count below rather than read as + * inline: a guard can fall through to `default:`, so such a command really is queued. + */ +const CASE_HEADER = /\bcase\s+(\.\w+(?:\s*,\s*\.\w+)*):/g; + +/** + * The commands `inlineResponse(for:)` answers without journaling them or putting them on the serial + * command queue, which is exactly why a reply to one is no evidence about queued work. + * + * A reader here can only fail in two directions, and each is caught: a misread that names an extra + * command fails `runner-readiness-routing.test.ts`, which compares this list against the daemon's + * `readinessProbe` trait in both directions, and a misread that loses an arm makes a real probe look + * queued, which that same tie reports. What is left to guard locally is the case both miss — a `case` + * these two patterns do not recognize, which would otherwise drop an arm and leave a set that still + * happens to match the trait. That is why a header must end at its colon: a guarded arm + * (`case .uptime where flag:`) can fall through to `default:`, so it names a command that is really + * queued some of the time, and it has to fail here rather than read as inline. + */ +export function readSwiftInlineCommands(): string[] { + const swift = fs.readFileSync(SWIFT_TRANSPORT, 'utf8'); + const body = INLINE_SWITCH.exec(swift)?.[1]; + assert.ok( + body !== undefined, + 'inlineResponse(for:) must keep a `switch command.command` with a default: arm, which is where ' + + 'queued commands begin. Without that shape there is no inline set to read.', + ); + + const headers = [...body.matchAll(CASE_HEADER)]; + // Every `case` keyword in the switch must belong to a header this reader parsed. An enum-qualified + // or guarded arm matches neither, and reading the switch without it would silently understate the + // inline set. + const caseKeywords = body.match(/\bcase\b/g) ?? []; + assert.equal( + headers.length, + caseKeywords.length, + 'inlineResponse(for:) has a `case` form this reader does not recognize (an enum-qualified or ' + + 'guarded arm?). Name it here before it decides a handoff.', + ); + const arms: string[] = []; + for (const [, header] of headers) { + for (const [, name] of (header ?? '').matchAll(/\.(\w+)/g)) arms.push(name!); + } + assert.ok(arms.length > 0, 'inlineResponse(for:) declares no inline command arms'); + return arms; +} + +/** + * The states `RunnerCommandLifecycleState` declares. `completed` and `failed` are written when the + * journal closes a command — by its response's `ok` in `finish`, or by a thrown error in `fail` — while + * `accepted` and `started` are written as execution opens. `notAccepted` is never written to an entry: + * `status` synthesizes it for an id the journal does not hold. + */ +function readSwiftLifecycleStates(): string[] { + const swift = fs.readFileSync(SWIFT_JOURNAL, 'utf8'); + const declaration = swift.match(/enum RunnerCommandLifecycleState: String \{([\s\S]*?)\n\}/); + const states = declaration?.[1]; + assert.ok( + states, + 'RunnerTests+CommandJournal.swift must declare RunnerCommandLifecycleState with a body', + ); + return [...states.matchAll(/^\s*case\s+(\w+)/gm)].map(([, name]) => name as string); +} + +/** One row of the settlement table the daemon must honor for a journal state. */ +export type RunnerLifecycleSettlementRow = Readonly<{ + lifecycleState: string; + /** Whether a terminal verdict for the command may discharge its abandoned charge. */ + settlesCharge: boolean; +}>; + +/** + * Every state the journal declares, each with the settlement the daemon owes it. A state the runner + * gains without a row here fails the caller, because an unread state would otherwise decide a runner's + * handoff by accident. + */ +export function requireLifecycleSettlementRows( + expectations: Readonly>, +): RunnerLifecycleSettlementRow[] { + const states = readSwiftLifecycleStates(); + const unruled = states.filter((state) => expectations[state] === undefined); + assert.deepEqual( + unruled, + [], + `the runner journal declares lifecycle state(s) ${unruled.join(', ')} that the daemon's ` + + 'settlement table has no row for. Name the verdict the charge accounting must reach for it.', + ); + const unobserved = Object.keys(expectations).filter((state) => !states.includes(state)); + assert.deepEqual( + unobserved, + [], + `the daemon's settlement table rules on state(s) ${unobserved.join(', ')} the runner journal no ` + + 'longer reports. Remove the row or restore the state.', + ); + return states.map((lifecycleState) => ({ + lifecycleState, + settlesCharge: expectations[lifecycleState]!, + })); +} diff --git a/packages/platform-apple/src/runner/runner-adoption.ts b/packages/platform-apple/src/runner/runner-adoption.ts index 763549dabd..16e4e0f008 100644 --- a/packages/platform-apple/src/runner/runner-adoption.ts +++ b/packages/platform-apple/src/runner/runner-adoption.ts @@ -33,6 +33,7 @@ import { } from './runner-xctestrun.ts'; import { normalizeRunnerStartupTimeoutMs, + RunnerCommandAccounting, type RunnerProcessHandle, type RunnerSession, } from './runner-session-types.ts'; @@ -304,8 +305,7 @@ function buildAdoptedRunnerSession( runnerLogPath: lease.runnerLogPath, // The probe already proved the runner answers commands. state: 'ready', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), startupTimeoutMs: normalizeRunnerStartupTimeoutMs( requireRunnerPhaseRemainingMs(options.budget, 'runner_session_adoption'), ), diff --git a/packages/platform-apple/src/runner/runner-command-recovery.ts b/packages/platform-apple/src/runner/runner-command-recovery.ts index 6345e0fd68..a53311e9b5 100644 --- a/packages/platform-apple/src/runner/runner-command-recovery.ts +++ b/packages/platform-apple/src/runner/runner-command-recovery.ts @@ -160,6 +160,10 @@ async function tryRecoverRunnerCommandAfterTransportError( } const lifecycleState = typeof status.lifecycleState === 'string' ? status.lifecycleState : ''; + // Terminal evidence says the runner finished *this* command, so that command's abandoned charge pays + // off and no other's does. `accepted`, `started`, and a state this daemon cannot name all keep the + // charge: the command may still be executing, and a runner kept on the kill path is the safe answer. + const charge = settleRunnerChargeForTerminalStatus(session, command, lifecycleState); emitDiagnostic({ level: 'debug', phase: 'ios_runner_command_status_recovery', @@ -168,6 +172,9 @@ async function tryRecoverRunnerCommandAfterTransportError( commandId: command.commandId, lifecycleState, ...readinessPreflight, + // Whether this terminal evidence discharged a debt. Absent when the state is not terminal, so + // nothing was attempted; `false` when the command held no abandoned charge to pay. + ...(charge === undefined ? {} : { abandonedChargeSettled: charge }), }, }); return handleRunnerCommandStatusRecovery( @@ -179,6 +186,38 @@ async function tryRecoverRunnerCommandAfterTransportError( ); } +/** + * The runner journal vocabulary, so the charge settlement below and the recovery verdict in + * {@link handleRunnerCommandStatusRecovery} cannot disagree about what a state means. + * `runner-swift-settlement-fixtures.ts` pins these names to `RunnerCommandLifecycleState`, and the + * recovery wiring rows are derived from that same declaration, so a state the runner gains has to be + * ruled here before it can decide a handoff. + * + * `completed` and `failed` close an entry — from the response's `ok` in `finish`, or a thrown error in + * `fail` — so execution ended, and each gets its own recovery verdict below. `accepted` and `started` + * are written as execution opens, so they share one in-flight verdict. `notAccepted` is what `status` + * reports for an id the journal never held, which this daemon cannot read as terminal. + */ +const RUNNER_TERMINAL_LIFECYCLE_STATES: ReadonlySet = new Set(['completed', 'failed']); +const RUNNER_IN_FLIGHT_LIFECYCLE_STATES: ReadonlySet = new Set(['accepted', 'started']); + +/** + * Discharges the abandoned charge terminal status proves landed (#2965). A status reply is served + * inline, so it is no evidence that queued work finished; this is the only place a status answer may + * settle a charge, and only the one its `statusCommandId` names. + * + * @returns whether the evidence paid a debt, or `undefined` when the state is not terminal and no + * settlement was attempted. + */ +function settleRunnerChargeForTerminalStatus( + session: RunnerSession, + command: RunnerCommand, + lifecycleState: string, +): boolean | undefined { + if (!RUNNER_TERMINAL_LIFECYCLE_STATES.has(lifecycleState)) return undefined; + return session.commandCharges.settleTerminalEvidence(command.commandId); +} + function handleRunnerCommandStatusRecovery( status: Record, lifecycleState: string, @@ -199,7 +238,7 @@ function handleRunnerCommandStatusRecovery( }; } - if (lifecycleState === 'accepted' || lifecycleState === 'started') { + if (RUNNER_IN_FLIGHT_LIFECYCLE_STATES.has(lifecycleState)) { return { type: 'skipInvalidation', reason: 'command_still_in_flight', diff --git a/packages/platform-apple/src/runner/runner-contract.ts b/packages/platform-apple/src/runner/runner-contract.ts index 86611eca91..27522d3c1b 100644 --- a/packages/platform-apple/src/runner/runner-contract.ts +++ b/packages/platform-apple/src/runner/runner-contract.ts @@ -37,7 +37,7 @@ export const MAIN_THREAD_TIMEOUT_RUNNER_CODE = 'MAIN_THREAD_TIMEOUT'; * starting when the next read arrives, so it is retriable for a `wait`, while the transport reads * it as a definite answer and never resends it. */ -export const APP_NOT_RUNNING_RUNNER_CODE = 'APP_NOT_RUNNING'; +const APP_NOT_RUNNING_RUNNER_CODE = 'APP_NOT_RUNNING'; export type RunnerCommand = { command: @@ -238,9 +238,12 @@ export type RunnerResponsePayload = { * The one decoding of a runner response body (#2662). The envelope arrives at three readers — a * command's own response, the lifecycle journal a status probe reads back after the transport * response was lost, and the adoption `uptime` probe — and all three must agree on what is - * readable, or a body one of them refuses becomes an answer for another. A body that is not JSON - * at all is transport-shaped failure: a runner that died mid-write must not be read as having - * answered. + * readable, or a body one of them refuses becomes an answer for another. + * + * Only a JSON object can be an envelope. A body that is not JSON at all, and a JSON scalar or array + * that carries no `ok`, are both transport-shaped: neither is something the runner's encoder emits, + * and reading either as an empty reply would let a proxy page or a half-written body answer for a + * command the runner may still be executing. */ export function decodeRunnerResponseBody(text: string): RunnerResponsePayload { let parsed: unknown; @@ -249,7 +252,14 @@ export function decodeRunnerResponseBody(text: string): RunnerResponsePayload { } catch { throw new AppError('COMMAND_FAILED', 'Invalid runner response', { text }); } - return parsed && typeof parsed === 'object' ? (parsed as RunnerResponsePayload) : {}; + if (!isRunnerEnvelopeObject(parsed)) { + throw new AppError('COMMAND_FAILED', 'Invalid runner response', { text }); + } + return parsed; +} + +function isRunnerEnvelopeObject(parsed: unknown): parsed is RunnerResponsePayload { + return typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed); } /** The runner's `ok` is a Swift `Bool`, so only the literal `true` is an answer. */ diff --git a/packages/platform-apple/src/runner/runner-session-types.ts b/packages/platform-apple/src/runner/runner-session-types.ts index 793a2f2656..a14ec2fd35 100644 --- a/packages/platform-apple/src/runner/runner-session-types.ts +++ b/packages/platform-apple/src/runner/runner-session-types.ts @@ -82,19 +82,11 @@ export type RunnerSession = { startupRetryWake?: AbortSignal; startupTimeoutMs?: number; /** - * Commands the runner accepted that this process has not seen answered. It comes down only when a - * response is decoded: an aborted or dropped exchange leaves the command running on the runner, - * and nothing in this process learns when that ends, so the count is what keeps such a runner off - * the handoff path (#2681). + * The exchanges this process sent this runner and has not seen answered, and which of them it gave + * up on. The session owns the one instance; command paths settle it and the handoff verdict reads + * it (#2681). Lives only on the session so it dies with invalidation and restart. */ - inFlightCommands: number; - /** - * Whether an exchange this process gave up on is among {@link inFlightCommands} — the sticky half - * of the occupancy report, since a cancellation or transport drop resets every live wait while the - * runner keeps working. An answered exchange forgives them (the runner is demonstrably serving - * again, and stamps any work still draining onto its reply) and clears this. - */ - hasAbandonedCommands: boolean; + commandCharges: RunnerCommandAccounting; // Records the last allowlisted mutating interaction that the runner confirmed // healthy (parsed ok, non-runnerFatal) for a given app bundle. Lives only on // the session object so it dies with every invalidation/restart (#702). @@ -185,6 +177,131 @@ export type RunnerDetachDecision = | { detach: true } | { detach: false; reason: RunnerDetachRefusal }; +/** One exchange the runner took and this process has not seen answered. */ +type RunnerCommandCharge = { + /** The id this exchange put on the wire, and the only key its evidence arrives under. */ + readonly commandId: string; + /** Whether this process gave up on the exchange while the runner may still be executing it. */ + abandoned: boolean; +}; + +/** + * What a session owes its runner: one charge per queued command sent and not yet answered. + * + * Invariants: + * - A charge is taken when a request goes out and released only by that command's own answer or its + * own terminal journal evidence. An exchange abandoned to a cancellation or a dropped transport + * keeps its charge, because the runner is still executing it and nothing here learns when that ends. + * - Only queued exchanges are charged. The runner answers a readiness probe inline, off its journal and + * its serial command queue, so a probe says nothing about what the queue still holds and a probe + * reply must be able to settle nothing. (The readiness preflight's own probe has always worked this + * way.) A probe that needs an answer gets one by being answered, not by discharging someone else. + * - Charges are ordered by send. A queued answer forgives at most one abandoned charge that predates + * it, since the serial queue only proves the work ahead of the answer finished; an abandoned charge + * sent later stays. An answer that lands on a charge already marked abandoned is that exchange's own + * late reply, so it forgives nothing further. + * - Terminal evidence names one `commandId` and discharges only abandoned charges carrying it: the + * runner runs one execution per id, so every send waiting on that execution landed. A charge still + * awaited belongs to a live exchange and stays for its own answer. + * + * {@link resolveRunnerDetachDecision} is the only reader, so a handoff never reconstructs occupancy + * from elsewhere. The owning session holds the one instance; it dies with invalidation and restart. + */ +export class RunnerCommandAccounting { + private charges: RunnerCommandCharge[] = []; + + /** Whether anything is owed: an exchange still awaited, or one abandoned and not yet proven landed. */ + get hasOutstandingCharges(): boolean { + return this.charges.length > 0; + } + + /** How many exchanges are charged. Diagnostic detail only; the handoff verdict reads the flags. */ + get outstandingChargeCount(): number { + return this.charges.length; + } + + /** Whether any outstanding charge was abandoned rather than still awaited. */ + get hasAbandonedCharges(): boolean { + return this.charges.some((charge) => charge.abandoned); + } + + /** Charge one queued send. From here a shutdown that hands the runner off orphans real work. */ + charge(commandId: string | undefined): void { + this.charges.push({ commandId: normalizeRunnerChargeId(commandId), abandoned: false }); + } + + /** + * This process stopped waiting on an exchange without an answer, so the charge stays and is marked + * abandoned. A failure naming a command with no charge belongs to an exchange that already settled; + * marking some other command's debt could hand off a runner that still holds the command whose send + * actually failed, so it marks nothing. + */ + markAbandoned(commandId: string | undefined): void { + const index = this.findChargeIndex(commandId); + if (index !== -1) this.charges[index]!.abandoned = true; + } + + /** + * Discharge an answered exchange, then forgive one abandoned charge sent before it — the serial + * queue makes this answer evidence that the queued handling ahead of it finished, and any work still + * draining is stamped on the reply itself. A run of dropped exchanges therefore keeps one residue + * per extra exchange rather than being guessed drained. + */ + settleAnswered(commandId: string | undefined): void { + const answeredIndex = this.findChargeIndex(commandId); + if (answeredIndex === -1) return; + const answered = this.charges[answeredIndex]!; + this.charges.splice(answeredIndex, 1); + if (answered.abandoned) return; + // After the splice, the charges that were sent before the answered one occupy the indices below + // it, which is exactly the prefix this answer is proof about. + const residueIndex = this.charges.findIndex( + (charge, index) => index < answeredIndex && charge.abandoned, + ); + if (residueIndex !== -1) this.charges.splice(residueIndex, 1); + } + + /** + * Terminal journal evidence — a state proving execution ended — for one command. The runner runs one + * execution per `commandId`, so that command landed and every abandoned charge waiting on that + * execution is discharged. A repeat observation, an unknown command, a still-awaited exchange, and a + * blank id all settle nothing; the only reason is that this command holds no abandoned charge. + * + * @returns whether this evidence paid a debt, so the caller can report which it was. + */ + settleTerminalEvidence(commandId: string | undefined): boolean { + const wanted = normalizeRunnerChargeId(commandId); + // A blank id is evidence about nothing: the runner journals only commands that carried one. + if (wanted === '') return false; + const stillOwed = this.charges.filter( + (charge) => charge.commandId !== wanted || !charge.abandoned, + ); + if (stillOwed.length === this.charges.length) return false; + this.charges = stillOwed; + return true; + } + + /** + * The newest charge under this id. The runner coalesces a repeated `commandId` onto one execution, + * so a live and an abandoned charge sharing an id are the same logical command, and the newest + * exchange settles first. An id the runner never received — a `status` probe carries none — matches + * no queued charge, since a probe is not charged in the first place. + */ + private findChargeIndex(commandId: string | undefined): number { + const wanted = normalizeRunnerChargeId(commandId); + if (wanted === '') return -1; + for (let index = this.charges.length - 1; index >= 0; index -= 1) { + if (this.charges[index]!.commandId === wanted) return index; + } + return -1; + } +} + +/** Ids match exactly as sent. */ +function normalizeRunnerChargeId(commandId: string | undefined): string { + return commandId?.trim() ?? ''; +} + /** * Whether this session's runner may be handed to the next daemon by a graceful shutdown (#2681). * `ready` is the only state that proves the runner serves requests: physical startup runs tens of @@ -196,15 +313,12 @@ export type RunnerDetachDecision = * command the next daemon sends it. */ export function resolveRunnerDetachDecision( - session: Pick< - RunnerSession, - 'state' | 'runnerMainThreadBusy' | 'inFlightCommands' | 'hasAbandonedCommands' - >, + session: Pick, ): RunnerDetachDecision { if (session.state !== 'ready') { return { detach: false, reason: 'runner_never_served_a_command' }; } - if (session.inFlightCommands > 0 || session.hasAbandonedCommands) { + if (session.commandCharges.hasOutstandingCharges) { return { detach: false, reason: 'command_in_flight' }; } if (isRunnerMainThreadOccupied(session)) { diff --git a/packages/platform-apple/src/runner/runner-session.ts b/packages/platform-apple/src/runner/runner-session.ts index 576dadd7d0..1c6430f7ad 100644 --- a/packages/platform-apple/src/runner/runner-session.ts +++ b/packages/platform-apple/src/runner/runner-session.ts @@ -84,6 +84,7 @@ import { normalizeRunnerStartupTimeoutMs, resolveRunnerDetachDecision, resolveRunnerSessionLiveness, + RunnerCommandAccounting, type RunnerDetachRefusal, type RunnerSession, type RunnerSessionLiveness, @@ -339,8 +340,7 @@ async function startRunnerSessionWithLease( endOutputObservation: runnerProcess.endOutputObservation, readLogTail: runnerProcess.readLogTail, state: 'starting', - inFlightCommands: 0, - hasAbandonedCommands: false, + commandCharges: new RunnerCommandAccounting(), startupRetryWake: runnerProcess.startupRetryWake, startupTimeoutMs: normalizeRunnerStartupTimeoutMs(startupTimeoutMs), startupTimings, @@ -758,6 +758,11 @@ export async function detachIosRunnerSessionsForShutdown(): Promise { sessionId: session.sessionId, lane: outcome.lane, reason: outcome.reason, + // A refused handoff is read from the daemon log, and the two refusals that name a charge look + // identical without this: an exchange still awaited is recovered by its own answer, while an + // abandoned residue waits for terminal evidence for its `commandId` (#2965). + outstandingCharges: session.commandCharges.outstandingChargeCount, + hasAbandonedCharges: session.commandCharges.hasAbandonedCharges, }, }); continue; @@ -867,10 +872,12 @@ export function validateRunnerDevice(device: DeviceInfo): void { } /** - * Runs one command through a session. The command send charges the session and only a decoded - * response discharges it: an exchange this process abandoned to a cancellation or a dropped + * Runs one command through a session. The command send charges the session and only this command's + * own answer discharges it: an exchange this process abandoned to a cancellation or a dropped * transport keeps the runner occupied, which is what a graceful shutdown reads before handing it to - * the next daemon (#2681). The readiness preflight's own `uptime` probe is not charged. + * the next daemon (#2681). A command the runner answers inline is not charged at all, the same way + * the readiness preflight's own probe is not: an inline reply is no evidence about the queued work a + * charge stands for, so charging one would let it settle another command's debt (#2965). */ export async function executeRunnerCommandWithSession( device: DeviceInfo, @@ -924,40 +931,10 @@ export async function executeRunnerCommandWithSession( } try { const data = await parseRunnerResponse(response, session, logAttempt); - settleRunnerCommandAnswered(session); - // Mirror the runner's own main-thread occupancy stamped on this response: a runner that - // served a read off the XCTest channel (e.g. a private-AX capture) while a tree crawl it - // abandoned still grinds reports busy, so the healthy response must not be read as drained. - // Only a present stamp carries information; a recovered or journal-replayed response is - // written unstamped by design, and its absence must leave a prior busy report intact. - const stampedMainThreadBusy = readRunnerMainThreadBusy(data); - if (stampedMainThreadBusy !== undefined) { - session.runnerMainThreadBusy = stampedMainThreadBusy; - } - const runnerFatalReason = resolveRunnerFatalReason(data); - if (runnerFatalReason) { - session.lastHealthyMutation = undefined; - await invalidateRunnerSession(session, runnerFatalReason); - } else if (canSkipRunnerReadinessPreflightAfterHealthyMutation(runnerCommand)) { - session.lastHealthyMutation = { - atMs: Date.now(), - appBundleId: runnerCommand.appBundleId, - }; - } + await settleRunnerAnsweredExchange(session, runnerCommand, data); return data; } catch (error) { - // A structured runner reply is an answer whatever it reports; a transport-shaped failure - // (aborted body read, malformed payload) answered nothing and keeps the runner charged (#2681). - settleRunnerCommandExchange(session, error); - // A main-thread occupancy report (`RUNNER_BUSY`, or the `MAIN_THREAD_TIMEOUT` the stalling - // command itself returns) marks the runner still draining. Any OTHER structured runner reply was - // served off that abandoned work, so it has drained; a transport-shaped error answered nothing - // and leaves the report intact (#2552). - if (isRunnerMainThreadOccupiedError(error)) { - session.runnerMainThreadBusy = true; - } else if (isStructuredRunnerFailure(error)) { - session.runnerMainThreadBusy = false; - } + const answered = recordUnansweredRunnerExchange(session, runnerCommand, error); const runnerFatalReason = resolveRunnerFatalErrorReason(error); if (runnerFatalReason) { session.lastHealthyMutation = undefined; @@ -968,11 +945,71 @@ export async function executeRunnerCommandWithSession( // runner died mid-response); structured runner failures carry a `runner` // detail and keep their recency — the runner proved it is alive by // answering at all. - if (isStructuredRunnerFailure(error)) throw error; + if (answered) throw error; throw markSkippedPreflightTransportError(error, session, preflightDecision); } } +/** + * Records that this command's exchange got an answer: it discharges the charge, mirrors the + * runner's occupancy report, and applies what the payload says about the session. + */ +async function settleRunnerAnsweredExchange( + session: RunnerSession, + runnerCommand: RunnerCommand, + data: Record, +): Promise { + session.commandCharges.settleAnswered(runnerCommand.commandId); + // Mirror the runner's own main-thread occupancy stamped on this response: a runner that + // served a read off the XCTest channel (e.g. a private-AX capture) while a tree crawl it + // abandoned still grinds reports busy, so the healthy response must not be read as drained. + // Only a present stamp carries information; a recovered or journal-replayed response is + // written unstamped by design, and its absence must leave a prior busy report intact. + const stampedMainThreadBusy = readRunnerMainThreadBusy(data); + if (stampedMainThreadBusy !== undefined) { + session.runnerMainThreadBusy = stampedMainThreadBusy; + } + const runnerFatalReason = resolveRunnerFatalReason(data); + if (runnerFatalReason) { + session.lastHealthyMutation = undefined; + await invalidateRunnerSession(session, runnerFatalReason); + } else if (canSkipRunnerReadinessPreflightAfterHealthyMutation(runnerCommand)) { + session.lastHealthyMutation = { + atMs: Date.now(), + appBundleId: runnerCommand.appBundleId, + }; + } +} + +/** + * Records what a failed response proves about this command's exchange, and returns whether the + * runner answered it. + */ +function recordUnansweredRunnerExchange( + session: RunnerSession, + runnerCommand: RunnerCommand, + error: unknown, +): boolean { + // A structured runner reply is an answer whatever it reports; a transport-shaped failure + // (aborted body read, malformed payload) answered nothing and keeps the runner charged (#2681). + const answered = isStructuredRunnerFailure(error); + if (answered) { + session.commandCharges.settleAnswered(runnerCommand.commandId); + } else { + session.commandCharges.markAbandoned(runnerCommand.commandId); + } + // A main-thread occupancy report (`RUNNER_BUSY`, or the `MAIN_THREAD_TIMEOUT` the stalling + // command itself returns) marks the runner still draining. Any OTHER structured runner reply was + // served off that abandoned work, so it has drained; a transport-shaped error answered nothing + // and leaves the report intact (#2552). + if (isRunnerMainThreadOccupiedError(error)) { + session.runnerMainThreadBusy = true; + } else if (answered) { + session.runnerMainThreadBusy = false; + } + return answered; +} + function readRunnerMainThreadBusy(data: Record): boolean | undefined { return typeof data.runnerMainThreadBusy === 'boolean' ? data.runnerMainThreadBusy : undefined; } @@ -1023,9 +1060,12 @@ async function sendRunnerCommandAfterPreflight(params: { : { command: runnerCommand.command, commandId: runnerCommand.commandId }; // From here the runner holds our request, and a shutdown that hands it off would orphan a command - // nobody is waiting for any more. The charge is released only where a response is decoded, so a - // cancellation or transport drop leaves the occupancy it really created (#2681). - session.inFlightCommands += 1; + // nobody is waiting for any more. A readiness probe is charged no more than the preflight's own + // probe is: the runner serves it inline, so its reply says nothing about the queued work a charge + // stands for, and charging it would let one probe settle another command's debt (#2965). The trait + // that names the probe is pinned to the runner's inline routing by `runner-readiness-routing.test.ts`. + const charged = !isRunnerReadinessProbeCommand(runnerCommand); + if (charged) session.commandCharges.charge(runnerCommand.commandId); try { return await withDiagnosticTimer( 'ios_runner_command_send', @@ -1052,40 +1092,11 @@ async function sendRunnerCommandAfterPreflight(params: { diagnosticData, ); } catch (error) { - markRunnerCommandAbandoned(session); + if (charged) session.commandCharges.markAbandoned(runnerCommand.commandId); throw error; } } -/** - * The runner answered this exchange, so it is serving again: this command is answered, and so is the - * abandoned charge an earlier cancellation left behind — work still draining is stamped on this very - * reply (#2552, #2681). A run of abandoned exchanges leaves one residue charge per extra exchange, - * which keeps such a runner on the kill path rather than guessing it drained. - */ -function settleRunnerCommandAnswered(session: RunnerSession): void { - const abandonedCharge = session.hasAbandonedCommands ? 1 : 0; - session.inFlightCommands = Math.max(0, session.inFlightCommands - 1 - abandonedCharge); - session.hasAbandonedCommands = false; -} - -/** - * This process stopped waiting without ever seeing an answer. The command may still be executing on - * the runner, so its occupancy stays charged: only an answered exchange clears it (#2681). - */ -function markRunnerCommandAbandoned(session: RunnerSession): void { - session.hasAbandonedCommands = true; -} - -/** - * Settles the charge for an exchange that ended outside the success path. A structured runner reply - * answers even when it reports a failure; a transport-shaped one answers nothing (#2681). - */ -function settleRunnerCommandExchange(session: RunnerSession, error: unknown): void { - if (isStructuredRunnerFailure(error)) settleRunnerCommandAnswered(session); - else markRunnerCommandAbandoned(session); -} - async function runRunnerReadinessPreflight(params: { device: DeviceInfo; session: RunnerSession;