fix: Connect the microphone when an inbound call is *answered* (#2609) (#2612)

- Closes: #2609

docs/uml/call-sequence.puml

@@ -12,11 +12,21 @@ opt <color:#43a047>**""live.microphone.always_connected""**</color> is enabled
   Mic --> User : Microphone access granted
 end
 
-User -> Card : Start a call
-note over User, Card
-  Via Call menu button or <color:#43a047>**""call_start""**</color> action.
-  Needs a camera that supports 2-way audio
-end note
+alt Outbound call
+  User -> Card : Start a call
+  note over User, Card
+    Via Call menu button or <color:#43a047>**""call_start""**</color> action.
+    Needs a camera that supports 2-way audio
+  end note
+else Inbound call
+  Guest -> Card : Rings
+  note over Card
+    Via a <color:#43a047>**""view.triggers.actions.trigger: call""**</color> trigger, e.g. a doorbell.
+    The card rings (<color:#43a047>**""live.controls.call.ringtone""**</color>) until answered,
+    rejected, or <color:#43a047>**""live.controls.call.unanswered_timeout_seconds""**</color> elapses
+  end note
+  User -> Card : Answer the call
+end
 
 opt <color:#43a047>**""live.microphone.always_connected""**</color> not enabled
   Card -> Mic : Microphone connection attempted (muted)
@@ -25,8 +35,9 @@ opt <color:#43a047>**""live.microphone.always_connected""**</color> not enabled
 end
 
 note over User, Mic
-  The microphone must connect for the call to proceed --
-  if access is denied or unsupported, the call does not start
+  The microphone is connected when the call is answered -- immediately
+  for an outbound call. An inbound call rings without it, so a doorbell
+  can ring even where microphone access needs a tap to be granted
 end note
 
 Card --> User : You hear the caller

src/card-controller/actions/actions/call-answer.ts

@@ -6,6 +6,6 @@ export class CallAnswerAction extends AdvancedCameraCardAction<CallAnswerActionC
   public async execute(api: CardActionsAPI): Promise<void> {
     await super.execute(api);
 
-    api.getCallManager().answer();
+    await api.getCallManager().answer();
   }
 }

src/card-controller/call/manager.ts

@@ -16,13 +16,13 @@ export class CallManager {
   private _ringtone = new Ringtone();
   private _unansweredTimer = new Timer();
 
-  // Identifies the current init/uninit cycle so an in-flight `start()`
-  // resuming from its microphone-connect await can detect that its CallManager
-  // was torn down -- or torn down and re-initialized -- while it was suspended
-  // and bail before installing a session or ringtone. Without this guard the
-  // resumed tail leaks audio onto the shared lock from an instance the user
-  // can no longer see or control, and may install state into a fresh
-  // lifecycle from a request that belongs to the previous one.
+  // Identifies the current init/uninit cycle so an in-flight `start()` or
+  // `answer()` resuming from its microphone-connect await can detect that its
+  // CallManager was torn down -- or torn down and re-initialized -- while it
+  // was suspended, and bail before installing a session or a ringtone. Without
+  // this guard the resumed tail leaks audio onto the shared lock from an
+  // instance the user can no longer see or control, and may install state into
+  // a fresh lifecycle from a request that belongs to the previous one.
   private _initGeneration = new Generation();
 
   constructor(api: CardCallAPI) {
@@ -104,19 +104,12 @@ export class CallManager {
       return false;
     }
 
-    const initGeneration = this._initGeneration.current();
-    const microphoneConnected = await this._connectMicrophone();
-    // If the init/uninit lifecycle advanced while the microphone connect was
-    // in flight, this request belongs to a previous lifecycle -- the view we
-    // captured may be stale, the triggering context is gone, and there is no
-    // clean teardown path for state we'd install here. Bail before touching
-    // `_call`, the ringtone lock, or surfacing a notification onto a torn-down
-    // NotificationManager.
-    if (!this._initGeneration.isCurrent(initGeneration)) {
-      return false;
-    }
-    if (!microphoneConnected) {
-      this._notifyError('error.call_microphone_forbidden', inbound);
+    // An inbound call has no use for the microphone until it is answered, and
+    // it arrives without the user gesture that browsers may require before
+    // granting microphone access -- so connecting here would let a refusal stop
+    // the call from ever ringing. The connect is deferred to `answer()`. An
+    // outbound call is answered by construction and needs it immediately.
+    if (!inbound && !(await this._connectMicrophone())) {
       return false;
     }
 
@@ -208,20 +201,39 @@ export class CallManager {
     return this._end(true);
   }
 
-  // Marks an inbound ringing call as answered: stops the ringtone, cancels
-  // the unanswered timer, and lets the normal call controls take over.
-  // No-op (returns false) if there is no call or it's already answered;
-  // rejecting a ringing call uses `end()` (same teardown).
-  public answer(): boolean {
-    if (!this._call || this._call.answered) {
+  // Marks an inbound ringing call as answered: connects the microphone, stops
+  // the ringtone, cancels the unanswered timer, and lets the normal call
+  // controls take over. Returns true iff the call was answered. No-op (returns
+  // false) if there is no call or it's already answered; rejecting a ringing
+  // call uses `end()` (same teardown).
+  public async answer(): Promise<boolean> {
+    const call = this._call;
+    if (!call || call.answered) {
       return false;
     }
+
+    // The user has acknowledged the ring, so silence it before the microphone
+    // connect, which may put a browser permission prompt on screen.
     this._ringtone.stop();
+
+    // An inbound call rings without the microphone, so this is where it is
+    // connected -- under the user gesture that answering provides. The call is
+    // left ringing on failure so it can be answered again.
+    if (!(await this._connectMicrophone())) {
+      return false;
+    }
+
+    // The call may have ended, or been superseded by another, while the
+    // microphone connect was in flight -- there is then nothing left to answer.
+    if (this._call !== call) {
+      return false;
+    }
+
     this._unansweredTimer.stop();
     // Replace (don't mutate) so Lit identity checks downstream pick up the
     // change. The `update()` below forces card.ts to re-render and re-read
     // `getCall()`, propagating the new session to the carousel.
-    this._call = { ...this._call, answered: true };
+    this._call = { ...call, answered: true };
     this._api.getConditionStateManager().setState({ call: 'answered' });
     this._api.getCardElementManager().update();
     return true;
@@ -377,7 +389,11 @@ export class CallManager {
       return false;
     }
 
-    if (microphoneManager.isForbidden()) {
+    // An earlier microphone denial does not stop an inbound call: the ring
+    // needs no microphone, and `answer()` retries the connect -- succeeding
+    // there clears the denial, and failing there reports it. An outbound call
+    // needs the microphone immediately, so a known denial ends it here.
+    if (!inbound && microphoneManager.isForbidden()) {
       this._notifyError('error.call_microphone_forbidden', inbound);
       return false;
     }
@@ -385,17 +401,37 @@ export class CallManager {
     return true;
   }
 
+  // Connects the microphone for a call. Returns true iff it is connected and
+  // this request still belongs to the current init/uninit lifecycle. A connect
+  // failure is surfaced as a notification.
   private async _connectMicrophone(): Promise<boolean> {
     const microphoneManager = this._api.getMicrophoneManager();
     if (microphoneManager.isConnected()) {
       return true;
     }
+
+    const initGeneration = this._initGeneration.current();
+    let connected = false;
     try {
       await microphoneManager.connect();
-      return true;
+      connected = true;
     } catch {
+      // Reported below, once this request is known to still be the current one.
+    }
+
+    // If the init/uninit lifecycle advanced while the connect was in flight,
+    // this request belongs to a previous lifecycle: the state it would act on
+    // is gone, and a notification would be surfaced onto a torn-down
+    // NotificationManager.
+    if (!this._initGeneration.isCurrent(initGeneration)) {
       return false;
     }
+
+    if (!connected) {
+      this._notifyError('error.call_microphone_forbidden', false);
+      return false;
+    }
+    return true;
   }
 
   private _hasCallCapability(cameraID: string): boolean {

tests/card-controller/call/manager.test.ts

@@ -544,6 +544,53 @@ describe('start', () => {
     expect(api.getViewManager().setViewByParameters).not.toBeCalled();
     expect(api.getNotificationManager().setNotification).toBeCalled();
   });
+
+  it('should ring an inbound call without connecting the microphone', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      microphoneConnected: false,
+      config: { live: { controls: { call: { ringtone: { type: 'chime' } } } } },
+    });
+    vi.mocked(api.getMicrophoneManager().connect).mockRejectedValue(new Error());
+    const manager = new CallManager(api);
+
+    // The microphone is only needed once the call is answered, so a connect
+    // that would fail must not stop the call from ringing.
+    expect(await manager.start({ inbound: true })).toBe(true);
+
+    expect(api.getMicrophoneManager().connect).not.toBeCalled();
+    expect(getRingtone().start).toBeCalled();
+  });
+
+  it('should ring an inbound call when the microphone is forbidden', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      microphoneForbidden: true,
+      config: { live: { controls: { call: { ringtone: { type: 'chime' } } } } },
+    });
+    const manager = new CallManager(api);
+
+    // An earlier denial must not silence a doorbell: the ring needs no
+    // microphone, and answering retries the connect.
+    expect(await manager.start({ inbound: true })).toBe(true);
+
+    expect(getRingtone().start).toBeCalled();
+  });
+
+  it('should abort an inbound call when the microphone is unsupported', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      microphoneSupported: false,
+      config: { live: { controls: { call: { ringtone: { type: 'chime' } } } } },
+    });
+    const manager = new CallManager(api);
+
+    // Unlike a denial, a browser without microphone support cannot start
+    // supporting it while the page is loaded, so the call can never be taken.
+    expect(await manager.start({ inbound: true })).toBe(false);
+
+    expect(getRingtone().start).not.toBeCalled();
+  });
 });
 
 // An inbound start request must not displace a call the user cares about
@@ -571,7 +618,7 @@ describe('inbound supersede policy', () => {
     manager.initialize();
     expect(await manager.start({ inbound: true })).toBe(true);
 
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
     expect(manager.getCall()?.answered).toBe(true);
 
     expect(await manager.start({ cameraID: 'camera.garage', inbound: true })).toBe(
@@ -626,7 +673,7 @@ describe('inbound supersede policy', () => {
     const manager = new CallManager(api);
     manager.initialize();
     expect(await manager.start({ inbound: true })).toBe(true);
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
 
     expect(await manager.start({ cameraID: 'camera.garage' })).toBe(true);
 
@@ -838,7 +885,7 @@ describe('endIf', () => {
       const manager = new CallManager(api);
       manager.initialize();
       expect(await manager.start({ inbound: true })).toBe(true);
-      expect(manager.answer()).toBe(true);
+      expect(await manager.answer()).toBe(true);
       expect(manager.getCall()?.answered).toBe(true);
 
       expect(manager.endIf({ answered: false })).toBe(false);
@@ -1109,11 +1156,11 @@ describe('initialize / uninitialize', () => {
   });
 });
 
-// `inbound: true` suppresses each of the preflight/validation notifications.
-// Every path that would call `_notifyError` is exercised under both the
-// non-inbound case (notification surfaced) and the inbound case (silent). The
-// non-inbound coverage already lives in the `start` describe above; here we
-// assert the inbound paths stay silent.
+// `inbound: true` suppresses each of the preflight/validation notifications an
+// inbound start can still reach. The non-inbound coverage (notification
+// surfaced) already lives in the `start` describe above; here we assert the
+// inbound paths stay silent. The forbidden-microphone check is deliberately
+// absent -- an inbound start no longer reaches it at all.
 describe('inbound option', () => {
   it('should suppress notification when the camera lacks live capability', async () => {
     const api = createAPI({
@@ -1142,29 +1189,6 @@ describe('inbound option', () => {
     expect(api.getNotificationManager().setNotification).not.toBeCalled();
   });
 
-  it('should suppress notification when the microphone is forbidden', async () => {
-    const api = createAPI({
-      view: createView({ camera: 'camera.office' }),
-      microphoneForbidden: true,
-    });
-
-    expect(await new CallManager(api).start({ inbound: true })).toBe(false);
-
-    expect(api.getNotificationManager().setNotification).not.toBeCalled();
-  });
-
-  it('should suppress notification when microphone connect rejects', async () => {
-    const api = createAPI({
-      view: createView({ camera: 'camera.office' }),
-      microphoneConnected: false,
-    });
-    vi.mocked(api.getMicrophoneManager().connect).mockRejectedValue(new Error('denied'));
-
-    expect(await new CallManager(api).start({ inbound: true })).toBe(false);
-
-    expect(api.getNotificationManager().setNotification).not.toBeCalled();
-  });
-
   it('should suppress notification when an explicit stream is not 2-way audio', async () => {
     const api = createAPI({ view: createView({ camera: 'camera.office' }) });
 
@@ -1249,11 +1273,11 @@ describe('answer', () => {
     expect(getRingtone().start).toBeCalled();
   });
 
-  it('should no-op when no call is active', () => {
+  it('should no-op when no call is active', async () => {
     const api = createAPI();
     const manager = new CallManager(api);
 
-    expect(manager.answer()).toBe(false);
+    expect(await manager.answer()).toBe(false);
   });
 
   it('should no-op when the call is already answered', async () => {
@@ -1264,11 +1288,11 @@ describe('answer', () => {
     const manager = new CallManager(api);
     manager.initialize();
     expect(await manager.start({ inbound: true })).toBe(true);
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
     vi.mocked(getRingtone().stop).mockClear();
     vi.mocked(api.getCardElementManager().update).mockClear();
 
-    expect(manager.answer()).toBe(false);
+    expect(await manager.answer()).toBe(false);
 
     expect(getRingtone().stop).not.toBeCalled();
     expect(api.getCardElementManager().update).not.toBeCalled();
@@ -1284,7 +1308,7 @@ describe('answer', () => {
     expect(await manager.start({ inbound: true })).toBe(true);
     const before = manager.getCall();
 
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
 
     const after = manager.getCall();
     expect(after?.answered).toBe(true);
@@ -1315,7 +1339,7 @@ describe('answer', () => {
       expect(await manager.start({ inbound: true })).toBe(true);
       vi.mocked(getRingtone().stop).mockClear();
 
-      expect(manager.answer()).toBe(true);
+      expect(await manager.answer()).toBe(true);
 
       expect(getRingtone().stop).toBeCalled();
 
@@ -1328,6 +1352,68 @@ describe('answer', () => {
     }
   });
 
+  it('should connect the microphone on answer', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      microphoneConnected: false,
+      config: inboundConfig,
+    });
+    vi.mocked(api.getMicrophoneManager().connect).mockResolvedValue();
+    const manager = new CallManager(api);
+    manager.initialize();
+    expect(await manager.start({ inbound: true })).toBe(true);
+
+    expect(await manager.answer()).toBe(true);
+
+    expect(api.getMicrophoneManager().connect).toBeCalled();
+  });
+
+  it('should silence the ringtone before the microphone connect', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      microphoneConnected: false,
+      config: inboundConfig,
+    });
+    let resolveConnect: () => void = () => {};
+    vi.mocked(api.getMicrophoneManager().connect).mockReturnValue(
+      new Promise<void>((resolve) => {
+        resolveConnect = resolve;
+      }),
+    );
+    const manager = new CallManager(api);
+    manager.initialize();
+    expect(await manager.start({ inbound: true })).toBe(true);
+    vi.mocked(getRingtone().stop).mockClear();
+
+    const answerPromise = manager.answer();
+
+    // The user has acknowledged the ring, so it must stop without waiting for a
+    // microphone permission prompt to be dealt with.
+    expect(getRingtone().stop).toBeCalled();
+
+    resolveConnect();
+    expect(await answerPromise).toBe(true);
+  });
+
+  it('should leave the call ringing when the microphone connect fails', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      microphoneConnected: false,
+      config: inboundConfig,
+    });
+    vi.mocked(api.getMicrophoneManager().connect).mockRejectedValue(new Error('denied'));
+    const manager = new CallManager(api);
+    manager.initialize();
+    expect(await manager.start({ inbound: true })).toBe(true);
+
+    expect(await manager.answer()).toBe(false);
+
+    // Answering is an explicit user gesture, so the failure is surfaced and the
+    // call remains answerable.
+    expect(api.getNotificationManager().setNotification).toBeCalled();
+    expect(manager.getCall()?.answered).toBe(false);
+  });
+
   it('should force a card re-render on answer', async () => {
     const api = createAPI({
       view: createView({ camera: 'camera.office' }),
@@ -1338,7 +1424,7 @@ describe('answer', () => {
     expect(await manager.start({ inbound: true })).toBe(true);
     vi.mocked(api.getCardElementManager().update).mockClear();
 
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
 
     // The card subtree depends on `getCall().answered`, which the manager
     // mutates outside the view-manager epoch -- so `update()` is what drives
@@ -1353,7 +1439,7 @@ describe('answer', () => {
     expect(await manager.start()).toBe(true);
 
     // Outbound starts answered, so `answer()` is a no-op.
-    expect(manager.answer()).toBe(false);
+    expect(await manager.answer()).toBe(false);
     expect(manager.getCall()?.answered).toBe(true);
   });
 
@@ -1593,7 +1679,7 @@ describe('unanswered timeout', () => {
     manager.initialize();
     expect(await manager.start({ inbound: true })).toBe(true);
 
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
     vi.advanceTimersByTime(60_000);
 
     expect(manager.isActive()).toBe(true);
@@ -1680,11 +1766,10 @@ describe('session end during setState', () => {
 });
 
 describe('state changes during in-flight start', () => {
-  it('should not install a session or ring when uninitialized mid-await', async () => {
+  it('should not install a session when uninitialized mid-await', async () => {
     const api = createAPI({
       view: createView({ camera: 'camera.office' }),
       microphoneConnected: false,
-      config: { live: { controls: { call: { ringtone: { type: 'chime' } } } } },
     });
     let resolveConnect: () => void = () => {};
     vi.mocked(api.getMicrophoneManager().connect).mockReturnValue(
@@ -1695,20 +1780,19 @@ describe('state changes during in-flight start', () => {
     const manager = new CallManager(api);
     manager.initialize();
 
-    const startPromise = manager.start({ inbound: true });
+    const startPromise = manager.start();
     manager.uninitialize();
     resolveConnect();
 
     expect(await startPromise).toBe(false);
-    expect(getRingtone().start).not.toBeCalled();
     expect(manager.isActive()).toBe(false);
+    expect(api.getViewManager().setViewByParameters).not.toBeCalled();
   });
 
-  it('should not install a session or ring when uninitialized and re-initialized mid-await', async () => {
+  it('should not install a session when uninitialized and re-initialized mid-await', async () => {
     const api = createAPI({
       view: createView({ camera: 'camera.office' }),
       microphoneConnected: false,
-      config: { live: { controls: { call: { ringtone: { type: 'chime' } } } } },
     });
     let resolveConnect: () => void = () => {};
     vi.mocked(api.getMicrophoneManager().connect).mockReturnValue(
@@ -1719,14 +1803,14 @@ describe('state changes during in-flight start', () => {
     const manager = new CallManager(api);
     manager.initialize();
 
-    const startPromise = manager.start({ inbound: true });
+    const startPromise = manager.start();
     manager.uninitialize();
     manager.initialize();
     resolveConnect();
 
     expect(await startPromise).toBe(false);
-    expect(getRingtone().start).not.toBeCalled();
     expect(manager.isActive()).toBe(false);
+    expect(api.getViewManager().setViewByParameters).not.toBeCalled();
   });
 
   it('should supersede a session installed by another start mid-await', async () => {
@@ -1792,6 +1876,84 @@ describe('state changes during in-flight start', () => {
   });
 });
 
+// Answering an inbound call is where its microphone is connected, so the same
+// mid-await state changes that `start()` guards against apply here too.
+describe('state changes during in-flight answer', () => {
+  const inboundConfig = {
+    live: { controls: { call: { ringtone: { type: 'chime' as const } } } },
+  };
+
+  // Starts a ringing inbound call whose subsequent `answer()` will suspend on
+  // the microphone connect until the returned settler is invoked.
+  const createRingingCall = async (
+    api: CardController,
+  ): Promise<{
+    manager: CallManager;
+    resolveConnect: () => void;
+    rejectConnect: (reason: unknown) => void;
+  }> => {
+    const manager = new CallManager(api);
+    manager.initialize();
+    expect(await manager.start({ inbound: true })).toBe(true);
+
+    vi.mocked(api.getMicrophoneManager().isConnected).mockReturnValue(false);
+    let resolveConnect: () => void = () => {};
+    let rejectConnect: (reason: unknown) => void = () => {};
+    vi.mocked(api.getMicrophoneManager().connect).mockReturnValue(
+      new Promise<void>((resolve, reject) => {
+        resolveConnect = resolve;
+        rejectConnect = reject;
+      }),
+    );
+    return { manager, resolveConnect, rejectConnect };
+  };
+
+  it('should not answer when uninitialized mid-await', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      config: inboundConfig,
+    });
+    const { manager, resolveConnect } = await createRingingCall(api);
+
+    const answerPromise = manager.answer();
+    manager.uninitialize();
+    resolveConnect();
+
+    expect(await answerPromise).toBe(false);
+    expect(manager.isActive()).toBe(false);
+  });
+
+  it('should suppress the microphone-failure notification when uninitialized mid-await', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      config: inboundConfig,
+    });
+    const { manager, rejectConnect } = await createRingingCall(api);
+
+    const answerPromise = manager.answer();
+    manager.uninitialize();
+    rejectConnect(new Error('denied'));
+
+    expect(await answerPromise).toBe(false);
+    expect(api.getNotificationManager().setNotification).not.toBeCalled();
+  });
+
+  it('should not answer a session that ended mid-await', async () => {
+    const api = createAPI({
+      view: createView({ camera: 'camera.office' }),
+      config: inboundConfig,
+    });
+    const { manager, resolveConnect } = await createRingingCall(api);
+
+    const answerPromise = manager.answer();
+    expect(manager.end()).toBe(true);
+    resolveConnect();
+
+    expect(await answerPromise).toBe(false);
+    expect(manager.isActive()).toBe(false);
+  });
+});
+
 // The phase the manager publishes is what automations actually react to, so
 // these drive a real ConditionStateManager and a real CallTrigger and assert
 // the transitions an automation would fire on, rather than that `setState` was
@@ -1877,7 +2039,7 @@ describe('published phase transitions in condition state', () => {
     expect(await manager.start({ inbound: true })).toBe(true);
     expect(answered).not.toHaveBeenCalled();
 
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
 
     expect(answered).toHaveBeenCalledTimes(1);
   });
@@ -1924,7 +2086,7 @@ describe('published phase transitions in condition state', () => {
     });
 
     expect(await manager.start({ inbound: true })).toBe(true);
-    expect(manager.answer()).toBe(true);
+    expect(await manager.answer()).toBe(true);
     expect(manager.end()).toBe(true);
 
     expect(hungUp).toHaveBeenCalledTimes(1);