fix: Ensure no camera outlives a failed initialization (#2657)

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<void>;
 
-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<Camera> {
+  async initialize(options: Options): Promise<this> {
     // 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<Entity | null> {
     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<void> {
     await this._getDoorbellEntities(hass, options);
   }
 
   private async _getDoorbellEntities(
     hass: HomeAssistant,
-    options: CameraInitializationOptions,
+    options: Options,
   ): Promise<void> {
     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<void> {}
+
+  /**
+   * 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<void> {}
 
   protected async _buildCapabilities(
     hass: HomeAssistant,
-    options: CameraInitializationOptions,
+    options: Options,
   ): Promise<Capabilities> {
     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<CapabilitiesRaw> {
     return {
       live: true,

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<Options> {
+  protected override async _initializeBeforeCapabilities(
     hass: HomeAssistant,
-    options: CameraInitializationOptions,
+    options: Options,
   ): Promise<void> {
     if (!this._entity) {
       throw new CameraNoEntityError(this.getConfig());
     }
-    await super._initialize(hass, options);
+    await super._initializeBeforeCapabilities(hass, options);
   }
 }

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<FrigateCameraInitializationOptions> {
   // 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<Camera> {
-    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<void> {
+    // 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<void> {
@@ -124,7 +122,9 @@ export class FrigateCamera extends Camera {
     return true;
   }
 
-  protected override async _initialize(hass: HomeAssistant): Promise<void> {
+  protected override async _initializeBeforeCapabilities(
+    hass: HomeAssistant,
+  ): Promise<void> {
     const config = this.getConfig();
     const hasCameraName = !!config.frigate?.camera_name;
     const cameraEntity = getCameraEntityFromConfig(config);

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<void> {
+    // 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<void> {
-    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<void> {
+  /**
+   * 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<CameraConfig, CameraManagerEngine>,
+  ): Promise<Camera[]> {
+    // 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<string> = 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<void> {
+    await allPromises(cameras, async (camera) => {
+      try {
+        await camera.destroy();
+      } catch (error: unknown) {
+        errorToConsole(error);
+      }
+    });
+  }
+
+  private async _initializeCameras(
+    camerasConfig: CameraConfig[],
+    generation: number,
+  ): Promise<void> {
     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<string> = 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(),

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<ReolinkCameraInitializationOptions> {
   // 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<Camera> {
-    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<void> {
-    await super._initialize(hass, options);
+    await super._initializeBeforeCapabilities(hass, options);
     await this._initializeChannel(hass, options.deviceRegistryManager);
     this._ptzEntities = await this._getPTZEntities(hass, options.entityRegistryManager);
   }

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<TPLinkCameraInitializationOptions> {
   private _ptzEntities: PTZEntities | null = null;
 
-  protected async _initialize(
+  protected async _initializeBeforeCapabilities(
     hass: HomeAssistant,
     options: TPLinkCameraInitializationOptions,
   ): Promise<void> {
-    await super._initialize(hass, options);
+    await super._initializeBeforeCapabilities(hass, options);
     this._ptzEntities = await this._getPTZEntities(hass, options.entityRegistryManager);
   }
 

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,