Bound the BLE connect and file-read waits so they report instead of hanging

index.html

@@ -393,6 +393,7 @@ <h1>Select USB Host Folder</h1>
                 </tr>
             </tbody>
         </table>
+        <div id="message"></div>
         <div id="firmware-update" class="firmware-update-suggestion-container"></div>
         <h3>More network devices<i class="refresh fa-solid fa-sync-alt" title="Refresh Device List"></i></h3>
         <div id="devices"></div>
@@ -435,6 +436,7 @@ <h3>More network devices<i class="refresh fa-solid fa-sync-alt" title="Refresh D
                 </tr>
             </tbody>
         </table>
+        <div id="message"></div>
         <div id="firmware-update" class="firmware-update-suggestion-container"></div>
         <div class="buttons centered">
             <button class="purple-button ok-button">Close</button>

js/common/ble-file-transfer.js

@@ -8,6 +8,52 @@ class FileTransferClient extends BLEFileTransferClient {
     constructor(bleDevice, bufferSize, workflow = null) {
         super(bleDevice, bufferSize);
         this._workflow = workflow;
+        this._bleDevice = bleDevice;
+    }
+
+    // Reject a read if the GATT link is already down, or drops while it is in
+    // flight, instead of returning a promise that can never settle.
+    //
+    // Upstream readFile()/listDir() install their promise's reject handler
+    // AFTER writing the request:
+    //
+    //     await this._write(header);
+    //     await this._write(encoded);
+    //     let p = new Promise((resolve, reject) => {
+    //         this._resolve = resolve;
+    //         this._reject = reject;      // too late
+    //     });
+    //
+    // On a dead link `_transfer` is null, so both writes throw; _write()
+    // swallows the error and calls onDisconnected(), which has no `_reject` to
+    // call yet. checkConnection() likewise catches its own failure and returns
+    // normally rather than rethrowing, so the read proceeds regardless. The
+    // returned promise is then never settled by anyone and the caller hangs --
+    // which is what left the editor spinning on "Current Device Info".
+    //
+    // Bound on liveness rather than elapsed time: a large file read over BLE can
+    // legitimately take tens of seconds, so a stopwatch would produce false
+    // failures, while a dropped link is unambiguous.
+    _whileConnected(operation) {
+        const device = this._bleDevice;
+        if (!device || !device.gatt || !device.gatt.connected) {
+            return Promise.reject(new Error("Bluetooth device is not connected"));
+        }
+        return new Promise((resolve, reject) => {
+            const onDisconnected = () => reject(new Error("Bluetooth device disconnected"));
+            device.addEventListener("gattserverdisconnected", onDisconnected, {once: true});
+            operation().then(resolve, reject).finally(() => {
+                device.removeEventListener("gattserverdisconnected", onDisconnected);
+            });
+        });
+    }
+
+    async readFile(path, raw = false) {
+        return await this._whileConnected(() => super.readFile(path, raw));
+    }
+
+    async listDir(path) {
+        return await this._whileConnected(() => super.listDir(path));
     }
 
     _signalMutatingOp() {

js/common/dialogs.js

@@ -340,9 +340,28 @@ class ButtonValueDialog extends GenericModal {
     }
 }
 
+// Report a failed device-info read in the dialog. Without this the read's
+// rejection escapes as an unhandled promise rejection and the dialog is simply
+// left blank, which reads as "the device answered with nothing" rather than
+// "we never reached the device".
+function showDeviceInfoError(modal, error) {
+    console.error("Unable to read device info:", error);
+    const msgElement = modal.querySelector("#message");
+    if (msgElement) {
+        msgElement.textContent =
+            "Could not read device information. The connection to the device was lost.";
+    }
+}
+
 class DiscoveryModal extends GenericModal {
     async _getVersionInfo() {
-        const deviceInfo = await this._showBusy(this._fileHelper.versionInfo());
+        let deviceInfo;
+        try {
+            deviceInfo = await this._showBusy(this._fileHelper.versionInfo());
+        } catch (error) {
+            showDeviceInfoError(this._currentModal, error);
+            return;
+        }
         this._currentModal.querySelector("#version").textContent = deviceInfo.version;
         const boardLink = this._currentModal.querySelector("#board");
         boardLink.href = `https://circuitpython.org/board/${deviceInfo.board_id}/`;
@@ -413,7 +432,13 @@ class DiscoveryModal extends GenericModal {
 
 class DeviceInfoModal extends GenericModal {
         async _getDeviceInfo() {
-        const deviceInfo = await this._showBusy(this._fileHelper.versionInfo());
+        let deviceInfo;
+        try {
+            deviceInfo = await this._showBusy(this._fileHelper.versionInfo());
+        } catch (error) {
+            showDeviceInfoError(this._currentModal, error);
+            return;
+        }
         this._currentModal.querySelector("#version").textContent = deviceInfo.version;
         const boardLink = this._currentModal.querySelector("#board");
         boardLink.href = `https://circuitpython.org/board/${deviceInfo.board_id}/`;

js/workflows/ble.js

@@ -25,6 +25,27 @@ const POST_OP_DISCONNECT_GRACE_MS = 4000;
 // Wait after GATT reconnects so the VM finishes booting before the next op.
 const POST_RECONNECT_SETTLE_MS = 2000;
 
+// How long to wait for an advertisement before connecting anyway. Chrome's
+// BlueZ backend never delivers advertisementreceived, so on Linux this event
+// does not arrive at all and an unbounded wait leaves the connect dialog open
+// forever with no feedback. macOS delivers the first event within ~30ms, so a
+// couple of seconds is generous everywhere it works. On Linux the wait is not
+// wasted even though nothing arrives: the discovery session that
+// watchAdvertisements() opens is what makes BlueZ (re)create its device object,
+// without which gatt.connect() rejects as "no longer in range".
+const ADVERTISEMENT_WAIT_MS = 2000;
+// How long to allow gatt.connect() before giving up. Chrome bounds this itself
+// at ~41s on Linux, but not while a watchAdvertisements() watch is armed -- in
+// that state the promise simply never settles. Successful connects have been
+// measured from 0.5s (macOS, Windows) up to 26.6s (Linux), hence the generous
+// ceiling.
+const CONNECT_TIMEOUT_MS = 30000;
+// Per-attempt bound for the silent reconnect after a firmware autoreload.
+// Shorter than CONNECT_TIMEOUT_MS because this path runs once per entry in
+// RECONNECT_DELAYS_MS, and a reconnect that has not landed within ten seconds
+// has stopped being silent regardless of whether it eventually succeeds.
+const SILENT_RECONNECT_TIMEOUT_MS = 10000;
+
 let btnRequestBluetoothDevice, btnReconnect;
 
 class BLEWorkflow extends Workflow {
@@ -62,6 +83,11 @@ class BLEWorkflow extends Workflow {
         // Track in-flight watchAdvertisements abort controllers so we can
         // cancel them when any device wins or when we tear down (#410).
         this._pendingAdvAborts = new Set();
+
+        // Only one device may attempt a connection at a time. Without this,
+        // several remembered devices whose advertisement waits expire together
+        // would all try to connect at once.
+        this._connectAttemptInFlight = false;
     }
 
     // Called by the FileTransferClient wrapper right before any mutating
@@ -131,10 +157,7 @@ class BLEWorkflow extends Workflow {
         }
         // Cancel any in-flight watchAdvertisements so a subsequent reconnect
         // doesn't pile up Chrome's per-device watch quota (#410).
-        for (const ctrl of this._pendingAdvAborts) {
-            ctrl.abort();
-        }
-        this._pendingAdvAborts.clear();
+        this._abortAdvWatches();
         await super.onDisconnected(e, reconnect);
     }
 
@@ -234,65 +257,132 @@ class BLEWorkflow extends Workflow {
         });
     }
 
+    // Abort pending advertisement watches, optionally sparing one. Deleting
+    // while iterating a Set is safe.
+    _abortAdvWatches(keep = null) {
+        for (const ctrl of this._pendingAdvAborts) {
+            if (ctrl !== keep) {
+                ctrl.abort();
+                this._pendingAdvAborts.delete(ctrl);
+            }
+        }
+    }
+
     async connectToBluetoothDevice(device) {
         const abortController = new AbortController();
         this._pendingAdvAborts.add(abortController);
         let advHandled = false;
 
-        async function onAdvertisementReceived(event) {
-            // Multiple ads can land in the same event-loop tick before
-            // abortController.abort() takes effect on the listener. Guard
-            // so we only run the connect flow once per device. See #410.
-            if (advHandled) {
+        // Runs either when an advertisement arrives or when we give up waiting
+        // for one. Guarded because multiple ads can land in the same event-loop
+        // tick before abortController.abort() takes effect on the listener, and
+        // because the timer can fire alongside a late advertisement. See #410.
+        const attemptConnect = async (reason) => {
+            if (advHandled || this._connectAttemptInFlight) {
                 return;
             }
             advHandled = true;
-            console.log('> Received advertisement from "' + device.name + '"...');
-            // This device won. Abort ALL pending watchAdvertisements
-            // (including this one) so other paired devices stop scanning
-            // and don't pile up Chrome's per-device watch quota.
-            for (const ctrl of this._pendingAdvAborts) {
-                ctrl.abort();
-            }
-            this._pendingAdvAborts.clear();
-            console.log('Connecting to GATT Server from "' + device.name + '"...');
+            this._connectAttemptInFlight = true;
+            clearTimeout(advTimer);
+
+            // This device won. Stop every pending watch, this device's
+            // included, BEFORE connecting -- Chrome holds a BlueZ discovery
+            // session for as long as any watch is armed, and connecting while
+            // one is active is what fails on Linux. Measured by driving
+            // Device1.Connect() directly: discovery stopped 36/36, discovery
+            // active 18/52. Intermittent -- a host suspend/resume clears the
+            // failing state until the next boot, so it may not reproduce. In
+            // the failing case the HCI create-connection is identical to a
+            // working one and the controller simply transmits nothing until
+            // the attempt is cancelled ~20s later.
+            this._abortAdvWatches();
             try {
-                this.bleServer = await device.gatt.connect();
-            } catch (error) {
-                console.log(error);
-                // TODO(ericzundel): Add to suggestBLEConnectAction if we can determine the exception type
-                this.showConnectStatus("Failed to connect to device. Try forgetting device from OS bluetooth devices and try again.");
-                // Disable the reconnect button
-                this.connectionStep(1);
-            }
-            if (this.bleServer && this.bleServer.connected) {
-                console.log('> Bluetooth device "' +  device.name + ' connected.');
-                await this.switchToDevice(device);
-            } else {
-                console.log('Unable to connect to bluetooth device "' +  device.name + '.');
+                await this._connectToGattServer(device, reason);
+            } finally {
+                this._connectAttemptInFlight = false;
             }
-        }
+        };
+
+        const advTimer = setTimeout(
+            () => attemptConnect(`no advertisement within ${ADVERTISEMENT_WAIT_MS / 1000}s`),
+            ADVERTISEMENT_WAIT_MS);
 
         // Use the abortController signal so we don't need to manage the
         // handler reference manually — the listener is auto-removed when
-        // onAdvertisementReceived calls abortController.abort().
-        device.addEventListener('advertisementreceived',
-            onAdvertisementReceived.bind(this),
-            {signal: abortController.signal});
+        // abortController.abort() is called.
+        device.addEventListener('advertisementreceived', () => {
+            console.log('> Received advertisement from "' + device.name + '"...');
+            attemptConnect('advertisement received');
+        }, {signal: abortController.signal});
 
         this.debugLog("Attempting to connect to " + device.name + "...");
         try {
             this.clearConnectStatus();
+            // Say something during the advertisement wait. On Linux it always
+            // runs to the full timeout, and silence looks like a hang.
+            this.showConnectStatus("Looking for " + device.name + "...");
             console.log('Watching advertisements from "' + device.name + '"...');
             console.log('If no advertisements are received, make sure the device is powered on and in range. You can also try resetting the device.');
             await device.watchAdvertisements({signal: abortController.signal});
         }
         catch (error) {
+            clearTimeout(advTimer);
             console.error(error);
             this.showConnectStatus(this._suggestBLEConnectActions(error));
         }
     }
 
+    // Connect with a bound. gatt.connect() does not always reject on its own --
+    // on Linux with a watch armed it never settles -- so race it against a timer
+    // and cancel with gatt.disconnect(), which is the only way page JS can abort
+    // an in-flight connect. Chrome has honoured disconnect() as a cancel since
+    // M140; before that the attempt is orphaned rather than aborted, so treat a
+    // timeout as fatal rather than assuming the adapter is left clean.
+    async _connectWithTimeout(device, timeoutMs) {
+        let connectTimer;
+        try {
+            return await Promise.race([
+                device.gatt.connect(),
+                new Promise((_, reject) => {
+                    connectTimer = setTimeout(() => {
+                        device.gatt.disconnect();
+                        reject(new Error(
+                            `connect did not complete within ${timeoutMs / 1000}s`));
+                    }, timeoutMs);
+                }),
+            ]);
+        } finally {
+            clearTimeout(connectTimer);
+        }
+    }
+
+    async _connectToGattServer(device, reason) {
+        console.log(`Connecting to GATT Server from "${device.name}" (${reason})...`);
+        this.showConnectStatus("Connecting to " + device.name + "...");
+
+        try {
+            this.bleServer = await this._connectWithTimeout(device, CONNECT_TIMEOUT_MS);
+        } catch (error) {
+            console.log(error);
+            // TODO(ericzundel): Add to suggestBLEConnectAction if we can determine the exception type
+            this.showConnectStatus(
+                `Could not connect to ${device.name}. Try again. If it keeps failing, forget the ` +
+                `device in your operating system's Bluetooth settings, then reload this page.`);
+            // Disable the reconnect button
+            this.connectionStep(1);
+            return;
+        }
+
+        if (this.bleServer && this.bleServer.connected) {
+            console.log('> Bluetooth device "' + device.name + '" connected.');
+            await this.switchToDevice(device);
+        } else {
+            console.log('Unable to connect to bluetooth device "' + device.name + '".');
+            this.showConnectStatus(`Could not connect to ${device.name}. Try again.`);
+            this.connectionStep(1);
+        }
+    }
+
     // Request Bluetooth Device
     async onRequestBluetoothDeviceButtonClick(e) {
         console.log('Requesting any Bluetooth device...');
@@ -405,7 +495,11 @@ class BLEWorkflow extends Workflow {
                 await sleep(delay);
                 try {
                     console.log(`Silent reconnect: attempting after ${delay}ms…`);
-                    this.bleServer = await this.bleDevice.gatt.connect();
+                    // Bounded: an unbounded connect here stalls the whole
+                    // reconnect ladder, and every mutating op waits on it via
+                    // awaitPostOpReconnect(), so a save appears to hang.
+                    this.bleServer = await this._connectWithTimeout(
+                        this.bleDevice, SILENT_RECONNECT_TIMEOUT_MS);
                     if (this.bleServer && this.bleServer.connected) {
                         console.log('Silent reconnect: GATT reconnected, rebinding characteristics…');
                         await this._rebindAfterSilentReconnect();