Repository navigation
fix(android): keep close --shutdown's IME restore out of the settings flush window - #3331
Conversation
… flush window AOSP SettingsProvider persists a setting change asynchronously (AOSP SettingsState.java caps the delayed XML flush at 2s). `ime set` answers from memory, so the close-time restore confirmed its read-back and then `close --shutdown` ran `adb emu kill` immediately; the emulator died before settings_secure.xml was rewritten and restarted with the test helper IME as the default keyboard. The restore now waits out a bounded flush settle after a confirmed restore, only when the close will kill that emulator, and keeps the pending recovery marker written through the window. The settle stops early with the request's abort signal, since a cancelled close never reaches the kill.
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 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
CI note: the only red check was the iOS workflow's |
An aborted close resolved the settle immediately, dropped the owned flag, and cleared the pending recovery marker as if the flush window had been waited out. The next close of that emulator then took the no-record fast path and could kill inside the still-open window — the #3318 failure reached through the ordinary abort-and-retry shape. The settle deadline is now process-owned state keyed like the recovery lock: an abort keeps it, and the next shutdown-bound close waits out the remaining window before returning to the kill. A marker clears only when the window it fences is fully covered. An aborted settle is never a completed settle.
|
P1 + P3 from the cubic review addressed in Defect: an aborted close resolved the settle early but still cleared the pending marker and left the owned flag dropped, so the next close of the same emulator took the Fix at the owning state: the flush-settle deadline became process-owned state ( New tests ( Validation on |
There was a problem hiding this comment.
1 issue found 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
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-android/src/ime-restore.ts">
<violation number="1" location="packages/platform-android/src/ime-restore.ts:100">
P1: This branch turns the former no-ownership early return into a recovery-complete `no-record`, so the following code clears the durable marker without inspecting the device. If startup retained a marker for an offline emulator, any later ordinary close can erase it while the helper IME remains active; preserve it unless this call observes the device clean or consumes a deadline from a confirmed restore.</violation>
</file>
The settle refactor turned the never-activated early return into an assignment falling through to the shared marker clear, so a close on a device this process never activated began erasing the durable pending marker having inspected nothing - severing the offline-retained orphan from startup recovery and the doctor check. The root cause was an overloaded reason: no-record meant both "device inspected, no rebind record" (complete; clear is right) and "never activated here" (nothing inspected; clear is wrong). Split them. A not-activated-here close clears only when it consumed a deadline a confirmed restore opened. Tests pin both directions: a retained marker survives a nothing-inspected close, and the second-close retry still clears after consuming the window.
|
Second P1 (marker-erase regression in the settle refactor) fixed in Acknowledged: the trace is right. Converting the never-activated early return into an assignment routed it through the shared marker clear, and Fix at the overloaded reason, not another boolean: Both directions pinned: 'devices this process never activated are left alone, marker and all' now writes the marker first and asserts it SURVIVES (the old test could not observe a clear — that is why the regression shipped); the mid-settle suite pins that the retry-after-abort still consumes its window and clears. Mutation-checked: deleting the ownership check fails 4 tests; widening the clear-earned condition back fails 2. Validation on |
|
Audit pointer for the second P1 (marker-erase regression): my thread reply is discussion_r4224358513 — GitHub collapses resolved threads, so it is easy to miss. Root cause in one paragraph: the flush-settle refactor turned the never-activated early return into an assignment falling through to the shared marker clear, and the reason type overloaded |
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
A retry close that both consumed a leftover deadline and confirmed a fresh restore slept twice back to back, holding the IME recovery lock for two flush windows. AOSP SettingsState.doWriteState() rewrites the whole settings file from the in-memory map, so one flush after the last write also persists every older pending change: the windows are a union, not a sum. The settle is now a single max(existing, write + cap) wait with one outcome for the marker-clear decision.
|
P3 (doubled flush window, discussion_r4224387869) fixed in The live-emulator evidence the body owed is now recorded: 3/3 cycles of |
|
Reply-in-thread audit for anyone reconciling review threads against this PR (all four cubic findings have an author reply; the threads were resolved by cubic's own bot,
Note for tooling: these are review-comment replies (nested in resolved threads); GitHub collapses resolved threads in the default view, and |
|
This PR is ready on the code side, but one piece of evidence is missing. At 3bbda23 all 19 checks pass, and the earlier iOS smoke failure was on a different lane and was re-run. There are no conflicts. Nothing else blocks it. The live result is 3/3 LatinIME after restart on an arm64 API 35 emulator, and it is author-reported. No run of the base commit on that same AVD is recorded. The issue reproduced on API 32/37 x86_64, so it is not yet shown that the base fails on this emulator. Please add one base-commit cycle on the same AVD: Not blocking, and you can take or leave these: Is there a smaller fix than the timed wait? I looked at two. Waiting for a durable flush signal does not work, because the provider has no shell flush and reading the The earlier bot threads are fixed at this head and can be resolved: the aborted-settle marker (#3331 (comment)), the abort-aware sleep mock test (#3331 (comment)), the 'not-activated-here' recovery reason (#3331 (comment)), and the coalesced flush window (#3331 (comment)). |
The kill that must miss the flush window can arrive from another session: an ordinary close confirmed the restore and dropped the owned flag, and a later close --shutdown on the same device then inspected nothing and killed at once. Registration now belongs to the confirmed restore itself, on every close; only kill-bound closes wait it out. ime-restore also stops re-deriving the kill-bound predicate from device kind, which lifecycle.ts already owns; it gates registration to emulators because only emulators can be killed by the runtime.
|
Requested base-control run: base does NOT reproduce on this AVD — recording that as a real finding, per the review ask. Base =
Head ( What this does and does not prove: the arm64 API 35 run cannot discriminate the fix — the base passes there too, so the earlier "3/3 LatinIME on head" was, as the reviewer suspected, evidence of no-new-breakage on that image, not of the fix. The base's sub-second close confirms the mechanism is present on head's terms (no settle at all on base), but this emulator's SettingsProvider evidently flushes faster than the reporter's. The reproduction's platform scope stands as reported: API 32/37 x86_64 (and per #3318, both google_apis and playstore images). The 2 s-cap risk in the body is unchanged and still the honest open item. I don't have an API 32 or 37 x86_64 AVD provisioned here; x86_64 images under this arm64 host run without HW acceleration, making boot cycles impractical as CI — happy to run one if someone provisions it or has a spare AVD serial. Falsification control on the hazard itself, same AVD, base dist: Both non-blocking reviewer points addressed in
Validation on |
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
…ry emulator kill on it Move flush-window registration inside restoreAndroidTestImeFor, the only function that confirms the ime set landed: no caller — close, cross-session, or daemon-startup orphan recovery — can restore without registering, and the startup scan can no longer leave an unguarded window (issue review P1). Enforce the window where the kill happens instead of at each close caller: awaitTestImeFlushWindow in the shutdown runtime holds every adb emu kill (close --shutdown, the standalone shutdown command, and any future caller) past the newest registered restore, keyed by serial because the window is a property of the device's SettingsProvider, not of any host state dir. Unify the recovery-marker clear on one decider shared by the close wrapper and the startup scan, and switch the settle evidence to monotonic last-restore timestamps so an aborted wait can never consume evidence the next kill-bound path still needs. Co-Authored-By: Apex <noreply@callstack.io>
Validation — head
|
iOS smoke red at
|
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
The window guard compared a wall-clock timestamp to a wall-clock read, so a forward host-clock step (NTP correction, VM resume) between registration and the kill could make the remaining wait negative and release the kill inside the provider flush window. Register and derive the remaining wait from performance.now(), the process monotonic clock the repo already uses for transport deadlines; the map is process-owned and never persists, so an absolute epoch time bought nothing. Pins the kill-site wait magnitude at the full registered budget and adds a forward wall-clock-jump regression test. Co-Authored-By: Apex <noreply@callstack.io>
Round seven (cubic) fixed — head
|
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
…k name The coordinator's confirmation of cubic's P1 adds the shape the code should carry: the map is now testImeLastRestoreAtPerfMs, so 'AtMs' no longer misreads as a wall-clock epoch, and the comment states the two-clock mechanism (sleep is timed on libuv's monotonic loop clock; a wall-clock-derived remaining time mixes clocks by construction — the skew stable-capture.ts documents). One monotonic source for registration and expiry, no clamp hiding anything. P3 extension: the abort test now pins the NUMBER the design rests on — the mark is seeded 1000 ms old so the first wait must derive ~1500, and after the aborted kill the retry re-derives the same remainder from the surviving mark. A constant sleep, or an abort that consumed the evidence, fails on the number. Co-Authored-By: Apex <noreply@callstack.io>
…window awaitTestImeFlushWindow read the mark once, slept, and reported 'covered' even when a newer restore had registered while it slept — releasing the kill inside the extension's window, which is #3318 again. It now re-reads the mark after every sleep and loops until the newest deadline passes, consuming only the mark each round waited for. The kill tests observe the middle of the wait with a deferred sleep: adb must not run while the wait is pending, and a restore landing mid-wait must produce a second, extension-sized sleep before the kill. Co-Authored-By: Apex <noreply@callstack.io>
Validation — head
|
The window is process-monotonic and in-memory by construction, so it cannot survive a daemon restart; every restore shares that boundary, not just startup recovery. Recording why the restart-crossing fix was not taken here: a cross-process deadline must be wall-clock or boot-time based, reopening the clock-skew class the monotonic clock exists to exclude, and a state-dir file host through the shutdown-runtime contract is a boundary change for a human reviewer to decide. Co-Authored-By: Apex <noreply@callstack.io>
Validation — head
|
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
…3346 The coordinator endorsed closing the cross-restart durability finding as a filed follow-up rather than expanding this PR's scope. The map comment now names the issue so the boundary and its owning work sit beside each other. Co-Authored-By: Apex <noreply@callstack.io>
Validation — head
|
The flush mark registers only after the shell call returns and the readback confirms, but the provider's window opens when the device accepts the write. A kill-bound wait reading the map during that gap saw 'idle' and fired adb mid-write. The registrar now opens a pending entry before issuing the write and closes it in a finally that lands after registration, and awaitTestImeFlushWindow drains in-flight writes at the top of every round before consulting marks — including a write that opens during its sleep. Also derives the flush-window tests' seeded elapsed values and sleep bounds from the now-exported SETTINGS_PROVIDER_FLUSH_SETTLE_MS so a changed window shifts their meaning instead of turning them red for a copied-number reason. Co-Authored-By: Apex <noreply@callstack.io>
…he race test Drop a useless spread; split awaitTestImeFlushWindow into drain, round, and consume helpers under the complexity threshold without changing its rounds' semantics. The in-flight test now drains a real event-loop turn before asserting, so it fails when the drain guard is removed (verified) instead of racing the kill's first microtask. Co-Authored-By: Apex <noreply@callstack.io>
The two remaining 500 ms margins become 0.8 * the window so every flush-window bound in both suites moves with the constant. Co-Authored-By: Apex <noreply@callstack.io>
Validation — heads
|
|
Coordinator round twelve processed against the current head: both findings were already fixed and replied ( |
|
Coordinator note processed; state checked against the live board at 2026-10-09T01:02:58Z. Trailer correction accepted and adopted: future board claims will carry the checked timestamp rather than an unsourced "all resolved" (the 00:31:41Z pair was live when my 00:34:51Z comment went out — whether from check staleness or my own lag, a timestamp makes the claim auditable either way). iOS Smoke red at The two cubic threads from 00:31:41Z: both closed and replied before this note (heads
Board at 01:02:58Z: threads 0 open; CI 0 fail, 16 pass, 2 pending (the Smoke re-runs), 1 skipping; mergeable at |
… the outcome The pending-write mechanism treats ISSUE as the moment the provider's window may start, but registration sat on the confirmed path only: a set-failed readback mismatch closed the pending entry with no mark, letting a drained kill-bound waiter read 'idle' and fire adb inside the window a half-accepted write may have opened. The mark now registers in the registrar's finally, beside the pending-close, for every issued emulator restore; registerTestImeRestore's max keeps that purely lengthening. The set-failed branch stays purely about recovery evidence: retained record and marker still cover retry. Co-Authored-By: Apex <noreply@callstack.io>
Validation — head
|
thymikee
added
the
ready-for-human
|
The code in b79910d now fixes what the earlier review of 3bbda23 left open: Not blocking: the comments in CI is green, with 18 of 18 checks passing at b79910d, and the author reports lint and fallow green at this head. I know of no conflicts. Of the inline threads resolved since the earlier review, one still applies: the in-memory-only window thread (#3331 (comment)). I accept it as non-blocking, because #3346 tracks it. The fixes for the others landed at this head: #3331 (comment) (startup recovery now registers correctly), #3331 (comment) (cross-session test order is fixed), #3331 (comment) (registration and expiry use one clock), #3331 (comment) (test floor no longer derived from the constant), #3331 (comment) (AbortError reason assertion is fixed), #3331 (comment) (the round loop extends on newer marks, covered at runtime.test.ts:172), #3331 (comment) (deferred sleep and no mid-wait Evidence has limits. No reproduction separates base from head, because the base also restores LatinIME on the author's arm64 API 35 AVD, and the reported API 32 and 37 x86_64 images were not run. What stands in for it is the author's report of a SIGKILL-within-1s hazard control on the same image, plus head cycles with a 2.85 s close and LatinIME after restart. I could not confirm that 2.5 s covers vendor images that override Nothing more is needed in code before merge. A maintainer should either accept the hazard-control evidence or ask for a base-versus-head |
…in retention in the set-failed test AGENTS.md keeps control-flow narration and review history out of implementation comments; each site now carries one invariant sentence. The set-failed flush-wait test seeds the durable marker and asserts record retention itself instead of crediting other tests for what it does not prove. Co-Authored-By: Apex <noreply@callstack.io>
|
Both cleanups done at Review history in code: round numbers, reviewer tags, and the "Invariants the review requires" framing are gone from The overclaiming test: fixed by seeding rather than softening, since the sentence described a real invariant worth pinning here. Evidence limits — what ran where, in one place: Run on the arm64 API 35 AVD (
Not run: any base-versus-head The |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…earn marker clears The mark map carried a timestamp alone, so the marker-clear rule could only infer that a covered window proved recovery. Registering every issued write from the finally made that inference unsound: a set-failed restore's mark, covered by a later not-activated-here kill-bound close, let that close clear the durable recovery marker while the helper was still active. Marks now carry the write's own provenance (atPerfMs + confirmed), the flush wait reports covered-issued vs covered-confirmed, and the clear rule consumes the fact. Registration stays in the finally for every issued emulator write: the hold is owed regardless; only confirmation retires retry evidence.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Verification record for
|
…sleep seedRestoreMark lived byte-identical in two test files, so the last mark-shape change had to land twice; it now lives in the IME fixtures module beside the fakes it serves, one edit per future shape change. The marker-retention test's sleep assertion promised the FULL flush hold but accepted any positive value; it now carries the file's own nearly-full-window bound, derived from the window constant.
|
Verification record for
|
|
This PR is ready. I re-reviewed it at b36626a. The code now fixes what the earlier review at b79910d (#3331 (comment)) raised, and I have no remaining code findings. CI is green, with 19 of 19 checks passing at b36626a, and there are no conflicts. Not blocking, and you can take or leave these: Of the open threads, these are fixed at this commit and can be resolved: #3331 (comment) (covered-confirmed required at On evidence, I did not run the unit tests or |
|
1 participant
Summary
close --shutdownleft an Android emulator restarted with the agent-device test IME as its default keyboard (#3318). The restore itself succeeded —default_input_methodread back correctly while the emulator was still running — but AOSP's SettingsProvider persists setting changes asynchronously (delayed XML flush, capped at 2 s inSettingsState.java), and the close finalizer ranadb emu killimmediately, beforesettings_secure.xmlwas rewritten. The reboot reloaded the stale file with the helper IME still default.The fix, enforced at both ends of the hazard: the one inner function that issues a restore’s
ime setregisters the device’s flush window (write + 2.5 s, timed on a monotonic per-serial clock — the window is a property of the device’s SettingsProvider, not of any host state dir) in the registrar’sfinallyfor every issued emulator write whatever the outcome, because a readback mismatch cannot prove the provider never accepted the write; and the one executor ofadb emu killwaits that window out before killing. Whether a write confirmed by read-back is a separate fact the mark carries, and it gates only one decision: whether an uninspected close may retire another session’s durable recovery marker. So every kill path is gated —close --shutdown, the standaloneshutdowncommand, and any future caller. A close cancelled mid-wait never reaches the kill and hands the remaining window to the next kill-bound caller; overlapping windows coalesce into one wait because the provider rewrites the whole settings file per flush. Touched files: 10.Validation
Per-head validation lives in comments: the latest validation comment records the tested head and its results (
pnpm check:affected --run,pnpm typecheck,pnpm fallow audit --base origin/main). New regression tests pin: restore-before-kill ordering with an awaited deferred restore, the flush deadline with signal forwarding, the marker surviving the settle, an abort mid-settle retaining marker + window with the retry consuming the remaining wait, coalescing of overlapping windows into a single wait, a cross-session kill-bound close waiting a window another close registered, a never-activated close leaving a retained marker untouched, a never-activated close covering an issued-but-unconfirmed window killing but keeping the marker, no settle on ordinary close / physical-device / failed-restore paths, the kill-site gate (kill waits a registered window / skips with none / abort never reachesadb emu kill), and startup-orphan recovery registering the window while paying no boot sleep.Live device evidence: on an arm64 API 35 (google_apis) emulator, head cycles run
open com.android.settings --relaunch(helper becomes default) →close --shutdown(~2.9 s, settle included) →-no-snapshot-loadrestart →LatinIME, 5/5. The base commit was then run on the same AVD as a control and does not reproduce there either (LatinIME 3/3, close in ~0.3 s), so this emulator cannot discriminate the fix — it proves no new breakage only. The reproduction stands on the reporter's platforms (API 32/37 x86_64). Falsification of the hazard itself on this image: a helperime setfollowed by SIGKILL of qemu within ~1 s did strand the helper across restart, confirming the lost-flush window exists here too; gracefuladb emu killjust usually wins the race on this fast image. A base run on API 32/37 is the missing discriminator and needs a provisioned AVD on those images.Review history, with the defects each round found in shipped code: round one resolved three blocking findings (test type errors, format gate, abort-signal gap in the settle); round two found that an aborted settle still cleared the marker, letting a retry kill inside the flush window (
10b4126e5); round three found that round-two's refactor routed the never-activated close through the shared marker clear, erasing a retained orphan marker (6c5221baf, splitnot-activated-herefromno-recordat the reason type); round four (cubic) found the double-window lock hold in the round-two/three design, fixed by coalescing to one deadline (3bbda232d); round five (independent review) found that an ordinary close registered no window, leaving a second session'sclose --shutdownfree to kill inside the flush window — every confirmed restore now registers its window, and the kill-bound predicate has a single owner (fcf968b22); round six (cubic) found that daemon-startup orphan recovery called the inner restore directly and registered no window, and that the standaloneshutdowncommand's kill was never gated by any close-side wait — registration moved inside the inner restore and the flush hold moved to the kill-site itself (83e78a7b8); round seven (cubic) found that the window guard timed itself on the wall clock, letting a forward host-clock step release the kill inside the flush window — the window is now timed on the process monotonic clock — encoded in the map name,testImeLastRestoreAtPerfMs— and the kill-site wait tests assert the derived magnitude and the abort/retry remainder re-derivation (5753cfa45,c352648db); round eight (cubic) found that the cancelled-kill test passed on any rejection rather than the cancellation it claims — it now binds the rejection by identity tocontroller.signal.reasonalongside theAbortErrorname (44ca81028,ab7334854); round nine (cubic) found the residual race in the wait itself: it read the mark once and reportedcoveredeven when a restore landed while it slept, releasing the kill inside the newer window — the wait now re-reads after every sleep and loops to the newest deadline, and the kill-site tests observe the middle of the wait with deferred sleeps (91d4c0b0a); round ten (cubic) confirmed that a daemon restart inside the window forfeits the remaining wait for a dead process's write — classified as a cross-process-deadline design boundary (a persisted deadline must be clock-independent and carry an expiry rule, reopening round seven's clock class otherwise) and filed as follow-up #3346, with the boundary recorded in the state module's doc comment (7c8297593,ce40232e3); round eleven (cubic) found that a kill racing an in-flight restore readidle— the window opens when the device acceptsime set, but the mark registered only after the readback — so in-flight writes are now tracked from issue and drained by the kill-side wait each round, the tests' hardcoded window literals moved behind an exported constant, and two local-only gate misses (an oxlint spread, a complexity threshold) were repaired after GitHub surfaced them (4d1b6d094,248aa0b35,b0516feb8); round twelve (coordinator) demanded the reachability proof, which produced a self-corrected ledger — proven interleaving is fire-and-forget startup recovery vs an early kill, with weaker pairs labeled candidate/open in-thread; round thirteen (coordinator) found the pending-write design had silently encoded "readback mismatch ⇒ no provider write" — the mark now registers in the registrar's finally for every issued emulator restore, whatever the outcome (b79910d46); round fourteen (cubic) found the finally-registration had silently inverted the marker-clear rule’s premise — the mark carried a timestamp alone, so a later uninspected kill-bound close covering a set-failed restore’s mark cleared the durable recovery marker while the helper was still active — the mark now carries its write’s own provenance, the flush wait reports covered-issued vs covered-confirmed, and the clear rule consumes that fact instead of inferring it from coverage (663f11d8c); round fifteen (cubic) collapsed a byte-identical mark seeder duplicated across two test files into the shared IME fixtures module, where the next mark-shape change lands in one edit, and tightened the marker-retention test’s sleep assertion from any-positive to the file’s own nearly-full-window bound (b36626a4f).Unresolved risk: the timed wait assumes the AOSP 2 s flush cap; a device overriding
SETTINGS_PROVIDER_MAX_WRITE_DELAY_MILLISupward could still lose the write.Closes #3318