From 79e1034be88f378dc8a5f08eaf2cb45550c690c4 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 10 Sep 2026 21:24:47 -0700 Subject: [PATCH 1/4] Queue same-process credential writes so each gets its own lock window Same-process writers all polled the on-disk lock against a deadline that started at call time, so one lock held past the timeout failed the whole burst instead of just the first waiter. Writes now chain per auth file, and temp paths get a per-call counter so two saves in one process can never share one. --- src/auth/store.test.ts | 63 ++++++++++++++++++++++++++++++++++++++++++ src/auth/store.ts | 37 ++++++++++++++++++++++--- 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/src/auth/store.test.ts b/src/auth/store.test.ts index 169773d25..174b64d2e 100644 --- a/src/auth/store.test.ts +++ b/src/auth/store.test.ts @@ -114,6 +114,69 @@ describe("createAuthStore", () => { } }); + test("gives queued same-process writes their own lock window", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-store-queue-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + settingsDirName: TEST_SETTINGS_DIR, + isTokens: isTestTokens, + }); + await store.saveProfile( + { + name: "work", + tokens: { access: "a", refresh: "r", expiresAt: 1 }, + createdAt: 1, + }, + home, + ); + + // A foreign process holds the credential lock past the first waiter's + // deadline, then releases; the write queued behind it must still land. + const lockPath = `${store.authPath(home)}.lock`; + await writeFile(lockPath, "foreign", { mode: 0o600 }); + + const first = store + .updateTokens( + "work", + { access: "first", refresh: "r1", expiresAt: 2 }, + home, + ) + .then( + () => "resolved" as const, + (error: unknown) => error, + ); + const second = store + .updateTokens( + "work", + { access: "second", refresh: "r2", expiresAt: 3 }, + home, + ) + .then( + () => "resolved" as const, + (error: unknown) => error, + ); + + await Bun.sleep(1_400); + await rm(lockPath, { force: true }); + + const firstResult = await first; + expect(firstResult).toBeInstanceOf(Error); + if (firstResult instanceof Error) { + expect(firstResult.message).toContain( + "Timed out waiting for OAuth credential lock", + ); + } + expect(await second).toBe("resolved"); + + expect((await store.loadProfile("work", home))?.tokens.access).toBe( + "second", + ); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + test("round-trips profiles under an injected home and survives corrupt files", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-store-")); try { diff --git a/src/auth/store.ts b/src/auth/store.ts index 349997663..70273ff98 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -48,6 +48,15 @@ interface AuthFile { const LOCK_RETRY_MS = 25; const LOCK_TIMEOUT_MS = 1_000; +// pid alone is not unique per call — concurrent saves in one process must not +// share a temp path or the second rename hits ENOENT after the first moves it. +let tmpWriteCounter = 0; + +// Same-process ops on one auth file queue here so a caller's lock deadline +// starts when it actually runs, not when it was invoked — otherwise one lock +// held past LOCK_TIMEOUT_MS fails the whole burst, not just the first waiter. +const updateChains = new Map>(); + const AuthFileShape = type({ profiles: "Record", }); @@ -115,7 +124,7 @@ export function createAuthStore( ): Promise { const path = authPath(home); await mkdir(dirname(path), { recursive: true, mode: 0o700 }); - const tmp = `${path}.${String(process.pid)}.tmp`; + const tmp = `${path}.${process.pid}.${(tmpWriteCounter += 1)}.tmp`; await writeFile(tmp, JSON.stringify(file, null, 2), { mode: 0o600 }); await rename(tmp, path); } @@ -158,6 +167,26 @@ export function createAuthStore( } } + function enqueueAuthFileOp( + home: string, + op: () => Promise, + ): Promise { + const path = authPath(home); + const previous = updateChains.get(path) ?? Promise.resolve(); + const run = previous.then( + () => withAuthFileLock(home, op), + () => withAuthFileLock(home, op), + ); + updateChains.set( + path, + run.then( + () => undefined, + () => undefined, + ), + ); + return run; + } + return { authPath, async listProfiles( @@ -179,7 +208,7 @@ export function createAuthStore( profile: AuthProfile, home: string = homedir(), ): Promise { - await withAuthFileLock(home, async () => { + await enqueueAuthFileOp(home, async () => { const file = await readAuthFile(home); file.profiles[profile.name] = profile; await writeAuthFile(file, home); @@ -192,7 +221,7 @@ export function createAuthStore( tokens: TTokens, home: string = homedir(), ): Promise { - await withAuthFileLock(home, async () => { + await enqueueAuthFileOp(home, async () => { const file = await readAuthFile(home); const existing = file.profiles[name]; if (existing === undefined) return; @@ -204,7 +233,7 @@ export function createAuthStore( name: string | undefined, home: string = homedir(), ): Promise { - return withAuthFileLock(home, async () => { + return enqueueAuthFileOp(home, async () => { const file = await readAuthFile(home); if (name === undefined) { const removed = Object.keys(file.profiles); From bb3e210b767623e6bb29888a04d22d8e9bd852a9 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 10 Sep 2026 21:40:32 -0700 Subject: [PATCH 2/4] Tighten auth-store concurrency tests around the write queue Replace the fixed 1.4s sleep with await-then-release so the lock-window case cannot flake if the event loop slips past the foreign lock removal, and add a same-process dual-profile save that pins the lost-update net without another timed wait. --- src/auth/store.test.ts | 96 ++++++++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 32 deletions(-) diff --git a/src/auth/store.test.ts b/src/auth/store.test.ts index 174b64d2e..d0333dea8 100644 --- a/src/auth/store.test.ts +++ b/src/auth/store.test.ts @@ -114,6 +114,54 @@ describe("createAuthStore", () => { } }); + test("queues same-process profile writes so neither save is lost", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-store-same-process-")); + try { + const store = createAuthStore({ + filename: "test-auth.json", + settingsDirName: TEST_SETTINGS_DIR, + isTokens: isTestTokens, + }); + + await Promise.all([ + store.saveProfile( + { + name: "personal", + tokens: { access: "p", refresh: "pr", expiresAt: 1 }, + createdAt: 1, + }, + home, + ), + store.saveProfile( + { + name: "work", + tokens: { access: "w", refresh: "wr", expiresAt: 2 }, + createdAt: 2, + }, + home, + ), + ]); + + const profiles = await store.listProfiles(home); + expect(profiles.map((profile) => profile.name)).toEqual([ + "personal", + "work", + ]); + expect(profiles.find((profile) => profile.name === "personal")).toEqual({ + name: "personal", + tokens: { access: "p", refresh: "pr", expiresAt: 1 }, + createdAt: 1, + }); + expect(profiles.find((profile) => profile.name === "work")).toEqual({ + name: "work", + tokens: { access: "w", refresh: "wr", expiresAt: 2 }, + createdAt: 2, + }); + } finally { + await rm(home, { recursive: true, force: true }); + } + }); + test("gives queued same-process writes their own lock window", async () => { const home = await mkdtemp(join(tmpdir(), "oauth-store-queue-")); try { @@ -131,43 +179,27 @@ describe("createAuthStore", () => { home, ); - // A foreign process holds the credential lock past the first waiter's - // deadline, then releases; the write queued behind it must still land. + // Hold the lock until the head of the same-process queue times out; the + // queued write must still get its own lock window after we release. const lockPath = `${store.authPath(home)}.lock`; await writeFile(lockPath, "foreign", { mode: 0o600 }); - const first = store - .updateTokens( - "work", - { access: "first", refresh: "r1", expiresAt: 2 }, - home, - ) - .then( - () => "resolved" as const, - (error: unknown) => error, - ); - const second = store - .updateTokens( - "work", - { access: "second", refresh: "r2", expiresAt: 3 }, - home, - ) - .then( - () => "resolved" as const, - (error: unknown) => error, - ); + const first = store.updateTokens( + "work", + { access: "first", refresh: "r1", expiresAt: 2 }, + home, + ); + const second = store.updateTokens( + "work", + { access: "second", refresh: "r2", expiresAt: 3 }, + home, + ); - await Bun.sleep(1_400); + await expect(first).rejects.toThrow( + "Timed out waiting for OAuth credential lock", + ); await rm(lockPath, { force: true }); - - const firstResult = await first; - expect(firstResult).toBeInstanceOf(Error); - if (firstResult instanceof Error) { - expect(firstResult.message).toContain( - "Timed out waiting for OAuth credential lock", - ); - } - expect(await second).toBe("resolved"); + await expect(second).resolves.toBeUndefined(); expect((await store.loadProfile("work", home))?.tokens.access).toBe( "second", From 2f185fe31b32f449647afe0b47824b7cdb16d3e5 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 10 Sep 2026 21:55:35 -0700 Subject: [PATCH 3/4] Strengthen the auth-store queue regression and clarify comments Replace the dual-save case that stayed green without the queue with a 50-way same-process burst that needs fresh lock deadlines, and document the queue plus defensive temp-counter in the store header. --- src/auth/store.test.ts | 69 +++++++++++++++++++++--------------------- src/auth/store.ts | 7 +++-- 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/src/auth/store.test.ts b/src/auth/store.test.ts index d0333dea8..8931a0267 100644 --- a/src/auth/store.test.ts +++ b/src/auth/store.test.ts @@ -114,8 +114,8 @@ describe("createAuthStore", () => { } }); - test("queues same-process profile writes so neither save is lost", async () => { - const home = await mkdtemp(join(tmpdir(), "oauth-store-same-process-")); + test("keeps a same-process burst of profile saves without shared-deadline loss", async () => { + const home = await mkdtemp(join(tmpdir(), "oauth-store-burst-")); try { const store = createAuthStore({ filename: "test-auth.json", @@ -123,40 +123,39 @@ describe("createAuthStore", () => { isTokens: isTestTokens, }); - await Promise.all([ - store.saveProfile( - { - name: "personal", - tokens: { access: "p", refresh: "pr", expiresAt: 1 }, - createdAt: 1, - }, - home, - ), - store.saveProfile( - { - name: "work", - tokens: { access: "w", refresh: "wr", expiresAt: 2 }, - createdAt: 2, - }, - home, + // Without the per-path queue, a large same-process burst shares one lock + // deadline from invoke time and some waiters time out. With the queue, + // each save gets its own window and all land. + const names = Array.from( + { length: 50 }, + (_, index) => `profile-${String(index)}`, + ); + const results = await Promise.allSettled( + names.map((name) => + store.saveProfile( + { + name, + tokens: { + access: `access-${name}`, + refresh: `refresh-${name}`, + expiresAt: 1, + }, + createdAt: 1, + }, + home, + ), ), - ]); + ); - const profiles = await store.listProfiles(home); - expect(profiles.map((profile) => profile.name)).toEqual([ - "personal", - "work", - ]); - expect(profiles.find((profile) => profile.name === "personal")).toEqual({ - name: "personal", - tokens: { access: "p", refresh: "pr", expiresAt: 1 }, - createdAt: 1, - }); - expect(profiles.find((profile) => profile.name === "work")).toEqual({ - name: "work", - tokens: { access: "w", refresh: "wr", expiresAt: 2 }, - createdAt: 2, - }); + const failures = results.flatMap((result, index) => + result.status === "rejected" + ? [`${names[index]}: ${String(result.reason)}`] + : [], + ); + expect(failures).toEqual([]); + expect((await store.listProfiles(home)).map((profile) => profile.name)).toEqual( + [...names].sort(), + ); } finally { await rm(home, { recursive: true, force: true }); } @@ -181,6 +180,8 @@ describe("createAuthStore", () => { // Hold the lock until the head of the same-process queue times out; the // queued write must still get its own lock window after we release. + // `second` may already be polling when `first` rejects — release must land + // inside LOCK_TIMEOUT_MS of that handoff. const lockPath = `${store.authPath(home)}.lock`; await writeFile(lockPath, "foreign", { mode: 0o600 }); diff --git a/src/auth/store.ts b/src/auth/store.ts index 70273ff98..903d50a33 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -18,7 +18,8 @@ export type { AuthProfile, BaseTokens }; // for the same provider, so credentials are keyed by a user-chosen profile name // within a single file. Tokens are credentials, so the file is owner-only (0o600) // and the directory 0o700. Writes go through a temp file + rename so a concurrent -// reader never observes a torn file. +// reader never observes a torn file. Same-process writers also queue per auth path +// so each lock wait starts its own deadline. export interface AuthStore { authPath: (home?: string) => string; @@ -48,8 +49,8 @@ interface AuthFile { const LOCK_RETRY_MS = 25; const LOCK_TIMEOUT_MS = 1_000; -// pid alone is not unique per call — concurrent saves in one process must not -// share a temp path or the second rename hits ENOENT after the first moves it. +// 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; // Same-process ops on one auth file queue here so a caller's lock deadline From a38bb46d8e1992952c67a097e7589b3325ef94f1 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 10 Sep 2026 21:56:55 -0700 Subject: [PATCH 4/4] Format the auth-store burst regression test --- src/auth/store.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/auth/store.test.ts b/src/auth/store.test.ts index 8931a0267..f2b980793 100644 --- a/src/auth/store.test.ts +++ b/src/auth/store.test.ts @@ -153,9 +153,9 @@ describe("createAuthStore", () => { : [], ); expect(failures).toEqual([]); - expect((await store.listProfiles(home)).map((profile) => profile.name)).toEqual( - [...names].sort(), - ); + expect( + (await store.listProfiles(home)).map((profile) => profile.name), + ).toEqual([...names].sort()); } finally { await rm(home, { recursive: true, force: true }); }