fix(mail-index): close handle on every pragma failure, reconcile contact/file deletes

QA pass on the encrypted mail index found two real gaps beyond what the
prior end-to-end fix pass caught:

1. store.ts's MailIndex.open() only wrapped SOME of the post-key pragma
   calls in a try/catch before this: the first attempt's `key` pragma and
   assertEncrypted() ran outside any try at all, and the wrong-key retry
   repeated the same gap. Any pragma throwing there (SQLITE_BUSY, a full
   disk on the first WAL write) leaked the native SQLite handle instead of
   closing it. Factored the open+key+verify+pragma sequence into openKeyed(),
   which guarantees a close before rethrowing on any failure, and reused it
   for both the first attempt and the retry.

2. reindex.ts never removed a deleted contact or file from the index. The
   `removed` field exists in the API and is fully tested at the store layer,
   but nothing in the renderer populates it, so a deleted contact/file stayed
   searchable - and retrievable by the AI feature - indefinitely. Mail and
   calendar can't use the same fix (their queries are date-windowed, so an id
   missing from one fetch may just be outside the window), but contacts/files
   have no date filter - a catch-up fetch that comes back under its cap IS
   the complete set, so anything locally indexed but absent from it is safely
   known to be deleted. Added strayIdsAfterCatchUp() and wired it into the
   catch-up path for those two types only.

Also read binding.ts, key.ts, paths.ts, jmap.ts, extract.ts, the FTS5
query builder, and both /api/offline/{search,reindex} routes end to end;
no other concrete bugs found there. Full findings reported separately.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

lib/mail-index/__tests__/reindex.test.ts

@@ -0,0 +1,31 @@
+import { describe, expect, it } from 'vitest';
+import { strayIdsAfterCatchUp } from '../reindex';
+
+describe('strayIdsAfterCatchUp', () => {
+  it('removes locally-indexed ids absent from a complete (uncapped) fetch', () => {
+    // Contacts/files have no date filter, so a catch-up query that comes back
+    // under the cap IS the whole account - anything indexed but missing from
+    // it was deleted. This is the only place a JMAP `destroyed` ever reaches
+    // these two content types, since nothing in the renderer populates
+    // reindex's `removed` field.
+    const existing = new Set(['a', 'b', 'c']);
+    expect(strayIdsAfterCatchUp(existing, ['a', 'c'], 2, 2_000)).toEqual(['b']);
+  });
+
+  it('does nothing when the fetch is empty but so is the local index', () => {
+    expect(strayIdsAfterCatchUp(new Set(), [], 0, 2_000)).toEqual([]);
+  });
+
+  it('never removes anything when the query hit its cap - a truncated page is not the whole world', () => {
+    // Exactly the case that would otherwise delete objects that are still
+    // live: an account with >= cap contacts/files, where "missing from this
+    // page" only means "not on this page", not "gone".
+    const existing = new Set(['a', 'b', 'c']);
+    expect(strayIdsAfterCatchUp(existing, ['a'], 2_000, 2_000)).toEqual([]);
+  });
+
+  it('is a no-op when nothing is stray', () => {
+    const existing = new Set(['a', 'b']);
+    expect(strayIdsAfterCatchUp(existing, ['a', 'b', 'c'], 3, 2_000)).toEqual([]);
+  });
+});

lib/mail-index/__tests__/store.test.ts

@@ -4,8 +4,9 @@ import path from 'node:path';
 import { randomBytes } from 'node:crypto';
 import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 import { isSqlcipherAvailable } from '../binding';
+import type { SqlcipherConstructor, SqlcipherDatabase, SqlcipherStatement } from '../binding';
 import { accountFileToken, getStoreDir, indexDbPath, STORE_DIR_ENV } from '../paths';
-import { MailIndex, toFtsMatchQuery, type IndexDoc } from '../store';
+import { MailIndex, openKeyed, toFtsMatchQuery, type IndexDoc } from '../store';
 
 describe('toFtsMatchQuery', () => {
   it('quotes every token so FTS5 operators in user input cannot break the query', () => {
@@ -78,6 +79,51 @@ describe('paths', () => {
   });
 });
 
+/** A fake `SqlcipherDatabase` whose `pragma()` is driven by the given handler. */
+function fakeSqlcipherCtor(
+  pragmaHandler: (source: string) => unknown,
+): { ctor: SqlcipherConstructor; instances: Array<{ closeCalls: number }> } {
+  const instances: Array<{ closeCalls: number }> = [];
+  function FakeDatabase(this: unknown, _path?: string): SqlcipherDatabase {
+    const state = { closeCalls: 0 };
+    instances.push(state);
+    const stmt: SqlcipherStatement = { run: () => ({ changes: 0, lastInsertRowid: 0 }), get: () => undefined, all: () => [] };
+    const db: SqlcipherDatabase = {
+      exec: () => {},
+      prepare: () => stmt,
+      pragma: pragmaHandler,
+      close: () => { state.closeCalls += 1; },
+    };
+    return db;
+  }
+  return { ctor: FakeDatabase as unknown as SqlcipherConstructor, instances };
+}
+
+describe('openKeyed', () => {
+  it('closes the connection before rethrowing when a pragma AFTER assertEncrypted fails', () => {
+    // The bug this guards: `journal_mode = WAL` (or any pragma after the key
+    // check) throwing must not leak the native handle - a `try` that only
+    // wrapped SOME of these calls previously let exactly this escape.
+    const { ctor, instances } = fakeSqlcipherCtor((source) => {
+      if (source === 'cipher_version') return [{ cipher_version: 'fake-4.5.0' }];
+      if (source === 'journal_mode = WAL') throw new Error('simulated pragma failure');
+      return undefined;
+    });
+    expect(() => openKeyed(ctor, '/fake/path.db', randomBytes(32))).toThrow(/simulated pragma failure/);
+    expect(instances).toHaveLength(1);
+    expect(instances[0].closeCalls).toBe(1);
+  });
+
+  it('tolerates the extra close when assertEncrypted itself is the failure (its own close is idempotent)', () => {
+    // assertEncrypted closes on its own failure before throwing; openKeyed's
+    // catch then calls close again. That second call must be harmless, not a
+    // new crash - hence >= 1 rather than a fixed count.
+    const { ctor, instances } = fakeSqlcipherCtor((source) => (source === 'cipher_version' ? [] : undefined));
+    expect(() => openKeyed(ctor, '/fake/path.db', randomBytes(32))).toThrow(/no SQLCipher support/);
+    expect(instances[0].closeCalls).toBeGreaterThanOrEqual(1);
+  });
+});
+
 function doc(overrides: Partial<IndexDoc> = {}): IndexDoc {
   return {
     jmapAccountId: 'acc1',

lib/mail-index/reindex.ts

@@ -106,6 +106,26 @@ function isoDaysFromNow(days: number): string {
   return new Date(Date.now() + days * 24 * 60 * 60 * 1000).toISOString();
 }
 
+/**
+ * Which locally-indexed ids are stray after an uncapped (contact/file)
+ * catch-up fetch, and therefore safe to remove as deleted.
+ *
+ * Only safe when `queriedCount < cap`: a query that hit the cap was
+ * truncated - "the rest weren't asked for", not "the rest are gone" - and
+ * treating a truncated page as the whole world would delete objects that are
+ * still live. Exported for unit testing; the database-touching caller is not.
+ */
+export function strayIdsAfterCatchUp(
+  existingIds: ReadonlySet<string>,
+  fetchedIds: readonly string[],
+  queriedCount: number,
+  cap: number,
+): string[] {
+  if (queriedCount >= cap) return [];
+  const fetched = new Set(fetchedIds);
+  return [...existingIds].filter((id) => !fetched.has(id));
+}
+
 /**
  * Which types this session can actually index. Calendar/contacts are session
  * capabilities; files is a PER-ACCOUNT capability (a server can advertise
@@ -151,8 +171,19 @@ interface FetchArgs {
   ids: readonly string[] | null;
 }
 
+interface FetchResult {
+  docs: IndexDoc[];
+  /**
+   * How many ids the type's OWN query returned, before any `Foo/get`
+   * chunking. Only set when `ids === null` (a catch-up fetch); used to tell a
+   * complete uncapped fetch apart from one truncated at its cap - see
+   * `strayIdsAfterCatchUp`, the only consumer.
+   */
+  queriedCount?: number;
+}
+
 /** Fetches and flattens one content type. `ids === null` means "the recent window". */
-async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise<IndexDoc[]> {
+async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise<FetchResult> {
   const { session, authHeader, jmapAccountId, ids } = args;
 
   switch (contentType) {
@@ -170,7 +201,7 @@ async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise<Ind
         );
         for (const email of emails) docs.push(extractMail(jmapAccountId, email));
       }
-      return docs;
+      return { docs, queriedCount: ids ? undefined : targetIds.length };
     }
     case 'calendar': {
       const targetIds = ids ?? await queryCalendarEventIds(
@@ -185,7 +216,7 @@ async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise<Ind
         );
         for (const event of events) docs.push(extractCalendarEvent(jmapAccountId, event));
       }
-      return docs;
+      return { docs, queriedCount: ids ? undefined : targetIds.length };
     }
     case 'contact': {
       const targetIds = ids ?? await queryContactIds(session, authHeader, jmapAccountId, CONTACTS_MAX);
@@ -196,7 +227,7 @@ async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise<Ind
         );
         for (const card of cards) docs.push(extractContact(jmapAccountId, card));
       }
-      return docs;
+      return { docs, queriedCount: ids ? undefined : targetIds.length };
     }
     case 'file': {
       const targetIds = ids ?? await queryFileIds(session, authHeader, jmapAccountId, FILES_MAX);
@@ -208,10 +239,11 @@ async function fetchDocs(contentType: ContentType, args: FetchArgs): Promise<Ind
       }
       // Paths need the whole set in hand, so this one can't stream per chunk.
       const paths = buildFilePaths(nodes);
-      return nodes
+      const docs = nodes
         // Directories are indexed too: "what's in the Invoices folder" is a
         // real query, and a folder row is a few bytes.
         .map((node) => extractFile(jmapAccountId, node, { path: paths.get(node.id) }));
+      return { docs, queriedCount: ids ? undefined : targetIds.length };
     }
   }
 }
@@ -292,7 +324,7 @@ export async function runIndex(
             ? requestedIds.slice(0, MAX_IDS_PER_CALL)
             : null;
 
-          const docs = await fetchDocs(contentType, {
+          const { docs, queriedCount } = await fetchDocs(contentType, {
             session, authHeader: indexSession.authHeader, jmapAccountId, ids,
           });
           written[contentType] = index.upsert(docs);
@@ -302,6 +334,27 @@ export async function runIndex(
             // contacts have no date, and file rows are metadata-sized.
             index.pruneOlderThan(jmapAccountId, 'mail', isoDaysFromNow(-INDEX_WINDOW_DAYS));
           }
+
+          // Contact/file DELETES: a JMAP `destroyed` only ever reaches this
+          // route via `req.removed`, which nothing in the renderer populates
+          // today - so without this, a deleted contact or file stays
+          // searchable (and retrievable by the AI feature) forever. Mail and
+          // calendar can't use the same trick: their queries are windowed by
+          // date, so an id missing from one fetch may simply be outside the
+          // window, not gone. Contacts/files have no date filter at all - the
+          // query is "the first N, capped" - so when a catch-up fetch (ids
+          // === null) comes back under the cap, it IS the complete set, and
+          // anything indexed but absent from it is safely known to be deleted.
+          if (queriedCount !== undefined && (contentType === 'contact' || contentType === 'file')) {
+            const cap = contentType === 'contact' ? CONTACTS_MAX : FILES_MAX;
+            const stale = strayIdsAfterCatchUp(
+              index.existingIds(jmapAccountId, contentType),
+              docs.map((d) => d.id),
+              queriedCount,
+              cap,
+            );
+            if (stale.length > 0) index.remove(jmapAccountId, contentType, stale);
+          }
         } catch (error) {
           // One unsupported or misbehaving type must not fail the others.
           const message = error instanceof Error ? error.message : String(error);

lib/mail-index/store.ts

@@ -15,7 +15,7 @@
 
 import fs from 'node:fs';
 import path from 'node:path';
-import { loadSqlcipher, type SqlcipherDatabase } from './binding';
+import { loadSqlcipher, type SqlcipherConstructor, type SqlcipherDatabase } from './binding';
 import { dbSiblings, indexDbPath } from './paths';
 
 export const SCHEMA_VERSION = 1;
@@ -137,6 +137,51 @@ export interface OpenOptions {
   key: Buffer;
 }
 
+/**
+ * Opens the connection, sets the SQLCipher key, verifies real encryption, and
+ * applies the fixed pragmas - the sequence both the first attempt and the
+ * wrong-key retry in `open()` need identically.
+ *
+ * On ANY failure the just-opened connection is closed before the error
+ * propagates. This matters beyond `assertEncrypted`'s own failure (which
+ * already closes): a bare `db.pragma(...)` throwing - SQLITE_BUSY, a full disk
+ * on the first WAL write, anything - must not leak the native handle either,
+ * which a `try` wrapped around only some of these calls previously missed.
+ *
+ * Exported so the cleanup guarantee can be unit-tested against a fake
+ * `SqlcipherDatabase` - a real double failure (wrong key, THEN a pragma
+ * failure on the freshly rebuilt file) is not practically reproducible
+ * against the real binding.
+ */
+export function openKeyed(
+  Database: SqlcipherConstructor,
+  dbPath: string,
+  key: Buffer,
+): { db: SqlcipherDatabase; version: number | null } {
+  const db = new Database(dbPath);
+  try {
+    // The key pragma must be the FIRST statement on the connection. Hex form
+    // means SQLCipher uses these 32 bytes as the raw key with no KDF, which is
+    // right for a random key (a passphrase would want the KDF).
+    db.pragma(`key = "x'${key.toString('hex')}'"`);
+    assertEncrypted(db, dbPath);
+    db.pragma('journal_mode = WAL');
+    db.pragma('synchronous = NORMAL');
+    // The offline replica (lib/offline-replica/**) is a SECOND connection to
+    // this same file, writing disjoint tables. WAL lets a writer and readers
+    // coexist, but two WRITERS get SQLITE_BUSY immediately without this - and
+    // both subsystems are driven by the same renderer push handler, so they
+    // genuinely do overlap.
+    db.pragma('busy_timeout = 8000');
+    return { db, version: readSchemaVersion(db) };
+  } catch (error) {
+    // Idempotent: assertEncrypted already closed on its own failure, so this
+    // is a harmless no-op in that case.
+    try { db.close(); } catch { /* already closed */ }
+    throw error;
+  }
+}
+
 export class MailIndex {
   private constructor(
     private readonly db: SqlcipherDatabase,
@@ -163,46 +208,24 @@ export class MailIndex {
     const dbPath = indexDbPath(storeDir, accountId);
     fs.mkdirSync(path.dirname(dbPath), { recursive: true, mode: 0o700 });
 
-    let db = new Database(dbPath);
-    // The key pragma must be the FIRST statement on the connection. Hex form
-    // means SQLCipher uses these 32 bytes as the raw key with no KDF, which is
-    // right for a random key (a passphrase would want the KDF).
-    db.pragma(`key = "x'${key.toString('hex')}'"`);
-    assertEncrypted(db, dbPath);
-
     // A wrong key surfaces here rather than at open: SQLCipher only reads the
     // header lazily. Treat it as "unreadable" and rebuild from scratch - the
     // index is derived data, so there is nothing to recover and never anything
     // to prompt the user for (the key was never a user secret).
-    let version: number | null;
+    let opened: { db: SqlcipherDatabase; version: number | null };
     try {
-      db.pragma('journal_mode = WAL');
-      db.pragma('synchronous = NORMAL');
-      // The offline replica (lib/offline-replica/**) is a SECOND connection to
-      // this same file, writing disjoint tables. WAL lets a writer and readers
-      // coexist, but two WRITERS get SQLITE_BUSY immediately without this - and
-      // both subsystems are driven by the same renderer push handler, so they
-      // genuinely do overlap.
-      db.pragma('busy_timeout = 8000');
-      version = readSchemaVersion(db);
+      opened = openKeyed(Database, dbPath, key);
     } catch {
-      db.close();
+      // `openKeyed` guarantees the failed connection above is already closed,
+      // so there is nothing to clean up here before retrying on a fresh file.
       for (const f of dbSiblings(dbPath)) {
         try { fs.rmSync(f, { force: true }); } catch { /* best effort */ }
       }
-      db = new Database(dbPath);
-      db.pragma(`key = "x'${key.toString('hex')}'"`);
-      assertEncrypted(db, dbPath);
-      db.pragma('journal_mode = WAL');
-      db.pragma('synchronous = NORMAL');
-      // The offline replica (lib/offline-replica/**) is a SECOND connection to
-      // this same file, writing disjoint tables. WAL lets a writer and readers
-      // coexist, but two WRITERS get SQLITE_BUSY immediately without this - and
-      // both subsystems are driven by the same renderer push handler, so they
-      // genuinely do overlap.
-      db.pragma('busy_timeout = 8000');
-      version = null;
+      opened = openKeyed(Database, dbPath, key);
+      opened.version = null; // fresh file - nothing to read
     }
+    const db = opened.db;
+    let version = opened.version;
 
     if (version !== null && version !== SCHEMA_VERSION) {
       // Rebuildable derived data: drop, don't migrate.