From dcd082eb89606e526c54e0b8dd27339f04e99640 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 08:01:08 +0200 Subject: [PATCH 1/4] fix: stop the slow-snapshot warning from firing on a single cold start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session's first capture folds one-time startup (runner launch, helper install) into its duration, and nearest-rank p95 over a small sample set equals its largest one or two values — so one 12s cold start produced 'snapshots are slow in this run: p95 12417ms over 1 captures' with hints blaming device load or a stale daemon, inviting exactly the restart spirals the hints exist to prevent (observed on every AppControlBench run). The warning now judges only warm captures (first sample excluded) and only once at least three exist; the displayed stats still cover every sample, so the cold start remains visible as maxMs. --- .../contracts/src/snapshot-diagnostics.ts | 22 +++++++++++++- src/__tests__/snapshot-diagnostics.test.ts | 30 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/packages/contracts/src/snapshot-diagnostics.ts b/packages/contracts/src/snapshot-diagnostics.ts index a109b63783..898a5b9696 100644 --- a/packages/contracts/src/snapshot-diagnostics.ts +++ b/packages/contracts/src/snapshot-diagnostics.ts @@ -4,6 +4,9 @@ import { isRecord } from './json.ts'; const SLOW_SNAPSHOT_P95_WARNING_MS = 1_500; +/** Warm captures needed before chronic slowness is distinguishable from noise. */ +const MIN_WARNING_SAMPLE_COUNT = 3; + export type SnapshotTimingSample = { durationMs: number; backend?: SnapshotBackend; @@ -47,7 +50,24 @@ export function summarizeSnapshotDiagnostics( ): SnapshotDiagnosticsSummary | undefined { const samples = session?.snapshotDiagnostics?.samples; if (!samples || samples.length === 0) return undefined; - return summarizeSnapshotTimingSamples(samples); + const stats = buildSnapshotTimingStats(samples); + // The session's FIRST capture folds one-time startup (runner launch, helper + // install) into its duration, and nearest-rank p95 over a small run equals + // its largest one or two samples — together they made a single cold start + // read as "device degraded" with restart-inviting hints. The warning judges + // only warm captures, and only once there are enough to mean anything; the + // displayed stats still cover every sample. + const warmSamples = samples.slice(1); + const warmStats = + warmSamples.length >= MIN_WARNING_SAMPLE_COUNT + ? buildSnapshotTimingStats(warmSamples) + : undefined; + return { + stats, + ...(warmStats && warmStats.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS + ? { warning: formatSlowSnapshotWarning(warmStats) } + : {}), + }; } export function summarizeSnapshotTimingSamples( diff --git a/src/__tests__/snapshot-diagnostics.test.ts b/src/__tests__/snapshot-diagnostics.test.ts index 70ebb35bfd..167b5d7c06 100644 --- a/src/__tests__/snapshot-diagnostics.test.ts +++ b/src/__tests__/snapshot-diagnostics.test.ts @@ -11,6 +11,8 @@ test('records session snapshot timing stats', () => { recordSnapshotTiming(session, { durationMs: 400, backend: 'android', platform: 'android' }); recordSnapshotTiming(session, { durationMs: 2_100, backend: 'android', platform: 'android' }); + // Two samples cannot distinguish chronic slowness from a one-off — stats are + // reported, but no warning fires. expect(summarizeSnapshotDiagnostics(session)).toEqual({ stats: { count: 2, @@ -21,10 +23,36 @@ test('records session snapshot timing stats', () => { platform: 'android', backends: { android: 2 }, }, - warning: expect.stringContaining('p95 2100ms over 2 captures'), }); }); +test('a slow cold-start capture alone does not warn', () => { + const session = {}; + + // First capture folds runner/helper startup (12s); warm captures are fast. + recordSnapshotTiming(session, { durationMs: 12_400, backend: 'xctest', platform: 'ios' }); + for (const durationMs of [520, 540, 510, 530, 525]) { + recordSnapshotTiming(session, { durationMs, backend: 'xctest', platform: 'ios' }); + } + + const summary = summarizeSnapshotDiagnostics(session); + expect(summary?.warning).toBeUndefined(); + // The cold sample still shows in the full stats. + expect(summary?.stats).toMatchObject({ count: 6, maxMs: 12_400 }); +}); + +test('chronically slow warm captures warn without counting the cold start', () => { + const session = {}; + + recordSnapshotTiming(session, { durationMs: 12_400, backend: 'xctest', platform: 'ios' }); + for (const durationMs of [2_600, 2_700, 2_500, 2_800]) { + recordSnapshotTiming(session, { durationMs, backend: 'xctest', platform: 'ios' }); + } + + const summary = summarizeSnapshotDiagnostics(session); + expect(summary?.warning).toContain('p95 2800ms over 4 captures'); +}); + test('merges snapshot diagnostics without inflating capture count', () => { const merged = mergeSnapshotDiagnostics([ { From d7ad2125e6db5f95ec09aaff9d9d16c26c1977ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 08:23:13 +0200 Subject: [PATCH 2/4] refactor: single-home the slow-snapshot warning policy (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Push the warm-judging rule down into summarizeSnapshotTimingSamples so the interactive session path and all three replay handler paths share one policy, and summarizeSnapshotDiagnostics returns to a one-line delegate. Merge no longer judges slowness from lossy order-less reconstructed samples (a run's cold start comes back as both its p95 and max): it aggregates display stats and carries a warning only when a constituent run judged one itself. MIN_WARNING_SAMPLE_COUNT renamed to MIN_WARM_SAMPLE_COUNT — it gates warm samples, not total captures. Cold-start regression tests move to the shared layer; suite-aggregation tests now pin that individually-silent runs merge silent. --- .../contracts/src/snapshot-diagnostics.ts | 54 +++++++++---------- src/__tests__/snapshot-diagnostics.test.ts | 47 ++++++++++------ .../session-replay-runtime-maestro.test.ts | 24 ++++++--- .../__tests__/session-test-suite.test.ts | 12 ++++- 4 files changed, 86 insertions(+), 51 deletions(-) diff --git a/packages/contracts/src/snapshot-diagnostics.ts b/packages/contracts/src/snapshot-diagnostics.ts index 898a5b9696..65d584828d 100644 --- a/packages/contracts/src/snapshot-diagnostics.ts +++ b/packages/contracts/src/snapshot-diagnostics.ts @@ -5,7 +5,7 @@ import { isRecord } from './json.ts'; const SLOW_SNAPSHOT_P95_WARNING_MS = 1_500; /** Warm captures needed before chronic slowness is distinguishable from noise. */ -const MIN_WARNING_SAMPLE_COUNT = 3; +const MIN_WARM_SAMPLE_COUNT = 3; export type SnapshotTimingSample = { durationMs: number; @@ -50,37 +50,21 @@ export function summarizeSnapshotDiagnostics( ): SnapshotDiagnosticsSummary | undefined { const samples = session?.snapshotDiagnostics?.samples; if (!samples || samples.length === 0) return undefined; - const stats = buildSnapshotTimingStats(samples); - // The session's FIRST capture folds one-time startup (runner launch, helper - // install) into its duration, and nearest-rank p95 over a small run equals - // its largest one or two samples — together they made a single cold start - // read as "device degraded" with restart-inviting hints. The warning judges - // only warm captures, and only once there are enough to mean anything; the - // displayed stats still cover every sample. - const warmSamples = samples.slice(1); - const warmStats = - warmSamples.length >= MIN_WARNING_SAMPLE_COUNT - ? buildSnapshotTimingStats(warmSamples) - : undefined; - return { - stats, - ...(warmStats && warmStats.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS - ? { warning: formatSlowSnapshotWarning(warmStats) } - : {}), - }; + return summarizeSnapshotTimingSamples(samples); } export function summarizeSnapshotTimingSamples( samples: SnapshotTimingSample[], ): SnapshotDiagnosticsSummary | undefined { if (samples.length === 0) return undefined; - const stats = buildSnapshotTimingStats(samples); - return { - stats, - ...(stats.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS - ? { warning: formatSlowSnapshotWarning(stats) } - : {}), - }; + // The first capture folds one-time startup (runner launch, helper install) + // into its duration, and nearest-rank p95 over a small sample set is just + // its max — so the warning judges warm captures only, and only once enough + // exist to mean anything. Displayed stats still cover every sample. + const warm = samples.slice(1); + const judged = + warm.length >= MIN_WARM_SAMPLE_COUNT ? buildSnapshotTimingStats(warm) : undefined; + return toSummary(buildSnapshotTimingStats(samples), judged); } export function mergeSnapshotDiagnostics( @@ -89,14 +73,30 @@ export function mergeSnapshotDiagnostics( const samples = summaries.flatMap((summary) => samplesFromStats(summary?.stats)); if (samples.length === 0) return undefined; const stats = buildSnapshotTimingStats(samples); + // Reconstructed samples are lossy and order-less (a run's cold start comes + // back as both its p95 and its max), so only layers holding ordered live + // samples judge slowness; merge aggregates display stats and carries a + // warning only when a constituent run judged one itself. return { stats, - ...(stats.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS + ...(summaries.some((summary) => summary?.warning) ? { warning: formatSlowSnapshotWarning(stats) } : {}), }; } +function toSummary( + stats: SnapshotTimingStats, + judged: SnapshotTimingStats | undefined, +): SnapshotDiagnosticsSummary { + return { + stats, + ...(judged && judged.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS + ? { warning: formatSlowSnapshotWarning(judged) } + : {}), + }; +} + export function readSnapshotDiagnosticsSummary( value: unknown, ): SnapshotDiagnosticsSummary | undefined { diff --git a/src/__tests__/snapshot-diagnostics.test.ts b/src/__tests__/snapshot-diagnostics.test.ts index 167b5d7c06..941340ea98 100644 --- a/src/__tests__/snapshot-diagnostics.test.ts +++ b/src/__tests__/snapshot-diagnostics.test.ts @@ -3,6 +3,7 @@ import { mergeSnapshotDiagnostics, recordSnapshotTiming, summarizeSnapshotDiagnostics, + summarizeSnapshotTimingSamples, } from '@agent-device/contracts/capture'; test('records session snapshot timing stats', () => { @@ -27,29 +28,25 @@ test('records session snapshot timing stats', () => { }); test('a slow cold-start capture alone does not warn', () => { - const session = {}; - // First capture folds runner/helper startup (12s); warm captures are fast. - recordSnapshotTiming(session, { durationMs: 12_400, backend: 'xctest', platform: 'ios' }); - for (const durationMs of [520, 540, 510, 530, 525]) { - recordSnapshotTiming(session, { durationMs, backend: 'xctest', platform: 'ios' }); - } + const samples = [12_400, 520, 540, 510, 530, 525].map((durationMs) => ({ + durationMs, + platform: 'ios' as const, + })); - const summary = summarizeSnapshotDiagnostics(session); + const summary = summarizeSnapshotTimingSamples(samples); expect(summary?.warning).toBeUndefined(); // The cold sample still shows in the full stats. expect(summary?.stats).toMatchObject({ count: 6, maxMs: 12_400 }); }); test('chronically slow warm captures warn without counting the cold start', () => { - const session = {}; + const samples = [12_400, 2_600, 2_700, 2_500, 2_800].map((durationMs) => ({ + durationMs, + platform: 'ios' as const, + })); - recordSnapshotTiming(session, { durationMs: 12_400, backend: 'xctest', platform: 'ios' }); - for (const durationMs of [2_600, 2_700, 2_500, 2_800]) { - recordSnapshotTiming(session, { durationMs, backend: 'xctest', platform: 'ios' }); - } - - const summary = summarizeSnapshotDiagnostics(session); + const summary = summarizeSnapshotTimingSamples(samples); expect(summary?.warning).toContain('p95 2800ms over 4 captures'); }); @@ -84,5 +81,25 @@ test('merges snapshot diagnostics without inflating capture count', () => { maxMs: 1_900, platform: 'android', }); - expect(merged?.warning).toContain('p95 1900ms over 3 captures'); + // Neither constituent judged a warning (reconstructed samples are lossy and + // order-less), so the merge must not resurrect one from the aggregate. + expect(merged?.warning).toBeUndefined(); +}); + +test('merge carries a warning only when a constituent run judged one', () => { + const stats = { + count: 4, + p50Ms: 2_600, + p95Ms: 2_800, + maxMs: 2_800, + slowThresholdMs: 1_500, + platform: 'ios', + } as const; + + const merged = mergeSnapshotDiagnostics([ + { stats: { ...stats } }, + { stats: { ...stats }, warning: 'Warning: ios snapshots are slow in this run: …' }, + ]); + + expect(merged?.warning).toContain('snapshots are slow in this run'); }); diff --git a/src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts b/src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts index 9c085b7151..43d22607c8 100644 --- a/src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts +++ b/src/daemon/handlers/__tests__/session-replay-runtime-maestro.test.ts @@ -254,7 +254,12 @@ test('.ad replay normalizes resolved gesture and swipe syntax into structured da test('runReplayScriptFile reports snapshot diagnostics from per-action session samples', async () => { const root = mkdtempForTestSync('agent-device-replay-snapshot-samples-'); const scriptPath = path.join(root, 'flow.ad'); - fs.writeFileSync(scriptPath, ['snapshot', 'snapshot', ''].join('\n')); + // Four warm captures beyond the cold start: the slow-run warning judges warm + // samples only and needs at least three of them. + fs.writeFileSync( + scriptPath, + ['snapshot', 'snapshot', 'snapshot', 'snapshot', 'snapshot', ''].join('\n'), + ); const sessionStore = new SessionStore(path.join(root, 'state')); sessionStore.set( 's', @@ -305,14 +310,19 @@ test('runReplayScriptFile reports snapshot diagnostics from per-action session s const diagnostics = response.data?.snapshotDiagnostics as | { stats?: { count?: number }; warning?: string } | undefined; - assert.equal(diagnostics?.stats?.count, 2); - assert.match(String(diagnostics?.warning), /p95 1900ms over 2 captures/); + assert.equal(diagnostics?.stats?.count, 5); + assert.match(String(diagnostics?.warning), /p95 1900ms over 4 captures/); }); test('runReplayScriptFile reports snapshot diagnostics on replay failure', async () => { const root = mkdtempForTestSync('agent-device-replay-snapshot-failure-'); const scriptPath = path.join(root, 'flow.ad'); - fs.writeFileSync(scriptPath, ['snapshot', 'click "Missing"', ''].join('\n')); + // Three warm captures precede the failing click so the failure-path summary + // has enough warm samples to judge. + fs.writeFileSync( + scriptPath, + ['snapshot', 'snapshot', 'snapshot', 'snapshot', 'click "Missing"', ''].join('\n'), + ); const sessionStore = new SessionStore(path.join(root, 'state')); sessionStore.set( 's', @@ -341,7 +351,7 @@ test('runReplayScriptFile reports snapshot diagnostics on replay failure', async backend: 'xctest', platform: 'ios', }); - if (captures === 1) return { ok: true, data: {} }; + if (captures < 5) return { ok: true, data: {} }; return { ok: false, error: { code: 'COMMAND_FAILED', message: 'button missing' } }; }, }); @@ -350,9 +360,9 @@ test('runReplayScriptFile reports snapshot diagnostics on replay failure', async const diagnostics = response.error.details?.snapshotDiagnostics as | { stats?: { count?: number; p95Ms?: number }; warning?: string } | undefined; - assert.equal(diagnostics?.stats?.count, 2); + assert.equal(diagnostics?.stats?.count, 5); assert.equal(diagnostics?.stats?.p95Ms, 2_100); - assert.match(String(diagnostics?.warning), /p95 2100ms over 2 captures/); + assert.match(String(diagnostics?.warning), /p95 2100ms over 4 captures/); }); test('runReplayScriptFile applies CLI env overrides before Maestro compat mapping', async () => { diff --git a/src/daemon/handlers/__tests__/session-test-suite.test.ts b/src/daemon/handlers/__tests__/session-test-suite.test.ts index 260516acfb..aa6d3e75f8 100644 --- a/src/daemon/handlers/__tests__/session-test-suite.test.ts +++ b/src/daemon/handlers/__tests__/session-test-suite.test.ts @@ -467,8 +467,12 @@ test('test aggregates snapshot diagnostics from replay session samples', async ( maxMs: 1_900, platform: 'android', }, - warning: expect.stringContaining('p95 1900ms over 2 captures'), }); + // Neither single-capture run had enough warm samples to judge slowness, so + // the merged suite report must not resurrect a warning from the aggregate. + expect( + (data.snapshotDiagnostics as Record | undefined)?.warning, + ).toBeUndefined(); expect((data.tests as Array>)[1]?.snapshotDiagnostics).toMatchObject({ stats: { count: 1, @@ -516,14 +520,18 @@ test('test aggregates snapshot diagnostics from failed replay session samples', const data = expectOkData(response); expect(data.failed).toBe(1); + // A single capture is indistinguishable from a cold start — diagnostics are + // reported without a slow-run warning. expect(data.snapshotDiagnostics).toMatchObject({ stats: { count: 1, p95Ms: 2_100, platform: 'android', }, - warning: expect.stringContaining('p95 2100ms over 1 captures'), }); + expect( + (data.snapshotDiagnostics as Record | undefined)?.warning, + ).toBeUndefined(); expect((data.tests as Array>)[0]).toMatchObject({ status: 'failed', snapshotDiagnostics: { From 65b3c84fc7901c92d84e5989b2cfc4c15a4ba17a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 08:36:34 +0200 Subject: [PATCH 3/4] style: oxfmt --- packages/contracts/src/snapshot-diagnostics.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/contracts/src/snapshot-diagnostics.ts b/packages/contracts/src/snapshot-diagnostics.ts index 65d584828d..8bc0944906 100644 --- a/packages/contracts/src/snapshot-diagnostics.ts +++ b/packages/contracts/src/snapshot-diagnostics.ts @@ -62,8 +62,7 @@ export function summarizeSnapshotTimingSamples( // its max — so the warning judges warm captures only, and only once enough // exist to mean anything. Displayed stats still cover every sample. const warm = samples.slice(1); - const judged = - warm.length >= MIN_WARM_SAMPLE_COUNT ? buildSnapshotTimingStats(warm) : undefined; + const judged = warm.length >= MIN_WARM_SAMPLE_COUNT ? buildSnapshotTimingStats(warm) : undefined; return toSummary(buildSnapshotTimingStats(samples), judged); } From 3e719696fe9de32d58700850533d8ed3d796dd0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Thu, 6 Aug 2026 10:28:13 +0200 Subject: [PATCH 4/4] fix: quorum-gate the warm warning and make the merged message speak about warned runs (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single warm outlier could still fire the warning (nearest-rank p95 is the maximum through nineteen samples): chronic now additionally requires at least two slow warm captures. And the merged warning formatted its number from the reconstructed aggregate, so one slow run among many fast ones produced 'slow: p95 ' — the merged message now reports how many runs warned and the worst warned run's own p95, never the aggregate. Regressions for both: one-warm-outlier stays silent; a slow run merged with many fast ones warns with the slow run's number while the aggregate p95 sits below the threshold. --- .../contracts/src/snapshot-diagnostics.ts | 63 +++++++++++++------ src/__tests__/snapshot-diagnostics.test.ts | 49 ++++++++++++++- 2 files changed, 93 insertions(+), 19 deletions(-) diff --git a/packages/contracts/src/snapshot-diagnostics.ts b/packages/contracts/src/snapshot-diagnostics.ts index 8bc0944906..5618fa04d2 100644 --- a/packages/contracts/src/snapshot-diagnostics.ts +++ b/packages/contracts/src/snapshot-diagnostics.ts @@ -7,6 +7,9 @@ const SLOW_SNAPSHOT_P95_WARNING_MS = 1_500; /** Warm captures needed before chronic slowness is distinguishable from noise. */ const MIN_WARM_SAMPLE_COUNT = 3; +/** Slow warm captures needed before slowness counts as chronic rather than a one-off hiccup. */ +const MIN_SLOW_WARM_SAMPLES = 2; + export type SnapshotTimingSample = { durationMs: number; backend?: SnapshotBackend; @@ -60,38 +63,52 @@ export function summarizeSnapshotTimingSamples( // The first capture folds one-time startup (runner launch, helper install) // into its duration, and nearest-rank p95 over a small sample set is just // its max — so the warning judges warm captures only, and only once enough - // exist to mean anything. Displayed stats still cover every sample. + // exist to mean anything. A single warm outlier is a hiccup, not chronic + // slowness: the warning additionally needs a quorum of slow warm captures, + // so nearest-rank-equals-max can never fire it alone. Displayed stats still + // cover every sample. const warm = samples.slice(1); const judged = warm.length >= MIN_WARM_SAMPLE_COUNT ? buildSnapshotTimingStats(warm) : undefined; - return toSummary(buildSnapshotTimingStats(samples), judged); + const slowWarmCount = warm.filter( + (sample) => sample.durationMs >= SLOW_SNAPSHOT_P95_WARNING_MS, + ).length; + const warns = + judged !== undefined && + judged.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS && + slowWarmCount >= MIN_SLOW_WARM_SAMPLES; + return { + stats: buildSnapshotTimingStats(samples), + ...(warns && judged ? { warning: formatSlowSnapshotWarning(judged) } : {}), + }; } export function mergeSnapshotDiagnostics( summaries: Array, ): SnapshotDiagnosticsSummary | undefined { - const samples = summaries.flatMap((summary) => samplesFromStats(summary?.stats)); + const present = summaries.filter( + (summary): summary is SnapshotDiagnosticsSummary => summary !== undefined, + ); + const samples = present.flatMap((summary) => samplesFromStats(summary.stats)); if (samples.length === 0) return undefined; const stats = buildSnapshotTimingStats(samples); // Reconstructed samples are lossy and order-less (a run's cold start comes // back as both its p95 and its max), so only layers holding ordered live // samples judge slowness; merge aggregates display stats and carries a - // warning only when a constituent run judged one itself. - return { - stats, - ...(summaries.some((summary) => summary?.warning) - ? { warning: formatSlowSnapshotWarning(stats) } - : {}), - }; -} - -function toSummary( - stats: SnapshotTimingStats, - judged: SnapshotTimingStats | undefined, -): SnapshotDiagnosticsSummary { + // warning only when a constituent run judged one itself. The message speaks + // about the warned runs, never the aggregate — one slow run among many fast + // ones would otherwise produce "slow: p95 ". + const warned = present.filter((summary) => summary.warning); return { stats, - ...(judged && judged.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS - ? { warning: formatSlowSnapshotWarning(judged) } + ...(warned.length > 0 + ? { + warning: formatMergedSlowSnapshotWarning({ + warnedCount: warned.length, + totalCount: present.length, + worst: warned.reduce((max, summary) => Math.max(max, summary.stats.p95Ms), 0), + platform: stats.platform, + }), + } : {}), }; } @@ -147,6 +164,16 @@ function formatSlowSnapshotWarning(stats: SnapshotTimingStats): string { return `Warning: ${platform}snapshots are slow in this run: p95 ${stats.p95Ms}ms over ${stats.count} captures. Possible causes: device load, app or dev server stuck, helper fallback, or stale daemon.`; } +function formatMergedSlowSnapshotWarning(params: { + warnedCount: number; + totalCount: number; + worst: number; + platform?: PublicPlatform; +}): string { + const platform = params.platform ? `${params.platform} ` : ''; + return `Warning: ${platform}snapshots were slow in ${params.warnedCount} of ${params.totalCount} runs (worst run p95 ${params.worst}ms). Possible causes: device load, app or dev server stuck, helper fallback, or stale daemon.`; +} + function readSnapshotTimingStats(value: unknown): SnapshotTimingStats | undefined { if (!isRecord(value)) return undefined; const required = readRequiredSnapshotTimingStats(value); diff --git a/src/__tests__/snapshot-diagnostics.test.ts b/src/__tests__/snapshot-diagnostics.test.ts index 941340ea98..230710af33 100644 --- a/src/__tests__/snapshot-diagnostics.test.ts +++ b/src/__tests__/snapshot-diagnostics.test.ts @@ -40,6 +40,18 @@ test('a slow cold-start capture alone does not warn', () => { expect(summary?.stats).toMatchObject({ count: 6, maxMs: 12_400 }); }); +test('a single warm outlier does not warn (quorum rule)', () => { + // One 12s hiccup among fast warm captures: nearest-rank p95 IS that outlier + // through n=19, so without a quorum the restart-inviting warning would fire + // from one bad sample. + const samples = [12_400, 520, 520, 12_400].map((durationMs) => ({ + durationMs, + platform: 'ios' as const, + })); + + expect(summarizeSnapshotTimingSamples(samples)?.warning).toBeUndefined(); +}); + test('chronically slow warm captures warn without counting the cold start', () => { const samples = [12_400, 2_600, 2_700, 2_500, 2_800].map((durationMs) => ({ durationMs, @@ -101,5 +113,40 @@ test('merge carries a warning only when a constituent run judged one', () => { { stats: { ...stats }, warning: 'Warning: ios snapshots are slow in this run: …' }, ]); - expect(merged?.warning).toContain('snapshots are slow in this run'); + expect(merged?.warning).toContain('slow in 1 of 2 runs'); + expect(merged?.warning).toContain('worst run p95 2800ms'); +}); + +test('merged warning speaks about the warned run, never the fast aggregate', () => { + // One slow warned run among many fast ones: the aggregate p95 is a fast + // number, and quoting it in a slowness warning would be self-contradictory. + const fast = { + count: 20, + p50Ms: 90, + p95Ms: 100, + maxMs: 100, + slowThresholdMs: 1_500, + platform: 'ios', + } as const; + const slow = { + count: 2, + p50Ms: 2_500, + p95Ms: 2_900, + maxMs: 2_900, + slowThresholdMs: 1_500, + platform: 'ios', + } as const; + + const merged = mergeSnapshotDiagnostics([ + { stats: { ...fast } }, + { stats: { ...fast } }, + { stats: { ...fast } }, + { stats: { ...slow }, warning: 'Warning: ios snapshots are slow in this run: …' }, + ]); + + // Aggregate p95 is fast — it must not appear as the headline number. + expect(merged?.stats.p95Ms).toBeLessThan(1_500); + expect(merged?.warning).toContain('slow in 1 of 4 runs'); + expect(merged?.warning).toContain('worst run p95 2900ms'); + expect(merged?.warning).not.toContain(`p95 ${merged?.stats.p95Ms}ms`); });