Repository navigation
fix: stop the slow-snapshot warning from firing on a single cold start - #1628
Conversation
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.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Thermo-nuclear quality reviewVerdict: the behavior change is right and well-evidenced, but it landed at the wrong layer — the policy now lives in one of the four paths that produce this summary, and the other three silently disagree with it. There's a code-judo move here that fixes all of them with a smaller diff than the current one. 1. The policy is forked: 3 of 4 summary-producing paths keep the old rule
The PR body's own trigger scenario — fresh daemon + runner, first capture folds startup — is exactly how replay runs execute in bench/CI. The false positive this PR kills still fires verbatim on every cold-started replay result, with the same restart-inviting hint text. Fixing one hand-picked call site is the structural problem: nothing enumerates "the paths that judge ordered live samples," so nothing forces them to agree, and today they don't. 2. The judo move: push the policy one level down and revert the forkThe diff currently un-delegates export function summarizeSnapshotTimingSamples(
samples: SnapshotTimingSample[],
): SnapshotDiagnosticsSummary | undefined {
if (samples.length === 0) return undefined;
// The first capture folds one-time startup (runner launch, helper install),
// and nearest-rank p95 over a small run 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);
}
function toSummary(
stats: SnapshotTimingStats,
judged: SnapshotTimingStats | undefined,
): SnapshotDiagnosticsSummary {
return {
stats,
...(judged && judged.p95Ms >= SLOW_SNAPSHOT_P95_WARNING_MS
? { warning: formatSlowSnapshotWarning(judged) }
: {}),
};
}
One honest caveat: for the maestro path with 3.
|
|
Review at exact head P1: the warm-only policy is applied only by P2: The direct-session tests are red-before meaningful, but only cover one entrypoint. Do not apply |
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.
|
All four points taken in d7ad212: 1+2. Policy single-homed via the judo move:
Nits: comment trimmed to the invariant only; constant renamed |
|
Re-review at P1: merge gates on P2: CI is green, but the prior P2 was not addressed; do not apply |
…bout warned runs (review) 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 <fast number>' — 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.
|
Both blockers addressed in 3e71969: P1 — the merged warning no longer formats from the reconstructed aggregate: it reports P2 — chronic now requires a quorum: at least Full affected suite green (diagnostics, suite-aggregation, maestro replay, serialization, capture). |
|
Re-reviewed exact head Readiness still needs exact-head live evidence under |
Exact-head live evidence (
|
| response | count | p50Ms | p95Ms | maxMs | warning |
|---|---|---|---|---|---|
| snap 1 | 1 | 12772 | 12772 | 12772 | none |
| snap 2 | 2 | 131 | 12772 | 12772 | none |
| snap 3 | 3 | 131 | 12772 | 12772 | none |
| snap 4 | 4 | 131 | 12772 | 12772 | none |
| snap 5 | 5 | 131 | 12772 | 12772 | none |
- No
snapshotDiagnostics.warningon any of the 5 responses — including snap 5, where the warm quorum (4 warm samples) exists and judges the warm set fast. Pre-fix main warns here from snap 1 on (p95 >= 1500over all samples, cold spike = p95). - Displayed stats still cover every sample:
countgrows 1→5 andmaxMskeeps the 12,772 ms cold spike visible, so the diagnostics don't hide the startup cost — they just stop blaming the device for it.
Second confirmation on a different app (Safari, Wikipedia article, --raw): first capture 2,495 ms, warm 218 ms — same shape, silent on all 5 responses.
Leg 2 — sustained-slow attempt
Tried to produce a genuine sustained-slow run on this rig: heavy Safari pages (268-node raw trees, warm 267 ms), plus captures under 14× yes CPU load (warm still ~218 ms). Healthy warm captures sit ~10× under the 1,500 ms threshold, so the warning is not reachable on a healthy simulator — which is the point of the fix: only a warm quorum of ≥2 slow captures among ≥3 warm samples can fire it. The sustained-slow and merged-run warning branches are pinned by unit regressions in src/__tests__/snapshot-diagnostics.test.ts (warm-quorum fire/non-fire, single-warm-outlier silence, merged message speaking about warned runs with a non-vacuous fast-aggregate case).
Raw JSON responses for both legs retained locally (snap-1..5.json per leg) — can attach if wanted.
|
1 participant
Why
The session's first capture folds one-time startup (runner launch, helper install) into its duration sample, and nearest-rank p95 over a small sample set is its largest one or two values. Net effect: one 12s runner cold-start produced
on a session whose steady-state captures ran 520ms. The hint blames device load / stale daemon — inviting exactly the agent restart spirals the warning exists to prevent. Observed on every AppControlBench run (bench = fresh daemon + runner per run, so every run opens with a cold sample); bsky-30's "p95 9830ms over 24 captures" was two outliers over a fast run.
What
The warning now judges only warm captures (first sample excluded) and only once at least three exist. Displayed stats still cover every sample — the cold start stays visible as
maxMs. The replay/merge path (mergeSnapshotDiagnostics, order-less reconstructed samples) is unchanged.Regression tests: a 12.4s cold start over fast warm captures stays silent; chronically slow warm captures still warn (and the count excludes the cold sample); two samples alone never warn.