Skip to content

fix: bound BLE acquire with a deadline and wire cancel into in-flight connects - #889

Closed
originalix wants to merge 4 commits into
onekeyfrom
fix/ble-acquire-deadline
Closed

fix: bound BLE acquire with a deadline and wire cancel into in-flight connects#889
originalix wants to merge 4 commits into
onekeyfrom
fix/ble-acquire-deadline

Conversation

@originalix

Copy link
Copy Markdown
Contributor

Context

Field incident (6.5.0 desktop, Classic 1S onboarding): checkAllFirmwareRelease hung for 306 s inside ensureConnected → connectDeviceForBle → device.acquire(). The Electron main process lost the $onekey-noble-ble-connect IPC reply in a two-connect race, and nothing on the renderer/hd-core side could unstick the call:

  • ensureConnected clears its per-try PollingTimeout timer before awaiting connectDeviceForBle, so the await has no deadline;
  • the abort signal is only polled at loop checkpoints, never wired into the in-flight acquire — cancel() had no effect until Electron GC finally reported "reply was never sent" ~5 minutes later.

The main-process no-reply hole is fixed since the keep-alive series (#847 wraps every noble callback in runBleCallbackOperation with timeouts), but hd-core still trusts any transport to always settle. This PR closes the structural hole.

Changes

  • raceBleAcquire: races device.acquire() against a 60 s deadline (BleTimeoutError, which the existing retry loop already handles) and the caller's AbortSignal (CallQueueActionCancelled, immediate).
  • On a deadline hit, the link is dropped (deviceConnector.disconnect + markTransportDisconnected) before rethrowing, so the retry cold-connects instead of stacking a second connect onto the half-open one — same recovery pattern the file already uses for ERROR_CODES_REQUIRE_DISCONNECT.
  • abortSignal is threaded from ensureConnected into connectDeviceForBle (and its retries), and CallQueueActionCancelled is added to ensureConnected's immediate-reject list so a cancel fails the call now instead of after another poll round.

Verification

  • tsc --noEmit error set identical to base (pre-existing unresolved-workspace-import errors only); eslint clean of new findings.
  • Worst case is now bounded: a wedged acquire fails in ≤60 s per attempt and every retry starts from a cold link; a user cancel takes effect immediately instead of "on call queue" minutes later.

@originalix

Copy link
Copy Markdown
Contributor Author

Note on versioning: npm already has up to 1.2.0-alpha.146 (consumed by side-branch releases incl. #886); #888 takes alpha.147 and this PR takes alpha.148. Whichever of the two merges second will need a trivial version-bump rebase since both touch every package.json.

@originalix

Copy link
Copy Markdown
Contributor Author

Added two follow-up commits after an adversarial self-review: (1) attach the acquire promise handlers before the aborted-at-entry early return and skip starting an acquire when the signal is already aborted — otherwise a cancel landing during the 3s retry backoff starts a post-cancel acquire whose eventual rejection is unhandled; (2) release the request-queue task on the onlyConnectBleDevice early return, which bypassed the normal-path releaseTask — in the field log a completed task haunted every queue snapshot and cancel sweep for 6 minutes.

@originalix

Copy link
Copy Markdown
Contributor Author

Consolidated into #888 (single branch/alpha for app-side self-testing), with the acquire deadline additionally scoped to env === 'desktop-web-ble' so react-native/lowlevel transports are untouched. The alpha.148 bump is superseded by #888's alpha.147.

@originalix originalix closed this Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant