fix(driver): price reclaim by the strategy it actually runs - #62
Merged
Conversation
`estimate("reclaim", spec)` returned 1s for iOS and 2s for Android against a
measured ~34s simulator erase, and the two consumers that read it were both
misled in the optimistic direction: a waiting requester's ETA, and the
stalled-transition threshold, which at 1s could never clear its own 60s floor.
`estimate` now takes a `DriverEstimate` rather than a bare operation name, so
`reclaim` carries the clean level `reclaimStrategy` already selects on:
- iOS erases at either level, priced at the measured 34s.
- Android splits: a `snapshot` restore is a load plus a readiness wait (6s),
while a `wipe` only shuts the emulator down and defers the wipe itself to the
next `makeReady` (3s) — cheaper at reclaim time, not dearer.
Two consumers the number was supposed to reach were never wired up, so the
estimate alone would not have shown anywhere:
- `estimatedReclaimMs` was hardcoded to 0 on every grant. It now carries what
releasing the device will cost, including for a warm grant that does no work.
- The `reclaiming` progress stage was documented in CLI.md and typed in the
contract, but nothing emitted it. A waiter queued behind a matching device's
reclaim now gets it, so `eta_seconds` reflects the erase it is waiting on.
`Doctor` takes the slower clean level, since a `reclaiming` record does not say
which one started it and a too-tight threshold invents false stalls.
Also raises the iOS cold-boot estimate from 30s to 60s: a cold boot measures
30–60s depending on the machine, and the quoted ETA should not be the best case.
The cost is a wider `provisioning` stall threshold, which is the cheaper error.
Closes #56
…ver call Measured on an M3 Pro against Pixel 8 / API 35: a `snapshot` reclaim runs 2.4-5.1s (n=9, median 4.6s; 3.7-5.7s with three concurrent), so the 6s estimate stands. A `wipe` reclaim runs 22.8-42.8s (n=5, median 31.7s) — ten times the 3s this guessed. The guess was wrong about what the number measures, not just about its value. `reclaim` does only shut the emulator down and defer the wipe to the next `makeReady`, as its comment said. But the device does not leave `reclaiming` there: `WarmPoolCoordinator#disposition` re-readies a device the pool wants to keep warm before committing the transition, so the wipe boot and the baseline re-capture fall inside the same window — and that window is what both consumers of this estimate actually observe. A device the pool does not keep warm settles in seconds; the slow branch is the one to quote, since under-quoting makes every kept-warm reclaim look stalled while over-quoting only delays a finding. Consequence for the stalled-transition threshold: Android's `reclaiming` threshold is now 32s * 3 = 96s and clears the 60s floor, where the guessed numbers left it landing on the floor exactly as before this work. Boot, provision, and the iOS numbers are unchanged; the same run confirms cold boot at 22.5-29.5s against a 31s estimate and provision at 0.9-1.5s against 1s.
The stage was the one new user-facing output here with no coverage past the coordinator: the unit test asserts the queue is notified, but nothing exercised the daemon push or the CLI's stderr line, and the path had never been observed against a real daemon. A hardware run tried and missed it -- with three AVDs in the pool and capacity to spare, the second request booted a shutdown device instead of queueing, so it reported `booting` and the branch stayed unproven. Capacity is what makes the case, not timing, so this pins maxRunning to 1 and gives the fake driver a reclaim latency: the waiter has nowhere else to go and reports the ETA. Verified to fail when the notification is removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #56.
What was wrong
estimate("reclaim", spec)returned 1s for iOS and 2s for Android against a measured ~34s simulator erase. Both consumers were misled in the optimistic direction — a waiting requester's ETA, and the stalled-transition threshold, which at 1s could never clear its own 60s floor, so the "derived from the driver's own estimates" property #37/#55 asked for was not in effect for reclaim.Two of the three acceptance criteria turned out to describe consumers that were never wired up, so fixing the number alone would not have shown anywhere:
estimatedReclaimMswas hardcoded to0in both timing helpers —estimated_reclaim_mshas always been a literal zero in the grant payload.reclaimingprogress stage is documented indocs/CLI.mdand typed in the contract, but nothing in production emitted it. Only a test ever pushed one.The change
estimatenow takes aDriverEstimaterather than a bare operation name, soreclaimcarries the clean levelreclaimStrategyalready selects on. All figures below are measured, not derived — see the measurement section.erasesnapshotwipeWiring the two dead consumers:
estimatedReclaimMscarries what releasing the device will cost — the one figure in a grant's timing that describes work still ahead of the holder rather than work already done. Set for provision, boot-shutdown, and (new) warm grants, which previously reported all zeros.reclaimingstage is emitted when a waiter is queued and a spec-matching device is currently being reclaimed. Purely informational: the plan is untouched and the device is granted through the normalreadypath when its reclaim commits.Doctortakes the slower of the two clean levels. Areclaimingrecord does not say which one started it, and the errors are not symmetric — too tight invents a false stall, too loose only delays a real finding. With the measured numbers this now matters in practice: Android'sreclaimingthreshold is 32s × 3 = 96s and clears the 60s floor, where the earlier guesses left it landing on the floor exactly as before this work. iOS clears it at 102s.sameSpecmoved todomain.ts; it was already duplicated in the planner and the warm-pool coordinator, and this needed a third copy.Measurement
Run on an M3 Pro / 38 GB, AC power, Android emulator 36.5.11.0, Pixel 8 / API 35 (
android-35/default/arm64-v8a), instrumented through simlock's owndevice.reclaimed/device.readyevents rather than by timing SDK calls externally.A wipe reclaim is ~10× what this PR first guessed, and the guess was wrong about what the number measures.
reclaimitself does only shut the emulator down and defer the wipe to the nextmakeReady— that part of the original comment was accurate. What it missed is that the device does not leavereclaimingthere:WarmPoolCoordinator#dispositionre-readies a device the pool wants to keep warm before committing the transition, so the wipe boot and the baseline re-capture fall inside the same window. That window — not the driver call — is what both consumers of this estimate observe: the ageDoctorcompares against, and how long a waiting requester actually waits. The estimate now models the window.It is bimodal: a device the pool does not keep warm settles in seconds. The slow branch is the one quoted, because under-quoting makes every kept-warm reclaim look stalled while over-quoting only delays a finding.
Other numbers from the same run, unchanged as a result:
device.purge-failedordevice.quarantinedevents, no failed boots.SNAPSHOT_BOOT_ESTIMATE_MS(4s) remains unvalidated — a snapshot boot needs the device shut down first, anddaemon stopdeliberately leaves emulators running (the warm pool survives a daemon restart;idle.shutdownAfterMsis what shuts them). It only feeds a diagnostic log threshold, so it is not load-bearing here.iOS cold boot: 30s → 60s
Separate from #56, and a judgement call worth flagging. A cold
simctl boottobootstatusmeasures 30–60s depending on the machine, and the quoted ETA should not be the best case. The cost is thatDoctor'sprovisioningstall threshold widens with it —(0.5 + 60) × 3 ≈ 181s, up from ~91s — so a genuinely hung boot goes unreported for roughly twice as long. That is the cheaper of the two errors, but it is a real trade-off rather than free safety; happy to revert this half if you'd rather keep the tighter threshold and quote the median instead.Tests
Doctor: afull-level reclaim past thestandardthreshold is not a stall; past thefullone it is.LeaseEngine: a waiter queued behind a reclaim getsqueuedthenreclaimingwith the driver's ETA; a waiter queued behind a leased device gets onlyqueued.LeaseEngine: grant timing carriesestimatedReclaimMs, including for a warm grant that does no work.reclaimingstage all the way out to the CLI's stderr — coordinator, daemon push, and renderedeta_seconds. This was the one new user-facing output with no coverage past the coordinator, and an attempt to observe it on real hardware missed the branch (three AVDs in the pool meant the second request booted a spare instead of queueing). Verified to fail when the notification is removed.pnpm checkgreen (typecheck, typecheck:e2e, lint, format:check, 642 unit, 33 e2e).fallowreports no new findings on the changed files.