Delete Natural Voice tokens the desktop discards, and never reset the saved voice on a failed read

docs/specs/alert.md

@@ -410,7 +410,7 @@ Every rule above holds for both engines.
 - **Never let the voice token reach a renderer**; the host adds it to the request (rationale), and only managed-voice approval writes it (`docs/specs/hosted.md` -> "Managed voice"). Where it may go: `docs/specs/security-local.md` → "Persisted state".
 - **Must bound the host request** at `MANAGED_VOICE_REQUEST_TIMEOUT_MS` (inside `SPEECH_ENGINE_TIMEOUT_MS`, so a fallback still fits) and `MAX_AUDIO_BYTES`, accepting only `audio/mpeg`, and store only a token of the `dmv_…` shape and a voice of the curated set (`MANAGED_VOICES`).
 - **Must cache clips by voice and text in the host's memory, bounded in count and bytes, never persisted**; a hit makes no request, and a token change empties it. **A 403 latches `notEntitled`**, which bypasses the cache until a speak succeeds, so a lapsed member hears the system voice; **a 401 forgets the token it refused, never one saved since**. An answer to an older token changes nothing.
-- **Must disable managed voice in memory immediately on Disconnect, including cached and in-flight results, even if clearing its persisted token fails.** A failure stays visible and retryable; it must never report a successful disconnect while the token remains on disk. **Disconnect must also delete the token at the voice origin, best effort and never awaited**, its retry included; under `nothing` it asks nothing, and the token lapses unused (`docs/specs/hosted.md` -> "Managed voice").
+- **Must disable managed voice in memory immediately on Disconnect, including cached and in-flight results, even if clearing its persisted token fails.** A failure stays visible and retryable; it must never report a successful disconnect while the token remains on disk. **Disconnect must also delete the token at the voice origin, best effort and never awaited**, its retry included, **as must the host for every approved token it discards unsaved** (approval canceled or overtaken, save failed); under `nothing` it asks nothing, and the token lapses unused (`docs/specs/hosted.md` -> "Managed voice").
 
 Source of truth: `withFallback` in `lib/src/lib/speech-engine.ts`; `createManagedVoiceEngine` in `lib/src/lib/managed-voice-engine.ts`; `createManagedVoiceHost` in `lib/src/host/managed-voice-host.ts`; `MANAGED_VOICES` in `remote-lib-common/src/remote/managed-voice.ts`.
 

lib/src/host/managed-voice-host.test.ts

@@ -23,8 +23,8 @@ const MANAGED_VOICE_TOKEN_URL = 'https://voice.dormouse.sh/api/voice/token';
 const requests = () => fetchMock.mock.calls.map(([url, init]: [string, RequestInit]) =>
   [url, init.method, (init.headers as Record<string, string>).authorization]);
 
-/** The next `readFile` fails with this, once: a Windows antivirus lock, say. */
-const readFault = vi.hoisted(() => ({ next: null as NodeJS.ErrnoException | null }));
+/** The next `readFile` fails with this, once and then `repeats` more times: a Windows antivirus lock, say. */
+const readFault = vi.hoisted(() => ({ next: null as NodeJS.ErrnoException | null, repeats: 0 }));
 const renameFault = vi.hoisted(() => ({ next: null as NodeJS.ErrnoException | null }));
 vi.mock('node:fs/promises', async (importOriginal) => {
   const actual = await importOriginal<typeof import('node:fs/promises')>();
@@ -38,7 +38,8 @@ vi.mock('node:fs/promises', async (importOriginal) => {
     }) as typeof actual.rename,
     readFile: (async (...args: Parameters<typeof actual.readFile>) => {
       const fault = readFault.next;
-      readFault.next = null;
+      if (readFault.repeats > 0) readFault.repeats--;
+      else readFault.next = null;
       if (fault) throw fault;
       return actual.readFile(...args);
     }) as typeof actual.readFile,
@@ -205,6 +206,18 @@ describe('configuration', () => {
     const unavailable = make({ stateDir: undefined });
     await approveVoice(unavailable);
     expect(await unavailable.handle({ op: 'status' })).toMatchObject({ configured: false, enrollment: { status: 'ended', message: expect.stringContaining('Could not save') } });
+    // Held by no one, so deleted at Hosted.
+    await vi.waitFor(() => expect(requests().at(-1)).toEqual([MANAGED_VOICE_TOKEN_URL, 'DELETE', `Bearer ${TOKEN}`]));
+  });
+
+  it('keeps the saved voice through a disconnect whose reads fail', async () => {
+    await writeFile(join(dir, MANAGED_VOICE_FILE), JSON.stringify({ token: TOKEN, voiceId: OTHER_VOICE }), { mode: 0o600 });
+    const restarted = make();
+    readFault.next = Object.assign(new Error('EBUSY: resource busy or locked'), { code: 'EBUSY' });
+    readFault.repeats = 1;
+    expect(await restarted.handle({ op: 'disconnect' })).toMatchObject({ ok: true, configured: false, voiceId: OTHER_VOICE });
+    expect(JSON.parse(await readFile(join(dir, MANAGED_VOICE_FILE), 'utf8'))).toEqual({ token: null, voiceId: OTHER_VOICE });
+    expect(broadcasts.at(-1)).toEqual(idle(false, OTHER_VOICE));
   });
 });
 
@@ -641,7 +654,9 @@ describe('independent managed-voice lifecycle', () => {
     answer(Response.json({ status: 'enabled', voiceToken: TOKEN }));
     await vi.advanceTimersByTimeAsync(5000);
     expect(await host.handle({ op: 'status' })).toEqual(idle(false));
-    expect(fetchMock).toHaveBeenCalledTimes(2);
+    // Polling stopped, and the late token is deleted at Hosted.
+    expect(fetchMock).toHaveBeenCalledTimes(3);
+    expect(requests().at(-1)).toEqual([MANAGED_VOICE_TOKEN_URL, 'DELETE', `Bearer ${TOKEN}`]);
   });
 
   it('disconnect during the saving notification cannot re-enable a late approval', async () => {
@@ -663,6 +678,33 @@ describe('independent managed-voice lifecycle', () => {
     await disconnect;
     expect(await host.handle({ op: 'status' })).toEqual(idle(false));
     expect(await make().handle({ op: 'status' })).toEqual(idle(false));
+    // The token the disconnect overtook, never saved, is deleted at Hosted.
+    await vi.waitFor(() => expect(requests().at(-1)).toEqual([MANAGED_VOICE_TOKEN_URL, 'DELETE', `Bearer ${TOKEN}`]));
+  });
+
+  it('deletes at Hosted a token a disconnect clears while it is being saved', async () => {
+    vi.useFakeTimers();
+    const files = fileManagedVoiceStore(dir);
+    host = createManagedVoiceHost({
+      store: {
+        read: files.read,
+        write(next, changed) {
+          // The disconnect queues behind this save, before it lands.
+          if (next.token !== null) void host.handle({ op: 'disconnect' });
+          return files.write(next, changed);
+        },
+      },
+      onStatus: (status) => void broadcasts.push(status),
+      fetch: fetchMock as unknown as typeof fetch,
+      relay: { origin: DEFAULT_RELAY_ORIGIN, mode: 'hosted' },
+      networkAllowed: async () => true,
+    });
+    fetchMock.mockResolvedValueOnce(Response.json(begun())).mockResolvedValueOnce(Response.json({ status: 'enabled', voiceToken: TOKEN }));
+    await host.handle({ op: 'beginEnrollment' });
+    await vi.advanceTimersByTimeAsync(1000);
+    await vi.waitFor(() => expect(requests().at(-1)).toEqual([MANAGED_VOICE_TOKEN_URL, 'DELETE', `Bearer ${TOKEN}`]));
+    expect(await host.handle({ op: 'status' })).toEqual(idle(false));
+    expect(JSON.parse(await readFile(join(dir, MANAGED_VOICE_FILE), 'utf8')).token).toBeNull();
   });
 
   it('stops approval polling when the network policy changes to Nowhere', async () => {

lib/src/host/managed-voice-host.ts

@@ -55,25 +55,34 @@ export interface ManagedVoiceStore {
    * lock cached as defaults would let the next edit overwrite the token.
    */
   read(): Promise<unknown>;
-  /** Persist `next`, of which only `changed` differs from what was read. */
+  /**
+   * Persist `next[changed]` alone, keeping the other field as stored: a
+   * caller whose read failed passes defaults there, which must never land.
+   */
   write(next: StoredManagedVoice, changed: keyof StoredManagedVoice): Promise<void>;
 }
 
-/** The sidecar's store: `managed-voice.json` in the owner-only state directory, rewritten whole. */
+/**
+ * The sidecar's store: `managed-voice.json` in the owner-only state directory,
+ * rewritten whole from a fresh read, so a write fails rather than reset the
+ * field it was not given.
+ */
 export function fileManagedVoiceStore(stateDir: string): ManagedVoiceStore {
   const file = join(stateDir, MANAGED_VOICE_FILE);
+  async function read(): Promise<unknown> {
+    try {
+      return JSON.parse(await readFile(file, 'utf8'));
+    } catch (error) {
+      // Only a missing or unparsable file is "no config": a transient lock
+      // (EBUSY, EPERM, EMFILE) is retried on the next read.
+      if ((error as NodeJS.ErrnoException).code === 'ENOENT' || error instanceof SyntaxError) return null;
+      throw error;
+    }
+  }
   return {
-    async read() {
-      try {
-        return JSON.parse(await readFile(file, 'utf8'));
-      } catch (error) {
-        // Only a missing or unparsable file is "no config": a transient lock
-        // (EBUSY, EPERM, EMFILE) is retried on the next read.
-        if ((error as NodeJS.ErrnoException).code === 'ENOENT' || error instanceof SyntaxError) return null;
-        throw error;
-      }
-    },
-    write: (next) => writeJsonAtomic(stateDir, file, next),
+    read,
+    write: async (next, changed) =>
+      writeJsonAtomic(stateDir, file, { ...normalizeStored(await read()), [changed]: next[changed] }),
   };
 }
 
@@ -224,17 +233,17 @@ export function createManagedVoiceHost(options: {
 
   /** Clear the stored token, memory before disk: a failed write must never leave speech armed. Runs on `serialize`. */
   async function clearToken(): Promise<void> {
-    const current = await load().catch(() => normalizeStored(null));
-    const next = { ...current, token: null };
-    loaded = Promise.resolve(next);
+    // Unread, the voice is unknown: never cached, and the store keeps it.
+    const current = await load().catch(() => null);
+    loaded = current ? Promise.resolve({ ...current, token: null }) : null;
     disconnectFailed = true;
-    uncleared = current.token ?? uncleared;
-    options.onStatus(status(next));
+    uncleared = current?.token ?? uncleared;
+    await announce();
     if (!store) throw new Error('no owner-only store');
-    await store.write(next, 'token');
+    await store.write({ ...current ?? normalizeStored(null), token: null }, 'token');
     disconnectFailed = false;
     uncleared = null;
-    options.onStatus(status(next));
+    await announce();
   }
 
   /** Any token change forgets every clip and every refusal the old one earned. */
@@ -246,7 +255,15 @@ export function createManagedVoiceHost(options: {
     return serialize(async () => {
       if (speakUrl === null) throw new Error('this build has no managed voice');
       if (token === null) {
-        await clearToken();
+        // Read here, after any save queued first: a token a disconnect
+        // overtakes mid-save is deleted too. A retry's is the one the failed
+        // clear left behind.
+        const held = (await load().catch(() => null))?.token ?? uncleared;
+        try {
+          await clearToken();
+        } finally {
+          if (held !== null) void forgetAtHosted(held);
+        }
       } else {
         await save('token', (config) => ({ ...config, token }), false);
         if (generation === tokenGeneration) disabled = false;
@@ -354,9 +371,9 @@ export function createManagedVoiceHost(options: {
   }
 
   /**
-   * Best effort, never awaited by turning off: delete `token` at Hosted, so a
-   * computer that turned Natural Voice off leaves nothing live there. Skipped
-   * under Nowhere, failed offline; either way the token lapses unused.
+   * Best effort, never awaited: delete `token` at Hosted, so a token this
+   * computer turned off or discarded unsaved leaves nothing live there.
+   * Skipped under Nowhere, failed offline; either way the token lapses unused.
    */
   async function forgetAtHosted(token: string): Promise<void> {
     try {
@@ -416,7 +433,12 @@ export function createManagedVoiceHost(options: {
           // Check outside the retrying exchange too: Nowhere ends this run.
           await requireEnrollmentNetwork();
           const answer = await pollHostedApproval(options.relay.origin, body.deviceCode, 'voice', enrollmentFetch, parseVoiceApproval);
-          if (sequence !== enrollmentSequence) return;
+          // Below, a token this host discards (canceled, overtaken, or never
+          // saved) is held by no one, so it is deleted at Hosted.
+          if (sequence !== enrollmentSequence) {
+            if (answer.status === 'enabled') void forgetAtHosted(answer.voiceToken);
+            return;
+          }
           if (answer.status === 'retry') {
             if (answer.slowDown) intervalMs = slowDownHostedApproval(intervalMs);
             schedule();
@@ -428,11 +450,15 @@ export function createManagedVoiceHost(options: {
           if (answer.status === 'enabled') {
             enrollment = { status: 'saving' };
             await announce();
-            if (sequence !== enrollmentSequence) return;
-            // A token never saved, or never received below, is held by no one,
-            // so the user is never asked to revoke it.
+            if (sequence !== enrollmentSequence) {
+              void forgetAtHosted(answer.voiceToken);
+              return;
+            }
             try { await setToken(answer.voiceToken); }
-            catch { throw new Error('Could not save Natural Voice on this computer. Try again.'); }
+            catch {
+              void forgetAtHosted(answer.voiceToken);
+              throw new Error('Could not save Natural Voice on this computer. Try again.');
+            }
             if (sequence === enrollmentSequence) { enrollment = null; await announce(); }
             return;
           }
@@ -459,13 +485,7 @@ export function createManagedVoiceHost(options: {
         cancelEnrollment();
         if (op === 'disconnect') {
           disabled = true;
-          // A retry's token is the one the failed clear left behind.
-          const held = (await load().catch(() => null))?.token ?? uncleared;
-          try {
-            await setToken(null);
-          } finally {
-            if (held !== null) void forgetAtHosted(held);
-          }
+          await setToken(null);
         } else await announce();
       }
       return { ok: true, ...status(await load()) };