Repository navigation
refactor(apple): simplify warm runner ownership - #3357
Conversation
…lator A runner retained after `close` owns its Simulator destination: when the Simulator is shut down externally, the retained xcodebuild's destination machinery silently reboots it and the device stays powered on until that runner dies (#3321). While a runner is warm-retained, hold one idle TCP connection to the runner's listener as a push signal. When an established watch connection closes during retention, read the device's boot identity once (bounded recheck, never a sample): a boot newer than the retention window proves Xcode rebooted the destination, so stop the retained runner — which powers the rebooted device back off — and record the typed `runner_destination_lost` reason the next `open` reports as a response warning. A same-boot close re-arms on the restarted generation; a never-established connection retries on a bounded backoff before being called `runner_unreachable`. Healthy warm reuse is untouched: killing a retained runner whose device was not rebooted leaves the device on (negative control), and the idle-stop policy still stands behind it. Mechanism evidence and per-claim measurements: PR body, issue #3321.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ined runner outright (#3321) Live runs showed three gaps: attach retries reset the window start, so a reboot looked older than the window; a shut-down device keeps its old launchd_sim listed for seconds, so the boot witness alone read a crash; and a graceful stop let the dying xcodebuild reboot the device again. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… and a verdict (#3321) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…ment generations (#3321) Review round on #3357. Retention (idle stop, destination watch, retention fact) now enters through one retainRunnerForReuse and a failed ensureRunnerSession restores it, a re-attach that connects classifies the replacement generation, an unreadable device state counts as loss, the notice read waits for an in-flight decision on an open window, and a stale handler no longer deletes a newer watch. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ner and releases its lease (#3321) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…ouched it (#3321) A failed start that released a retained runner could re-arm retention over a runner a concurrent start was already using, because the restore keyed on session identity. Retention state changes now advance a per-device epoch and a restore is valid only at the epoch it released at. retainRunnerForReuse checks the session before it touches any retention state. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…eparately (#3321) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
I found one problem to fix in fbbf157, and the rest is minor. Checks are green: 21 checks, none failing. There are no conflicts. The new Not blocking, and you can take or leave these: in runner-destination-watch.ts, Could this be much smaller? If any loss of the retained runner's listener during retention (an established socket that closes, or a refused first attach) simply stopped the runner, the boot-identity classification, same-boot re-arm, replacement-generation retry loop, confirm/recheck/retry timers, their env vars and the On the open review threads, the cubic-dev-ai thread on runnerRetentionEpochs growth does not apply: the map grows by one integer per device, so it is bounded by device count, and reclaiming entries would reopen the restore race. You can resolve it. On evidence, the live 5/5 simulator runs appear only in the PR body and I saw no artifacts, and no live run is stated for the |
… that replaces rather than amends (#3321) Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Thanks for the review. State at Required change: done. Non-blocking, all taken:
Scope question, which needs a maintainer decision: keep the classification, or simplify to "any lost listener stops the runner"? What simplifying removes: boot-identity classification, same-boot re-arm, replacement-generation classification, confirm and recheck delays, and the Why I recommend keeping it, from the live runs:
If you prefer the smaller version: stop on any established close, keep the retry budget, drop the boot port and re-arm, and use one Evidence. The live runs are author-run, and I did not keep artifacts from the earlier ones. Here is a fresh run at Cycles 1 and 2 (6 s and 10 s gaps) end |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Follow-up to the probe thread: a state listing that times out or is unreadable ( |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Commit note for whoever reads history or squashes: |
…nd cause text (#3321) runner_unreachable meant both 'the runner stopped answering' and 'our own state probe could not be read'. The probe verdict is now runner_destination_unverified, and the open warning's cause text is a total map over the reasons. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
In 9c71e66 |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…3321) Comment-only; no behaviour change. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… classifying the loss (#3321) Drops the boot-identity classification, same-boot re-arm, replacement-generation retry, confirm/recheck timers, their env vars, the two host ports and the extra probe export. The watcher now stops the retained runner when its idle connection closes, or when the port stays refused past a bounded retry budget; the next open reports reason=runner_connection_lost. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Decision taken: the simple design, in Removed: the boot-identity classification, same-boot re-arm, replacement-generation retry, the confirm and recheck timers with their three env vars, the Behaviour now: a closed established connection, or a port still refused after 15 one-second retries, stops the runner. The notice is a single Size: the production diff is about 320 added lines (was about 690); the whole PR is 692 added lines including tests (was 1,342). Live at this head: 8 s and 12 s gaps between Accepted costs: a runner that crashes during retention and is restarted by Xcode also loses warm reuse, and a shutdown during the first seconds after |
There was a problem hiding this comment.
All reported issues were addressed across 29 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Thanks for the update. At 8afe959 most of the earlier review is fixed: the simulator export is back to main, the destination watch files are gone, the docs now scope cleanup to graceful daemon shutdown, and the keyed-lock and retention-timer fixes are in. CI is green, with 22 checks and none failing on 8afe959, and there are no conflicts. I did not run the tests, and the one open item is live evidence. The new route is a watch opened after the first exchange, and its socket close triggers Once that run is posted, this is ready to merge from my side. The cubic-dev-ai threads are all fixed at this head or resolved as benign, so the author can resolve them: #3357 (comment), #3357 (comment), #3357 (comment), #3357 (comment), #3357 (comment), #3357 (comment), #3357 (comment), #3357 (comment) (a late canceled health result must not invalidate a retained generation, and the |
Summary
Give warm Apple runners one lifecycle owner. A ready, idle iOS-family Simulator runner owns its listener watch and retention timer; successful reuse ends retention, and listener loss or expiry stops that generation.
Close retires prewarm recovery and stops unfinished, busy, or disconnected runners. Graceful daemon exit stops idle retained runners; active handoff remains supported. Shared locks track their held lifetime and reentrant work, so deferred callbacks cannot overlap another owner. Clamp idle durations to Node's timer limit.
Remove the separate retention registry, epochs, rollback, watcher attach retries, and next-open notice plumbing. 19 files; 186 fewer production TypeScript lines than the incoming PR head. Scope includes #3359 and the lock primitive its callbacks depend on.
Closes #3321. Closes #3359.
Validation
Final head
8afe9598ef; runtime checks at06362cacdd(the last commit only wraps the ADR):pnpm check:quickandpnpm check:affected --runpassed: 4,839 tests across 575 files, plus layering, Fallow, build, and guidance checks.pnpm check:xctest-selectionand the iOS XCTest build passed. Behavioral regressions and mutations demonstrate the overflow, reentrant microtask, cold-close, cancellation, watch-identity, and actual idle-disposal boundaries.--clean; then 60 seconds Shutdown with no lease. Earlier cold-close/shutdown runs at 1–3s gaps also stayed off for 60s.