fix(android): rebind the test IME when its commit went to a stale input session - #3061
okwasniewski wants to merge 9 commits into
Conversation
…ut session
Android can leave the test IME holding an input session the app has
already replaced: the app then drops every commit ('Session id
mismatch header.sessionId: 0 currentSessionId: 1'), and re-tapping a
field that already has focus starts no new session, so the fill retry
failed the same way. When a helper commit left the field showing its
hint, the retry now rebinds the IME (ime disable, enable, set) before
re-focusing, which starts a fresh session with the focused field.
![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 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
A commit none of which reached the field (hint showing, or the value the field held before) triggers the rebind, so a pre-filled field's dropped clear-and-commit recovers too. A field back on its hint is no app formatting, so it never ends the retry as unconfirmed evidence. The rebind reads the selected IME back; if another IME is selected the device leaves the helper route and the fill stops retrying.
|
I found one problem in 153e257 that should be fixed before merge. CI is green, with all 13 checks passing, and I know of no conflicts. The rebind failure path in rebindAndroidImeHelperChecked can leave the device on the test IME or without its original keyboard, and it leaves no recovery marker. It runs Not blocking, and you can take or leave these: the rebind-failed test at text-input-test-ime.test.ts#L372 uses a bare I did not rerun the emulator benchmark or the logcat sequence from the PR body, and the Before merge, the rebind failure branch must hand ownership back through the ime-restore owner, under the recovery lock and settled by read-back. |
…the helper The rebind failure path dropped the device from the owned set outside the recovery lock and without settling the restore record or marker, so a failed read-back (settings get timing out) made close-time restore report no-record and strand the user on the helper. The rebind now lives with the activation owner, never drops ownership, and marks the device displaced instead: close-time restore then returns it to the recorded previous IME even when Android fell back to another IME, and a later activation keeps the recorded previous IME as the restore target.
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
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-state.ts">
<violation number="1" location="packages/platform-android/src/ime-state.ts:11">
P3: This comment's grammar obscures the ownership invariant; rewrite it to state that the rebind disabled the helper and Android selected a fallback IME.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // Owned devices whose helper a rebind disabled without confirming it selected again. The IME | ||
| // Android fell back to is not the user's choice, so it must not become the restore target. |
There was a problem hiding this comment.
P3: This comment's grammar obscures the ownership invariant; rewrite it to state that the rebind disabled the helper and Android selected a fallback IME.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At packages/platform-android/src/ime-state.ts, line 11:
<comment>This comment's grammar obscures the ownership invariant; rewrite it to state that the rebind disabled the helper and Android selected a fallback IME.</comment>
<file context>
@@ -8,6 +8,10 @@ import { getAndroidImeHelperDeviceKey } from './ime-helper.ts';
// route text entry through the broadcast channel.
export const activeTestImeDevices = new Set<string>();
+// Owned devices whose helper a rebind disabled without confirming it selected again. The IME
+// Android fell back to is not the user's choice, so it must not become the restore target.
+export const rebindDisplacedTestImeDevices = new Set<string>();
</file context>
| // Owned devices whose helper a rebind disabled without confirming it selected again. The IME | |
| // Android fell back to is not the user's choice, so it must not become the restore target. | |
| // Owned devices whose helper was disabled by a rebind without confirming it was selected again. The | |
| // fallback IME Android selected is not the user's choice, so it must not become the restore target. |
The rebind now runs under the owner's recovery lock, found through the ownership entry that activation records with its state dir, so a close-time restore can no longer interleave with it. An on-device record is written before the helper is disabled and cleared only once the helper reads back selected; restore and activation read that record instead of a process-lived set, so startup recovery after a crash also returns the user's IME. A rebind whose read-back throws counts as unconfirmed, and text entry rebinds an unconfirmed helper before any broadcast or fails with android_test_ime_rebind_unconfirmed.
The rebind's catch-all turned request cancellation into an unconfirmed rebind. A canceled request now rejects as canceled, keyed on the typed error; the device record written before the rebind stays set, so restore still returns the user's IME.
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Thanks for the update. The stale-session rebind is not ready to merge on 580217b, because the new device route still has no live proof and the Coverage check fails. The Coverage failure comes from this PR. The delta changes the device-facing rebind route in ime-activation.ts. Every stale-session recovery now writes, reads back and deletes a secure setting around ime disable/enable/set. An unconfirmed rebind now keeps ownership and re-rebinds at admission (text-input.ts). The only live evidence in the PR body comes from an earlier head. Nothing yet shows that the settings round-trip keeps recovery working on a stressed emulator. Nothing shows that close and startup recovery return the user's keyboard after an unconfirmed rebind. Please rerun the stressed Pixel 7 API 36 benchmark on 580217b. Show logcat with a session-id mismatch, then Not blocking, and you can take or leave it: the PR body and your latest comment say a failed rebind takes the device off the helper route, but the head keeps ownership and fails the next entry with Next, please teach the fake in |
…r cleared An unreadable record keeps the recorded restore target, an idempotent activation keeps a rebind the record still marks, and a rebind stays unconfirmed until its record is written and later cleared.
|
Thanks for the update. The 580217b findings are addressed in a258fbc, and the fake now has the new settings key, but one fail-open path remains in restore, and the live run is still missing. Restore still fails open on an unreadable displacement record. Activation now treats an undefined read as unknown, but https://github.com/callstack/agent-device/blob/a258fbc/packages/platform-android/src/ime-restore.ts#L88 checks The settings write, read-back and delete around ime disable/enable/set at https://github.com/callstack/agent-device/blob/a258fbc/packages/platform-android/src/ime-activation.ts#L275 changed again in this delta, and the only live run in the PR body predates 580217b. Please run the stressed Pixel 7 API 36 benchmark on a258fbc or later. Post logcat that shows the session-id mismatch, then android_test_ime_rebind, then the helper onCreate, then a successful commitText. Also post Not blocking: the comment at https://github.com/callstack/agent-device/blob/a258fbc/packages/platform-android/src/ime-activation.ts#L296 says the next entry "retries the clear", but I did not run the tests locally, and CI is still pending. Coverage and Integration run the platform-android IME tests this delta changes, so a failure there would be related to this PR. I also did not read every assertion of the new idempotent-activation test. Before merge, restore must keep the marker on an unreadable record, and the live run must be posted on the new head. |
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
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-activation.ts">
<violation number="1" location="packages/platform-android/src/ime-activation.ts:96">
P3: `settleRebindDisplacement` returns `true` (settled) without looking at the device record when `priorPersistedIme === undefined`, so an activation can claim `rebindUnconfirmed: false` while `agent_device_ime_helper_rebind_displaced` still reads `1`. That stale record can originate from the claim-already-active path (it never writes a previous-IME record) followed by a failed rebind, or from a crash between `clearPersistedPreviousIme` and `clearPersistedRebindDisplacement` in restore; `restoreAndroidTestImeFor` then sees the missing previous-IME record as `no-record` and never clears it. Later, `readActivationRestoreTarget` keeps `priorPersistedIme` instead of the current IME, and close-time restore treats the stale `1` as a genuine displacement and runs `ime set previousIme`, overriding an IME the user switched to mid-session — the exact 'user's own switch' invariant the restore path otherwise preserves. Gate the settle on the record itself instead of on the prior-IME record.</violation>
<violation number="2" location="packages/platform-android/src/ime-activation.ts:215">
P1: Publish ownership before awaiting the displacement-record cleanup. If that ADB read/delete rejects after the helper switch, activation fails with the marker and restore record still present, but close-time restore sees no in-process owner and leaves the user on the helper until a later startup recovery; keep the owner marked unconfirmed, then update its flag after settlement.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| const rebindSettled = await settleRebindDisplacement(adb, priorPersistedIme); | ||
| activeTestImeDevices.set(deviceKey, { | ||
| stateDir: options.stateDir, | ||
| rebindUnconfirmed: !rebindSettled, | ||
| }); |
There was a problem hiding this comment.
P1: Publish ownership before awaiting the displacement-record cleanup. If that ADB read/delete rejects after the helper switch, activation fails with the marker and restore record still present, but close-time restore sees no in-process owner and leaves the user on the helper until a later startup recovery; keep the owner marked unconfirmed, then update its flag after settlement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At packages/platform-android/src/ime-activation.ts, line 215:
<comment>Publish ownership before awaiting the displacement-record cleanup. If that ADB read/delete rejects after the helper switch, activation fails with the marker and restore record still present, but close-time restore sees no in-process owner and leaves the user on the helper until a later startup recovery; keep the owner marked unconfirmed, then update its flag after settlement.</comment>
<file context>
@@ -222,8 +212,11 @@ async function activateAndroidTestImeAfterStartupRecovery(
// active ownership after the helper is confirmed active on the device.
- activeTestImeDevices.set(deviceKey, { stateDir: options.stateDir, rebindUnconfirmed: false });
- await settleRebindDisplacement(adb, priorPersistedIme);
+ const rebindSettled = await settleRebindDisplacement(adb, priorPersistedIme);
+ activeTestImeDevices.set(deviceKey, {
+ stateDir: options.stateDir,
</file context>
| const rebindSettled = await settleRebindDisplacement(adb, priorPersistedIme); | |
| activeTestImeDevices.set(deviceKey, { | |
| stateDir: options.stateDir, | |
| rebindUnconfirmed: !rebindSettled, | |
| }); | |
| const ownership = { | |
| stateDir: options.stateDir, | |
| rebindUnconfirmed: true, | |
| }; | |
| activeTestImeDevices.set(deviceKey, ownership); | |
| const rebindSettled = await settleRebindDisplacement(adb, priorPersistedIme); | |
| ownership.rebindUnconfirmed = !rebindSettled; |
| if (priorPersistedIme === undefined) return true; | ||
| return await clearPersistedRebindDisplacement(adb); |
There was a problem hiding this comment.
P3: settleRebindDisplacement returns true (settled) without looking at the device record when priorPersistedIme === undefined, so an activation can claim rebindUnconfirmed: false while agent_device_ime_helper_rebind_displaced still reads 1. That stale record can originate from the claim-already-active path (it never writes a previous-IME record) followed by a failed rebind, or from a crash between clearPersistedPreviousIme and clearPersistedRebindDisplacement in restore; restoreAndroidTestImeFor then sees the missing previous-IME record as no-record and never clears it. Later, readActivationRestoreTarget keeps priorPersistedIme instead of the current IME, and close-time restore treats the stale 1 as a genuine displacement and runs ime set previousIme, overriding an IME the user switched to mid-session — the exact 'user's own switch' invariant the restore path otherwise preserves. Gate the settle on the record itself instead of on the prior-IME record.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At packages/platform-android/src/ime-activation.ts, line 96:
<comment>`settleRebindDisplacement` returns `true` (settled) without looking at the device record when `priorPersistedIme === undefined`, so an activation can claim `rebindUnconfirmed: false` while `agent_device_ime_helper_rebind_displaced` still reads `1`. That stale record can originate from the claim-already-active path (it never writes a previous-IME record) followed by a failed rebind, or from a crash between `clearPersistedPreviousIme` and `clearPersistedRebindDisplacement` in restore; `restoreAndroidTestImeFor` then sees the missing previous-IME record as `no-record` and never clears it. Later, `readActivationRestoreTarget` keeps `priorPersistedIme` instead of the current IME, and close-time restore treats the stale `1` as a genuine displacement and runs `ime set previousIme`, overriding an IME the user switched to mid-session — the exact 'user's own switch' invariant the restore path otherwise preserves. Gate the settle on the record itself instead of on the prior-IME record.</comment>
<file context>
@@ -80,15 +81,20 @@ async function readActivationRestoreTarget(
-): Promise<void> {
- if (priorPersistedIme !== undefined) await clearPersistedRebindDisplacement(adb);
+): Promise<boolean> {
+ if (priorPersistedIme === undefined) return true;
+ return await clearPersistedRebindDisplacement(adb);
}
</file context>
| if (priorPersistedIme === undefined) return true; | |
| return await clearPersistedRebindDisplacement(adb); | |
| const record = await readPersistedRebindDisplacement(adb); | |
| if (record === false) return true; | |
| return await clearPersistedRebindDisplacement(adb); |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Code-quality review. The fix works, but the rebind is bolted on rather than modeled. It is a third IME lifecycle transaction living inside the activation module. Its state is spread across a durable settings key, a mutable in-memory flag and four readers that each treat "can't read the key" differently. The fill path and the type path also handle a failed rebind differently. Inline comments cover each point. They skip points already raised (restore !== true fail-open, allowFailure on ime verbs, the hardcoded component, comment wording, settle ordering, live-run request).
The PR body is also stale. It says a failed rebind drops the device from activeTestImeDevices, but since 8dd4668 ownership is kept with rebindUnconfirmed: true.
| beforeTarget: AndroidFillVerification['targetInput'], | ||
| ): Promise<boolean> { | ||
| if (isAndroidImeCommitDropped(lastVerification, beforeTarget)) { | ||
| if (!(await rebindAndroidTestIme(device))) return false; |
There was a problem hiding this comment.
The same failure, a rebind that cannot confirm the helper, gives two different results depending on the call path. In admitAndroidTextChannel it throws a typed COMMAND_FAILED with reason: 'android_test_ime_rebind_unconfirmed' and a "close and reopen the session" hint. Here it returns false, the loop breaks, and the caller reports the first attempt's generic "Android fill verification failed" with no rebind reason. Because of this, the new test "fillAndroid stops on an unconfirmed rebind…" can only assert code: 'COMMAND_FAILED'. An agent that reads that error retries the fill, which then fails differently through the admit path.
Use one policy. Call confirmAndroidTestImeRebound(device) here and throw instead of returning a boolean. prepareAndroidImeHelperRetry then returns void, the lastVerification && !(await …) break guard goes away, and the fill test can assert details.reason === 'android_test_ime_rebind_unconfirmed' like the type tests do.
| * is cleared only on confirmation, so restore — at close or after a crash — returns an unconfirmed | ||
| * device to the user's IME even when Android fell back to another one. | ||
| */ | ||
| export async function rebindAndroidTestIme(device: DeviceInfo): Promise<boolean> { |
There was a problem hiding this comment.
This is a third IME lifecycle transaction, next to activation and restore. It has its own lock section, durable record, diagnostics and failure modes, but it is appended to ime-activation.ts (204 → 329 lines), and its eight tests went into ime-activation.test.ts (139 → 388). Move rebindAndroidTestIme and rebindAndReadSelectedIme to ime-rebind.ts, and their tests to ime-rebind.test.ts.
While moving it, fix the contract. A bare boolean collapses four outcomes: no owner, ownership replaced, record write failed, and helper not selected or unreadable. The rest goes through a side channel that mutates ownership.rebindUnconfirmed on the shared map value (L285, L298). Return a discriminated outcome instead, for example { kind: 'confirmed' } | { kind: 'not-owned' } | { kind: 'unconfirmed', cause: 'record-write' | 'helper-not-selected' | 'read-failed' }, so callers switch on one value. This also fixes the "helper not selected" message, which is wrong for read failures.
| } | ||
|
|
||
| /** Whether the device record marks an unconfirmed rebind, or `undefined` when it cannot be read. */ | ||
| export async function readPersistedRebindDisplacement( |
There was a problem hiding this comment.
readPersistedRebindDisplacement returns boolean | undefined, and each caller decides on the spot what undefined means:
readActivationRestoreTargettreats only=== falseas clear.claimAlreadyActiveTestIme(ime-activation.ts:253) uses!== false.- restore (ime-restore.ts:88) uses
!== true, which is the fail-open already flagged. - the write and clear helpers turn it back into a boolean.
On top of that, ownership.rebindUnconfirmed is an in-memory copy of the same fact, and ime-activation.ts:252 ORs the two together. That gives two sources of truth and four separate readings of an unknown value. This is how the restore bug got in, and the next reader will add a fifth reading.
Model the device record once. Use one reader, for example readAndroidTestImeDeviceRecord(adb): { kind: 'unreadable' } | { kind: 'absent' } | { kind: 'owned'; previousIme: string; rebindDisplaced: boolean }, that fails closed on unreadable in one place. Activation, the idempotent claim and restore then switch on that union. Derive the in-memory flag from the record instead of keeping it beside it.
| } | ||
|
|
||
| /** Whether none of a helper commit reached the field: it shows its hint, or the value it held before. */ | ||
| function isAndroidImeCommitDropped( |
There was a problem hiding this comment.
isAndroidImeCommitDropped combines two clauses that buildAndroidFillUnconfirmedVerification already checks: hintShowing === true (fill-verification.ts:165) and beforeTarget.text === verification.actual (fill-verification.ts:174). The "nothing of the commit landed" rule now lives in two modules and can drift. Export isAndroidFillCommitDropped(verification, beforeTarget) from fill-verification.ts, use it in the soft-success guard in place of those two clauses, and import it here. Move its test cases into fill-verification.test.ts.
| // the field may also have gone to a stale input session, which only a rebind of the IME replaces. | ||
| for (let attempt = 0; attempt < 2; attempt += 1) { | ||
| if (attempt > 0) await focusAndroid(device, x, y); | ||
| if ( |
There was a problem hiding this comment.
This is now a 2-iteration for whose counter is never read. The retry gate is lastVerification && !(await prepare…) with a break, and the result comes back as lastVerification as AndroidFillVerification. What the code does is: try once, and if the failure is not a soft success, prepare and try again. Write it that way with a local attemptFill():
const first = await attemptFill();
if (first.ok || buildAndroidFillUnconfirmedVerification(…)) return first;
await prepareAndroidImeHelperRetry(…);
return await attemptFill();The cast and the null-initialised accumulator go away. With the throw from the L303 comment, the boolean return goes away as well.
Fixes #3052.
Summary
On an API 36 emulator under load,
fillinto a freshly mounted, autofocused React NativeTextInputsometimes fails withAndroid fill verification failed. The field keeps showing its hint (hintShowing: true). The test IME commits (commitText length=N), but the app drops the text:The IME is left holding an input session the app has already replaced, so the app rejects everything it sends:
beginBatchEdit,performContextMenuAction,commitText,endBatchEdit. The fill retry re-taps the field, but a tap on a field that already has focus starts no new session, so the retry failed the same way 1.6 s later. In logcat, every failure was one such stale episode, covering both attempts.When none of a helper commit reached the field (it shows its hint, or the value it held before the fill), the retry now rebinds the IME with
ime disable,ime enableandime seton the helper service before re-focusing. The rebind recreates the IME service, which starts a fresh session with the focused field.android_test_ime_rebind_failed), so the next text entry goes through the checked activation path, and the fill stops retrying.Touched:
ime-helper.ts(rebindAndroidImeHelper),text-input.ts(the retry),fill-verification.ts(the soft-success guard), and 1 test file.Validation
e2e mobile benchmark,
sequential onboarding echoes the exact valuesrepeated on a Pixel 7 API 36 emulator with 8 CPU-stress processes, logcat captured:mainmainOn this branch, the recovery shows in logcat as the session mismatch, then the IME's
onCreate(the rebind), thenclearTextandcommitTextwith no further mismatch.text-input-test-ime.test.ts:ime disable,enable,set, in that order and before the retry's tap, and the text lands (fails onmain);All fail on
main.pnpm check:affected --run: passed.pnpm test:coverage:ci: passed. Changed-line gate: passed.