-
Notifications
You must be signed in to change notification settings - Fork 37
fix: harden desktop BLE — stale peripheral cache, state listener lifetime, acquire deadline #888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: onekey
Are you sure you want to change the base?
Changes from 15 commits
0ccaa0d
006f903
d1fa81c
f6ceaf5
6f1d6ce
2544b0b
82a9616
0b81bf2
f15c0e2
881065c
4eff565
66fdd17
0631611
b1161c7
afed1c8
17d6012
8a6f829
ceb65f7
dc82ee3
d6be62d
3dd1b96
72e137f
dcbf3a7
aa45004
25e3854
64dcbd5
ce2d8e3
b2fa03f
25680af
ea906ff
b46341b
b2cb245
f177b02
12d79d1
be22a5d
294d4c3
081c18f
5df24cf
7148e31
dcdc438
bfb070f
37976e1
268ee94
cb79652
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -415,6 +415,10 @@ const onCallDevice = async ( | |
| if (method.payload?.onlyConnectBleDevice) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: [Connect-only success path still skips method disposal] This early return now completes tracing and removes the request task, but it still bypasses the common cleanup block that calls Dispose the method before returning from this branch, or route the response through the same cleanup/finally path used by normal calls.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: [Connect-only success path still skips method disposal] This early return now completes tracing and removes the request task, but it still bypasses the common cleanup block that calls Dispose the method before returning from this branch, or route the response through the same cleanup/finally path used by normal calls.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: [Connect-only success bypasses method disposal] This early return now completes tracing and removes the request task, but it still bypasses the common cleanup path that calls Please dispose the method before returning from this branch, or route the response through the same cleanup/finally path used by normal calls. |
||
| preWarmCallbackTask?.resolve(); | ||
| Log.debug('Call API - only connect ble device: ', device?.mainId); | ||
| // This early return bypasses the normal-path releaseTask at the end of the | ||
| // call; without it the task leaks and haunts every later queue snapshot | ||
| // and cancel sweep (field log: a completed task lingered for 6 minutes). | ||
| requestQueue.releaseTask(method.responseID); | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
|
||
| return createResponseMessage(method.responseID, true, null); | ||
| } | ||
|
|
||
|
|
@@ -954,7 +958,60 @@ export function isMissingDetectedProtocolV2Error(method: BaseMethod, error: unkn | |
| * If the Bluetooth connection times out, retry up to 6 times | ||
| * @param retryCount - Current retry count (default 0) | ||
| */ | ||
| async function connectDeviceForBle(method: BaseMethod, device: Device, retryCount = 0) { | ||
| // device.acquire awaits a transport reply with no deadline of its own; a | ||
| // transport that never settles (field case: Electron main lost an IPC reply, | ||
| // "reply was never sent" after 5 minutes) hangs the call forever and cancel() | ||
| // only takes effect at poll checkpoints. Race acquire against a deadline and | ||
| // the caller's abort signal so the hang is bounded and cancel is immediate. | ||
| const BLE_ACQUIRE_DEADLINE_MS = 60 * 1000; | ||
|
|
||
| function raceBleAcquire<T>(acquirePromise: Promise<T>, abortSignal?: AbortSignal): Promise<T> { | ||
| return new Promise<T>((resolve, reject) => { | ||
| let settled = false; | ||
| const settle = (fn: () => void) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(deadline); | ||
| abortSignal?.removeEventListener('abort', onAbort); | ||
| fn(); | ||
| }; | ||
| const onAbort = () => | ||
| settle(() => reject(ERRORS.TypedError(HardwareErrorCode.CallQueueActionCancelled))); | ||
| const deadline = setTimeout( | ||
| () => | ||
| settle(() => | ||
| reject( | ||
| ERRORS.TypedError( | ||
| HardwareErrorCode.BleTimeoutError, | ||
| `BLE acquire exceeded ${BLE_ACQUIRE_DEADLINE_MS}ms deadline` | ||
| ) | ||
| ) | ||
| ), | ||
| BLE_ACQUIRE_DEADLINE_MS | ||
| ); | ||
| // Attach before any early return so a late settlement of acquirePromise | ||
| // is always consumed — an abort or deadline must never leave the acquire | ||
| // rejection unhandled. | ||
| acquirePromise.then( | ||
| value => settle(() => resolve(value)), | ||
| error => settle(() => reject(error)) | ||
| ); | ||
| if (abortSignal) { | ||
| if (abortSignal.aborted) { | ||
| onAbort(); | ||
| return; | ||
| } | ||
| abortSignal.addEventListener('abort', onAbort); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| async function connectDeviceForBle( | ||
| method: BaseMethod, | ||
| device: Device, | ||
| abortSignal?: AbortSignal, | ||
| retryCount = 0 | ||
| ) { | ||
| try { | ||
| if (device.wasInterruptedByUser()) { | ||
| throw ERRORS.TypedError(HardwareErrorCode.DeviceInterruptedFromUser); | ||
|
|
@@ -968,9 +1025,43 @@ async function connectDeviceForBle(method: BaseMethod, device: Device, retryCoun | |
| !device.commands || | ||
| device.commands.disposed; | ||
| if (shouldAcquire) { | ||
| await device.acquire(method.payload.connectProtocol, { | ||
| forceProtocolDetection: method.payload.forceProtocolDetection, | ||
| }); | ||
| // The deadline/abort guards are scoped to the desktop electron | ||
| // transport: its IPC acquire is the only path with a proven | ||
| // never-settling failure mode, while react-native/lowlevel acquire may | ||
| // legitimately block on a user-driven system bonding prompt for longer | ||
| // than any sane deadline. Other envs keep the plain acquire unchanged. | ||
| const useAcquireGuards = DataManager.getSettings('env') === 'desktop-web-ble'; | ||
| // A cancel landing during the retry backoff must not start a new acquire. | ||
| if (useAcquireGuards && abortSignal?.aborted) { | ||
| throw ERRORS.TypedError(HardwareErrorCode.CallQueueActionCancelled); | ||
| } | ||
| if (!useAcquireGuards) { | ||
| await device.acquire(method.payload.connectProtocol, { | ||
| forceProtocolDetection: method.payload.forceProtocolDetection, | ||
| }); | ||
| } else { | ||
| try { | ||
| await raceBleAcquire( | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Timed-out acquire can mutate a newer connection attempt] When the deadline or abort wins, Invalidate the abandoned acquire with the same interruption or generation mechanism used for cancellation, and guard the state-commit section so a late completion cannot overwrite the active connection.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in
Your stronger suggestion — moving the deadline into the Electron transport so it aborts the underlying IPC — is the better end state and I have not done it here; this keeps the fix inside Core where the retry lives. Happy to follow up with the transport-level abort in a separate change. Duplicate threads for this same finding are being resolved together; this one carries the resolution.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: this fix is deferred, not shipped. It landed in Reason: this file's connection path is currently under field validation against a 6.5.0 control build — the cold-connect route, the forced reset and the cache eviction were each derived from device logs, and changing them mid-validation would invalidate the comparison. The finding itself stands and is queued to land once the root cause is confirmed. Nothing under test was affected: the app PR pins 1.2.0-alpha.165, which never contained these changes.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Timed-out acquire can mutate a newer connection attempt] When the deadline or abort wins, Invalidate the abandoned acquire with the same interruption or generation mechanism used for cancellation, and guard the state-commit section so a late completion cannot overwrite the active connection.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Acquire deadline allows overlapping native acquires] If the deadline wins, only Please invalidate the expired connection-attempt generation and disconnect by the known BLE connect ID before starting another acquire. |
||
| device.acquire(method.payload.connectProtocol, { | ||
| forceProtocolDetection: method.payload.forceProtocolDetection, | ||
| }), | ||
| abortSignal | ||
| ); | ||
| } catch (err) { | ||
| // A deadline hit means the transport is wedged mid-acquire; drop the | ||
| // link before the retry so it cold-connects instead of stacking a | ||
| // second connect onto the half-open one. | ||
| if ( | ||
|
originalix marked this conversation as resolved.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Acquire timeout retries while the original BLE acquire is still in flight] When the 60-second race expires, only the wrapper promise is rejected; the underlying Please invalidate/cancel the underlying acquire (or advance the connection-attempt generation so late completion is discarded) and wait for that operation to be safely quarantined before starting a retry.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Acquire timeout retries while the original BLE acquire is still in flight] When the 60-second race expires, only the wrapper promise is rejected; the underlying Please invalidate or cancel the underlying acquire, or advance the connection-attempt generation so late completion is discarded, and quarantine that operation before starting a retry.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: [Acquire timeout retries while the original BLE acquire is still in flight] When the deadline or cancellation fires, only the wrapper promise is rejected; the underlying Please cancel or invalidate the underlying acquire and quarantine late completion with the connection-attempt generation before starting a retry. |
||
| err.errorCode === HardwareErrorCode.BleTimeoutError && | ||
|
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
originalix marked this conversation as resolved.
|
||
| device.mainId && | ||
| device.deviceConnector | ||
| ) { | ||
| await device.deviceConnector.disconnect(device.mainId).catch(() => undefined); | ||
| device.markTransportDisconnected(); | ||
| } | ||
| throw err; | ||
| } | ||
| } | ||
| } | ||
| if (method.payload?.onlyConnectBleDevice) { | ||
| if (shouldAcquire) { | ||
|
|
@@ -1010,7 +1101,7 @@ async function connectDeviceForBle(method: BaseMethod, device: Device, retryCoun | |
| const nextRetry = retryCount + 1; | ||
| Log.debug(`Bluetooth connection will retry, retry count: ${nextRetry}`); | ||
| await wait(3000); | ||
| await connectDeviceForBle(method, device, nextRetry); | ||
| await connectDeviceForBle(method, device, abortSignal, nextRetry); | ||
| } else { | ||
| throw err; | ||
| } | ||
|
|
@@ -1120,7 +1211,7 @@ const ensureConnected = async ( | |
| if (tryCount === 1) { | ||
| device.beginConnectionAttempt(); | ||
| } | ||
| await connectDeviceForBle(method, device); | ||
| await connectDeviceForBle(method, device, abortSignal); | ||
| } | ||
| resolve(device); | ||
| return; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.