Serialize OAuth profile-store read-modify-write updates

src/auth/oauth/oauth.test.ts

@@ -1,12 +1,12 @@
 import { afterEach, describe, expect, test } from "bun:test";
-import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
+import { mkdir, mkdtemp, readFile, rm, rmdir, unlink, utimes, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 
 import { baseTokensFromResponse, postToken, type OAuthClientConfig } from "./client.js";
 import { startOAuthLogin } from "./login.js";
 import { createTokenSession } from "./session.js";
-import { createAuthStore, type AuthProfile, type BaseTokens } from "./store.js";
+import { createAuthStore, type AuthProfile, type BaseTokens, withStoreFileLock } from "./store.js";
 
 const config: OAuthClientConfig = {
   clientId: "client-id",
@@ -134,6 +134,291 @@ describe("createAuthStore", () => {
     }
   });
 
+  test("concurrent saveProfile calls preserve all profiles", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-lock-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        isTokens: isTestTokens,
+      });
+
+      // Fire concurrent saves for distinct profile names. Without the store
+      // lock the last writer would silently drop profiles saved by others.
+      const count = 20;
+      const saves = Array.from({ length: count }, (_, i) =>
+        store.saveProfile(
+          {
+            name: `p-${i}`,
+            tokens: { access: `a-${i}`, refresh: `r-${i}`, expiresAt: i },
+            createdAt: i,
+          },
+          home,
+        ),
+      );
+      await Promise.all(saves);
+
+      const profiles = await store.listProfiles(home);
+      expect(profiles).toHaveLength(count);
+      for (let i = 0; i < count; i++) {
+        const p = profiles.find((p) => p.name === `p-${i}`);
+        expect(p).toBeDefined();
+        expect(p!.tokens.access).toBe(`a-${i}`);
+      }
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("concurrent updateTokens and saveProfile do not lose profiles", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-lock-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        isTokens: isTestTokens,
+      });
+
+      // Seed two profiles.
+      await store.saveProfile(
+        { name: "a", tokens: { access: "a0", refresh: "r0", expiresAt: 1 }, createdAt: 1 },
+        home,
+      );
+      await store.saveProfile(
+        { name: "b", tokens: { access: "b0", refresh: "r0", expiresAt: 1 }, createdAt: 2 },
+        home,
+      );
+
+      // Concurrently update tokens for both profiles plus a full save of a third profile.
+      await Promise.all([
+        store.updateTokens("a", { access: "a1", refresh: "r1", expiresAt: 10 }, home),
+        store.updateTokens("b", { access: "b1", refresh: "r1", expiresAt: 10 }, home),
+        store.saveProfile(
+          { name: "c", tokens: { access: "c1", refresh: "r1", expiresAt: 10 }, createdAt: 3 },
+          home,
+        ),
+      ]);
+
+      const profiles = await store.listProfiles(home);
+      const names = profiles.map((p) => p.name).sort();
+      expect(names).toEqual(["a", "b", "c"]);
+      expect(await store.loadProfile("a", home)).toEqual({
+        name: "a",
+        tokens: { access: "a1", refresh: "r1", expiresAt: 10 },
+        createdAt: 1,
+      });
+      expect(await store.loadProfile("b", home)).toEqual({
+        name: "b",
+        tokens: { access: "b1", refresh: "r1", expiresAt: 10 },
+        createdAt: 2,
+      });
+      expect(await store.loadProfile("c", home)).toEqual({
+        name: "c",
+        tokens: { access: "c1", refresh: "r1", expiresAt: 10 },
+        createdAt: 3,
+      });
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("lock directory is cleaned up after operations", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-lock-cleanup-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        isTokens: isTestTokens,
+      });
+
+      await store.saveProfile(
+        { name: "x", tokens: { access: "a", refresh: "r", expiresAt: 1 }, createdAt: 1 },
+        home,
+      );
+
+      const lockDir = join(home, ".corbits", "test-auth.json.lock");
+      await expect(readFile(join(lockDir, "owner"), "utf8")).rejects.toThrow();
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("concurrent removeProfile does not leave stale profiles", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-lock-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        isTokens: isTestTokens,
+      });
+
+      const count = 10;
+      for (let i = 0; i < count; i++) {
+        await store.saveProfile(
+          {
+            name: `r-${i}`,
+            tokens: { access: `a-${i}`, refresh: `r-${i}`, expiresAt: i },
+            createdAt: i,
+          },
+          home,
+        );
+      }
+
+      // Concurrently remove all profiles.
+      const removals = Array.from({ length: count }, (_, i) => store.removeProfile(`r-${i}`, home));
+      const results = await Promise.all(removals);
+
+      // Each removal should have returned exactly one name.
+      for (const removed of results) {
+        expect(removed).toHaveLength(1);
+      }
+
+      expect(await store.listProfiles(home)).toEqual([]);
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("malformed stale lock is recovered automatically", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-stale-lock-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        isTokens: isTestTokens,
+      });
+
+      const lockDir = join(home, ".corbits", "test-auth.json.lock");
+      await mkdir(lockDir, { recursive: true, mode: 0o700 });
+      const ownerFile = join(lockDir, "owner");
+      await writeFile(ownerFile, "malformed");
+      const past = new Date(Date.now() - 60_000); // 60s in the past
+      await utimes(ownerFile, past, past);
+
+      // The store should detect the stale lock, remove it, and succeed.
+      await store.saveProfile(
+        { name: "after-stale", tokens: { access: "a", refresh: "r", expiresAt: 1 }, createdAt: 1 },
+        home,
+      );
+
+      const profiles = await store.listProfiles(home);
+      expect(profiles).toHaveLength(1);
+      expect(profiles[0]).toBeDefined();
+      expect(profiles[0]!.name).toBe("after-stale");
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("an old lock owned by a live process is not displaced", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-live-lock-"));
+    try {
+      const lockPath = join(home, ".corbits", "test-auth.json.lock");
+      await mkdir(lockPath, { recursive: true, mode: 0o700 });
+      const ownerFile = join(lockPath, "owner");
+      await writeFile(ownerFile, JSON.stringify({ pid: process.pid, owner: "live-holder" }));
+      const past = new Date(Date.now() - 60_000);
+      await utimes(ownerFile, past, past);
+
+      let entered = false;
+      const waiting = withStoreFileLock(lockPath, async () => {
+        entered = true;
+      });
+      await Bun.sleep(150);
+
+      expect(entered).toBe(false);
+      await unlink(ownerFile);
+      await rmdir(lockPath);
+      await waiting;
+      expect(entered).toBe(true);
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("lock cleanup leaves a replacement lock owned by another holder", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-lock-owner-"));
+    try {
+      const lockPath = join(home, ".corbits", "test-auth.json.lock");
+      const replacement = JSON.stringify({ pid: process.pid, owner: "replacement" });
+
+      await withStoreFileLock(lockPath, async () => {
+        await unlink(join(lockPath, "owner"));
+        await rmdir(lockPath);
+        await mkdir(lockPath, { mode: 0o700 });
+        await writeFile(join(lockPath, "owner"), replacement);
+      });
+
+      expect(await readFile(join(lockPath, "owner"), "utf8")).toBe(replacement);
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("two stale reclaimers cannot enter the critical section together", async () => {
+    const home = await mkdtemp(join(tmpdir(), "oauth-stale-reclaimers-"));
+    try {
+      const lockPath = join(home, ".corbits", "test-auth.json.lock");
+      await mkdir(lockPath, { recursive: true, mode: 0o700 });
+      const ownerFile = join(lockPath, "owner");
+      await writeFile(ownerFile, JSON.stringify({ pid: 2_147_483_647, owner: "dead" }));
+      const past = new Date(Date.now() - 60_000);
+      await utimes(ownerFile, past, past);
+
+      let active = 0;
+      let entered = 0;
+      let maxActive = 0;
+      const contender = () =>
+        withStoreFileLock(lockPath, async () => {
+          active += 1;
+          entered += 1;
+          maxActive = Math.max(maxActive, active);
+          await Bun.sleep(100);
+          active -= 1;
+        });
+
+      await Promise.all([contender(), contender()]);
+
+      expect(entered).toBe(2);
+      expect(maxActive).toBe(1);
+    } finally {
+      await rm(home, { recursive: true, force: true });
+    }
+  });
+
+  test("different home directories do not contend on the same lock", async () => {
+    const homeA = await mkdtemp(join(tmpdir(), "oauth-home-a-"));
+    const homeB = await mkdtemp(join(tmpdir(), "oauth-home-b-"));
+    try {
+      const store = createAuthStore<TestTokens>({
+        filename: "test-auth.json",
+        isTokens: isTestTokens,
+      });
+
+      // Concurrent saves to different homes should not block each other.
+      await Promise.all([
+        store.saveProfile(
+          { name: "from-a", tokens: { access: "a", refresh: "r", expiresAt: 1 }, createdAt: 1 },
+          homeA,
+        ),
+        store.saveProfile(
+          { name: "from-b", tokens: { access: "b", refresh: "r", expiresAt: 2 }, createdAt: 2 },
+          homeB,
+        ),
+      ]);
+
+      const profilesA = await store.listProfiles(homeA);
+      const profilesB = await store.listProfiles(homeB);
+
+      expect(profilesA).toHaveLength(1);
+      expect(profilesB).toHaveLength(1);
+      const firstA = profilesA[0];
+      const firstB = profilesB[0];
+      expect(firstA).toBeDefined();
+      expect(firstB).toBeDefined();
+      expect(firstA!.name).toBe("from-a");
+      expect(firstB!.name).toBe("from-b");
+    } finally {
+      await rm(homeA, { recursive: true, force: true });
+      await rm(homeB, { recursive: true, force: true });
+    }
+  });
+
   test("drops invalid profile entries instead of wedging on them", async () => {
     const home = await mkdtemp(join(tmpdir(), "oauth-store-"));
     try {