fix(auth): recover stale credential file locks via PID liveness

Author: TheGreatAxios

src/auth/store.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test } from "bun:test";
-import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
+import { mkdir, mkdtemp, readFile, rm, utimes, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { type } from "arktype";
@@ -328,6 +328,142 @@ describe("createAuthStore", () => {
     }
   });
 
+  test("takes over a dead-holder lock instead of timing out", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-store-takeover-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        settingsDirName: TEST_SETTINGS_DIR,
+        isTokens: isTestTokens,
+      });
+      const lockPath = `${store.authPath(home)}.lock`;
+      await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
+      // The maximum pid_t can never be a live holder: kill(pid, 0) answers
+      // ESRCH (or EINVAL), both of which read as dead.
+      await writeFile(lockPath, `${2_147_483_647}`, { mode: 0o600 });
+
+      const profile = {
+        name: "work",
+        tokens: { access: "a", refresh: "r", expiresAt: 1 },
+        createdAt: 1,
+      };
+      await expect(store.saveProfile(profile, home)).resolves.toBeUndefined();
+      expect(await store.loadProfile("work", home)).toEqual(profile);
+      await expect(readFile(lockPath, "utf8")).rejects.toThrow("ENOENT");
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("waits on a live-holder lock and times out without touching it", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-store-live-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        settingsDirName: TEST_SETTINGS_DIR,
+        isTokens: isTestTokens,
+        lockTimeoutMs: 100,
+      });
+      const lockPath = `${store.authPath(home)}.lock`;
+      await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
+      await writeFile(lockPath, `${process.pid}`, { mode: 0o600 });
+
+      await expect(
+        store.saveProfile(
+          {
+            name: "work",
+            tokens: { access: "a", refresh: "r", expiresAt: 1 },
+            createdAt: 1,
+          },
+          home,
+        ),
+      ).rejects.toThrow(
+        `Timed out waiting for OAuth credential lock ${lockPath}. ` +
+          "If no Corbits process is running, remove this lock file manually and retry.",
+      );
+      expect(await readFile(lockPath, "utf8")).toBe(`${process.pid}`);
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("takes over a stale legacy lock but waits on a fresh one", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-store-legacy-"));
+    try {
+      const staleStore = createAuthStore<TestTokens>({
+        filename: "stale-auth.json",
+        settingsDirName: TEST_SETTINGS_DIR,
+        isTokens: isTestTokens,
+      });
+      const staleLockPath = `${staleStore.authPath(home)}.lock`;
+      await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
+      await writeFile(staleLockPath, "legacy-orphan", { mode: 0o600 });
+      await utimes(
+        staleLockPath,
+        new Date(),
+        new Date(Date.now() - 60_000),
+      );
+
+      const profile = {
+        name: "work",
+        tokens: { access: "a", refresh: "r", expiresAt: 1 },
+        createdAt: 1,
+      };
+      await expect(
+        staleStore.saveProfile(profile, home),
+      ).resolves.toBeUndefined();
+      expect(await staleStore.loadProfile("work", home)).toEqual(profile);
+
+      const freshStore = createAuthStore<TestTokens>({
+        filename: "fresh-auth.json",
+        settingsDirName: TEST_SETTINGS_DIR,
+        isTokens: isTestTokens,
+        lockTimeoutMs: 100,
+      });
+      const freshLockPath = `${freshStore.authPath(home)}.lock`;
+      await writeFile(freshLockPath, "legacy-orphan", { mode: 0o600 });
+      await expect(
+        freshStore.saveProfile(profile, home),
+      ).rejects.toThrow("Timed out waiting for OAuth credential lock");
+      expect(await readFile(freshLockPath, "utf8")).toBe("legacy-orphan");
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("saves when a contended lock vanishes mid-wait", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-store-vanish-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        settingsDirName: TEST_SETTINGS_DIR,
+        isTokens: isTestTokens,
+      });
+      const lockPath = `${store.authPath(home)}.lock`;
+      await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
+      await writeFile(lockPath, `${2_147_483_647}`, { mode: 0o600 });
+
+      // Yank the stale lock out from under the waiter: whether the waiter
+      // observes the dead PID, an ENOENT read, or an ENOENT unlink, it must
+      // retry the exclusive create and land the save — never throw ENOENT.
+      const pending = store.saveProfile(
+        {
+          name: "work",
+          tokens: { access: "a", refresh: "r", expiresAt: 1 },
+          createdAt: 1,
+        },
+        home,
+      );
+      await rm(lockPath, { force: true });
+      await expect(pending).resolves.toBeUndefined();
+      expect((await store.loadProfile("work", home))?.tokens.access).toBe(
+        "a",
+      );
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
   test("fails closed with manual recovery guidance when an orphan lock exists", async () => {
     const home = await mkdtemp(join(tmpdir(), "oauth-store-orphan-"));
     try {

src/auth/store.ts

@@ -3,6 +3,7 @@ import {
   open,
   readFile,
   rename,
+  stat,
   unlink,
   writeFile,
 } from "node:fs/promises";
@@ -60,6 +61,12 @@ interface AuthFile<TTokens extends BaseTokens> {
 const LOCK_RETRY_MS = 25;
 const LOCK_TIMEOUT_MS = 1_000;
 
+// Age at which a lock file carrying no holder PID (a foreign writer, or a
+// lock predating PID tagging) is presumed orphaned by a crashed holder and
+// taken over. Stays far above LOCK_TIMEOUT_MS so a waiter never declares a
+// live holder stale mid-wait; PID-tagged locks ignore this horizon.
+const LOCK_STALE_MS = 5_000;
+
 // Per-call unique temp (pid + counter). Matches mcp/auth-store — pid alone is not
 // unique per call if writeAuthFile ever overlaps in-process.
 let tmpWriteCounter = 0;
@@ -97,6 +104,57 @@ function isProfile<TTokens extends BaseTokens>(
   return isTokens(parsed.tokens);
 }
 
+function holderPid(content: string): number | null {
+  const pid = Number(content.split(":")[0]?.trim());
+  return Number.isInteger(pid) && pid > 0 ? pid : null;
+}
+
+// kill(pid, 0) liveness: success means the process exists (alive); EPERM
+// means it exists but belongs to another user (alive); ESRCH/EINVAL mean no
+// such process (dead). Any other failure reads as alive — never steal a live
+// holder's lock on a confused signal check; the waiter times out with a
+// recovery hint instead.
+function isPidAlive(pid: number): boolean {
+  try {
+    process.kill(pid, 0);
+    return true;
+  } catch (err) {
+    const code = (err as NodeJS.ErrnoException)?.code;
+    return code !== "ESRCH" && code !== "EINVAL";
+  }
+}
+
+// Null when the lock vanished under the waiter (a release raced the read);
+// anything but ENOENT propagates — permission and disk errors must surface,
+// not read as an empty lock.
+async function readLockContent(lockPath: string): Promise<string | null> {
+  try {
+    return await readFile(lockPath, "utf8");
+  } catch (err) {
+    if (isErrnoCode(err, "ENOENT")) return null;
+    throw err;
+  }
+}
+
+// True when the observed lock is safe to take over: a tagged holder whose
+// PID is dead (crashed — reachable under any timeout), or a legacy untagged
+// lock older than the stale horizon. A vanished lock reads as not stale;
+// the acquire loop retries the exclusive create instead.
+async function isLockStale(
+  lockPath: string,
+  content: string,
+): Promise<boolean> {
+  const pid = holderPid(content);
+  if (pid !== null) return !isPidAlive(pid);
+  try {
+    const info = await stat(lockPath);
+    return Date.now() - info.mtimeMs > LOCK_STALE_MS;
+  } catch (err) {
+    if (isErrnoCode(err, "ENOENT")) return false;
+    throw err;
+  }
+}
+
 export function createAuthStore<TTokens extends BaseTokens>(
   options: AuthStoreOptions<TTokens>,
 ): AuthStore<TTokens> {
@@ -153,10 +211,44 @@ export function createAuthStore<TTokens extends BaseTokens>(
 
     while (true) {
       try {
-        lock = await open(lockPath, "wx", 0o600);
+        const handle = await open(lockPath, "wx", 0o600);
+        try {
+          await handle.writeFile(`${process.pid}`, "utf8");
+        } catch (writeError) {
+          try {
+            await handle.close();
+          } catch {
+            // Ignore close errors on the cleanup path; the write error below
+            // is the one the caller must see.
+          }
+          try {
+            await unlink(lockPath);
+          } catch (unlinkError) {
+            if (!isErrnoCode(unlinkError, "ENOENT")) throw unlinkError;
+          }
+          throw writeError;
+        }
+        lock = handle;
         break;
       } catch (error) {
         if (!isErrnoCode(error, "EEXIST")) throw error;
+        // A crashed holder never releases: take over a stale lock rather
+        // than brick the store. A lock that vanished under the read
+        // (a release raced us) is not stale — retry the exclusive create.
+        const content = await readLockContent(lockPath);
+        if (content !== null && (await isLockStale(lockPath, content))) {
+          // Re-check before unlinking so a concurrent takeover winner's
+          // fresh lock is never mistaken for the stale entry just observed.
+          if ((await readLockContent(lockPath)) === content) {
+            try {
+              await unlink(lockPath);
+            } catch (unlinkError) {
+              // A concurrent winner unlinked first; retry the create.
+              if (!isErrnoCode(unlinkError, "ENOENT")) throw unlinkError;
+            }
+          }
+          continue;
+        }
         if (Date.now() >= deadline) {
           throw new Error(
             `Timed out waiting for OAuth credential lock ${lockPath}. ` +
@@ -173,8 +265,17 @@ export function createAuthStore<TTokens extends BaseTokens>(
     } finally {
       try {
         await lock.close();
+      } catch {
+        // The callback's result (or error) owns this return path; a close
+        // failure must not mask it. The unlink below still runs.
       } finally {
-        await unlink(lockPath);
+        // Swallow ENOENT only: a stale-takeover steal legitimately removes
+        // the file first, but permission and disk errors must surface.
+        try {
+          await unlink(lockPath);
+        } catch (error) {
+          if (!isErrnoCode(error, "ENOENT")) throw error;
+        }
       }
     }
   }