diff --git a/src/camera-manager/camera.ts b/src/camera-manager/camera.ts index 67ce76c2..8c89e5ab 100644 --- a/src/camera-manager/camera.ts +++ b/src/camera-manager/camera.ts @@ -17,7 +17,7 @@ import type { Entity, EntityRegistryManager } from '../ha/registry/entity/types' import type { HassStateDifference, HomeAssistant } from '../ha/types'; import { localize } from '../localize/localize'; import type { CapabilitiesRaw, CapabilityKey, Endpoint } from '../types'; -import { arrayify } from '../utils/basic'; +import { arrayify, errorToConsole } from '../utils/basic'; import { isGo2RTCLiveProvider, liveProviderSupports2WayAudio, @@ -56,7 +56,9 @@ export interface CameraInitializationOptions { type DestroyCallback = () => void | Promise; -export class Camera { +export class Camera< + Options extends CameraInitializationOptions = CameraInitializationOptions, +> { protected _config: CameraConfig; protected _engine: CameraManagerEngine; protected _capabilities?: Capabilities; @@ -87,7 +89,7 @@ export class Camera { return this._initialized; } - async initialize(options: CameraInitializationOptions): Promise { + async initialize(options: Options): Promise { // Freeze a single HASS snapshot for the whole (async, multi-step) // initialization so every step observes a consistent entity world; live // subscriptions below still use the manager's current watchers. @@ -96,40 +98,55 @@ export class Camera { return this; } - this._entity = await this._resolveEntity(hass, options); - await this._initialize(hass, options); + // Subscriptions are registered part-way through, so a later failure would + // otherwise strand them on a camera nobody holds a reference to. + try { + this._entity = await this._resolveEntity(hass, options); + await this._initializeBeforeCapabilities(hass, options); - this._capabilities = - options.capabilityOptions?.capabilities ?? - this._capabilities ?? - (await this._buildCapabilities(hass, options)); + this._capabilities = + options.capabilityOptions?.capabilities ?? + this._capabilities ?? + (await this._buildCapabilities(hass, options)); - // The else path is tested, but the `v8` coverage provider miscounts it: a - // missing `else` is given the count of the `if` statement minus the count - // of its body, and the engine only counts code after an `await` for the - // calls that actually paused there. Calls that took an earlier `??` value - // above skipped the `await`, which makes the first number the smaller one - // and the result negative. - // See: https://github.com/AriPerkkio/ast-v8-to-istanbul/issues/148 - /* v8 ignore else -- @preserve */ - if (this._capabilities.has('trigger')) { - await this._getTriggerEntities(hass, options); - this._config.triggers.entities = uniq(this._config.triggers.entities); + // The else path is tested, but the `v8` coverage provider miscounts it: a + // missing `else` is given the count of the `if` statement minus the count + // of its body, and the engine only counts code after an `await` for the + // calls that actually paused there. Calls that took an earlier `??` value + // above skipped the `await`, which makes the first number the smaller one + // and the result negative. + // See: https://github.com/AriPerkkio/ast-v8-to-istanbul/issues/148 + /* v8 ignore else -- @preserve */ + if (this._capabilities.has('trigger')) { + await this._getTriggerEntities(hass, options); + this._config.triggers.entities = uniq(this._config.triggers.entities); - // Subscribe to state based triggers (sync; no race with destroy). - const stateWatcher = options.hassManager.getStateWatcher(); - stateWatcher.subscribe(this._stateChangeHandler, this._config.triggers.entities); - this._onDestroy(() => stateWatcher.unsubscribe(this._stateChangeHandler)); + // Subscribe to state based triggers (sync; no race with destroy). + const stateWatcher = options.hassManager.getStateWatcher(); + stateWatcher.subscribe(this._stateChangeHandler, this._config.triggers.entities); + this._onDestroy(() => stateWatcher.unsubscribe(this._stateChangeHandler)); - // Subscribe to event based triggers. List-form `event_type` expands into - // one subscription per type sharing the same data/context matcher. - const eventWatcher = options.hassManager.getEventWatcher(); - for (const event of this._config.triggers.events) { - for (const request of this._buildEventSubscriptionRequests(event)) { - eventWatcher.subscribe(request); - this._onDestroy(() => eventWatcher.unsubscribe(request)); + // Subscribe to event based triggers. List-form `event_type` expands into + // one subscription per type sharing the same data/context matcher. + const eventWatcher = options.hassManager.getEventWatcher(); + for (const event of this._config.triggers.events) { + for (const request of this._buildEventSubscriptionRequests(event)) { + eventWatcher.subscribe(request); + this._onDestroy(() => eventWatcher.unsubscribe(request)); + } } } + + await this._initializeAfterCapabilities(options); + } catch (e) { + try { + await this.destroy(); + } catch (destroyError: unknown) { + // A camera that cannot clean up must not replace the failure that is + // actually worth reporting. + errorToConsole(destroyError); + } + throw e; } this._initialized = true; @@ -160,7 +177,7 @@ export class Camera { private async _resolveEntity( hass: HomeAssistant, - options: CameraInitializationOptions, + options: Options, ): Promise { const cameraEntityID = getCameraEntityFromConfig(this._config); if (!cameraEntityID || !options.entityRegistryManager) { @@ -175,14 +192,14 @@ export class Camera { */ protected async _getTriggerEntities( hass: HomeAssistant, - options: CameraInitializationOptions, + options: Options, ): Promise { await this._getDoorbellEntities(hass, options); } private async _getDoorbellEntities( hass: HomeAssistant, - options: CameraInitializationOptions, + options: Options, ): Promise { if ( !this._config.triggers.doorbell || @@ -216,16 +233,26 @@ export class Camera { /** * Subclass initialization hook. Override for async initialization work. */ - protected async _initialize( + protected async _initializeBeforeCapabilities( // eslint-disable-next-line @typescript-eslint/no-unused-vars _hass: HomeAssistant, // eslint-disable-next-line @typescript-eslint/no-unused-vars - _options: CameraInitializationOptions, + _options: Options, + ): Promise {} + + /** + * Subclass initialization hook for work that needs the built capabilities. + * Runs inside the initialization guard, so whatever it registers is released + * if it, or anything after it, throws. + */ + protected async _initializeAfterCapabilities( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + _options: Options, ): Promise {} protected async _buildCapabilities( hass: HomeAssistant, - options: CameraInitializationOptions, + options: Options, ): Promise { const rawCapabilities = await this._getRawCapabilities(hass, options); const config = this.getConfig(); @@ -271,7 +298,7 @@ export class Camera { */ protected async _getRawCapabilities( _hass: HomeAssistant, - options: CameraInitializationOptions, + options: Options, ): Promise { return { live: true, diff --git a/src/camera-manager/entity-camera.ts b/src/camera-manager/entity-camera.ts index 149501d2..23cabb9f 100644 --- a/src/camera-manager/entity-camera.ts +++ b/src/camera-manager/entity-camera.ts @@ -8,14 +8,16 @@ import { CameraNoEntityError } from './error'; * subclass turns absence into an error for engines that cannot function * without it (motionEye, Reolink, TPLink). */ -export class EntityCamera extends Camera { - protected override async _initialize( +export class EntityCamera< + Options extends CameraInitializationOptions = CameraInitializationOptions, +> extends Camera { + protected override async _initializeBeforeCapabilities( hass: HomeAssistant, - options: CameraInitializationOptions, + options: Options, ): Promise { if (!this._entity) { throw new CameraNoEntityError(this.getConfig()); } - await super._initialize(hass, options); + await super._initializeBeforeCapabilities(hass, options); } } diff --git a/src/camera-manager/frigate/camera.ts b/src/camera-manager/frigate/camera.ts index 3f20730c..f1f9d8b2 100644 --- a/src/camera-manager/frigate/camera.ts +++ b/src/camera-manager/frigate/camera.ts @@ -46,27 +46,25 @@ export const isBirdseye = (cameraConfig: CameraConfig): boolean => { return cameraConfig.frigate.camera_name === CAMERA_BIRDSEYE; }; -export class FrigateCamera extends Camera { +export class FrigateCamera extends Camera { // Short-circuits subscription when destroy() was invoked while base // initialization was still awaiting. Set BEFORE awaiting `super.destroy()` so // an in-flight initialize() sees the flip immediately. private _destroyed = false; - public async initialize(options: FrigateCameraInitializationOptions): Promise { - await super.initialize(options); - - // A destroy() during the await above means the camera is being torn down; - // it must not register live subscriptions afterward. + protected override async _initializeAfterCapabilities( + options: FrigateCameraInitializationOptions, + ): Promise { + // A destroy() while the base class was still initializing means the camera + // is being torn down; it must not register live subscriptions afterward. if (this._destroyed) { - return this; + return; } if (this._capabilities?.has('trigger')) { this._subscribeToEvents(options.frigateEventWatcher); this._subscribeToReviews(options.frigateReviewWatcher); } - - return this; } public override async destroy(): Promise { @@ -124,7 +122,9 @@ export class FrigateCamera extends Camera { return true; } - protected override async _initialize(hass: HomeAssistant): Promise { + protected override async _initializeBeforeCapabilities( + hass: HomeAssistant, + ): Promise { const config = this.getConfig(); const hasCameraName = !!config.frigate?.camera_name; const cameraEntity = getCameraEntityFromConfig(config); diff --git a/src/camera-manager/manager.ts b/src/camera-manager/manager.ts index eb6f0de8..ad18d7ee 100644 --- a/src/camera-manager/manager.ts +++ b/src/camera-manager/manager.ts @@ -16,15 +16,18 @@ import type { Endpoint } from '../types.js'; import { allPromises, arrayify, + errorToConsole, isTruthy, recursivelyMergeObjectsNotArrays, setify, } from '../utils/basic.js'; import { getCameraID } from '../utils/camera.js'; +import { Generation } from '../utils/concurrency/generation.js'; import { log } from '../utils/debug.js'; import { ViewItemClassifier } from '../view/item-classifier.js'; import type { ViewItem, ViewMedia } from '../view/item.js'; import type { ViewItemCapabilities } from '../view/types.js'; +import type { Camera } from './camera.js'; import { Capabilities } from './capabilities.js'; import { CameraManagerEngineFactory } from './engine-factory.js'; import type { CameraManagerEngine } from './engine.js'; @@ -137,6 +140,14 @@ export class CameraManager { private _store: CameraManagerStore; private _requestLimit = new PQueue(); + // Cameras take time to build, so a teardown or a newer initialization can + // arrive mid-build and leave the finished cameras with no owner. + private _generation = new Generation(); + + // Handing cameras to the store is not atomic, so commits and teardowns run + // one at a time and cannot observe each other half-applied. + private _storeCommits = new PQueue({ concurrency: 1 }); + constructor( api: CardCameraAPI, options?: { @@ -155,6 +166,11 @@ export class CameraManager { } public async initializeCamerasFromConfig(): Promise { + // Taken before the early return below: a call that cannot proceed still + // supersedes an older one whose cameras are being built from a + // configuration that no longer applies. + const generation = this._generation.next(); + const config = this._api.getConfigManager().getConfig(); const hass = this._api.getHASSManager().getHASS(); @@ -173,11 +189,12 @@ export class CameraManager { recursivelyMergeObjectsNotArrays(config?.cameras_global, camera), ); - await this._initializeCameras(cameras); + await this._initializeCameras(cameras, generation); } public async destroy(): Promise { - await this._store.reset(); + this._generation.invalidate(); + await this._storeCommits.add(() => this._store.reset()); } private async _getEnginesForCameras( @@ -221,7 +238,74 @@ export class CameraManager { return output; } - private async _initializeCameras(camerasConfig: CameraConfig[]): Promise { + /** + * Create a camera for each engine, and assign each its ID. An initialized + * camera holds live subscriptions, so either every camera is returned ready + * for the store to own, or none survive: any failure destroys all of them + * before throwing. + */ + private async _createCameras( + engineByConfig: Map, + ): Promise { + // A camera that fails is taken out of the results rather than abandoning + // the others mid-flight, which would leave an initialized camera with + // nobody holding a reference to it. + const failures: unknown[] = []; + const cameras = ( + await allPromises(engineByConfig, ([cameraConfig, engine]) => + engine.createCamera(cameraConfig).catch((error: unknown) => { + failures.push(error); + return null; + }), + ) + ).filter(isTruthy); + + try { + if (failures.length) { + throw failures[0]; + } + + const cameraIDs: Set = new Set(); + + // Do the additions based off the result-order, to ensure the map order is + // preserved. + for (const camera of cameras) { + const cameraID = getCameraID(camera.getConfig()); + + if (!cameraID) { + throw new CameraNoIDError(camera.getConfig()); + } + + if (cameraIDs.has(cameraID)) { + throw new CameraDuplicateIDError(camera.getConfig()); + } + + // Always ensure the actual ID used in the card is in the configuration itself. + camera.setID(cameraID); + cameraIDs.add(cameraID); + } + } catch (e) { + await this._destroyCameras(cameras); + throw e; + } + + return cameras; + } + + private async _destroyCameras(cameras: Camera[]): Promise { + await allPromises(cameras, async (camera) => { + try { + await camera.destroy(); + } catch (error: unknown) { + errorToConsole(error); + } + }); + } + + private async _initializeCameras( + camerasConfig: CameraConfig[], + generation: number, + ): Promise { const initializationStartTime = new Date(); const hass = this._api.getHASSManager().getHASS(); @@ -230,19 +314,13 @@ export class CameraManager { return; } - const hasAutoTriggers = (config: CameraConfig): boolean => { - return ( - config.triggers.motion || config.triggers.occupancy || config.triggers.doorbell - ); - }; + const requiresAutoTriggerDetection = camerasConfig.some( + ({ triggers }) => triggers.motion || triggers.occupancy || triggers.doorbell, + ); - if ( - // If any camera requires automatic trigger detection ... - camerasConfig.some((config) => hasAutoTriggers(config)) - ) { - // ... then we need to populate the entity cache by fetching all entities - // from Home Assistant. Attempt to do this once upfront, to avoid each - // camera doing needing to fetch entity state. + if (requiresAutoTriggerDetection) { + // Populate the entity cache by fetching all entities from Home Assistant + // once upfront, to avoid each camera needing to fetch entity state. await this._api.getEntityRegistryManager().fetchEntityList(hass); } @@ -250,38 +328,21 @@ export class CameraManager { // engine. See: https://github.com/dermotduffy/advanced-camera-card/issues/941 const engineByConfig = await this._getEnginesForCameras(camerasConfig); - // Configuration is initialized in parallel. - const cameras = await allPromises( - engineByConfig.entries(), - async ([cameraConfig, engine]) => await engine.createCamera(cameraConfig), - ); + const cameras = await this._createCameras(engineByConfig); - const destroyCameras = async () => { - await allPromises(cameras, (camera) => camera.destroy()); - }; - const cameraIDs: Set = new Set(); - - // Do the additions based off the result-order, to ensure the map order is - // preserved. - for (const camera of cameras) { - const cameraID = getCameraID(camera.getConfig()); - - if (!cameraID) { - await destroyCameras(); - throw new CameraNoIDError(camera.getConfig()); + // The store mutates incrementally, so staleness is re-checked inside the + // queue rather than before it: a teardown or a later initialization that + // arrives mid-commit would otherwise interleave with this one. + await this._storeCommits.add(async () => { + // Nothing will ever own these cameras, so they are destroyed instead of + // being handed to a store that has moved on. + if (!this._generation.isCurrent(generation)) { + await this._destroyCameras(cameras); + return; } - if (cameraIDs.has(cameraID)) { - await destroyCameras(); - throw new CameraDuplicateIDError(camera.getConfig()); - } - - // Always ensure the actual ID used in the card is in the configuration itself. - camera.setID(cameraID); - cameraIDs.add(cameraID); - } - - await this._store.setCameras(cameras); + await this._store.setCameras(cameras); + }); log( this._api.getConfigManager().getCardWideConfig(), diff --git a/src/camera-manager/reolink/camera.ts b/src/camera-manager/reolink/camera.ts index 962eadb0..2ec2aa52 100644 --- a/src/camera-manager/reolink/camera.ts +++ b/src/camera-manager/reolink/camera.ts @@ -14,7 +14,7 @@ import { type PTZCapabilities, } from '../../types'; import { createSelectOptionAction } from '../../utils/action.js'; -import type { Camera, CameraInitializationOptions } from '../camera'; +import type { CameraInitializationOptions } from '../camera'; import { EntityCamera } from '../entity-camera'; import { ReolinkInitializationError } from '../error'; import type { CameraEndpointsContext, CameraProxyConfig } from '../types'; @@ -77,7 +77,7 @@ const PTZ_BUTTON_ENTITY_KEYS: readonly (keyof PTZButtonEntities)[] = [ 'zoom_out', ]; -export class ReolinkCamera extends EntityCamera { +export class ReolinkCamera extends EntityCamera { // The HostID identifying the camera or NVR. private _reolinkHostID: string | null = null; @@ -90,16 +90,6 @@ export class ReolinkCamera extends EntityCamera { // Entities used for PTZ control. private _ptzEntities: PTZEntities | null = null; - /** - * Reolink cameras require additional options not present in the base class - * initialization options, so this ~empty method is used to expand the type - * expectations. Without this, callers cannot specify objects (e.g. the device - * registry) without TypeScript errors. - */ - public async initialize(options: ReolinkCameraInitializationOptions): Promise { - return super.initialize(options); - } - private async _getChannelFromConfigurationURL( hass: HomeAssistant, deviceRegistryManager: DeviceRegistryManager, @@ -171,11 +161,11 @@ export class ReolinkCamera extends EntityCamera { this._reolinkCameraUID = reolinkCameraUID; } - protected async _initialize( + protected async _initializeBeforeCapabilities( hass: HomeAssistant, options: ReolinkCameraInitializationOptions, ): Promise { - await super._initialize(hass, options); + await super._initializeBeforeCapabilities(hass, options); await this._initializeChannel(hass, options.deviceRegistryManager); this._ptzEntities = await this._getPTZEntities(hass, options.entityRegistryManager); } diff --git a/src/camera-manager/tplink/camera.ts b/src/camera-manager/tplink/camera.ts index 7c2b6c80..27ff4488 100644 --- a/src/camera-manager/tplink/camera.ts +++ b/src/camera-manager/tplink/camera.ts @@ -23,14 +23,14 @@ interface PTZEntities { } type PTZEntity = keyof PTZEntities; -export class TPLinkCamera extends EntityCamera { +export class TPLinkCamera extends EntityCamera { private _ptzEntities: PTZEntities | null = null; - protected async _initialize( + protected async _initializeBeforeCapabilities( hass: HomeAssistant, options: TPLinkCameraInitializationOptions, ): Promise { - await super._initialize(hass, options); + await super._initializeBeforeCapabilities(hass, options); this._ptzEntities = await this._getPTZEntities(hass, options.entityRegistryManager); } diff --git a/src/camera-manager/tplink/engine-tplink.ts b/src/camera-manager/tplink/engine-tplink.ts index e816f599..baa874ff 100644 --- a/src/camera-manager/tplink/engine-tplink.ts +++ b/src/camera-manager/tplink/engine-tplink.ts @@ -12,6 +12,11 @@ import { import { TPLinkCamera } from './camera'; export class TPLinkCameraManagerEngine extends GenericCameraManagerEngine { + // TPLink cameras require a registry manager to resolve their PTZ entities, + // which the constructor below guarantees; the base engine only optionally + // has one. + protected declare _entityRegistryManager: EntityRegistryManager; + constructor( entityRegistryManager: EntityRegistryManager, hassManager: HASSManagerReadonlyInterface, diff --git a/tests/browser/fake-hass.ts b/tests/browser/fake-hass.ts index b7f8f88c..9133f3dc 100644 --- a/tests/browser/fake-hass.ts +++ b/tests/browser/fake-hass.ts @@ -101,6 +101,7 @@ export class FakeHASS { private _isAdmin: boolean; private _handlers = new Map(); private _commandLog: MessageBase[] = []; + private _openEventSubscriptions = 0; constructor(options?: FakeHASSOptions) { this._language = options?.language ?? 'en'; @@ -129,6 +130,13 @@ export class FakeHASS { this._handlers.set(type, handler); } + /** + * Number of event subscriptions not yet released. + */ + public getOpenEventSubscriptionCount(): number { + return this._openEventSubscriptions; + } + /** * Every WebSocket command the card has issued, in order. */ @@ -202,7 +210,13 @@ export class FakeHASS { private _createConnection(): Connection { const connection = mock(); connection.subscribeMessage.mockResolvedValue(() => Promise.resolve()); - connection.subscribeEvents.mockResolvedValue(() => Promise.resolve()); + connection.subscribeEvents.mockImplementation(async () => { + this._openEventSubscriptions++; + return () => { + this._openEventSubscriptions--; + return Promise.resolve(); + }; + }); // `callWS` and `sendMessagePromise` are the same request/response channel, // so both go through one handler table. Given two tables, a command diff --git a/tests/browser/mounted-card.ts b/tests/browser/mounted-card.ts index 89536628..e7e3d843 100644 --- a/tests/browser/mounted-card.ts +++ b/tests/browser/mounted-card.ts @@ -491,6 +491,13 @@ export class MountedCard { this.card.hass = this._hass.getHASS(); } + /** + * How many Home Assistant event subscriptions the card currently holds open. + */ + public getOpenEventSubscriptionCount(): number { + return this._hass.getOpenEventSubscriptionCount(); + } + /** * Hand the card a new `hass` with nothing in it changed. */ diff --git a/tests/camera-manager/camera.test.ts b/tests/camera-manager/camera.test.ts index 77471835..9fcca3e4 100644 --- a/tests/camera-manager/camera.test.ts +++ b/tests/camera-manager/camera.test.ts @@ -726,6 +726,66 @@ describe('Camera', () => { expect(eventWatcher.unsubscribe).toHaveBeenCalled(); }); + it('should release earlier subscriptions when a later subscription throws', async () => { + const camera = new Camera( + createCameraConfig({ + id: 'camera_1', + triggers: { + events: [{ event_type: 'zha_event' }], + }, + }), + new GenericCameraManagerEngine(createHASSManager()), + ); + + const error = new Error('subscribe failed'); + const stateWatcher = mock(); + const eventWatcher = mock(); + vi.mocked(eventWatcher.subscribe).mockImplementation(() => { + throw error; + }); + + await expect( + camera.initialize({ + hassManager: createHASSManager({ stateWatcher, eventWatcher }), + capabilityOptions: { capabilities: createCapabilities({ trigger: true }) }, + }), + ).rejects.toThrow(error); + + // The state subscription was registered before the failure, so nothing + // else can release it. + expect(stateWatcher.subscribe).toHaveBeenCalled(); + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + }); + + it('should report the initialization failure when cleanup also fails', async () => { + const camera = new Camera( + createCameraConfig({ + id: 'camera_1', + triggers: { + events: [{ event_type: 'zha_event' }], + }, + }), + new GenericCameraManagerEngine(createHASSManager()), + ); + + const error = new Error('subscribe failed'); + const stateWatcher = mock(); + vi.mocked(stateWatcher.unsubscribe).mockRejectedValue(new Error('destroy failed')); + const eventWatcher = mock(); + vi.mocked(eventWatcher.subscribe).mockImplementation(() => { + throw error; + }); + + await expect( + camera.initialize({ + hassManager: createHASSManager({ stateWatcher, eventWatcher }), + capabilityOptions: { capabilities: createCapabilities({ trigger: true }) }, + }), + ).rejects.toThrow(error); + + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + }); + it('should attach a context-only matcher when only a context filter is set', async () => { const camera = new Camera( createCameraConfig({ diff --git a/tests/camera-manager/frigate/camera.test.ts b/tests/camera-manager/frigate/camera.test.ts index 5ccbebd0..b45cd344 100644 --- a/tests/camera-manager/frigate/camera.test.ts +++ b/tests/camera-manager/frigate/camera.test.ts @@ -19,6 +19,7 @@ import type { FrigateReviewWatcher, } from '../../../src/camera-manager/frigate/watcher'; import type { ActionsExecutor } from '../../../src/card-controller/actions/types'; +import type { StateWatcherSubscriptionInterface } from '../../../src/card-controller/hass/state-watcher'; import type { PTZAction } from '../../../src/config/schema/actions/custom/ptz'; import type { CameraTriggerMediaEventType } from '../../../src/config/schema/cameras'; import type { @@ -831,6 +832,46 @@ describe('FrigateCamera', () => { ); }); + it('should release base class subscriptions when subscribing throws', async () => { + const camera = new FrigateCamera( + createCameraConfig({ + frigate: { + client_id: 'CLIENT_ID', + camera_name: 'CAMERA', + }, + triggers: { + media_events: ['events'], + entities: ['binary_sensor.motion'], + }, + }), + mock(), + ); + + const error = new Error('subscribe failed'); + const eventWatcher = mock(); + vi.mocked(eventWatcher.subscribe).mockImplementation(() => { + throw error; + }); + + const stateWatcher = mock(); + + await expect( + camera.initialize({ + hassManager: createHASSManager({ stateWatcher }), + entityRegistryManager: mock(), + frigateEventWatcher: eventWatcher, + frigateReviewWatcher: mock(), + }), + ).rejects.toThrow(error); + + // The state subscription belongs to the base class, which registered it + // before `_initializeAfterCapabilities` ran and therefore before this + // failure. Nothing here knows about it, so destroying the whole camera is + // the only thing that can release it. + expect(stateWatcher.subscribe).toHaveBeenCalled(); + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + }); + it('should not subscribe with no trigger events', async () => { const camera = new FrigateCamera( createCameraConfig({ @@ -969,8 +1010,8 @@ describe('FrigateCamera', () => { await camera.destroy(); - // `_destroyed` short-circuits initialize() after the pending await, so - // neither watcher is ever subscribed. + // `_destroyed` short-circuits `_initializeAfterCapabilities`, so neither + // watcher is ever subscribed. expect(eventWatcher.subscribe).not.toHaveBeenCalled(); expect(reviewWatcher.subscribe).not.toHaveBeenCalled(); diff --git a/tests/camera-manager/manager.browser.test.ts b/tests/camera-manager/manager.browser.test.ts new file mode 100644 index 00000000..666a57f8 --- /dev/null +++ b/tests/camera-manager/manager.browser.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it, vi } from 'vitest'; + +import type { RawAdvancedCameraCardConfig } from '../../src/config/types'; +import { MountedCardFactory, type MountedCard } from '../browser/mounted-card'; +import { + createStillCameraHASS, + createStillImageCameraConfig, + createStillImageCardConfig, + getBlockNotificationText, +} from '../browser/test-utils'; + +const INIT_FAILED_ISSUE_HEADING = 'Initialization failed'; + +const TRIGGERING_CAMERA_ENTITY = 'camera.triggering'; +const OTHER_TRIGGERING_CAMERA_ENTITY = 'camera.triggering_too'; + +/** + * A camera that subscribes to a Home Assistant event once initialized, so that + * whether it was cleaned up is externally observable. + */ +const createSubscribingCameraConfig = ( + cameraEntity: string, + cameraID?: string, +): RawAdvancedCameraCardConfig => ({ + ...createStillImageCameraConfig(cameraEntity), + ...(cameraID && { id: cameraID }), + triggers: { + events: [{ event_type: 'acc_test_event' }], + }, +}); + +describe('CameraManager', () => { + it('should release the subscriptions of cameras that initialized before initialization failed', async () => { + // Duplicate identifiers are rejected only after every camera has been + // built, so both cameras are live and subscribed when the failure happens. + const DUPLICATE_ID = 'duplicate'; + + const card = await MountedCardFactory.createFromSource( + createStillImageCardConfig({ + cameras: [ + createSubscribingCameraConfig(TRIGGERING_CAMERA_ENTITY, DUPLICATE_ID), + createSubscribingCameraConfig(OTHER_TRIGGERING_CAMERA_ENTITY, DUPLICATE_ID), + ], + view: { issues: { retry_seconds: 0 } }, + }), + createStillCameraHASS({ + cameras: [TRIGGERING_CAMERA_ENTITY, OTHER_TRIGGERING_CAMERA_ENTITY], + }), + ); + + await vi.waitFor(() => + expect(getBlockNotificationText(card.card)).toContain(INIT_FAILED_ISSUE_HEADING), + ); + + // Both cameras are unreachable once initialization has failed, so nothing + // else could ever release what they subscribed to. + await vi.waitFor(() => expect(card.getOpenEventSubscriptionCount()).toBe(0)); + + card.destroy(); + }); + + it('should release camera subscriptions when the card is taken off the page', async () => { + const card: MountedCard = await MountedCardFactory.createFromSource( + createStillImageCardConfig({ + cameras: [createSubscribingCameraConfig(TRIGGERING_CAMERA_ENTITY)], + }), + createStillCameraHASS({ cameras: [TRIGGERING_CAMERA_ENTITY] }), + ); + + await vi.waitFor(() => expect(card.getOpenEventSubscriptionCount()).toBe(1)); + + card.detach(); + + await vi.waitFor(() => expect(card.getOpenEventSubscriptionCount()).toBe(0)); + + card.destroy(); + }); +}); diff --git a/tests/camera-manager/manager.test.ts b/tests/camera-manager/manager.test.ts index 2f1623db..0b46b7f5 100644 --- a/tests/camera-manager/manager.test.ts +++ b/tests/camera-manager/manager.test.ts @@ -16,6 +16,7 @@ import { CameraQueryClassifier, QueryResultClassifier, } from '../../src/camera-manager/manager.js'; +import type { CameraManagerStore } from '../../src/camera-manager/store.js'; import { Engine, QueryResultsType, @@ -266,6 +267,11 @@ describe('CameraManager', () => { engine?: CameraManagerEngine, cameras: { config?: CameraConfig; + + // Replaces what the engine does for this camera, for cases the default + // path cannot express (e.g. failing, or completing out of order). + createCamera?: (cameraConfig: CameraConfig) => Promise; + engineType?: Engine | null; capabilties?: Capabilities; stateWatcher?: StateWatcherSubscriptionInterface; @@ -290,13 +296,14 @@ describe('CameraManager', () => { camera.engineType === undefined ? Engine.Generic : camera.engineType; if (engineType) { vi.mocked(mockEngine.createCamera).mockImplementationOnce( - async (cameraConfig: CameraConfig): Promise => - await createInitializedCamera( - cameraConfig, - mockEngine, - camera.capabilties ?? createCapabilities(), - camera.stateWatcher, - ), + camera.createCamera ?? + (async (cameraConfig: CameraConfig): Promise => + await createInitializedCamera( + cameraConfig, + mockEngine, + camera.capabilties ?? createCapabilities(), + camera.stateWatcher, + )), ); } vi.mocked(mockFactory.getEngineForCamera).mockResolvedValueOnce(engineType); @@ -437,6 +444,237 @@ describe('CameraManager', () => { expect(order).toEqual(['destroy-done', 'destroy-done', 'throw']); }); + describe('should handle a camera that fails to initialize', () => { + const createAPI = (): CardController => { + const api = createCardAPI(); + vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS()); + return api; + }; + + it('should destroy a camera that initialized before the failure', async () => { + const stateWatcher = mock(); + const error = new Error('initialization failed'); + + const manager = createCameraManager(createAPI(), mock(), [ + { capabilties: createCapabilities({ trigger: true }), stateWatcher }, + { createCamera: () => Promise.reject(error) }, + ]); + + await expect(manager.initializeCamerasFromConfig()).rejects.toThrow(error); + + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + expect(manager.getStore().getCameraCount()).toBe(0); + }); + + it('should destroy a camera that initialized after the failure', async () => { + const engine = mock(); + const stateWatcher = mock(); + const error = new Error('initialization failed'); + + let releaseSlowCamera: () => void = () => {}; + const slowCameraReady = new Promise((resolve) => { + releaseSlowCamera = resolve; + }); + + const manager = createCameraManager(createAPI(), engine, [ + { createCamera: () => Promise.reject(error) }, + { + createCamera: async (cameraConfig) => { + await slowCameraReady; + return await createInitializedCamera( + cameraConfig, + engine, + createCapabilities({ trigger: true }), + stateWatcher, + ); + }, + }, + ]); + + const initialization = manager.initializeCamerasFromConfig(); + releaseSlowCamera(); + + await expect(initialization).rejects.toThrow(error); + + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + }); + + it('should report the initialization failure when a camera cannot be destroyed', async () => { + const error = new Error('initialization failed'); + + const unluckyWatcher = mock(); + vi.mocked(unluckyWatcher.unsubscribe).mockRejectedValue( + new Error('destroy failed'), + ); + + // Destroy completion is observable through the trigger-path + // unsubscribe, without spying on any Camera method. + const slowWatcher = mock(); + let releaseSlowDestroy: () => void = () => {}; + vi.mocked(slowWatcher.unsubscribe).mockImplementation( + () => + new Promise((resolve) => { + releaseSlowDestroy = resolve; + }), + ); + + const cameraEntry = { capabilties: createCapabilities({ trigger: true }) }; + const manager = createCameraManager(createAPI(), mock(), [ + { ...cameraEntry, stateWatcher: unluckyWatcher }, + { ...cameraEntry, stateWatcher: slowWatcher }, + { createCamera: () => Promise.reject(error) }, + ]); + + let settled = false; + const initialization = manager + .initializeCamerasFromConfig() + .catch((e: unknown) => { + settled = true; + throw e; + }); + + await vi.waitFor(() => expect(slowWatcher.unsubscribe).toHaveBeenCalled()); + expect(settled).toBe(false); + + releaseSlowDestroy(); + + // The failing destroy neither masks the initialization error nor + // prevents the other camera from being destroyed. + await expect(initialization).rejects.toThrow(error); + expect(unluckyWatcher.unsubscribe).toHaveBeenCalled(); + }); + }); + + describe('should discard cameras nothing will own', () => { + const createSlowManager = (): { + manager: CameraManager; + stateWatcher: StateWatcherSubscriptionInterface; + releaseCamera: () => void; + } => { + const api = createCardAPI(); + vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS()); + + const engine = mock(); + const stateWatcher = mock(); + + let releaseCamera: () => void = () => {}; + const cameraReady = new Promise((resolve) => { + releaseCamera = resolve; + }); + + const manager = createCameraManager(api, engine, [ + { + createCamera: async (cameraConfig) => { + await cameraReady; + return await createInitializedCamera( + cameraConfig, + engine, + createCapabilities({ trigger: true }), + stateWatcher, + ); + }, + }, + ]); + + return { manager, stateWatcher, releaseCamera: () => releaseCamera() }; + }; + + it('should destroy cameras built after the manager was destroyed', async () => { + const { manager, stateWatcher, releaseCamera } = createSlowManager(); + + const initialization = manager.initializeCamerasFromConfig(); + await manager.destroy(); + releaseCamera(); + await initialization; + + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + expect(manager.getStore().getCameraCount()).toBe(0); + }); + + it('should destroy cameras built by a superseded initialization', async () => { + const { manager, stateWatcher, releaseCamera } = createSlowManager(); + + const superseded = manager.initializeCamerasFromConfig(); + + // A second initialization takes over. It cannot build cameras of its + // own from the exhausted mocks; what is under test is that the first + // initialization's cameras are discarded rather than stored. + const current = manager.initializeCamerasFromConfig(); + releaseCamera(); + + await expect(current).rejects.toThrow(CameraNoEngineError); + await superseded; + + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + expect(manager.getStore().getCameraCount()).toBe(0); + }); + + it('should discard cameras superseded by a call that cannot proceed', async () => { + const { manager, stateWatcher, releaseCamera } = createSlowManager(); + + const superseded = manager.initializeCamerasFromConfig(); + + // A newer call that returns early still supersedes: the cameras being + // built belong to a configuration that no longer applies. + vi.mocked(manager['_api'].getConfigManager().getConfig).mockReturnValue(null); + await manager.initializeCamerasFromConfig(); + + releaseCamera(); + await superseded; + + expect(stateWatcher.unsubscribe).toHaveBeenCalled(); + expect(manager.getStore().getCameraCount()).toBe(0); + }); + + it('should not reset the store while a commit is in flight', async () => { + const api = createCardAPI(); + vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS()); + vi.mocked(api.getConfigManager().getConfig).mockReturnValue( + createConfig({ cameras: [{ id: 'id', engine: 'generic' }] }), + ); + + const engine = mock(); + const factory = mock(); + vi.mocked(factory.createEngine).mockResolvedValue(engine); + vi.mocked(factory.getEngineForCamera).mockResolvedValue(Engine.Generic); + vi.mocked(engine.createCamera).mockImplementation( + async (cameraConfig: CameraConfig) => + await createInitializedCamera(cameraConfig, engine, createCapabilities()), + ); + + // The real store mutates incrementally; a mock makes the commit window + // externally controllable. + const order: string[] = []; + const store = mock(); + let releaseCommit: () => void = () => {}; + vi.mocked(store.setCameras).mockImplementation( + () => + new Promise((resolve) => { + order.push('commit-start'); + releaseCommit = () => { + order.push('commit-end'); + resolve(); + }; + }), + ); + vi.mocked(store.reset).mockImplementation(async () => { + order.push('reset'); + }); + + const manager = new CameraManager(api, { factory, store }); + + const initialization = manager.initializeCamerasFromConfig(); + await vi.waitFor(() => expect(store.setCameras).toHaveBeenCalled()); + + const destruction = manager.destroy(); + releaseCommit(); + await Promise.all([initialization, destruction]); + + // The reset waits for the commit rather than interleaving with it. + expect(order).toEqual(['commit-start', 'commit-end', 'reset']); + }); + }); + it('should reject missing engine', async () => { const api = createCardAPI(); vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS());