From eb60135a334455345405de850d48453fecc77477 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 00:10:03 +0200 Subject: [PATCH 01/24] wip --- src/device/deviceManager.ts | 83 +++++++++++++-------------- src/device/provider/deviceProvider.ts | 46 +++------------ 2 files changed, 46 insertions(+), 83 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index d88047a0..d0844d7f 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -21,9 +21,15 @@ export enum DeviceManagerEvent { deviceNotification = 'deviceNotification', } -type AcquireResult = - | { successful: true } - | { successful: false, reason: string }; +type AcquireResult = + | { successful: true, device: D } + | { successful: false, reason: unknown }; + +type QueueEntry = { + deviceDetectionInfo: DeviceDetectionInfo; + deviceOffer: () => Promise; + resolve: (result: AcquireResult) => void; +}; type DeviceManagerEventMap = { [DeviceManagerEvent.deviceConnected]: [device: AnyDevice]; @@ -39,7 +45,7 @@ export default class DeviceManager private readonly logger: Logger; - private readonly detectedDeviceAcquireQueue: Map void }[]> = new Map(); + private readonly detectedDeviceAcquireQueue: Map[]> = new Map(); private readonly connectedDevices: Map; @@ -110,51 +116,27 @@ export default class DeviceManager this.clearDetectedDeviceAcquireQueue(deviceInfo.detectionId, `Device with id '${deviceInfo.detectionId}' has disappeared`); } - public async acquireDetectedDevice(deviceId: DeviceId): Promise + public async offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { - return new Promise((resolve) => { - const deviceQueue = this.detectedDeviceAcquireQueue.get(deviceId); + return new Promise>((resolve) => { + const deviceQueue = this.detectedDeviceAcquireQueue.get(deviceDetectionInfo.detectionId); if (undefined === deviceQueue) { - resolve({ successful: false, reason: `Device with id '${deviceId}' is not available for claiming` }); + resolve({ successful: false, reason: new Error(`Device with id '${deviceDetectionInfo.detectionId}' is not available for claiming`) }); return; } // Always add to queue first - deviceQueue.push({ resolve }); + deviceQueue.push({ deviceDetectionInfo, deviceOffer, resolve }); - // If we're first in line, resolve immediately + // If we're first in line, run our offer immediately if (deviceQueue.length === 1) { - resolve({ successful: true }); + this.runNextInQueue(deviceQueue); } }); } - public releaseDetectedDevice(deviceId: DeviceId): void - { - const deviceQueue = this.detectedDeviceAcquireQueue.get(deviceId); - - if (undefined === deviceQueue) { - return; - } - - // Release current claimant and hand off the claim to the next waiter - deviceQueue.shift(); - - if (deviceQueue.length === 0) { - this.detectedDeviceAcquireQueue.delete(deviceId); - return; - } - - deviceQueue[0]?.resolve({ successful: true }); - } - - /** - * Registers a fully connected device, unless its known device (identified by the final - * `getDeviceId`) is disabled - then the device is closed and registered for retry instead. - * Returns whether the device was added. - */ - public addDevice(deviceInfo: DeviceDetectionInfo, device: AnyDevice): boolean + private addDevice(deviceInfo: DeviceDetectionInfo, device: AnyDevice): boolean { if (!this.isDeviceEnabled(device.getDeviceId)) { this.logger.info(`Not adding device '${device.getDeviceId}' since it is disabled`); @@ -163,7 +145,6 @@ export default class DeviceManager .catch((e: unknown) => logError(this.logger, `Failed to close disabled device '${device.getDeviceId}'`, e)); this.registerPendingRetry(deviceInfo, device.getDeviceId, closingDevice); - this.releaseDetectedDevice(deviceInfo.detectionId); return false; } @@ -178,8 +159,6 @@ export default class DeviceManager this.eventEmitter.emit(DeviceManagerEvent.deviceConnected, device); - this.claimDetectedDevice(deviceInfo.detectionId); - return true; } @@ -227,11 +206,6 @@ export default class DeviceManager } } - public claimDetectedDevice(deviceId: DeviceId): void - { - this.clearDetectedDeviceAcquireQueue(deviceId, `Device with id '${deviceId}' has been claimed by another provider`); - } - public getConnectedDevices(): AnyDevice[] { return Array.from(this.connectedDevices.values()); @@ -286,6 +260,27 @@ export default class DeviceManager } } + private runNextInQueue(deviceQueue: QueueEntry[]): void + { + const entry = deviceQueue[0]; + + if (undefined === entry) { + return; + } + + entry.deviceOffer() + .then((device) => { + if (device === undefined) { + entry.resolve({ successful: false, reason: new Error(`Device offer for '${entry.deviceDetectionInfo.detectionId}' returned undefined`) }); + return; + } + + const added = this.addDevice(entry.deviceDetectionInfo, device); + entry.resolve(added ? { successful: true, device } : { successful: false, reason: new Error(`Device '${device.getDeviceId}' is disabled not added`) }); + }) + .catch((e: unknown) => entry.resolve({ successful: false, reason: e })); + } + private clearDetectedDeviceAcquireQueue(deviceId: string, reason: string): void { for (const entry of this.detectedDeviceAcquireQueue.get(deviceId) ?? []) { diff --git a/src/device/provider/deviceProvider.ts b/src/device/provider/deviceProvider.ts index 0cb0f9f6..72c2873b 100644 --- a/src/device/provider/deviceProvider.ts +++ b/src/device/provider/deviceProvider.ts @@ -5,6 +5,7 @@ import { asyncHandler } from '../../util/async.js'; import { logError } from '../../util/error.js'; import { AnyDevice, DeviceEvent } from '../device.js'; import { DeviceId } from '../deviceId.js'; +import BaseError from 'modern-errors'; export type AnyDeviceProvider = DeviceProvider; @@ -104,54 +105,21 @@ export default abstract class DeviceProvider this.createDevice(deviceDetectionInfo)); - if (!acquireResult.successful) { - this.logger.debug(`Could not acquire device: ${acquireResult.reason}`); - return; - } - - let device: D | undefined; - - try { - device = await this.createDevice(deviceDetectionInfo); - } catch (e: unknown) { - logError(this.logger, `Error while connecting to device '${deviceDetectionInfo.detectionId}'`, e); - await this.abortDetection(deviceDetectionInfo); - return; - } - - if (undefined === device || this.isStopped()) { - try { - if (undefined !== device) { - await device.close(); - } - } finally { - await this.abortDetection(deviceDetectionInfo); - } + if (!result.successful) { + this.logger.info(`Device offer for '${deviceDetectionInfo.detectionId}' was rejected: ${BaseError.normalize(result.reason).message}`); + await this.onConnectFailed(deviceDetectionInfo); return; } - device.on(DeviceEvent.deviceDisconnected, (d) => this.connectedDevices.delete(d.getDeviceId)); - - // The device manager may reject the device, e.g. because it is disabled - if (!this.deviceManager.addDevice(deviceDetectionInfo, device)) { - return; - } + result.device.on(DeviceEvent.deviceDisconnected, (d) => this.connectedDevices.delete(d.getDeviceId)); - this.connectedDevices.set(device.getDeviceId, device); + this.connectedDevices.set(result.device.getDeviceId, result.device); this.logger.info(`Connected devices: ${this.connectedDevices.size}`); } - private async abortDetection(deviceDetectionInfo: DDI): Promise { - try { - await this.onConnectFailed(deviceDetectionInfo); - } finally { - this.deviceManager.releaseDetectedDevice(deviceDetectionInfo.detectionId); - } - } - protected abstract canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DDI; protected abstract createDevice(deviceDetectionInfo: DDI): Promise; From d23a8ed02cefa13f97449d4827b222621a48289c Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 08:12:08 +0200 Subject: [PATCH 02/24] fix: repair device offer queue hand-off, stop race and disabled-device gating - DeviceManager.offerDevice(): advance the acquire queue to the next waiter on every non-terminal offer outcome (undefined, thrown, disabled-rejected), not just on success. Previously a failed offer left the queue stuck, breaking the multi-provider fallback (e.g. trying multiple serial protocols against the same detected port) and permanently blocking re-announcement of that detection id. - Add DeviceOfferRejectedError to distinguish manager-level rejections (disabled, claimed elsewhere, revoked, unavailable) from a real connect failure, so DeviceProvider only runs onConnectFailed() for the latter. - DeviceProvider: restore the isStopped() guard (checked at offer execution time) so a device that connects after the provider was stopped gets closed instead of registered. - DeviceProvider: track a device in the provider's own connectedDevices map inside the offer closure, before handing it back to the manager, instead of after the awaited offerDevice() call resolves. addDevice() emits deviceConnected synchronously as part of that resolution, so the previous ordering left a window where external listeners of that event (e.g. VirtualDeviceProvider's settings-change discovery) could observe the device as connected before the owning provider's own bookkeeping reflected it - causing dynamic device removal handling to silently drop devices depending on scheduling. Also log connected device count immediately at each add/remove instead of reading the shared count later, avoiding redundant/misleading repeated log lines. - DeviceManager.announceDetectedDevice(): remove the early isDeviceEnabled(detectionId) gate. Detection id is preliminary and can be shared by multiple protocol providers computing distinct canonical ids (e.g. several serial protocols probing the same port); gating on it incorrectly blocked every provider whenever it happened to collide with an unrelated disabled known device. The actual disabled check now happens once via addDevice()'s canonical-id check after a provider connects, which already drives the pending-retry re-announce mechanism on its own. - Rewrite/extend unit tests for the new offerDevice() API and the above behaviors. --- src/device/deviceManager.ts | 49 ++- src/device/deviceOfferRejectedError.ts | 8 + src/device/provider/deviceProvider.ts | 49 ++- tests/unit/device/deviceManager.spec.ts | 316 +++++++++++------- .../device/provider/deviceProvider.spec.ts | 154 ++++++++- 5 files changed, 429 insertions(+), 147 deletions(-) create mode 100644 src/device/deviceOfferRejectedError.ts diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index d0844d7f..cdbd09a2 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -7,6 +7,7 @@ import Logger from '../logging/Logger.js'; import { logError } from '../util/error.js'; import { DeviceId } from './deviceId.js'; import SettingsManager from '../settings/settingsManager.js'; +import DeviceOfferRejectedError from './deviceOfferRejectedError.js'; export type DeviceDetectionInfo = { type: string; @@ -89,13 +90,6 @@ export default class DeviceManager return; } - if (!this.isDeviceEnabled(deviceInfo.detectionId)) { - this.logger.debug(`Device with id '${deviceInfo.detectionId}' is disabled, not announcing it as detected`); - // No connection happened yet, so the detection id doubles as the canonical id here - this.registerPendingRetry(deviceInfo, deviceInfo.detectionId); - return; - } - this.logger.info(`Detected new device with id ${deviceInfo.detectionId}`); this.detectedDeviceAcquireQueue.set(deviceInfo.detectionId, []); @@ -122,7 +116,7 @@ export default class DeviceManager const deviceQueue = this.detectedDeviceAcquireQueue.get(deviceDetectionInfo.detectionId); if (undefined === deviceQueue) { - resolve({ successful: false, reason: new Error(`Device with id '${deviceDetectionInfo.detectionId}' is not available for claiming`) }); + resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is not available anymore for claiming`) }); return; } @@ -268,23 +262,54 @@ export default class DeviceManager return; } + const detectionId = entry.deviceDetectionInfo.detectionId; + entry.deviceOffer() .then((device) => { if (device === undefined) { - entry.resolve({ successful: false, reason: new Error(`Device offer for '${entry.deviceDetectionInfo.detectionId}' returned undefined`) }); + entry.resolve({ successful: false, reason: new Error(`Device offer for '${detectionId}' returned undefined`) }); + this.advanceQueue(detectionId, deviceQueue); return; } const added = this.addDevice(entry.deviceDetectionInfo, device); - entry.resolve(added ? { successful: true, device } : { successful: false, reason: new Error(`Device '${device.getDeviceId}' is disabled not added`) }); + + if (added) { + entry.resolve({ successful: true, device }); + this.clearDetectedDeviceAcquireQueue(detectionId, `Device '${detectionId}' has been claimed by another provider`); + return; + } + + entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`) }); + this.advanceQueue(detectionId, deviceQueue); }) - .catch((e: unknown) => entry.resolve({ successful: false, reason: e })); + .catch((e: unknown) => { + entry.resolve({ successful: false, reason: e }); + this.advanceQueue(detectionId, deviceQueue); + }); + } + + /** + * Drops the just-settled entry and hands off to the next waiter, if any - mirrors the old + * releaseDetectedDevice() hand-off. Deletes the queue entirely once empty so the device can + * be re-announced (announceDetectedDevice() gates on the map key existing). + */ + private advanceQueue(detectionId: string, deviceQueue: QueueEntry[]): void + { + deviceQueue.shift(); + + if (deviceQueue.length === 0) { + this.detectedDeviceAcquireQueue.delete(detectionId); + return; + } + + this.runNextInQueue(deviceQueue); } private clearDetectedDeviceAcquireQueue(deviceId: string, reason: string): void { for (const entry of this.detectedDeviceAcquireQueue.get(deviceId) ?? []) { - entry.resolve({ successful: false, reason }); + entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(reason) }); } this.detectedDeviceAcquireQueue.delete(deviceId); diff --git a/src/device/deviceOfferRejectedError.ts b/src/device/deviceOfferRejectedError.ts new file mode 100644 index 00000000..5ed67843 --- /dev/null +++ b/src/device/deviceOfferRejectedError.ts @@ -0,0 +1,8 @@ +/** + * Marks a device offer rejection as a manager-level decision (queue unavailable, device + * disabled, claimed by another provider, revoked, reset) as opposed to the offer itself failing + * (thrown error or `undefined` returned). `DeviceProvider` uses this to decide whether + * `onConnectFailed()` should run - it shouldn't for manager-level decisions, only for the + * provider's own failed attempt. + */ +export default class DeviceOfferRejectedError extends Error {} diff --git a/src/device/provider/deviceProvider.ts b/src/device/provider/deviceProvider.ts index 72c2873b..511564a7 100644 --- a/src/device/provider/deviceProvider.ts +++ b/src/device/provider/deviceProvider.ts @@ -1,5 +1,6 @@ import EventEmitter from 'events'; import DeviceManager, { DeviceDetectionInfo, DeviceManagerEvent } from '../deviceManager.js'; +import DeviceOfferRejectedError from '../deviceOfferRejectedError.js'; import Logger from '../../logging/Logger.js'; import { asyncHandler } from '../../util/async.js'; import { logError } from '../../util/error.js'; @@ -105,19 +106,49 @@ export default abstract class DeviceProvider this.createDevice(deviceDetectionInfo)); + const result = await this.deviceManager.offerDevice(deviceDetectionInfo, async () => { + const device = await this.createDevice(deviceDetectionInfo); - if (!result.successful) { - this.logger.info(`Device offer for '${deviceDetectionInfo.detectionId}' was rejected: ${BaseError.normalize(result.reason).message}`); - await this.onConnectFailed(deviceDetectionInfo); - return; - } + if (undefined === device) { + return undefined; + } - result.device.on(DeviceEvent.deviceDisconnected, (d) => this.connectedDevices.delete(d.getDeviceId)); + // Provider was stopped while the offer was in flight (or waiting in queue) - don't + // hand a connected device to a stopped provider, treat it like a failed offer instead + if (this.isStopped()) { + try { + await device.close(); + } catch (e: unknown) { + logError(this.logger, `Failed to close device '${device.getDeviceId}' after provider was stopped`, e); + } + return undefined; + } - this.connectedDevices.set(result.device.getDeviceId, result.device); + // Tracked here, before handing the device back to the manager, since addDevice() + // emits deviceConnected synchronously as soon as this offer settles - our own + // bookkeeping must already be in place by then for any listener of that event to see + // consistent state. If the manager ends up rejecting the device anyway (e.g. + // disabled), its own close() call fires deviceDisconnected, which the listener below + // uses to roll this back. + device.on(DeviceEvent.deviceDisconnected, (d) => { + this.connectedDevices.delete(d.getDeviceId); + this.logger.info(`Connected devices: ${this.connectedDevices.size}`); + }); + this.connectedDevices.set(device.getDeviceId, device); + this.logger.info(`Connected devices: ${this.connectedDevices.size}`); + + return device; + }); - this.logger.info(`Connected devices: ${this.connectedDevices.size}`); + if (!result.successful) { + this.logger.info(`Device offer for '${deviceDetectionInfo.detectionId}' was rejected: ${BaseError.normalize(result.reason).message}`); + + // Only a real connect failure (thrown/undefined offer) warrants provider cleanup - + // manager-level rejections (disabled, claimed elsewhere, revoked, unavailable) don't + if (!(result.reason instanceof DeviceOfferRejectedError)) { + await this.onConnectFailed(deviceDetectionInfo); + } + } } protected abstract canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DDI; diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index 77901675..f0d89da0 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -1,8 +1,9 @@ -import {describe, it, expect, beforeEach} from "vitest"; +import {describe, it, expect, beforeEach, vi} from "vitest"; import {mock,mockClear} from "vitest-mock-extended"; import DeviceManager, { DeviceManagerEvent, DeviceDetectionInfo } from "../../../src/device/deviceManager.js"; +import DeviceOfferRejectedError from "../../../src/device/deviceOfferRejectedError.js"; import {EventEmitter} from "events"; -import Device from "../../../src/device/device.js"; +import Device, { AnyDevice } from "../../../src/device/device.js"; import TestDevice from "./testDevice.js"; import Logger from "../../../src/logging/Logger.js"; import { DeviceId } from "../../../src/device/deviceId.js"; @@ -15,9 +16,17 @@ describe('deviceManager', () => { // device as enabled - the desired default for tests unrelated to the enable/disable feature. const mockedSettingsManager = mock(); + // Announces the device and immediately offers it for connection - the only way to get a + // device registered through the public API now that addDevice() is private. + const connectDevice = async (manager: DeviceManager, deviceInfo: DeviceDetectionInfo, device: AnyDevice) => { + manager.announceDetectedDevice(deviceInfo); + return manager.offerDevice(deviceInfo, () => Promise.resolve(device)); + }; + it('it adds device to managed devices and emits an event', async () => { const mockedDeviceManagerEventEmitter = mock(); + mockedDeviceManagerEventEmitter.emit.mockReturnValue(true); const mockedLogger = mock(); mockedLogger.child.mockReturnValue(mockedLogger); @@ -26,18 +35,19 @@ describe('deviceManager', () => { const deviceId = DeviceId.create('test-device-id'); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; // New device connected expect(deviceManager.getConnectedDevices().length).toBe(0); - deviceManager.addDevice({ type: 'test', detectionId: deviceId }, device); + mockClear(mockedDeviceManagerEventEmitter); // drop the constructor-time noise, if any + await connectDevice(deviceManager, deviceInfo, device); let actualDevices = deviceManager.getConnectedDevices(); expect(actualDevices.length).toBe(1); expect(actualDevices[0]).toBe(device); - expect(mockedDeviceManagerEventEmitter.emit).toBeCalledTimes(1); expect(mockedDeviceManagerEventEmitter.emit).toBeCalledWith(DeviceManagerEvent.deviceConnected, device); expect(mockedLogger.child).toBeCalledWith({ name: DeviceManager.name }); }); @@ -48,25 +58,25 @@ describe('deviceManager', () => { const connectedDevices = new Map(); const deviceId = DeviceId.create('test-device-id'); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; const mockedDeviceManagerEventEmitter = mock(); + mockedDeviceManagerEventEmitter.emit.mockReturnValue(true); const mockedLogger = mock(); mockedLogger.child.mockReturnValue(mockedLogger); const deviceManager = new DeviceManager(mockedDeviceManagerEventEmitter, connectedDevices, mockedSettingsManager, mockedLogger); - deviceManager.addDevice({ type: 'test', detectionId: deviceId }, device); + await connectDevice(deviceManager, deviceInfo, device); + mockClear(mockedDeviceManagerEventEmitter); // Connected device refreshed await device.refresh(); - expect(mockedDeviceManagerEventEmitter.emit).toBeCalledTimes(2); - expect(mockedDeviceManagerEventEmitter.emit).toHaveBeenNthCalledWith(1, DeviceManagerEvent.deviceConnected, device); - expect(mockedDeviceManagerEventEmitter.emit).toHaveBeenNthCalledWith(2, DeviceManagerEvent.deviceRefreshed, device); + expect(mockedDeviceManagerEventEmitter.emit).toBeCalledTimes(1); + expect(mockedDeviceManagerEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceRefreshed, device); expect(mockedLogger.child).toBeCalledWith({ name: DeviceManager.name }); - - mockClear(mockedDeviceManagerEventEmitter); }); it('it emits an event on device update', async () => { @@ -74,24 +84,26 @@ describe('deviceManager', () => { const connectedDevices = new Map(); const deviceId = DeviceId.create('test-device-id'); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; const mockedDeviceManagerEventEmitter = mock(); + mockedDeviceManagerEventEmitter.emit.mockReturnValue(true); const mockedLogger = mock(); mockedLogger.child.mockReturnValue(mockedLogger); const deviceManager = new DeviceManager(mockedDeviceManagerEventEmitter, connectedDevices, mockedSettingsManager, mockedLogger); - deviceManager.addDevice({ type: 'test', detectionId: deviceId }, device); + await connectDevice(deviceManager, deviceInfo, device); + mockClear(mockedDeviceManagerEventEmitter); // Connected device closed await device.close(); expect(deviceManager.getConnectedDevices().length).toBe(0); - expect(mockedDeviceManagerEventEmitter.emit).toBeCalledTimes(2); - expect(mockedDeviceManagerEventEmitter.emit).toHaveBeenNthCalledWith(1, DeviceManagerEvent.deviceConnected, device); - expect(mockedDeviceManagerEventEmitter.emit).toHaveBeenNthCalledWith(2, DeviceManagerEvent.deviceDisconnected, device); + expect(mockedDeviceManagerEventEmitter.emit).toBeCalledTimes(1); + expect(mockedDeviceManagerEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDisconnected, device); expect(mockedLogger.child).toBeCalledWith({ name: DeviceManager.name }); }); @@ -166,11 +178,17 @@ describe('deviceManager', () => { manager.announceDetectedDevice(deviceInfo); - const result = await manager.acquireDetectedDevice(deviceId); + const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); expect(result.successful).toBe(false); }); - it('does not emit deviceDetected for a device belonging to a disabled known device', () => { + it('still emits deviceDetected even when the detection id matches a disabled known device', () => { + // Detection id is preliminary/raw - e.g. for serial ports, multiple protocol + // providers share the same detectionId but each computes its own distinct canonical + // id via handshake. Gating here on detectionId's own enabled state would incorrectly + // block every provider, including ones whose real canonical id isn't disabled at + // all. The disabled check that actually matters happens per-canonical-id in + // addDevice(), once a provider has connected and learned the final id. mockedEventEmitter.emit.mockReturnValue(true); const settings = new Settings(); @@ -182,11 +200,11 @@ describe('deviceManager', () => { manager.announceDetectedDevice(deviceInfo); - expect(mockedEventEmitter.emit).not.toHaveBeenCalled(); + expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); }); }); - describe('acquireDetectedDevice', () => { + describe('offerDevice', () => { let mockedLogger: ReturnType>; let mockedEventEmitter: ReturnType>; const deviceId = DeviceId.create('device-2'); @@ -199,64 +217,135 @@ describe('deviceManager', () => { mockedEventEmitter.emit.mockReturnValue(true); }); - it('returns failure when device is not in the detect queue', async () => { + it('rejects with DeviceOfferRejectedError when device is not in the detect queue', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - const result = await manager.acquireDetectedDevice(deviceId); + const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); }); - it('resolves immediately with success for the first caller', async () => { + it('runs the first offer immediately and adds the device on success', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - const result = await manager.acquireDetectedDevice(deviceId); + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(device)); - expect(result).toStrictEqual({ successful: true }); + expect(result).toStrictEqual({ successful: true, device }); + expect(manager.getConnectedDevices()).toContain(device); }); - it('queues the second caller until the first releases', async () => { + it('does not run a second offer while the first is still pending', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - await manager.acquireDetectedDevice(deviceId); - const secondCallerPromise = manager.acquireDetectedDevice(deviceId); - manager.releaseDetectedDevice(deviceId); + let resolveFirstOffer!: (device: AnyDevice | undefined) => void; + const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); + const secondOfferFn = vi.fn(() => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); - const result = await secondCallerPromise; - expect(result).toStrictEqual({ successful: true }); + const firstResultPromise = manager.offerDevice(deviceInfo, () => firstOfferPromise); + manager.offerDevice(deviceInfo, secondOfferFn); + + expect(secondOfferFn).not.toHaveBeenCalled(); + + resolveFirstOffer(undefined); + await firstResultPromise; }); - }); - describe('releaseDetectedDevice', () => { - let mockedLogger: ReturnType>; - let mockedEventEmitter: ReturnType>; - const deviceId = DeviceId.create('device-3'); - const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; + it('hands off to the next queued offer when the first one returns undefined', async () => { + const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + manager.announceDetectedDevice(deviceInfo); - beforeEach(() => { - mockedLogger = mock(); - mockedLogger.child.mockReturnValue(mockedLogger); - mockedEventEmitter = mock(); - mockedEventEmitter.emit.mockReturnValue(true); + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + + const firstResultPromise = manager.offerDevice(deviceInfo, () => Promise.resolve(undefined)); + const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.resolve(device)); + + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult.successful).toBe(false); + expect(secondResult).toStrictEqual({ successful: true, device }); }); - it('is a no-op when device is not in the acquire queue', () => { + it('hands off to the next queued offer when the first one throws', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + manager.announceDetectedDevice(deviceInfo); + + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const offerError = new Error('connection failed'); + + const firstResultPromise = manager.offerDevice(deviceInfo, () => Promise.reject(offerError)); + const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.resolve(device)); - expect(() => manager.releaseDetectedDevice(DeviceId.create('unknown'))).not.toThrow(); + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult).toStrictEqual({ successful: false, reason: offerError }); + expect(secondResult).toStrictEqual({ successful: true, device }); }); - it('removes device from queue after the only waiter releases', async () => { + it('hands off to the next queued offer when the first device is disabled', async () => { + // Detection id itself must stay enabled/unknown so announce() actually creates the + // queue - only the canonical id of the first offered device (learned only once + // connected, e.g. during a handshake) is disabled. + const localDetectionId = DeviceId.create('device-2-detection'); + const localDeviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: localDetectionId }; + const disabledCanonicalId = DeviceId.create('device-2-disabled-canonical'); + + const settings = new Settings(); + settings.addKnownDevice(new KnownDevice(disabledCanonicalId, 'Foo', 'test', 'test', {}, false)); + const settingsManager = mock(); + settingsManager.getSettings.mockReturnValue(settings); + + const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); + manager.announceDetectedDevice(localDeviceInfo); + + const disabledDevice = new TestDevice(disabledCanonicalId, 'Foo', new Date(), false, new EventEmitter()); + const enabledDevice = new TestDevice(DeviceId.create('device-2-enabled-canonical'), 'Foo', new Date(), false, new EventEmitter()); + + const firstResultPromise = manager.offerDevice(localDeviceInfo, () => Promise.resolve(disabledDevice)); + const secondResultPromise = manager.offerDevice(localDeviceInfo, () => Promise.resolve(enabledDevice)); + + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult.successful).toBe(false); + expect(!firstResult.successful && firstResult.reason).toBeInstanceOf(DeviceOfferRejectedError); + expect(secondResult).toStrictEqual({ successful: true, device: enabledDevice }); + }); + + it('clears the queue and re-allows announcing after the only offer fails', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - await manager.acquireDetectedDevice(deviceId); - manager.releaseDetectedDevice(deviceId); + await manager.offerDevice(deviceInfo, () => Promise.resolve(undefined)); - const result = await manager.acquireDetectedDevice(deviceId); - expect(result.successful).toBe(false); + mockClear(mockedEventEmitter); + mockedEventEmitter.emit.mockReturnValue(true); + + manager.announceDetectedDevice(deviceInfo); + + expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); + }); + + it('rejects other queued offers with DeviceOfferRejectedError once a device is claimed', async () => { + const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + manager.announceDetectedDevice(deviceInfo); + + let resolveFirstOffer!: (device: AnyDevice | undefined) => void; + const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + + const firstResultPromise = manager.offerDevice(deviceInfo, () => firstOfferPromise); + const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.reject(new Error('should never run'))); + + resolveFirstOffer(device); + + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult).toStrictEqual({ successful: true, device }); + expect(secondResult.successful).toBe(false); + expect(!secondResult.successful && secondResult.reason).toBeInstanceOf(DeviceOfferRejectedError); }); }); @@ -273,16 +362,18 @@ describe('deviceManager', () => { mockedEventEmitter.emit.mockReturnValue(true); }); - it('resolves a pending second caller with failure', async () => { + it('resolves a pending offer with failure', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - await manager.acquireDetectedDevice(deviceId); // first caller holds - const pendingPromise = manager.acquireDetectedDevice(deviceId); // second waits + + // First offer never settles on its own, so it's still holding the queue when revoked + const pendingPromise = manager.offerDevice(deviceInfo, () => new Promise(() => {})); manager.revokeDetectedDevice(deviceInfo); const result = await pendingPromise; expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); }); it('drops a disabled device from pending retry so it is not re-announced after re-enabling', async () => { @@ -294,9 +385,15 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); - // Announced while disabled -> parked in pending retry, no deviceDetected emitted. + // Announced (detection id happens to match the disabled known device) - the provider + // still gets a chance to offer it, but addDevice() rejects it once connected since + // it's disabled, parking it in pending retry. manager.announceDetectedDevice(deviceInfo); - expect(mockedEventEmitter.emit).not.toHaveBeenCalled(); + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(device)); + expect(result.successful).toBe(false); + + mockClear(mockedEventEmitter); // Device physically disappears while still disabled. manager.revokeDetectedDevice(deviceInfo); @@ -309,32 +406,6 @@ describe('deviceManager', () => { }); }); - describe('claimDetectedDevice', () => { - let mockedLogger: ReturnType>; - let mockedEventEmitter: ReturnType>; - const deviceId = DeviceId.create('device-5'); - const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; - - beforeEach(() => { - mockedLogger = mock(); - mockedLogger.child.mockReturnValue(mockedLogger); - mockedEventEmitter = mock(); - mockedEventEmitter.emit.mockReturnValue(true); - }); - - it('resolves a pending caller with failure', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - await manager.acquireDetectedDevice(deviceId); // first caller holds - const pendingPromise = manager.acquireDetectedDevice(deviceId); // second waits - - manager.claimDetectedDevice(deviceId); - - const result = await pendingPromise; - expect(result.successful).toBe(false); - }); - }); - describe('isDeviceEnabled', () => { let mockedLogger: ReturnType>; @@ -375,48 +446,60 @@ describe('deviceManager', () => { }); }); - describe('addDevice - disabled devices', () => { + describe('offerDevice - disabled devices', () => { let mockedLogger: ReturnType>; + let mockedEventEmitter: ReturnType>; beforeEach(() => { mockedLogger = mock(); mockedLogger.child.mockReturnValue(mockedLogger); + mockedEventEmitter = mock(); + mockedEventEmitter.emit.mockReturnValue(true); }); - it('does not register a device belonging to a disabled known device and closes it', () => { - const deviceId = DeviceId.create('disabled-device'); + it('does not register a device whose canonical id belongs to a disabled known device, and closes it', async () => { + // Detection id is unknown/enabled so announce() lets it through and the offer runs - + // the device only turns out to be disabled once its canonical id is learned, e.g. + // during a handshake. This is the only way to reach addDevice()'s own disabled-check + // through the public API now that it's private. + const detectionId = DeviceId.create('disabled-device-detection'); + const canonicalId = DeviceId.create('disabled-device-canonical'); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId }; + const settings = new Settings(); - settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, false)); + settings.addKnownDevice(new KnownDevice(canonicalId, 'Foo', 'test', 'test', {}, false)); const settingsManager = mock(); settingsManager.getSettings.mockReturnValue(settings); const connectedDevices = new Map(); - const manager = new DeviceManager(mock(), connectedDevices, settingsManager, mockedLogger); + const manager = new DeviceManager(mockedEventEmitter, connectedDevices, settingsManager, mockedLogger); - const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const device = new TestDevice(canonicalId, 'Foo', new Date(), false, new EventEmitter()); - const added = manager.addDevice({ type: 'test', detectionId: deviceId }, device); + const result = await connectDevice(manager, deviceInfo, device); - expect(added).toBe(false); + expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); expect(manager.getConnectedDevices()).toHaveLength(0); }); - it('registers a device belonging to an enabled known device', () => { + it('registers a device belonging to an enabled known device', async () => { const deviceId = DeviceId.create('enabled-device'); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; const settings = new Settings(); settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, true)); const settingsManager = mock(); settingsManager.getSettings.mockReturnValue(settings); - const manager = new DeviceManager(mock(), new Map(), settingsManager, mockedLogger); + const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - const added = manager.addDevice({ type: 'test', detectionId: deviceId }, device); + const result = await connectDevice(manager, deviceInfo, device); - expect(added).toBe(true); + expect(result.successful).toBe(true); expect(manager.getConnectedDevices()).toHaveLength(1); }); }); @@ -431,6 +514,7 @@ describe('deviceManager', () => { it('closes connected devices whose known device has been disabled', async () => { const deviceId = DeviceId.create('device-to-disable'); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; const enabledSettings = new Settings(); enabledSettings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, true)); @@ -438,10 +522,12 @@ describe('deviceManager', () => { settingsManager.getSettings.mockReturnValue(enabledSettings); const connectedDevices = new Map(); - const manager = new DeviceManager(mock(), connectedDevices, settingsManager, mockedLogger); + const mockedEventEmitter = mock(); + mockedEventEmitter.emit.mockReturnValue(true); + const manager = new DeviceManager(mockedEventEmitter, connectedDevices, settingsManager, mockedLogger); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - manager.addDevice({ type: 'test', detectionId: deviceId }, device); + await connectDevice(manager, deviceInfo, device); expect(manager.getConnectedDevices()).toHaveLength(1); const disabledSettings = new Settings(); @@ -455,6 +541,7 @@ describe('deviceManager', () => { it('leaves devices belonging to still-enabled known devices connected', async () => { const deviceId = DeviceId.create('device-still-enabled'); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; const settings = new Settings(); settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, true)); @@ -462,44 +549,19 @@ describe('deviceManager', () => { settingsManager.getSettings.mockReturnValue(settings); const connectedDevices = new Map(); - const manager = new DeviceManager(mock(), connectedDevices, settingsManager, mockedLogger); - - const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - manager.addDevice({ type: 'test', detectionId: deviceId }, device); - - await manager.onSettingsChanged(); - - expect(manager.getConnectedDevices()).toHaveLength(1); - }); - - it('re-announces a device rejected by announceDetectedDevice once its known device gets re-enabled', async () => { - const deviceId = DeviceId.create('device-pending-1'); - const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; - - const disabledSettings = new Settings(); - disabledSettings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, false)); - - const settingsManager = mock(); - settingsManager.getSettings.mockReturnValue(disabledSettings); - const mockedEventEmitter = mock(); mockedEventEmitter.emit.mockReturnValue(true); + const manager = new DeviceManager(mockedEventEmitter, connectedDevices, settingsManager, mockedLogger); - const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); - - manager.announceDetectedDevice(deviceInfo); - expect(mockedEventEmitter.emit).not.toHaveBeenCalled(); - - const enabledSettings = new Settings(); - enabledSettings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, true)); - settingsManager.getSettings.mockReturnValue(enabledSettings); + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + await connectDevice(manager, deviceInfo, device); await manager.onSettingsChanged(); - expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); + expect(manager.getConnectedDevices()).toHaveLength(1); }); - it('re-announces a device rejected by addDevice() only once its canonical known device gets re-enabled', async () => { + it('re-announces a device rejected by the offer only once its canonical known device gets re-enabled', async () => { // The device is detected under a preliminary id, but its final/canonical id (only // known after connecting, e.g. a serial number read during a handshake) is different. const detectionId = DeviceId.create('device-pending-2-detected'); @@ -521,8 +583,12 @@ describe('deviceManager', () => { // Simulate a provider that connected a device via the detected-device pipeline whose // final id turns out to belong to a disabled device. const device = new TestDevice(canonicalId, 'Foo', new Date(), false, new EventEmitter()); - const added = manager.addDevice(deviceInfo, device); - expect(added).toBe(false); + const result = await connectDevice(manager, deviceInfo, device); + expect(result.successful).toBe(false); + + // Drop the deviceDetected emit from announcing above - only the re-announce below is + // under test here, same as the original addDevice()-based version of this test. + mockClear(mockedEventEmitter); // An unrelated settings change while the canonical device is still disabled must NOT // retry it (it would if the retry were gated by the still-unknown detection id). @@ -552,7 +618,7 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - manager.addDevice(deviceInfo, device); + await connectDevice(manager, deviceInfo, device); mockClear(mockedEventEmitter); diff --git a/tests/unit/device/provider/deviceProvider.spec.ts b/tests/unit/device/provider/deviceProvider.spec.ts index 66dd89c4..ecadcca3 100644 --- a/tests/unit/device/provider/deviceProvider.spec.ts +++ b/tests/unit/device/provider/deviceProvider.spec.ts @@ -2,10 +2,12 @@ import { describe, expect, it, vi } from 'vitest'; import { mock } from 'vitest-mock-extended'; import EventEmitter from 'events'; import DeviceProvider from '../../../../src/device/provider/deviceProvider.js'; -import DeviceManager, { DeviceDetectionInfo } from '../../../../src/device/deviceManager.js'; +import DeviceManager, { DeviceDetectionInfo, DeviceManagerEvent } from '../../../../src/device/deviceManager.js'; import { AnyDevice } from '../../../../src/device/device.js'; import Logger from '../../../../src/logging/Logger.js'; import SettingsManager from '../../../../src/settings/settingsManager.js'; +import Settings from '../../../../src/settings/settings.js'; +import KnownDevice from '../../../../src/settings/knownDevice.js'; import { DeviceId } from '../../../../src/device/deviceId.js'; import TestDevice from '../testDevice.js'; @@ -50,6 +52,54 @@ class DetectingTestProvider extends DeviceProvider { return Promise.resolve(new TestDevice(deviceDetectionInfo.detectionId, 'Foo', new Date(), false, new EventEmitter())); } + + // Exposes the protected getConnectedDevice() so tests can check the provider's own + // bookkeeping directly, e.g. from within a deviceManager event listener. + public hasDeviceLocally(deviceId: DeviceId): boolean { + return undefined !== this.getConnectedDevice(deviceId); + } +} + +// createDevice() resolution is controlled from outside via the injected promise, to simulate a +// slow connect attempt that's still in flight when the provider gets stopped. +class SlowCreateDeviceProvider extends DeviceProvider +{ + public constructor(deviceManager: DeviceManager, private readonly createDevicePromise: Promise) { + super(deviceManager, new EventEmitter(), mock()); + } + + protected canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DeviceDetectionInfo { + return deviceDetectionInfo.type === 'test'; + } + + protected createDevice(_deviceDetectionInfo: DeviceDetectionInfo): Promise { + return this.createDevicePromise; + } +} + +// Tracks onConnectFailed() calls so tests can assert it does/doesn't run for a given rejection. +class TrackingTestProvider extends DeviceProvider +{ + public onConnectFailedCalls = 0; + + public constructor( + deviceManager: DeviceManager, + private readonly createDeviceFn: (deviceDetectionInfo: DeviceDetectionInfo) => Promise + ) { + super(deviceManager, new EventEmitter(), mock()); + } + + protected canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DeviceDetectionInfo { + return deviceDetectionInfo.type === 'test'; + } + + protected createDevice(deviceDetectionInfo: DeviceDetectionInfo): Promise { + return this.createDeviceFn(deviceDetectionInfo); + } + + protected override async onConnectFailed(_deviceDetectionInfo: DeviceDetectionInfo): Promise { + this.onConnectFailedCalls++; + } } describe('DeviceProvider', () => { @@ -125,4 +175,106 @@ describe('DeviceProvider', () => { await vi.waitFor(() => expect(deviceManager.getConnectedDevices()).toHaveLength(1)); }); }); + + describe('handleDeviceDetection', () => { + it('has the device in its own connected list by the time deviceManager emits deviceConnected', async () => { + const settingsManager = mock(); + settingsManager.getSettings.mockReturnValue(undefined); + + const logger = mock(); + logger.child.mockReturnValue(logger); + + const deviceManager = new DeviceManager(new EventEmitter(), new Map(), settingsManager, logger); + const provider = new DetectingTestProvider(deviceManager); + await provider.start(); + + const deviceId = DeviceId.create('device-race'); + let sawItLocallyOnConnect = false; + + deviceManager.on(DeviceManagerEvent.deviceConnected, (device) => { + if (device.getDeviceId === deviceId) { + sawItLocallyOnConnect = provider.hasDeviceLocally(deviceId); + } + }); + + deviceManager.announceDetectedDevice({ type: 'test', detectionId: deviceId }); + + await vi.waitFor(() => expect(deviceManager.getConnectedDevices()).toHaveLength(1)); + + expect(sawItLocallyOnConnect).toBe(true); + }); + + it('closes and does not register a device that connects after the provider was stopped', async () => { + const settingsManager = mock(); + settingsManager.getSettings.mockReturnValue(undefined); + + const logger = mock(); + logger.child.mockReturnValue(logger); + + const deviceManager = new DeviceManager(new EventEmitter(), new Map(), settingsManager, logger); + + let resolveCreateDevice!: (device: AnyDevice | undefined) => void; + const createDevicePromise = new Promise((resolve) => { resolveCreateDevice = resolve; }); + + const provider = new SlowCreateDeviceProvider(deviceManager, createDevicePromise); + await provider.start(); + + deviceManager.announceDetectedDevice({ type: 'test', detectionId: DeviceId.create('device-stopped') }); + + // Provider is stopped while createDevice() is still pending + await provider.stop(); + + const device = new TestDevice(DeviceId.create('device-stopped'), 'Foo', new Date(), false, new EventEmitter()); + const closeSpy = vi.spyOn(device, 'close'); + + resolveCreateDevice(device); + + await vi.waitFor(() => expect(closeSpy).toHaveBeenCalled()); + + expect(deviceManager.getConnectedDevices()).toHaveLength(0); + }); + + it('calls onConnectFailed when the offer itself throws', async () => { + const settingsManager = mock(); + settingsManager.getSettings.mockReturnValue(undefined); + + const logger = mock(); + logger.child.mockReturnValue(logger); + + const deviceManager = new DeviceManager(new EventEmitter(), new Map(), settingsManager, logger); + const provider = new TrackingTestProvider(deviceManager, () => Promise.reject(new Error('connect failed'))); + + await provider.start(); + deviceManager.announceDetectedDevice({ type: 'test', detectionId: DeviceId.create('device-throw') }); + + await vi.waitFor(() => expect(provider.onConnectFailedCalls).toBe(1)); + }); + + it('does not call onConnectFailed when the device is rejected for being disabled', async () => { + const deviceId = DeviceId.create('device-disabled-oncf'); + const settings = new Settings(); + settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, false)); + + const settingsManager = mock(); + settingsManager.getSettings.mockReturnValue(settings); + + const logger = mock(); + logger.child.mockReturnValue(logger); + + const deviceManager = new DeviceManager(new EventEmitter(), new Map(), settingsManager, logger); + const provider = new TrackingTestProvider( + deviceManager, + (deviceDetectionInfo) => Promise.resolve(new TestDevice(deviceDetectionInfo.detectionId, 'Foo', new Date(), false, new EventEmitter())) + ); + + await provider.start(); + deviceManager.announceDetectedDevice({ type: 'test', detectionId: deviceId }); + + // Flush the announce -> offer -> addDevice -> resolve microtask chain before asserting + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(deviceManager.getConnectedDevices()).toHaveLength(0); + expect(provider.onConnectFailedCalls).toBe(0); + }); + }); }); From 777a9b99e050bdd5d7b2d46e008d016d7e6fe481 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 08:26:31 +0200 Subject: [PATCH 03/24] refactor: convert runNextInQueue to async/await Replace .then()/.catch() chaining with a single try/await/catch block - same behavior and microtask timing, just more linear to read. advanceQueue() stays synchronous since it has no async work of its own; its recursive call into runNextInQueue() and the executor call site in offerDevice() are marked void, matching the existing fire-and-forget convention used elsewhere in the codebase. --- src/device/deviceManager.ts | 46 ++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index cdbd09a2..c1e113f1 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -125,7 +125,7 @@ export default class DeviceManager // If we're first in line, run our offer immediately if (deviceQueue.length === 1) { - this.runNextInQueue(deviceQueue); + void this.runNextInQueue(deviceQueue); } }); } @@ -254,7 +254,7 @@ export default class DeviceManager } } - private runNextInQueue(deviceQueue: QueueEntry[]): void + private async runNextInQueue(deviceQueue: QueueEntry[]): Promise { const entry = deviceQueue[0]; @@ -264,29 +264,29 @@ export default class DeviceManager const detectionId = entry.deviceDetectionInfo.detectionId; - entry.deviceOffer() - .then((device) => { - if (device === undefined) { - entry.resolve({ successful: false, reason: new Error(`Device offer for '${detectionId}' returned undefined`) }); - this.advanceQueue(detectionId, deviceQueue); - return; - } + try { + const device = await entry.deviceOffer(); - const added = this.addDevice(entry.deviceDetectionInfo, device); + if (undefined === device) { + entry.resolve({ successful: false, reason: new Error(`Device offer for '${detectionId}' returned undefined`) }); + this.advanceQueue(detectionId, deviceQueue); + return; + } - if (added) { - entry.resolve({ successful: true, device }); - this.clearDetectedDeviceAcquireQueue(detectionId, `Device '${detectionId}' has been claimed by another provider`); - return; - } + const added = this.addDevice(entry.deviceDetectionInfo, device); - entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`) }); - this.advanceQueue(detectionId, deviceQueue); - }) - .catch((e: unknown) => { - entry.resolve({ successful: false, reason: e }); - this.advanceQueue(detectionId, deviceQueue); - }); + if (added) { + entry.resolve({ successful: true, device }); + this.clearDetectedDeviceAcquireQueue(detectionId, `Device '${detectionId}' has been claimed by another provider`); + return; + } + + entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`) }); + this.advanceQueue(detectionId, deviceQueue); + } catch (e: unknown) { + entry.resolve({ successful: false, reason: e }); + this.advanceQueue(detectionId, deviceQueue); + } } /** @@ -303,7 +303,7 @@ export default class DeviceManager return; } - this.runNextInQueue(deviceQueue); + void this.runNextInQueue(deviceQueue); } private clearDetectedDeviceAcquireQueue(deviceId: string, reason: string): void From aba609f3b22341eb3a52f7f6f1a5f488e2d1d711 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 08:57:23 +0200 Subject: [PATCH 04/24] refactor: clean up deviceManager/deviceProvider leftovers - Inline registerPendingRetry() into addDevice(), its only caller now that announceDetectedDevice() no longer has its own disabled-gate call site. - Dedup the repeated resolve-failure+advance-queue pairing in runNextInQueue() into a local rejectAndAdvance() closure. - Drop the unnecessary async modifier from offerDevice() - its body just constructs and returns a Promise directly, no await needed. - Inline the trivial single-use refreshDevice()/removeDevice() helpers directly into their event listener registrations in addDevice(). - Reword advanceQueue()'s doc comment to drop the reference to the no-longer-existing releaseDetectedDevice(). - Rename leftover acquire/claim terminology to match the offer-based model: AcquireResult -> OfferResult, detectedDeviceAcquireQueue -> detectedDeviceOfferQueue, clearDetectedDeviceAcquireQueue -> clearDetectedDeviceOfferQueue, plus matching comment/log wording. - Remove the dead eventEmitter field from the DeviceProvider base class - assigned in the constructor but never read anywhere in the base class or any of its ~8 subclasses. Threaded the removal through every provider subclass, the two dedicated provider factories (ButtplugIoWebsocketDeviceProviderFactory, VirtualDeviceProviderFactory, whose own eventEmitter/eventEmitterFactory fields became dead in turn), the DI wiring in deviceServiceProvider.ts, and the local test provider classes in deviceProvider.spec.ts/deviceProviderManager.spec.ts. --- src/device/deviceManager.ts | 88 ++++++++----------- .../protocol/airotic/airoticDeviceProvider.ts | 4 +- .../buttplugIoWebsocketDeviceProvider.ts | 4 +- ...uttplugIoWebsocketDeviceProviderFactory.ts | 6 -- .../estim2b/estim2bSerialDeviceProvider.ts | 4 +- .../slvCtrlPlusSerialDeviceProvider.ts | 4 +- .../protocol/virtual/virtualDeviceProvider.ts | 4 +- .../virtual/virtualDeviceProviderFactory.ts | 6 -- .../protocol/zc95/zc95SerialDeviceProvider.ts | 4 +- src/device/provider/bleDeviceProvider.ts | 5 +- src/device/provider/deviceProvider.ts | 8 +- src/device/provider/serialDeviceProvider.ts | 4 +- src/serviceProvider/deviceServiceProvider.ts | 6 -- .../device/provider/deviceProvider.spec.ts | 8 +- .../provider/deviceProviderManager.spec.ts | 2 +- 15 files changed, 53 insertions(+), 104 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index c1e113f1..3a04774a 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -22,14 +22,14 @@ export enum DeviceManagerEvent { deviceNotification = 'deviceNotification', } -type AcquireResult = +type OfferResult = | { successful: true, device: D } | { successful: false, reason: unknown }; type QueueEntry = { deviceDetectionInfo: DeviceDetectionInfo; deviceOffer: () => Promise; - resolve: (result: AcquireResult) => void; + resolve: (result: OfferResult) => void; }; type DeviceManagerEventMap = { @@ -46,7 +46,7 @@ export default class DeviceManager private readonly logger: Logger; - private readonly detectedDeviceAcquireQueue: Map[]> = new Map(); + private readonly detectedDeviceOfferQueue: Map[]> = new Map(); private readonly connectedDevices: Map; @@ -81,7 +81,7 @@ export default class DeviceManager public announceDetectedDevice(deviceInfo: DeviceDetectionInfo): void { - if (this.detectedDeviceAcquireQueue.has(deviceInfo.detectionId)) { + if (this.detectedDeviceOfferQueue.has(deviceInfo.detectionId)) { return; } @@ -92,14 +92,14 @@ export default class DeviceManager this.logger.info(`Detected new device with id ${deviceInfo.detectionId}`); - this.detectedDeviceAcquireQueue.set(deviceInfo.detectionId, []); + this.detectedDeviceOfferQueue.set(deviceInfo.detectionId, []); const hadListeners = this.eventEmitter.emit(DeviceManagerEvent.deviceDetected, deviceInfo); if (!hadListeners) { - // no subscribed providers, remove empty list from acquire queue for this device + // no subscribed providers, remove empty list from offer queue for this device this.logger.info(`No provider available for detected device with id '${deviceInfo.detectionId}'`); - this.detectedDeviceAcquireQueue.delete(deviceInfo.detectionId); + this.detectedDeviceOfferQueue.delete(deviceInfo.detectionId); } } @@ -107,16 +107,16 @@ export default class DeviceManager { // A device that physically disappeared should no longer be retried on re-enable this.pendingRetries.delete(deviceInfo.detectionId); - this.clearDetectedDeviceAcquireQueue(deviceInfo.detectionId, `Device with id '${deviceInfo.detectionId}' has disappeared`); + this.clearDetectedDeviceOfferQueue(deviceInfo.detectionId, `Device with id '${deviceInfo.detectionId}' has disappeared`); } - public async offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> + public offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { - return new Promise>((resolve) => { - const deviceQueue = this.detectedDeviceAcquireQueue.get(deviceDetectionInfo.detectionId); + return new Promise>((resolve) => { + const deviceQueue = this.detectedDeviceOfferQueue.get(deviceDetectionInfo.detectionId); if (undefined === deviceQueue) { - resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is not available anymore for claiming`) }); + resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is not available anymore for offering`) }); return; } @@ -138,15 +138,19 @@ export default class DeviceManager const closingDevice = device.close() .catch((e: unknown) => logError(this.logger, `Failed to close disabled device '${device.getDeviceId}'`, e)); - this.registerPendingRetry(deviceInfo, device.getDeviceId, closingDevice); + // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it + this.pendingRetries.set(deviceInfo.detectionId, { deviceInfo, canonicalId: device.getDeviceId, closingDevice }); return false; } this.connectedDevices.set(device.getDeviceId, device); - device.on(DeviceEvent.deviceRefreshed, (d) => this.refreshDevice(d)); - device.on(DeviceEvent.deviceDisconnected, (d) => this.removeDevice(d)); + device.on(DeviceEvent.deviceRefreshed, (d) => this.eventEmitter.emit(DeviceManagerEvent.deviceRefreshed, d)); + device.on(DeviceEvent.deviceDisconnected, (d) => { + this.connectedDevices.delete(d.getDeviceId); + this.eventEmitter.emit(DeviceManagerEvent.deviceDisconnected, d); + }); device.on(DeviceEvent.deviceNotification, (d, notification) => this.eventEmitter.emit(DeviceManagerEvent.deviceNotification, d, notification)); this.initDeviceRefresher(device); @@ -156,15 +160,6 @@ export default class DeviceManager return true; } - // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it - private registerPendingRetry( - deviceInfo: DeviceDetectionInfo, - canonicalId: DeviceId, - closingDevice?: Promise - ): void { - this.pendingRetries.set(deviceInfo.detectionId, { deviceInfo, canonicalId, closingDevice }); - } - public async onSettingsChanged(): Promise { await this.settingsChangeQueue.push(() => this.applySettingsChange()); } @@ -243,8 +238,8 @@ export default class DeviceManager } } - for (const [deviceId] of this.detectedDeviceAcquireQueue) { - this.clearDetectedDeviceAcquireQueue(deviceId, 'Device manager reset'); + for (const [deviceId] of this.detectedDeviceOfferQueue) { + this.clearDetectedDeviceOfferQueue(deviceId, 'Device manager reset'); } this.pendingRetries.clear(); @@ -264,12 +259,16 @@ export default class DeviceManager const detectionId = entry.deviceDetectionInfo.detectionId; + const rejectAndAdvance = (reason: unknown): void => { + entry.resolve({ successful: false, reason }); + this.advanceQueue(detectionId, deviceQueue); + }; + try { const device = await entry.deviceOffer(); if (undefined === device) { - entry.resolve({ successful: false, reason: new Error(`Device offer for '${detectionId}' returned undefined`) }); - this.advanceQueue(detectionId, deviceQueue); + rejectAndAdvance(new Error(`Device offer for '${detectionId}' returned undefined`)); return; } @@ -277,42 +276,40 @@ export default class DeviceManager if (added) { entry.resolve({ successful: true, device }); - this.clearDetectedDeviceAcquireQueue(detectionId, `Device '${detectionId}' has been claimed by another provider`); + this.clearDetectedDeviceOfferQueue(detectionId, `Device '${detectionId}' has been claimed by another provider`); return; } - entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`) }); - this.advanceQueue(detectionId, deviceQueue); + rejectAndAdvance(new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`)); } catch (e: unknown) { - entry.resolve({ successful: false, reason: e }); - this.advanceQueue(detectionId, deviceQueue); + rejectAndAdvance(e); } } /** - * Drops the just-settled entry and hands off to the next waiter, if any - mirrors the old - * releaseDetectedDevice() hand-off. Deletes the queue entirely once empty so the device can - * be re-announced (announceDetectedDevice() gates on the map key existing). + * Drops the just-settled entry and hands off to the next waiter, if any. Deletes the queue + * entirely once empty so the device can be re-announced (announceDetectedDevice() gates on + * the map key existing). */ private advanceQueue(detectionId: string, deviceQueue: QueueEntry[]): void { deviceQueue.shift(); if (deviceQueue.length === 0) { - this.detectedDeviceAcquireQueue.delete(detectionId); + this.detectedDeviceOfferQueue.delete(detectionId); return; } void this.runNextInQueue(deviceQueue); } - private clearDetectedDeviceAcquireQueue(deviceId: string, reason: string): void + private clearDetectedDeviceOfferQueue(deviceId: string, reason: string): void { - for (const entry of this.detectedDeviceAcquireQueue.get(deviceId) ?? []) { + for (const entry of this.detectedDeviceOfferQueue.get(deviceId) ?? []) { entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(reason) }); } - this.detectedDeviceAcquireQueue.delete(deviceId); + this.detectedDeviceOfferQueue.delete(deviceId); } private initDeviceRefresher(device: AnyDevice): void { @@ -342,15 +339,4 @@ export default class DeviceManager device.on(DeviceEvent.deviceDisconnected, () => deviceRefreshInterval.clear()); } - - private removeDevice(device: AnyDevice): void - { - this.connectedDevices.delete(device.getDeviceId); - this.eventEmitter.emit(DeviceManagerEvent.deviceDisconnected, device); - } - - private refreshDevice(device: AnyDevice): void - { - this.eventEmitter.emit(DeviceManagerEvent.deviceRefreshed, device); - } } diff --git a/src/device/protocol/airotic/airoticDeviceProvider.ts b/src/device/protocol/airotic/airoticDeviceProvider.ts index ede27b87..dbdb5aae 100644 --- a/src/device/protocol/airotic/airoticDeviceProvider.ts +++ b/src/device/protocol/airotic/airoticDeviceProvider.ts @@ -1,4 +1,3 @@ -import EventEmitter from 'events'; import BaseError from 'modern-errors'; import DeviceManager from '../../deviceManager.js'; import AiroticDevice from './airoticDevice.js'; @@ -24,10 +23,9 @@ export default class AiroticDeviceProvider extends BleDeviceProvider { private readonly deviceManager: DeviceManager; - private readonly eventEmitterFactory: EventEmitterFactory; - private readonly deviceFactory: VirtualDeviceFactory; private readonly settingsManager: SettingsManager; @@ -20,13 +17,11 @@ export default class VirtualDeviceProviderFactory implements DeviceProviderFacto public constructor( deviceManager: DeviceManager, - eventEmitterFactory: EventEmitterFactory, deviceFactory: VirtualDeviceFactory, settingsManager: SettingsManager, logger: Logger ) { this.deviceManager = deviceManager; - this.eventEmitterFactory = eventEmitterFactory; this.deviceFactory = deviceFactory; this.settingsManager = settingsManager; this.logger = logger; @@ -35,7 +30,6 @@ export default class VirtualDeviceProviderFactory implements DeviceProviderFacto public create(): VirtualDeviceProvider { return new VirtualDeviceProvider( this.deviceManager, - this.eventEmitterFactory.create(), this.deviceFactory, this.settingsManager, this.logger, diff --git a/src/device/protocol/zc95/zc95SerialDeviceProvider.ts b/src/device/protocol/zc95/zc95SerialDeviceProvider.ts index 52174b91..4a05b566 100644 --- a/src/device/protocol/zc95/zc95SerialDeviceProvider.ts +++ b/src/device/protocol/zc95/zc95SerialDeviceProvider.ts @@ -1,6 +1,5 @@ import { SerialPortStream } from '@serialport/stream'; import { BindingInterface } from '@serialport/bindings-interface'; -import EventEmitter from 'events'; import Logger from '../../../logging/Logger.js'; import SerialDeviceProvider, { SerialDeviceProviderPortOpenOptions } from '../../provider/serialDeviceProvider.js'; import Zc95DeviceFactory from './zc95DeviceFactory.js'; @@ -28,11 +27,10 @@ export default class Zc95SerialDeviceProvider extends SerialDeviceProvider extends { private readonly bleObserver: BleObserver; - protected constructor(deviceManager: DeviceManager, bleObserver: BleObserver, eventEmitter: EventEmitter, logger: Logger) { - super(deviceManager, eventEmitter, logger); + protected constructor(deviceManager: DeviceManager, bleObserver: BleObserver, logger: Logger) { + super(deviceManager, logger); this.bleObserver = bleObserver; } diff --git a/src/device/provider/deviceProvider.ts b/src/device/provider/deviceProvider.ts index 511564a7..dd2def51 100644 --- a/src/device/provider/deviceProvider.ts +++ b/src/device/provider/deviceProvider.ts @@ -1,4 +1,3 @@ -import EventEmitter from 'events'; import DeviceManager, { DeviceDetectionInfo, DeviceManagerEvent } from '../deviceManager.js'; import DeviceOfferRejectedError from '../deviceOfferRejectedError.js'; import Logger from '../../logging/Logger.js'; @@ -14,8 +13,6 @@ export default abstract class DeviceProvider = new Map(); @@ -24,9 +21,8 @@ export default abstract class DeviceProvider { const device = await this.createDevice(deviceDetectionInfo); diff --git a/src/device/provider/serialDeviceProvider.ts b/src/device/provider/serialDeviceProvider.ts index f6954b7d..41e48a38 100644 --- a/src/device/provider/serialDeviceProvider.ts +++ b/src/device/provider/serialDeviceProvider.ts @@ -1,5 +1,4 @@ import DeviceProvider from './deviceProvider.js'; -import EventEmitter from 'events'; import Logger from '../../logging/Logger.js'; import { BindingInterface, PortInfo } from '@serialport/bindings-interface'; import { SerialPortOpenOptions } from 'serialport'; @@ -24,10 +23,9 @@ export default abstract class SerialDeviceProvider new ButtplugIoWebsocketDeviceProviderFactory( container.get('device.manager'), - container.get('factory.eventEmitter').create(), container.get('device.serial.factory.buttplugIo'), container.get('logger.default'), ) @@ -138,7 +136,6 @@ export default class DeviceServiceProvider implements ServiceProvider new VirtualDeviceProviderFactory( container.get('device.manager'), - container.get('factory.eventEmitter'), container.get('device.virtual.factory'), container.get('settings.manager'), container.get('logger.default'), @@ -223,7 +220,6 @@ export default class DeviceServiceProvider implements ServiceProvider public doStopCalls = 0; public constructor(deviceManager: DeviceManager = mock()) { - super(deviceManager, new EventEmitter(), mock()); + super(deviceManager, mock()); } protected canHandleDeviceDetectionInfo(_deviceDetectionInfo: DeviceDetectionInfo): _deviceDetectionInfo is DeviceDetectionInfo { @@ -42,7 +42,7 @@ class TestProvider extends DeviceProvider class DetectingTestProvider extends DeviceProvider { public constructor(deviceManager: DeviceManager) { - super(deviceManager, new EventEmitter(), mock()); + super(deviceManager, mock()); } protected canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DeviceDetectionInfo { @@ -65,7 +65,7 @@ class DetectingTestProvider extends DeviceProvider { public constructor(deviceManager: DeviceManager, private readonly createDevicePromise: Promise) { - super(deviceManager, new EventEmitter(), mock()); + super(deviceManager, mock()); } protected canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DeviceDetectionInfo { @@ -86,7 +86,7 @@ class TrackingTestProvider extends DeviceProvider Promise ) { - super(deviceManager, new EventEmitter(), mock()); + super(deviceManager, mock()); } protected canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DeviceDetectionInfo { diff --git a/tests/unit/device/provider/deviceProviderManager.spec.ts b/tests/unit/device/provider/deviceProviderManager.spec.ts index c1e6a99d..8cb5758e 100644 --- a/tests/unit/device/provider/deviceProviderManager.spec.ts +++ b/tests/unit/device/provider/deviceProviderManager.spec.ts @@ -22,7 +22,7 @@ class RecordingDeviceProvider extends DeviceProvider = Promise.resolve(); public constructor() { - super(mock(), new EventEmitter(), mock()); + super(mock(), mock()); } public setStartGate(gate: Promise): void { From d3a9448eec355767eba197761707ff55aec06e51 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 20:08:46 +0200 Subject: [PATCH 05/24] Remove unused imports --- tests/integration/deviceEvents.spec.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/integration/deviceEvents.spec.ts b/tests/integration/deviceEvents.spec.ts index 4d8af9f3..acb18713 100644 --- a/tests/integration/deviceEvents.spec.ts +++ b/tests/integration/deviceEvents.spec.ts @@ -1,12 +1,10 @@ -import { afterAll, assert, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { DeviceManagerEvent } from '../../src/device/deviceManager.js'; import { AnyDevice } from '../../src/device/device.js'; import { AttributeValue } from '../../src/device/attribute/deviceAttribute.js'; import Settings from '../../src/settings/settings.js'; import KnownDevice from '../../src/settings/knownDevice.js'; import DeviceSource from '../../src/settings/deviceSource.js'; -import RandomGeneratorVirtualDeviceLogic from '../../src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.js'; -import VirtualDevice from '../../src/device/protocol/virtual/virtualDevice.js'; import { TEST_DEVICE_ID, TEST_SOURCE_ID, From fad4703ae53fea05cabb0cf24c73d1e9620fb902 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 20:27:10 +0200 Subject: [PATCH 06/24] fix: reject stale offers that settle after a revoke/reset (coderabbit) Addresses CodeRabbit review threads on PR #99: - runNextInQueue(): after a device offer resolves, verify the manager's offer queue for this detection id still points at the same queue array before calling addDevice(). Without this, an offer that settles with a device *after* revokeDetectedDevice()/reset() already cleared the queue and resolved the caller with a rejection would still get registered via addDevice() - connecting a device for hardware that's already known to be gone, with nothing left to close it. If the queue was cleared/replaced, the device is now closed instead and no state is touched. - advanceQueue(): same staleness check, applied more broadly. Without it, a stale failure (undefined/thrown/disabled-rejected) settling after a revoke/reset could shift()/delete() a completely different, currently-legitimate queue for the same detection id (e.g. one created by a fresh re-announce), corrupting unrelated in-progress detections. - Two new deviceManager.spec.ts tests, both confirmed to fail without this fix: one proving a stale offer's device gets closed and never registered, another proving a stale advanceQueue() call doesn't wipe a fresh, still-pending queue for the same detection id. --- src/device/deviceManager.ts | 15 ++++++ tests/unit/device/deviceManager.spec.ts | 70 +++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 3a04774a..7015ebb5 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -272,6 +272,15 @@ export default class DeviceManager return; } + // The queue may have been cleared (revoke/reset) or replaced (a fresh announce for + // the same detection id) while this offer was in flight - the caller already got a + // settled result for the old queue, so don't hand it a device it never asked for. + if (this.detectedDeviceOfferQueue.get(detectionId) !== deviceQueue) { + await device.close() + .catch((e: unknown) => logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e)); + return; + } + const added = this.addDevice(entry.deviceDetectionInfo, device); if (added) { @@ -293,6 +302,12 @@ export default class DeviceManager */ private advanceQueue(detectionId: string, deviceQueue: QueueEntry[]): void { + // The queue may already have been cleared/replaced (revoke, reset, or a fresh announce + // for the same detection id) - don't shift/delete a queue we no longer own. + if (this.detectedDeviceOfferQueue.get(detectionId) !== deviceQueue) { + return; + } + deviceQueue.shift(); if (deviceQueue.length === 0) { diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index f0d89da0..58d18f1d 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -376,6 +376,76 @@ describe('deviceManager', () => { expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); }); + it('closes a device whose offer resolves after the queue was revoked, without registering it', async () => { + const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + manager.announceDetectedDevice(deviceInfo); + + let resolveOffer!: (device: AnyDevice | undefined) => void; + const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); + const resultPromise = manager.offerDevice(deviceInfo, () => offerPromise); + + // Device physically disappears while the offer is still in flight. + manager.revokeDetectedDevice(deviceInfo); + + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const closeSpy = vi.spyOn(device, 'close'); + + // The offer only settles now, after the queue was already cleared - the caller + // already got a rejected result above, so this device must never be registered. + resolveOffer(device); + + const result = await resultPromise; + expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); + + await vi.waitFor(() => expect(closeSpy).toHaveBeenCalled()); + expect(manager.getConnectedDevices()).toHaveLength(0); + }); + + it('does not corrupt a fresh, still-pending queue when a stale offer fails after a revoke', async () => { + const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + manager.announceDetectedDevice(deviceInfo); + + let rejectStaleOffer!: (reason: unknown) => void; + const staleOfferPromise = new Promise((_resolve, reject) => { rejectStaleOffer = reject; }); + const staleResultPromise = manager.offerDevice(deviceInfo, () => staleOfferPromise); + + // Device physically disappears while the stale offer is still in flight. + manager.revokeDetectedDevice(deviceInfo); + + // Re-announced under the same detection id (e.g. redetected) - a fresh queue now + // exists, with its own still-pending offer. + manager.announceDetectedDevice(deviceInfo); + + let resolveFreshOffer!: (device: AnyDevice | undefined) => void; + const freshOfferPromise = new Promise((resolve) => { resolveFreshOffer = resolve; }); + const freshResultPromise = manager.offerDevice(deviceInfo, () => freshOfferPromise); + + // The stale offer only fails now, well after it was revoked and superseded - while + // the fresh offer is still pending. Without the advanceQueue() staleness guard, this + // would incorrectly delete the map entry currently pointing at the fresh, still-in- + // flight queue. + rejectStaleOffer(new Error('stale offer failed')); + await staleResultPromise; + + // A second provider trying the same detection id right now must still be queued up + // behind the fresh offer, not told the device is unavailable (which is what would + // happen if the stale advanceQueue() call had wrongly wiped the still-valid queue). + const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.reject(new Error('should never run'))); + + const freshDevice = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + resolveFreshOffer(freshDevice); + + const [freshResult, secondResult] = await Promise.all([freshResultPromise, secondResultPromise]); + + expect(freshResult).toStrictEqual({ successful: true, device: freshDevice }); + expect(secondResult.successful).toBe(false); + // Specifically "claimed by another provider" (queued behind the still-valid fresh + // queue) - not "not available anymore for offering", which would mean the queue was + // wrongly wiped by the stale advanceQueue() call. + expect(!secondResult.successful && (secondResult.reason as Error).message).toContain('claimed by another provider'); + }); + it('drops a disabled device from pending retry so it is not re-announced after re-enabling', async () => { const settings = new Settings(); settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, false)); From 842582e3e30ab05cb0ffb22f8bc9690b95b65692 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 20:52:26 +0200 Subject: [PATCH 07/24] refactor: improve naming clarity in deviceManager.ts - clearDetectedDeviceOfferQueue()'s parameter and reset()'s loop variable were named deviceId but actually hold a detection id (matching every other use of this concept in the file) - renamed to detectionId to avoid confusion with the canonical device id that deviceId means everywhere else (isDeviceEnabled(), getConnectedDevice()). - Standardized on deviceDetectionInfo for every DeviceDetectionInfo-typed symbol (was inconsistently deviceInfo in some places, deviceDetectionInfo in others): announceDetectedDevice(), revokeDetectedDevice(), addDevice(), the pendingRetries map's value field, applySettingsChange()'s destructuring, and the deviceDetected event's tuple label. - Renamed detectedDeviceOfferQueue to detectedDeviceOfferQueues (plural) - it's a map of one independent queue per detection id, not a single queue. All internal/private renames - no public signature changes, no test changes needed. --- src/device/deviceManager.ts | 58 ++++++++++++++++++------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 7015ebb5..1f746b23 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -36,7 +36,7 @@ type DeviceManagerEventMap = { [DeviceManagerEvent.deviceConnected]: [device: AnyDevice]; [DeviceManagerEvent.deviceDisconnected]: [device: AnyDevice]; [DeviceManagerEvent.deviceRefreshed]: [device: AnyDevice]; - [DeviceManagerEvent.deviceDetected]: [deviceInfo: DeviceDetectionInfo]; + [DeviceManagerEvent.deviceDetected]: [deviceDetectionInfo: DeviceDetectionInfo]; [DeviceManagerEvent.deviceNotification]: [device: AnyDevice, notification: DeviceNotification]; } @@ -46,7 +46,7 @@ export default class DeviceManager private readonly logger: Logger; - private readonly detectedDeviceOfferQueue: Map[]> = new Map(); + private readonly detectedDeviceOfferQueues: Map[]> = new Map(); private readonly connectedDevices: Map; @@ -56,9 +56,9 @@ export default class DeviceManager * Devices whose retry is pending because their known device is disabled; re-announced once * it gets (re-)enabled, see `onSettingsChanged()`. `canonicalId` is the device's final id * whose enablement gates the retry (protocols may only learn it during a handshake, so it - * can differ from the map key, the preliminary `deviceInfo.detectionId`). + * can differ from the map key, the preliminary `deviceDetectionInfo.detectionId`). */ - private readonly pendingRetries: Map }> = new Map(); + private readonly pendingRetries: Map }> = new Map(); // Serializes onSettingsChanged() runs so rapid settings changes don't interleave private readonly settingsChangeQueue: SequentialTaskQueue = new SequentialTaskQueue(); @@ -79,41 +79,41 @@ export default class DeviceManager return this.settingsManager.getSettings()?.getKnownDeviceById(deviceId)?.enabled ?? true; } - public announceDetectedDevice(deviceInfo: DeviceDetectionInfo): void + public announceDetectedDevice(deviceDetectionInfo: DeviceDetectionInfo): void { - if (this.detectedDeviceOfferQueue.has(deviceInfo.detectionId)) { + if (this.detectedDeviceOfferQueues.has(deviceDetectionInfo.detectionId)) { return; } - if (this.connectedDevices.has(deviceInfo.detectionId)) { - this.logger.debug(`Device with id '${deviceInfo.detectionId}' is already connected, not announcing it as detected`); + if (this.connectedDevices.has(deviceDetectionInfo.detectionId)) { + this.logger.debug(`Device with id '${deviceDetectionInfo.detectionId}' is already connected, not announcing it as detected`); return; } - this.logger.info(`Detected new device with id ${deviceInfo.detectionId}`); + this.logger.info(`Detected new device with id ${deviceDetectionInfo.detectionId}`); - this.detectedDeviceOfferQueue.set(deviceInfo.detectionId, []); + this.detectedDeviceOfferQueues.set(deviceDetectionInfo.detectionId, []); - const hadListeners = this.eventEmitter.emit(DeviceManagerEvent.deviceDetected, deviceInfo); + const hadListeners = this.eventEmitter.emit(DeviceManagerEvent.deviceDetected, deviceDetectionInfo); if (!hadListeners) { // no subscribed providers, remove empty list from offer queue for this device - this.logger.info(`No provider available for detected device with id '${deviceInfo.detectionId}'`); - this.detectedDeviceOfferQueue.delete(deviceInfo.detectionId); + this.logger.info(`No provider available for detected device with id '${deviceDetectionInfo.detectionId}'`); + this.detectedDeviceOfferQueues.delete(deviceDetectionInfo.detectionId); } } - public revokeDetectedDevice(deviceInfo: DeviceDetectionInfo): void + public revokeDetectedDevice(deviceDetectionInfo: DeviceDetectionInfo): void { // A device that physically disappeared should no longer be retried on re-enable - this.pendingRetries.delete(deviceInfo.detectionId); - this.clearDetectedDeviceOfferQueue(deviceInfo.detectionId, `Device with id '${deviceInfo.detectionId}' has disappeared`); + this.pendingRetries.delete(deviceDetectionInfo.detectionId); + this.clearDetectedDeviceOfferQueue(deviceDetectionInfo.detectionId, `Device with id '${deviceDetectionInfo.detectionId}' has disappeared`); } public offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { return new Promise>((resolve) => { - const deviceQueue = this.detectedDeviceOfferQueue.get(deviceDetectionInfo.detectionId); + const deviceQueue = this.detectedDeviceOfferQueues.get(deviceDetectionInfo.detectionId); if (undefined === deviceQueue) { resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is not available anymore for offering`) }); @@ -130,7 +130,7 @@ export default class DeviceManager }); } - private addDevice(deviceInfo: DeviceDetectionInfo, device: AnyDevice): boolean + private addDevice(deviceDetectionInfo: DeviceDetectionInfo, device: AnyDevice): boolean { if (!this.isDeviceEnabled(device.getDeviceId)) { this.logger.info(`Not adding device '${device.getDeviceId}' since it is disabled`); @@ -139,7 +139,7 @@ export default class DeviceManager .catch((e: unknown) => logError(this.logger, `Failed to close disabled device '${device.getDeviceId}'`, e)); // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it - this.pendingRetries.set(deviceInfo.detectionId, { deviceInfo, canonicalId: device.getDeviceId, closingDevice }); + this.pendingRetries.set(deviceDetectionInfo.detectionId, { deviceDetectionInfo, canonicalId: device.getDeviceId, closingDevice }); return false; } @@ -179,7 +179,7 @@ export default class DeviceManager } } - for (const [detectionId, { deviceInfo, canonicalId, closingDevice }] of this.pendingRetries) { + for (const [detectionId, { deviceDetectionInfo, canonicalId, closingDevice }] of this.pendingRetries) { if (!this.isDeviceEnabled(canonicalId)) { continue; } @@ -191,7 +191,7 @@ export default class DeviceManager await closingDevice; } - this.announceDetectedDevice(deviceInfo); + this.announceDetectedDevice(deviceDetectionInfo); } } @@ -238,8 +238,8 @@ export default class DeviceManager } } - for (const [deviceId] of this.detectedDeviceOfferQueue) { - this.clearDetectedDeviceOfferQueue(deviceId, 'Device manager reset'); + for (const [detectionId] of this.detectedDeviceOfferQueues) { + this.clearDetectedDeviceOfferQueue(detectionId, 'Device manager reset'); } this.pendingRetries.clear(); @@ -275,7 +275,7 @@ export default class DeviceManager // The queue may have been cleared (revoke/reset) or replaced (a fresh announce for // the same detection id) while this offer was in flight - the caller already got a // settled result for the old queue, so don't hand it a device it never asked for. - if (this.detectedDeviceOfferQueue.get(detectionId) !== deviceQueue) { + if (this.detectedDeviceOfferQueues.get(detectionId) !== deviceQueue) { await device.close() .catch((e: unknown) => logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e)); return; @@ -304,27 +304,27 @@ export default class DeviceManager { // The queue may already have been cleared/replaced (revoke, reset, or a fresh announce // for the same detection id) - don't shift/delete a queue we no longer own. - if (this.detectedDeviceOfferQueue.get(detectionId) !== deviceQueue) { + if (this.detectedDeviceOfferQueues.get(detectionId) !== deviceQueue) { return; } deviceQueue.shift(); if (deviceQueue.length === 0) { - this.detectedDeviceOfferQueue.delete(detectionId); + this.detectedDeviceOfferQueues.delete(detectionId); return; } void this.runNextInQueue(deviceQueue); } - private clearDetectedDeviceOfferQueue(deviceId: string, reason: string): void + private clearDetectedDeviceOfferQueue(detectionId: string, reason: string): void { - for (const entry of this.detectedDeviceOfferQueue.get(deviceId) ?? []) { + for (const entry of this.detectedDeviceOfferQueues.get(detectionId) ?? []) { entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(reason) }); } - this.detectedDeviceOfferQueue.delete(deviceId); + this.detectedDeviceOfferQueues.delete(detectionId); } private initDeviceRefresher(device: AnyDevice): void { From cbf16aed575fcc5d25b3c54bb4760c3330a99576 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 21:11:29 +0200 Subject: [PATCH 08/24] refactor: further naming clarity in deviceManager.ts - QueueEntry -> PendingDetectedDeviceOffer: mirrors the already-renamed detectedDeviceOfferQueues field it lives in (a map of queues, each holding these entries). - pendingRetries -> detectedDisabledDevices: the old name didn't say what was pending or why; the map holds detections rejected because their known device is currently disabled, parked for re-announcement on re-enable. - closingDevice -> deviceReleased: it's a Promise signaling when the device's underlying resource (serial port, BLE connection, etc.) has been released by the old instance, awaited before re-announcing to avoid a new connection attempt racing the old one's teardown - not literally "a closing device". --- src/device/deviceManager.ts | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 1f746b23..a15b6c7c 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -26,7 +26,7 @@ type OfferResult = | { successful: true, device: D } | { successful: false, reason: unknown }; -type QueueEntry = { +type PendingDetectedDeviceOffer = { deviceDetectionInfo: DeviceDetectionInfo; deviceOffer: () => Promise; resolve: (result: OfferResult) => void; @@ -46,19 +46,19 @@ export default class DeviceManager private readonly logger: Logger; - private readonly detectedDeviceOfferQueues: Map[]> = new Map(); + private readonly detectedDeviceOfferQueues: Map[]> = new Map(); private readonly connectedDevices: Map; private readonly settingsManager: SettingsManager; /** - * Devices whose retry is pending because their known device is disabled; re-announced once - * it gets (re-)enabled, see `onSettingsChanged()`. `canonicalId` is the device's final id - * whose enablement gates the retry (protocols may only learn it during a handshake, so it + * Detected devices currently disabled via their known device; re-announced once it gets + * (re-)enabled, see `onSettingsChanged()`. `canonicalId` is the device's final id whose + * enablement gates the re-announce (protocols may only learn it during a handshake, so it * can differ from the map key, the preliminary `deviceDetectionInfo.detectionId`). */ - private readonly pendingRetries: Map }> = new Map(); + private readonly detectedDisabledDevices: Map }> = new Map(); // Serializes onSettingsChanged() runs so rapid settings changes don't interleave private readonly settingsChangeQueue: SequentialTaskQueue = new SequentialTaskQueue(); @@ -106,7 +106,7 @@ export default class DeviceManager public revokeDetectedDevice(deviceDetectionInfo: DeviceDetectionInfo): void { // A device that physically disappeared should no longer be retried on re-enable - this.pendingRetries.delete(deviceDetectionInfo.detectionId); + this.detectedDisabledDevices.delete(deviceDetectionInfo.detectionId); this.clearDetectedDeviceOfferQueue(deviceDetectionInfo.detectionId, `Device with id '${deviceDetectionInfo.detectionId}' has disappeared`); } @@ -135,11 +135,11 @@ export default class DeviceManager if (!this.isDeviceEnabled(device.getDeviceId)) { this.logger.info(`Not adding device '${device.getDeviceId}' since it is disabled`); - const closingDevice = device.close() + const deviceReleased = device.close() .catch((e: unknown) => logError(this.logger, `Failed to close disabled device '${device.getDeviceId}'`, e)); // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it - this.pendingRetries.set(deviceDetectionInfo.detectionId, { deviceDetectionInfo, canonicalId: device.getDeviceId, closingDevice }); + this.detectedDisabledDevices.set(deviceDetectionInfo.detectionId, { deviceDetectionInfo, canonicalId: device.getDeviceId, deviceReleased }); return false; } @@ -179,16 +179,16 @@ export default class DeviceManager } } - for (const [detectionId, { deviceDetectionInfo, canonicalId, closingDevice }] of this.pendingRetries) { + for (const [detectionId, { deviceDetectionInfo, canonicalId, deviceReleased }] of this.detectedDisabledDevices) { if (!this.isDeviceEnabled(canonicalId)) { continue; } - this.pendingRetries.delete(detectionId); + this.detectedDisabledDevices.delete(detectionId); // Make sure a device rejected by addDevice() has finished closing before re-announcing - if (undefined !== closingDevice) { - await closingDevice; + if (undefined !== deviceReleased) { + await deviceReleased; } this.announceDetectedDevice(deviceDetectionInfo); @@ -242,14 +242,14 @@ export default class DeviceManager this.clearDetectedDeviceOfferQueue(detectionId, 'Device manager reset'); } - this.pendingRetries.clear(); + this.detectedDisabledDevices.clear(); if (undefined !== closeError) { throw closeError; } } - private async runNextInQueue(deviceQueue: QueueEntry[]): Promise + private async runNextInQueue(deviceQueue: PendingDetectedDeviceOffer[]): Promise { const entry = deviceQueue[0]; @@ -300,7 +300,7 @@ export default class DeviceManager * entirely once empty so the device can be re-announced (announceDetectedDevice() gates on * the map key existing). */ - private advanceQueue(detectionId: string, deviceQueue: QueueEntry[]): void + private advanceQueue(detectionId: string, deviceQueue: PendingDetectedDeviceOffer[]): void { // The queue may already have been cleared/replaced (revoke, reset, or a fresh announce // for the same detection id) - don't shift/delete a queue we no longer own. From f583d161c124fa6603784109f70ae3adad2d4519 Mon Sep 17 00:00:00 2001 From: HRS Date: Tue, 28 Jul 2026 22:00:00 +0200 Subject: [PATCH 09/24] Simplify runNextOfferInQueue, eliminate undefined from device offer callstack - Merge rejectAndAdvance closure into rejectCurrentOfferInQueueAndAdvance - Rename runNextInQueue/rejectOfferQueueHeadAndAdvance for clarity - Replace undefined-on-failure with throw-on-failure across the whole createDevice/connectSerialDevice/connectBleDevice/deviceOffer callstack, since every concrete provider either always resolves with a device or throws - undefined was never a real signal - Narrow D | undefined to D throughout DeviceProvider, BleDeviceProvider, SerialDeviceProvider and DeviceManager's offerDevice/PendingDetectedDeviceOffer - Update tests accordingly, dropping one test whose scenario became type-impossible --- src/device/deviceManager.ts | 58 ++++++++----------- .../protocol/airotic/airoticDeviceProvider.ts | 4 +- .../buttplugIoWebsocketDeviceProvider.ts | 2 +- .../estim2b/estim2bSerialDeviceProvider.ts | 2 +- .../slvCtrlPlusSerialDeviceProvider.ts | 2 +- .../protocol/virtual/virtualDeviceProvider.ts | 2 +- .../protocol/zc95/zc95SerialDeviceProvider.ts | 2 +- src/device/provider/bleDeviceProvider.ts | 4 +- src/device/provider/deviceProvider.ts | 10 +--- src/device/provider/serialDeviceProvider.ts | 38 +++++------- tests/unit/device/deviceManager.spec.ts | 39 ++++--------- .../device/provider/deviceProvider.spec.ts | 18 +++--- .../provider/deviceProviderManager.spec.ts | 4 +- 13 files changed, 72 insertions(+), 113 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index a15b6c7c..427290d7 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -28,10 +28,16 @@ type OfferResult = type PendingDetectedDeviceOffer = { deviceDetectionInfo: DeviceDetectionInfo; - deviceOffer: () => Promise; + deviceOffer: () => Promise; resolve: (result: OfferResult) => void; }; +type DisabledDetectedDevice = { + deviceDetectionInfo: DeviceDetectionInfo; + canonicalId: DeviceId; + deviceReleased: Promise; +}; + type DeviceManagerEventMap = { [DeviceManagerEvent.deviceConnected]: [device: AnyDevice]; [DeviceManagerEvent.deviceDisconnected]: [device: AnyDevice]; @@ -52,13 +58,7 @@ export default class DeviceManager private readonly settingsManager: SettingsManager; - /** - * Detected devices currently disabled via their known device; re-announced once it gets - * (re-)enabled, see `onSettingsChanged()`. `canonicalId` is the device's final id whose - * enablement gates the re-announce (protocols may only learn it during a handshake, so it - * can differ from the map key, the preliminary `deviceDetectionInfo.detectionId`). - */ - private readonly detectedDisabledDevices: Map }> = new Map(); + private readonly detectedDisabledDevices: Map = new Map(); // Serializes onSettingsChanged() runs so rapid settings changes don't interleave private readonly settingsChangeQueue: SequentialTaskQueue = new SequentialTaskQueue(); @@ -110,7 +110,7 @@ export default class DeviceManager this.clearDetectedDeviceOfferQueue(deviceDetectionInfo.detectionId, `Device with id '${deviceDetectionInfo.detectionId}' has disappeared`); } - public offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> + public offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { return new Promise>((resolve) => { const deviceQueue = this.detectedDeviceOfferQueues.get(deviceDetectionInfo.detectionId); @@ -125,7 +125,7 @@ export default class DeviceManager // If we're first in line, run our offer immediately if (deviceQueue.length === 1) { - void this.runNextInQueue(deviceQueue); + void this.runNextOfferInQueue(deviceQueue); } }); } @@ -179,19 +179,17 @@ export default class DeviceManager } } - for (const [detectionId, { deviceDetectionInfo, canonicalId, deviceReleased }] of this.detectedDisabledDevices) { - if (!this.isDeviceEnabled(canonicalId)) { + for (const [detectionId, disabledDetectedDevice] of this.detectedDisabledDevices) { + if (!this.isDeviceEnabled(disabledDetectedDevice.canonicalId)) { continue; } this.detectedDisabledDevices.delete(detectionId); // Make sure a device rejected by addDevice() has finished closing before re-announcing - if (undefined !== deviceReleased) { - await deviceReleased; - } + await disabledDetectedDevice.deviceReleased; - this.announceDetectedDevice(deviceDetectionInfo); + this.announceDetectedDevice(disabledDetectedDevice.deviceDetectionInfo); } } @@ -249,7 +247,7 @@ export default class DeviceManager } } - private async runNextInQueue(deviceQueue: PendingDetectedDeviceOffer[]): Promise + private async runNextOfferInQueue(deviceQueue: PendingDetectedDeviceOffer[]): Promise { const entry = deviceQueue[0]; @@ -259,19 +257,9 @@ export default class DeviceManager const detectionId = entry.deviceDetectionInfo.detectionId; - const rejectAndAdvance = (reason: unknown): void => { - entry.resolve({ successful: false, reason }); - this.advanceQueue(detectionId, deviceQueue); - }; - try { const device = await entry.deviceOffer(); - if (undefined === device) { - rejectAndAdvance(new Error(`Device offer for '${detectionId}' returned undefined`)); - return; - } - // The queue may have been cleared (revoke/reset) or replaced (a fresh announce for // the same detection id) while this offer was in flight - the caller already got a // settled result for the old queue, so don't hand it a device it never asked for. @@ -289,19 +277,21 @@ export default class DeviceManager return; } - rejectAndAdvance(new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`)); + this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`)); } catch (e: unknown) { - rejectAndAdvance(e); + this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, e); } } /** - * Drops the just-settled entry and hands off to the next waiter, if any. Deletes the queue - * entirely once empty so the device can be re-announced (announceDetectedDevice() gates on - * the map key existing). + * Resolves the queue's head entry with the given failure reason, then hands off to the next + * waiter, if any. Deletes the queue entirely once empty so the device can be re-announced + * (announceDetectedDevice() gates on the map key existing). */ - private advanceQueue(detectionId: string, deviceQueue: PendingDetectedDeviceOffer[]): void + private rejectCurrentOfferInQueueAndAdvance(detectionId: string, deviceQueue: PendingDetectedDeviceOffer[], reason: unknown): void { + deviceQueue[0]?.resolve({ successful: false, reason }); + // The queue may already have been cleared/replaced (revoke, reset, or a fresh announce // for the same detection id) - don't shift/delete a queue we no longer own. if (this.detectedDeviceOfferQueues.get(detectionId) !== deviceQueue) { @@ -315,7 +305,7 @@ export default class DeviceManager return; } - void this.runNextInQueue(deviceQueue); + void this.runNextOfferInQueue(deviceQueue); } private clearDetectedDeviceOfferQueue(detectionId: string, reason: string): void diff --git a/src/device/protocol/airotic/airoticDeviceProvider.ts b/src/device/protocol/airotic/airoticDeviceProvider.ts index dbdb5aae..1f51e50d 100644 --- a/src/device/protocol/airotic/airoticDeviceProvider.ts +++ b/src/device/protocol/airotic/airoticDeviceProvider.ts @@ -30,7 +30,7 @@ export default class AiroticDeviceProvider extends BleDeviceProvider { + protected override async connectBleDevice(deviceDetectionInfo: BleDeviceDetectionInfo): Promise { const transport = await promiseWithTimeout(BleUartDeviceTransport.create( deviceDetectionInfo.peripheral, AiroticDeviceProvider.UART_RX_CHAR_UUID, @@ -46,7 +46,7 @@ export default class AiroticDeviceProvider extends BleDeviceProvider { + protected override createDevice(deviceDetectionInfo: ButtplugIoDeviceDetectionInfo): Promise { const device = this.buttplugIoDeviceFactory.create( deviceDetectionInfo.detectionId, deviceDetectionInfo.buttplugClientDevice, diff --git a/src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts b/src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts index 9a4e0dc1..e28d42bf 100644 --- a/src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts +++ b/src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts @@ -35,7 +35,7 @@ export default class EStim2bSerialDeviceProvider extends SerialDeviceProvider): Promise { + protected async connectSerialDevice(deviceDetectionInfo: SerialDeviceDetectionInfo, port: SerialPortStream): Promise { const parser = port.pipe(new ReadlineParser({ delimiter: '\n' })); const syncPort = new SynchronousSerialPort(deviceDetectionInfo.portInfo, parser, port, this.logger); const transport = this.transportFactory.create(syncPort, undefined, Buffer.from('\r')); diff --git a/src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts b/src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts index bdbab3af..c2d18a69 100644 --- a/src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts +++ b/src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts @@ -39,7 +39,7 @@ export default class SlvCtrlPlusSerialDeviceProvider extends SerialDeviceProvide this.deviceTransportFactory = deviceTransportFactory; } - protected async connectSerialDevice(deviceDetectionInfo: SerialDeviceDetectionInfo, port: SerialPortStream): Promise + protected async connectSerialDevice(deviceDetectionInfo: SerialDeviceDetectionInfo, port: SerialPortStream): Promise { const parser = port.pipe(new ReadlineParser({ delimiter: SlvCtrlProtocol.EOF })); const syncPort = new SynchronousSerialPort(deviceDetectionInfo.portInfo, parser, port, this.logger); diff --git a/src/device/protocol/virtual/virtualDeviceProvider.ts b/src/device/protocol/virtual/virtualDeviceProvider.ts index 871b83a6..d6e7ece3 100644 --- a/src/device/protocol/virtual/virtualDeviceProvider.ts +++ b/src/device/protocol/virtual/virtualDeviceProvider.ts @@ -56,7 +56,7 @@ export default class VirtualDeviceProvider extends DeviceProvider | undefined> { + protected override createDevice(deviceDetectionInfo: VirtualDeviceDetectionInfo): Promise> { this.logger.info(`Virtual device detected: ${deviceDetectionInfo.knownDevice.name}`, deviceDetectionInfo.knownDevice); return this.deviceFactory.create(deviceDetectionInfo.knownDevice, VirtualDeviceProvider.providerName); diff --git a/src/device/protocol/zc95/zc95SerialDeviceProvider.ts b/src/device/protocol/zc95/zc95SerialDeviceProvider.ts index 4a05b566..f340a403 100644 --- a/src/device/protocol/zc95/zc95SerialDeviceProvider.ts +++ b/src/device/protocol/zc95/zc95SerialDeviceProvider.ts @@ -36,7 +36,7 @@ export default class Zc95SerialDeviceProvider extends SerialDeviceProvider): Promise { + protected async connectSerialDevice(deviceDetectionInfo: SerialDeviceDetectionInfo, port: SerialPortStream): Promise { const serialLogger = this.logger.child({ name: Zc95Device.name }) const parser = port.pipe(new FrameParser({ stx: Zc95Protocol.STX, etx: Zc95Protocol.ETX })); diff --git a/src/device/provider/bleDeviceProvider.ts b/src/device/provider/bleDeviceProvider.ts index d5b6eca4..3df661d6 100644 --- a/src/device/provider/bleDeviceProvider.ts +++ b/src/device/provider/bleDeviceProvider.ts @@ -28,7 +28,7 @@ export default abstract class BleDeviceProvider extends return deviceDetectionInfo.type === 'ble'; } - protected override createDevice(deviceDetectionInfo: BleDeviceDetectionInfo): Promise { + protected override createDevice(deviceDetectionInfo: BleDeviceDetectionInfo): Promise { return this.connectBleDevice(deviceDetectionInfo); } @@ -52,5 +52,5 @@ export default abstract class BleDeviceProvider extends } } - protected abstract connectBleDevice(deviceDetectionInfo: BleDeviceDetectionInfo): Promise; + protected abstract connectBleDevice(deviceDetectionInfo: BleDeviceDetectionInfo): Promise; } diff --git a/src/device/provider/deviceProvider.ts b/src/device/provider/deviceProvider.ts index dd2def51..9e24d9a3 100644 --- a/src/device/provider/deviceProvider.ts +++ b/src/device/provider/deviceProvider.ts @@ -105,10 +105,6 @@ export default abstract class DeviceProvider { const device = await this.createDevice(deviceDetectionInfo); - if (undefined === device) { - return undefined; - } - // Provider was stopped while the offer was in flight (or waiting in queue) - don't // hand a connected device to a stopped provider, treat it like a failed offer instead if (this.isStopped()) { @@ -117,7 +113,7 @@ export default abstract class DeviceProvider; + protected abstract createDevice(deviceDetectionInfo: DDI): Promise; // eslint-disable-next-line @typescript-eslint/no-unused-vars protected async onConnectFailed(deviceDetectionInfo: DDI): Promise { diff --git a/src/device/provider/serialDeviceProvider.ts b/src/device/provider/serialDeviceProvider.ts index 41e48a38..ef2a156f 100644 --- a/src/device/provider/serialDeviceProvider.ts +++ b/src/device/provider/serialDeviceProvider.ts @@ -7,7 +7,6 @@ import SerialPortFactory from '../../factory/serialPortFactory.js'; import { AutoDetectTypes } from '@serialport/bindings-cpp'; import BaseError from 'modern-errors'; import DeviceManager, { DeviceDetectionInfo } from '../deviceManager.js'; -import { logError } from '../../util/error.js'; import SerialPortObserver, { SerialDeviceDetectionInfo } from '../transport/serialPortObserver.js'; import { AnyPeripheralDevice } from '../peripheralDevice.js'; @@ -43,7 +42,7 @@ export default abstract class SerialDeviceProvider { + protected override async createDevice(deviceDetectionInfo: SerialDeviceDetectionInfo): Promise { const portInfo = deviceDetectionInfo.portInfo; this.logger.info(`Connection attempt for serial device '${portInfo.path}' (s/n: ${portInfo.serialNumber})`); @@ -54,9 +53,6 @@ export default abstract class SerialDeviceProvider((resolve, reject) => { port.open(err => err ? reject(err) : resolve()); @@ -64,32 +60,24 @@ export default abstract class SerialDeviceProvider((resolve, reject) => { port.close(err => err ? reject(err) : resolve()); }); } - this.logger.info(`Could not connect to serial device '${portInfo.path}': ${attemptFailureReason}`); - } else { - this.logger.info(`Successfully connected to serial device '${portInfo.path}'`); - this.logger.debug(`Assigned device id: ${device.getDeviceId} (${portInfo.path})`); - } - return device; + const error = BaseError.normalize(e); + this.logger.info(`Could not connect to serial device '${portInfo.path}': ${error.message}`); + + throw e; + } } // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -97,7 +85,7 @@ export default abstract class SerialDeviceProvider): Promise; + protected abstract connectSerialDevice(deviceDetectionInfo: SerialDeviceDetectionInfo, port: SerialPortStream): Promise; protected abstract getSerialDeviceProviderPortOpenOptions(portInfo: PortInfo): SerialDeviceProviderPortOpenOptions; } diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index 58d18f1d..a2efc7bb 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -241,8 +241,8 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - let resolveFirstOffer!: (device: AnyDevice | undefined) => void; - const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); + let resolveFirstOffer!: (device: AnyDevice) => void; + const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); const secondOfferFn = vi.fn(() => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); const firstResultPromise = manager.offerDevice(deviceInfo, () => firstOfferPromise); @@ -250,25 +250,10 @@ describe('deviceManager', () => { expect(secondOfferFn).not.toHaveBeenCalled(); - resolveFirstOffer(undefined); + resolveFirstOffer(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter())); await firstResultPromise; }); - it('hands off to the next queued offer when the first one returns undefined', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - - const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - - const firstResultPromise = manager.offerDevice(deviceInfo, () => Promise.resolve(undefined)); - const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.resolve(device)); - - const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); - - expect(firstResult.successful).toBe(false); - expect(secondResult).toStrictEqual({ successful: true, device }); - }); - it('hands off to the next queued offer when the first one throws', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); @@ -318,7 +303,7 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - await manager.offerDevice(deviceInfo, () => Promise.resolve(undefined)); + await manager.offerDevice(deviceInfo, () => Promise.reject(new Error('connect failed'))); mockClear(mockedEventEmitter); mockedEventEmitter.emit.mockReturnValue(true); @@ -332,8 +317,8 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - let resolveFirstOffer!: (device: AnyDevice | undefined) => void; - const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); + let resolveFirstOffer!: (device: AnyDevice) => void; + const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); const firstResultPromise = manager.offerDevice(deviceInfo, () => firstOfferPromise); @@ -367,7 +352,7 @@ describe('deviceManager', () => { manager.announceDetectedDevice(deviceInfo); // First offer never settles on its own, so it's still holding the queue when revoked - const pendingPromise = manager.offerDevice(deviceInfo, () => new Promise(() => {})); + const pendingPromise = manager.offerDevice(deviceInfo, () => new Promise(() => {})); manager.revokeDetectedDevice(deviceInfo); @@ -380,8 +365,8 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); - let resolveOffer!: (device: AnyDevice | undefined) => void; - const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); + let resolveOffer!: (device: AnyDevice) => void; + const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); const resultPromise = manager.offerDevice(deviceInfo, () => offerPromise); // Device physically disappears while the offer is still in flight. @@ -407,7 +392,7 @@ describe('deviceManager', () => { manager.announceDetectedDevice(deviceInfo); let rejectStaleOffer!: (reason: unknown) => void; - const staleOfferPromise = new Promise((_resolve, reject) => { rejectStaleOffer = reject; }); + const staleOfferPromise = new Promise((_resolve, reject) => { rejectStaleOffer = reject; }); const staleResultPromise = manager.offerDevice(deviceInfo, () => staleOfferPromise); // Device physically disappears while the stale offer is still in flight. @@ -417,8 +402,8 @@ describe('deviceManager', () => { // exists, with its own still-pending offer. manager.announceDetectedDevice(deviceInfo); - let resolveFreshOffer!: (device: AnyDevice | undefined) => void; - const freshOfferPromise = new Promise((resolve) => { resolveFreshOffer = resolve; }); + let resolveFreshOffer!: (device: AnyDevice) => void; + const freshOfferPromise = new Promise((resolve) => { resolveFreshOffer = resolve; }); const freshResultPromise = manager.offerDevice(deviceInfo, () => freshOfferPromise); // The stale offer only fails now, well after it was revoked and superseded - while diff --git a/tests/unit/device/provider/deviceProvider.spec.ts b/tests/unit/device/provider/deviceProvider.spec.ts index 505d6210..b6a3bca2 100644 --- a/tests/unit/device/provider/deviceProvider.spec.ts +++ b/tests/unit/device/provider/deviceProvider.spec.ts @@ -24,8 +24,8 @@ class TestProvider extends DeviceProvider return false; } - protected createDevice(_deviceDetectionInfo: DeviceDetectionInfo): Promise { - return Promise.resolve(undefined); + protected createDevice(deviceDetectionInfo: DeviceDetectionInfo): Promise { + return Promise.resolve(new TestDevice(deviceDetectionInfo.detectionId, 'Foo', new Date(), false, new EventEmitter())); } protected override async doStart(): Promise { @@ -49,7 +49,7 @@ class DetectingTestProvider extends DeviceProvider { + protected createDevice(deviceDetectionInfo: DeviceDetectionInfo): Promise { return Promise.resolve(new TestDevice(deviceDetectionInfo.detectionId, 'Foo', new Date(), false, new EventEmitter())); } @@ -64,7 +64,7 @@ class DetectingTestProvider extends DeviceProvider { - public constructor(deviceManager: DeviceManager, private readonly createDevicePromise: Promise) { + public constructor(deviceManager: DeviceManager, private readonly createDevicePromise: Promise) { super(deviceManager, mock()); } @@ -72,7 +72,7 @@ class SlowCreateDeviceProvider extends DeviceProvider { + protected createDevice(_deviceDetectionInfo: DeviceDetectionInfo): Promise { return this.createDevicePromise; } } @@ -84,7 +84,7 @@ class TrackingTestProvider extends DeviceProvider Promise + private readonly createDeviceFn: (deviceDetectionInfo: DeviceDetectionInfo) => Promise ) { super(deviceManager, mock()); } @@ -93,7 +93,7 @@ class TrackingTestProvider extends DeviceProvider { + protected createDevice(deviceDetectionInfo: DeviceDetectionInfo): Promise { return this.createDeviceFn(deviceDetectionInfo); } @@ -213,8 +213,8 @@ describe('DeviceProvider', () => { const deviceManager = new DeviceManager(new EventEmitter(), new Map(), settingsManager, logger); - let resolveCreateDevice!: (device: AnyDevice | undefined) => void; - const createDevicePromise = new Promise((resolve) => { resolveCreateDevice = resolve; }); + let resolveCreateDevice!: (device: AnyDevice) => void; + const createDevicePromise = new Promise((resolve) => { resolveCreateDevice = resolve; }); const provider = new SlowCreateDeviceProvider(deviceManager, createDevicePromise); await provider.start(); diff --git a/tests/unit/device/provider/deviceProviderManager.spec.ts b/tests/unit/device/provider/deviceProviderManager.spec.ts index 8cb5758e..f68babb7 100644 --- a/tests/unit/device/provider/deviceProviderManager.spec.ts +++ b/tests/unit/device/provider/deviceProviderManager.spec.ts @@ -49,8 +49,8 @@ class RecordingDeviceProvider extends DeviceProvider { - return Promise.resolve(undefined); + protected createDevice(_deviceDetectionInfo: DeviceDetectionInfo): Promise { + return Promise.resolve(mock()); } } From 2b7e23730e7953661be9a92a5ec5acd4c3c872a5 Mon Sep 17 00:00:00 2001 From: HRS Date: Wed, 29 Jul 2026 06:22:10 +0200 Subject: [PATCH 10/24] Extract detected-device offer queue mechanics into DetectedDeviceOfferQueue - New DetectedDeviceOfferQueue class owns queue storage, enqueue/dequeue, hand-off on failure, and TOCTOU staleness protection - previously mixed into DeviceManager alongside device registry and disabled-device retry concerns - DeviceManager.addDevice() now returns DeviceOfferRejectedError | undefined instead of boolean, injected into the queue as a DeviceOfferAcceptor callback, preserving specific rejection messages - announceDetectedDevice/offerDevice/revokeDetectedDevice/reset become thin delegations to the queue plus their own non-queue concerns (connectedDevices checks, event emission, detectedDisabledDevices) - Move queue race/hand-off/staleness tests into a dedicated detectedDeviceOfferQueue.spec.ts testing the class in isolation with a mock acceptor; deviceManager.spec.ts keeps end-to-end and delegation tests - Update deviceOfferRejectedError.ts's stale doc comment reference to the since-removed undefined return path --- src/device/detectedDeviceOfferQueue.ts | 152 +++++++++++ src/device/deviceManager.ts | 119 +------- src/device/deviceOfferRejectedError.ts | 5 +- .../device/detectedDeviceOfferQueue.spec.ts | 253 ++++++++++++++++++ tests/unit/device/deviceManager.spec.ts | 175 ------------ 5 files changed, 419 insertions(+), 285 deletions(-) create mode 100644 src/device/detectedDeviceOfferQueue.ts create mode 100644 tests/unit/device/detectedDeviceOfferQueue.spec.ts diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts new file mode 100644 index 00000000..29ca26e3 --- /dev/null +++ b/src/device/detectedDeviceOfferQueue.ts @@ -0,0 +1,152 @@ +import { AnyDevice } from './device.js'; +import { DeviceDetectionInfo } from './deviceManager.js'; +import DeviceOfferRejectedError from './deviceOfferRejectedError.js'; +import Logger from '../logging/Logger.js'; +import { logError } from '../util/error.js'; + +export type OfferResult = + | { successful: true, device: D } + | { successful: false, reason: unknown }; + +/** + * Decides whether a device offered through the queue is actually accepted (e.g. registered in + * the device registry). Returns `undefined` on acceptance, or the rejection reason otherwise. + */ +export type DeviceOfferAcceptor = (deviceDetectionInfo: DeviceDetectionInfo, device: AnyDevice) => DeviceOfferRejectedError | undefined; + +type PendingDetectedDeviceOffer = { + deviceDetectionInfo: DeviceDetectionInfo; + deviceOffer: () => Promise; + resolve: (result: OfferResult) => void; +}; + +/** + * Serializes competing offers for the same detected device (e.g. multiple protocol providers + * racing to claim the same serial port) into one queue per detection id, running one offer at a + * time and handing the result to an injected acceptor to decide success or failure. + */ +export default class DetectedDeviceOfferQueue +{ + private readonly queues: Map[]> = new Map(); + + private readonly accept: DeviceOfferAcceptor; + + private readonly logger: Logger; + + public constructor(accept: DeviceOfferAcceptor, logger: Logger) { + this.accept = accept; + this.logger = logger; + } + + public has(detectionId: string): boolean + { + return this.queues.has(detectionId); + } + + public open(detectionId: string): void + { + this.queues.set(detectionId, []); + } + + public discard(detectionId: string): void + { + this.queues.delete(detectionId); + } + + public offer(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> + { + return new Promise>((resolve) => { + const deviceQueue = this.queues.get(deviceDetectionInfo.detectionId); + + if (undefined === deviceQueue) { + resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is not available anymore for offering`) }); + return; + } + + // Always add to queue first + deviceQueue.push({ deviceDetectionInfo, deviceOffer, resolve }); + + // If we're first in line, run our offer immediately + if (deviceQueue.length === 1) { + void this.runNextOfferInQueue(deviceQueue); + } + }); + } + + public clear(detectionId: string, reason: string): void + { + for (const entry of this.queues.get(detectionId) ?? []) { + entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(reason) }); + } + + this.queues.delete(detectionId); + } + + public clearAll(reason: string): void + { + for (const [detectionId] of this.queues) { + this.clear(detectionId, reason); + } + } + + private async runNextOfferInQueue(deviceQueue: PendingDetectedDeviceOffer[]): Promise + { + const entry = deviceQueue[0]; + + if (undefined === entry) { + return; + } + + const detectionId = entry.deviceDetectionInfo.detectionId; + + try { + const device = await entry.deviceOffer(); + + // The queue may have been cleared (revoke/reset) or replaced (a fresh announce for + // the same detection id) while this offer was in flight - the caller already got a + // settled result for the old queue, so don't hand it a device it never asked for. + if (this.queues.get(detectionId) !== deviceQueue) { + await device.close() + .catch((e: unknown) => logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e)); + return; + } + + const rejection = this.accept(entry.deviceDetectionInfo, device); + + if (undefined === rejection) { + entry.resolve({ successful: true, device }); + this.clear(detectionId, `Device '${detectionId}' has been claimed by another provider`); + return; + } + + this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, rejection); + } catch (e: unknown) { + this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, e); + } + } + + /** + * Resolves the queue's head entry with the given failure reason, then hands off to the next + * waiter, if any. Deletes the queue entirely once empty so the device can be re-announced + * (DeviceManager.announceDetectedDevice() gates on the queue existing). + */ + private rejectCurrentOfferInQueueAndAdvance(detectionId: string, deviceQueue: PendingDetectedDeviceOffer[], reason: unknown): void + { + deviceQueue[0]?.resolve({ successful: false, reason }); + + // The queue may already have been cleared/replaced (revoke, reset, or a fresh announce + // for the same detection id) - don't shift/delete a queue we no longer own. + if (this.queues.get(detectionId) !== deviceQueue) { + return; + } + + deviceQueue.shift(); + + if (deviceQueue.length === 0) { + this.queues.delete(detectionId); + return; + } + + void this.runNextOfferInQueue(deviceQueue); + } +} diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 427290d7..ae5229eb 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -8,6 +8,7 @@ import { logError } from '../util/error.js'; import { DeviceId } from './deviceId.js'; import SettingsManager from '../settings/settingsManager.js'; import DeviceOfferRejectedError from './deviceOfferRejectedError.js'; +import DetectedDeviceOfferQueue, { OfferResult } from './detectedDeviceOfferQueue.js'; export type DeviceDetectionInfo = { type: string; @@ -22,16 +23,6 @@ export enum DeviceManagerEvent { deviceNotification = 'deviceNotification', } -type OfferResult = - | { successful: true, device: D } - | { successful: false, reason: unknown }; - -type PendingDetectedDeviceOffer = { - deviceDetectionInfo: DeviceDetectionInfo; - deviceOffer: () => Promise; - resolve: (result: OfferResult) => void; -}; - type DisabledDetectedDevice = { deviceDetectionInfo: DeviceDetectionInfo; canonicalId: DeviceId; @@ -52,7 +43,7 @@ export default class DeviceManager private readonly logger: Logger; - private readonly detectedDeviceOfferQueues: Map[]> = new Map(); + private readonly offerQueue: DetectedDeviceOfferQueue; private readonly connectedDevices: Map; @@ -73,6 +64,7 @@ export default class DeviceManager this.logger = logger.child({ name: DeviceManager.name }); this.connectedDevices = connectedDevices; this.settingsManager = settingsManager; + this.offerQueue = new DetectedDeviceOfferQueue((deviceDetectionInfo, device) => this.addDevice(deviceDetectionInfo, device), this.logger); } public isDeviceEnabled(deviceId: DeviceId): boolean { @@ -81,7 +73,7 @@ export default class DeviceManager public announceDetectedDevice(deviceDetectionInfo: DeviceDetectionInfo): void { - if (this.detectedDeviceOfferQueues.has(deviceDetectionInfo.detectionId)) { + if (this.offerQueue.has(deviceDetectionInfo.detectionId)) { return; } @@ -92,14 +84,14 @@ export default class DeviceManager this.logger.info(`Detected new device with id ${deviceDetectionInfo.detectionId}`); - this.detectedDeviceOfferQueues.set(deviceDetectionInfo.detectionId, []); + this.offerQueue.open(deviceDetectionInfo.detectionId); const hadListeners = this.eventEmitter.emit(DeviceManagerEvent.deviceDetected, deviceDetectionInfo); if (!hadListeners) { // no subscribed providers, remove empty list from offer queue for this device this.logger.info(`No provider available for detected device with id '${deviceDetectionInfo.detectionId}'`); - this.detectedDeviceOfferQueues.delete(deviceDetectionInfo.detectionId); + this.offerQueue.discard(deviceDetectionInfo.detectionId); } } @@ -107,30 +99,15 @@ export default class DeviceManager { // A device that physically disappeared should no longer be retried on re-enable this.detectedDisabledDevices.delete(deviceDetectionInfo.detectionId); - this.clearDetectedDeviceOfferQueue(deviceDetectionInfo.detectionId, `Device with id '${deviceDetectionInfo.detectionId}' has disappeared`); + this.offerQueue.clear(deviceDetectionInfo.detectionId, `Device with id '${deviceDetectionInfo.detectionId}' has disappeared`); } public offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { - return new Promise>((resolve) => { - const deviceQueue = this.detectedDeviceOfferQueues.get(deviceDetectionInfo.detectionId); - - if (undefined === deviceQueue) { - resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is not available anymore for offering`) }); - return; - } - - // Always add to queue first - deviceQueue.push({ deviceDetectionInfo, deviceOffer, resolve }); - - // If we're first in line, run our offer immediately - if (deviceQueue.length === 1) { - void this.runNextOfferInQueue(deviceQueue); - } - }); + return this.offerQueue.offer(deviceDetectionInfo, deviceOffer); } - private addDevice(deviceDetectionInfo: DeviceDetectionInfo, device: AnyDevice): boolean + private addDevice(deviceDetectionInfo: DeviceDetectionInfo, device: AnyDevice): DeviceOfferRejectedError | undefined { if (!this.isDeviceEnabled(device.getDeviceId)) { this.logger.info(`Not adding device '${device.getDeviceId}' since it is disabled`); @@ -141,7 +118,7 @@ export default class DeviceManager // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it this.detectedDisabledDevices.set(deviceDetectionInfo.detectionId, { deviceDetectionInfo, canonicalId: device.getDeviceId, deviceReleased }); - return false; + return new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`); } this.connectedDevices.set(device.getDeviceId, device); @@ -157,7 +134,7 @@ export default class DeviceManager this.eventEmitter.emit(DeviceManagerEvent.deviceConnected, device); - return true; + return undefined; } public async onSettingsChanged(): Promise { @@ -236,9 +213,7 @@ export default class DeviceManager } } - for (const [detectionId] of this.detectedDeviceOfferQueues) { - this.clearDetectedDeviceOfferQueue(detectionId, 'Device manager reset'); - } + this.offerQueue.clearAll('Device manager reset'); this.detectedDisabledDevices.clear(); @@ -247,76 +222,6 @@ export default class DeviceManager } } - private async runNextOfferInQueue(deviceQueue: PendingDetectedDeviceOffer[]): Promise - { - const entry = deviceQueue[0]; - - if (undefined === entry) { - return; - } - - const detectionId = entry.deviceDetectionInfo.detectionId; - - try { - const device = await entry.deviceOffer(); - - // The queue may have been cleared (revoke/reset) or replaced (a fresh announce for - // the same detection id) while this offer was in flight - the caller already got a - // settled result for the old queue, so don't hand it a device it never asked for. - if (this.detectedDeviceOfferQueues.get(detectionId) !== deviceQueue) { - await device.close() - .catch((e: unknown) => logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e)); - return; - } - - const added = this.addDevice(entry.deviceDetectionInfo, device); - - if (added) { - entry.resolve({ successful: true, device }); - this.clearDetectedDeviceOfferQueue(detectionId, `Device '${detectionId}' has been claimed by another provider`); - return; - } - - this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`)); - } catch (e: unknown) { - this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, e); - } - } - - /** - * Resolves the queue's head entry with the given failure reason, then hands off to the next - * waiter, if any. Deletes the queue entirely once empty so the device can be re-announced - * (announceDetectedDevice() gates on the map key existing). - */ - private rejectCurrentOfferInQueueAndAdvance(detectionId: string, deviceQueue: PendingDetectedDeviceOffer[], reason: unknown): void - { - deviceQueue[0]?.resolve({ successful: false, reason }); - - // The queue may already have been cleared/replaced (revoke, reset, or a fresh announce - // for the same detection id) - don't shift/delete a queue we no longer own. - if (this.detectedDeviceOfferQueues.get(detectionId) !== deviceQueue) { - return; - } - - deviceQueue.shift(); - - if (deviceQueue.length === 0) { - this.detectedDeviceOfferQueues.delete(detectionId); - return; - } - - void this.runNextOfferInQueue(deviceQueue); - } - - private clearDetectedDeviceOfferQueue(detectionId: string, reason: string): void - { - for (const entry of this.detectedDeviceOfferQueues.get(detectionId) ?? []) { - entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(reason) }); - } - - this.detectedDeviceOfferQueues.delete(detectionId); - } - private initDeviceRefresher(device: AnyDevice): void { this.logger.info(`Initializing refresher for device '${device.getDeviceName}' (id: ${device.getDeviceId})`); const deviceRefreshIntervalMs = device.getRefreshInterval; diff --git a/src/device/deviceOfferRejectedError.ts b/src/device/deviceOfferRejectedError.ts index 5ed67843..e7bdccdb 100644 --- a/src/device/deviceOfferRejectedError.ts +++ b/src/device/deviceOfferRejectedError.ts @@ -1,8 +1,7 @@ /** * Marks a device offer rejection as a manager-level decision (queue unavailable, device * disabled, claimed by another provider, revoked, reset) as opposed to the offer itself failing - * (thrown error or `undefined` returned). `DeviceProvider` uses this to decide whether - * `onConnectFailed()` should run - it shouldn't for manager-level decisions, only for the - * provider's own failed attempt. + * (a thrown error). `DeviceProvider` uses this to decide whether `onConnectFailed()` should run - + * it shouldn't for manager-level decisions, only for the provider's own failed attempt. */ export default class DeviceOfferRejectedError extends Error {} diff --git a/tests/unit/device/detectedDeviceOfferQueue.spec.ts b/tests/unit/device/detectedDeviceOfferQueue.spec.ts new file mode 100644 index 00000000..328aa020 --- /dev/null +++ b/tests/unit/device/detectedDeviceOfferQueue.spec.ts @@ -0,0 +1,253 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { mock } from 'vitest-mock-extended'; +import { EventEmitter } from 'events'; +import DetectedDeviceOfferQueue, { DeviceOfferAcceptor } from '../../../src/device/detectedDeviceOfferQueue.js'; +import DeviceOfferRejectedError from '../../../src/device/deviceOfferRejectedError.js'; +import { DeviceDetectionInfo } from '../../../src/device/deviceManager.js'; +import { AnyDevice } from '../../../src/device/device.js'; +import { DeviceId } from '../../../src/device/deviceId.js'; +import Logger from '../../../src/logging/Logger.js'; +import TestDevice from './testDevice.js'; + +describe('DetectedDeviceOfferQueue', () => { + let mockedLogger: ReturnType>; + const deviceId = DeviceId.create('device-1'); + const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; + + // Accepts every device offered - the default acceptor for tests only concerned with queue + // mechanics (ordering, hand-off, staleness), not with acceptance decisions themselves. + const acceptAlways: DeviceOfferAcceptor = () => undefined; + + beforeEach(() => { + mockedLogger = mock(); + mockedLogger.child.mockReturnValue(mockedLogger); + }); + + describe('has / open / discard', () => { + it('reflects open() and discard()', () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + + expect(queue.has(deviceId)).toBe(false); + + queue.open(deviceId); + expect(queue.has(deviceId)).toBe(true); + + queue.discard(deviceId); + expect(queue.has(deviceId)).toBe(false); + }); + }); + + describe('offer', () => { + it('rejects with DeviceOfferRejectedError when the queue does not exist', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + + const result = await queue.offer(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); + + expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); + }); + + it('runs the first offer immediately and resolves successfully when accepted', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + queue.open(deviceId); + + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const result = await queue.offer(deviceInfo, () => Promise.resolve(device)); + + expect(result).toStrictEqual({ successful: true, device }); + }); + + it('does not run a second offer while the first is still pending', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + queue.open(deviceId); + + let resolveFirstOffer!: (device: AnyDevice) => void; + const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); + const secondOfferFn = vi.fn(() => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); + + const firstResultPromise = queue.offer(deviceInfo, () => firstOfferPromise); + queue.offer(deviceInfo, secondOfferFn); + + expect(secondOfferFn).not.toHaveBeenCalled(); + + resolveFirstOffer(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter())); + await firstResultPromise; + }); + + it('hands off to the next queued offer when the first one throws', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + queue.open(deviceId); + + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const offerError = new Error('connection failed'); + + const firstResultPromise = queue.offer(deviceInfo, () => Promise.reject(offerError)); + const secondResultPromise = queue.offer(deviceInfo, () => Promise.resolve(device)); + + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult).toStrictEqual({ successful: false, reason: offerError }); + expect(secondResult).toStrictEqual({ successful: true, device }); + }); + + it('hands off to the next queued offer when the acceptor rejects the first device', async () => { + const rejection = new DeviceOfferRejectedError('rejected by acceptor'); + const accept = vi.fn() + .mockReturnValueOnce(rejection) + .mockReturnValueOnce(undefined); + const queue = new DetectedDeviceOfferQueue(accept, mockedLogger); + queue.open(deviceId); + + const firstDevice = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const secondDevice = new TestDevice(DeviceId.create('device-1-second'), 'Foo', new Date(), false, new EventEmitter()); + + const firstResultPromise = queue.offer(deviceInfo, () => Promise.resolve(firstDevice)); + const secondResultPromise = queue.offer(deviceInfo, () => Promise.resolve(secondDevice)); + + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult).toStrictEqual({ successful: false, reason: rejection }); + expect(secondResult).toStrictEqual({ successful: true, device: secondDevice }); + }); + + it('reopens for a new offer after the only queued offer is rejected', async () => { + const queue = new DetectedDeviceOfferQueue(() => new DeviceOfferRejectedError('rejected'), mockedLogger); + queue.open(deviceId); + + await queue.offer(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); + + expect(queue.has(deviceId)).toBe(false); + + queue.open(deviceId); + expect(queue.has(deviceId)).toBe(true); + }); + + it('rejects other queued offers with DeviceOfferRejectedError once a device is claimed', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + queue.open(deviceId); + + let resolveFirstOffer!: (device: AnyDevice) => void; + const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + + const firstResultPromise = queue.offer(deviceInfo, () => firstOfferPromise); + const secondResultPromise = queue.offer(deviceInfo, () => Promise.reject(new Error('should never run'))); + + resolveFirstOffer(device); + + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult).toStrictEqual({ successful: true, device }); + expect(secondResult.successful).toBe(false); + expect(!secondResult.successful && secondResult.reason).toBeInstanceOf(DeviceOfferRejectedError); + }); + }); + + describe('clear', () => { + it('resolves a pending offer with failure', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + queue.open(deviceId); + + // First offer never settles on its own, so it's still holding the queue when cleared + const pendingPromise = queue.offer(deviceInfo, () => new Promise(() => {})); + + queue.clear(deviceId, 'revoked'); + + const result = await pendingPromise; + expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); + }); + + it('closes a device whose offer resolves after the queue was cleared, without accepting it', async () => { + const accept = vi.fn(); + const queue = new DetectedDeviceOfferQueue(accept, mockedLogger); + queue.open(deviceId); + + let resolveOffer!: (device: AnyDevice) => void; + const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); + const resultPromise = queue.offer(deviceInfo, () => offerPromise); + + // Device physically disappears while the offer is still in flight. + queue.clear(deviceId, 'revoked'); + + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const closeSpy = vi.spyOn(device, 'close'); + + // The offer only settles now, after the queue was already cleared - the caller + // already got a rejected result above, so this device must never be accepted. + resolveOffer(device); + + const result = await resultPromise; + expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); + + await vi.waitFor(() => expect(closeSpy).toHaveBeenCalled()); + expect(accept).not.toHaveBeenCalled(); + }); + + it('does not corrupt a fresh, still-pending queue when a stale offer fails after a clear', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + queue.open(deviceId); + + let rejectStaleOffer!: (reason: unknown) => void; + const staleOfferPromise = new Promise((_resolve, reject) => { rejectStaleOffer = reject; }); + const staleResultPromise = queue.offer(deviceInfo, () => staleOfferPromise); + + // Device physically disappears while the stale offer is still in flight. + queue.clear(deviceId, 'revoked'); + + // Re-opened under the same detection id (e.g. redetected) - a fresh queue now + // exists, with its own still-pending offer. + queue.open(deviceId); + + let resolveFreshOffer!: (device: AnyDevice) => void; + const freshOfferPromise = new Promise((resolve) => { resolveFreshOffer = resolve; }); + const freshResultPromise = queue.offer(deviceInfo, () => freshOfferPromise); + + // The stale offer only fails now, well after it was cleared and superseded - while + // the fresh offer is still pending. Without the staleness guard, this would + // incorrectly wipe the still-valid, currently in-flight fresh queue. + rejectStaleOffer(new Error('stale offer failed')); + await staleResultPromise; + + // A second offer for the same detection id right now must still be queued up behind + // the fresh offer, not told the device is unavailable (which is what would happen if + // the stale processing had wrongly wiped the still-valid queue). + const secondResultPromise = queue.offer(deviceInfo, () => Promise.reject(new Error('should never run'))); + + const freshDevice = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + resolveFreshOffer(freshDevice); + + const [freshResult, secondResult] = await Promise.all([freshResultPromise, secondResultPromise]); + + expect(freshResult).toStrictEqual({ successful: true, device: freshDevice }); + expect(secondResult.successful).toBe(false); + // Specifically "claimed by another provider" (queued behind the still-valid fresh + // queue) - not "not available anymore for offering", which would mean the queue was + // wrongly wiped by the stale processing. + expect(!secondResult.successful && (secondResult.reason as Error).message).toContain('claimed by another provider'); + }); + }); + + describe('clearAll', () => { + it('clears every open queue', async () => { + const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const otherDeviceId = DeviceId.create('device-2'); + + queue.open(deviceId); + queue.open(otherDeviceId); + + const firstResultPromise = queue.offer(deviceInfo, () => new Promise(() => {})); + const secondResultPromise = queue.offer({ type: 'test', detectionId: otherDeviceId }, () => new Promise(() => {})); + + queue.clearAll('reset'); + + const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); + + expect(firstResult.successful).toBe(false); + expect(secondResult.successful).toBe(false); + expect(queue.has(deviceId)).toBe(false); + expect(queue.has(otherDeviceId)).toBe(false); + }); + }); +}); diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index a2efc7bb..56c43da1 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -217,15 +217,6 @@ describe('deviceManager', () => { mockedEventEmitter.emit.mockReturnValue(true); }); - it('rejects with DeviceOfferRejectedError when device is not in the detect queue', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - - const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); - - expect(result.successful).toBe(false); - expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); - }); - it('runs the first offer immediately and adds the device on success', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); @@ -237,68 +228,6 @@ describe('deviceManager', () => { expect(manager.getConnectedDevices()).toContain(device); }); - it('does not run a second offer while the first is still pending', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - - let resolveFirstOffer!: (device: AnyDevice) => void; - const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); - const secondOfferFn = vi.fn(() => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); - - const firstResultPromise = manager.offerDevice(deviceInfo, () => firstOfferPromise); - manager.offerDevice(deviceInfo, secondOfferFn); - - expect(secondOfferFn).not.toHaveBeenCalled(); - - resolveFirstOffer(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter())); - await firstResultPromise; - }); - - it('hands off to the next queued offer when the first one throws', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - - const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - const offerError = new Error('connection failed'); - - const firstResultPromise = manager.offerDevice(deviceInfo, () => Promise.reject(offerError)); - const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.resolve(device)); - - const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); - - expect(firstResult).toStrictEqual({ successful: false, reason: offerError }); - expect(secondResult).toStrictEqual({ successful: true, device }); - }); - - it('hands off to the next queued offer when the first device is disabled', async () => { - // Detection id itself must stay enabled/unknown so announce() actually creates the - // queue - only the canonical id of the first offered device (learned only once - // connected, e.g. during a handshake) is disabled. - const localDetectionId = DeviceId.create('device-2-detection'); - const localDeviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: localDetectionId }; - const disabledCanonicalId = DeviceId.create('device-2-disabled-canonical'); - - const settings = new Settings(); - settings.addKnownDevice(new KnownDevice(disabledCanonicalId, 'Foo', 'test', 'test', {}, false)); - const settingsManager = mock(); - settingsManager.getSettings.mockReturnValue(settings); - - const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); - manager.announceDetectedDevice(localDeviceInfo); - - const disabledDevice = new TestDevice(disabledCanonicalId, 'Foo', new Date(), false, new EventEmitter()); - const enabledDevice = new TestDevice(DeviceId.create('device-2-enabled-canonical'), 'Foo', new Date(), false, new EventEmitter()); - - const firstResultPromise = manager.offerDevice(localDeviceInfo, () => Promise.resolve(disabledDevice)); - const secondResultPromise = manager.offerDevice(localDeviceInfo, () => Promise.resolve(enabledDevice)); - - const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); - - expect(firstResult.successful).toBe(false); - expect(!firstResult.successful && firstResult.reason).toBeInstanceOf(DeviceOfferRejectedError); - expect(secondResult).toStrictEqual({ successful: true, device: enabledDevice }); - }); - it('clears the queue and re-allows announcing after the only offer fails', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); @@ -312,26 +241,6 @@ describe('deviceManager', () => { expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); }); - - it('rejects other queued offers with DeviceOfferRejectedError once a device is claimed', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - - let resolveFirstOffer!: (device: AnyDevice) => void; - const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); - const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - - const firstResultPromise = manager.offerDevice(deviceInfo, () => firstOfferPromise); - const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.reject(new Error('should never run'))); - - resolveFirstOffer(device); - - const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); - - expect(firstResult).toStrictEqual({ successful: true, device }); - expect(secondResult.successful).toBe(false); - expect(!secondResult.successful && secondResult.reason).toBeInstanceOf(DeviceOfferRejectedError); - }); }); describe('revokeDetectedDevice', () => { @@ -347,90 +256,6 @@ describe('deviceManager', () => { mockedEventEmitter.emit.mockReturnValue(true); }); - it('resolves a pending offer with failure', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - - // First offer never settles on its own, so it's still holding the queue when revoked - const pendingPromise = manager.offerDevice(deviceInfo, () => new Promise(() => {})); - - manager.revokeDetectedDevice(deviceInfo); - - const result = await pendingPromise; - expect(result.successful).toBe(false); - expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); - }); - - it('closes a device whose offer resolves after the queue was revoked, without registering it', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - - let resolveOffer!: (device: AnyDevice) => void; - const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); - const resultPromise = manager.offerDevice(deviceInfo, () => offerPromise); - - // Device physically disappears while the offer is still in flight. - manager.revokeDetectedDevice(deviceInfo); - - const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - const closeSpy = vi.spyOn(device, 'close'); - - // The offer only settles now, after the queue was already cleared - the caller - // already got a rejected result above, so this device must never be registered. - resolveOffer(device); - - const result = await resultPromise; - expect(result.successful).toBe(false); - expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); - - await vi.waitFor(() => expect(closeSpy).toHaveBeenCalled()); - expect(manager.getConnectedDevices()).toHaveLength(0); - }); - - it('does not corrupt a fresh, still-pending queue when a stale offer fails after a revoke', async () => { - const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - - let rejectStaleOffer!: (reason: unknown) => void; - const staleOfferPromise = new Promise((_resolve, reject) => { rejectStaleOffer = reject; }); - const staleResultPromise = manager.offerDevice(deviceInfo, () => staleOfferPromise); - - // Device physically disappears while the stale offer is still in flight. - manager.revokeDetectedDevice(deviceInfo); - - // Re-announced under the same detection id (e.g. redetected) - a fresh queue now - // exists, with its own still-pending offer. - manager.announceDetectedDevice(deviceInfo); - - let resolveFreshOffer!: (device: AnyDevice) => void; - const freshOfferPromise = new Promise((resolve) => { resolveFreshOffer = resolve; }); - const freshResultPromise = manager.offerDevice(deviceInfo, () => freshOfferPromise); - - // The stale offer only fails now, well after it was revoked and superseded - while - // the fresh offer is still pending. Without the advanceQueue() staleness guard, this - // would incorrectly delete the map entry currently pointing at the fresh, still-in- - // flight queue. - rejectStaleOffer(new Error('stale offer failed')); - await staleResultPromise; - - // A second provider trying the same detection id right now must still be queued up - // behind the fresh offer, not told the device is unavailable (which is what would - // happen if the stale advanceQueue() call had wrongly wiped the still-valid queue). - const secondResultPromise = manager.offerDevice(deviceInfo, () => Promise.reject(new Error('should never run'))); - - const freshDevice = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - resolveFreshOffer(freshDevice); - - const [freshResult, secondResult] = await Promise.all([freshResultPromise, secondResultPromise]); - - expect(freshResult).toStrictEqual({ successful: true, device: freshDevice }); - expect(secondResult.successful).toBe(false); - // Specifically "claimed by another provider" (queued behind the still-valid fresh - // queue) - not "not available anymore for offering", which would mean the queue was - // wrongly wiped by the stale advanceQueue() call. - expect(!secondResult.successful && (secondResult.reason as Error).message).toContain('claimed by another provider'); - }); - it('drops a disabled device from pending retry so it is not re-announced after re-enabling', async () => { const settings = new Settings(); settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, false)); From 9f5def9e5d58a9991871e99b7c16b69a3205fa02 Mon Sep 17 00:00:00 2001 From: HRS Date: Wed, 29 Jul 2026 20:22:31 +0200 Subject: [PATCH 11/24] Remove injected acceptor from DetectedDeviceOfferQueue - DetectedDeviceOfferQueue no longer takes a DeviceOfferAcceptor callback - it's now a pure race/hand-off/cancellation queue with no opinion on what 'accepted' means. deviceOffer may resolve with either the connected device or a DeviceOfferRejectedError value, checked via instanceof - DeviceManager.offerDevice() wraps the disabled-device check itself in the deviceOffer closure passed to the queue, and on a successful result calls the new registerDevice() (renamed from addDevice, now pure registration with no accept/reject decision-making) - Update detectedDeviceOfferQueue.spec.ts to match: rejection tests now resolve deviceOffer with a DeviceOfferRejectedError directly instead of injecting a mock acceptor --- src/device/detectedDeviceOfferQueue.ts | 166 ++++++++---------- src/device/deviceManager.ts | 42 +++-- .../device/detectedDeviceOfferQueue.spec.ts | 69 ++++---- 3 files changed, 136 insertions(+), 141 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index 29ca26e3..76092f85 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -1,3 +1,4 @@ +import { CancellationToken, cancellationTokenReasons, sequentialTaskQueueEvents, SequentialTaskQueue } from 'sequential-task-queue'; import { AnyDevice } from './device.js'; import { DeviceDetectionInfo } from './deviceManager.js'; import DeviceOfferRejectedError from './deviceOfferRejectedError.js'; @@ -8,33 +9,30 @@ export type OfferResult = | { successful: true, device: D } | { successful: false, reason: unknown }; -/** - * Decides whether a device offered through the queue is actually accepted (e.g. registered in - * the device registry). Returns `undefined` on acceptance, or the rejection reason otherwise. - */ -export type DeviceOfferAcceptor = (deviceDetectionInfo: DeviceDetectionInfo, device: AnyDevice) => DeviceOfferRejectedError | undefined; - -type PendingDetectedDeviceOffer = { - deviceDetectionInfo: DeviceDetectionInfo; - deviceOffer: () => Promise; - resolve: (result: OfferResult) => void; -}; - /** * Serializes competing offers for the same detected device (e.g. multiple protocol providers * racing to claim the same serial port) into one queue per detection id, running one offer at a - * time and handing the result to an injected acceptor to decide success or failure. + * time. `deviceOffer` may resolve with either the connected device (accepted) or a + * `DeviceOfferRejectedError` (rejected) - the queue itself has no opinion on what "accepted" + * means, that decision is entirely up to the caller. + * + * Built on `SequentialTaskQueue`, which natively provides what a hand-rolled array-based queue + * would otherwise need to reimplement: cancelling the whole queue (`clear()`/`clearAll()`) + * rejects the currently running offer *and* every still-queued one immediately, without ever + * invoking the callback of an offer that never got its turn. */ export default class DetectedDeviceOfferQueue { - private readonly queues: Map[]> = new Map(); + private readonly queues: Map = new Map(); - private readonly accept: DeviceOfferAcceptor; + // The reason passed to the most recent clear()/clearAll() call for a detection id, so that + // offers cancelled by it (which only carry a generic cancellationTokenReasons sentinel) can + // still be resolved with a meaningful, specific reason. + private readonly clearReasons: Map = new Map(); private readonly logger: Logger; - public constructor(accept: DeviceOfferAcceptor, logger: Logger) { - this.accept = accept; + public constructor(logger: Logger) { this.logger = logger; } @@ -45,108 +43,90 @@ export default class DetectedDeviceOfferQueue public open(detectionId: string): void { - this.queues.set(detectionId, []); - } - - public discard(detectionId: string): void - { - this.queues.delete(detectionId); - } - - public offer(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> - { - return new Promise>((resolve) => { - const deviceQueue = this.queues.get(deviceDetectionInfo.detectionId); + const queue = new SequentialTaskQueue(); - if (undefined === deviceQueue) { - resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is not available anymore for offering`) }); - return; - } + // Fires once the queue has processed every offer (successfully, by failure, or by + // cancellation) and nothing else is pending - drop it so the detection id becomes + // available for a fresh announce again (has() gates on the map key existing). + queue.on(sequentialTaskQueueEvents.drained, () => this.queues.delete(detectionId)); - // Always add to queue first - deviceQueue.push({ deviceDetectionInfo, deviceOffer, resolve }); - - // If we're first in line, run our offer immediately - if (deviceQueue.length === 1) { - void this.runNextOfferInQueue(deviceQueue); - } - }); + this.queues.set(detectionId, queue); + this.clearReasons.delete(detectionId); } - public clear(detectionId: string, reason: string): void + public discard(detectionId: string): void { - for (const entry of this.queues.get(detectionId) ?? []) { - entry.resolve({ successful: false, reason: new DeviceOfferRejectedError(reason) }); - } - this.queues.delete(detectionId); } - public clearAll(reason: string): void - { - for (const [detectionId] of this.queues) { - this.clear(detectionId, reason); - } - } - - private async runNextOfferInQueue(deviceQueue: PendingDetectedDeviceOffer[]): Promise + public offer(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { - const entry = deviceQueue[0]; + const detectionId = deviceDetectionInfo.detectionId; + const queue = this.queues.get(detectionId); - if (undefined === entry) { - return; + if (undefined === queue) { + return Promise.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${detectionId}' is not available anymore for offering`) }); } - const detectionId = entry.deviceDetectionInfo.detectionId; + const task = queue.push(async (cancellationToken: CancellationToken): Promise> => { + const device = await deviceOffer(); - try { - const device = await entry.deviceOffer(); + if (device instanceof DeviceOfferRejectedError) { + return { successful: false, reason: device }; + } // The queue may have been cleared (revoke/reset) or replaced (a fresh announce for // the same detection id) while this offer was in flight - the caller already got a - // settled result for the old queue, so don't hand it a device it never asked for. - if (this.queues.get(detectionId) !== deviceQueue) { - await device.close() - .catch((e: unknown) => logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e)); - return; + // settled result for it, so don't hand it a device it never asked for. + if (true === cancellationToken.cancelled) { + try { + await device.close(); + } catch (e: unknown) { + logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e); + } + return { successful: false, reason: new DeviceOfferRejectedError(this.clearReasons.get(detectionId) ?? 'Device offer was cancelled') }; } - const rejection = this.accept(entry.deviceDetectionInfo, device); - - if (undefined === rejection) { - entry.resolve({ successful: true, device }); - this.clear(detectionId, `Device '${detectionId}' has been claimed by another provider`); - return; - } + return { successful: true, device }; + }); - this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, rejection); - } catch (e: unknown) { - this.rejectCurrentOfferInQueueAndAdvance(detectionId, deviceQueue, e); - } + return Promise.resolve(task.then( + (result: OfferResult): OfferResult => { + if (result.successful) { + // Success - reject every other still-queued offer for this detection id without + // running them, and drop the queue so a fresh announce can happen later. + this.clear(detectionId, `Device '${detectionId}' has been claimed by another provider`); + } + + return result; + }, + // Only reached if the offer was cancelled while still queued, never even starting + // (our callback above never ran at all) - translate the generic sentinel the same way. + (reason: unknown): OfferResult => ({ + successful: false, + reason: (reason === cancellationTokenReasons.cancel || reason === cancellationTokenReasons.timeout) + ? new DeviceOfferRejectedError(this.clearReasons.get(detectionId) ?? 'Device offer was cancelled') + : reason, + }) + )); } - /** - * Resolves the queue's head entry with the given failure reason, then hands off to the next - * waiter, if any. Deletes the queue entirely once empty so the device can be re-announced - * (DeviceManager.announceDetectedDevice() gates on the queue existing). - */ - private rejectCurrentOfferInQueueAndAdvance(detectionId: string, deviceQueue: PendingDetectedDeviceOffer[], reason: unknown): void + public clear(detectionId: string, reason: string): void { - deviceQueue[0]?.resolve({ successful: false, reason }); + const queue = this.queues.get(detectionId); - // The queue may already have been cleared/replaced (revoke, reset, or a fresh announce - // for the same detection id) - don't shift/delete a queue we no longer own. - if (this.queues.get(detectionId) !== deviceQueue) { - return; + if (undefined !== queue) { + this.clearReasons.set(detectionId, reason); + void queue.cancel(); } - deviceQueue.shift(); + this.queues.delete(detectionId); + } - if (deviceQueue.length === 0) { - this.queues.delete(detectionId); - return; + public clearAll(reason: string): void + { + for (const [detectionId] of this.queues) { + this.clear(detectionId, reason); } - - void this.runNextOfferInQueue(deviceQueue); } } diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index ae5229eb..32f45fb4 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -64,7 +64,7 @@ export default class DeviceManager this.logger = logger.child({ name: DeviceManager.name }); this.connectedDevices = connectedDevices; this.settingsManager = settingsManager; - this.offerQueue = new DetectedDeviceOfferQueue((deviceDetectionInfo, device) => this.addDevice(deviceDetectionInfo, device), this.logger); + this.offerQueue = new DetectedDeviceOfferQueue(this.logger); } public isDeviceEnabled(deviceId: DeviceId): boolean { @@ -102,27 +102,35 @@ export default class DeviceManager this.offerQueue.clear(deviceDetectionInfo.detectionId, `Device with id '${deviceDetectionInfo.detectionId}' has disappeared`); } - public offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> + public async offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { - return this.offerQueue.offer(deviceDetectionInfo, deviceOffer); - } + const result = await this.offerQueue.offer(deviceDetectionInfo, async () => { + const device = await deviceOffer(); - private addDevice(deviceDetectionInfo: DeviceDetectionInfo, device: AnyDevice): DeviceOfferRejectedError | undefined - { - if (!this.isDeviceEnabled(device.getDeviceId)) { - this.logger.info(`Not adding device '${device.getDeviceId}' since it is disabled`); + if (!this.isDeviceEnabled(device.getDeviceId)) { + this.logger.info(`Not adding device '${device.getDeviceId}' since it is disabled`); + + const deviceReleased = device.close() + .catch((e: unknown) => logError(this.logger, `Failed to close disabled device '${device.getDeviceId}'`, e)); - const deviceReleased = device.close() - .catch((e: unknown) => logError(this.logger, `Failed to close disabled device '${device.getDeviceId}'`, e)); + // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it + this.detectedDisabledDevices.set(deviceDetectionInfo.detectionId, { deviceDetectionInfo, canonicalId: device.getDeviceId, deviceReleased }); - // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it - this.detectedDisabledDevices.set(deviceDetectionInfo.detectionId, { deviceDetectionInfo, canonicalId: device.getDeviceId, deviceReleased }); + return new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`); + } + + return device; + }); - return new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`); + if (result.successful) { + this.registerDevice(result.device); } - this.connectedDevices.set(device.getDeviceId, device); + return result; + } + private registerDevice(device: AnyDevice): void + { device.on(DeviceEvent.deviceRefreshed, (d) => this.eventEmitter.emit(DeviceManagerEvent.deviceRefreshed, d)); device.on(DeviceEvent.deviceDisconnected, (d) => { this.connectedDevices.delete(d.getDeviceId); @@ -132,9 +140,9 @@ export default class DeviceManager this.initDeviceRefresher(device); - this.eventEmitter.emit(DeviceManagerEvent.deviceConnected, device); + this.connectedDevices.set(device.getDeviceId, device); - return undefined; + this.eventEmitter.emit(DeviceManagerEvent.deviceConnected, device); } public async onSettingsChanged(): Promise { @@ -163,7 +171,7 @@ export default class DeviceManager this.detectedDisabledDevices.delete(detectionId); - // Make sure a device rejected by addDevice() has finished closing before re-announcing + // Make sure a device rejected for being disabled has finished closing before re-announcing await disabledDetectedDevice.deviceReleased; this.announceDetectedDevice(disabledDetectedDevice.deviceDetectionInfo); diff --git a/tests/unit/device/detectedDeviceOfferQueue.spec.ts b/tests/unit/device/detectedDeviceOfferQueue.spec.ts index 328aa020..b4910be6 100644 --- a/tests/unit/device/detectedDeviceOfferQueue.spec.ts +++ b/tests/unit/device/detectedDeviceOfferQueue.spec.ts @@ -1,7 +1,7 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { mock } from 'vitest-mock-extended'; import { EventEmitter } from 'events'; -import DetectedDeviceOfferQueue, { DeviceOfferAcceptor } from '../../../src/device/detectedDeviceOfferQueue.js'; +import DetectedDeviceOfferQueue from '../../../src/device/detectedDeviceOfferQueue.js'; import DeviceOfferRejectedError from '../../../src/device/deviceOfferRejectedError.js'; import { DeviceDetectionInfo } from '../../../src/device/deviceManager.js'; import { AnyDevice } from '../../../src/device/device.js'; @@ -14,10 +14,6 @@ describe('DetectedDeviceOfferQueue', () => { const deviceId = DeviceId.create('device-1'); const deviceInfo: DeviceDetectionInfo = { type: 'test', detectionId: deviceId }; - // Accepts every device offered - the default acceptor for tests only concerned with queue - // mechanics (ordering, hand-off, staleness), not with acceptance decisions themselves. - const acceptAlways: DeviceOfferAcceptor = () => undefined; - beforeEach(() => { mockedLogger = mock(); mockedLogger.child.mockReturnValue(mockedLogger); @@ -25,7 +21,7 @@ describe('DetectedDeviceOfferQueue', () => { describe('has / open / discard', () => { it('reflects open() and discard()', () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); expect(queue.has(deviceId)).toBe(false); @@ -39,7 +35,7 @@ describe('DetectedDeviceOfferQueue', () => { describe('offer', () => { it('rejects with DeviceOfferRejectedError when the queue does not exist', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); const result = await queue.offer(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); @@ -48,7 +44,7 @@ describe('DetectedDeviceOfferQueue', () => { }); it('runs the first offer immediately and resolves successfully when accepted', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); @@ -58,7 +54,7 @@ describe('DetectedDeviceOfferQueue', () => { }); it('does not run a second offer while the first is still pending', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); let resolveFirstOffer!: (device: AnyDevice) => void; @@ -75,7 +71,7 @@ describe('DetectedDeviceOfferQueue', () => { }); it('hands off to the next queued offer when the first one throws', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); @@ -90,18 +86,14 @@ describe('DetectedDeviceOfferQueue', () => { expect(secondResult).toStrictEqual({ successful: true, device }); }); - it('hands off to the next queued offer when the acceptor rejects the first device', async () => { - const rejection = new DeviceOfferRejectedError('rejected by acceptor'); - const accept = vi.fn() - .mockReturnValueOnce(rejection) - .mockReturnValueOnce(undefined); - const queue = new DetectedDeviceOfferQueue(accept, mockedLogger); - queue.open(deviceId); - - const firstDevice = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + it('hands off to the next queued offer when the first device is rejected', async () => { + const rejection = new DeviceOfferRejectedError('rejected'); const secondDevice = new TestDevice(DeviceId.create('device-1-second'), 'Foo', new Date(), false, new EventEmitter()); - const firstResultPromise = queue.offer(deviceInfo, () => Promise.resolve(firstDevice)); + const queue = new DetectedDeviceOfferQueue(mockedLogger); + queue.open(deviceId); + + const firstResultPromise = queue.offer(deviceInfo, () => Promise.resolve(rejection)); const secondResultPromise = queue.offer(deviceInfo, () => Promise.resolve(secondDevice)); const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); @@ -111,10 +103,10 @@ describe('DetectedDeviceOfferQueue', () => { }); it('reopens for a new offer after the only queued offer is rejected', async () => { - const queue = new DetectedDeviceOfferQueue(() => new DeviceOfferRejectedError('rejected'), mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); - await queue.offer(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); + await queue.offer(deviceInfo, () => Promise.resolve(new DeviceOfferRejectedError('rejected'))); expect(queue.has(deviceId)).toBe(false); @@ -123,7 +115,7 @@ describe('DetectedDeviceOfferQueue', () => { }); it('rejects other queued offers with DeviceOfferRejectedError once a device is claimed', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); let resolveFirstOffer!: (device: AnyDevice) => void; @@ -145,7 +137,7 @@ describe('DetectedDeviceOfferQueue', () => { describe('clear', () => { it('resolves a pending offer with failure', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); // First offer never settles on its own, so it's still holding the queue when cleared @@ -159,13 +151,21 @@ describe('DetectedDeviceOfferQueue', () => { }); it('closes a device whose offer resolves after the queue was cleared, without accepting it', async () => { - const accept = vi.fn(); - const queue = new DetectedDeviceOfferQueue(accept, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); let resolveOffer!: (device: AnyDevice) => void; const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); - const resultPromise = queue.offer(deviceInfo, () => offerPromise); + let offerStarted = false; + const resultPromise = queue.offer(deviceInfo, () => { + offerStarted = true; + return offerPromise; + }); + + // Wait for the offer to actually start running (SequentialTaskQueue starts tasks via + // its scheduler, not synchronously) before clearing, to genuinely simulate a revoke + // while the offer is in flight rather than while it's still merely queued. + await vi.waitFor(() => expect(offerStarted).toBe(true)); // Device physically disappears while the offer is still in flight. queue.clear(deviceId, 'revoked'); @@ -182,16 +182,23 @@ describe('DetectedDeviceOfferQueue', () => { expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); await vi.waitFor(() => expect(closeSpy).toHaveBeenCalled()); - expect(accept).not.toHaveBeenCalled(); }); it('does not corrupt a fresh, still-pending queue when a stale offer fails after a clear', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); queue.open(deviceId); let rejectStaleOffer!: (reason: unknown) => void; const staleOfferPromise = new Promise((_resolve, reject) => { rejectStaleOffer = reject; }); - const staleResultPromise = queue.offer(deviceInfo, () => staleOfferPromise); + let staleOfferStarted = false; + const staleResultPromise = queue.offer(deviceInfo, () => { + staleOfferStarted = true; + return staleOfferPromise; + }); + + // Wait for the stale offer to actually start running before clearing, to genuinely + // simulate a revoke while it's in flight rather than while it's still merely queued. + await vi.waitFor(() => expect(staleOfferStarted).toBe(true)); // Device physically disappears while the stale offer is still in flight. queue.clear(deviceId, 'revoked'); @@ -231,7 +238,7 @@ describe('DetectedDeviceOfferQueue', () => { describe('clearAll', () => { it('clears every open queue', async () => { - const queue = new DetectedDeviceOfferQueue(acceptAlways, mockedLogger); + const queue = new DetectedDeviceOfferQueue(mockedLogger); const otherDeviceId = DeviceId.create('device-2'); queue.open(deviceId); From fd66dc9e746a0464a8d625ccb10b8166ad55549d Mon Sep 17 00:00:00 2001 From: HRS Date: Wed, 29 Jul 2026 20:35:48 +0200 Subject: [PATCH 12/24] Make DetectedDeviceOfferQueue.open() a no-op if already open Prevents open() from replacing an already-active queue, which would orphan whatever offer is still running on the old instance. That orphaned offer keeps running to completion on its own and, if it later succeeds, unconditionally cancels whatever queue currently exists for that detection id - the fresh one just created. Add a regression test verifying a pending offer keeps its place in line across a redundant open() call. --- src/device/detectedDeviceOfferQueue.ts | 16 +++--------- .../device/detectedDeviceOfferQueue.spec.ts | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index 76092f85..c4aba564 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -9,18 +9,6 @@ export type OfferResult = | { successful: true, device: D } | { successful: false, reason: unknown }; -/** - * Serializes competing offers for the same detected device (e.g. multiple protocol providers - * racing to claim the same serial port) into one queue per detection id, running one offer at a - * time. `deviceOffer` may resolve with either the connected device (accepted) or a - * `DeviceOfferRejectedError` (rejected) - the queue itself has no opinion on what "accepted" - * means, that decision is entirely up to the caller. - * - * Built on `SequentialTaskQueue`, which natively provides what a hand-rolled array-based queue - * would otherwise need to reimplement: cancelling the whole queue (`clear()`/`clearAll()`) - * rejects the currently running offer *and* every still-queued one immediately, without ever - * invoking the callback of an offer that never got its turn. - */ export default class DetectedDeviceOfferQueue { private readonly queues: Map = new Map(); @@ -43,6 +31,10 @@ export default class DetectedDeviceOfferQueue public open(detectionId: string): void { + if (this.queues.has(detectionId)) { + return; + } + const queue = new SequentialTaskQueue(); // Fires once the queue has processed every offer (successfully, by failure, or by diff --git a/tests/unit/device/detectedDeviceOfferQueue.spec.ts b/tests/unit/device/detectedDeviceOfferQueue.spec.ts index b4910be6..d71c18e2 100644 --- a/tests/unit/device/detectedDeviceOfferQueue.spec.ts +++ b/tests/unit/device/detectedDeviceOfferQueue.spec.ts @@ -31,6 +31,32 @@ describe('DetectedDeviceOfferQueue', () => { queue.discard(deviceId); expect(queue.has(deviceId)).toBe(false); }); + + it('does not replace an already-open queue, so a pending offer keeps its place in line', async () => { + const queue = new DetectedDeviceOfferQueue(mockedLogger); + queue.open(deviceId); + + let resolveFirstOffer!: (device: AnyDevice) => void; + const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); + const firstResultPromise = queue.offer(deviceInfo, () => firstOfferPromise); + + // Calling open() again while a queue is already active for this detection id must be + // a no-op - it must not replace the queue and orphan the offer already running on it. + queue.open(deviceId); + + const secondOfferFn = vi.fn(() => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); + const secondResultPromise = queue.offer(deviceInfo, secondOfferFn); + + // Give the task scheduler (setImmediate-based) a chance to run - if open() had + // replaced the queue, the second offer would be on its own fresh, empty queue and + // would run here despite the first offer still being unresolved. + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(secondOfferFn).not.toHaveBeenCalled(); + + resolveFirstOffer(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter())); + + await Promise.all([firstResultPromise, secondResultPromise]); + }); }); describe('offer', () => { From db2b59893ab60e843b5e459b2c358dfdfec60a5d Mon Sep 17 00:00:00 2001 From: HRS Date: Wed, 29 Jul 2026 22:10:36 +0200 Subject: [PATCH 13/24] Fix cancellation check and extract runOffer() in DetectedDeviceOfferQueue - Critical fix: the success-path check used 'false === cancellationToken.cancelled', but cancellationToken.cancelled is undefined (not false) until actually cancelled, so the check never matched and every device offer was being treated as cancelled and closed right after connecting. Fixed to 'true !== cancellationToken.cancelled' - Extract the task callback passed to queue.push() into its own private runOffer() method, restructured with an early return for the success case instead of nesting the cancellation handling - Extract the deviceOffer callback signature into a named DeviceOffer type --- src/device/detectedDeviceOfferQueue.ts | 61 ++++++++++++++------------ 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index c4aba564..ec05a668 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -9,6 +9,8 @@ export type OfferResult = | { successful: true, device: D } | { successful: false, reason: unknown }; +type DeviceOffer = () => Promise; + export default class DetectedDeviceOfferQueue { private readonly queues: Map = new Map(); @@ -37,9 +39,7 @@ export default class DetectedDeviceOfferQueue const queue = new SequentialTaskQueue(); - // Fires once the queue has processed every offer (successfully, by failure, or by - // cancellation) and nothing else is pending - drop it so the detection id becomes - // available for a fresh announce again (has() gates on the map key existing). + // Clean up empty queues queue.on(sequentialTaskQueueEvents.drained, () => this.queues.delete(detectionId)); this.queues.set(detectionId, queue); @@ -51,7 +51,7 @@ export default class DetectedDeviceOfferQueue this.queues.delete(detectionId); } - public offer(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> + public offer(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: DeviceOffer): Promise> { const detectionId = deviceDetectionInfo.detectionId; const queue = this.queues.get(detectionId); @@ -60,33 +60,12 @@ export default class DetectedDeviceOfferQueue return Promise.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${detectionId}' is not available anymore for offering`) }); } - const task = queue.push(async (cancellationToken: CancellationToken): Promise> => { - const device = await deviceOffer(); - - if (device instanceof DeviceOfferRejectedError) { - return { successful: false, reason: device }; - } - - // The queue may have been cleared (revoke/reset) or replaced (a fresh announce for - // the same detection id) while this offer was in flight - the caller already got a - // settled result for it, so don't hand it a device it never asked for. - if (true === cancellationToken.cancelled) { - try { - await device.close(); - } catch (e: unknown) { - logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e); - } - return { successful: false, reason: new DeviceOfferRejectedError(this.clearReasons.get(detectionId) ?? 'Device offer was cancelled') }; - } - - return { successful: true, device }; - }); + const task = queue.push((cancellationToken: CancellationToken) => this.runOffer(deviceOffer, detectionId, cancellationToken)); return Promise.resolve(task.then( (result: OfferResult): OfferResult => { if (result.successful) { - // Success - reject every other still-queued offer for this detection id without - // running them, and drop the queue so a fresh announce can happen later. + // Reject every other still-queued offer for this detection id without this.clear(detectionId, `Device '${detectionId}' has been claimed by another provider`); } @@ -103,6 +82,34 @@ export default class DetectedDeviceOfferQueue )); } + private async runOffer( + deviceOffer: DeviceOffer, + detectionId: string, + cancellationToken: CancellationToken + ): Promise> { + const device = await deviceOffer(); + + if (device instanceof DeviceOfferRejectedError) { + return { successful: false, reason: device }; + } + + if (true !== cancellationToken.cancelled) { + return { successful: true, device }; + } + + // In case this offer lost the race against another offer: close the device and reject the offer with a meaningful reason. + try { + await device.close(); + } catch (e: unknown) { + logError(this.logger, `Failed to close device '${device.getDeviceId}' offered after its queue was cleared`, e); + } + + return { + successful: false, + reason: new DeviceOfferRejectedError(this.clearReasons.get(detectionId) ?? 'Device offer was cancelled'), + }; + } + public clear(detectionId: string, reason: string): void { const queue = this.queues.get(detectionId); From 84ca43b2550bfbd73a010daf3e4c4fbd058f7726 Mon Sep 17 00:00:00 2001 From: HRS Date: Wed, 29 Jul 2026 22:28:07 +0200 Subject: [PATCH 14/24] Extract createAndRegisterDevice() from DeviceProvider.handleDeviceDetection() Pulls the device-offer closure (create device, check isStopped(), wire disconnect listener, register in connectedDevices) into its own named private method, mirroring the same extraction done in DetectedDeviceOfferQueue.runOffer(). connectedDevices.set() and the isStopped() check stay together in their original order - moving connectedDevices bookkeeping later would race both the deviceConnected event (listeners could see it before the provider's own bookkeeping is updated) and stop()'s device-closing loop (a device could be registered with DeviceManager but never make it into connectedDevices in time to be closed on shutdown). --- src/device/provider/deviceProvider.ts | 55 +++++++++++++-------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/src/device/provider/deviceProvider.ts b/src/device/provider/deviceProvider.ts index 9e24d9a3..a40003b7 100644 --- a/src/device/provider/deviceProvider.ts +++ b/src/device/provider/deviceProvider.ts @@ -102,35 +102,7 @@ export default abstract class DeviceProvider { - const device = await this.createDevice(deviceDetectionInfo); - - // Provider was stopped while the offer was in flight (or waiting in queue) - don't - // hand a connected device to a stopped provider, treat it like a failed offer instead - if (this.isStopped()) { - try { - await device.close(); - } catch (e: unknown) { - logError(this.logger, `Failed to close device '${device.getDeviceId}' after provider was stopped`, e); - } - throw new Error(`Provider was stopped while connecting device '${deviceDetectionInfo.detectionId}'`); - } - - // Tracked here, before handing the device back to the manager, since addDevice() - // emits deviceConnected synchronously as soon as this offer settles - our own - // bookkeeping must already be in place by then for any listener of that event to see - // consistent state. If the manager ends up rejecting the device anyway (e.g. - // disabled), its own close() call fires deviceDisconnected, which the listener below - // uses to roll this back. - device.on(DeviceEvent.deviceDisconnected, (d) => { - this.connectedDevices.delete(d.getDeviceId); - this.logger.info(`Connected devices: ${this.connectedDevices.size}`); - }); - this.connectedDevices.set(device.getDeviceId, device); - this.logger.info(`Connected devices: ${this.connectedDevices.size}`); - - return device; - }); + const result = await this.deviceManager.offerDevice(deviceDetectionInfo, () => this.createAndRegisterDevice(deviceDetectionInfo)); if (!result.successful) { this.logger.info(`Device offer for '${deviceDetectionInfo.detectionId}' was rejected: ${BaseError.normalize(result.reason).message}`); @@ -143,6 +115,31 @@ export default abstract class DeviceProvider + { + const device = await this.createDevice(deviceDetectionInfo); + + // Provider was stopped while the offer was in flight + if (this.isStopped()) { + try { + await device.close(); + } catch (e: unknown) { + logError(this.logger, `Failed to close device '${device.getDeviceId}' after provider was stopped`, e); + } + throw new Error(`Provider was stopped while connecting device '${deviceDetectionInfo.detectionId}'`); + } + + device.on(DeviceEvent.deviceDisconnected, (d) => { + this.connectedDevices.delete(d.getDeviceId); + this.logger.info(`Connected devices: ${this.connectedDevices.size}`); + }); + + this.connectedDevices.set(device.getDeviceId, device); + this.logger.info(`Connected devices: ${this.connectedDevices.size}`); + + return device; + } + protected abstract canHandleDeviceDetectionInfo(deviceDetectionInfo: DeviceDetectionInfo): deviceDetectionInfo is DDI; protected abstract createDevice(deviceDetectionInfo: DDI): Promise; From 627183b087d641082bae525e5185fec092cc080f Mon Sep 17 00:00:00 2001 From: HRS Date: Wed, 29 Jul 2026 23:17:38 +0200 Subject: [PATCH 15/24] Address CodeRabbit PR #99 review threads - detectedDeviceOfferQueue.ts: guard the drained listener in open() with an identity check before deleting from queues, so it can't wipe a fresh queue installed by a later clear()+open() cycle. Confirmed via source-reading and empirical testing against the installed sequential-task-queue version that this isn't currently reachable (cancellation always settles synchronously within cancel()'s own call stack), but the guard is cheap and matches the same defensive pattern used elsewhere in this class - serialDeviceProvider.ts / airoticDeviceProvider.ts: wrap cleanup awaits (port.close() / transport.close()) in their own try/catch in the failure paths, so a failing cleanup no longer silently replaces the original, more meaningful connection/handshake error - matches the log-and-swallow convention used everywhere else close() is called in this codebase --- src/device/detectedDeviceOfferQueue.ts | 7 +++++-- src/device/protocol/airotic/airoticDeviceProvider.ts | 7 ++++++- src/device/provider/serialDeviceProvider.ts | 11 ++++++++--- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index ec05a668..efc60488 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -39,8 +39,11 @@ export default class DetectedDeviceOfferQueue const queue = new SequentialTaskQueue(); - // Clean up empty queues - queue.on(sequentialTaskQueueEvents.drained, () => this.queues.delete(detectionId)); + queue.on(sequentialTaskQueueEvents.drained, () => { + if (this.queues.get(detectionId) === queue) { + this.queues.delete(detectionId); + } + }); this.queues.set(detectionId, queue); this.clearReasons.delete(detectionId); diff --git a/src/device/protocol/airotic/airoticDeviceProvider.ts b/src/device/protocol/airotic/airoticDeviceProvider.ts index 1f51e50d..4d6ee468 100644 --- a/src/device/protocol/airotic/airoticDeviceProvider.ts +++ b/src/device/protocol/airotic/airoticDeviceProvider.ts @@ -3,6 +3,7 @@ import DeviceManager from '../../deviceManager.js'; import AiroticDevice from './airoticDevice.js'; import Logger from '../../../logging/Logger.js'; import { promiseWithTimeout } from '../../../util/async.js'; +import { logError } from '../../../util/error.js'; import BleObserver, { BleDeviceDetectionInfo } from '../../transport/bleObserver.js'; import BleUartDeviceTransport from '../../transport/bleDeviceTransport.js'; import AiroticProtocol from './airoticProtocol.js'; @@ -45,7 +46,11 @@ export default class AiroticDeviceProvider extends BleDeviceProvider((resolve, reject) => { - port.close(err => err ? reject(err) : resolve()); - }); + try { + await new Promise((resolve, reject) => { + port.close(err => err ? reject(err) : resolve()); + }); + } catch (closeError: unknown) { + logError(this.logger, `Failed to close serial port '${portInfo.path}' after a failed connection attempt`, closeError); + } } const error = BaseError.normalize(e); From 040eceb1b79142a67f192c6bf134b6efcbaed309 Mon Sep 17 00:00:00 2001 From: HRS Date: Wed, 29 Jul 2026 23:39:18 +0200 Subject: [PATCH 16/24] Fix TOCTOU race parking a revoked disabled device for retry DeviceManager.offerDevice()'s wrapped closure made its enablement decision (and detectedDisabledDevices side effect) purely based on the raw connect result, with no way to know the offer had already been revoked/reset while connecting was still in flight. A slow connect to a disabled device's canonical id that finished after a revoke would still get parked in detectedDisabledDevices, undoing revokeDetectedDevice()'s own cleanup and causing a spurious re-announce attempt for hardware already known to be gone if later re-enabled. Thread the queue's own CancellationToken into the DeviceOffer callback (DetectedDeviceOfferQueue.runOffer() now passes it through) so the closure can check it before making any enablement decision, closing the device and returning a DeviceOfferRejectedError directly instead. Add a regression test verified to fail without the fix (spurious re-announce after re-enabling) and pass with it. --- src/device/detectedDeviceOfferQueue.ts | 4 +-- src/device/deviceManager.ts | 12 ++++++- tests/unit/device/deviceManager.spec.ts | 45 +++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index efc60488..dfab0334 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -9,7 +9,7 @@ export type OfferResult = | { successful: true, device: D } | { successful: false, reason: unknown }; -type DeviceOffer = () => Promise; +type DeviceOffer = (cancellationToken: CancellationToken) => Promise; export default class DetectedDeviceOfferQueue { @@ -90,7 +90,7 @@ export default class DetectedDeviceOfferQueue detectionId: string, cancellationToken: CancellationToken ): Promise> { - const device = await deviceOffer(); + const device = await deviceOffer(cancellationToken); if (device instanceof DeviceOfferRejectedError) { return { successful: false, reason: device }; diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 32f45fb4..c0845a77 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -104,9 +104,19 @@ export default class DeviceManager public async offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { - const result = await this.offerQueue.offer(deviceDetectionInfo, async () => { + const result = await this.offerQueue.offer(deviceDetectionInfo, async (cancellationToken) => { const device = await deviceOffer(); + if (true === cancellationToken.cancelled) { + try { + await device.close(); + } catch (e: unknown) { + logError(this.logger, `Failed to close device '${device.getDeviceId}' after its offer was cancelled`, e); + } + + return new DeviceOfferRejectedError(`Device offer for '${deviceDetectionInfo.detectionId}' was cancelled`); + } + if (!this.isDeviceEnabled(device.getDeviceId)) { this.logger.info(`Not adding device '${device.getDeviceId}' since it is disabled`); diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index 56c43da1..05ab3663 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -284,6 +284,51 @@ describe('deviceManager', () => { expect(mockedEventEmitter.emit).not.toHaveBeenCalled(); }); + + it('does not park a device for retry if it was revoked while the offer was still connecting', async () => { + const settings = new Settings(); + settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, false)); + + const settingsManager = mock(); + settingsManager.getSettings.mockReturnValue(settings); + + const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); + + manager.announceDetectedDevice(deviceInfo); + + let resolveOffer!: (device: AnyDevice) => void; + const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); + let offerStarted = false; + const resultPromise = manager.offerDevice(deviceInfo, () => { + offerStarted = true; + return offerPromise; + }); + + // Wait for the connect attempt to actually start before revoking, to genuinely + // simulate a revoke while it's in flight rather than while it's still merely queued. + await vi.waitFor(() => expect(offerStarted).toBe(true)); + + // Device physically disappears while the (disabled) device is still connecting. + manager.revokeDetectedDevice(deviceInfo); + + // The connect attempt only succeeds now, after the revoke already settled the caller. + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + resolveOffer(device); + + const result = await resultPromise; + expect(result.successful).toBe(false); + + mockClear(mockedEventEmitter); + mockedEventEmitter.emit.mockReturnValue(true); + + // Re-enabling it must NOT resurrect the gone device - it should never have been + // parked for retry in the first place, since it was already known to be gone by the + // time it "connected". + settings.addKnownDevice(new KnownDevice(deviceId, 'Foo', 'test', 'test', {}, true)); + await manager.onSettingsChanged(); + + expect(mockedEventEmitter.emit).not.toHaveBeenCalled(); + }); }); describe('isDeviceEnabled', () => { From 6b51a0a3b04c1b1206701bd2404d3d6b656d3b41 Mon Sep 17 00:00:00 2001 From: HRS Date: Thu, 30 Jul 2026 08:30:08 +0200 Subject: [PATCH 17/24] Fix CodeRabbit thread 2: discard a queue nobody actually offered to - detectedDeviceOfferQueue.ts: add hadOffers(detectionId), backed by an offeredQueues map storing the exact queue instance an offer was made against (not just a bare Set), so a stale record from a superseded queue generation can never be mistaken for a fresh one - deviceManager.ts: announceDetectedDevice() now checks hadOffers() synchronously right after emit() returns - if listeners exist but none of them actually called offerDevice() (e.g. no subscribed provider's canHandleDeviceDetectionInfo() matched), discard the queue instead of leaving it open forever and permanently blocking future announces for that detection id. Distinct log message from the existing !hadListeners case - deviceManager.spec.ts: add a reactToDetection() helper that wires a mocked EventEmitter to synchronously call offerDevice() on deviceDetected, matching how a real DeviceProvider actually reacts; rework the connectDevice helper and several tests to use it instead of calling offerDevice() as a separate, disconnected statement after announceDetectedDevice() - several of these were previously passing for the wrong reason (accidentally short-circuiting through paths they weren't meant to exercise) No queueMicrotask deferral needed - traced the full real call chain and confirmed every DeviceProvider always calls offerDevice() synchronously within emit()'s own call stack, so the synchronous check is correct for production; an earlier microtask-based attempt was reverted as unnecessary test-shaped complexity leaking into production code --- src/device/detectedDeviceOfferQueue.ts | 18 ++++- src/device/deviceManager.ts | 10 ++- tests/unit/device/deviceManager.spec.ts | 101 ++++++++++++++++++------ 3 files changed, 98 insertions(+), 31 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index dfab0334..538771fa 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -15,11 +15,10 @@ export default class DetectedDeviceOfferQueue { private readonly queues: Map = new Map(); - // The reason passed to the most recent clear()/clearAll() call for a detection id, so that - // offers cancelled by it (which only carry a generic cancellationTokenReasons sentinel) can - // still be resolved with a meaningful, specific reason. private readonly clearReasons: Map = new Map(); + private readonly offeredQueues: Map = new Map(); + private readonly logger: Logger; public constructor(logger: Logger) { @@ -42,16 +41,26 @@ export default class DetectedDeviceOfferQueue queue.on(sequentialTaskQueueEvents.drained, () => { if (this.queues.get(detectionId) === queue) { this.queues.delete(detectionId); + this.offeredQueues.delete(detectionId); } }); this.queues.set(detectionId, queue); this.clearReasons.delete(detectionId); + this.offeredQueues.delete(detectionId); } public discard(detectionId: string): void { this.queues.delete(detectionId); + this.offeredQueues.delete(detectionId); + } + + public hadOffers(detectionId: string): boolean + { + const queue = this.queues.get(detectionId); + + return undefined !== queue && this.offeredQueues.get(detectionId) === queue; } public offer(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: DeviceOffer): Promise> @@ -63,6 +72,8 @@ export default class DetectedDeviceOfferQueue return Promise.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${detectionId}' is not available anymore for offering`) }); } + this.offeredQueues.set(detectionId, queue); + const task = queue.push((cancellationToken: CancellationToken) => this.runOffer(deviceOffer, detectionId, cancellationToken)); return Promise.resolve(task.then( @@ -123,6 +134,7 @@ export default class DetectedDeviceOfferQueue } this.queues.delete(detectionId); + this.offeredQueues.delete(detectionId); } public clearAll(reason: string): void diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index c0845a77..8c38283d 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -88,9 +88,13 @@ export default class DeviceManager const hadListeners = this.eventEmitter.emit(DeviceManagerEvent.deviceDetected, deviceDetectionInfo); - if (!hadListeners) { - // no subscribed providers, remove empty list from offer queue for this device - this.logger.info(`No provider available for detected device with id '${deviceDetectionInfo.detectionId}'`); + if (!this.offerQueue.hadOffers(deviceDetectionInfo.detectionId)) { + if (!hadListeners) { + this.logger.info(`No provider available for detected device with id '${deviceDetectionInfo.detectionId}'`); + } else { + this.logger.info(`No provider could handle detected device with id '${deviceDetectionInfo.detectionId}'`); + } + this.offerQueue.discard(deviceDetectionInfo.detectionId); } } diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index 05ab3663..e32c07e6 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -16,11 +16,32 @@ describe('deviceManager', () => { // device as enabled - the desired default for tests unrelated to the enable/disable feature. const mockedSettingsManager = mock(); - // Announces the device and immediately offers it for connection - the only way to get a - // device registered through the public API now that addDevice() is private. - const connectDevice = async (manager: DeviceManager, deviceInfo: DeviceDetectionInfo, device: AnyDevice) => { + // Configures a mocked EventEmitter to synchronously react to deviceDetected the way a real + // DeviceProvider does (see DeviceProvider.handleDeviceDetection(), which calls offerDevice() + // synchronously within its own emit() dispatch) - announceDetectedDevice()'s hadOffers() + // check depends on this happening synchronously, which a bare mock doesn't do on its own. + const reactToDetection = (mockedEventEmitter: ReturnType>, reaction: () => void) => { + mockedEventEmitter.emit.mockImplementation((event: string | symbol) => { + if (event === DeviceManagerEvent.deviceDetected) { + reaction(); + } + return true; + }); + }; + + // Announces the device and immediately offers it for connection, simulating a provider that + // synchronously reacts to the announcement - the only way to get a device registered through + // the public API now that addDevice() is private. + const connectDevice = (manager: DeviceManager, mockedEventEmitter: ReturnType>, deviceInfo: DeviceDetectionInfo, device: AnyDevice) => { + let offerPromise!: ReturnType; + + reactToDetection(mockedEventEmitter, () => { + offerPromise = manager.offerDevice(deviceInfo, () => Promise.resolve(device)); + }); + manager.announceDetectedDevice(deviceInfo); - return manager.offerDevice(deviceInfo, () => Promise.resolve(device)); + + return offerPromise; }; it('it adds device to managed devices and emits an event', async () => { @@ -41,7 +62,7 @@ describe('deviceManager', () => { expect(deviceManager.getConnectedDevices().length).toBe(0); mockClear(mockedDeviceManagerEventEmitter); // drop the constructor-time noise, if any - await connectDevice(deviceManager, deviceInfo, device); + await connectDevice(deviceManager, mockedDeviceManagerEventEmitter, deviceInfo, device); let actualDevices = deviceManager.getConnectedDevices(); @@ -68,7 +89,7 @@ describe('deviceManager', () => { const deviceManager = new DeviceManager(mockedDeviceManagerEventEmitter, connectedDevices, mockedSettingsManager, mockedLogger); - await connectDevice(deviceManager, deviceInfo, device); + await connectDevice(deviceManager, mockedDeviceManagerEventEmitter, deviceInfo, device); mockClear(mockedDeviceManagerEventEmitter); // Connected device refreshed @@ -94,7 +115,7 @@ describe('deviceManager', () => { const deviceManager = new DeviceManager(mockedDeviceManagerEventEmitter, connectedDevices, mockedSettingsManager, mockedLogger); - await connectDevice(deviceManager, deviceInfo, device); + await connectDevice(deviceManager, mockedDeviceManagerEventEmitter, deviceInfo, device); mockClear(mockedDeviceManagerEventEmitter); // Connected device closed @@ -153,9 +174,15 @@ describe('deviceManager', () => { }); it('does not re-announce a device already in the acquire queue', () => { - mockedEventEmitter.emit.mockReturnValue(true); const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + // Offer never settles on its own, so the queue is still legitimately open (an offer + // is genuinely in progress) when the second announce comes in - that's what's under + // test here, distinct from the "nothing ever offered" discard behavior below. + reactToDetection(mockedEventEmitter, () => { + void manager.offerDevice(deviceInfo, () => new Promise(() => {})); + }); + manager.announceDetectedDevice(deviceInfo); manager.announceDetectedDevice(deviceInfo); @@ -182,6 +209,23 @@ describe('deviceManager', () => { expect(result.successful).toBe(false); }); + it('discards the queue and allows re-announcing when listeners exist but none of them offer a device', () => { + // Simulates a subscribed provider whose canHandleDeviceDetectionInfo() declines this + // detection's type, so it never calls offerDevice() - hadListeners is true, but + // nothing ever offers. + mockedEventEmitter.emit.mockReturnValue(true); + const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + + manager.announceDetectedDevice(deviceInfo); + + mockClear(mockedEventEmitter); + mockedEventEmitter.emit.mockReturnValue(true); + + manager.announceDetectedDevice(deviceInfo); + + expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); + }); + it('still emits deviceDetected even when the detection id matches a disabled known device', () => { // Detection id is preliminary/raw - e.g. for serial ports, multiple protocol // providers share the same detectionId but each computes its own distinct canonical @@ -219,10 +263,9 @@ describe('deviceManager', () => { it('runs the first offer immediately and adds the device on success', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(device)); + const result = await connectDevice(manager, mockedEventEmitter, deviceInfo, device); expect(result).toStrictEqual({ successful: true, device }); expect(manager.getConnectedDevices()).toContain(device); @@ -230,9 +273,14 @@ describe('deviceManager', () => { it('clears the queue and re-allows announcing after the only offer fails', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - await manager.offerDevice(deviceInfo, () => Promise.reject(new Error('connect failed'))); + let resultPromise!: ReturnType; + reactToDetection(mockedEventEmitter, () => { + resultPromise = manager.offerDevice(deviceInfo, () => Promise.reject(new Error('connect failed'))); + }); + + manager.announceDetectedDevice(deviceInfo); + await resultPromise; mockClear(mockedEventEmitter); mockedEventEmitter.emit.mockReturnValue(true); @@ -268,9 +316,8 @@ describe('deviceManager', () => { // Announced (detection id happens to match the disabled known device) - the provider // still gets a chance to offer it, but addDevice() rejects it once connected since // it's disabled, parking it in pending retry. - manager.announceDetectedDevice(deviceInfo); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(device)); + const result = await connectDevice(manager, mockedEventEmitter, deviceInfo, device); expect(result.successful).toBe(false); mockClear(mockedEventEmitter); @@ -294,16 +341,20 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); - manager.announceDetectedDevice(deviceInfo); - let resolveOffer!: (device: AnyDevice) => void; const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); let offerStarted = false; - const resultPromise = manager.offerDevice(deviceInfo, () => { - offerStarted = true; - return offerPromise; + let resultPromise!: ReturnType; + + reactToDetection(mockedEventEmitter, () => { + resultPromise = manager.offerDevice(deviceInfo, () => { + offerStarted = true; + return offerPromise; + }); }); + manager.announceDetectedDevice(deviceInfo); + // Wait for the connect attempt to actually start before revoking, to genuinely // simulate a revoke while it's in flight rather than while it's still merely queued. await vi.waitFor(() => expect(offerStarted).toBe(true)); @@ -402,7 +453,7 @@ describe('deviceManager', () => { const device = new TestDevice(canonicalId, 'Foo', new Date(), false, new EventEmitter()); - const result = await connectDevice(manager, deviceInfo, device); + const result = await connectDevice(manager, mockedEventEmitter, deviceInfo, device); expect(result.successful).toBe(false); expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); @@ -422,7 +473,7 @@ describe('deviceManager', () => { const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - const result = await connectDevice(manager, deviceInfo, device); + const result = await connectDevice(manager, mockedEventEmitter, deviceInfo, device); expect(result.successful).toBe(true); expect(manager.getConnectedDevices()).toHaveLength(1); @@ -452,7 +503,7 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, connectedDevices, settingsManager, mockedLogger); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - await connectDevice(manager, deviceInfo, device); + await connectDevice(manager, mockedEventEmitter, deviceInfo, device); expect(manager.getConnectedDevices()).toHaveLength(1); const disabledSettings = new Settings(); @@ -479,7 +530,7 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, connectedDevices, settingsManager, mockedLogger); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - await connectDevice(manager, deviceInfo, device); + await connectDevice(manager, mockedEventEmitter, deviceInfo, device); await manager.onSettingsChanged(); @@ -508,7 +559,7 @@ describe('deviceManager', () => { // Simulate a provider that connected a device via the detected-device pipeline whose // final id turns out to belong to a disabled device. const device = new TestDevice(canonicalId, 'Foo', new Date(), false, new EventEmitter()); - const result = await connectDevice(manager, deviceInfo, device); + const result = await connectDevice(manager, mockedEventEmitter, deviceInfo, device); expect(result.successful).toBe(false); // Drop the deviceDetected emit from announcing above - only the re-announce below is @@ -543,7 +594,7 @@ describe('deviceManager', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), settingsManager, mockedLogger); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); - await connectDevice(manager, deviceInfo, device); + await connectDevice(manager, mockedEventEmitter, deviceInfo, device); mockClear(mockedEventEmitter); From 22f5555b729c42555c67a7863e7cfb75996a8f03 Mon Sep 17 00:00:00 2001 From: HRS Date: Sat, 1 Aug 2026 13:42:07 +0200 Subject: [PATCH 18/24] Simplify DetectedDeviceOfferQueue to lazy, self-opening queues Switch from sequential-task-queue to the @timesplinter fork, which propagates custom cancellation reasons natively via cancellationToken.reason - removing the clearReasons workaround map this queue previously needed. Make offer() open its own queue lazily (getOrCreateQueue()) instead of requiring announceDetectedDevice() to proactively open/discard one and track hadOffers() via a second map. A detection that nobody recognizes no longer creates any queue at all, so there is nothing to discard. Add revoke()/dropIfRevoked(): a revoked (physically disappeared) detection stays closed as a tombstone instead of being deleted, so a late offer arriving after the revoke sees it and rejects itself rather than reconnecting a device that's already gone. Fix the drained-event handler to not delete a closed queue on its own drain, since revoke's close(true, reason) triggers that same drain. Add a connectedDevices check to offerDevice() so a late offer against an already-claimed device is rejected immediately, and restore the has()-based reentrancy guard in announceDetectedDevice() to avoid re-emitting deviceDetected while a round is still in flight. Update detectedDeviceOfferQueue.spec.ts for the new lazy-open API and add coverage for revoke/dropIfRevoked. Fix two deviceManager.spec.ts tests whose assertions encoded the old proactive-open behavior. --- package-lock.json | 11 +- package.json | 2 +- src/device/detectedDeviceOfferQueue.ts | 90 ++++++----- src/device/deviceManager.ts | 29 ++-- src/device/provider/deviceProviderManager.ts | 2 +- src/device/updater/bufferedDeviceUpdater.ts | 2 +- src/serial/synchronousSerialPort.ts | 2 +- .../device/detectedDeviceOfferQueue.spec.ts | 140 ++++++++++-------- tests/unit/device/deviceManager.spec.ts | 16 +- 9 files changed, 157 insertions(+), 137 deletions(-) diff --git a/package-lock.json b/package-lock.json index a7fede70..2ef25135 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,6 +9,7 @@ "@sinclair/typebox": "^0.34.47", "@stoprocent/noble": "^2.3.17", "@timesplinter/pimple": "^2.1.1", + "@timesplinter/sequential-task-queue": "^1.3.1", "ajv": "^8.17.1", "ajv-formats": "^3.0.1", "buttplug": "^3.2.2", @@ -24,7 +25,6 @@ "read-last-lines": "^1.8.0", "reflect-metadata": "^0.1.13", "say": "^0.16.0", - "sequential-task-queue": "^1.2.1", "serialport": "^13.0.0", "socket.io": "^4.8.3", "speaker": "https://github.com/SlvCtrlPlus/node-speaker/releases/download/v0.1.0/speaker-v0.1.0.tgz", @@ -3171,6 +3171,12 @@ "integrity": "sha512-Sy0nqk5480ZukdvEfx9mCmKMJ+jCnjiXePO7Flf+4RK5W5bs1qyGdExQJZ6tg6SoCKQekRvpaW6GNGl/Zf3WeA==", "license": "LGPL-3.0-or-later" }, + "node_modules/@timesplinter/sequential-task-queue": { + "version": "1.3.1", + "resolved": "https://registry.npmjs.org/@timesplinter/sequential-task-queue/-/sequential-task-queue-1.3.1.tgz", + "integrity": "sha512-eyMiLZZCx/1xJbQ3oQAyG9b/TBXKgBjpVfb1G6CgjIlsgpYTtZv/euSdJDEM4giLDKOC85JJnqrt66tTiY6aRQ==", + "license": "MIT" + }, "node_modules/@tybys/wasm-util": { "version": "0.10.3", "resolved": "https://registry.npmjs.org/@tybys/wasm-util/-/wasm-util-0.10.3.tgz", @@ -8243,9 +8249,6 @@ "url": "https://opencollective.com/express" } }, - "node_modules/sequential-task-queue": { - "version": "1.2.1" - }, "node_modules/serialport": { "version": "13.0.0", "license": "MIT", diff --git a/package.json b/package.json index a63890f6..1bca2260 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,7 @@ "read-last-lines": "^1.8.0", "reflect-metadata": "^0.1.13", "say": "^0.16.0", - "sequential-task-queue": "^1.2.1", + "@timesplinter/sequential-task-queue": "^1.3.1", "serialport": "^13.0.0", "socket.io": "^4.8.3", "speaker": "https://github.com/SlvCtrlPlus/node-speaker/releases/download/v0.1.0/speaker-v0.1.0.tgz", diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index 538771fa..351be4b5 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -1,4 +1,4 @@ -import { CancellationToken, cancellationTokenReasons, sequentialTaskQueueEvents, SequentialTaskQueue } from 'sequential-task-queue'; +import { CancellationToken, sequentialTaskQueueEvents, SequentialTaskQueue } from '@timesplinter/sequential-task-queue'; import { AnyDevice } from './device.js'; import { DeviceDetectionInfo } from './deviceManager.js'; import DeviceOfferRejectedError from './deviceOfferRejectedError.js'; @@ -15,72 +15,54 @@ export default class DetectedDeviceOfferQueue { private readonly queues: Map = new Map(); - private readonly clearReasons: Map = new Map(); - - private readonly offeredQueues: Map = new Map(); - private readonly logger: Logger; public constructor(logger: Logger) { this.logger = logger; } - public has(detectionId: string): boolean + private getOrCreateQueue(detectionId: string): SequentialTaskQueue { - return this.queues.has(detectionId); - } + let queue = this.queues.get(detectionId); - public open(detectionId: string): void - { - if (this.queues.has(detectionId)) { - return; + if (queue !== undefined) { + return queue; } - const queue = new SequentialTaskQueue(); + queue = new SequentialTaskQueue(); queue.on(sequentialTaskQueueEvents.drained, () => { - if (this.queues.get(detectionId) === queue) { + // A revoked (closed) queue must survive its own drain - it's kept around deliberately + // as a tombstone so a late offer can still see it and reject itself. + if (this.queues.get(detectionId) === queue && !queue.isClosed) { this.queues.delete(detectionId); - this.offeredQueues.delete(detectionId); } }); this.queues.set(detectionId, queue); - this.clearReasons.delete(detectionId); - this.offeredQueues.delete(detectionId); - } - - public discard(detectionId: string): void - { - this.queues.delete(detectionId); - this.offeredQueues.delete(detectionId); - } - - public hadOffers(detectionId: string): boolean - { - const queue = this.queues.get(detectionId); - return undefined !== queue && this.offeredQueues.get(detectionId) === queue; + return queue; } public offer(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: DeviceOffer): Promise> { const detectionId = deviceDetectionInfo.detectionId; - const queue = this.queues.get(detectionId); + const queue = this.getOrCreateQueue(detectionId); - if (undefined === queue) { - return Promise.resolve({ successful: false, reason: new DeviceOfferRejectedError(`Device with id '${detectionId}' is not available anymore for offering`) }); + if (queue.isClosed) { + return Promise.resolve({ + successful: false, + reason: new DeviceOfferRejectedError(`Device with id '${detectionId}' is not available anymore for offering`), + }); } - this.offeredQueues.set(detectionId, queue); - - const task = queue.push((cancellationToken: CancellationToken) => this.runOffer(deviceOffer, detectionId, cancellationToken)); + const task = queue.push((cancellationToken: CancellationToken) => this.runOffer(deviceOffer, cancellationToken)); return Promise.resolve(task.then( (result: OfferResult): OfferResult => { if (result.successful) { // Reject every other still-queued offer for this detection id without - this.clear(detectionId, `Device '${detectionId}' has been claimed by another provider`); + this.clear(detectionId, new DeviceOfferRejectedError(`Device '${detectionId}' has been claimed by another provider`)); } return result; @@ -89,16 +71,13 @@ export default class DetectedDeviceOfferQueue // (our callback above never ran at all) - translate the generic sentinel the same way. (reason: unknown): OfferResult => ({ successful: false, - reason: (reason === cancellationTokenReasons.cancel || reason === cancellationTokenReasons.timeout) - ? new DeviceOfferRejectedError(this.clearReasons.get(detectionId) ?? 'Device offer was cancelled') - : reason, + reason: reason, }) )); } private async runOffer( deviceOffer: DeviceOffer, - detectionId: string, cancellationToken: CancellationToken ): Promise> { const device = await deviceOffer(cancellationToken); @@ -120,24 +99,43 @@ export default class DetectedDeviceOfferQueue return { successful: false, - reason: new DeviceOfferRejectedError(this.clearReasons.get(detectionId) ?? 'Device offer was cancelled'), + reason: cancellationToken.reason, }; } - public clear(detectionId: string, reason: string): void + public has(detectionId: string): boolean + { + return this.queues.has(detectionId); + } + + public dropIfRevoked(detectionId: string): void + { + const queue = this.queues.get(detectionId); + + if (queue !== undefined && queue.isClosed) { + this.queues.delete(detectionId); + } + } + + public clear(detectionId: string, reason: DeviceOfferRejectedError): void { const queue = this.queues.get(detectionId); if (undefined !== queue) { - this.clearReasons.set(detectionId, reason); - void queue.cancel(); + void queue.cancel(reason); } this.queues.delete(detectionId); - this.offeredQueues.delete(detectionId); } - public clearAll(reason: string): void + public revoke(detectionId: string, reason: DeviceOfferRejectedError): void + { + const queue = this.getOrCreateQueue(detectionId); + + void queue.close(true, reason); + } + + public clearAll(reason: DeviceOfferRejectedError): void { for (const [detectionId] of this.queues) { this.clear(detectionId, reason); diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 8c38283d..c1689c2f 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -1,6 +1,6 @@ import { AnyDevice, DeviceEvent, DeviceNotification } from './device.js'; import EventEmitter from 'events'; -import { SequentialTaskQueue } from 'sequential-task-queue'; +import { SequentialTaskQueue } from '@timesplinter/sequential-task-queue'; import DeviceState from './deviceState.js'; import { setIntervalAsync } from '../util/async.js'; import Logger from '../logging/Logger.js'; @@ -82,20 +82,16 @@ export default class DeviceManager return; } - this.logger.info(`Detected new device with id ${deviceDetectionInfo.detectionId}`); + this.offerQueue.dropIfRevoked(deviceDetectionInfo.detectionId); - this.offerQueue.open(deviceDetectionInfo.detectionId); + this.logger.info(`Detected new device with id ${deviceDetectionInfo.detectionId}`); const hadListeners = this.eventEmitter.emit(DeviceManagerEvent.deviceDetected, deviceDetectionInfo); - if (!this.offerQueue.hadOffers(deviceDetectionInfo.detectionId)) { - if (!hadListeners) { - this.logger.info(`No provider available for detected device with id '${deviceDetectionInfo.detectionId}'`); - } else { - this.logger.info(`No provider could handle detected device with id '${deviceDetectionInfo.detectionId}'`); - } - - this.offerQueue.discard(deviceDetectionInfo.detectionId); + if (!hadListeners) { + this.logger.info(`No active/started providers. Handling of device detection with id '${deviceDetectionInfo.detectionId}' not possible`); + } else if (!this.offerQueue.has(deviceDetectionInfo.detectionId)) { + this.logger.info(`No provider could handle detected device with id '${deviceDetectionInfo.detectionId}'`); } } @@ -103,11 +99,18 @@ export default class DeviceManager { // A device that physically disappeared should no longer be retried on re-enable this.detectedDisabledDevices.delete(deviceDetectionInfo.detectionId); - this.offerQueue.clear(deviceDetectionInfo.detectionId, `Device with id '${deviceDetectionInfo.detectionId}' has disappeared`); + this.offerQueue.revoke( + deviceDetectionInfo.detectionId, + new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' has disappeared`) + ); } public async offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { + if (this.connectedDevices.has(deviceDetectionInfo.detectionId)) { + return { successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is already connected`) }; + } + const result = await this.offerQueue.offer(deviceDetectionInfo, async (cancellationToken) => { const device = await deviceOffer(); @@ -235,7 +238,7 @@ export default class DeviceManager } } - this.offerQueue.clearAll('Device manager reset'); + this.offerQueue.clearAll(new DeviceOfferRejectedError('Device manager reset')); this.detectedDisabledDevices.clear(); diff --git a/src/device/provider/deviceProviderManager.ts b/src/device/provider/deviceProviderManager.ts index be6617e6..0bd91979 100644 --- a/src/device/provider/deviceProviderManager.ts +++ b/src/device/provider/deviceProviderManager.ts @@ -1,4 +1,4 @@ -import { SequentialTaskQueue } from 'sequential-task-queue'; +import { SequentialTaskQueue } from '@timesplinter/sequential-task-queue'; import Settings from '../../settings/settings.js'; import DeviceSource from '../../settings/deviceSource.js'; import DeviceProviderFactory from './deviceProviderFactory.js'; diff --git a/src/device/updater/bufferedDeviceUpdater.ts b/src/device/updater/bufferedDeviceUpdater.ts index 1f21669c..b58b97bf 100644 --- a/src/device/updater/bufferedDeviceUpdater.ts +++ b/src/device/updater/bufferedDeviceUpdater.ts @@ -1,6 +1,6 @@ import { AnyDevice, DeviceData } from '../device.js'; import DeviceUpdaterInterface from './deviceUpdaterInterface.js'; -import { SequentialTaskQueue } from 'sequential-task-queue'; +import { SequentialTaskQueue } from '@timesplinter/sequential-task-queue'; export default class BufferedDeviceUpdater implements DeviceUpdaterInterface { diff --git a/src/serial/synchronousSerialPort.ts b/src/serial/synchronousSerialPort.ts index 39616b81..4db0ad65 100644 --- a/src/serial/synchronousSerialPort.ts +++ b/src/serial/synchronousSerialPort.ts @@ -1,5 +1,5 @@ import { Readable, Writable } from 'stream'; -import { cancellationTokenReasons, SequentialTaskQueue, TaskOptions } from 'sequential-task-queue'; +import { cancellationTokenReasons, SequentialTaskQueue, TaskOptions } from '@timesplinter/sequential-task-queue'; import { PortInfo } from '@serialport/bindings-interface'; import Logger from '../logging/Logger.js'; import { asyncHandler } from '../util/async.js'; diff --git a/tests/unit/device/detectedDeviceOfferQueue.spec.ts b/tests/unit/device/detectedDeviceOfferQueue.spec.ts index d71c18e2..6d510cea 100644 --- a/tests/unit/device/detectedDeviceOfferQueue.spec.ts +++ b/tests/unit/device/detectedDeviceOfferQueue.spec.ts @@ -19,61 +19,26 @@ describe('DetectedDeviceOfferQueue', () => { mockedLogger.child.mockReturnValue(mockedLogger); }); - describe('has / open / discard', () => { - it('reflects open() and discard()', () => { + describe('has', () => { + it('is false before any offer and true while one is pending', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); expect(queue.has(deviceId)).toBe(false); - queue.open(deviceId); - expect(queue.has(deviceId)).toBe(true); - - queue.discard(deviceId); - expect(queue.has(deviceId)).toBe(false); - }); - - it('does not replace an already-open queue, so a pending offer keeps its place in line', async () => { - const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); - - let resolveFirstOffer!: (device: AnyDevice) => void; - const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); - const firstResultPromise = queue.offer(deviceInfo, () => firstOfferPromise); - - // Calling open() again while a queue is already active for this detection id must be - // a no-op - it must not replace the queue and orphan the offer already running on it. - queue.open(deviceId); - - const secondOfferFn = vi.fn(() => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); - const secondResultPromise = queue.offer(deviceInfo, secondOfferFn); - - // Give the task scheduler (setImmediate-based) a chance to run - if open() had - // replaced the queue, the second offer would be on its own fresh, empty queue and - // would run here despite the first offer still being unresolved. - await new Promise((resolve) => setTimeout(resolve, 20)); - expect(secondOfferFn).not.toHaveBeenCalled(); + const pendingPromise = queue.offer(deviceInfo, () => new Promise(() => {})); - resolveFirstOffer(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter())); + await vi.waitFor(() => expect(queue.has(deviceId)).toBe(true)); - await Promise.all([firstResultPromise, secondResultPromise]); + queue.clear(deviceId, new DeviceOfferRejectedError('test cleanup')); + await pendingPromise; }); }); describe('offer', () => { - it('rejects with DeviceOfferRejectedError when the queue does not exist', async () => { + it('lazily opens a queue and runs the offer even without any prior activity for the detection id', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - - const result = await queue.offer(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); - - expect(result.successful).toBe(false); - expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); - }); - - it('runs the first offer immediately and resolves successfully when accepted', async () => { - const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); - const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const result = await queue.offer(deviceInfo, () => Promise.resolve(device)); expect(result).toStrictEqual({ successful: true, device }); @@ -81,7 +46,6 @@ describe('DetectedDeviceOfferQueue', () => { it('does not run a second offer while the first is still pending', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); let resolveFirstOffer!: (device: AnyDevice) => void; const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); @@ -98,7 +62,6 @@ describe('DetectedDeviceOfferQueue', () => { it('hands off to the next queued offer when the first one throws', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); const offerError = new Error('connection failed'); @@ -117,7 +80,6 @@ describe('DetectedDeviceOfferQueue', () => { const secondDevice = new TestDevice(DeviceId.create('device-1-second'), 'Foo', new Date(), false, new EventEmitter()); const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); const firstResultPromise = queue.offer(deviceInfo, () => Promise.resolve(rejection)); const secondResultPromise = queue.offer(deviceInfo, () => Promise.resolve(secondDevice)); @@ -128,21 +90,22 @@ describe('DetectedDeviceOfferQueue', () => { expect(secondResult).toStrictEqual({ successful: true, device: secondDevice }); }); - it('reopens for a new offer after the only queued offer is rejected', async () => { + it('lazily reopens for a new offer after the only queued offer is rejected', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); await queue.offer(deviceInfo, () => Promise.resolve(new DeviceOfferRejectedError('rejected'))); expect(queue.has(deviceId)).toBe(false); - queue.open(deviceId); + const pendingPromise = queue.offer(deviceInfo, () => new Promise(() => {})); expect(queue.has(deviceId)).toBe(true); + + queue.clear(deviceId, new DeviceOfferRejectedError('test cleanup')); + await pendingPromise; }); it('rejects other queued offers with DeviceOfferRejectedError once a device is claimed', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); let resolveFirstOffer!: (device: AnyDevice) => void; const firstOfferPromise = new Promise((resolve) => { resolveFirstOffer = resolve; }); @@ -164,12 +127,11 @@ describe('DetectedDeviceOfferQueue', () => { describe('clear', () => { it('resolves a pending offer with failure', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); // First offer never settles on its own, so it's still holding the queue when cleared const pendingPromise = queue.offer(deviceInfo, () => new Promise(() => {})); - queue.clear(deviceId, 'revoked'); + queue.clear(deviceId, new DeviceOfferRejectedError('revoked')); const result = await pendingPromise; expect(result.successful).toBe(false); @@ -178,7 +140,6 @@ describe('DetectedDeviceOfferQueue', () => { it('closes a device whose offer resolves after the queue was cleared, without accepting it', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); let resolveOffer!: (device: AnyDevice) => void; const offerPromise = new Promise((resolve) => { resolveOffer = resolve; }); @@ -194,7 +155,7 @@ describe('DetectedDeviceOfferQueue', () => { await vi.waitFor(() => expect(offerStarted).toBe(true)); // Device physically disappears while the offer is still in flight. - queue.clear(deviceId, 'revoked'); + queue.clear(deviceId, new DeviceOfferRejectedError('revoked')); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); const closeSpy = vi.spyOn(device, 'close'); @@ -212,7 +173,6 @@ describe('DetectedDeviceOfferQueue', () => { it('does not corrupt a fresh, still-pending queue when a stale offer fails after a clear', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - queue.open(deviceId); let rejectStaleOffer!: (reason: unknown) => void; const staleOfferPromise = new Promise((_resolve, reject) => { rejectStaleOffer = reject; }); @@ -227,12 +187,10 @@ describe('DetectedDeviceOfferQueue', () => { await vi.waitFor(() => expect(staleOfferStarted).toBe(true)); // Device physically disappears while the stale offer is still in flight. - queue.clear(deviceId, 'revoked'); - - // Re-opened under the same detection id (e.g. redetected) - a fresh queue now - // exists, with its own still-pending offer. - queue.open(deviceId); + queue.clear(deviceId, new DeviceOfferRejectedError('revoked')); + // Re-detected under the same detection id - offer() lazily opens a fresh queue, with + // its own still-pending offer. let resolveFreshOffer!: (device: AnyDevice) => void; const freshOfferPromise = new Promise((resolve) => { resolveFreshOffer = resolve; }); const freshResultPromise = queue.offer(deviceInfo, () => freshOfferPromise); @@ -267,13 +225,10 @@ describe('DetectedDeviceOfferQueue', () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); const otherDeviceId = DeviceId.create('device-2'); - queue.open(deviceId); - queue.open(otherDeviceId); - const firstResultPromise = queue.offer(deviceInfo, () => new Promise(() => {})); const secondResultPromise = queue.offer({ type: 'test', detectionId: otherDeviceId }, () => new Promise(() => {})); - queue.clearAll('reset'); + queue.clearAll(new DeviceOfferRejectedError('reset')); const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); @@ -283,4 +238,61 @@ describe('DetectedDeviceOfferQueue', () => { expect(queue.has(otherDeviceId)).toBe(false); }); }); + + describe('revoke / dropIfRevoked', () => { + it('blocks a subsequent offer even when nothing was ever offered before the revoke', async () => { + const queue = new DetectedDeviceOfferQueue(mockedLogger); + + queue.revoke(deviceId, new DeviceOfferRejectedError('device disappeared')); + + const result = await queue.offer(deviceInfo, () => Promise.reject(new Error('should never run'))); + + expect(result.successful).toBe(false); + expect(!result.successful && result.reason).toBeInstanceOf(DeviceOfferRejectedError); + }); + + it('cancels an in-flight offer and keeps rejecting further offers after the revoke', async () => { + const queue = new DetectedDeviceOfferQueue(mockedLogger); + + let offerStarted = false; + const resultPromise = queue.offer(deviceInfo, () => { + offerStarted = true; + return new Promise(() => {}); + }); + + await vi.waitFor(() => expect(offerStarted).toBe(true)); + + queue.revoke(deviceId, new DeviceOfferRejectedError('device disappeared')); + + const result = await resultPromise; + expect(result.successful).toBe(false); + + // The tombstone must survive the drain triggered by the revoke's own cancellation - + // a late offer arriving right after must still see it and reject itself, instead of + // unknowingly reopening a queue for a device that's already confirmed gone. + const lateResult = await queue.offer(deviceInfo, () => Promise.reject(new Error('should never run'))); + + expect(lateResult.successful).toBe(false); + expect(!lateResult.successful && lateResult.reason).toBeInstanceOf(DeviceOfferRejectedError); + }); + + it('allows a fresh offer to succeed again once dropIfRevoked acknowledges a genuine redetection', async () => { + const queue = new DetectedDeviceOfferQueue(mockedLogger); + + queue.revoke(deviceId, new DeviceOfferRejectedError('device disappeared')); + queue.dropIfRevoked(deviceId); + + const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); + const result = await queue.offer(deviceInfo, () => Promise.resolve(device)); + + expect(result).toStrictEqual({ successful: true, device }); + }); + + it('dropIfRevoked is a no-op when there is nothing to drop', () => { + const queue = new DetectedDeviceOfferQueue(mockedLogger); + + expect(() => queue.dropIfRevoked(deviceId)).not.toThrow(); + expect(queue.has(deviceId)).toBe(false); + }); + }); }); diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index e32c07e6..126bff92 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -18,8 +18,9 @@ describe('deviceManager', () => { // Configures a mocked EventEmitter to synchronously react to deviceDetected the way a real // DeviceProvider does (see DeviceProvider.handleDeviceDetection(), which calls offerDevice() - // synchronously within its own emit() dispatch) - announceDetectedDevice()'s hadOffers() - // check depends on this happening synchronously, which a bare mock doesn't do on its own. + // synchronously within its own emit() dispatch) - announceDetectedDevice()'s post-emit + // offerQueue.has() check depends on this happening synchronously, which a bare mock doesn't + // do on its own. const reactToDetection = (mockedEventEmitter: ReturnType>, reaction: () => void) => { mockedEventEmitter.emit.mockImplementation((event: string | symbol) => { if (event === DeviceManagerEvent.deviceDetected) { @@ -173,12 +174,12 @@ describe('deviceManager', () => { expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); }); - it('does not re-announce a device already in the acquire queue', () => { + it('does not re-announce a device while an offer for it is still in flight', () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); // Offer never settles on its own, so the queue is still legitimately open (an offer // is genuinely in progress) when the second announce comes in - that's what's under - // test here, distinct from the "nothing ever offered" discard behavior below. + // test here, distinct from the "nothing ever offered" behavior below. reactToDetection(mockedEventEmitter, () => { void manager.offerDevice(deviceInfo, () => new Promise(() => {})); }); @@ -199,14 +200,17 @@ describe('deviceManager', () => { expect(mockedEventEmitter.emit).not.toHaveBeenCalled(); }); - it('removes device from queue when no listeners respond to deviceDetected', async () => { + it('still allows a later offer to succeed on its own after no listeners responded to deviceDetected', async () => { mockedEventEmitter.emit.mockReturnValue(false); const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.announceDetectedDevice(deviceInfo); + // announceDetectedDevice() no longer opens/reserves anything proactively - offer() + // lazily opens its own queue, so a provider calling offerDevice() later succeeds + // rather than being told the device is "not available anymore". const result = await manager.offerDevice(deviceInfo, () => Promise.resolve(new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()))); - expect(result.successful).toBe(false); + expect(result.successful).toBe(true); }); it('discards the queue and allows re-announcing when listeners exist but none of them offer a device', () => { From 69e18f16988fedec6178295ffe3dce5d528a8192 Mon Sep 17 00:00:00 2001 From: HRS Date: Sat, 1 Aug 2026 14:08:17 +0200 Subject: [PATCH 19/24] Clean up device offer rejection log messages DeviceOfferRejectedError messages previously repeated the detection id even though the only place that reads them (DeviceProvider's rejection log) already includes it in the surrounding text, and ran through BaseError.normalize() which wraps a plain Error into a "Error: " string - producing lines like: Device offer for '' was rejected: Error: Device '' has been claimed by another provider Drop the redundant id from each message so it reads as a clause after the log line's own id, and read DeviceOfferRejectedError's .message directly instead of normalizing it (still normalized for genuine thrown failures, which aren't ours to pre-format). Reworded the log line itself to "Offer for device detection with id '' ..." to match the actual DeviceDetectionInfo/detectionId terminology used throughout this code. --- src/device/detectedDeviceOfferQueue.ts | 4 ++-- src/device/deviceManager.ts | 8 ++++---- src/device/provider/deviceProvider.ts | 9 +++++---- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index 351be4b5..f1dcbdb7 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -52,7 +52,7 @@ export default class DetectedDeviceOfferQueue if (queue.isClosed) { return Promise.resolve({ successful: false, - reason: new DeviceOfferRejectedError(`Device with id '${detectionId}' is not available anymore for offering`), + reason: new DeviceOfferRejectedError('Device is not available anymore for offering'), }); } @@ -62,7 +62,7 @@ export default class DetectedDeviceOfferQueue (result: OfferResult): OfferResult => { if (result.successful) { // Reject every other still-queued offer for this detection id without - this.clear(detectionId, new DeviceOfferRejectedError(`Device '${detectionId}' has been claimed by another provider`)); + this.clear(detectionId, new DeviceOfferRejectedError('Device has been claimed by another provider')); } return result; diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index c1689c2f..099bd7e0 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -101,14 +101,14 @@ export default class DeviceManager this.detectedDisabledDevices.delete(deviceDetectionInfo.detectionId); this.offerQueue.revoke( deviceDetectionInfo.detectionId, - new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' has disappeared`) + new DeviceOfferRejectedError('Device has disappeared') ); } public async offerDevice(deviceDetectionInfo: DeviceDetectionInfo, deviceOffer: () => Promise): Promise> { if (this.connectedDevices.has(deviceDetectionInfo.detectionId)) { - return { successful: false, reason: new DeviceOfferRejectedError(`Device with id '${deviceDetectionInfo.detectionId}' is already connected`) }; + return { successful: false, reason: new DeviceOfferRejectedError('Device is already connected') }; } const result = await this.offerQueue.offer(deviceDetectionInfo, async (cancellationToken) => { @@ -121,7 +121,7 @@ export default class DeviceManager logError(this.logger, `Failed to close device '${device.getDeviceId}' after its offer was cancelled`, e); } - return new DeviceOfferRejectedError(`Device offer for '${deviceDetectionInfo.detectionId}' was cancelled`); + return new DeviceOfferRejectedError('Device offer was cancelled'); } if (!this.isDeviceEnabled(device.getDeviceId)) { @@ -133,7 +133,7 @@ export default class DeviceManager // Keyed by detection id so revokeDetectedDevice() (which only has that id) can drop it this.detectedDisabledDevices.set(deviceDetectionInfo.detectionId, { deviceDetectionInfo, canonicalId: device.getDeviceId, deviceReleased }); - return new DeviceOfferRejectedError(`Device '${device.getDeviceId}' is disabled, not added`); + return new DeviceOfferRejectedError(`Device with id '${device.getDeviceId}' is currently disabled`); } return device; diff --git a/src/device/provider/deviceProvider.ts b/src/device/provider/deviceProvider.ts index a40003b7..94bb51b1 100644 --- a/src/device/provider/deviceProvider.ts +++ b/src/device/provider/deviceProvider.ts @@ -105,11 +105,12 @@ export default abstract class DeviceProvider this.createAndRegisterDevice(deviceDetectionInfo)); if (!result.successful) { - this.logger.info(`Device offer for '${deviceDetectionInfo.detectionId}' was rejected: ${BaseError.normalize(result.reason).message}`); + if (result.reason instanceof DeviceOfferRejectedError) { + this.logger.info(`Offer for device detection with id '${deviceDetectionInfo.detectionId}' was rejected: ${result.reason.message}`); + } else { + this.logger.info(`Offer for device detection with id '${deviceDetectionInfo.detectionId}' failed: ${BaseError.normalize(result.reason).message}`); - // Only a real connect failure (a thrown offer) warrants provider cleanup - - // manager-level rejections (disabled, claimed elsewhere, revoked, unavailable) don't - if (!(result.reason instanceof DeviceOfferRejectedError)) { + // Only a real connect failure (a thrown offer) warrants provider cleanup await this.onConnectFailed(deviceDetectionInfo); } } From bc9414f89409f546412e340d618d85833078f02c Mon Sep 17 00:00:00 2001 From: HRS Date: Sat, 1 Aug 2026 14:39:54 +0200 Subject: [PATCH 20/24] Fix dropIfRevoked() being unreachable behind the has() reentrancy guard announceDetectedDevice() checked offerQueue.has() before calling dropIfRevoked(), but has() can't distinguish a genuinely in-flight queue from a closed tombstone left by revoke() - both just look like "an entry exists" to it. So once a detection id was ever revoked, has() would return true forever and short-circuit before dropIfRevoked() got a chance to clear the stale tombstone, permanently blocking any future announce for that id. This was invisible to unit tests but broke every integration test that revokes and later reuses the same detection id (which the beforeEach hooks in tests/integration/devices/*.spec.ts do on every run, since SerialPortObserver's managedDevices map isn't reset by DeviceManager.reset() and revokes the previous test's port on the next discoverSerialDevices() call). Fix: run dropIfRevoked() first, before the has() guard, so a stale tombstone is cleared before it's ever consulted. Added a unit test that reproduces this exact sequence (announce, revoke, re-announce) and fails without the fix. --- src/device/deviceManager.ts | 4 ++-- src/device/deviceOfferRejectedError.ts | 6 ------ tests/unit/device/deviceManager.spec.ts | 20 ++++++++++++++++++++ 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 099bd7e0..d4b58eab 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -73,6 +73,8 @@ export default class DeviceManager public announceDetectedDevice(deviceDetectionInfo: DeviceDetectionInfo): void { + this.offerQueue.dropIfRevoked(deviceDetectionInfo.detectionId); + if (this.offerQueue.has(deviceDetectionInfo.detectionId)) { return; } @@ -82,8 +84,6 @@ export default class DeviceManager return; } - this.offerQueue.dropIfRevoked(deviceDetectionInfo.detectionId); - this.logger.info(`Detected new device with id ${deviceDetectionInfo.detectionId}`); const hadListeners = this.eventEmitter.emit(DeviceManagerEvent.deviceDetected, deviceDetectionInfo); diff --git a/src/device/deviceOfferRejectedError.ts b/src/device/deviceOfferRejectedError.ts index e7bdccdb..950592e4 100644 --- a/src/device/deviceOfferRejectedError.ts +++ b/src/device/deviceOfferRejectedError.ts @@ -1,7 +1 @@ -/** - * Marks a device offer rejection as a manager-level decision (queue unavailable, device - * disabled, claimed by another provider, revoked, reset) as opposed to the offer itself failing - * (a thrown error). `DeviceProvider` uses this to decide whether `onConnectFailed()` should run - - * it shouldn't for manager-level decisions, only for the provider's own failed attempt. - */ export default class DeviceOfferRejectedError extends Error {} diff --git a/tests/unit/device/deviceManager.spec.ts b/tests/unit/device/deviceManager.spec.ts index 126bff92..3406c289 100644 --- a/tests/unit/device/deviceManager.spec.ts +++ b/tests/unit/device/deviceManager.spec.ts @@ -191,6 +191,26 @@ describe('deviceManager', () => { expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); }); + it('allows re-announcing a device after it was revoked (tombstone must not permanently block it)', () => { + const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); + + mockedEventEmitter.emit.mockReturnValue(true); + manager.announceDetectedDevice(deviceInfo); + + // Device physically disappears - revoke() leaves a closed tombstone behind (not a + // plain delete) so a late offer arriving after this point still rejects itself. + manager.revokeDetectedDevice(deviceInfo); + + mockClear(mockedEventEmitter); + mockedEventEmitter.emit.mockReturnValue(true); + + // Genuine redetection (e.g. replugged) must not be blocked by that leftover + // tombstone - dropIfRevoked() has to run before the has() reentrancy guard sees it. + manager.announceDetectedDevice(deviceInfo); + + expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); + }); + it('does not emit event when device is already connected', () => { const connectedDevices = new Map([[deviceId, mock()]]); const manager = new DeviceManager(mockedEventEmitter, connectedDevices, mockedSettingsManager, mockedLogger); From 84964fee1b3a88f98696460ace10cb0130bd6ccd Mon Sep 17 00:00:00 2001 From: HRS Date: Sat, 1 Aug 2026 16:09:27 +0200 Subject: [PATCH 21/24] Unify DetectedDeviceOfferQueue on close(), address remaining CodeRabbit nitpicks - close() (renamed from clear(), private), revoke(), and closeAll() (via close()) now all consistently use queue.close(true, reason) as the one "shut this queue down" primitive, instead of the old clear() using the weaker cancel(reason). The only difference between them is what happens to the map entry afterward: close() deletes it, revoke() keeps it as a tombstone, closeAll() deletes all of them. No behavior change for close()'s caller (offer()'s success path) - runOffer() only ever reads cancellationToken.cancelled/.reason, never queue.isClosed, and close() already calls cancel() internally with identical mechanics. - Dropped the this.queues.get(detectionId) === queue identity check in the drained handler, keeping only !queue.isClosed. Traced the scenario it was meant to guard against (a cancelled, still-running task whose underlying deviceOffer() promise settles much later) and found the sequential-task-queue library already prevents it via its own `if (this.currentTask !== task) return;` guard in doneTask() - a task cancelled while running immediately clears currentTask, so its later, real settlement can never re-reach the emit('drained') line a second time. Verified by running the regression test written specifically for this scenario ("does not corrupt a fresh, still-pending queue when a stale offer fails after a clear") and the full suite 3x - all pass. The !queue.isClosed half stays, since that's what makes revoke()'s tombstone survive its own drain (the bug fixed in bc9414f). The drained handler as a whole stays too - it's the only thing that cleans up a queue that completes naturally (e.g. a single offer that fails on its own, never reaching offer()'s close() call at all); without it that entry would block the detectionId from ever being offered again. - Made the old clear() private (deviceManager.ts never called it directly anymore since revoke() took over the "device disappeared" case) and renamed it to close() to match what it actually does now. closeAll() (renamed from clearAll) goes back to looping detectionIds and delegating to close() per entry, now that close() itself uses the close() primitive. - Fixed truncated/inaccurate comments in offer()'s .then() handlers, added a doc comment to has() clarifying it can't distinguish a genuinely active queue from a closed revoke() tombstone (the exact ambiguity behind the dropIfRevoked()-ordering bug in bc9414f), snapshotted detectedDisabledDevices before iterating in applySettingsChange() (announceDetectedDevice() calls inside that loop could otherwise append to the same map mid-iteration), and replaced a fixed setTimeout(10) in deviceProvider.spec.ts's disabled-device test with a deterministic wait on the device's close() spy. Test file updated: direct clear() calls now go through closeAll() (functionally identical when only one queue is in play), folded into a renamed describe('closeAll', ...) block. Kept two of those tests intact rather than treating them as redundant with the "claimed by another provider" test - they exercise the cancel-a-running-task path directly, which that other test's queued-but-never-started offer can't reach (the queue's own scheduler uses setImmediate, a macrotask, while our .then() continuation runs as a microtask that always drains first, so a queued sibling can only ever be cancelled before it starts). Left the Promise.resolve(task.then(...)) wrapper in offer() as-is - task.then() returns a PromiseLike, not a full Promise (per the fork's CancellablePromiseLike extends PromiseLike), so the wrapper is required to satisfy offer()'s Promise> return type, not redundant as originally flagged by CodeRabbit. --- src/device/detectedDeviceOfferQueue.ts | 28 ++++++++++++------- src/device/deviceManager.ts | 4 +-- .../device/detectedDeviceOfferQueue.spec.ts | 23 +++++++-------- .../device/provider/deviceProvider.spec.ts | 13 +++++++-- 4 files changed, 42 insertions(+), 26 deletions(-) diff --git a/src/device/detectedDeviceOfferQueue.ts b/src/device/detectedDeviceOfferQueue.ts index f1dcbdb7..681ffde2 100644 --- a/src/device/detectedDeviceOfferQueue.ts +++ b/src/device/detectedDeviceOfferQueue.ts @@ -34,7 +34,7 @@ export default class DetectedDeviceOfferQueue queue.on(sequentialTaskQueueEvents.drained, () => { // A revoked (closed) queue must survive its own drain - it's kept around deliberately // as a tombstone so a late offer can still see it and reject itself. - if (this.queues.get(detectionId) === queue && !queue.isClosed) { + if (!queue.isClosed) { this.queues.delete(detectionId); } }); @@ -61,14 +61,16 @@ export default class DetectedDeviceOfferQueue return Promise.resolve(task.then( (result: OfferResult): OfferResult => { if (result.successful) { - // Reject every other still-queued offer for this detection id without - this.clear(detectionId, new DeviceOfferRejectedError('Device has been claimed by another provider')); + // Reject every other still-queued offer for this detection id without them + // ever running, since this device has already been claimed. + this.close(detectionId, new DeviceOfferRejectedError('Device has been claimed by another provider')); } return result; }, - // Only reached if the offer was cancelled while still queued, never even starting - // (our callback above never ran at all) - translate the generic sentinel the same way. + // Reached either if the offer was cancelled while still queued (never even starting - + // our callback above never ran) or if deviceOffer() itself rejected/threw uncaught - + // translate both the same way. (reason: unknown): OfferResult => ({ successful: false, reason: reason, @@ -103,6 +105,12 @@ export default class DetectedDeviceOfferQueue }; } + /** + * True if a queue currently exists for this detection id - either genuinely active/in-flight, + * or a closed tombstone left behind by revoke(). Does not distinguish between the two; + * callers that need "is a fresh announce still blocked by a past revoke" must call + * dropIfRevoked() first. + */ public has(detectionId: string): boolean { return this.queues.has(detectionId); @@ -117,12 +125,12 @@ export default class DetectedDeviceOfferQueue } } - public clear(detectionId: string, reason: DeviceOfferRejectedError): void + private close(detectionId: string, reason: DeviceOfferRejectedError): void { const queue = this.queues.get(detectionId); if (undefined !== queue) { - void queue.cancel(reason); + void queue.close(true, reason); } this.queues.delete(detectionId); @@ -135,10 +143,10 @@ export default class DetectedDeviceOfferQueue void queue.close(true, reason); } - public clearAll(reason: DeviceOfferRejectedError): void + public closeAll(reason: DeviceOfferRejectedError): void { - for (const [detectionId] of this.queues) { - this.clear(detectionId, reason); + for (const detectionId of this.queues.keys()) { + this.close(detectionId, reason); } } } diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index d4b58eab..4ca5a859 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -181,7 +181,7 @@ export default class DeviceManager } } - for (const [detectionId, disabledDetectedDevice] of this.detectedDisabledDevices) { + for (const [detectionId, disabledDetectedDevice] of [...this.detectedDisabledDevices]) { if (!this.isDeviceEnabled(disabledDetectedDevice.canonicalId)) { continue; } @@ -238,7 +238,7 @@ export default class DeviceManager } } - this.offerQueue.clearAll(new DeviceOfferRejectedError('Device manager reset')); + this.offerQueue.closeAll(new DeviceOfferRejectedError('Device manager reset')); this.detectedDisabledDevices.clear(); diff --git a/tests/unit/device/detectedDeviceOfferQueue.spec.ts b/tests/unit/device/detectedDeviceOfferQueue.spec.ts index 6d510cea..10810af1 100644 --- a/tests/unit/device/detectedDeviceOfferQueue.spec.ts +++ b/tests/unit/device/detectedDeviceOfferQueue.spec.ts @@ -29,7 +29,7 @@ describe('DetectedDeviceOfferQueue', () => { await vi.waitFor(() => expect(queue.has(deviceId)).toBe(true)); - queue.clear(deviceId, new DeviceOfferRejectedError('test cleanup')); + queue.closeAll(new DeviceOfferRejectedError('test cleanup')); await pendingPromise; }); }); @@ -100,7 +100,7 @@ describe('DetectedDeviceOfferQueue', () => { const pendingPromise = queue.offer(deviceInfo, () => new Promise(() => {})); expect(queue.has(deviceId)).toBe(true); - queue.clear(deviceId, new DeviceOfferRejectedError('test cleanup')); + queue.closeAll(new DeviceOfferRejectedError('test cleanup')); await pendingPromise; }); @@ -124,14 +124,17 @@ describe('DetectedDeviceOfferQueue', () => { }); }); - describe('clear', () => { + describe('closeAll', () => { + // closeAll() replaces clear()'s narrower "just this one id" purpose - these exercise the + // same underlying close()/cancel() mechanics, scoped to a single queue at a time. + it('resolves a pending offer with failure', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); - // First offer never settles on its own, so it's still holding the queue when cleared + // First offer never settles on its own, so it's still holding the queue when closed const pendingPromise = queue.offer(deviceInfo, () => new Promise(() => {})); - queue.clear(deviceId, new DeviceOfferRejectedError('revoked')); + queue.closeAll(new DeviceOfferRejectedError('revoked')); const result = await pendingPromise; expect(result.successful).toBe(false); @@ -155,7 +158,7 @@ describe('DetectedDeviceOfferQueue', () => { await vi.waitFor(() => expect(offerStarted).toBe(true)); // Device physically disappears while the offer is still in flight. - queue.clear(deviceId, new DeviceOfferRejectedError('revoked')); + queue.closeAll(new DeviceOfferRejectedError('revoked')); const device = new TestDevice(deviceId, 'Foo', new Date(), false, new EventEmitter()); const closeSpy = vi.spyOn(device, 'close'); @@ -187,7 +190,7 @@ describe('DetectedDeviceOfferQueue', () => { await vi.waitFor(() => expect(staleOfferStarted).toBe(true)); // Device physically disappears while the stale offer is still in flight. - queue.clear(deviceId, new DeviceOfferRejectedError('revoked')); + queue.closeAll(new DeviceOfferRejectedError('revoked')); // Re-detected under the same detection id - offer() lazily opens a fresh queue, with // its own still-pending offer. @@ -218,17 +221,15 @@ describe('DetectedDeviceOfferQueue', () => { // wrongly wiped by the stale processing. expect(!secondResult.successful && (secondResult.reason as Error).message).toContain('claimed by another provider'); }); - }); - describe('clearAll', () => { - it('clears every open queue', async () => { + it('closes every open queue', async () => { const queue = new DetectedDeviceOfferQueue(mockedLogger); const otherDeviceId = DeviceId.create('device-2'); const firstResultPromise = queue.offer(deviceInfo, () => new Promise(() => {})); const secondResultPromise = queue.offer({ type: 'test', detectionId: otherDeviceId }, () => new Promise(() => {})); - queue.clearAll(new DeviceOfferRejectedError('reset')); + queue.closeAll(new DeviceOfferRejectedError('reset')); const [firstResult, secondResult] = await Promise.all([firstResultPromise, secondResultPromise]); diff --git a/tests/unit/device/provider/deviceProvider.spec.ts b/tests/unit/device/provider/deviceProvider.spec.ts index b6a3bca2..813a57d2 100644 --- a/tests/unit/device/provider/deviceProvider.spec.ts +++ b/tests/unit/device/provider/deviceProvider.spec.ts @@ -262,16 +262,23 @@ describe('DeviceProvider', () => { logger.child.mockReturnValue(logger); const deviceManager = new DeviceManager(new EventEmitter(), new Map(), settingsManager, logger); + + let closeSpy: ReturnType | undefined; const provider = new TrackingTestProvider( deviceManager, - (deviceDetectionInfo) => Promise.resolve(new TestDevice(deviceDetectionInfo.detectionId, 'Foo', new Date(), false, new EventEmitter())) + (deviceDetectionInfo) => { + const device = new TestDevice(deviceDetectionInfo.detectionId, 'Foo', new Date(), false, new EventEmitter()); + closeSpy = vi.spyOn(device, 'close'); + return Promise.resolve(device); + } ); await provider.start(); deviceManager.announceDetectedDevice({ type: 'test', detectionId: deviceId }); - // Flush the announce -> offer -> addDevice -> resolve microtask chain before asserting - await new Promise((resolve) => setTimeout(resolve, 10)); + // Disabled devices are closed internally by offerDevice() once rejected - wait for + // that deterministically instead of a fixed sleep. + await vi.waitFor(() => expect(closeSpy).toHaveBeenCalled()); expect(deviceManager.getConnectedDevices()).toHaveLength(0); expect(provider.onConnectFailedCalls).toBe(0); From 6ab565cf96d7d906a5b58ee50ae75123cb480c81 Mon Sep 17 00:00:00 2001 From: HRS Date: Sat, 1 Aug 2026 17:40:16 +0200 Subject: [PATCH 22/24] Better error handling for failed handshake --- src/device/protocol/airotic/airoticDeviceProvider.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/device/protocol/airotic/airoticDeviceProvider.ts b/src/device/protocol/airotic/airoticDeviceProvider.ts index 4d6ee468..2a483d40 100644 --- a/src/device/protocol/airotic/airoticDeviceProvider.ts +++ b/src/device/protocol/airotic/airoticDeviceProvider.ts @@ -3,7 +3,6 @@ import DeviceManager from '../../deviceManager.js'; import AiroticDevice from './airoticDevice.js'; import Logger from '../../../logging/Logger.js'; import { promiseWithTimeout } from '../../../util/async.js'; -import { logError } from '../../../util/error.js'; import BleObserver, { BleDeviceDetectionInfo } from '../../transport/bleObserver.js'; import BleUartDeviceTransport from '../../transport/bleDeviceTransport.js'; import AiroticProtocol from './airoticProtocol.js'; @@ -46,12 +45,15 @@ export default class AiroticDeviceProvider extends BleDeviceProvider Date: Sat, 1 Aug 2026 18:36:58 +0200 Subject: [PATCH 23/24] Cancel pending offers before closing connected devices in reset() reset() closed all already-connected devices first, then cancelled in-flight offer-queue entries afterward - leaving a window where a not-yet-connected offer could still resolve successfully and get registered via registerDevice() while the close loop was still busy awaiting other devices, landing a new connected device mid-reset. closeAll() itself is fully synchronous and doesn't touch connectedDevices at all, so moving it to the first line of reset() cancels every pending offer before the function ever yields to the event loop, closing that window down to effectively zero instead of "however long the close loop takes". --- src/device/deviceManager.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 4ca5a859..496c6d4f 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -225,6 +225,11 @@ export default class DeviceManager public async reset(): Promise { + // Cancel every pending offer before closing anything - otherwise a not-yet-connected + // offer could still resolve and get registered while the loop below is busy awaiting + // already-connected devices' close(), landing a new device mid-reset. + this.offerQueue.closeAll(new DeviceOfferRejectedError('Device manager reset')); + let closeError: unknown; for (const [, device] of this.connectedDevices) { @@ -238,8 +243,6 @@ export default class DeviceManager } } - this.offerQueue.closeAll(new DeviceOfferRejectedError('Device manager reset')); - this.detectedDisabledDevices.clear(); if (undefined !== closeError) { From 0bf02dc2f0ab68fe1846a8faf59d550cc7ad7324 Mon Sep 17 00:00:00 2001 From: HRS Date: Sat, 1 Aug 2026 18:38:41 +0200 Subject: [PATCH 24/24] Remove comment --- src/device/deviceManager.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/device/deviceManager.ts b/src/device/deviceManager.ts index 496c6d4f..c625cbaa 100644 --- a/src/device/deviceManager.ts +++ b/src/device/deviceManager.ts @@ -225,9 +225,6 @@ export default class DeviceManager public async reset(): Promise { - // Cancel every pending offer before closing anything - otherwise a not-yet-connected - // offer could still resolve and get registered while the loop below is busy awaiting - // already-connected devices' close(), landing a new device mid-reset. this.offerQueue.closeAll(new DeviceOfferRejectedError('Device manager reset')); let closeError: unknown;