fix(android): wait out a device-offline refusal and retry once - #3055
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-android/src/adb-provider-scope.ts">
<violation number="1" location="packages/platform-android/src/adb-provider-scope.ts:129">
P2: When the caller passes no `timeoutMs`, the retry runs with `timeoutMs: undefined` — unbounded. The wait gets a 15s default (`ANDROID_DEVICE_OFFLINE_WAIT_MS`), so a device that answers the wait but then wedges (e.g. adbd still restarting, or an install held on an OEM confirmation dialog) holds the caller for 15s plus an unlimited second attempt — the kind of long hang the PR's fast-fail goal is meant to avoid. Give the no-timeout retry the same 15s default as the wait.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| options?.signal?.throwIfAborted(); | ||
| const retryOptions = | ||
| budgetMs === undefined | ||
| ? options |
There was a problem hiding this comment.
P2: When the caller passes no timeoutMs, the retry runs with timeoutMs: undefined — unbounded. The wait gets a 15s default (ANDROID_DEVICE_OFFLINE_WAIT_MS), so a device that answers the wait but then wedges (e.g. adbd still restarting, or an install held on an OEM confirmation dialog) holds the caller for 15s plus an unlimited second attempt — the kind of long hang the PR's fast-fail goal is meant to avoid. Give the no-timeout retry the same 15s default as the wait.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-android/src/adb-provider-scope.ts, line 129:
<comment>When the caller passes no `timeoutMs`, the retry runs with `timeoutMs: undefined` — unbounded. The wait gets a 15s default (`ANDROID_DEVICE_OFFLINE_WAIT_MS`), so a device that answers the wait but then wedges (e.g. adbd still restarting, or an install held on an OEM confirmation dialog) holds the caller for 15s plus an unlimited second attempt — the kind of long hang the PR's fast-fail goal is meant to avoid. Give the no-timeout retry the same 15s default as the wait.</comment>
<file context>
@@ -60,15 +60,105 @@ export function createDeviceAdbExecutor(
+ options?.signal?.throwIfAborted();
+ const retryOptions =
+ budgetMs === undefined
+ ? options
+ : { ...options, timeoutMs: Math.max(1, budgetMs - (Date.now() - startedAt)) };
+ const retry = await attemptAdb(serial, async () => await run(retryOptions));
</file context>
| ? options | |
| ? { ...options, timeoutMs: ANDROID_DEVICE_OFFLINE_WAIT_MS } |
There was a problem hiding this comment.
[claude-opus-5-5] responding on behalf of @okwasniewski
Declining. A caller with no timeoutMs already runs its first attempt unbounded; the retry keeps that same contract, and bounding it would change behavior for callers that deliberately set none (long installs). The added cost is at most the 15 s wait, and only once per stayed-offline window. Leaving this open for the maintainer.
|
[claude-opus-5-5] responding on behalf of @okwasniewski The Coverage failure is unrelated to this diff: |
|
The reconnect fix in 290b28b looks right, but one test does not prove the main rule of this PR. The retry must share the caller's timeout with the wait, and no test checks the retry half of that. In adb-provider-scope.test.ts, the wait-for-device stub resolves at once. So the retry timeout is 4000 or 3999 ms. If Not blocking, and you can take or leave these. The offline-key at adb-provider-scope.ts:77 puts the per-call port first, while The live Pixel 7 API 36 reconnect run appears only in the PR body, with no log attached. It also called The only failing check is the daemon-client transport test 'a delayed restart health probe stops at the RPC deadline without retrying'. This PR changes only |
An emulator drops to offline for a few seconds after boot while adbd restarts. adb refuses commands on the host side then, so a helper install could fail with 'adb: device offline'. The device-scoped executor now waits for the device and runs the command once more. The wait and retry stay inside the caller's timeoutMs (wait gets at most half, 15s cap). A device that stays offline through its wait fails fast for 30s instead of making every call wait.
…budget - match only stderr that is exactly adb's device-offline refusal, empty stdout, no timeout, so device output never triggers a rerun - wait takes half of the remaining budget; no budget left, refusal stands - wait uses the command's adb server and env - timeouts and cancels keep the stayed-offline mark - key the mark by adb server and serial
290b28b to
0be2e35
Compare
…left The wait stub now moves the clock 1500 ms, so the retry must get 4000 - 1500 ms; a retry given the caller's full timeout fails the test. The abort and timeout cases assert the error they reject with. The stayed-offline key takes the route's installed adb server first, as the route itself does.
…fusal The adb failure classifier now records hostRefusal (adbHostRefusal on thrown errors) when stderr is exactly the host adb's device-offline refusal, stdout is empty, and the command did not time out. The retry keys only on that detail, so the separate stderr predicate is gone. The broad device_offline family is unchanged. Stayed-offline marks move to their own module, which drops lapsed marks when it records a new one.
* origin/main: 0.21.17 feat(daemon): report the host CPU architecture in /health (callstack#3048) feat: add daemon policy to confine devices, commands, and device shutdown (callstack#3064) test(web): wait for the killed fake daemon to be reaped before asserting it is gone (callstack#3066) fix(ios): write the simulator clipboard from the runner (callstack#3065) test(daemon-client): a restart probe that fails outright near the RPC deadline reports the daemon unavailable (callstack#3058) fix(daemon-client): a client whose daemon lost the start race adopts the winner (callstack#3057) fix(android): honor boot --timeout as the emulator boot deadline (callstack#3059) test(ios-smoke): wait once more when the runner is still starting behind a deep link (callstack#3063)
There was a problem hiding this comment.
2 issues found across 5 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/adb-failure.test.ts">
<violation number="1" location="packages/platform-android/src/adb-failure.test.ts:30">
P3: The expected `hint` is derived from `classifyAndroidAdbFailure('device offline')` — the same function under test — so the hint assertion compares the implementation with itself and can never fail on wrong hint text. Both code paths also share the same `ANDROID_ADB_DEVICE_OFFLINE_FAILURE` constant, making the comparison structurally guaranteed. Inline the literal hint text (or drop the hint field) so the expected value is independent of the code it verifies.</violation>
</file>
<file name="packages/platform-android/src/adb-provider-scope.ts">
<violation number="1" location="packages/platform-android/src/adb-provider-scope.ts:123">
P2: The stayed-offline cooldown is a module-level map with no in-flight guard between the `has()` check and the `mark()` at the end of `retryOnceAfterDeviceOffline`. When two adb commands for the same device run concurrently while it is offline, both pass `devicesStayingOffline.has(device) === false` before either one marks, so both execute the full up-to-15s `wait-for-device` and both retry; the second `mark()` then re-arms the cooldown to a fresh 30s window on top of the first. The "fails fast" benefit only applies to commands arriving after both retries finish, so parallel e2e adb calls each pay the full wait during a boot-offline window. Consider keying an in-flight wait promise so concurrent refusals for the same server+serial join the running wait/retry instead of duplicating it.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| ): Promise<AndroidAdbExecutorResult> { | ||
| const startedAt = Date.now(); | ||
| const first = await attemptAdb(device, async () => await run(options)); | ||
| if (!first.offline || devicesStayingOffline.has(device)) { |
There was a problem hiding this comment.
P2: The stayed-offline cooldown is a module-level map with no in-flight guard between the has() check and the mark() at the end of retryOnceAfterDeviceOffline. When two adb commands for the same device run concurrently while it is offline, both pass devicesStayingOffline.has(device) === false before either one marks, so both execute the full up-to-15s wait-for-device and both retry; the second mark() then re-arms the cooldown to a fresh 30s window on top of the first. The "fails fast" benefit only applies to commands arriving after both retries finish, so parallel e2e adb calls each pay the full wait during a boot-offline window. Consider keying an in-flight wait promise so concurrent refusals for the same server+serial join the running wait/retry instead of duplicating it.
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/adb-provider-scope.ts, line 123:
<comment>The stayed-offline cooldown is a module-level map with no in-flight guard between the `has()` check and the `mark()` at the end of `retryOnceAfterDeviceOffline`. When two adb commands for the same device run concurrently while it is offline, both pass `devicesStayingOffline.has(device) === false` before either one marks, so both execute the full up-to-15s `wait-for-device` and both retry; the second `mark()` then re-arms the cooldown to a fresh 30s window on top of the first. The "fails fast" benefit only applies to commands arriving after both retries finish, so parallel e2e adb calls each pay the full wait during a boot-offline window. Consider keying an in-flight wait promise so concurrent refusals for the same server+serial join the running wait/retry instead of duplicating it.</comment>
<file context>
@@ -118,7 +120,7 @@ async function retryOnceAfterDeviceOffline(
const startedAt = Date.now();
const first = await attemptAdb(device, async () => await run(options));
- if (!first.offline || (devicesStayingOffline.get(device) ?? 0) > Date.now()) {
+ if (!first.offline || devicesStayingOffline.has(device)) {
return first.outcome();
}
</file context>
| for (const stderr of ['adb: device offline\n', "error: device 'emulator-5554' offline"]) { | ||
| assert.deepEqual(classifyAndroidAdbFailure(stderr), { | ||
| reason: 'device_offline', | ||
| hint: classifyAndroidAdbFailure('device offline')?.hint, |
There was a problem hiding this comment.
P3: The expected hint is derived from classifyAndroidAdbFailure('device offline') — the same function under test — so the hint assertion compares the implementation with itself and can never fail on wrong hint text. Both code paths also share the same ANDROID_ADB_DEVICE_OFFLINE_FAILURE constant, making the comparison structurally guaranteed. Inline the literal hint text (or drop the hint field) so the expected value is independent of the code it verifies.
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/adb-failure.test.ts, line 30:
<comment>The expected `hint` is derived from `classifyAndroidAdbFailure('device offline')` — the same function under test — so the hint assertion compares the implementation with itself and can never fail on wrong hint text. Both code paths also share the same `ANDROID_ADB_DEVICE_OFFLINE_FAILURE` constant, making the comparison structurally guaranteed. Inline the literal hint text (or drop the hint field) so the expected value is independent of the code it verifies.</comment>
<file context>
@@ -23,6 +23,38 @@ test('ADB failure classification keeps transport on stderr and install verdicts
+ for (const stderr of ['adb: device offline\n', "error: device 'emulator-5554' offline"]) {
+ assert.deepEqual(classifyAndroidAdbFailure(stderr), {
+ reason: 'device_offline',
+ hint: classifyAndroidAdbFailure('device offline')?.hint,
+ retriable: true,
+ hostRefusal: true,
</file context>
attemptAdb asks isOfflineRefusalResult or isOfflineRefusalError instead of reading the classification inline, which keeps it under the fallow complexity threshold.
|
I found no code blockers at dd05dd9. The earlier findings from #3055 (comment) are fixed, and I found no new problems in the code. Not blocking, and you can take or leave these: the expected hint in https://github.com/callstack/agent-device/blob/dd05dd9/packages/platform-android/src/adb-failure.test.ts#L30 comes from I did not run the tests, and the live emulator run exists only in the PR body with no log, so it is not independently confirmed. That run also called Smoke Tests passed on dd05dd9. Coverage fails, and this PR causes it: the eager-closure budget test reports that |
thymikee
added
the
ready-for-human
… scope A separate module added one module to the eager import closure of platform-android mechanics.ts, which failed the eager-closure budget.
|
The PR is ready. The fixes since the earlier review (dd05dd9) look good at 192f759, and no code changes are needed. The Coverage failure was the eager-closure budget (177 vs 176 modules for mechanics.ts), and this change fixes it by removing the separate module and its static edge. All 13 checks are passing now. Not blocking: createStayedOfflineDevices and StayedOfflineDevices are now exported from the provider-scope module and only adb-provider-scope.test.ts uses them, so they are test-only exports of a production module; you can test the window through the retry route or leave it as it is. I did not run the tests or the eager-closure budget test locally. The budget fix is inferred from the removed import edge and the green checks. This change does not touch adb-failure.ts, which the earlier review already covered. The emulator run is still described only in the PR body, and since this change is a pure in-package move, it needs no new device evidence. I know of no conflicts, and the next step is a maintainer merge decision. |
Summary
An Android emulator can drop back to
offlinefor a few seconds aftersys.boot_completedwhile adbd restarts. In that window, the host adb refuses every command withadb: device offlinebefore anything reaches the device.adb-failure.tsalready classified this asdevice_offlineand marked it retriable, but no caller ever acted on that.In the e2e mobile benchmark, the first test after boot hit that window during the snapshot helper install (job):
The device-scoped executor (
createSerialAdbExecutor) now reacts to adevice offlinerefusal, whether it comes back as a result or as a thrown error. It runsadb -s <serial> wait-for-deviceagainst the same adb server and environment, then retries the command once. Only the bare host refusal counts: stderr exactlyadb: device offlineorerror: device offline, empty stdout, and no timeout. Output from a command that ran on the device never matches. The refusal comes from the host adb, so the command never ran and the retry cannot repeat a side effect. If the device is still offline, the second refusal surfaces with the existing classification and hint.The retry is bounded so short probes keep their budgets:
timeoutMscovers the wait and the retry together. The wait takes at most half of what is left, and 15 s when the caller sets no timeout. With no budget left, the refusal stands. An abort during the wait skips the retry.doctor, the boot uptime probe, and snapshot-helper retirement.Touched: 2 source files (
adb-provider-scope.ts,adb-failure.ts) and 2 test files.Validation
Live, on a Pixel 7 API 36 emulator:
adb -s emulator-5554 reconnect devicefollowed immediately by a guardedshell truethroughcreateDeviceAdbExecutor. Onmain, 5/5 attempts fail withadb: device offline(11-13 ms). With this change, 6/6 succeed (440-560 ms) with and without a caller timeout, and so does the thrown-error path. This also confirms the real host serializes the waitFor-only invocation.adb-provider-scope.test.tscovers:device offlineis not retried;Each new assertion fails without the fix.
adb-executor.test.ts: the retriable-classification test now models a device that stays offline (the command, the wait, the retry).pnpm check:affected --run: passed.pnpm test:coverage:ci: passed. Changed-line gate: passed (100%, 51/51).