fix: Fix leaky Frigate subscriptions/unsubscriptions (#2513)

tests/camera-manager/frigate/camera.test.ts

@@ -1,5 +1,5 @@
 import { format } from 'date-fns';
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { assert, beforeEach, describe, expect, it, vi } from 'vitest';
 import { mock } from 'vitest-mock-extended';
 import { CameraManagerEngine } from '../../../src/camera-manager/engine';
 import { FrigateCamera } from '../../../src/camera-manager/frigate/camera';
@@ -27,6 +27,7 @@ import { ViewMediaType } from '../../../src/view/item';
 import { EntityRegistryManagerMock } from '../../ha/registry/entity/mock';
 import {
   createCameraConfig,
+  createCapabilities,
   createHASS,
   createRegistryEntity,
   createStateEntity,
@@ -965,6 +966,61 @@ describe('FrigateCamera', () => {
       expect(eventWatcher.unsubscribe).toBeCalled();
     });
 
+    it('should unsubscribe on destroy while event subscription is pending', async () => {
+      const camera = new FrigateCamera(
+        createCameraConfig({
+          frigate: { client_id: 'CLIENT_ID', camera_name: 'front_door' },
+          triggers: {
+            media_events: ['events'],
+            reviews: { severities: ['high'] },
+          },
+        }),
+        mock<CameraManagerEngine>(),
+      );
+      const hass = createHASS();
+      let resolveSubscribe: () => void = () => {};
+      const eventWatcher = mock<FrigateEventWatcher>();
+      const reviewWatcher = mock<FrigateReviewWatcher>();
+      vi.mocked(eventWatcher.subscribe).mockReturnValue(
+        new Promise<void>((resolve) => {
+          resolveSubscribe = resolve;
+        }),
+      );
+
+      const initializePromise = camera.initialize({
+        hass: hass,
+        entityRegistryManager: mock<EntityRegistryManager>(),
+        stateWatcher: mock<StateWatcherSubscriptionInterface>(),
+        eventWatcher: mock<EventWatcherSubscriptionInterface>(),
+        frigateEventWatcher: eventWatcher,
+        frigateReviewWatcher: reviewWatcher,
+
+        // Pre-built so `_buildCapabilities` (which calls the un-mocked
+        // `liveProviderSupports2WayAudio`) is skipped and init reaches the
+        // pending Frigate event subscribe.
+        capabilityOptions: { capabilities: createCapabilities({ trigger: true }) },
+      });
+      await vi.waitFor(() => expect(eventWatcher.subscribe).toBeCalled());
+
+      await camera.destroy();
+
+      // Destroy iterated `_destroyCallbacks` and called the unsubscribe that
+      // was registered before the (still pending) event subscribe.
+      const subscribeCall = vi.mocked(eventWatcher.subscribe).mock.calls[0];
+      assert(subscribeCall);
+      expect(eventWatcher.unsubscribe).toBeCalledWith(subscribeCall[1]);
+
+      resolveSubscribe();
+      await initializePromise;
+
+      // The subsequent `_subscribeToReviews` short-circuited on `_destroyed`,
+      // so the review watcher was never subscribed (and so never needs an
+      // unsubscribe -- which would otherwise be ordered before the subscribe
+      // in the per-key PQueue and leak the resulting subscription).
+      expect(reviewWatcher.subscribe).not.toBeCalled();
+      expect(reviewWatcher.unsubscribe).not.toBeCalled();
+    });
+
     describe('should call handler correctly', () => {
       describe('should handle event type correctly', () => {
         it.each([

tests/camera-manager/frigate/watcher.test.ts

@@ -138,12 +138,19 @@ describe('FrigateEventWatcher', () => {
     const subscribePromise = stateWatcher.subscribe(hass, request);
 
     // Unsubscribe while subscription is still pending.
-    await stateWatcher.unsubscribe(request);
+    const unsubscribePromise = stateWatcher.unsubscribe(request);
 
-    // Complete the subscription.
+    // Complete the subscription: both subscribe and unsubscribe await the same
+    // pending promise, and unsubscribe then invokes the resolved unsub.
+    const unsubscribeCallback = vi.fn();
     assert(resolveSubscription);
-    resolveSubscription(vi.fn());
+    resolveSubscription(unsubscribeCallback);
     await subscribePromise;
+    await unsubscribePromise;
+
+    expect(unsubscribeCallback).toBeCalledTimes(1);
+    callHASubscribeMessageCallback(hass, JSON.stringify(createEventChange()));
+    expect(request.callback).not.toBeCalled();
   });
 
   describe('should call handler', () => {

tests/camera-manager/manager.test.ts

@@ -27,6 +27,7 @@ import {
   QueryType,
 } from '../../src/camera-manager/types.js';
 import { CardController } from '../../src/card-controller/controller.js';
+import { StateWatcherSubscriptionInterface } from '../../src/card-controller/hass/state-watcher.js';
 import { sortItems } from '../../src/card-controller/view/sort.js';
 import { CameraConfig } from '../../src/config/schema/cameras.js';
 import { advancedCameraCardConfigSchema } from '../../src/config/schema/types.js';
@@ -274,6 +275,7 @@ describe('CameraManager', () => {
       config?: CameraConfig;
       engineType?: Engine | null;
       capabilties?: Capabilities;
+      stateWatcher?: StateWatcherSubscriptionInterface;
     }[] = [{}],
     factory?: CameraManagerEngineFactory,
   ): CameraManager => {
@@ -300,6 +302,7 @@ describe('CameraManager', () => {
               cameraConfig,
               mockEngine,
               camera.capabilties ?? createCapabilities(),
+              camera.stateWatcher,
             ),
         );
       }
@@ -399,6 +402,48 @@ describe('CameraManager', () => {
       );
     });
 
+    it('should await camera destruction before throwing on duplicate id', async () => {
+      const api = createCardAPI();
+      vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS());
+
+      const cameraConfig = createCameraConfig({
+        id: 'DUPLICATE',
+        engine: 'generic',
+      });
+
+      // Camera.destroy() awaits its _destroyCallbacks, one of which is the
+      // trigger-path unsubscribe. By returning a deferred Promise from each
+      // camera's stateWatcher.unsubscribe, we make destroy completion
+      // externally observable without spying on any Camera method.
+      const order: string[] = [];
+      const buildStateWatcher = (): StateWatcherSubscriptionInterface => {
+        const watcher = mock<StateWatcherSubscriptionInterface>();
+        vi.mocked(watcher.unsubscribe).mockImplementation(
+          () =>
+            new Promise<void>((resolve) =>
+              setTimeout(() => {
+                order.push('destroy-done');
+                resolve();
+              }, 0),
+            ),
+        );
+        return watcher;
+      };
+
+      const cameraEntry = {
+        config: cameraConfig,
+        capabilties: createCapabilities({ trigger: true }),
+      };
+      const manager = createCameraManager(api, mock<CameraManagerEngine>(), [
+        { ...cameraEntry, stateWatcher: buildStateWatcher() },
+        { ...cameraEntry, stateWatcher: buildStateWatcher() },
+      ]);
+
+      await manager.initializeCamerasFromConfig().catch(() => order.push('throw'));
+
+      expect(order).toEqual(['destroy-done', 'destroy-done', 'throw']);
+    });
+
     it('should reject missing engine', async () => {
       const api = createCardAPI();
       vi.mocked(api.getHASSManager().getHASS).mockReturnValue(createHASS());

tests/test-utils.ts

@@ -120,11 +120,12 @@ export const createInitializedCamera = async (
   config: CameraConfig,
   engine: CameraManagerEngine,
   capabilities?: Capabilities,
+  stateWatcher?: StateWatcherSubscriptionInterface,
 ): Promise<Camera> => {
   const camera = new Camera(config, engine);
   await camera.initialize({
     hass: createHASS(),
-    stateWatcher: mock<StateWatcherSubscriptionInterface>(),
+    stateWatcher: stateWatcher ?? mock<StateWatcherSubscriptionInterface>(),
     eventWatcher: mock<EventWatcherSubscriptionInterface>(),
     ...(capabilities ? { capabilityOptions: { capabilities } } : {}),
   });

tests/utils/keyed-subscription-manager.test.ts

@@ -0,0 +1,102 @@
+import { describe, expect, it, vi } from 'vitest';
+import { KeyedSubscriptionManager } from '../../src/utils/keyed-subscription-manager';
+
+interface TestRequest {
+  key: string;
+  callback: () => void;
+}
+
+const create = (): KeyedSubscriptionManager<string, TestRequest> =>
+  new KeyedSubscriptionManager<string, TestRequest>((r) => r.key);
+
+describe('KeyedSubscriptionManager', () => {
+  it('should open the subscription once per key regardless of subscriber count', async () => {
+    const manager = create();
+    const subscribeFn = vi.fn().mockResolvedValue(vi.fn());
+
+    await manager.subscribe({ key: 'a', callback: vi.fn() }, subscribeFn);
+    await manager.subscribe({ key: 'a', callback: vi.fn() }, subscribeFn);
+
+    expect(subscribeFn).toBeCalledTimes(1);
+  });
+
+  it('should open a separate subscription for each distinct key', async () => {
+    const manager = create();
+    const subscribeFn = vi.fn().mockResolvedValue(vi.fn());
+
+    await manager.subscribe({ key: 'a', callback: vi.fn() }, subscribeFn);
+    await manager.subscribe({ key: 'b', callback: vi.fn() }, subscribeFn);
+
+    expect(subscribeFn).toBeCalledTimes(2);
+  });
+
+  it('should tear down the subscription only when the last subscriber for a key unsubscribes', async () => {
+    const manager = create();
+    const unsub = vi.fn();
+    const subscribeFn = vi.fn().mockResolvedValue(unsub);
+
+    const req1 = { key: 'a', callback: vi.fn() };
+    const req2 = { key: 'a', callback: vi.fn() };
+    await manager.subscribe(req1, subscribeFn);
+    await manager.subscribe(req2, subscribeFn);
+
+    await manager.unsubscribe(req1);
+    expect(unsub).not.toBeCalled();
+
+    await manager.unsubscribe(req2);
+    expect(unsub).toBeCalledTimes(1);
+  });
+
+  it('should await a pending subscribe before tearing down when unsubscribed mid-flight', async () => {
+    const manager = create();
+    const unsub = vi.fn();
+
+    let resolveOpen: ((cb: () => Promise<void>) => void) | undefined;
+    const openPromise = new Promise<() => Promise<void>>((resolve) => {
+      resolveOpen = resolve;
+    });
+    const subscribeFn = vi.fn().mockReturnValue(openPromise);
+
+    const req = { key: 'a', callback: vi.fn() };
+    const subscribePromise = manager.subscribe(req, subscribeFn);
+    const unsubscribePromise = manager.unsubscribe(req);
+
+    resolveOpen?.(unsub);
+    await subscribePromise;
+    await unsubscribePromise;
+
+    expect(unsub).toBeCalledTimes(1);
+  });
+
+  it('should expose the requests matching a given key', async () => {
+    const manager = create();
+    const subscribeFn = vi.fn().mockResolvedValue(vi.fn());
+
+    const reqA1 = { key: 'a', callback: vi.fn() };
+    const reqA2 = { key: 'a', callback: vi.fn() };
+    const reqB = { key: 'b', callback: vi.fn() };
+    await manager.subscribe(reqA1, subscribeFn);
+    await manager.subscribe(reqA2, subscribeFn);
+    await manager.subscribe(reqB, subscribeFn);
+
+    expect(manager.getRequestsForKey('a')).toEqual([reqA1, reqA2]);
+    expect(manager.getRequestsForKey('b')).toEqual([reqB]);
+
+    await manager.unsubscribe(reqA1);
+    expect(manager.getRequestsForKey('a')).toEqual([reqA2]);
+  });
+
+  it('should treat unsubscribe of an unknown request as a no-op', async () => {
+    const manager = create();
+    const unsub = vi.fn();
+    const subscribeFn = vi.fn().mockResolvedValue(unsub);
+
+    const subscribed = { key: 'a', callback: vi.fn() };
+    await manager.subscribe(subscribed, subscribeFn);
+
+    await manager.unsubscribe({ key: 'a', callback: vi.fn() });
+
+    expect(unsub).not.toBeCalled();
+    expect(manager.getRequestsForKey('a')).toEqual([subscribed]);
+  });
+});