From 585424edc0d9b6ecc10d6e1eb001f1b9b8e21693 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 23 Oct 2026 20:10:26 +0200 Subject: [PATCH 01/12] fix(auth): serialize session renewal and drop refused sessions Git runs the credential helper several times per operation, often in parallel. Each process that found a lapsed session exchanged the same refresh token; the platform rotates it on use and revokes the whole grant when an old one is replayed. A refused session also stayed in the store, so every later lookup sent the dead token again. - renewal runs under a lock file in ~/.archgate (O_EXCL create, taken over after 30s, renews without it if the wait runs out) and re-reads the session once the lock is held - a refresh token the platform refuses is removed from the store, unless a sign-in has replaced that record meanwhile Signed-off-by: Rhuan Barreto --- src/helpers/credential-store.ts | 109 ++++++---- src/helpers/refresh-lock.ts | 144 +++++++++++++ .../helpers/credential-store-renewal.test.ts | 201 +++++++++++++++--- tests/helpers/refresh-lock.test.ts | 181 ++++++++++++++++ 4 files changed, 564 insertions(+), 71 deletions(-) create mode 100644 src/helpers/refresh-lock.ts create mode 100644 tests/helpers/refresh-lock.test.ts diff --git a/src/helpers/credential-store.ts b/src/helpers/credential-store.ts index 3333e58c..7cadfced 100644 --- a/src/helpers/credential-store.ts +++ b/src/helpers/credential-store.ts @@ -24,14 +24,11 @@ import { type TokenSet, } from "./platform-auth"; import { PLUGINS_HOST } from "./plugin-install"; +import { withRefreshLock } from "./refresh-lock"; import { SessionExpiredError } from "./session-expired-error"; const CREDENTIAL_TIMEOUT_MS = 3_000; -/** How often, and how far apart, a losing renewal re-reads the store. */ -const RENEWAL_REREADS = 4; -const RENEWAL_REREAD_DELAY_MS = 250; - /** * Account the platform session is filed under. * @@ -306,58 +303,80 @@ async function resolveWithStatus(): Promise<{ }> { const { session, timedOut } = await readSession(); if (!session) return { credentials: null, timedOut }; + if (!isExpired(session.tokens.expiresAt)) { + return { credentials: sessionCredentials(session), timedOut: false }; + } + return { credentials: await withRefreshLock(renew), timedOut: false }; +} - const credentials = isExpired(session.tokens.expiresAt) - ? await renew(session) - : { token: session.tokens.accessToken, github_user: session.user }; - return { credentials, timedOut: false }; +function sessionCredentials(session: Session): StoredCredentials { + return { token: session.tokens.accessToken, github_user: session.user }; } /** - * Exchange a refresh token, tolerating a sibling process having rotated it. + * Renew the stored session; runs under the refresh lock. The session is + * re-read because a waiter usually finds it already renewed, and exchanging + * the old token then is a replay that revokes the grant. A refused session is + * removed so later lookups do not resend it. * - * Git invokes the credential helper many times for one operation, so several - * processes can read the same expired session and refresh concurrently. The - * platform rotates the refresh token, which makes every exchange after the - * first fail; re-reading the store recovers the token the winner saved. - * - * @param stale - The expired session as read before renewal. - * @returns Credentials from this renewal, or from whichever process won. - * @throws {UserError} When no usable session exists afterwards. + * @returns Credentials ready to send, or `null` when no session is stored. + * @throws {SessionExpiredError} When the platform refuses the refresh token. */ -async function renew(stale: Session): Promise { +async function renew(): Promise { + const { session } = await readSession(); + if (!session) return null; + if (!isExpired(session.tokens.expiresAt)) { + logDebug("token set was renewed by another process"); + return sessionCredentials(session); + } + + let renewed: TokenSet; try { - const renewed = await platformAuth.refreshAccessToken( - stale.tokens.refreshToken + renewed = await platformAuth.refreshAccessToken( + session.tokens.refreshToken ); - if (!(await saveTokenSet(stale.user, renewed))) { - // The exchange rotated the refresh token, so the stored one is already - // dead: this request can finish, but the session cannot be renewed again. - logWarn( - "Renewed credentials could not be stored.", - "Run `archgate login` to sign in again." - ); - } - return { token: renewed.accessToken, github_user: stale.user }; } catch (error) { - // The winner stores its result only after its own exchange, so a loser - // can be refused before that write lands: re-read a few times first. - /* oxlint-disable no-await-in-loop -- bounded re-reads, in order */ - for (let attempt = 0; attempt < RENEWAL_REREADS; attempt += 1) { - if (attempt > 0) await Bun.sleep(RENEWAL_REREAD_DELAY_MS); - const { session } = await readSession(); - if ( - session && - session.tokens.refreshToken !== stale.tokens.refreshToken && - !isExpired(session.tokens.expiresAt) - ) { - logDebug("token set was renewed by another process", { attempt }); - return { token: session.tokens.accessToken, github_user: session.user }; - } - } - /* oxlint-enable no-await-in-loop */ + if (error instanceof SessionExpiredError) await discardSession(session); throw error; } + + if (!(await saveTokenSet(session.user, renewed))) { + // The exchange rotated the refresh token, so the stored one is already + // dead: this request can finish, but the session cannot be renewed again. + logWarn( + "Renewed credentials could not be stored.", + "Run `archgate login` to sign in again." + ); + } + return { token: renewed.accessToken, github_user: session.user }; +} + +/** + * Remove a session whose refresh token the platform refused. + * + * Only the record holding that very token is removed: `archgate login` does + * not take the refresh lock, so a sign-in finishing meanwhile may already + * have replaced it, and that new session must survive. + */ +async function discardSession(refused: Session): Promise { + const { credentials } = await gitCredentialFill(AUTH_HOST, SESSION_ACCOUNT); + if (!credentials) return; + const stored = SessionSchema.safeParse(safeJsonParse(credentials.password)); + if ( + !stored.success || + stored.data.tokens.refreshToken !== refused.tokens.refreshToken + ) { + return; + } + const removed = await gitCredential( + "reject", + AUTH_HOST, + credentials.username, + credentials.password + ); + logDebug( + removed ? "refused session removed" : "refused session could not be removed" + ); } /** diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts new file mode 100644 index 00000000..0bbef6f7 --- /dev/null +++ b/src/helpers/refresh-lock.ts @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +/** + * Cross-process lock around session renewal. Parallel credential-helper runs + * would otherwise exchange the same refresh token, and the platform revokes + * the whole grant on a replayed one. An `O_EXCL` file is the lock; one older + * than {@link LockTimings.staleMs} is treated as abandoned. + */ + +import { + closeSync, + mkdirSync, + openSync, + statSync, + unlinkSync, + writeSync, +} from "node:fs"; +import { dirname } from "node:path"; + +import { logDebug } from "./log"; +import { internalPath } from "./paths"; + +/** Timings, injectable so tests do not wait in real time. */ +export interface LockTimings { + /** How long a waiter tries before running without the lock. */ + waitMs: number; + /** Delay between attempts while another process holds the lock. */ + pollMs: number; + /** + * Age past which a lock counts as abandoned. It exceeds the longest a + * holder can need: the token request's 15 s timeout plus the store write + * and its read-back. + */ + staleMs: number; +} + +const DEFAULT_TIMINGS: LockTimings = { + waitMs: 40_000, + pollMs: 100, + staleMs: 30_000, +}; + +/** Where the lock lives: next to the rest of the CLI's per-user state. */ +function refreshLockPath(): string { + return internalPath("refresh.lock"); +} + +/** The `code` of a Node file-system error, or `undefined` for anything else. */ +function errorCode(error: unknown): unknown { + return error instanceof Error && "code" in error ? error.code : undefined; +} + +/** + * Try once to create the lock file. + * + * @returns `"acquired"` when this process now holds it, `"held"` when another + * process does, or `"unavailable"` when the file cannot be created at all. + */ +function tryCreate(path: string): "acquired" | "held" | "unavailable" { + try { + const fd = openSync(path, "wx"); + try { + writeSync(fd, String(process.pid)); + } finally { + closeSync(fd); + } + return "acquired"; + } catch (error) { + return errorCode(error) === "EEXIST" ? "held" : "unavailable"; + } +} + +/** Remove a lock older than `staleMs`; true when the create should be retried at once. */ +function removeIfStale(path: string, staleMs: number): boolean { + try { + if (Date.now() - statSync(path).mtimeMs < staleMs) return false; + unlinkSync(path); + logDebug("removed an abandoned refresh lock"); + return true; + } catch (error) { + // Gone already: retry at once. A lock that cannot be removed is waited + // on like a live one, so the deadline still applies. + return errorCode(error) === "ENOENT"; + } +} + +/** Delete the lock this process holds; a missing file is fine. */ +function release(path: string): void { + try { + unlinkSync(path); + } catch { + // Taken over as stale by a waiter; nothing left to release. + } +} + +/** + * Run `task` while holding the refresh lock. + * + * When the lock cannot be had — the directory is not writable, or the holder + * outlasts `waitMs` — `task` runs anyway: a renewal that might race is better + * than failing the git operation that needs the token. + * + * @param task - The work to serialize; it should re-read shared state first. + * @param path - Lock file; defaults to {@link refreshLockPath}. + * @param timings - Overrides for {@link DEFAULT_TIMINGS}. + */ +export async function withRefreshLock( + task: () => Promise, + path: string = refreshLockPath(), + timings: Partial = {} +): Promise { + const { waitMs, pollMs, staleMs } = { ...DEFAULT_TIMINGS, ...timings }; + try { + mkdirSync(dirname(path), { recursive: true }); + } catch { + // tryCreate reports the directory as unavailable below. + } + + const deadline = Date.now() + waitMs; + let held = false; + /* oxlint-disable no-await-in-loop -- polling one lock, attempts in order */ + while (!held) { + const outcome = tryCreate(path); + if (outcome === "acquired") { + held = true; + } else if (outcome === "unavailable") { + logDebug("refresh lock unavailable; renewing without it"); + break; + } else if (!removeIfStale(path, staleMs)) { + if (Date.now() >= deadline) { + logDebug("refresh lock still held; renewing without it"); + break; + } + await Bun.sleep(pollMs); + } + } + /* oxlint-enable no-await-in-loop */ + + try { + return await task(); + } finally { + if (held) release(path); + } +} diff --git a/tests/helpers/credential-store-renewal.test.ts b/tests/helpers/credential-store-renewal.test.ts index d5424f5e..59d2195b 100644 --- a/tests/helpers/credential-store-renewal.test.ts +++ b/tests/helpers/credential-store-renewal.test.ts @@ -1,6 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Archgate -import { describe, expect, spyOn, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; import * as credentialStore from "../../src/helpers/credential-store"; import { @@ -12,6 +12,7 @@ import { import * as logMod from "../../src/helpers/log"; import { platformAuth } from "../../src/helpers/platform-auth"; import type { TokenSet } from "../../src/helpers/platform-auth"; +import * as refreshLock from "../../src/helpers/refresh-lock"; import { SessionExpiredError } from "../../src/helpers/session-expired-error"; import { UserError } from "../../src/helpers/user-error"; import { rejectionMessage } from "../test-utils"; @@ -36,6 +37,33 @@ function sessionRecord( return gitCredentialStub("archgate", JSON.stringify({ user, tokens })); } +// The lock itself is covered in refresh-lock.test.ts; here it only has to +// stay out of the real home directory. +let lockSpy: ReturnType>; +beforeEach(() => { + lockSpy = spyOn(refreshLock, "withRefreshLock").mockImplementation( + async (task) => task() + ); +}); +afterEach(() => { + lockSpy.mockRestore(); +}); + +/** The stdin of every `git credential ` call the spy recorded. */ +async function stdinOf( + spawnSpy: ReturnType>, + action: string +): Promise { + return Promise.all( + spawnSpy.mock.calls + .filter((call) => [...call[0]].includes(action)) + .map(async (call) => { + const stdin = (call[1] as { stdin?: unknown } | undefined)?.stdin; + return stdin instanceof Blob ? stdin.text() : ""; + }) + ); +} + describe("credential renewal", () => { describe("invalidateAccessToken", () => { const tokens = { @@ -251,67 +279,98 @@ describe("clearCredentials with an unresponsive helper", () => { }); }); -// Git runs the helper many times per operation, so processes can race to -// refresh the same expired token set. The platform rotates the refresh -// token, so every exchange after the first fails. -describe("concurrent renewal", () => { +// Git runs the helper several times per operation, often in parallel. The +// platform rotates the refresh token and revokes the grant when an old one is +// replayed, so renewal runs under a lock and re-reads the session inside it. +describe("renewal under the refresh lock", () => { const expired = { accessToken: "ey.stale", refreshToken: "refresh-old", expiresAt: Date.now() - 1_000, }; const rotated = { - accessToken: "ey.fromWinner", + accessToken: "ey.fromHolder", refreshToken: "refresh-rotated", expiresAt: Date.now() + 3_600_000, }; - test("uses the token another process already saved", async () => { - let read = 0; - const fillSpy = spyOn(Bun, "spawn").mockImplementation(() => { - read += 1; - return sessionRecord("octocat", read === 1 ? expired : rotated); - }); + /** A `git credential` call that exits non-zero. */ + function failingCall(): ReturnType { + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + return { + stdout: new Response("").body, + exited: Promise.resolve(1), + kill: () => { + // Nothing to kill: the stub has already settled. + }, + } as unknown as ReturnType; + } + + test("runs the renewal inside the lock", async () => { + const fillSpy = spyOn(Bun, "spawn").mockImplementation(() => + sessionRecord("octocat", expired) + ); const refreshSpy = spyOn( platformAuth, "refreshAccessToken" - ).mockRejectedValue(new SessionExpiredError()); + ).mockResolvedValue(rotated); + const saveSpy = spyOn(credentialStore, "saveTokenSet").mockResolvedValue( + true + ); try { expect(await resolveAccessToken()).toEqual({ - token: "ey.fromWinner", + token: "ey.fromHolder", github_user: "octocat", }); + expect(lockSpy).toHaveBeenCalledTimes(1); } finally { + saveSpy.mockRestore(); refreshSpy.mockRestore(); fillSpy.mockRestore(); } }); - // The winner's write lands after its own exchange, so the loser may have - // to look more than once before the rotated record is there. - test("waits for a winner whose write lands late", async () => { + test("uses the token set the lock holder saved, without exchanging", async () => { let read = 0; const fillSpy = spyOn(Bun, "spawn").mockImplementation(() => { read += 1; - return sessionRecord("octocat", read >= 3 ? rotated : expired); + return sessionRecord("octocat", read === 1 ? expired : rotated); }); - const refreshSpy = spyOn( - platformAuth, - "refreshAccessToken" - ).mockRejectedValue(new SessionExpiredError()); + const refreshSpy = spyOn(platformAuth, "refreshAccessToken"); try { expect(await resolveAccessToken()).toEqual({ - token: "ey.fromWinner", + token: "ey.fromHolder", github_user: "octocat", }); + expect(refreshSpy).not.toHaveBeenCalled(); } finally { refreshSpy.mockRestore(); fillSpy.mockRestore(); } }); - test("rethrows when the stored token set did not change", async () => { - const fillSpy = spyOn(Bun, "spawn").mockImplementation(() => + test("reports signed out when the session is gone once the lock is held", async () => { + let read = 0; + const fillSpy = spyOn(Bun, "spawn").mockImplementation(() => { + read += 1; + return read === 1 + ? sessionRecord("octocat", expired) + : gitCredentialStub("", ""); + }); + const refreshSpy = spyOn(platformAuth, "refreshAccessToken"); + try { + expect(await resolveAccessToken()).toBeNull(); + expect(refreshSpy).not.toHaveBeenCalled(); + } finally { + refreshSpy.mockRestore(); + fillSpy.mockRestore(); + } + }); + + // Keeping a refused session would send the same dead token on every later + // lookup. + test("removes the session whose refresh token was refused", async () => { + const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => sessionRecord("octocat", expired) ); const refreshSpy = spyOn( @@ -322,9 +381,99 @@ describe("concurrent renewal", () => { expect(await rejectionMessage(resolveAccessToken())).toContain( "session has expired" ); + const rejected = await stdinOf(spawnSpy, "reject"); + expect(rejected).toHaveLength(1); + expect(rejected[0]).toContain("host=auth.archgate.dev"); + expect(rejected[0]).toContain("refresh-old"); } finally { refreshSpy.mockRestore(); - fillSpy.mockRestore(); + spawnSpy.mockRestore(); + } + }); + + // `archgate login` does not take the lock, so a sign-in can land between + // the refused exchange and the removal. + test("keeps a session that replaced the refused one", async () => { + let read = 0; + const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => { + read += 1; + return sessionRecord("octocat", read <= 2 ? expired : rotated); + }); + const refreshSpy = spyOn( + platformAuth, + "refreshAccessToken" + ).mockRejectedValue(new SessionExpiredError()); + try { + expect(await rejectionMessage(resolveAccessToken())).toContain( + "session has expired" + ); + expect(await stdinOf(spawnSpy, "reject")).toHaveLength(0); + } finally { + refreshSpy.mockRestore(); + spawnSpy.mockRestore(); + } + }); + + test.each([ + ["no record", () => gitCredentialStub("", "")], + ["an unreadable record", () => gitCredentialStub("archgate", "not-json")], + ])("removes nothing when the store then holds %s", async (_label, later) => { + let read = 0; + const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => { + read += 1; + return read <= 2 ? sessionRecord("octocat", expired) : later(); + }); + const refreshSpy = spyOn( + platformAuth, + "refreshAccessToken" + ).mockRejectedValue(new SessionExpiredError()); + try { + expect(await rejectionMessage(resolveAccessToken())).toContain( + "session has expired" + ); + expect(await stdinOf(spawnSpy, "reject")).toHaveLength(0); + } finally { + refreshSpy.mockRestore(); + spawnSpy.mockRestore(); + } + }); + + test("still reports signed out when the removal fails", async () => { + const spawnSpy = spyOn(Bun, "spawn").mockImplementation((cmd) => + Array.isArray(cmd) && cmd.includes("reject") + ? failingCall() + : sessionRecord("octocat", expired) + ); + const refreshSpy = spyOn( + platformAuth, + "refreshAccessToken" + ).mockRejectedValue(new SessionExpiredError()); + try { + expect(await loadCredentials()).toBeNull(); + expect(await stdinOf(spawnSpy, "reject")).toHaveLength(1); + } finally { + refreshSpy.mockRestore(); + spawnSpy.mockRestore(); + } + }); + + // Only a refused grant ends the session; a service failure must leave it. + test("keeps the session when renewal fails for another reason", async () => { + const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => + sessionRecord("octocat", expired) + ); + const refreshSpy = spyOn( + platformAuth, + "refreshAccessToken" + ).mockRejectedValue(new UserError("Could not reach the sign-in service.")); + try { + expect(await rejectionMessage(resolveAccessToken())).toContain( + "Could not reach" + ); + expect(await stdinOf(spawnSpy, "reject")).toHaveLength(0); + } finally { + refreshSpy.mockRestore(); + spawnSpy.mockRestore(); } }); }); diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts new file mode 100644 index 00000000..1aa520b1 --- /dev/null +++ b/tests/helpers/refresh-lock.test.ts @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import * as fs from "node:fs"; +import { existsSync, mkdtempSync, utimesSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { withRefreshLock } from "../../src/helpers/refresh-lock"; +import { restoreEnv, safeRmSync } from "../test-utils"; + +describe("withRefreshLock", () => { + let tempDir: string; + let lockPath: string; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-refresh-lock-")); + lockPath = join(tempDir, "state", "refresh.lock"); + }); + + afterEach(() => { + safeRmSync(tempDir); + }); + + test("holds the lock while the task runs and removes it afterwards", async () => { + let heldDuringTask = false; + const result = await withRefreshLock(async () => { + heldDuringTask = existsSync(lockPath); + return "done"; + }, lockPath); + + expect(result).toBe("done"); + expect(heldDuringTask).toBe(true); + expect(existsSync(lockPath)).toBe(false); + }); + + test("removes the lock when the task throws", async () => { + const run = withRefreshLock(async () => { + throw new Error("renewal failed"); + }, lockPath); + + expect(run).rejects.toThrow("renewal failed"); + await run.catch(() => { + // Asserted above; awaited so the finally block has run. + }); + expect(existsSync(lockPath)).toBe(false); + }); + + test("runs concurrent tasks one at a time", async () => { + const events: string[] = []; + const task = (name: string) => async () => { + events.push(`${name}:start`); + await Bun.sleep(30); + events.push(`${name}:end`); + }; + + await Promise.all([ + withRefreshLock(task("a"), lockPath, { pollMs: 5 }), + withRefreshLock(task("b"), lockPath, { pollMs: 5 }), + ]); + + expect(events).toHaveLength(4); + expect(events[1]).toBe(events[0]?.replace("start", "end")); + expect(events[3]).toBe(events[2]?.replace("start", "end")); + }); + + test("takes over a lock abandoned by a dead process", async () => { + fs.mkdirSync(join(tempDir, "state")); + writeFileSync(lockPath, "4242"); + const old = new Date(Date.now() - 60_000); + utimesSync(lockPath, old, old); + + let heldDuringTask = ""; + await withRefreshLock( + async () => { + heldDuringTask = fs.readFileSync(lockPath, "utf8"); + }, + lockPath, + { staleMs: 1_000 } + ); + + expect(heldDuringTask).toBe(String(process.pid)); + expect(existsSync(lockPath)).toBe(false); + }); + + // A slow holder must not block the git operation indefinitely. + test("runs without the lock once the wait runs out", async () => { + fs.mkdirSync(join(tempDir, "state")); + writeFileSync(lockPath, "4242"); + + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockPath, + { waitMs: 20, pollMs: 5 } + ); + + expect(ran).toBe(true); + // The other holder's lock is left alone. + expect(fs.readFileSync(lockPath, "utf8")).toBe("4242"); + }); + + test("runs without the lock when the lock directory cannot be created", async () => { + const blocker = join(tempDir, "not-a-dir"); + writeFileSync(blocker, ""); + + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + join(blocker, "refresh.lock") + ); + + expect(ran).toBe(true); + }); + + // A lock that cannot be deleted is waited on like a live one, so the loop + // cannot spin. + test("waits out a stale lock it cannot remove", async () => { + fs.mkdirSync(join(tempDir, "state")); + writeFileSync(lockPath, "4242"); + const unlinkSpy = spyOn(fs, "unlinkSync").mockImplementation(() => { + throw Object.assign(new Error("denied"), { code: "EPERM" }); + }); + try { + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockPath, + { waitMs: 20, pollMs: 5, staleMs: 0 } + ); + expect(ran).toBe(true); + } finally { + unlinkSpy.mockRestore(); + } + expect(existsSync(lockPath)).toBe(true); + }); + + test("retries at once when a stale lock vanishes before its removal", async () => { + fs.mkdirSync(join(tempDir, "state")); + writeFileSync(lockPath, "4242"); + const statSpy = spyOn(fs, "statSync").mockImplementationOnce(() => { + fs.rmSync(lockPath); + throw Object.assign(new Error("gone"), { code: "ENOENT" }); + }); + try { + let heldDuringTask = false; + await withRefreshLock( + async () => { + heldDuringTask = existsSync(lockPath); + }, + lockPath, + { waitMs: 1_000, pollMs: 5 } + ); + expect(heldDuringTask).toBe(true); + } finally { + statSpy.mockRestore(); + } + }); + + // HOME is read by the path helper at call time, so overriding it is the + // supported way to keep the default path inside the sandbox (ARCH-005). + test("defaults to the lock file in the archgate state directory", async () => { + const originalHome = Bun.env.HOME; + Bun.env.HOME = tempDir; + try { + let heldDuringTask = false; + await withRefreshLock(async () => { + heldDuringTask = existsSync(join(tempDir, ".archgate", "refresh.lock")); + }); + expect(heldDuringTask).toBe(true); + } finally { + restoreEnv("HOME", originalHome); + } + }); +}); From 90c0d120b627290cd04989d063532a49738f041a Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 23 Oct 2026 20:15:29 +0200 Subject: [PATCH 02/12] fix(auth): release only the refresh lock this process holds The lock file carries a per-run token. Release and stale takeover check it, so a holder that outlives the stale window cannot delete a waiter's lock, and a lock file whose token could not be written is removed at once. Signed-off-by: Rhuan Barreto --- src/helpers/refresh-lock.ts | 73 +++++++++----- tests/helpers/refresh-lock.test.ts | 147 +++++++++++++++++++++-------- 2 files changed, 161 insertions(+), 59 deletions(-) diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index 0bbef6f7..c0c33bdc 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -11,6 +11,7 @@ import { closeSync, mkdirSync, openSync, + readFileSync, statSync, unlinkSync, writeSync, @@ -50,30 +51,52 @@ function errorCode(error: unknown): unknown { return error instanceof Error && "code" in error ? error.code : undefined; } +/** The holder's token stored in the lock file, or `null` when unreadable. */ +function readToken(path: string): string | null { + try { + return readFileSync(path, "utf8"); + } catch { + return null; + } +} + /** - * Try once to create the lock file. + * Try once to create the lock file, writing `token` into it. * * @returns `"acquired"` when this process now holds it, `"held"` when another * process does, or `"unavailable"` when the file cannot be created at all. */ -function tryCreate(path: string): "acquired" | "held" | "unavailable" { +function tryCreate( + path: string, + token: string +): "acquired" | "held" | "unavailable" { + let fd: number; try { - const fd = openSync(path, "wx"); - try { - writeSync(fd, String(process.pid)); - } finally { - closeSync(fd); - } - return "acquired"; + fd = openSync(path, "wx"); } catch (error) { return errorCode(error) === "EEXIST" ? "held" : "unavailable"; } + try { + writeSync(fd, token); + return "acquired"; + } catch { + // A lock nobody can identify as theirs would stall every waiter until it + // turns stale, so it is removed straight away. + release(path, null); + return "unavailable"; + } finally { + closeSync(fd); + } } /** Remove a lock older than `staleMs`; true when the create should be retried at once. */ function removeIfStale(path: string, staleMs: number): boolean { try { + const token = readToken(path); if (Date.now() - statSync(path).mtimeMs < staleMs) return false; + // Another waiter may have taken the lock over since the read; only the + // abandoned holder's file is removed. + if (readToken(path) !== token) return true; unlinkSync(path); logDebug("removed an abandoned refresh lock"); return true; @@ -84,12 +107,16 @@ function removeIfStale(path: string, staleMs: number): boolean { } } -/** Delete the lock this process holds; a missing file is fine. */ -function release(path: string): void { +/** + * Delete the lock file if it still carries `token` (any file when `null`). + * A waiter that took the lock over as stale owns it now, so it is left alone. + */ +function release(path: string, token: string | null): void { + if (token !== null && readToken(path) !== token) return; try { unlinkSync(path); } catch { - // Taken over as stale by a waiter; nothing left to release. + // Already gone, or held open by another program; it turns stale either way. } } @@ -116,29 +143,31 @@ export async function withRefreshLock( // tryCreate reports the directory as unavailable below. } + const token = `${process.pid}:${crypto.randomUUID()}`; const deadline = Date.now() + waitMs; let held = false; /* oxlint-disable no-await-in-loop -- polling one lock, attempts in order */ - while (!held) { - const outcome = tryCreate(path); + for (;;) { + const outcome = tryCreate(path, token); if (outcome === "acquired") { held = true; - } else if (outcome === "unavailable") { + break; + } + if (outcome === "unavailable") { logDebug("refresh lock unavailable; renewing without it"); break; - } else if (!removeIfStale(path, staleMs)) { - if (Date.now() >= deadline) { - logDebug("refresh lock still held; renewing without it"); - break; - } - await Bun.sleep(pollMs); } + if (Date.now() >= deadline) { + logDebug("refresh lock still held; renewing without it"); + break; + } + if (!removeIfStale(path, staleMs)) await Bun.sleep(pollMs); } /* oxlint-enable no-await-in-loop */ try { return await task(); } finally { - if (held) release(path); + if (held) release(path, token); } } diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index 1aa520b1..a3f6345b 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -2,12 +2,19 @@ // Copyright 2026 Archgate import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; import * as fs from "node:fs"; -import { existsSync, mkdtempSync, utimesSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + utimesSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { withRefreshLock } from "../../src/helpers/refresh-lock"; -import { restoreEnv, safeRmSync } from "../test-utils"; +import { rejectionMessage, restoreEnv, safeRmSync } from "../test-utils"; describe("withRefreshLock", () => { let tempDir: string; @@ -22,27 +29,40 @@ describe("withRefreshLock", () => { safeRmSync(tempDir); }); + /** A lock file left by another process, optionally backdated. */ + function foreignLock(ageMs = 0): void { + mkdirSync(join(tempDir, "state"), { recursive: true }); + writeFileSync(lockPath, "4242:other"); + if (ageMs > 0) { + const then = new Date(Date.now() - ageMs); + utimesSync(lockPath, then, then); + } + } + test("holds the lock while the task runs and removes it afterwards", async () => { - let heldDuringTask = false; const result = await withRefreshLock(async () => { - heldDuringTask = existsSync(lockPath); + expect(existsSync(lockPath)).toBe(true); return "done"; }, lockPath); expect(result).toBe("done"); - expect(heldDuringTask).toBe(true); expect(existsSync(lockPath)).toBe(false); }); - test("removes the lock when the task throws", async () => { - const run = withRefreshLock(async () => { - throw new Error("renewal failed"); + test("writes a token naming this process", async () => { + await withRefreshLock(async () => { + expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); }, lockPath); + }); - expect(run).rejects.toThrow("renewal failed"); - await run.catch(() => { - // Asserted above; awaited so the finally block has run. - }); + test("removes the lock when the task throws", async () => { + expect( + await rejectionMessage( + withRefreshLock(async () => { + throw new Error("renewal failed"); + }, lockPath) + ) + ).toBe("renewal failed"); expect(existsSync(lockPath)).toBe(false); }); @@ -59,34 +79,50 @@ describe("withRefreshLock", () => { withRefreshLock(task("b"), lockPath, { pollMs: 5 }), ]); - expect(events).toHaveLength(4); - expect(events[1]).toBe(events[0]?.replace("start", "end")); - expect(events[3]).toBe(events[2]?.replace("start", "end")); + const [first] = events; + const second = first === "a:start" ? "b" : "a"; + expect(events).toEqual([ + first, + first.replace("start", "end"), + `${second}:start`, + `${second}:end`, + ]); }); test("takes over a lock abandoned by a dead process", async () => { - fs.mkdirSync(join(tempDir, "state")); - writeFileSync(lockPath, "4242"); - const old = new Date(Date.now() - 60_000); - utimesSync(lockPath, old, old); + foreignLock(60_000); - let heldDuringTask = ""; await withRefreshLock( async () => { - heldDuringTask = fs.readFileSync(lockPath, "utf8"); + expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); }, lockPath, { staleMs: 1_000 } ); - expect(heldDuringTask).toBe(String(process.pid)); + expect(existsSync(lockPath)).toBe(false); + }); + + // A holder that outlives the stale window must not delete a waiter's lock. + test("leaves a lock that carries another process's token", async () => { + await withRefreshLock(async () => { + writeFileSync(lockPath, "4242:taker"); + }, lockPath); + + expect(readFileSync(lockPath, "utf8")).toBe("4242:taker"); + }); + + test("finishes cleanly when its lock was removed under it", async () => { + await withRefreshLock(async () => { + fs.rmSync(lockPath); + }, lockPath); + expect(existsSync(lockPath)).toBe(false); }); // A slow holder must not block the git operation indefinitely. test("runs without the lock once the wait runs out", async () => { - fs.mkdirSync(join(tempDir, "state")); - writeFileSync(lockPath, "4242"); + foreignLock(); let ran = false; await withRefreshLock( @@ -98,8 +134,7 @@ describe("withRefreshLock", () => { ); expect(ran).toBe(true); - // The other holder's lock is left alone. - expect(fs.readFileSync(lockPath, "utf8")).toBe("4242"); + expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); }); test("runs without the lock when the lock directory cannot be created", async () => { @@ -117,11 +152,27 @@ describe("withRefreshLock", () => { expect(ran).toBe(true); }); + // A lock nobody can identify would stall every waiter until it turned stale. + test("removes a lock file it created but could not write", async () => { + const writeSpy = spyOn(fs, "writeSync").mockImplementationOnce(() => { + throw Object.assign(new Error("disk full"), { code: "ENOSPC" }); + }); + try { + let ran = false; + await withRefreshLock(async () => { + ran = true; + expect(existsSync(lockPath)).toBe(false); + }, lockPath); + expect(ran).toBe(true); + } finally { + writeSpy.mockRestore(); + } + }); + // A lock that cannot be deleted is waited on like a live one, so the loop // cannot spin. test("waits out a stale lock it cannot remove", async () => { - fs.mkdirSync(join(tempDir, "state")); - writeFileSync(lockPath, "4242"); + foreignLock(); const unlinkSpy = spyOn(fs, "unlinkSync").mockImplementation(() => { throw Object.assign(new Error("denied"), { code: "EPERM" }); }); @@ -138,42 +189,64 @@ describe("withRefreshLock", () => { } finally { unlinkSpy.mockRestore(); } - expect(existsSync(lockPath)).toBe(true); + expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); }); test("retries at once when a stale lock vanishes before its removal", async () => { - fs.mkdirSync(join(tempDir, "state")); - writeFileSync(lockPath, "4242"); + foreignLock(); const statSpy = spyOn(fs, "statSync").mockImplementationOnce(() => { fs.rmSync(lockPath); throw Object.assign(new Error("gone"), { code: "ENOENT" }); }); try { - let heldDuringTask = false; await withRefreshLock( async () => { - heldDuringTask = existsSync(lockPath); + expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); }, lockPath, { waitMs: 1_000, pollMs: 5 } ); - expect(heldDuringTask).toBe(true); } finally { statSpy.mockRestore(); } }); + // Two waiters can find the same stale lock; the second must not delete the + // lock the first has just created. + test("leaves a stale lock that changed hands while it looked", async () => { + foreignLock(60_000); + const original = fs.readFileSync; + let reads = 0; + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses + const readSpy = spyOn(fs, "readFileSync").mockImplementation((( + ...args: Parameters + ) => { + reads += 1; + // The second read of the first stale check sees a new holder. + return reads === 2 ? "9999:newer" : original(...args); + }) as typeof fs.readFileSync); + try { + await withRefreshLock(async () => {}, lockPath, { + staleMs: 1_000, + pollMs: 5, + }); + expect(reads).toBeGreaterThan(2); + } finally { + readSpy.mockRestore(); + } + }); + // HOME is read by the path helper at call time, so overriding it is the // supported way to keep the default path inside the sandbox (ARCH-005). test("defaults to the lock file in the archgate state directory", async () => { const originalHome = Bun.env.HOME; Bun.env.HOME = tempDir; try { - let heldDuringTask = false; await withRefreshLock(async () => { - heldDuringTask = existsSync(join(tempDir, ".archgate", "refresh.lock")); + expect(existsSync(join(tempDir, ".archgate", "refresh.lock"))).toBe( + true + ); }); - expect(heldDuringTask).toBe(true); } finally { restoreEnv("HOME", originalHome); } From 69cb66e833bb0133c9fc7e82c69842fcab45b3b8 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 00:02:40 +0200 Subject: [PATCH 03/12] fix(auth): guard lock release and reclaim, re-read before discarding Release and stale-lock removal run under a short-lived guard file, so neither can unlink a lock that replaced the one it checked, and an unreadable token never counts as a match. A refused renewal re-reads the store briefly before discarding the session, in case a process that renewed without the lock saved a newer one. Signed-off-by: Rhuan Barreto --- src/helpers/credential-store.ts | 34 ++++++- src/helpers/refresh-lock.ts | 92 +++++++++++++----- .../helpers/credential-store-renewal.test.ts | 9 +- tests/helpers/refresh-lock.test.ts | 97 ++++++++++++++----- 4 files changed, 181 insertions(+), 51 deletions(-) diff --git a/src/helpers/credential-store.ts b/src/helpers/credential-store.ts index 7cadfced..c0f3e52a 100644 --- a/src/helpers/credential-store.ts +++ b/src/helpers/credential-store.ts @@ -29,6 +29,10 @@ import { SessionExpiredError } from "./session-expired-error"; const CREDENTIAL_TIMEOUT_MS = 3_000; +/** How often, and how far apart, a refused renewal re-reads the store. */ +const RENEWAL_REREADS = 4; +const RENEWAL_REREAD_DELAY_MS = 250; + /** * Account the platform session is filed under. * @@ -336,7 +340,10 @@ async function renew(): Promise { session.tokens.refreshToken ); } catch (error) { - if (error instanceof SessionExpiredError) await discardSession(session); + if (!(error instanceof SessionExpiredError)) throw error; + const renewed = await sessionRenewedElsewhere(session); + if (renewed) return sessionCredentials(renewed); + await discardSession(session); throw error; } @@ -351,6 +358,31 @@ async function renew(): Promise { return { token: renewed.accessToken, github_user: session.user }; } +/** + * Wait briefly for a session another process saved after this one's exchange + * was refused. That happens when the refresh lock could not be had and two + * processes renewed at once; the winner writes only after its own exchange. + */ +async function sessionRenewedElsewhere( + refused: Session +): Promise { + /* oxlint-disable no-await-in-loop -- bounded re-reads, in order */ + for (let attempt = 0; attempt < RENEWAL_REREADS; attempt += 1) { + if (attempt > 0) await Bun.sleep(RENEWAL_REREAD_DELAY_MS); + const { session } = await readSession(); + if ( + session && + session.tokens.refreshToken !== refused.tokens.refreshToken && + !isExpired(session.tokens.expiresAt) + ) { + logDebug("token set was renewed by another process", { attempt }); + return session; + } + } + /* oxlint-enable no-await-in-loop */ + return null; +} + /** * Remove a session whose refresh token the platform refused. * diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index c0c33bdc..5985e14b 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -51,12 +51,48 @@ function errorCode(error: unknown): unknown { return error instanceof Error && "code" in error ? error.code : undefined; } -/** The holder's token stored in the lock file, or `null` when unreadable. */ -function readToken(path: string): string | null { +/** + * How long a guard file may exist before it counts as abandoned. Its holder + * only reads, stats and unlinks one file with no `await` in between. + */ +const GUARD_STALE_MS = 5_000; + +/** Attempts, and the pause between them, to take a busy guard on release. */ +const RELEASE_GUARD_TRIES = 20; +const RELEASE_GUARD_PAUSE_MS = 5; + +/** + * Run `action` while holding the guard file next to the lock. Release and + * reclaim check the lock and then unlink it; the guard keeps either from + * removing a lock that replaced the one it checked. Acquiring needs no guard, + * since `wx` cannot create the lock while the checked file still exists. + * + * @returns `false` when the guard is held elsewhere and `action` did not run. + */ +function withGuard(path: string, action: () => void): boolean { + const guard = `${path}.guard`; + let fd: number; try { - return readFileSync(path, "utf8"); + fd = openSync(guard, "wx"); } catch { - return null; + try { + if (Date.now() - statSync(guard).mtimeMs < GUARD_STALE_MS) return false; + unlinkSync(guard); + fd = openSync(guard, "wx"); + } catch { + return false; + } + } + closeSync(fd); + try { + action(); + return true; + } finally { + try { + unlinkSync(guard); + } catch { + // Removed as abandoned by another process; nothing left to release. + } } } @@ -91,33 +127,41 @@ function tryCreate( /** Remove a lock older than `staleMs`; true when the create should be retried at once. */ function removeIfStale(path: string, staleMs: number): boolean { - try { - const token = readToken(path); - if (Date.now() - statSync(path).mtimeMs < staleMs) return false; - // Another waiter may have taken the lock over since the read; only the - // abandoned holder's file is removed. - if (readToken(path) !== token) return true; - unlinkSync(path); - logDebug("removed an abandoned refresh lock"); - return true; - } catch (error) { - // Gone already: retry at once. A lock that cannot be removed is waited - // on like a live one, so the deadline still applies. - return errorCode(error) === "ENOENT"; - } + let retry = false; + const ran = withGuard(path, () => { + try { + if (Date.now() - statSync(path).mtimeMs < staleMs) return; + unlinkSync(path); + logDebug("removed an abandoned refresh lock"); + retry = true; + } catch (error) { + // Gone already: retry at once. A lock that cannot be checked or removed + // is waited on like a live one, so the deadline still applies. + retry = errorCode(error) === "ENOENT"; + } + }); + return ran && retry; } /** * Delete the lock file if it still carries `token` (any file when `null`). - * A waiter that took the lock over as stale owns it now, so it is left alone. + * A lock another process now holds, or one whose token cannot be read, is + * left alone; an unreleased lock turns stale. */ function release(path: string, token: string | null): void { - if (token !== null && readToken(path) !== token) return; - try { - unlinkSync(path); - } catch { - // Already gone, or held open by another program; it turns stale either way. + const unlinkIfOurs = () => { + try { + if (token !== null && readFileSync(path, "utf8") !== token) return; + unlinkSync(path); + } catch { + // Already gone, unreadable or held open by another program. + } + }; + for (let attempt = 0; attempt < RELEASE_GUARD_TRIES; attempt += 1) { + if (withGuard(path, unlinkIfOurs)) return; + Bun.sleepSync(RELEASE_GUARD_PAUSE_MS); } + logDebug("refresh lock guard stayed busy; leaving the lock to turn stale"); } /** diff --git a/tests/helpers/credential-store-renewal.test.ts b/tests/helpers/credential-store-renewal.test.ts index 59d2195b..1efe0f28 100644 --- a/tests/helpers/credential-store-renewal.test.ts +++ b/tests/helpers/credential-store-renewal.test.ts @@ -393,7 +393,7 @@ describe("renewal under the refresh lock", () => { // `archgate login` does not take the lock, so a sign-in can land between // the refused exchange and the removal. - test("keeps a session that replaced the refused one", async () => { + test("uses a session saved elsewhere after the refusal", async () => { let read = 0; const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => { read += 1; @@ -404,9 +404,10 @@ describe("renewal under the refresh lock", () => { "refreshAccessToken" ).mockRejectedValue(new SessionExpiredError()); try { - expect(await rejectionMessage(resolveAccessToken())).toContain( - "session has expired" - ); + expect(await resolveAccessToken()).toEqual({ + token: "ey.fromHolder", + github_user: "octocat", + }); expect(await stdinOf(spawnSpy, "reject")).toHaveLength(0); } finally { refreshSpy.mockRestore(); diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index a3f6345b..bd9835c0 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -211,29 +211,82 @@ describe("withRefreshLock", () => { } }); - // Two waiters can find the same stale lock; the second must not delete the - // lock the first has just created. - test("leaves a stale lock that changed hands while it looked", async () => { - foreignLock(60_000); - const original = fs.readFileSync; - let reads = 0; - // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses - const readSpy = spyOn(fs, "readFileSync").mockImplementation((( - ...args: Parameters - ) => { - reads += 1; - // The second read of the first stale check sees a new holder. - return reads === 2 ? "9999:newer" : original(...args); - }) as typeof fs.readFileSync); - try { - await withRefreshLock(async () => {}, lockPath, { - staleMs: 1_000, - pollMs: 5, - }); - expect(reads).toBeGreaterThan(2); - } finally { - readSpy.mockRestore(); + /** A guard file left by another process, optionally backdated. */ + function foreignGuard(ageMs = 0): void { + mkdirSync(join(tempDir, "state"), { recursive: true }); + const guardPath = `${lockPath}.guard`; + writeFileSync(guardPath, ""); + if (ageMs > 0) { + const then = new Date(Date.now() - ageMs); + utimesSync(guardPath, then, then); } + } + + // Another process is mid-check on the same stale lock, so this one waits + // instead of removing a file whose ownership may be changing. + test("does not reclaim a stale lock while another process holds the guard", async () => { + foreignLock(60_000); + foreignGuard(); + + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockPath, + { staleMs: 1_000, waitMs: 20, pollMs: 5 } + ); + + expect(ran).toBe(true); + expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); + }); + + test("takes over a guard abandoned by a dead process", async () => { + foreignLock(60_000); + foreignGuard(60_000); + + await withRefreshLock( + async () => { + expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); + }, + lockPath, + { staleMs: 1_000, pollMs: 5 } + ); + + expect(existsSync(lockPath)).toBe(false); + expect(existsSync(`${lockPath}.guard`)).toBe(false); + }); + + test("waits when an abandoned guard cannot be removed", async () => { + foreignLock(60_000); + // A directory in the guard's place: `wx` reports it as existing, and it + // cannot be unlinked as a file. + const guardPath = `${lockPath}.guard`; + mkdirSync(guardPath); + const then = new Date(Date.now() - 60_000); + utimesSync(guardPath, then, then); + + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockPath, + { staleMs: 1_000, waitMs: 20, pollMs: 5 } + ); + + expect(ran).toBe(true); + expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); + }); + + // A release that never gets the guard leaves its lock to turn stale rather + // than unlinking without the check. + test("leaves its lock in place when the guard stays busy", async () => { + await withRefreshLock(async () => { + foreignGuard(); + }, lockPath); + + expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); }); // HOME is read by the path helper at call time, so overriding it is the From 0b52e5d0a86b32aca28934c9925c92679ada22b5 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 00:08:47 +0200 Subject: [PATCH 04/12] fix(deps): update zod to 4.6.6 Resolves GHSA-mrhx-c7xc-m3w5, which fails the dependency audit in CI. Signed-off-by: Rhuan Barreto --- bun.lock | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bun.lock b/bun.lock index 49c1f8a9..70024558 100644 --- a/bun.lock +++ b/bun.lock @@ -23,7 +23,7 @@ "oxlint-tsgolint": "7.0.2001", "posthog-node": "5.53.0", "typescript": "7.0.2", - "zod": "4.6.5", + "zod": "4.6.6", }, }, }, @@ -584,7 +584,7 @@ "yargs-parser": ["yargs-parser@22.0.0", "", {}, "sha512-rwu/ClNdSMpkSrUb+d6BRsSkLUq1fmfsY6TOpYzTwvwkg1/NRG85KBy3kq++A8LKQwX6lsu+aWad+2khvuXrqw=="], - "zod": ["zod@4.6.5", "", {}, "sha512-v5l/aFXZQeai4awLbOpSoHecE9UiMrnfx75tEXLjNonXVARxQ5mOeipTjROUchszUNCqnE+hqAMujRsRHsut2Q=="], + "zod": ["zod@4.6.6", "", {}, "sha512-4UmzQUF5fxvfcYHlbjBmejU+w+hbDnWFx4aS8kBx7nPBHcO2SqPbpAG8snbdg+Xse4pRefR98hgjFgIMCI/UhA=="], "@inquirer/checkbox/@inquirer/type": ["@inquirer/type@4.2.0", "", { "peerDependencies": { "@types/node": ">=18" }, "optionalPeers": ["@types/node"] }, "sha512-2rnHX8RPo4glJYqnFc/v3nQeIhDcjyRRyaT7MGcfL/8xv/8K03SXYoA2tX06WVSKywWOE/ncbG3dtuzLjR5TCQ=="], diff --git a/package.json b/package.json index ea4195cb..7f8e73c0 100644 --- a/package.json +++ b/package.json @@ -84,7 +84,7 @@ "oxlint-tsgolint": "7.0.2001", "posthog-node": "5.53.0", "typescript": "7.0.2", - "zod": "4.6.5" + "zod": "4.6.6" }, "readme": "README.md" } From 1f43a21eac14d19875e55fdf31696ca64b3a5479 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 00:08:48 +0200 Subject: [PATCH 05/12] fix(auth): make guard takeover and cleanup safe against replacement Each guard carries a token and is removed only while it still holds it. An abandoned guard is reclaimed by renaming it to a unique name; if the renamed file turns out to be fresh, another reclaimer replaced the stale one in between, so it is linked back and this process yields. Signed-off-by: Rhuan Barreto --- src/helpers/refresh-lock.ts | 86 +++++++++++++---- tests/helpers/refresh-lock.test.ts | 142 ++++++++++++++++++++++++++--- 2 files changed, 195 insertions(+), 33 deletions(-) diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index 5985e14b..8cb91ef8 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -9,9 +9,11 @@ import { closeSync, + linkSync, mkdirSync, openSync, readFileSync, + renameSync, statSync, unlinkSync, writeSync, @@ -61,6 +63,66 @@ const GUARD_STALE_MS = 5_000; const RELEASE_GUARD_TRIES = 20; const RELEASE_GUARD_PAUSE_MS = 5; +/** Create the guard carrying `token`; false when it exists or cannot be written. */ +function createGuard(guard: string, token: string): boolean { + let fd: number; + try { + fd = openSync(guard, "wx"); + } catch { + return false; + } + try { + writeSync(fd, token); + return true; + } catch { + closeSync(fd); + fd = -1; + try { + unlinkSync(guard); + } catch { + // Left for the stale check to reclaim. + } + return false; + } finally { + if (fd !== -1) closeSync(fd); + } +} + +/** + * Remove an abandoned guard; true when it is gone. The guard is renamed first, + * which only one process can do to a given file. If the renamed file turns + * out to be fresh, another reclaimer replaced the stale one in between, so it + * is linked back (`link` fails if a guard exists again) and this call yields. + */ +function reclaimGuard(guard: string): boolean { + try { + if (Date.now() - statSync(guard).mtimeMs < GUARD_STALE_MS) return false; + const moved = `${guard}.${crypto.randomUUID()}`; + renameSync(guard, moved); + const fresh = Date.now() - statSync(moved).mtimeMs < GUARD_STALE_MS; + if (fresh) { + try { + linkSync(moved, guard); + } catch { + // A newer guard already stands; the one moved here is lost. + } + } + unlinkSync(moved); + return !fresh; + } catch { + return false; + } +} + +/** Delete the guard only while it still carries `token`. */ +function dropGuard(guard: string, token: string): void { + try { + if (readFileSync(guard, "utf8") === token) unlinkSync(guard); + } catch { + // Gone already; nothing left to release. + } +} + /** * Run `action` while holding the guard file next to the lock. Release and * reclaim check the lock and then unlink it; the guard keeps either from @@ -71,28 +133,16 @@ const RELEASE_GUARD_PAUSE_MS = 5; */ function withGuard(path: string, action: () => void): boolean { const guard = `${path}.guard`; - let fd: number; - try { - fd = openSync(guard, "wx"); - } catch { - try { - if (Date.now() - statSync(guard).mtimeMs < GUARD_STALE_MS) return false; - unlinkSync(guard); - fd = openSync(guard, "wx"); - } catch { - return false; - } - } - closeSync(fd); + const token = crypto.randomUUID(); + const held = + createGuard(guard, token) || + (reclaimGuard(guard) && createGuard(guard, token)); + if (!held) return false; try { action(); return true; } finally { - try { - unlinkSync(guard); - } catch { - // Removed as abandoned by another process; nothing left to release. - } + dropGuard(guard, token); } } diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index bd9835c0..47a93b95 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -259,26 +259,138 @@ describe("withRefreshLock", () => { test("waits when an abandoned guard cannot be removed", async () => { foreignLock(60_000); - // A directory in the guard's place: `wx` reports it as existing, and it - // cannot be unlinked as a file. - const guardPath = `${lockPath}.guard`; - mkdirSync(guardPath); - const then = new Date(Date.now() - 60_000); - utimesSync(guardPath, then, then); + foreignGuard(60_000); + const renameSpy = spyOn(fs, "renameSync").mockImplementation(() => { + throw Object.assign(new Error("denied"), { code: "EPERM" }); + }); + try { + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockPath, + { staleMs: 1_000, waitMs: 20, pollMs: 5 } + ); + expect(ran).toBe(true); + } finally { + renameSpy.mockRestore(); + } + expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); + }); - let ran = false; - await withRefreshLock( - async () => { - ran = true; - }, - lockPath, - { staleMs: 1_000, waitMs: 20, pollMs: 5 } - ); + // Two processes can both find the same guard abandoned. The one that + // reclaims second must not remove the guard the first has just created. + test("leaves a guard another reclaimer created after the stale check", async () => { + foreignLock(60_000); + foreignGuard(60_000); + const guardPath = `${lockPath}.guard`; + const original = fs.statSync; + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses + const statSpy = spyOn(fs, "statSync").mockImplementationOnce((( + ...args: Parameters + ) => { + // This process sees the abandoned guard; another reclaims it and + // creates its own before this one acts. + const seen = original(...args); + fs.rmSync(guardPath); + writeFileSync(guardPath, "other-reclaimer"); + return seen; + }) as typeof fs.statSync); + try { + await withRefreshLock(async () => {}, lockPath, { + staleMs: 1_000, + waitMs: 20, + pollMs: 5, + }); + } finally { + statSpy.mockRestore(); + } - expect(ran).toBe(true); + expect(readFileSync(guardPath, "utf8")).toBe("other-reclaimer"); expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); }); + test("gives way when a fresh guard cannot be put back", async () => { + foreignLock(60_000); + foreignGuard(60_000); + const guardPath = `${lockPath}.guard`; + const original = fs.statSync; + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses + const statSpy = spyOn(fs, "statSync").mockImplementationOnce((( + ...args: Parameters + ) => { + const seen = original(...args); + fs.rmSync(guardPath); + writeFileSync(guardPath, "other-reclaimer"); + return seen; + }) as typeof fs.statSync); + const linkSpy = spyOn(fs, "linkSync").mockImplementationOnce(() => { + throw Object.assign(new Error("exists"), { code: "EEXIST" }); + }); + try { + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockPath, + { staleMs: 1_000, waitMs: 20, pollMs: 5 } + ); + expect(ran).toBe(true); + } finally { + linkSpy.mockRestore(); + statSpy.mockRestore(); + } + }); + + test("removes a guard file it created but could not write", async () => { + let writes = 0; + const original = fs.writeSync; + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses + const writeSpy = spyOn(fs, "writeSync").mockImplementation((( + ...args: Parameters + ) => { + writes += 1; + // The first write is the lock's token; the second is the release guard's. + if (writes === 2) { + throw Object.assign(new Error("disk full"), { code: "ENOSPC" }); + } + return original(...args); + }) as typeof fs.writeSync); + try { + await withRefreshLock(async () => {}, lockPath); + } finally { + writeSpy.mockRestore(); + } + + expect(writes).toBeGreaterThan(2); + expect(existsSync(lockPath)).toBe(false); + expect(existsSync(`${lockPath}.guard`)).toBe(false); + }); + + test("keeps a guard whose token changed while it was held", async () => { + const guardPath = `${lockPath}.guard`; + const original = fs.readFileSync; + let reads = 0; + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses + const readSpy = spyOn(fs, "readFileSync").mockImplementation((( + ...args: Parameters + ) => { + reads += 1; + // The release reads the lock's token first; the guard is replaced then. + if (reads === 1) writeFileSync(guardPath, "replacement"); + return original(...args); + }) as typeof fs.readFileSync); + try { + await withRefreshLock(async () => {}, lockPath); + } finally { + readSpy.mockRestore(); + } + + expect(readFileSync(guardPath, "utf8")).toBe("replacement"); + }); + // A release that never gets the guard leaves its lock to turn stale rather // than unlinking without the check. test("leaves its lock in place when the guard stays busy", async () => { From 28530a1bb1e5d602fd7fe6117f42a375810712de Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 00:15:25 +0200 Subject: [PATCH 06/12] fix(auth): queue renewals as unique entries instead of one lock file Each renewal files ~/.archgate/refresh-lock/-; the oldest live entry holds the lock. Names never repeat, so removing an abandoned entry or releasing one's own can never remove another holder's, and no takeover ever exposes a held path. A caller decides only once the clock has passed its own timestamp, so an entry filed in the same millisecond is already listed. Waiters refresh their entry while polling, so only entries whose process stopped count as abandoned. Signed-off-by: Rhuan Barreto --- src/helpers/refresh-lock.ts | 244 +++++------------ tests/helpers/refresh-lock.test.ts | 409 ++++++++++------------------- 2 files changed, 209 insertions(+), 444 deletions(-) diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index 8cb91ef8..b603da57 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -3,22 +3,19 @@ /** * Cross-process lock around session renewal. Parallel credential-helper runs * would otherwise exchange the same refresh token, and the platform revokes - * the whole grant on a replayed one. An `O_EXCL` file is the lock; one older - * than {@link LockTimings.staleMs} is treated as abandoned. + * the whole grant on a replayed one. Each caller files a uniquely named entry + * in a directory, and the oldest live entry holds the lock. */ import { - closeSync, - linkSync, mkdirSync, - openSync, - readFileSync, - renameSync, + readdirSync, statSync, unlinkSync, - writeSync, + utimesSync, + writeFileSync, } from "node:fs"; -import { dirname } from "node:path"; +import { join } from "node:path"; import { logDebug } from "./log"; import { internalPath } from "./paths"; @@ -30,7 +27,7 @@ export interface LockTimings { /** Delay between attempts while another process holds the lock. */ pollMs: number; /** - * Age past which a lock counts as abandoned. It exceeds the longest a + * Age past which an entry counts as abandoned. It exceeds the longest a * holder can need: the token request's 15 s timeout plus the store write * and its read-back. */ @@ -43,175 +40,70 @@ const DEFAULT_TIMINGS: LockTimings = { staleMs: 30_000, }; -/** Where the lock lives: next to the rest of the CLI's per-user state. */ -function refreshLockPath(): string { - return internalPath("refresh.lock"); -} +/** Width of the zero-padded millisecond timestamp that starts each entry name. */ +const STAMP_WIDTH = 15; -/** The `code` of a Node file-system error, or `undefined` for anything else. */ -function errorCode(error: unknown): unknown { - return error instanceof Error && "code" in error ? error.code : undefined; -} +/** Entry names: a sortable creation time, then a UUID so no name repeats. */ +const ENTRY_PATTERN = /^\d{15}-[0-9a-f-]{36}$/u; -/** - * How long a guard file may exist before it counts as abandoned. Its holder - * only reads, stats and unlinks one file with no `await` in between. - */ -const GUARD_STALE_MS = 5_000; - -/** Attempts, and the pause between them, to take a busy guard on release. */ -const RELEASE_GUARD_TRIES = 20; -const RELEASE_GUARD_PAUSE_MS = 5; - -/** Create the guard carrying `token`; false when it exists or cannot be written. */ -function createGuard(guard: string, token: string): boolean { - let fd: number; - try { - fd = openSync(guard, "wx"); - } catch { - return false; - } - try { - writeSync(fd, token); - return true; - } catch { - closeSync(fd); - fd = -1; - try { - unlinkSync(guard); - } catch { - // Left for the stale check to reclaim. - } - return false; - } finally { - if (fd !== -1) closeSync(fd); - } +/** Where the entries live: next to the rest of the CLI's per-user state. */ +function refreshLockDir(): string { + return internalPath("refresh-lock"); } -/** - * Remove an abandoned guard; true when it is gone. The guard is renamed first, - * which only one process can do to a given file. If the renamed file turns - * out to be fresh, another reclaimer replaced the stale one in between, so it - * is linked back (`link` fails if a guard exists again) and this call yields. - */ -function reclaimGuard(guard: string): boolean { +/** Delete one entry; a missing file is fine. */ +function removeEntry(dir: string, name: string): void { try { - if (Date.now() - statSync(guard).mtimeMs < GUARD_STALE_MS) return false; - const moved = `${guard}.${crypto.randomUUID()}`; - renameSync(guard, moved); - const fresh = Date.now() - statSync(moved).mtimeMs < GUARD_STALE_MS; - if (fresh) { - try { - linkSync(moved, guard); - } catch { - // A newer guard already stands; the one moved here is lost. - } - } - unlinkSync(moved); - return !fresh; + unlinkSync(join(dir, name)); } catch { - return false; - } -} - -/** Delete the guard only while it still carries `token`. */ -function dropGuard(guard: string, token: string): void { - try { - if (readFileSync(guard, "utf8") === token) unlinkSync(guard); - } catch { - // Gone already; nothing left to release. + // Already gone. } } /** - * Run `action` while holding the guard file next to the lock. Release and - * reclaim check the lock and then unlink it; the guard keeps either from - * removing a lock that replaced the one it checked. Acquiring needs no guard, - * since `wx` cannot create the lock while the checked file still exists. - * - * @returns `false` when the guard is held elsewhere and `action` did not run. + * Mark a waiting entry as alive, so the age check only ever catches entries + * whose process stopped polling. False when the entry is gone. */ -function withGuard(path: string, action: () => void): boolean { - const guard = `${path}.guard`; - const token = crypto.randomUUID(); - const held = - createGuard(guard, token) || - (reclaimGuard(guard) && createGuard(guard, token)); - if (!held) return false; +function touchEntry(dir: string, name: string): boolean { + const now = new Date(); try { - action(); + utimesSync(join(dir, name), now, now); return true; - } finally { - dropGuard(guard, token); + } catch { + return false; } } /** - * Try once to create the lock file, writing `token` into it. + * Whether `own` is the oldest live entry. Entries older than `staleMs` are + * removed on the way: names never repeat, so deleting one can only ever + * remove the abandoned entry itself. * - * @returns `"acquired"` when this process now holds it, `"held"` when another - * process does, or `"unavailable"` when the file cannot be created at all. + * @returns `null` when the directory cannot be read. */ -function tryCreate( - path: string, - token: string -): "acquired" | "held" | "unavailable" { - let fd: number; - try { - fd = openSync(path, "wx"); - } catch (error) { - return errorCode(error) === "EEXIST" ? "held" : "unavailable"; - } +function holdsLock(dir: string, own: string, staleMs: number): boolean | null { + let names: string[]; try { - writeSync(fd, token); - return "acquired"; + names = readdirSync(dir).filter((name) => ENTRY_PATTERN.test(name)); } catch { - // A lock nobody can identify as theirs would stall every waiter until it - // turns stale, so it is removed straight away. - release(path, null); - return "unavailable"; - } finally { - closeSync(fd); + return null; } -} - -/** Remove a lock older than `staleMs`; true when the create should be retried at once. */ -function removeIfStale(path: string, staleMs: number): boolean { - let retry = false; - const ran = withGuard(path, () => { + const live: string[] = []; + for (const name of names) { + let age: number; try { - if (Date.now() - statSync(path).mtimeMs < staleMs) return; - unlinkSync(path); - logDebug("removed an abandoned refresh lock"); - retry = true; - } catch (error) { - // Gone already: retry at once. A lock that cannot be checked or removed - // is waited on like a live one, so the deadline still applies. - retry = errorCode(error) === "ENOENT"; - } - }); - return ran && retry; -} - -/** - * Delete the lock file if it still carries `token` (any file when `null`). - * A lock another process now holds, or one whose token cannot be read, is - * left alone; an unreleased lock turns stale. - */ -function release(path: string, token: string | null): void { - const unlinkIfOurs = () => { - try { - if (token !== null && readFileSync(path, "utf8") !== token) return; - unlinkSync(path); + age = Date.now() - statSync(join(dir, name)).mtimeMs; } catch { - // Already gone, unreadable or held open by another program. + continue; + } + if (name !== own && age >= staleMs) { + removeEntry(dir, name); + logDebug("removed an abandoned refresh lock entry"); + } else { + live.push(name); } - }; - for (let attempt = 0; attempt < RELEASE_GUARD_TRIES; attempt += 1) { - if (withGuard(path, unlinkIfOurs)) return; - Bun.sleepSync(RELEASE_GUARD_PAUSE_MS); } - logDebug("refresh lock guard stayed busy; leaving the lock to turn stale"); + return live.toSorted()[0] === own; } /** @@ -222,46 +114,50 @@ function release(path: string, token: string | null): void { * than failing the git operation that needs the token. * * @param task - The work to serialize; it should re-read shared state first. - * @param path - Lock file; defaults to {@link refreshLockPath}. + * @param dir - Entry directory; defaults to {@link refreshLockDir}. * @param timings - Overrides for {@link DEFAULT_TIMINGS}. */ export async function withRefreshLock( task: () => Promise, - path: string = refreshLockPath(), + dir: string = refreshLockDir(), timings: Partial = {} ): Promise { const { waitMs, pollMs, staleMs } = { ...DEFAULT_TIMINGS, ...timings }; + const createdAt = Date.now(); + const own = `${String(createdAt).padStart(STAMP_WIDTH, "0")}-${crypto.randomUUID()}`; + + let filed = true; try { - mkdirSync(dirname(path), { recursive: true }); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, own), String(process.pid), { flag: "wx" }); } catch { - // tryCreate reports the directory as unavailable below. + filed = false; + logDebug("refresh lock unavailable; renewing without it"); } - const token = `${process.pid}:${crypto.randomUUID()}`; + // A caller filed later in the same millisecond can sort first, so the + // decision waits until the clock has moved on: anyone filed after that + // sorts after this entry, and anyone filed before is already listed. + /* oxlint-disable no-await-in-loop -- one wait, then polling in order */ + while (filed && Date.now() <= createdAt) await Bun.sleep(1); const deadline = Date.now() + waitMs; - let held = false; - /* oxlint-disable no-await-in-loop -- polling one lock, attempts in order */ - for (;;) { - const outcome = tryCreate(path, token); - if (outcome === "acquired") { - held = true; - break; - } - if (outcome === "unavailable") { - logDebug("refresh lock unavailable; renewing without it"); - break; - } - if (Date.now() >= deadline) { - logDebug("refresh lock still held; renewing without it"); + while (filed) { + const holds = holdsLock(dir, own, staleMs); + if (holds === true) break; + if (holds === null || Date.now() >= deadline || !touchEntry(dir, own)) { + // Withdrawn so that waiters behind it do not wait on it. + removeEntry(dir, own); + filed = false; + logDebug("refresh lock not acquired; renewing without it"); break; } - if (!removeIfStale(path, staleMs)) await Bun.sleep(pollMs); + await Bun.sleep(pollMs); } /* oxlint-enable no-await-in-loop */ try { return await task(); } finally { - if (held) release(path, token); + if (filed) removeEntry(dir, own); } } diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index 47a93b95..27aad476 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -6,6 +6,7 @@ import { existsSync, mkdirSync, mkdtempSync, + readdirSync, readFileSync, utimesSync, writeFileSync, @@ -16,317 +17,213 @@ import { join } from "node:path"; import { withRefreshLock } from "../../src/helpers/refresh-lock"; import { rejectionMessage, restoreEnv, safeRmSync } from "../test-utils"; +/** An entry name for `stamp`, with a UUID that sorts first or last. */ +function entryName(stamp: number, uuid: "low" | "high"): string { + const id = + uuid === "low" + ? "00000000-0000-4000-8000-000000000000" + : "ffffffff-ffff-4fff-bfff-ffffffffffff"; + return `${String(stamp).padStart(15, "0")}-${id}`; +} + describe("withRefreshLock", () => { let tempDir: string; - let lockPath: string; + let lockDir: string; beforeEach(() => { tempDir = mkdtempSync(join(tmpdir(), "archgate-refresh-lock-")); - lockPath = join(tempDir, "state", "refresh.lock"); + lockDir = join(tempDir, "refresh-lock"); }); afterEach(() => { safeRmSync(tempDir); }); - /** A lock file left by another process, optionally backdated. */ - function foreignLock(ageMs = 0): void { - mkdirSync(join(tempDir, "state"), { recursive: true }); - writeFileSync(lockPath, "4242:other"); + /** An entry filed by another process, optionally backdated. */ + function foreignEntry(name: string, ageMs = 0): string { + mkdirSync(lockDir, { recursive: true }); + const path = join(lockDir, name); + writeFileSync(path, "4242"); if (ageMs > 0) { const then = new Date(Date.now() - ageMs); - utimesSync(lockPath, then, then); + utimesSync(path, then, then); } + return path; } - test("holds the lock while the task runs and removes it afterwards", async () => { + test("files an entry while the task runs and removes it afterwards", async () => { const result = await withRefreshLock(async () => { - expect(existsSync(lockPath)).toBe(true); + const names = readdirSync(lockDir); + expect(names).toHaveLength(1); + expect(readFileSync(join(lockDir, names[0] ?? ""), "utf8")).toBe( + String(process.pid) + ); return "done"; - }, lockPath); + }, lockDir); expect(result).toBe("done"); - expect(existsSync(lockPath)).toBe(false); + expect(readdirSync(lockDir)).toEqual([]); }); - test("writes a token naming this process", async () => { - await withRefreshLock(async () => { - expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); - }, lockPath); - }); - - test("removes the lock when the task throws", async () => { + test("removes its entry when the task throws", async () => { expect( await rejectionMessage( withRefreshLock(async () => { throw new Error("renewal failed"); - }, lockPath) + }, lockDir) ) ).toBe("renewal failed"); - expect(existsSync(lockPath)).toBe(false); + expect(readdirSync(lockDir)).toEqual([]); }); - test("runs concurrent tasks one at a time", async () => { - const events: string[] = []; - const task = (name: string) => async () => { - events.push(`${name}:start`); - await Bun.sleep(30); - events.push(`${name}:end`); + // The property the lock exists for: renewals never overlap, even when + // several start at once and an abandoned entry has to be cleared first. + test("never runs two tasks at once", async () => { + foreignEntry(entryName(1, "low"), 60_000); + let active = 0; + let peak = 0; + let finished = 0; + const task = async () => { + active += 1; + peak = Math.max(peak, active); + await Bun.sleep(10); + active -= 1; + finished += 1; }; - await Promise.all([ - withRefreshLock(task("a"), lockPath, { pollMs: 5 }), - withRefreshLock(task("b"), lockPath, { pollMs: 5 }), - ]); + await Promise.all( + Array.from({ length: 6 }, async () => + withRefreshLock(task, lockDir, { pollMs: 2, staleMs: 1_000 }) + ) + ); - const [first] = events; - const second = first === "a:start" ? "b" : "a"; - expect(events).toEqual([ - first, - first.replace("start", "end"), - `${second}:start`, - `${second}:end`, - ]); + expect(finished).toBe(6); + expect(peak).toBe(1); + expect(readdirSync(lockDir)).toEqual([]); }); - test("takes over a lock abandoned by a dead process", async () => { - foreignLock(60_000); + test("removes an entry abandoned by a dead process", async () => { + const abandoned = foreignEntry(entryName(1, "low"), 60_000); await withRefreshLock( async () => { - expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); + expect(existsSync(abandoned)).toBe(false); }, - lockPath, + lockDir, { staleMs: 1_000 } ); - - expect(existsSync(lockPath)).toBe(false); - }); - - // A holder that outlives the stale window must not delete a waiter's lock. - test("leaves a lock that carries another process's token", async () => { - await withRefreshLock(async () => { - writeFileSync(lockPath, "4242:taker"); - }, lockPath); - - expect(readFileSync(lockPath, "utf8")).toBe("4242:taker"); - }); - - test("finishes cleanly when its lock was removed under it", async () => { - await withRefreshLock(async () => { - fs.rmSync(lockPath); - }, lockPath); - - expect(existsSync(lockPath)).toBe(false); }); // A slow holder must not block the git operation indefinitely. - test("runs without the lock once the wait runs out", async () => { - foreignLock(); + test("runs without the lock once the wait for a live holder runs out", async () => { + const holder = foreignEntry(entryName(1, "low")); let ran = false; await withRefreshLock( async () => { ran = true; + // Withdrawn before running, so it does not hold up later waiters. + expect(readdirSync(lockDir)).toEqual([entryName(1, "low")]); }, - lockPath, + lockDir, { waitMs: 20, pollMs: 5 } ); expect(ran).toBe(true); - expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); + expect(existsSync(holder)).toBe(true); }); - test("runs without the lock when the lock directory cannot be created", async () => { - const blocker = join(tempDir, "not-a-dir"); - writeFileSync(blocker, ""); + test("is not held up by an entry filed after its own", async () => { + foreignEntry(entryName(Date.now() + 60_000, "low")); - let ran = false; + let entries = 0; await withRefreshLock( async () => { - ran = true; + entries = readdirSync(lockDir).length; }, - join(blocker, "refresh.lock") + lockDir, + { waitMs: 1_000 } ); - expect(ran).toBe(true); + expect(entries).toBe(2); }); - // A lock nobody can identify would stall every waiter until it turned stale. - test("removes a lock file it created but could not write", async () => { - const writeSpy = spyOn(fs, "writeSync").mockImplementationOnce(() => { - throw Object.assign(new Error("disk full"), { code: "ENOSPC" }); - }); - try { - let ran = false; - await withRefreshLock(async () => { - ran = true; - expect(existsSync(lockPath)).toBe(false); - }, lockPath); - expect(ran).toBe(true); - } finally { - writeSpy.mockRestore(); - } - }); - - // A lock that cannot be deleted is waited on like a live one, so the loop - // cannot spin. - test("waits out a stale lock it cannot remove", async () => { - foreignLock(); - const unlinkSpy = spyOn(fs, "unlinkSync").mockImplementation(() => { - throw Object.assign(new Error("denied"), { code: "EPERM" }); + // Two callers can file in the same millisecond, and the later one can sort + // first. The decision waits for the clock to move on, so the earlier one + // sees the later entry and yields instead of running alongside it. + test("yields to an entry filed in the same millisecond that sorts first", async () => { + const realNow = Date.now.bind(Date); + const stamp = realNow(); + let calls = 0; + const nowSpy = spyOn(Date, "now").mockImplementation(() => { + calls += 1; + if (calls === 1) { + foreignEntry(entryName(stamp, "low")); + return stamp; + } + return calls === 2 ? stamp : realNow(); }); try { - let ran = false; await withRefreshLock( async () => { - ran = true; + expect(readdirSync(lockDir)).toEqual([entryName(stamp, "low")]); }, - lockPath, - { waitMs: 20, pollMs: 5, staleMs: 0 } + lockDir, + { waitMs: 20, pollMs: 5 } ); - expect(ran).toBe(true); } finally { - unlinkSpy.mockRestore(); + nowSpy.mockRestore(); } - expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); }); - test("retries at once when a stale lock vanishes before its removal", async () => { - foreignLock(); - const statSpy = spyOn(fs, "statSync").mockImplementationOnce(() => { - fs.rmSync(lockPath); - throw Object.assign(new Error("gone"), { code: "ENOENT" }); - }); - try { - await withRefreshLock( - async () => { - expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); - }, - lockPath, - { waitMs: 1_000, pollMs: 5 } - ); - } finally { - statSpy.mockRestore(); - } - }); + test("ignores files that are not entries", async () => { + mkdirSync(lockDir, { recursive: true }); + writeFileSync(join(lockDir, "notes.txt"), ""); - /** A guard file left by another process, optionally backdated. */ - function foreignGuard(ageMs = 0): void { - mkdirSync(join(tempDir, "state"), { recursive: true }); - const guardPath = `${lockPath}.guard`; - writeFileSync(guardPath, ""); - if (ageMs > 0) { - const then = new Date(Date.now() - ageMs); - utimesSync(guardPath, then, then); - } - } + let entries: string[] = []; + await withRefreshLock(async () => { + entries = readdirSync(lockDir); + }, lockDir); - // Another process is mid-check on the same stale lock, so this one waits - // instead of removing a file whose ownership may be changing. - test("does not reclaim a stale lock while another process holds the guard", async () => { - foreignLock(60_000); - foreignGuard(); + expect(entries).toHaveLength(2); + expect(readdirSync(lockDir)).toEqual(["notes.txt"]); + }); + + test("runs without the lock when the directory cannot be created", async () => { + const blocker = join(tempDir, "not-a-dir"); + writeFileSync(blocker, ""); let ran = false; await withRefreshLock( async () => { ran = true; }, - lockPath, - { staleMs: 1_000, waitMs: 20, pollMs: 5 } + join(blocker, "refresh-lock") ); expect(ran).toBe(true); - expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); - }); - - test("takes over a guard abandoned by a dead process", async () => { - foreignLock(60_000); - foreignGuard(60_000); - - await withRefreshLock( - async () => { - expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); - }, - lockPath, - { staleMs: 1_000, pollMs: 5 } - ); - - expect(existsSync(lockPath)).toBe(false); - expect(existsSync(`${lockPath}.guard`)).toBe(false); }); - test("waits when an abandoned guard cannot be removed", async () => { - foreignLock(60_000); - foreignGuard(60_000); - const renameSpy = spyOn(fs, "renameSync").mockImplementation(() => { - throw Object.assign(new Error("denied"), { code: "EPERM" }); + test("withdraws and runs without the lock when the directory cannot be read", async () => { + const readSpy = spyOn(fs, "readdirSync").mockImplementationOnce(() => { + throw Object.assign(new Error("denied"), { code: "EACCES" }); }); try { let ran = false; - await withRefreshLock( - async () => { - ran = true; - }, - lockPath, - { staleMs: 1_000, waitMs: 20, pollMs: 5 } - ); + await withRefreshLock(async () => { + ran = true; + }, lockDir); expect(ran).toBe(true); } finally { - renameSpy.mockRestore(); - } - expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); - }); - - // Two processes can both find the same guard abandoned. The one that - // reclaims second must not remove the guard the first has just created. - test("leaves a guard another reclaimer created after the stale check", async () => { - foreignLock(60_000); - foreignGuard(60_000); - const guardPath = `${lockPath}.guard`; - const original = fs.statSync; - // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses - const statSpy = spyOn(fs, "statSync").mockImplementationOnce((( - ...args: Parameters - ) => { - // This process sees the abandoned guard; another reclaims it and - // creates its own before this one acts. - const seen = original(...args); - fs.rmSync(guardPath); - writeFileSync(guardPath, "other-reclaimer"); - return seen; - }) as typeof fs.statSync); - try { - await withRefreshLock(async () => {}, lockPath, { - staleMs: 1_000, - waitMs: 20, - pollMs: 5, - }); - } finally { - statSpy.mockRestore(); + readSpy.mockRestore(); } - - expect(readFileSync(guardPath, "utf8")).toBe("other-reclaimer"); - expect(readFileSync(lockPath, "utf8")).toBe("4242:other"); + expect(readdirSync(lockDir)).toEqual([]); }); - test("gives way when a fresh guard cannot be put back", async () => { - foreignLock(60_000); - foreignGuard(60_000); - const guardPath = `${lockPath}.guard`; - const original = fs.statSync; - // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses - const statSpy = spyOn(fs, "statSync").mockImplementationOnce((( - ...args: Parameters - ) => { - const seen = original(...args); - fs.rmSync(guardPath); - writeFileSync(guardPath, "other-reclaimer"); - return seen; - }) as typeof fs.statSync); - const linkSpy = spyOn(fs, "linkSync").mockImplementationOnce(() => { - throw Object.assign(new Error("exists"), { code: "EEXIST" }); + test("skips an entry that vanishes while it is checked", async () => { + foreignEntry(entryName(1, "low")); + const statSpy = spyOn(fs, "statSync").mockImplementationOnce(() => { + throw Object.assign(new Error("gone"), { code: "ENOENT" }); }); try { let ran = false; @@ -334,83 +231,55 @@ describe("withRefreshLock", () => { async () => { ran = true; }, - lockPath, - { staleMs: 1_000, waitMs: 20, pollMs: 5 } + lockDir, + { waitMs: 1_000, pollMs: 5 } ); expect(ran).toBe(true); } finally { - linkSpy.mockRestore(); statSpy.mockRestore(); } }); - test("removes a guard file it created but could not write", async () => { - let writes = 0; - const original = fs.writeSync; + // A waiter without an entry is not in line, so it stops waiting rather + // than polling for a turn that cannot come. + test("stops waiting when its own entry disappears", async () => { + const holderName = entryName(1, "low"); + foreignEntry(holderName); + const original = fs.readdirSync; // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses - const writeSpy = spyOn(fs, "writeSync").mockImplementation((( - ...args: Parameters + const readSpy = spyOn(fs, "readdirSync").mockImplementationOnce((( + ...args: Parameters ) => { - writes += 1; - // The first write is the lock's token; the second is the release guard's. - if (writes === 2) { - throw Object.assign(new Error("disk full"), { code: "ENOSPC" }); + for (const name of original(lockDir)) { + if (name !== holderName) fs.rmSync(join(lockDir, name)); } return original(...args); - }) as typeof fs.writeSync); + }) as typeof fs.readdirSync); try { - await withRefreshLock(async () => {}, lockPath); - } finally { - writeSpy.mockRestore(); - } - - expect(writes).toBeGreaterThan(2); - expect(existsSync(lockPath)).toBe(false); - expect(existsSync(`${lockPath}.guard`)).toBe(false); - }); - - test("keeps a guard whose token changed while it was held", async () => { - const guardPath = `${lockPath}.guard`; - const original = fs.readFileSync; - let reads = 0; - // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses - const readSpy = spyOn(fs, "readFileSync").mockImplementation((( - ...args: Parameters - ) => { - reads += 1; - // The release reads the lock's token first; the guard is replaced then. - if (reads === 1) writeFileSync(guardPath, "replacement"); - return original(...args); - }) as typeof fs.readFileSync); - try { - await withRefreshLock(async () => {}, lockPath); + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockDir, + { waitMs: 60_000, pollMs: 5 } + ); + expect(ran).toBe(true); } finally { readSpy.mockRestore(); } - - expect(readFileSync(guardPath, "utf8")).toBe("replacement"); - }); - - // A release that never gets the guard leaves its lock to turn stale rather - // than unlinking without the check. - test("leaves its lock in place when the guard stays busy", async () => { - await withRefreshLock(async () => { - foreignGuard(); - }, lockPath); - - expect(readFileSync(lockPath, "utf8")).toStartWith(`${process.pid}:`); }); // HOME is read by the path helper at call time, so overriding it is the // supported way to keep the default path inside the sandbox (ARCH-005). - test("defaults to the lock file in the archgate state directory", async () => { + test("defaults to the archgate state directory", async () => { const originalHome = Bun.env.HOME; Bun.env.HOME = tempDir; try { await withRefreshLock(async () => { - expect(existsSync(join(tempDir, ".archgate", "refresh.lock"))).toBe( - true - ); + expect( + readdirSync(join(tempDir, ".archgate", "refresh-lock")) + ).toHaveLength(1); }); } finally { restoreEnv("HOME", originalHome); From ea86aaed59068db7b9a350b82002f1060b4630c4 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 00:19:50 +0200 Subject: [PATCH 07/12] fix(auth): mark a renewal before it takes its timestamp A caller creates pending- before reading the clock and removes it once its entry is filed, and nobody holds the lock while a live marker exists. A caller paused between its timestamp and its entry is therefore always visible, and a caller that marks itself after a decision takes a strictly later timestamp. Signed-off-by: Rhuan Barreto --- src/helpers/refresh-lock.ts | 68 +++++++++++++++++------- tests/helpers/refresh-lock.test.ts | 85 ++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 19 deletions(-) diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index b603da57..ec3622c2 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -4,7 +4,8 @@ * Cross-process lock around session renewal. Parallel credential-helper runs * would otherwise exchange the same refresh token, and the platform revokes * the whole grant on a replayed one. Each caller files a uniquely named entry - * in a directory, and the oldest live entry holds the lock. + * in a directory, and the oldest live entry holds the lock once no caller is + * still choosing its place (the bakery algorithm). */ import { @@ -46,6 +47,10 @@ const STAMP_WIDTH = 15; /** Entry names: a sortable creation time, then a UUID so no name repeats. */ const ENTRY_PATTERN = /^\d{15}-[0-9a-f-]{36}$/u; +/** Marker a caller holds while it picks its timestamp and files its entry. */ +const MARKER_PREFIX = "pending-"; +const MARKER_PATTERN = /^pending-[0-9a-f-]{36}$/u; + /** Where the entries live: next to the rest of the CLI's per-user state. */ function refreshLockDir(): string { return internalPath("refresh-lock"); @@ -75,16 +80,46 @@ function touchEntry(dir: string, name: string): boolean { } /** - * Whether `own` is the oldest live entry. Entries older than `staleMs` are - * removed on the way: names never repeat, so deleting one can only ever - * remove the abandoned entry itself. + * File this caller's entry. The marker exists before the timestamp is taken, + * so a caller that has read its timestamp but not yet filed is never + * invisible: anyone deciding meanwhile sees the marker and waits. + * + * @returns The entry name and its timestamp, or `null` when nothing could be filed. + */ +function fileEntry(dir: string): { own: string; createdAt: number } | null { + const id = crypto.randomUUID(); + const marker = `${MARKER_PREFIX}${id}`; + try { + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, marker), String(process.pid), { flag: "wx" }); + } catch { + return null; + } + try { + const createdAt = Date.now(); + const own = `${String(createdAt).padStart(STAMP_WIDTH, "0")}-${id}`; + writeFileSync(join(dir, own), String(process.pid), { flag: "wx" }); + return { own, createdAt }; + } catch { + return null; + } finally { + removeEntry(dir, marker); + } +} + +/** + * Whether `own` is the oldest live entry and no other caller is still filing. + * Entries and markers older than `staleMs` are removed on the way: names never + * repeat, so deleting one can only ever remove the abandoned file itself. * * @returns `null` when the directory cannot be read. */ function holdsLock(dir: string, own: string, staleMs: number): boolean | null { let names: string[]; try { - names = readdirSync(dir).filter((name) => ENTRY_PATTERN.test(name)); + names = readdirSync(dir).filter( + (name) => ENTRY_PATTERN.test(name) || MARKER_PATTERN.test(name) + ); } catch { return null; } @@ -103,6 +138,7 @@ function holdsLock(dir: string, own: string, staleMs: number): boolean | null { live.push(name); } } + if (live.some((name) => MARKER_PATTERN.test(name))) return false; return live.toSorted()[0] === own; } @@ -123,21 +159,15 @@ export async function withRefreshLock( timings: Partial = {} ): Promise { const { waitMs, pollMs, staleMs } = { ...DEFAULT_TIMINGS, ...timings }; - const createdAt = Date.now(); - const own = `${String(createdAt).padStart(STAMP_WIDTH, "0")}-${crypto.randomUUID()}`; - - let filed = true; - try { - mkdirSync(dir, { recursive: true }); - writeFileSync(join(dir, own), String(process.pid), { flag: "wx" }); - } catch { - filed = false; - logDebug("refresh lock unavailable; renewing without it"); - } + const entry = fileEntry(dir); + let filed = entry !== null; + if (!filed) logDebug("refresh lock unavailable; renewing without it"); + const own = entry?.own ?? ""; + const createdAt = entry?.createdAt ?? 0; - // A caller filed later in the same millisecond can sort first, so the - // decision waits until the clock has moved on: anyone filed after that - // sorts after this entry, and anyone filed before is already listed. + // Deciding only after the clock passes this entry's timestamp makes every + // caller who marks itself later take a strictly later timestamp, so it + // sorts after this entry; anyone who marked itself earlier is listed. /* oxlint-disable no-await-in-loop -- one wait, then polling in order */ while (filed && Date.now() <= createdAt) await Bun.sleep(1); const deadline = Date.now() + waitMs; diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index 27aad476..baef8d4a 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -176,6 +176,91 @@ describe("withRefreshLock", () => { } }); + // Process A has marked itself and read its timestamp, but its entry is not + // filed yet. B must not start: once A files, A sorts first. A runs, then + // finishes, and only then does B's task start. + test("waits for a caller that marked itself but has not filed yet", async () => { + const stampA = Date.now() - 5; + const markerA = join( + lockDir, + "pending-00000000-0000-4000-8000-00000000000a" + ); + const entryA = join(lockDir, entryName(stampA, "low")); + mkdirSync(lockDir, { recursive: true }); + writeFileSync(markerA, "4242"); + + let aRunning = false; + let overlapped = false; + let bRan = false; + const b = withRefreshLock( + async () => { + bRan = true; + if (aRunning) overlapped = true; + }, + lockDir, + { pollMs: 2, waitMs: 5_000 } + ); + + // A resumes: it files its older entry, drops its marker and runs. + await Bun.sleep(30); + expect(bRan).toBe(false); + writeFileSync(entryA, "4242"); + fs.rmSync(markerA); + aRunning = true; + await Bun.sleep(30); + expect(bRan).toBe(false); + aRunning = false; + fs.rmSync(entryA); + + await b; + expect(bRan).toBe(true); + expect(overlapped).toBe(false); + }); + + test("removes a marker abandoned by a dead process", async () => { + mkdirSync(lockDir, { recursive: true }); + const marker = join( + lockDir, + "pending-00000000-0000-4000-8000-00000000000b" + ); + writeFileSync(marker, "4242"); + const then = new Date(Date.now() - 60_000); + utimesSync(marker, then, then); + + await withRefreshLock( + async () => { + expect(existsSync(marker)).toBe(false); + }, + lockDir, + { staleMs: 1_000 } + ); + }); + + test("runs without the lock when its entry cannot be filed", async () => { + let writes = 0; + const original = fs.writeFileSync; + const writeSpy = spyOn(fs, "writeFileSync").mockImplementation( + (...args: Parameters) => { + writes += 1; + // The first write is the marker; the second is the entry. + if (writes === 2) { + throw Object.assign(new Error("disk full"), { code: "ENOSPC" }); + } + original(...args); + } + ); + try { + let ran = false; + await withRefreshLock(async () => { + ran = true; + }, lockDir); + expect(ran).toBe(true); + } finally { + writeSpy.mockRestore(); + } + expect(readdirSync(lockDir)).toEqual([]); + }); + test("ignores files that are not entries", async () => { mkdirSync(lockDir, { recursive: true }); writeFileSync(join(lockDir, "notes.txt"), ""); From 896a34640fc3bbd39add27dec4bc4e1e9d39f2be Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 00:23:27 +0200 Subject: [PATCH 08/12] fix(auth): rescan when a listed marker vanishes A marker that disappears between the directory listing and its age check usually means its entry was just filed after the listing was taken. The lock check now answers not-yet and the next poll rescans, instead of deciding from the stale listing. Signed-off-by: Rhuan Barreto --- src/helpers/refresh-lock.ts | 3 ++ tests/helpers/refresh-lock.test.ts | 46 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index ec3622c2..a11b07c6 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -129,6 +129,9 @@ function holdsLock(dir: string, own: string, staleMs: number): boolean | null { try { age = Date.now() - statSync(join(dir, name)).mtimeMs; } catch { + // A vanished marker usually means its entry was just filed, after this + // listing was taken; only a fresh listing can show it. + if (MARKER_PATTERN.test(name)) return false; continue; } if (name !== own && age >= staleMs) { diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index baef8d4a..2b5ef90e 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -217,6 +217,52 @@ describe("withRefreshLock", () => { expect(overlapped).toBe(false); }); + // B lists the directory while A still has only its marker. A then files its + // older entry and drops the marker before B checks it. B must rescan rather + // than decide from the listing that predates A's entry. + test("rescans when a listed marker turns into an entry", async () => { + const markerName = "pending-00000000-0000-4000-8000-00000000000c"; + const markerA = join(lockDir, markerName); + const entryA = join(lockDir, entryName(Date.now() - 5, "low")); + mkdirSync(lockDir, { recursive: true }); + writeFileSync(markerA, "4242"); + + let aRunning = false; + let overlapped = false; + let bRan = false; + const original = fs.readdirSync; + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses + const readSpy = spyOn(fs, "readdirSync").mockImplementationOnce((( + ...args: Parameters + ) => { + const listing = original(...args); + // A files its entry, drops its marker and runs for a while. + writeFileSync(entryA, "4242"); + fs.rmSync(markerA); + aRunning = true; + setTimeout(() => { + aRunning = false; + fs.rmSync(entryA); + }, 40); + return listing; + }) as typeof fs.readdirSync); + try { + await withRefreshLock( + async () => { + bRan = true; + if (aRunning) overlapped = true; + }, + lockDir, + { pollMs: 2, waitMs: 5_000 } + ); + } finally { + readSpy.mockRestore(); + } + + expect(bRan).toBe(true); + expect(overlapped).toBe(false); + }); + test("removes a marker abandoned by a dead process", async () => { mkdirSync(lockDir, { recursive: true }); const marker = join( From ff6a4f36f2df4e3fab65dc44882abeab87f30db2 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 01:03:35 +0200 Subject: [PATCH 09/12] fix(auth): keep lock errors visible and carry the helper timeout removeEntry ignores only ENOENT and logs any other failure, and an abandoned entry that cannot be deleted keeps counting as a holder. touchEntry withdraws only when the entry is gone; other failures are logged, and a waiter whose refreshes keep failing withdraws after half of staleMs, before others can reap it. renew reports a helper timeout on its re-read, so loadCredentials skips the legacy lookup. Signed-off-by: Rhuan Barreto --- src/helpers/credential-store.ts | 25 +++-- src/helpers/refresh-lock.ts | 53 +++++++--- .../helpers/credential-store-renewal.test.ts | 25 +++++ tests/helpers/refresh-lock.test.ts | 100 +++++++++++++++++- 4 files changed, 178 insertions(+), 25 deletions(-) diff --git a/src/helpers/credential-store.ts b/src/helpers/credential-store.ts index c0f3e52a..f685fe3f 100644 --- a/src/helpers/credential-store.ts +++ b/src/helpers/credential-store.ts @@ -310,7 +310,7 @@ async function resolveWithStatus(): Promise<{ if (!isExpired(session.tokens.expiresAt)) { return { credentials: sessionCredentials(session), timedOut: false }; } - return { credentials: await withRefreshLock(renew), timedOut: false }; + return withRefreshLock(renew); } function sessionCredentials(session: Session): StoredCredentials { @@ -323,15 +323,19 @@ function sessionCredentials(session: Session): StoredCredentials { * the old token then is a replay that revokes the grant. A refused session is * removed so later lookups do not resend it. * - * @returns Credentials ready to send, or `null` when no session is stored. + * @returns Credentials ready to send, or `null` when no session is stored, + * and whether the store's helper timed out on the read. * @throws {SessionExpiredError} When the platform refuses the refresh token. */ -async function renew(): Promise { - const { session } = await readSession(); - if (!session) return null; +async function renew(): Promise<{ + credentials: StoredCredentials | null; + timedOut: boolean; +}> { + const { session, timedOut } = await readSession(); + if (!session) return { credentials: null, timedOut }; if (!isExpired(session.tokens.expiresAt)) { logDebug("token set was renewed by another process"); - return sessionCredentials(session); + return { credentials: sessionCredentials(session), timedOut: false }; } let renewed: TokenSet; @@ -342,7 +346,9 @@ async function renew(): Promise { } catch (error) { if (!(error instanceof SessionExpiredError)) throw error; const renewed = await sessionRenewedElsewhere(session); - if (renewed) return sessionCredentials(renewed); + if (renewed) { + return { credentials: sessionCredentials(renewed), timedOut: false }; + } await discardSession(session); throw error; } @@ -355,7 +361,10 @@ async function renew(): Promise { "Run `archgate login` to sign in again." ); } - return { token: renewed.accessToken, github_user: session.user }; + return { + credentials: { token: renewed.accessToken, github_user: session.user }, + timedOut: false, + }; } /** diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index a11b07c6..a5e3f9f8 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -56,26 +56,44 @@ function refreshLockDir(): string { return internalPath("refresh-lock"); } -/** Delete one entry; a missing file is fine. */ -function removeEntry(dir: string, name: string): void { +/** The `code` of a Node file-system error, or `undefined` for anything else. */ +function errorCode(error: unknown): unknown { + return error instanceof Error && "code" in error ? error.code : undefined; +} + +/** Delete one entry; true when it is gone, including when it already was. */ +function removeEntry(dir: string, name: string): boolean { try { unlinkSync(join(dir, name)); - } catch { - // Already gone. + return true; + } catch (error) { + if (errorCode(error) === "ENOENT") return true; + logDebug("could not remove a refresh lock entry", { + name, + code: errorCode(error), + }); + return false; } } /** - * Mark a waiting entry as alive, so the age check only ever catches entries - * whose process stopped polling. False when the entry is gone. + * Mark a waiting entry as alive, so the age check only catches entries of + * processes that are not polling. + * + * @returns `"touched"`, `"gone"` when the entry does not exist, or + * `"failed"` when it exists but its time cannot be updated. */ -function touchEntry(dir: string, name: string): boolean { +function touchEntry(dir: string, name: string): "touched" | "gone" | "failed" { const now = new Date(); try { utimesSync(join(dir, name), now, now); - return true; - } catch { - return false; + return "touched"; + } catch (error) { + if (errorCode(error) === "ENOENT") return "gone"; + logDebug("could not refresh a refresh lock entry", { + code: errorCode(error), + }); + return "failed"; } } @@ -134,8 +152,7 @@ function holdsLock(dir: string, own: string, staleMs: number): boolean | null { if (MARKER_PATTERN.test(name)) return false; continue; } - if (name !== own && age >= staleMs) { - removeEntry(dir, name); + if (name !== own && age >= staleMs && removeEntry(dir, name)) { logDebug("removed an abandoned refresh lock entry"); } else { live.push(name); @@ -174,10 +191,20 @@ export async function withRefreshLock( /* oxlint-disable no-await-in-loop -- one wait, then polling in order */ while (filed && Date.now() <= createdAt) await Bun.sleep(1); const deadline = Date.now() + waitMs; + // Others reap an entry untouched for `staleMs`, so a waiter whose refreshes + // keep failing withdraws halfway there, before it can be mistaken for dead. + let lastAlive = Date.now(); while (filed) { const holds = holdsLock(dir, own, staleMs); if (holds === true) break; - if (holds === null || Date.now() >= deadline || !touchEntry(dir, own)) { + const touch = holds === null ? "gone" : touchEntry(dir, own); + if (touch === "touched") lastAlive = Date.now(); + const withdraw = + holds === null || + touch === "gone" || + Date.now() >= deadline || + Date.now() - lastAlive >= staleMs / 2; + if (withdraw) { // Withdrawn so that waiters behind it do not wait on it. removeEntry(dir, own); filed = false; diff --git a/tests/helpers/credential-store-renewal.test.ts b/tests/helpers/credential-store-renewal.test.ts index 1efe0f28..e3201b80 100644 --- a/tests/helpers/credential-store-renewal.test.ts +++ b/tests/helpers/credential-store-renewal.test.ts @@ -367,6 +367,31 @@ describe("renewal under the refresh lock", () => { } }); + // A helper that hangs on the re-read inside the lock times out once; the + // legacy lookup would only wait for the same helper again. + test("skips the legacy lookup when the re-read inside the lock times out", async () => { + let call = 0; + const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => { + call += 1; + if (call === 1) return sessionRecord("octocat", expired); + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + return { + stdout: new ReadableStream({ start() {} }), + exited: new Promise(() => {}), + kill: () => {}, + } as unknown as ReturnType; + }); + const refreshSpy = spyOn(platformAuth, "refreshAccessToken"); + try { + expect(await loadCredentials()).toBeNull(); + expect(call).toBe(2); + expect(refreshSpy).not.toHaveBeenCalled(); + } finally { + refreshSpy.mockRestore(); + spawnSpy.mockRestore(); + } + }); + // Keeping a refused session would send the same dead token on every later // lookup. test("removes the session whose refresh token was refused", async () => { diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index 2b5ef90e..3ee5b1de 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -352,11 +352,20 @@ describe("withRefreshLock", () => { }); test("skips an entry that vanishes while it is checked", async () => { - foreignEntry(entryName(1, "low")); - const statSpy = spyOn(fs, "statSync").mockImplementationOnce(() => { - throw Object.assign(new Error("gone"), { code: "ENOENT" }); - }); + const holder = entryName(1, "low"); + foreignEntry(holder); + const realStat = fs.statSync; + // oxlint-disable-next-line typescript/no-unsafe-type-assertion -- one overload is all the code under test uses + const statSpy = spyOn(fs, "statSync").mockImplementation((( + ...args: Parameters + ) => { + if (String(args[0]).endsWith(holder)) { + throw Object.assign(new Error("gone"), { code: "ENOENT" }); + } + return realStat(...args); + }) as typeof fs.statSync); try { + const started = Date.now(); let ran = false; await withRefreshLock( async () => { @@ -366,11 +375,94 @@ describe("withRefreshLock", () => { { waitMs: 1_000, pollMs: 5 } ); expect(ran).toBe(true); + // Skipped at once, not run after the wait deadline. + expect(Date.now() - started).toBeLessThan(500); } finally { statSpy.mockRestore(); } }); + // An abandoned entry that cannot be deleted still counts as a holder, so + // the caller keeps waiting rather than acting on a removal that failed. + test("keeps waiting on an abandoned entry it cannot remove", async () => { + const abandoned = foreignEntry(entryName(1, "low"), 60_000); + const unlinkSpy = spyOn(fs, "unlinkSync").mockImplementation((path) => { + if (String(path) === abandoned) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } + fs.rmSync(path); + }); + try { + let ran = false; + await withRefreshLock( + async () => { + ran = true; + // Withdrawn after the deadline; the abandoned entry is still there. + expect(readdirSync(lockDir)).toEqual([entryName(1, "low")]); + }, + lockDir, + { staleMs: 1_000, waitMs: 20, pollMs: 5 } + ); + expect(ran).toBe(true); + } finally { + unlinkSpy.mockRestore(); + } + }); + + // Others reap entries untouched for `staleMs`, so a waiter whose refreshes + // keep failing withdraws before it can be mistaken for dead. + test("withdraws once its refreshes have failed for half the stale window", async () => { + foreignEntry(entryName(1, "low")); + const touchSpy = spyOn(fs, "utimesSync").mockImplementation(() => { + throw Object.assign(new Error("denied"), { code: "EPERM" }); + }); + try { + const started = Date.now(); + let ran = false; + await withRefreshLock( + async () => { + ran = true; + expect(readdirSync(lockDir)).toEqual([entryName(1, "low")]); + }, + lockDir, + { staleMs: 60, waitMs: 5_000, pollMs: 5 } + ); + expect(ran).toBe(true); + expect(Date.now() - started).toBeLessThan(2_000); + } finally { + touchSpy.mockRestore(); + } + }); + + test("keeps waiting through a refresh failure that recovers", async () => { + const holder = foreignEntry(entryName(1, "low")); + let touches = 0; + const realTouch = fs.utimesSync; + const touchSpy = spyOn(fs, "utimesSync").mockImplementation((...args) => { + touches += 1; + if (touches === 1) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } + // The holder finishes once this waiter has refreshed successfully. + fs.rmSync(holder, { force: true }); + realTouch(...args); + }); + try { + await withRefreshLock( + async () => { + // Still filed: it ran holding the lock, not after withdrawing. + expect(readdirSync(lockDir)).toHaveLength(1); + expect(existsSync(holder)).toBe(false); + }, + lockDir, + { staleMs: 10_000, waitMs: 5_000, pollMs: 5 } + ); + expect(touches).toBeGreaterThan(1); + } finally { + touchSpy.mockRestore(); + } + }); + // A waiter without an entry is not in line, so it stops waiting rather // than polling for a turn that cannot come. test("stops waiting when its own entry disappears", async () => { From cc6090fdc515c5a1735551b0af4cba70137dab08 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 01:07:34 +0200 Subject: [PATCH 10/12] fix(auth): bound waiter sleeps by the heartbeat window A waiter sleeps at most until half of staleMs has passed since its last successful refresh and checks that window before trying to acquire, so a poll interval at or above staleMs can neither let a healthy waiter go stale nor delay withdrawal after failed refreshes. Heartbeat tests move to refresh-lock-heartbeat.test.ts. Signed-off-by: Rhuan Barreto --- src/helpers/refresh-lock.ts | 26 ++-- tests/helpers/refresh-lock-heartbeat.test.ts | 146 +++++++++++++++++++ tests/helpers/refresh-lock.test.ts | 54 ------- 3 files changed, 160 insertions(+), 66 deletions(-) create mode 100644 tests/helpers/refresh-lock-heartbeat.test.ts diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index a5e3f9f8..60ba4efb 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -191,19 +191,20 @@ export async function withRefreshLock( /* oxlint-disable no-await-in-loop -- one wait, then polling in order */ while (filed && Date.now() <= createdAt) await Bun.sleep(1); const deadline = Date.now() + waitMs; - // Others reap an entry untouched for `staleMs`, so a waiter whose refreshes - // keep failing withdraws halfway there, before it can be mistaken for dead. + // Others reap an entry untouched for `staleMs`. A waiter refreshes its entry + // at least every half of that, and withdraws once half has passed without a + // successful refresh, before it can be mistaken for dead. + const heartbeatMs = staleMs / 2; let lastAlive = Date.now(); while (filed) { - const holds = holdsLock(dir, own, staleMs); - if (holds === true) break; - const touch = holds === null ? "gone" : touchEntry(dir, own); - if (touch === "touched") lastAlive = Date.now(); - const withdraw = - holds === null || - touch === "gone" || - Date.now() >= deadline || - Date.now() - lastAlive >= staleMs / 2; + let withdraw = Date.now() - lastAlive >= heartbeatMs; + if (!withdraw) { + const holds = holdsLock(dir, own, staleMs); + if (holds === true) break; + const touch = holds === null ? "gone" : touchEntry(dir, own); + if (touch === "touched") lastAlive = Date.now(); + withdraw = touch === "gone" || Date.now() >= deadline; + } if (withdraw) { // Withdrawn so that waiters behind it do not wait on it. removeEntry(dir, own); @@ -211,7 +212,8 @@ export async function withRefreshLock( logDebug("refresh lock not acquired; renewing without it"); break; } - await Bun.sleep(pollMs); + const untilOverdue = lastAlive + heartbeatMs - Date.now(); + await Bun.sleep(Math.max(1, Math.min(pollMs, untilOverdue))); } /* oxlint-enable no-await-in-loop */ diff --git a/tests/helpers/refresh-lock-heartbeat.test.ts b/tests/helpers/refresh-lock-heartbeat.test.ts new file mode 100644 index 00000000..e792469e --- /dev/null +++ b/tests/helpers/refresh-lock-heartbeat.test.ts @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import * as fs from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readdirSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { withRefreshLock } from "../../src/helpers/refresh-lock"; +import { safeRmSync } from "../test-utils"; + +/** The name a live holder that sorts before any new caller would file. */ +const HOLDER = `${String(1).padStart(15, "0")}-00000000-0000-4000-8000-000000000000`; + +// A waiter refreshes its entry while it polls, because other callers remove +// entries left untouched for `staleMs`. +describe("withRefreshLock heartbeat", () => { + let tempDir: string; + let lockDir: string; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-refresh-heartbeat-")); + lockDir = join(tempDir, "refresh-lock"); + }); + + afterEach(() => { + safeRmSync(tempDir); + }); + + /** A live entry filed by another process that holds the lock. */ + function holderEntry(): string { + mkdirSync(lockDir, { recursive: true }); + const path = join(lockDir, HOLDER); + writeFileSync(path, "4242"); + return path; + } + + // Others reap entries untouched for `staleMs`, so a waiter whose refreshes + // keep failing withdraws before it can be mistaken for dead. + test("withdraws once its refreshes have failed for half the stale window", async () => { + holderEntry(); + const touchSpy = spyOn(fs, "utimesSync").mockImplementation(() => { + throw Object.assign(new Error("denied"), { code: "EPERM" }); + }); + try { + const started = Date.now(); + let ran = false; + await withRefreshLock( + async () => { + ran = true; + expect(readdirSync(lockDir)).toEqual([HOLDER]); + }, + lockDir, + { staleMs: 60, waitMs: 5_000, pollMs: 5 } + ); + expect(ran).toBe(true); + expect(Date.now() - started).toBeLessThan(2_000); + } finally { + touchSpy.mockRestore(); + } + }); + + // A poll interval longer than the stale window must not let a waiter sleep + // past the point where others would reap it. + test("withdraws on time when polls are slower than the stale window", async () => { + holderEntry(); + const touchSpy = spyOn(fs, "utimesSync").mockImplementation(() => { + throw Object.assign(new Error("denied"), { code: "EPERM" }); + }); + try { + const started = Date.now(); + await withRefreshLock(async () => {}, lockDir, { + staleMs: 60, + pollMs: 1_000, + waitMs: 5_000, + }); + expect(Date.now() - started).toBeLessThan(500); + } finally { + touchSpy.mockRestore(); + } + }); + + test("refreshes often enough to stay live when polls are slower than the stale window", async () => { + const holder = holderEntry(); + const touched: number[] = []; + const realTouch = fs.utimesSync; + const touchSpy = spyOn(fs, "utimesSync").mockImplementation((...args) => { + touched.push(Date.now()); + realTouch(...args); + }); + setTimeout(() => { + fs.rmSync(holder, { force: true }); + }, 150); + try { + const started = Date.now(); + await withRefreshLock( + async () => { + // Still filed: it acquired the lock rather than withdrawing. + expect(readdirSync(lockDir)).toHaveLength(1); + }, + lockDir, + { staleMs: 60, pollMs: 1_000, waitMs: 5_000 } + ); + expect(Date.now() - started).toBeLessThan(900); + const gaps = touched.slice(1).map((at, i) => at - (touched[i] ?? at)); + expect(gaps.filter((gap) => gap >= 60)).toEqual([]); + } finally { + touchSpy.mockRestore(); + } + }); + + test("keeps waiting through a refresh failure that recovers", async () => { + const holder = holderEntry(); + let touches = 0; + const realTouch = fs.utimesSync; + const touchSpy = spyOn(fs, "utimesSync").mockImplementation((...args) => { + touches += 1; + if (touches === 1) { + throw Object.assign(new Error("busy"), { code: "EBUSY" }); + } + // The holder finishes once this waiter has refreshed successfully. + fs.rmSync(holder, { force: true }); + realTouch(...args); + }); + try { + await withRefreshLock( + async () => { + // Still filed: it ran holding the lock, not after withdrawing. + expect(readdirSync(lockDir)).toHaveLength(1); + expect(existsSync(holder)).toBe(false); + }, + lockDir, + { staleMs: 10_000, waitMs: 5_000, pollMs: 5 } + ); + expect(touches).toBeGreaterThan(1); + } finally { + touchSpy.mockRestore(); + } + }); +}); diff --git a/tests/helpers/refresh-lock.test.ts b/tests/helpers/refresh-lock.test.ts index 3ee5b1de..804deaf4 100644 --- a/tests/helpers/refresh-lock.test.ts +++ b/tests/helpers/refresh-lock.test.ts @@ -409,60 +409,6 @@ describe("withRefreshLock", () => { } }); - // Others reap entries untouched for `staleMs`, so a waiter whose refreshes - // keep failing withdraws before it can be mistaken for dead. - test("withdraws once its refreshes have failed for half the stale window", async () => { - foreignEntry(entryName(1, "low")); - const touchSpy = spyOn(fs, "utimesSync").mockImplementation(() => { - throw Object.assign(new Error("denied"), { code: "EPERM" }); - }); - try { - const started = Date.now(); - let ran = false; - await withRefreshLock( - async () => { - ran = true; - expect(readdirSync(lockDir)).toEqual([entryName(1, "low")]); - }, - lockDir, - { staleMs: 60, waitMs: 5_000, pollMs: 5 } - ); - expect(ran).toBe(true); - expect(Date.now() - started).toBeLessThan(2_000); - } finally { - touchSpy.mockRestore(); - } - }); - - test("keeps waiting through a refresh failure that recovers", async () => { - const holder = foreignEntry(entryName(1, "low")); - let touches = 0; - const realTouch = fs.utimesSync; - const touchSpy = spyOn(fs, "utimesSync").mockImplementation((...args) => { - touches += 1; - if (touches === 1) { - throw Object.assign(new Error("busy"), { code: "EBUSY" }); - } - // The holder finishes once this waiter has refreshed successfully. - fs.rmSync(holder, { force: true }); - realTouch(...args); - }); - try { - await withRefreshLock( - async () => { - // Still filed: it ran holding the lock, not after withdrawing. - expect(readdirSync(lockDir)).toHaveLength(1); - expect(existsSync(holder)).toBe(false); - }, - lockDir, - { staleMs: 10_000, waitMs: 5_000, pollMs: 5 } - ); - expect(touches).toBeGreaterThan(1); - } finally { - touchSpy.mockRestore(); - } - }); - // A waiter without an entry is not in line, so it stops waiting rather // than polling for a turn that cannot come. test("stops waiting when its own entry disappears", async () => { From 4f6b1353e06b7ec70bba25ab6205def57a8239d5 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 01:10:47 +0200 Subject: [PATCH 11/12] fix(auth): refresh a waiting entry well before its withdrawal point A waiter refreshes once a quarter of staleMs has passed and withdraws only after half passes without a successful refresh, so a timer that fires late still refreshes in time. After a failed refresh the next wake-up is the withdrawal point rather than a 1 ms spin. Signed-off-by: Rhuan Barreto --- src/helpers/refresh-lock.ts | 17 +++++++----- tests/helpers/refresh-lock-heartbeat.test.ts | 27 ++++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/src/helpers/refresh-lock.ts b/src/helpers/refresh-lock.ts index 60ba4efb..01df5fba 100644 --- a/src/helpers/refresh-lock.ts +++ b/src/helpers/refresh-lock.ts @@ -192,12 +192,13 @@ export async function withRefreshLock( while (filed && Date.now() <= createdAt) await Bun.sleep(1); const deadline = Date.now() + waitMs; // Others reap an entry untouched for `staleMs`. A waiter refreshes its entry - // at least every half of that, and withdraws once half has passed without a - // successful refresh, before it can be mistaken for dead. - const heartbeatMs = staleMs / 2; + // once a quarter of that has passed, and withdraws only after half passes + // without a successful refresh, so a late timer still refreshes in time. + const refreshMs = staleMs / 4; + const withdrawMs = staleMs / 2; let lastAlive = Date.now(); while (filed) { - let withdraw = Date.now() - lastAlive >= heartbeatMs; + let withdraw = Date.now() - lastAlive >= withdrawMs; if (!withdraw) { const holds = holdsLock(dir, own, staleMs); if (holds === true) break; @@ -212,8 +213,12 @@ export async function withRefreshLock( logDebug("refresh lock not acquired; renewing without it"); break; } - const untilOverdue = lastAlive + heartbeatMs - Date.now(); - await Bun.sleep(Math.max(1, Math.min(pollMs, untilOverdue))); + // After a failed refresh the next refresh time has passed, so the next + // wake-up is the withdrawal point instead. + const now = Date.now(); + const nextRefresh = lastAlive + refreshMs; + const wakeAt = nextRefresh > now ? nextRefresh : lastAlive + withdrawMs; + await Bun.sleep(Math.max(1, Math.min(pollMs, wakeAt - now))); } /* oxlint-enable no-await-in-loop */ diff --git a/tests/helpers/refresh-lock-heartbeat.test.ts b/tests/helpers/refresh-lock-heartbeat.test.ts index e792469e..622da7e9 100644 --- a/tests/helpers/refresh-lock-heartbeat.test.ts +++ b/tests/helpers/refresh-lock-heartbeat.test.ts @@ -108,6 +108,8 @@ describe("withRefreshLock heartbeat", () => { { staleMs: 60, pollMs: 1_000, waitMs: 5_000 } ); expect(Date.now() - started).toBeLessThan(900); + // Several refreshes, so the gap check below compares real intervals. + expect(touched.length).toBeGreaterThanOrEqual(3); const gaps = touched.slice(1).map((at, i) => at - (touched[i] ?? at)); expect(gaps.filter((gap) => gap >= 60)).toEqual([]); } finally { @@ -115,6 +117,31 @@ describe("withRefreshLock heartbeat", () => { } }); + // Timers can fire late. A healthy waiter must still refresh before its + // withdrawal point rather than give up. + test("stays in line when its timers fire late", async () => { + const holder = holderEntry(); + const realSleep = Bun.sleep; + const sleepSpy = spyOn(Bun, "sleep").mockImplementation(async (ms) => + realSleep(Number(ms) + 10) + ); + setTimeout(() => { + fs.rmSync(holder, { force: true }); + }, 150); + try { + await withRefreshLock( + async () => { + // Still filed: it acquired the lock rather than withdrawing. + expect(readdirSync(lockDir)).toHaveLength(1); + }, + lockDir, + { staleMs: 80, pollMs: 1_000, waitMs: 5_000 } + ); + } finally { + sleepSpy.mockRestore(); + } + }); + test("keeps waiting through a refresh failure that recovers", async () => { const holder = holderEntry(); let touches = 0; From f0fe3340f423f524fe234b17f505c3df9838e18a Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Sat, 24 Oct 2026 02:01:55 +0200 Subject: [PATCH 12/12] test: widen heartbeat timing margins and assert the holder finished The healthy-heartbeat and late-timer tests use a 400 ms stale window and release the holder after 500 ms, so parallel test scheduling cannot push a refresh past the withdrawal point. Both assert the holder's entry is gone when the task runs, which a waiter that withdrew early cannot satisfy. Signed-off-by: Rhuan Barreto --- tests/helpers/refresh-lock-heartbeat.test.ts | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/helpers/refresh-lock-heartbeat.test.ts b/tests/helpers/refresh-lock-heartbeat.test.ts index 622da7e9..9f985bd3 100644 --- a/tests/helpers/refresh-lock-heartbeat.test.ts +++ b/tests/helpers/refresh-lock-heartbeat.test.ts @@ -96,22 +96,24 @@ describe("withRefreshLock heartbeat", () => { }); setTimeout(() => { fs.rmSync(holder, { force: true }); - }, 150); + }, 500); try { const started = Date.now(); await withRefreshLock( async () => { - // Still filed: it acquired the lock rather than withdrawing. + // It acquired the lock once the holder finished, rather than + // withdrawing and running while the holder's entry still stood. + expect(existsSync(holder)).toBe(false); expect(readdirSync(lockDir)).toHaveLength(1); }, lockDir, - { staleMs: 60, pollMs: 1_000, waitMs: 5_000 } + { staleMs: 400, pollMs: 1_000, waitMs: 5_000 } ); expect(Date.now() - started).toBeLessThan(900); // Several refreshes, so the gap check below compares real intervals. expect(touched.length).toBeGreaterThanOrEqual(3); const gaps = touched.slice(1).map((at, i) => at - (touched[i] ?? at)); - expect(gaps.filter((gap) => gap >= 60)).toEqual([]); + expect(gaps.filter((gap) => gap >= 400)).toEqual([]); } finally { touchSpy.mockRestore(); } @@ -123,19 +125,21 @@ describe("withRefreshLock heartbeat", () => { const holder = holderEntry(); const realSleep = Bun.sleep; const sleepSpy = spyOn(Bun, "sleep").mockImplementation(async (ms) => - realSleep(Number(ms) + 10) + realSleep(Number(ms) + 50) ); setTimeout(() => { fs.rmSync(holder, { force: true }); - }, 150); + }, 500); try { await withRefreshLock( async () => { - // Still filed: it acquired the lock rather than withdrawing. + // It acquired the lock once the holder finished, rather than + // withdrawing and running while the holder's entry still stood. + expect(existsSync(holder)).toBe(false); expect(readdirSync(lockDir)).toHaveLength(1); }, lockDir, - { staleMs: 80, pollMs: 1_000, waitMs: 5_000 } + { staleMs: 400, pollMs: 1_000, waitMs: 5_000 } ); } finally { sleepSpy.mockRestore();