Skip to content

Commit 3b0e4db

Browse files
thymikeethymikee
committed
fix(replay): narrow #1385 retry to a real content-quality taxonomy, anchor the deadline
Address second review pass on #1386: P1 - `retriable === true` was too broad: Android's adb layer (adb-executor.ts) marks genuine transport mechanism failures retriable too (connection_dropped, device_offline, server_version_mismatch - an unchanged retry of the SAME adb command can succeed there), so those consumed the launch-race retry budget despite being exactly the mechanism failures the ADR says must fail fast. Replace the check with the narrow discriminator: the thrown error's androidSnapshotHelperFailureReason must be one of the three literal content verdicts rejectAndroidHelperContentUnavailable attaches (empty-helper-output, system-window-only, content-poor-app-window) - never the free-form message androidSnapshotHelperCaptureError sets for a crashed/timed-out helper, and never present at all for a permanently missing helper artifact. Added a regression test for an adb mechanism failure marked retriable at the transport level, proving it still fails on the first attempt. P2 - the 12s deadline started AFTER the first (unbounded) capture attempt, so the effective wall-clock cost was "first capture + 12s of retries", undocumented and untested as such. Anchor the deadline before the first attempt instead, and rewrite the comments/ADR to state precisely what is and is not bounded: the delay list caps attempt count, the deadline caps sleep time from entry, neither caps an individual capture's own duration. Added a fake-timer test proving the deadline (not just the delay array's length) is what stops retries when captures themselves consume real time.
1 parent 87c6fbf commit 3b0e4db

4 files changed

Lines changed: 205 additions & 44 deletions

File tree

‎docs/adr/0012-interactive-replay.md‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -477,24 +477,35 @@ target-binding divergences reported before the device action. This is not genera
477477
> launching/mounting when it lands, producing a transient content-quality verdict — `capture-failed`
478478
> (Android's snapshot helper returns "insufficient foreground app content" while the app is mounting, and
479479
> the capture path throws) or `sparse-snapshot` (iOS's private-AX fallback under load) — that is not a
480-
> real divergence, only an unlucky capture. This capture now retries with a bounded backoff (fixed delay
481-
> list, capped at a 12s wall-clock deadline) before falling through to `identity-unverifiable`, mirroring
482-
> the keep-polling semantics `wait`'s recorded-landmark identity verification (#1349) already applies on
483-
> its own (post-resolution) path. The retry is opt-in per call site (`retryLaunchRace`), not a change to
484-
> every `captureDivergenceObservation` caller: only the pre-dispatch verification gate in
480+
> real divergence, only an unlucky capture. This capture now retries with a bounded backoff (fixed
481+
> 7-entry delay list, 12s DELAY-ONLY budget — see below) before falling through to `identity-unverifiable`,
482+
> mirroring the keep-polling semantics `wait`'s recorded-landmark identity verification (#1349) already
483+
> applies on its own (post-resolution) path. The retry is opt-in per call site (`retryLaunchRace`), not a
484+
> change to every `captureDivergenceObservation` caller: only the pre-dispatch verification gate in
485485
> `verifyReplayActionTarget` races a launch this way — the post-failure diagnostic capture
486486
> (`buildReplayFailureDivergence`) and the post-resolution guard-mismatch capture both follow an
487487
> already-real failure, where retrying would only delay an already-decided divergence.
488488
>
489489
> The retry loop is further gated on the SAME content-quality-vs-mechanism-failure taxonomy #1381 draws
490490
> for the wait keep-poll loop (`isUnreadableCaptureContentError`): the non-throwing `sparse-snapshot`
491491
> verdict always retries (it is already a content-quality signal), but a thrown `capture-failed` only
492-
> retries when the underlying error is explicitly marked `retriable: true` — the SAME signal Android's
493-
> helper capture path already emits for a content-poor/system-window-only rejection
494-
> (`rejectAndroidHelperContentUnavailable`, `platforms/android/snapshot.ts`) and leaves unset for a
495-
> permanent one (the helper artifact itself missing, `androidSnapshotHelperUnavailableError`). A
496-
> mechanism failure — helper artifact missing, device offline — therefore still fails on the first
497-
> attempt rather than spending the full 12s retry budget on a foregone conclusion.
492+
> retries when the underlying error's `androidSnapshotHelperFailureReason` is one of the three literal
493+
> codes `rejectAndroidHelperContentUnavailable` (`platforms/android/snapshot.ts`) attaches to a
494+
> content-poor/system-window-only rejection — `empty-helper-output`, `system-window-only`,
495+
> `content-poor-app-window` (mirroring `AndroidHelperContentRecoveryDecision['reason']`,
496+
> `platforms/android/snapshot-content-recovery.ts`). This is deliberately narrower than the error's own
497+
> generic `retriable` flag: Android's adb layer separately marks true mechanism failures retriable too
498+
> (`connection_dropped`, `device_offline`, `server_version_mismatch` — an unchanged retry of the SAME adb
499+
> command can succeed there), and a helper artifact permanently missing
500+
> (`androidSnapshotHelperUnavailableError`) carries neither signal. A mechanism failure therefore still
501+
> fails on the first attempt rather than spending the retry budget on a foregone conclusion, regardless of
502+
> what its own `retriable` flag says.
503+
>
504+
> The 12s budget is a DELAY-ONLY bound, anchored at the first capture attempt: it caps how long this loop
505+
> sleeps between retries, not how long any individual capture attempt itself may run (a capture already
506+
> carries its own platform-level timeouts this loop does not shorten or re-implement). The fixed-length
507+
> delay array is a separate, independent bound on attempt COUNT, so a mocked-instant `sleep` in unit tests
508+
> cannot turn this into a real-time busy-loop.
498509
499510
### 4. Divergence wire contract and replay-only resume
500511

‎src/daemon/handlers/__tests__/session-replay-divergence.test.ts‎

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,16 @@ vi.mock('../../../core/dispatch.ts', async (importOriginal) => {
1414
// in `snapshot-capture.ts` (`capturePostActionAwareSnapshot`) awaits; making it
1515
// instant does not change control flow — the loop still runs, retries, and
1616
// re-captures — so the test still proves two dispatches and use of the retried
17-
// tree. No other test in this file triggers a sleep path.
17+
// tree. The #1385 `retryLaunchRace` retry loop (`captureDivergenceObservation`)
18+
// awaits this SAME `sleep`, further down in this file.
1819
vi.mock('../../../utils/timeouts.ts', async (importOriginal) => {
1920
const actual = await importOriginal<typeof import('../../../utils/timeouts.ts')>();
2021
return { ...actual, sleep: vi.fn(async () => {}) };
2122
});
2223

2324
import { dispatchCommand } from '../../../core/dispatch.ts';
2425
import type { RawSnapshotNode } from '../../../kernel/snapshot.ts';
26+
import { AppError } from '../../../kernel/errors.ts';
2527
import {
2628
makeAndroidSession,
2729
makeIosSession,
@@ -32,7 +34,10 @@ import {
3234
walkNonRawAndroidFixture,
3335
} from '../../../__tests__/test-utils/android-ui-hierarchy-fixtures.ts';
3436
import { SessionStore } from '../../session-store.ts';
35-
import { buildReplayFailureDivergence } from '../session-replay-divergence.ts';
37+
import {
38+
buildReplayFailureDivergence,
39+
captureDivergenceObservation,
40+
} from '../session-replay-divergence.ts';
3641

3742
const mockDispatchCommand = vi.mocked(dispatchCommand);
3843

@@ -1135,3 +1140,56 @@ test.each([
11351140
expect(screen.refs.some((ref) => ref.label === 'Battery charging, 100 percent.')).toBe(false);
11361141
expect(screen.refs.some((ref) => ref.label === 'Wifi signal full.')).toBe(false);
11371142
});
1143+
1144+
// #1385 P2: the retry deadline is a DELAY-ONLY budget, not a per-attempt
1145+
// capture timeout — this loop does not itself bound how long a single
1146+
// capture attempt runs, only how much SLEEP time it spends between attempts
1147+
// (measured from the first attempt). This test proves that bound is real
1148+
// wall-clock behavior, not merely "run the fixed 7-entry delay list": each
1149+
// mocked capture attempt here advances the (fake) system clock by 5s to
1150+
// stand in for a slow capture, so the 12s deadline is exhausted after just 3
1151+
// attempts — far short of the 8 attempts (1 + 7 delays) the array alone would
1152+
// allow — proving the deadline, not the array length, is what stopped it.
1153+
test('captureDivergenceObservation retryLaunchRace: the 12s deadline bounds retries even when captures themselves consume wall-clock time', async () => {
1154+
vi.useFakeTimers();
1155+
try {
1156+
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-device-replay-divergence-deadline-'));
1157+
const sessionStore = new SessionStore(path.join(root, 'sessions'));
1158+
const sessionName = 'default';
1159+
sessionStore.set(sessionName, makeIosSession(sessionName, { appBundleId: 'com.example.app' }));
1160+
1161+
mockDispatchCommand.mockImplementation(async () => {
1162+
vi.setSystemTime(new Date(Date.now() + 5_000));
1163+
throw new AppError(
1164+
'COMMAND_FAILED',
1165+
'Android snapshot helper returned insufficient foreground app content',
1166+
{ androidSnapshotHelperFailureReason: 'content-poor-app-window', retriable: true },
1167+
);
1168+
});
1169+
1170+
const action = {
1171+
ts: 0,
1172+
command: 'click',
1173+
positionals: ['label="Save"'],
1174+
flags: {},
1175+
result: { selectorChain: ['label="Save"', 'id="save"'] },
1176+
};
1177+
1178+
const observation = await captureDivergenceObservation({
1179+
session: sessionStore.get(sessionName)!,
1180+
sessionName,
1181+
sessionStore,
1182+
logPath: path.join(root, 'daemon.log'),
1183+
action,
1184+
retryLaunchRace: true,
1185+
});
1186+
1187+
expect(observation.state).toBe('unavailable');
1188+
// 1 initial attempt + 2 retries: the 3rd retry's pre-sleep remaining-budget
1189+
// check sees the deadline already passed (3 x 5s = 15s > 12s) and breaks
1190+
// BEFORE a 4th capture — not the 8 attempts the delay array alone permits.
1191+
expect(mockDispatchCommand).toHaveBeenCalledTimes(3);
1192+
} finally {
1193+
vi.useRealTimers();
1194+
}
1195+
});

‎src/daemon/handlers/__tests__/session-replay-target-verification-runtime.test.ts‎

Lines changed: 66 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -500,17 +500,20 @@ test('a target-binding divergence carries a real computed resume (step 5 wiring,
500500
// #1385: a step-2 press right after `open --relaunch` can capture while the
501501
// app is still launching/mounting. The bounded retry in
502502
// `captureDivergenceObservation` (`retryLaunchRace`) only rides out a
503-
// content-quality verdict — the same `retriable: true` signal Android's
504-
// helper capture path already emits for a content-poor/system-window-only
505-
// rejection (`rejectAndroidHelperContentUnavailable`) — not a mechanism
506-
// failure (helper artifact missing, device offline), which must still fail
507-
// fast. `throwRetriableCaptureFailure`/`throwPermanentCaptureFailure` below
508-
// build the two shapes.
509-
function throwRetriableCaptureFailure(): never {
503+
// content-quality verdict — Android's helper path attaches
504+
// `androidSnapshotHelperFailureReason` as one of exactly three literals
505+
// ('empty-helper-output' | 'system-window-only' | 'content-poor-app-window')
506+
// on that verdict (`rejectAndroidHelperContentUnavailable`) — never a
507+
// mechanism failure (helper artifact missing, device offline, adb connection
508+
// dropped), which must still fail fast even when the underlying error
509+
// separately carries `retriable: true` for its own (adb-level, same-command)
510+
// retry semantics. The three `throw*` helpers below build the three shapes
511+
// this distinction must tell apart.
512+
function throwContentQualityCaptureFailure(): never {
510513
throw new AppError(
511514
'COMMAND_FAILED',
512515
'Android snapshot helper returned insufficient foreground app content',
513-
{ retriable: true },
516+
{ androidSnapshotHelperFailureReason: 'content-poor-app-window', retriable: true },
514517
);
515518
}
516519

@@ -521,19 +524,32 @@ function throwPermanentCaptureFailure(): never {
521524
);
522525
}
523526

527+
function throwAdbMechanismFailureMarkedRetriable(): never {
528+
// Mirrors `classifyAdbFailure`'s `connection_dropped` entry
529+
// (adb-executor.ts): `retriable: true` at the adb-transport level (retrying
530+
// the SAME adb command can succeed), but not evidence of a content-quality
531+
// capture verdict — a launch-race retry has no reason to believe THIS
532+
// failure resolves itself, and must not be fooled by the bare `retriable`
533+
// flag into treating it as one.
534+
throw new AppError('COMMAND_FAILED', 'adb: transport error', {
535+
adbFailure: 'connection_dropped',
536+
retriable: true,
537+
});
538+
}
539+
524540
test('a transient content-quality capture failure right after launch recovers within the bounded retry, and the action still dispatches', async () => {
525541
const root = fs.mkdtempSync(
526542
path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-recover-'),
527543
);
528544
const { sessionStore, sessionName } = setupSession(root);
529545
const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']);
530546

531-
// First two captures fail closed with a content-quality (retriable) verdict
532-
// (mirrors a mid-launch/mid-mount capture); the third lands after the app
533-
// settles and finds the recorded target.
547+
// First two captures fail closed with a content-quality verdict (mirrors a
548+
// mid-launch/mid-mount capture); the third lands after the app settles and
549+
// finds the recorded target.
534550
mockDispatchCommand
535-
.mockImplementationOnce(async () => throwRetriableCaptureFailure())
536-
.mockImplementationOnce(async () => throwRetriableCaptureFailure())
551+
.mockImplementationOnce(async () => throwContentQualityCaptureFailure())
552+
.mockImplementationOnce(async () => throwContentQualityCaptureFailure())
537553
.mockResolvedValue({
538554
nodes: [
539555
{
@@ -573,7 +589,7 @@ test('a content-quality capture failure that never recovers still fails closed a
573589
const { sessionStore, sessionName } = setupSession(root);
574590
const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']);
575591

576-
mockDispatchCommand.mockImplementation(async () => throwRetriableCaptureFailure());
592+
mockDispatchCommand.mockImplementation(async () => throwContentQualityCaptureFailure());
577593

578594
const invoked: DaemonRequest[] = [];
579595
const response = await runReplayScriptFile({
@@ -608,8 +624,9 @@ test('a permanent (non-content-quality) capture failure fails fast without retry
608624
const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']);
609625

610626
// A missing helper artifact is not something a launch-race retry can fix —
611-
// it never carries `retriable: true`, so the capture must fail on the
612-
// FIRST attempt, not spend the retry budget on a foregone conclusion.
627+
// it never carries `androidSnapshotHelperFailureReason` at all, so the
628+
// capture must fail on the FIRST attempt, not spend the retry budget on a
629+
// foregone conclusion.
613630
mockDispatchCommand.mockImplementation(async () => throwPermanentCaptureFailure());
614631

615632
const invoked: DaemonRequest[] = [];
@@ -631,3 +648,36 @@ test('a permanent (non-content-quality) capture failure fails fast without retry
631648
expect(divergence.kind).toBe('identity-unverifiable');
632649
expect(mockDispatchCommand).toHaveBeenCalledTimes(1);
633650
});
651+
652+
test('an adb mechanism failure marked retriable at the transport level still fails fast (retriable is not a content-quality signal)', async () => {
653+
const root = fs.mkdtempSync(
654+
path.join(os.tmpdir(), 'agent-device-replay-target-verify-launch-race-adb-mechanism-'),
655+
);
656+
const { sessionStore, sessionName } = setupSession(root);
657+
const filePath = writeReplayFile(root, [SAVE_ANNOTATION, 'click id="save"']);
658+
659+
// A dropped adb connection carries `retriable: true` (retrying the SAME adb
660+
// command can succeed) but no `androidSnapshotHelperFailureReason` content
661+
// verdict — the launch-race retry must not treat the bare `retriable` flag
662+
// as proof this is a "the app is still mounting" content-quality failure.
663+
mockDispatchCommand.mockImplementation(async () => throwAdbMechanismFailureMarkedRetriable());
664+
665+
const invoked: DaemonRequest[] = [];
666+
const response = await runReplayScriptFile({
667+
req: baseReq({ positionals: [filePath] }),
668+
sessionName,
669+
logPath: path.join(root, 'daemon.log'),
670+
sessionStore,
671+
invoke: async (req) => {
672+
invoked.push(req);
673+
return { ok: true, data: {} };
674+
},
675+
});
676+
677+
expect(invoked.length).toBe(0);
678+
expect(response.ok).toBe(false);
679+
if (response.ok) return;
680+
const divergence = response.error.details?.divergence as Record<string, unknown>;
681+
expect(divergence.kind).toBe('identity-unverifiable');
682+
expect(mockDispatchCommand).toHaveBeenCalledTimes(1);
683+
});

0 commit comments

Comments
 (0)