fix(storage): skip the full startup permission walk after one clean pass (#878)

packages/plugin/src/features/magic-context/storage-db.ts

@@ -184,8 +184,8 @@ function ensureSecureStorageDir(dir: string): void {
     ensureStorageDirectorySync(dir);
 }
 
-function tightenStorageTree(dir: string): void {
-    const { failures } = tightenStorageTreeSync(dir);
+function tightenStorageTree(dbPath: string): void {
+    const { failures } = tightenStorageTreeSync(dirname(dbPath), dbPath);
     log(`[magic-context] storage permission tightening failures=${failures}`);
 }
 
@@ -1131,7 +1131,7 @@ function finishDatabaseOpen(
     // created them. Externally managed trusted-group storage skips this entirely.
     // SQLite creates WAL sidecars itself; the startup tree pass also covers files
     // materialized during this open. The helper logs failure counts only.
-    tightenStorageTree(dirname(dbPath));
+    tightenStorageTree(dbPath);
     databases.set(dbPath, db);
     pathByDatabase.set(db, dbPath);
     persistenceByDatabase.set(db, true);
@@ -2691,7 +2691,7 @@ export function openDatabase(dbPathOrOptions?: string | OpenDatabaseOptions): Da
             migrateLegacyStorageIfNeeded(dbPath, dbDir);
         }
         ensureSecureStorageDir(dbDir);
-        tightenStorageTree(dbDir);
+        tightenStorageTree(dbPath);
         if (!existsSync(dbPath) && shouldEnforcePrivateStoragePermissions()) {
             try {
                 writeStorageFileSync(dbPath, new Uint8Array(), { flag: "wx" });
@@ -2813,7 +2813,7 @@ export async function openDatabaseAsync(
         try {
             if (!explicitDbPath) migrateLegacyStorageIfNeeded(dbPath, dbDir);
             ensureSecureStorageDir(dbDir);
-            tightenStorageTree(dbDir);
+            tightenStorageTree(dbPath);
             if (!existsSync(dbPath) && shouldEnforcePrivateStoragePermissions()) {
                 try {
                     writeStorageFileSync(dbPath, new Uint8Array(), { flag: "wx" });

packages/plugin/src/hooks/magic-context/lkg-recovery-fence.test.ts

@@ -1,6 +1,7 @@
 import { Database as BunDatabase } from "bun:sqlite";
-import { afterEach, describe, expect, test } from "bun:test";
+import { afterEach, describe, expect, spyOn, test } from "bun:test";
 import { spawnSync } from "node:child_process";
+import * as fs from "node:fs";
 import { existsSync, readdirSync, readFileSync, rmSync, statSync } from "node:fs";
 import { dirname, join } from "node:path";
 import {
@@ -10,7 +11,7 @@ import {
 import { clearSession, closeDatabase, openDatabase } from "../../features/magic-context/storage";
 import { deleteSessionScopedRows } from "../../features/magic-context/storage-session-tables";
 import type { Database } from "../../shared/sqlite";
-import { writeStorageFileSync } from "../../shared/storage-permissions";
+import { tightenStorageTreeSync, writeStorageFileSync } from "../../shared/storage-permissions";
 import { cleanupTestTempDir, createTestTempDir } from "../../shared/test-temp-dir";
 import {
     clearPersistedLkgSlotStrict,
@@ -117,6 +118,37 @@ ${code}`,
 }
 
 describe.skipIf(process.platform === "win32")("durable recovery fence", () => {
+    test("locked publications remove their directory after two operations on the same fence", () => {
+        const f = fixture();
+        try {
+            expect(existsSync(`${f.path}.locks`)).toBe(false);
+            f.dirty();
+            expect(existsSync(`${f.path}.locks`)).toBe(false);
+            expect(saveLkgSlotToDb(f.db, "session", slot, undefined, { rebuild: true })).toBe(true);
+            expect(existsSync(`${f.path}.locks`)).toBe(false);
+        } finally {
+            f.raw.close();
+        }
+    });
+    test("temp cleanup removes only the known name without listing the fence folder", () => {
+        const f = fixture();
+        const known = `${f.path}.tmp`;
+        const unrelated = `${f.path}.unrelated.tmp`;
+        writeStorageFileSync(known, "stale", { forcePrivate: true });
+        writeStorageFileSync(unrelated, "keep", { forcePrivate: true });
+        const list = spyOn(fs, "readdirSync");
+        try {
+            f.dirty();
+            expect(existsSync(known)).toBe(false);
+            expect(readFileSync(unrelated, "utf8")).toBe("keep");
+            expect(
+                list.mock.calls.filter(([path]) => String(path) === dirname(f.path)),
+            ).toHaveLength(0);
+        } finally {
+            list.mockRestore();
+            f.raw.close();
+        }
+    });
     test("warm synchronous captures perform zero fence I/O", () => {
         const f = fixture();
         const operations: string[] = [];
@@ -400,14 +432,23 @@ for (let i=0, attempts=0; i<12;) {
             f.raw.close();
         }
     });
-    test("session clear removes the certificate and orphan temporary records", () => {
+    test("session clear removes the certificate and its known temporary record", () => {
         const f = fixture();
         try {
-            writeStorageFileSync(`${f.path}.dead.tmp`, "orphan", { forcePrivate: true });
+            writeStorageFileSync(`${f.path}.tmp`, "orphan", { forcePrivate: true });
+            writeStorageFileSync(`${f.path}.dead.tmp`, "legacy", { forcePrivate: true });
+            const old = new Date(Date.now() - 11 * 60_000);
+            fs.utimesSync(`${f.path}.dead.tmp`, old, old);
+            const unrelated = join(dirname(f.path), "unrelated.tmp");
+            writeStorageFileSync(unrelated, "keep", { forcePrivate: true });
             clearPersistedLkgSlotStrict(f.db, "session");
             expect(f.load()).toBeUndefined();
             expect(existsSync(f.path)).toBe(false);
+            expect(existsSync(`${f.path}.tmp`)).toBe(false);
+            expect(existsSync(`${f.path}.dead.tmp`)).toBe(true);
+            expect(tightenStorageTreeSync(dirname(dirname(f.path))).failures).toBe(0);
             expect(existsSync(`${f.path}.dead.tmp`)).toBe(false);
+            expect(readFileSync(unrelated, "utf8")).toBe("keep");
             clearLkgRecoveryFence("session");
         } finally {
             f.raw.close();

packages/plugin/src/hooks/magic-context/lkg-recovery-fence.ts

@@ -1,6 +1,6 @@
 import { createHash, randomUUID } from "node:crypto";
-import { lstatSync, readdirSync, readFileSync, rmdirSync, rmSync } from "node:fs";
-import { basename, dirname, join } from "node:path";
+import { lstatSync, readFileSync, rmdirSync, rmSync } from "node:fs";
+import { dirname, join } from "node:path";
 import { BoundedSessionMap } from "../../shared/bounded-session-map";
 import { getMagicContextStorageDir } from "../../shared/data-path";
 import { getHarness } from "../../shared/harness";
@@ -123,6 +123,7 @@ function publishRecord(record: Omit<Fence, "checksum">): void {
             lkgRecoveryFencePath(record.sessionId),
             JSON.stringify({ ...record, checksum: hash(JSON.stringify(payload(record))) }),
             () => observer?.("temp-synced"),
+            `${lkgRecoveryFencePath(record.sessionId)}.tmp`,
         );
         if (record.state === "dirty") observer?.("dirty");
         remember(record.sessionId, {
@@ -141,6 +142,8 @@ function publishRecord(record: Omit<Fence, "checksum">): void {
             writeStorageFileDurableAtomicSync(
                 lkgRecoveryFencePath(record.sessionId),
                 JSON.stringify({ ...dirty, checksum: hash(JSON.stringify(payload(dirty))) }),
+                undefined,
+                `${lkgRecoveryFencePath(record.sessionId)}.tmp`,
             );
         }
         remember(record.sessionId, { state: "invalid", record: null });
@@ -201,10 +204,8 @@ function locked<T>(sessionId: string, work: () => T): T {
     ioObserver?.("mutex");
     return withStorageFileMutex(`${lkgRecoveryFencePath(sessionId)}.locks`, () => {
         const path = lkgRecoveryFencePath(sessionId);
-        for (const name of readdirSync(dirname(path))) {
-            if (name.startsWith(`${basename(path)}.`) && name.endsWith(".tmp"))
-                rmSync(join(dirname(path), name), { force: true });
-        }
+        // The mutex prevents concurrent use of the temp name. A folder listing is not necessary.
+        rmSync(`${path}.tmp`, { force: true });
         return work();
     });
 }

packages/plugin/src/shared/storage-file-mutex-review.test.ts

@@ -1,6 +1,6 @@
 import { expect, spyOn, test } from "bun:test";
 import { spawnSync } from "node:child_process";
-import { readdirSync, readFileSync, rmdirSync, writeFileSync } from "node:fs";
+import { existsSync, readdirSync, readFileSync, rmdirSync, rmSync, writeFileSync } from "node:fs";
 import { join } from "node:path";
 import { __resetRpcIdentityTestHooks, __setRpcIdentityTestHooks } from "./rpc-utils";
 import { withStorageFileMutex } from "./storage-file-mutex";
@@ -9,6 +9,46 @@ import { createTestTempDir } from "./test-temp-dir";
 
 const mutex = join(import.meta.dir, "storage-file-mutex.ts");
 
+test("mutex removes its empty directory after sequential operations and thrown work", () => {
+    const { dir: root, cleanup } = createTestTempDir("mc-mutex-sequence-");
+    const directory = join(root, "locks");
+    try {
+        for (let i = 0; i < 2; i++) {
+            expect(withStorageFileMutex(directory, () => i)).toBe(i);
+            expect(existsSync(directory)).toBe(false);
+        }
+        expect(() =>
+            withStorageFileMutex(directory, () => {
+                throw new Error("work failed");
+            }),
+        ).toThrow("work failed");
+        expect(existsSync(directory)).toBe(false);
+    } finally {
+        cleanup();
+    }
+});
+
+test("mutex directory cleanup preserves a ticket published during locked work", () => {
+    const { dir: root, cleanup } = createTestTempDir("mc-mutex-ticket-");
+    const directory = join(root, "locks");
+    const contender = join(directory, `${process.pid}-00000000-0000-0000-0000-000000000000.lock`);
+    try {
+        expect(
+            withStorageFileMutex(directory, () => {
+                writeFileSync(contender, "choosing", { mode: 0o600 });
+                return "done";
+            }),
+        ).toBe("done");
+        expect(readFileSync(contender, "utf8")).toBe("choosing");
+        expect(readdirSync(directory)).toHaveLength(1);
+        rmSync(contender);
+        withStorageFileMutex(directory, () => undefined);
+        expect(existsSync(directory)).toBe(false);
+    } finally {
+        cleanup();
+    }
+});
+
 test("mutex retries ticket creation when empty-directory cleanup wins the mkdir race", () => {
     const { dir: root, cleanup } = createTestTempDir("mc-mutex-rmdir-");
     const directory = join(root, "locks");
@@ -101,7 +141,7 @@ test.skipIf(process.platform === "win32")(
                 200,
             );
             expect(reached).toBe(true);
-            expect(readdirSync(join(root, "locks"))).toEqual([]);
+            expect(existsSync(join(root, "locks"))).toBe(false);
         } finally {
             if (child.exitCode === null) {
                 child.kill("SIGKILL");

packages/plugin/src/shared/storage-file-mutex.ts

@@ -1,6 +1,6 @@
 import { execFileSync } from "node:child_process";
 import { createHash, randomUUID } from "node:crypto";
-import { readdirSync, readFileSync, rmSync } from "node:fs";
+import { readdirSync, readFileSync, rmdirSync, rmSync } from "node:fs";
 import { join } from "node:path";
 import { isPidAlive, readProcessStartTime } from "./rpc-utils";
 import {
@@ -136,5 +136,10 @@ export function withStorageFileMutex<T>(directory: string, work: () => T, timeou
         return result;
     } finally {
         rmSync(ownPath, { force: true });
+        try {
+            rmdirSync(directory);
+        } catch {
+            // Do not remove a live contender's ticket.
+        }
     }
 }