From 61bd8f874aa3c97e92db4d7d07cc7ce9ccc227da Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 1 Sep 2026 23:34:13 -0700 Subject: [PATCH 1/4] Serialize OAuth profile-store read-modify-write updates --- src/auth/oauth/oauth.test.ts | 133 +++++++++++++++++++++++++++++++++++ src/auth/oauth/store.ts | 103 ++++++++++++++++++++++----- 2 files changed, 217 insertions(+), 19 deletions(-) diff --git a/src/auth/oauth/oauth.test.ts b/src/auth/oauth/oauth.test.ts index aa6f225e7..b112c737b 100644 --- a/src/auth/oauth/oauth.test.ts +++ b/src/auth/oauth/oauth.test.ts @@ -134,6 +134,139 @@ describe("createAuthStore", () => { } }); + test("concurrent saveProfile calls preserve all profiles", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-lock-")); + try { + const store = createAuthStore({ + 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({ + 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". + 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: "a", tokens: { access: "a2", refresh: "r2", expiresAt: 20 }, createdAt: 1 }, + home, + ), + ]); + + const profiles = await store.listProfiles(home); + const names = profiles.map((p) => p.name).sort(); + expect(names).toEqual(["a", "b"]); + + for (const p of profiles) { + expect(typeof p.tokens.access).toBe("string"); + expect(typeof p.tokens.refresh).toBe("string"); + expect(typeof p.tokens.expiresAt).toBe("number"); + } + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + + test("lock file is cleaned up after operations", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-lock-cleanup-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + await store.saveProfile( + { name: "x", tokens: { access: "a", refresh: "r", expiresAt: 1 }, createdAt: 1 }, + home, + ); + + // The lock file must not linger after the operation. + const lockFile = join(home, ".corbits", "test-auth.json.lock"); + await expect(readFile(lockFile, "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({ + 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("drops invalid profile entries instead of wedging on them", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-store-")); try { diff --git a/src/auth/oauth/store.ts b/src/auth/oauth/store.ts index e109023b9..ea8f7488e 100644 --- a/src/auth/oauth/store.ts +++ b/src/auth/oauth/store.ts @@ -1,4 +1,4 @@ -import { mkdir, readFile, rename, writeFile } from "node:fs/promises"; +import { mkdir, open, readFile, rename, stat, unlink, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; import { SETTINGS_DIR_NAME } from "../../branding.js"; @@ -104,6 +104,65 @@ export function createAuthStore( await rename(tmp, path); } + // Exclusive file lock serialises read-modify-write operations so concurrent + // CLI sessions cannot clobber each other's profiles or fresher tokens. + const LOCK_RETRY_MS = 50; + const LOCK_TIMEOUT_MS = 15_000; + const STALE_LOCK_MS = 30_000; + + function lockPath(home: string): string { + return `${authPath(home)}.lock`; + } + + async function withStoreLock(home: string, fn: () => Promise): Promise { + const path = lockPath(home); + await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + const deadline = Date.now() + LOCK_TIMEOUT_MS; + + while (true) { + let fd: Awaited> | undefined; + try { + fd = await open(path, "wx"); + await fd.close(); + break; + } catch (err) { + if (fd !== undefined) await fd.close().catch(() => {}); + + if ( + typeof err === "object" && + err !== null && + "code" in err && + (err as { code?: unknown }).code === "EEXIST" + ) { + // Another process holds the lock; check for a stale lock. + try { + const info = await stat(path); + if (Date.now() - info.mtimeMs > STALE_LOCK_MS) { + await unlink(path).catch(() => {}); + continue; + } + } catch { + // Lock vanished between the failed create and the stat — retry. + } + + if (Date.now() >= deadline) { + throw new Error(`Timed out acquiring store lock: ${path}`); + } + + await new Promise((r) => setTimeout(r, LOCK_RETRY_MS)); + continue; + } + throw err; + } + } + + try { + return await fn(); + } finally { + await unlink(path).catch(() => {}); + } + } + return { authPath, async listProfiles(home: string = homedir()): Promise[]> { @@ -118,30 +177,36 @@ export function createAuthStore( return file.profiles[name]; }, async saveProfile(profile: AuthProfile, home: string = homedir()): Promise { - const file = await readAuthFile(home); - file.profiles[profile.name] = profile; - await writeAuthFile(file, home); + return withStoreLock(home, async () => { + const file = await readAuthFile(home); + file.profiles[profile.name] = profile; + await writeAuthFile(file, home); + }); }, // Persist refreshed tokens for an existing profile, preserving createdAt. A // no-op if the profile no longer exists (e.g. removed in another session). async updateTokens(name: string, tokens: TTokens, home: string = homedir()): Promise { - const file = await readAuthFile(home); - const existing = file.profiles[name]; - if (existing === undefined) return; - file.profiles[name] = { ...existing, tokens }; - await writeAuthFile(file, home); + return withStoreLock(home, async () => { + const file = await readAuthFile(home); + const existing = file.profiles[name]; + if (existing === undefined) return; + file.profiles[name] = { ...existing, tokens }; + await writeAuthFile(file, home); + }); }, async removeProfile(name: string | undefined, home: string = homedir()): Promise { - const file = await readAuthFile(home); - if (name === undefined) { - const removed = Object.keys(file.profiles); - await writeAuthFile({ profiles: {} }, home); - return removed; - } - if (file.profiles[name] === undefined) return []; - Reflect.deleteProperty(file.profiles, name); - await writeAuthFile(file, home); - return [name]; + return withStoreLock(home, async () => { + const file = await readAuthFile(home); + if (name === undefined) { + const removed = Object.keys(file.profiles); + await writeAuthFile({ profiles: {} }, home); + return removed; + } + if (file.profiles[name] === undefined) return []; + Reflect.deleteProperty(file.profiles, name); + await writeAuthFile(file, home); + return [name]; + }); }, }; } From 0020b209ec2164064382085dd32cb987dd21aa54 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 1 Sep 2026 23:43:45 -0700 Subject: [PATCH 2/4] Reduce stale-lock threshold below lock timeout and add edge-case tests --- src/auth/oauth/oauth.test.ts | 71 +++++++++++++++++++++++++++++++++++- src/auth/oauth/store.ts | 2 +- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/src/auth/oauth/oauth.test.ts b/src/auth/oauth/oauth.test.ts index b112c737b..05f7b2e6a 100644 --- a/src/auth/oauth/oauth.test.ts +++ b/src/auth/oauth/oauth.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { 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"; @@ -267,6 +267,75 @@ describe("createAuthStore", () => { } }); + test("stale lock file is recovered automatically", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-stale-lock-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + isTokens: isTestTokens, + }); + + // Simulate a stale lock by creating the lock file and backdating its mtime. + const lockDir = join(home, ".corbits"); + await mkdir(lockDir, { recursive: true, mode: 0o700 }); + const lockFile = join(lockDir, "test-auth.json.lock"); + await writeFile(lockFile, ""); + const past = new Date(Date.now() - 60_000); // 60s in the past + await utimes(lockFile, 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("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({ + 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 { diff --git a/src/auth/oauth/store.ts b/src/auth/oauth/store.ts index ea8f7488e..27885cb6c 100644 --- a/src/auth/oauth/store.ts +++ b/src/auth/oauth/store.ts @@ -108,7 +108,7 @@ export function createAuthStore( // CLI sessions cannot clobber each other's profiles or fresher tokens. const LOCK_RETRY_MS = 50; const LOCK_TIMEOUT_MS = 15_000; - const STALE_LOCK_MS = 30_000; + const STALE_LOCK_MS = 10_000; function lockPath(home: string): string { return `${authPath(home)}.lock`; From 94744c4bd8fe26f9487ed7e864fb729df286b776 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 2 Sep 2026 13:46:04 -0700 Subject: [PATCH 3/4] Serialize OAuth profile store updates --- src/auth/oauth/oauth.test.ts | 75 ++++++++++++-- src/auth/oauth/store.ts | 194 +++++++++++++++++++++++++---------- 2 files changed, 201 insertions(+), 68 deletions(-) diff --git a/src/auth/oauth/oauth.test.ts b/src/auth/oauth/oauth.test.ts index 05f7b2e6a..545fce4f2 100644 --- a/src/auth/oauth/oauth.test.ts +++ b/src/auth/oauth/oauth.test.ts @@ -1,12 +1,12 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, readFile, rm, utimes, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, readFile, rm, 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", @@ -187,25 +187,34 @@ describe("createAuthStore", () => { home, ); - // Concurrently update tokens for both profiles plus a full save of "a". + // 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: "a", tokens: { access: "a2", refresh: "r2", expiresAt: 20 }, createdAt: 1 }, + { 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"]); - - for (const p of profiles) { - expect(typeof p.tokens.access).toBe("string"); - expect(typeof p.tokens.refresh).toBe("string"); - expect(typeof p.tokens.expiresAt).toBe("number"); - } + 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 }); } @@ -298,6 +307,50 @@ describe("createAuthStore", () => { } }); + test("an old lock owned by a live process is not displaced", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-live-lock-")); + try { + const lockDir = join(home, ".corbits"); + await mkdir(lockDir, { recursive: true, mode: 0o700 }); + const lockFile = join(lockDir, "test-auth.json.lock"); + await writeFile(lockFile, JSON.stringify({ pid: process.pid, owner: "live-holder" })); + const past = new Date(Date.now() - 60_000); + await utimes(lockFile, past, past); + + let entered = false; + const waiting = withStoreFileLock(lockFile, async () => { + entered = true; + }); + await Bun.sleep(150); + + expect(entered).toBe(false); + await unlink(lockFile); + 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 lockDir = join(home, ".corbits"); + await mkdir(lockDir, { recursive: true, mode: 0o700 }); + const lockFile = join(lockDir, "test-auth.json.lock"); + const replacement = JSON.stringify({ pid: process.pid, owner: "replacement" }); + + await withStoreFileLock(lockFile, async () => { + await unlink(lockFile); + await writeFile(lockFile, replacement); + }); + + expect(await readFile(lockFile, "utf8")).toBe(replacement); + } 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-")); diff --git a/src/auth/oauth/store.ts b/src/auth/oauth/store.ts index 27885cb6c..da1f08696 100644 --- a/src/auth/oauth/store.ts +++ b/src/auth/oauth/store.ts @@ -1,6 +1,8 @@ +import { randomUUID } from "node:crypto"; import { mkdir, open, readFile, rename, stat, unlink, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; +import { type } from "arktype"; import { SETTINGS_DIR_NAME } from "../../branding.js"; // On-disk store for named OAuth profiles. A user may hold multiple subscriptions @@ -43,6 +45,138 @@ interface AuthFile { profiles: Record>; } +const LOCK_RETRY_MS = 50; +const LOCK_TIMEOUT_MS = 15_000; +const STALE_LOCK_MS = 10_000; + +const LockOwner = type({ + pid: "number.integer > 0", + owner: "string", +}); + +interface LockSnapshot { + contents: string; + dev: number; + ino: number; + mtimeMs: number; +} + +function isErrno(error: unknown, code: string): boolean { + return ( + typeof error === "object" && + error !== null && + "code" in error && + (error as { code?: unknown }).code === code + ); +} + +async function readLockSnapshot(path: string): Promise { + try { + const before = await stat(path); + const contents = await readFile(path, "utf8"); + const after = await stat(path); + if (before.dev !== after.dev || before.ino !== after.ino) return undefined; + return { contents, dev: after.dev, ino: after.ino, mtimeMs: after.mtimeMs }; + } catch (error) { + if (isErrno(error, "ENOENT")) return undefined; + throw error; + } +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (error) { + return isErrno(error, "EPERM"); + } +} + +async function removeLockSnapshot(path: string, expected: LockSnapshot): Promise { + const current = await readLockSnapshot(path); + if ( + current === undefined || + current.dev !== expected.dev || + current.ino !== expected.ino || + current.contents !== expected.contents + ) { + return false; + } + + try { + await unlink(path); + return true; + } catch (error) { + if (isErrno(error, "ENOENT")) return false; + throw error; + } +} + +async function createLock(path: string, contents: string): Promise { + const handle = await open(path, "wx", 0o600); + try { + await handle.writeFile(contents); + const info = await handle.stat(); + return { contents, dev: info.dev, ino: info.ino, mtimeMs: info.mtimeMs }; + } catch (cause) { + const info = await handle.stat().catch(() => undefined); + if (info !== undefined) { + await removeLockSnapshot(path, { + contents, + dev: info.dev, + ino: info.ino, + mtimeMs: info.mtimeMs, + }).catch(() => false); + } + throw new Error(`Failed to create store lock: ${path}`, { cause }); + } finally { + await handle.close().catch(() => {}); + } +} + +async function removeStaleLock(path: string): Promise { + const snapshot = await readLockSnapshot(path); + if (snapshot === undefined || Date.now() - snapshot.mtimeMs <= STALE_LOCK_MS) return false; + + let owner: typeof LockOwner.infer | undefined; + try { + const parsed = LockOwner(JSON.parse(snapshot.contents)); + if (!(parsed instanceof type.errors)) owner = parsed; + } catch { + // Locks written by interrupted or older clients have no usable owner metadata. + } + + if (owner !== undefined && isProcessAlive(owner.pid)) return false; + return removeLockSnapshot(path, snapshot); +} + +export async function withStoreFileLock(path: string, fn: () => Promise): Promise { + await mkdir(dirname(path), { recursive: true, mode: 0o700 }); + const contents = JSON.stringify({ pid: process.pid, owner: randomUUID() }); + const deadline = Date.now() + LOCK_TIMEOUT_MS; + let acquired: LockSnapshot; + + while (true) { + try { + acquired = await createLock(path, contents); + break; + } catch (error) { + if (!isErrno(error, "EEXIST")) throw error; + + if (await removeStaleLock(path)) continue; + if (Date.now() >= deadline) throw new Error(`Timed out acquiring store lock: ${path}`); + + await Bun.sleep(LOCK_RETRY_MS); + } + } + + try { + return await fn(); + } finally { + await removeLockSnapshot(path, acquired); + } +} + function isProfile( value: unknown, isTokens: (value: unknown) => value is TTokens, @@ -104,63 +238,9 @@ export function createAuthStore( await rename(tmp, path); } - // Exclusive file lock serialises read-modify-write operations so concurrent - // CLI sessions cannot clobber each other's profiles or fresher tokens. - const LOCK_RETRY_MS = 50; - const LOCK_TIMEOUT_MS = 15_000; - const STALE_LOCK_MS = 10_000; - - function lockPath(home: string): string { - return `${authPath(home)}.lock`; - } - - async function withStoreLock(home: string, fn: () => Promise): Promise { - const path = lockPath(home); - await mkdir(dirname(path), { recursive: true, mode: 0o700 }); - const deadline = Date.now() + LOCK_TIMEOUT_MS; - - while (true) { - let fd: Awaited> | undefined; - try { - fd = await open(path, "wx"); - await fd.close(); - break; - } catch (err) { - if (fd !== undefined) await fd.close().catch(() => {}); - - if ( - typeof err === "object" && - err !== null && - "code" in err && - (err as { code?: unknown }).code === "EEXIST" - ) { - // Another process holds the lock; check for a stale lock. - try { - const info = await stat(path); - if (Date.now() - info.mtimeMs > STALE_LOCK_MS) { - await unlink(path).catch(() => {}); - continue; - } - } catch { - // Lock vanished between the failed create and the stat — retry. - } - - if (Date.now() >= deadline) { - throw new Error(`Timed out acquiring store lock: ${path}`); - } - - await new Promise((r) => setTimeout(r, LOCK_RETRY_MS)); - continue; - } - throw err; - } - } - - try { - return await fn(); - } finally { - await unlink(path).catch(() => {}); - } + // Serialise read-modify-write operations across concurrent CLI processes. + function withStoreLock(home: string, fn: () => Promise): Promise { + return withStoreFileLock(`${authPath(home)}.lock`, fn); } return { From 49b41f5f03e6af6fe9ec72b10e5296767bf2ba55 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 2 Sep 2026 14:12:01 -0700 Subject: [PATCH 4/4] Fix stale OAuth lock reclamation race --- src/auth/oauth/oauth.test.ts | 80 ++++++++++++++------- src/auth/oauth/store.ts | 132 +++++++++++++++++++++++------------ 2 files changed, 141 insertions(+), 71 deletions(-) diff --git a/src/auth/oauth/oauth.test.ts b/src/auth/oauth/oauth.test.ts index 545fce4f2..09461ce5b 100644 --- a/src/auth/oauth/oauth.test.ts +++ b/src/auth/oauth/oauth.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, readFile, rm, unlink, utimes, 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"; @@ -220,7 +220,7 @@ describe("createAuthStore", () => { } }); - test("lock file is cleaned up after operations", async () => { + test("lock directory is cleaned up after operations", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-lock-cleanup-")); try { const store = createAuthStore({ @@ -233,9 +233,8 @@ describe("createAuthStore", () => { home, ); - // The lock file must not linger after the operation. - const lockFile = join(home, ".corbits", "test-auth.json.lock"); - await expect(readFile(lockFile, "utf8")).rejects.toThrow(); + 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 }); } @@ -276,7 +275,7 @@ describe("createAuthStore", () => { } }); - test("stale lock file is recovered automatically", async () => { + test("malformed stale lock is recovered automatically", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-stale-lock-")); try { const store = createAuthStore({ @@ -284,13 +283,12 @@ describe("createAuthStore", () => { isTokens: isTestTokens, }); - // Simulate a stale lock by creating the lock file and backdating its mtime. - const lockDir = join(home, ".corbits"); + const lockDir = join(home, ".corbits", "test-auth.json.lock"); await mkdir(lockDir, { recursive: true, mode: 0o700 }); - const lockFile = join(lockDir, "test-auth.json.lock"); - await writeFile(lockFile, ""); + const ownerFile = join(lockDir, "owner"); + await writeFile(ownerFile, "malformed"); const past = new Date(Date.now() - 60_000); // 60s in the past - await utimes(lockFile, past, past); + await utimes(ownerFile, past, past); // The store should detect the stale lock, remove it, and succeed. await store.saveProfile( @@ -310,21 +308,22 @@ describe("createAuthStore", () => { test("an old lock owned by a live process is not displaced", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-live-lock-")); try { - const lockDir = join(home, ".corbits"); - await mkdir(lockDir, { recursive: true, mode: 0o700 }); - const lockFile = join(lockDir, "test-auth.json.lock"); - await writeFile(lockFile, JSON.stringify({ pid: process.pid, owner: "live-holder" })); + 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(lockFile, past, past); + await utimes(ownerFile, past, past); let entered = false; - const waiting = withStoreFileLock(lockFile, async () => { + const waiting = withStoreFileLock(lockPath, async () => { entered = true; }); await Bun.sleep(150); expect(entered).toBe(false); - await unlink(lockFile); + await unlink(ownerFile); + await rmdir(lockPath); await waiting; expect(entered).toBe(true); } finally { @@ -335,17 +334,48 @@ describe("createAuthStore", () => { test("lock cleanup leaves a replacement lock owned by another holder", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-lock-owner-")); try { - const lockDir = join(home, ".corbits"); - await mkdir(lockDir, { recursive: true, mode: 0o700 }); - const lockFile = join(lockDir, "test-auth.json.lock"); + const lockPath = join(home, ".corbits", "test-auth.json.lock"); const replacement = JSON.stringify({ pid: process.pid, owner: "replacement" }); - await withStoreFileLock(lockFile, async () => { - await unlink(lockFile); - await writeFile(lockFile, 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(lockFile, "utf8")).toBe(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 }); } diff --git a/src/auth/oauth/store.ts b/src/auth/oauth/store.ts index da1f08696..0953eb1ed 100644 --- a/src/auth/oauth/store.ts +++ b/src/auth/oauth/store.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { mkdir, open, readFile, rename, stat, unlink, writeFile } from "node:fs/promises"; +import { mkdir, open, readFile, rename, rmdir, stat, unlink, writeFile } from "node:fs/promises"; import { homedir } from "node:os"; import { dirname, join } from "node:path"; import { type } from "arktype"; @@ -56,11 +56,12 @@ const LockOwner = type({ interface LockSnapshot { contents: string; - dev: number; - ino: number; mtimeMs: number; } +const LOCK_OWNER_FILE = "owner"; +const LOCK_RECLAIM_DIR = "reclaim"; + function isErrno(error: unknown, code: string): boolean { return ( typeof error === "object" && @@ -72,11 +73,10 @@ function isErrno(error: unknown, code: string): boolean { async function readLockSnapshot(path: string): Promise { try { - const before = await stat(path); - const contents = await readFile(path, "utf8"); - const after = await stat(path); - if (before.dev !== after.dev || before.ino !== after.ino) return undefined; - return { contents, dev: after.dev, ino: after.ino, mtimeMs: after.mtimeMs }; + const ownerPath = join(path, LOCK_OWNER_FILE); + const contents = await readFile(ownerPath, "utf8"); + const info = await stat(ownerPath); + return { contents, mtimeMs: info.mtimeMs }; } catch (error) { if (isErrno(error, "ENOENT")) return undefined; throw error; @@ -92,73 +92,113 @@ function isProcessAlive(pid: number): boolean { } } -async function removeLockSnapshot(path: string, expected: LockSnapshot): Promise { - const current = await readLockSnapshot(path); - if ( - current === undefined || - current.dev !== expected.dev || - current.ino !== expected.ino || - current.contents !== expected.contents - ) { - return false; +async function removeDirectory(path: string): Promise { + try { + await rmdir(path); + return true; + } catch (error) { + if (isErrno(error, "ENOENT") || isErrno(error, "ENOTEMPTY")) return false; + throw error; } +} +async function claimLockForRemoval(path: string): Promise { try { - await unlink(path); + await mkdir(join(path, LOCK_RECLAIM_DIR), { mode: 0o700 }); return true; } catch (error) { - if (isErrno(error, "ENOENT")) return false; + if (isErrno(error, "EEXIST") || isErrno(error, "ENOENT")) return false; throw error; } } -async function createLock(path: string, contents: string): Promise { - const handle = await open(path, "wx", 0o600); +async function removeOwnedLock(path: string, expectedContents: string): Promise { + const deadline = Date.now() + LOCK_TIMEOUT_MS; + while (!(await claimLockForRemoval(path))) { + if (Date.now() >= deadline) return false; + await Bun.sleep(LOCK_RETRY_MS); + } + let removedOwner = false; try { - await handle.writeFile(contents); - const info = await handle.stat(); - return { contents, dev: info.dev, ino: info.ino, mtimeMs: info.mtimeMs }; - } catch (cause) { - const info = await handle.stat().catch(() => undefined); - if (info !== undefined) { - await removeLockSnapshot(path, { - contents, - dev: info.dev, - ino: info.ino, - mtimeMs: info.mtimeMs, - }).catch(() => false); - } - throw new Error(`Failed to create store lock: ${path}`, { cause }); + const current = await readLockSnapshot(path); + if (current === undefined || current.contents !== expectedContents) return false; + await unlink(join(path, LOCK_OWNER_FILE)); + removedOwner = true; } finally { - await handle.close().catch(() => {}); + await removeDirectory(join(path, LOCK_RECLAIM_DIR)); + if (removedOwner) await removeDirectory(path); } + return removedOwner; } -async function removeStaleLock(path: string): Promise { - const snapshot = await readLockSnapshot(path); - if (snapshot === undefined || Date.now() - snapshot.mtimeMs <= STALE_LOCK_MS) return false; +async function createLock(path: string, contents: string): Promise { + await mkdir(path, { mode: 0o700 }); + const ownerPath = join(path, LOCK_OWNER_FILE); + try { + const handle = await open(ownerPath, "wx", 0o600); + try { + await handle.writeFile(contents); + } finally { + await handle.close(); + } + } catch (cause) { + await unlink(ownerPath).catch(() => {}); + await removeDirectory(path).catch(() => false); + throw new Error(`Failed to create store lock: ${path}`, { cause }); + } +} - let owner: typeof LockOwner.infer | undefined; +function parseLockOwner(snapshot: LockSnapshot | undefined): typeof LockOwner.infer | undefined { + if (snapshot === undefined) return undefined; try { const parsed = LockOwner(JSON.parse(snapshot.contents)); - if (!(parsed instanceof type.errors)) owner = parsed; + return parsed instanceof type.errors ? undefined : parsed; } catch { // Locks written by interrupted or older clients have no usable owner metadata. + return undefined; } +} - if (owner !== undefined && isProcessAlive(owner.pid)) return false; - return removeLockSnapshot(path, snapshot); +async function isStaleLock(path: string, snapshot: LockSnapshot | undefined): Promise { + let mtimeMs = snapshot?.mtimeMs; + if (mtimeMs === undefined) { + try { + mtimeMs = (await stat(path)).mtimeMs; + } catch (error) { + if (isErrno(error, "ENOENT")) return false; + throw error; + } + } + if (Date.now() - mtimeMs <= STALE_LOCK_MS) return false; + const owner = parseLockOwner(snapshot); + return owner === undefined || !isProcessAlive(owner.pid); +} + +async function removeStaleLock(path: string): Promise { + const initial = await readLockSnapshot(path); + if (!(await isStaleLock(path, initial))) return false; + if (!(await claimLockForRemoval(path))) return false; + let shouldRemove = false; + try { + const snapshot = await readLockSnapshot(path); + if (!(await isStaleLock(path, snapshot))) return false; + if (snapshot !== undefined) await unlink(join(path, LOCK_OWNER_FILE)); + shouldRemove = true; + } finally { + await removeDirectory(join(path, LOCK_RECLAIM_DIR)); + if (shouldRemove) await removeDirectory(path); + } + return shouldRemove; } export async function withStoreFileLock(path: string, fn: () => Promise): Promise { await mkdir(dirname(path), { recursive: true, mode: 0o700 }); const contents = JSON.stringify({ pid: process.pid, owner: randomUUID() }); const deadline = Date.now() + LOCK_TIMEOUT_MS; - let acquired: LockSnapshot; while (true) { try { - acquired = await createLock(path, contents); + await createLock(path, contents); break; } catch (error) { if (!isErrno(error, "EEXIST")) throw error; @@ -173,7 +213,7 @@ export async function withStoreFileLock(path: string, fn: () => Promise): try { return await fn(); } finally { - await removeLockSnapshot(path, acquired); + await removeOwnedLock(path, contents); } }