Repository navigation
perf(ios-runner): don't restart the settle window when re-binding an already-foreground target - #3532
Conversation
…he same foreground target A skipped activation (target already foreground) re-bound the target and restarted the 250 ms first-interaction window, charging the first tap after every relaunch for time the targetReset round trip had already spent. Only an actual activation, a different bundle, or a different process restarts the window; a same-target reacquisition keeps an existing deadline and stamps only when none exists.
…pped for any reason but an announced relaunch A dropped binding with a leftover deadline must mean the announced external relaunch, so a skipped activation can keep that deadline safely. Error invalidations and the host-app rebind now clear it and the next activation stamps a fresh window as before.
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Code looks good at fb1e51a, but the PR is not ready to merge yet: the live A/B numbers appear only in the PR body, and no runner.log excerpt shows ACTIVATE_SKIPPED reason=already_foreground followed by no restamp. Please attach that excerpt from the tested commit. Both Smoke Tests jobs are still queued, not failed. They drive iOS sim open and interaction flows through prepareActivatedTarget and activateTarget, which this PR changes, so their result counts once they finish. There are no conflicts. Could the runner record where the settle window came from instead of inferring it? For example, resetTargetAfterExternalRelaunch could stamp the window with an origin, like firstInteractionWindow = (readyUptime, origin: .announcedRelaunch | .boundTarget(bundleId, pid)). activateTarget would then keep a pending window only when no activation ran and the origin is the announced relaunch or the same binding. The nil writes in invalidateCachedTarget and ensureRunnerHostAppActive, and reacquiresBoundTarget, would become unnecessary, because no surviving deadline would need to mean anything. A smaller option is to keep the window on a skipped activation only when previousBundleId == nil, and drop the pid comparison. All of this is runner-local, and the targetReset wire contract can stay as it is. Not blocking, and you can take or leave these: reacquiresBoundTarget is a second identity rule beside shouldRefreshCachedTarget (Lifecycle.swift:267-272), and they disagree when one pid is missing (cached nil, observed 42). Its same-bundle, non-nil branch is reachable only through a foreground-state race, since every activateTarget caller enters with a dropped or different bundle or a non-foreground state. The new firstInteractionReadyUptime = nil in ensureRunnerHostAppActive never has an effect, because both callers (CommandDispatch:611, 634) run after invalidateCachedTarget("missing_app_bundle") at :603 already clears the deadline. Either express the rule through shouldRefreshCachedTarget's semantics or key it on the dropped binding only, and drop the nil write or make it the only owner. I did not run the runner unit tests or the claimed mutations. I read the pre-change code to see that the keep-elapsed test is red on base. I also did not check that the daemon sends targetReset only after the relaunched process is foreground, and the reset deadline is the right anchor only if it does (notifyAppleRunnerRelaunch in packages/platform-apple/src/lifecycle.ts). The test-name thread still stands: target_terminated comes from a successful terminate (CommandDispatch.swift:476-478), so "ForError" in LifecycleCacheTests.swift:122 is misleading. Two other cubic-dev-ai threads do not apply, so please resolve them. The nil==nil pid thread matches the existing shouldRefreshCachedTarget rule, and the same-bundle branch is only reachable through a foreground race. The host-app activation thread does not apply because host-app activation never stamped a window, and invalidateCachedTarget at :603 already clears the deadline before both callers run. Before merge, Smoke Tests must finish green on fb1e51a, and the design question above needs an answer. |
|
@thymikee implemented the origin-tagged window in
Live runner.log, second tap after |
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
…s dropped again A snapshot or XCTest failure that drops the binding after a targetReset left the relaunch-origin window in place, so a later skipped activation could reuse it. Invalidation now re-attributes a surviving window to an activation, so only the binding the reset dropped can inherit it.
|
The code in 518f832 looks good, and it fixes the open P2 from the earlier review at fb1e51a (#3532 (comment)). On evidence: I read the new test against the pre-delta code and it would fail there, but I did not run the runner LifecycleCache tests or the claimed mutation of removing :221. The tests call Of the cubic-dev-ai threads, these no longer apply, so please resolve them. #3532 (comment) is fixed at head, with Nothing else is needed before merge, so this is ready for a maintainer to merge. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
After
open --relaunch, the first tap waited an extra 250 ms. The daemon'stargetResetstarts the post-activation settle window, then the tap re-binds the target throughactivateTarget(reason: bundle_changed), which restarted the window even when activation was skipped (ACTIVATE_SKIPPED reason=already_foreground).The runner now records where the window came from instead of inferring it:
FirstInteractionWindow { readyUptime, origin }, with origin.activationor.announcedRelaunch(bundleId:).targetResetstamps.announcedRelaunchwith the binding it dropped (nil if none, which matches any bundle; the wire carries no bundle). Every other stamp is.activation.activateTargetkeeps the existing window only when no activation ran and the origin is an announced relaunch of this bundle; otherwise it stamps a fresh window.reacquiresBoundTarget, the window-mode enum, and the nil writes ininvalidateCachedTargetandensureRunnerHostAppActive. A stale.activationwindow is harmless because it restamps.The invariant stays "at least 250 ms between activating a target and its first interaction" (#2395).
Validation
Tested commit
3aabddd92c.15 LifecycleCache runner unit tests: Executed 15, 0 failures. Mutations caught: keep the window regardless of origin (3 failures); ignore whether an activation ran (2 failures).
pnpm check:affected --run,pnpm lint, andcheck:xctest-selectionpass.Live runner.log (see PR comment):
ACTIVATE_SKIPPEDis followed by the text-input probe with no 250 ms gap.A/B measured at
fb1e51accd; the redesign keeps the same relaunch-to-skipped-activation behavior. 5 paired rounds on one iPhone 17 Pro / iOS 26.2 sim,open com.apple.Preferences --relaunchthenclick 'label="General"':main)Every paired round improved (−156 to −252 ms).