fix(timeouts): prove request envelopes cover their worst-case work - #2916
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 8ff29c0. The ledger test drives fold through the real client, daemon, and fake provider route and sums the recorded step budgets, and the prepare test ties the envelope to the handler budget.. The only production change widens the fold (240 s to 255 s) and prepare envelopes, so it can only reduce client timeouts. Smoke Tests was still queued or running at review time, so there is nothing to attribute yet. I did not run the tests or the #2879 mutation checks myself. Not blocking: it would help to record the #2879 mutation-check results in the Validation section, and to use "Refs #2879" instead of "Closes #2879", because the issue stays open until the Step 2 issues are filed. The branch now conflicts with main, so it needs a rebase before the label can go on. |
FOLD_REQUEST_TIMEOUT_MS was a literal whose basis lived only in a comment, and that comment kept drifting from the route: it first cited one 30s helper build and four 20s hinge reads, missing two 5s display-inventory reads and the HID-dispatch guard/grace, then #2858 hand-summed the route again for its own preparation-phase change and landed a fresh hand-summed figure with the same failure mode. Add a ledger test that drives fold through the public client and daemon against a fake Apple tool provider recording each call's timeoutMs, including the fold-helper cache's toolchain probes and build. A calibration run learns the route's hinge-settle-attempt count from an oscillating angle that never settles; a measured run uses the same oscillation but lands the last allowed read on target, so it succeeds after making exactly as many reads. Summing every call's timeout (minus 1ms, plus any kill grace) gives the route's worst case that still succeeds, asserted against the resolved command envelope with the usual 30s daemon-result margin. The test imports no platform-apple step figure other than MAX_FOLD_DURATION_MS, so it keeps proving the bound however the route's steps change next. Widen FOLD_REQUEST_TIMEOUT_MS to 255_000 (the smallest 5s multiple at least ledger + margin) and point both the constant's comment and the descriptor-timeout-policy pinning test at the ledger test instead of a hand-summed figure.
…udget readPrepareIosRunnerTimeoutMs's fallback and the client envelope both resolved to PREPARE_REQUEST_TIMEOUT_MS (240_000), so with no --timeout the daemon-side runner budget and the client envelope were identical: a slow cold runner build ended in a client-side timeout and daemon reset instead of the daemon's own typed runner_phase_budget result. Every other bounded command keeps a 30s margin between its daemon-side budget and its envelope; prepare's explicit-timeout case already widened past its own base by that same margin (widenToUserBudget), only the default case had none. Split the daemon-side runner budget out as PREPARE_STARTUP_BUDGET_MS (240_000, unchanged) and derive PREPARE_REQUEST_TIMEOUT_MS from it plus the margin (270_000). The handler falls back to the startup budget, not the now-wider request timeout. Add a handler-level test that calls handlePrepareCommand directly with a fake runner binding recording the timeoutMs it receives, then checks that value plus the margin against the same request's resolved client envelope for the no-flag, --timeout above default, and --timeout below default cases - the rule the fix proves, not just the constants it compiles to.
The fold worst-case ledger fake defaulted an unset timeoutMs to 0, so a provider call reaching it with no bound cost 0 virtual ms instead of exposing the true unbounded worst case. Its runCommand handler also skipped `open -a Simulator` before recording, dropping any such call from the ledger with no error. Build the ledger on the shared recording provider and assert every call carries a finite, positive timeoutMs, so an unbounded call fails the test instead of passing silently.
The ledger test runs two cold folds through a fresh daemon each. After a platform-apple source change the first import is untransformed, and the test ran past vitest's 5 s default, which hid the ledger printout behind a timeout. Match the provider-scenario precedent of an explicit budget.
8ff29c0 to
6c7f602
Compare
|
Rebased onto main at |
6c7f602 to
c581589
Compare
|
Addressed the review at 8ff29c0. New head c581589. Rebase (main removed CHANGELOG.md in #2922): main advanced twice more since the last push #2879 mutation-check results (each reverted after its run):
Each failure prints the ledger. Kept the settle-attempts mutation's test on a 20s budget (added Wording: already Gate on c581589: Evidence (targeted vitest, this head): Envelope-mutation control (
|
|
The code looks correct at 6c7f602, and this round fixes the conflict from the earlier review at 8ff29c0. The Validation section still names the tested SHA as 8ff29c0, not 6c7f602; the logical patch only differs by the 20 s test budget, but can you update it to point at the reviewed commit? Can you record the #2879 mutation-check results against 6c7f602 in the Validation section: the three fold-ledger mutations (hinge timeout at 30 s, settle attempts at 6, one extra 30 s runXcrun call) and the session-prepare fallback revert? I read the code and believe the test goes red on the old 240_000 ms envelope, but I did not run the ledger test myself, so I can't confirm the measured virtualElapsedMs or that it exceeds the 210 s threshold that would fail the old envelope. The two Smoke Tests jobs are still running and I have no failure excerpt from them. This PR widens the client envelope for prepare (240 s to 270 s) and fold (240 s to 255 s), and iOS smoke may run prepare ios-runner; since the change only widens those envelopes, it should not make a request time out sooner. The rest of the diff (runner Swift, ADRs, Android hierarchy) comes from the rebase onto upstream, not from this change. The next step is to add the #2879 mutation-check results to the Validation section against 6c7f602 and wait for Smoke Tests to finish. The branch has since been rebased to c581589; its four commits are identical to the ones reviewed here, so this review covers that head too. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
Refs #2879 (Step 1). Makes the client request-envelope-covers-worst-case invariant executable
for
foldandprepare, instead of living only in comments and hand-summed literals.fold: the ledger test drives fold through the real public-client → daemon → fakeApple-tool-provider route, summing per-step
timeoutMsfrom real recorded calls (not ahand-sum); every recorded call must carry a finite, positive
timeoutMsor the fake throws.FOLD_REQUEST_TIMEOUT_MSmoves 210 s → 255 s (post-fix(ios): drop -Werror from runtime clang builds and cache the fold helper build #2858 helper-build cost, matching theissue's prediction).
prepare: a handler-level test tiessession-prepare.ts's fallback runner budget to theclient envelope with the standard margin.
PREPARE_REQUEST_TIMEOUT_MSis now derived from anunchanged
PREPARE_STARTUP_BUDGET_MS(240 s) plus the 30 s margin (270 s), not a separatehand-set literal.
batch/replay,longpress,install/reinstall/install_source) is out of scope;those issues are not yet filed (read-only
ghaccess this stage).Touched files: 5 (
timeout-policy.ts,session-prepare.ts, three test files).Validation
Tested SHA:
c5815897530bd4606ae0622799bc2e454d0092b6(rebased ontomain0.21.14,622435ecc5).pnpm check:affected --run: 478/478 files, 3538/3538 tests, exit 0.pnpm check:production-exports: exit 0; 68 pre-existing unused-export findings, none in thisdiff's files.
pnpm check:layering: 246/246 green (R11, R13 included).#2879mutation checks, each reverted after its run:IOS_HINGE_ANGLE_TIMEOUT_MS20s → 30sIOS_FOLD_POSE_SETTLE_ATTEMPTS4 → 6simctlcall in fold posePREPARE_REQUEST_TIMEOUT_MSLive validation: not applicable. Both changes are client-side envelope-constant arithmetic; no
step's timeout, retry, or order in the real fold/prepare route changed, so there is no on-device
timing to observe beyond what the ledger and handler tests already assert.
Remaining risks: Step 2 issues still need filing before #2879 can close.