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" } diff --git a/src/helpers/credential-store.ts b/src/helpers/credential-store.ts index 3333e58c..f685fe3f 100644 --- a/src/helpers/credential-store.ts +++ b/src/helpers/credential-store.ts @@ -24,11 +24,12 @@ 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. */ +/** How often, and how far apart, a refused renewal re-reads the store. */ const RENEWAL_REREADS = 4; const RENEWAL_REREAD_DELAY_MS = 250; @@ -306,58 +307,117 @@ 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 withRefreshLock(renew); +} - 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. - * - * 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. + * 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. * - * @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, + * and whether the store's helper timed out on the read. + * @throws {SessionExpiredError} When the platform refuses the refresh token. */ -async function renew(stale: Session): Promise { +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 { credentials: sessionCredentials(session), timedOut: false }; + } + + 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 }; - } + if (!(error instanceof SessionExpiredError)) throw error; + const renewed = await sessionRenewedElsewhere(session); + if (renewed) { + return { credentials: sessionCredentials(renewed), timedOut: false }; } - /* oxlint-enable no-await-in-loop */ + 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 { + credentials: { token: renewed.accessToken, github_user: session.user }, + timedOut: false, + }; +} + +/** + * 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. + * + * 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..01df5fba --- /dev/null +++ b/src/helpers/refresh-lock.ts @@ -0,0 +1,230 @@ +// 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. Each caller files a uniquely named entry + * in a directory, and the oldest live entry holds the lock once no caller is + * still choosing its place (the bakery algorithm). + */ + +import { + mkdirSync, + readdirSync, + statSync, + unlinkSync, + utimesSync, + writeFileSync, +} from "node:fs"; +import { join } 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 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. + */ + staleMs: number; +} + +const DEFAULT_TIMINGS: LockTimings = { + waitMs: 40_000, + pollMs: 100, + staleMs: 30_000, +}; + +/** Width of the zero-padded millisecond timestamp that starts each entry name. */ +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"); +} + +/** 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)); + 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 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): "touched" | "gone" | "failed" { + const now = new Date(); + try { + utimesSync(join(dir, name), now, now); + return "touched"; + } catch (error) { + if (errorCode(error) === "ENOENT") return "gone"; + logDebug("could not refresh a refresh lock entry", { + code: errorCode(error), + }); + return "failed"; + } +} + +/** + * 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) || MARKER_PATTERN.test(name) + ); + } catch { + return null; + } + const live: string[] = []; + for (const name of names) { + let age: number; + 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 && removeEntry(dir, name)) { + logDebug("removed an abandoned refresh lock entry"); + } else { + live.push(name); + } + } + if (live.some((name) => MARKER_PATTERN.test(name))) return false; + return live.toSorted()[0] === own; +} + +/** + * 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 dir - Entry directory; defaults to {@link refreshLockDir}. + * @param timings - Overrides for {@link DEFAULT_TIMINGS}. + */ +export async function withRefreshLock( + task: () => Promise, + dir: string = refreshLockDir(), + timings: Partial = {} +): Promise { + const { waitMs, pollMs, staleMs } = { ...DEFAULT_TIMINGS, ...timings }; + 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; + + // 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; + // Others reap an entry untouched for `staleMs`. A waiter refreshes its entry + // 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 >= withdrawMs; + 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); + filed = false; + logDebug("refresh lock not acquired; renewing without it"); + break; + } + // 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 */ + + try { + return await task(); + } finally { + if (filed) removeEntry(dir, own); + } +} diff --git a/tests/helpers/credential-store-renewal.test.ts b/tests/helpers/credential-store-renewal.test.ts index d5424f5e..e3201b80 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,49 +279,150 @@ 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 () => { + /** 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" + ).mockResolvedValue(rotated); + const saveSpy = spyOn(credentialStore, "saveTokenSet").mockResolvedValue( + true + ); + try { + expect(await resolveAccessToken()).toEqual({ + token: "ey.fromHolder", + github_user: "octocat", + }); + expect(lockSpy).toHaveBeenCalledTimes(1); + } finally { + saveSpy.mockRestore(); + refreshSpy.mockRestore(); + fillSpy.mockRestore(); + } + }); + + 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 === 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(); } }); - // 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("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 sessionRecord("octocat", read >= 3 ? rotated : expired); + 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(); + } + }); + + // 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 () => { + const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => + sessionRecord("octocat", expired) + ); + const refreshSpy = spyOn( + platformAuth, + "refreshAccessToken" + ).mockRejectedValue(new SessionExpiredError()); + try { + 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(); + spawnSpy.mockRestore(); + } + }); + + // `archgate login` does not take the lock, so a sign-in can land between + // the refused exchange and the removal. + test("uses a session saved elsewhere after the refusal", async () => { + let read = 0; + const spawnSpy = spyOn(Bun, "spawn").mockImplementation(() => { + read += 1; + return sessionRecord("octocat", read <= 2 ? expired : rotated); }); const refreshSpy = spyOn( platformAuth, @@ -301,19 +430,25 @@ describe("concurrent renewal", () => { ).mockRejectedValue(new SessionExpiredError()); try { expect(await resolveAccessToken()).toEqual({ - token: "ey.fromWinner", + token: "ey.fromHolder", github_user: "octocat", }); + expect(await stdinOf(spawnSpy, "reject")).toHaveLength(0); } finally { refreshSpy.mockRestore(); - fillSpy.mockRestore(); + spawnSpy.mockRestore(); } }); - test("rethrows when the stored token set did not change", async () => { - const fillSpy = spyOn(Bun, "spawn").mockImplementation(() => - sessionRecord("octocat", expired) - ); + 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" @@ -322,9 +457,49 @@ describe("concurrent renewal", () => { expect(await rejectionMessage(resolveAccessToken())).toContain( "session has expired" ); + expect(await stdinOf(spawnSpy, "reject")).toHaveLength(0); } finally { refreshSpy.mockRestore(); - fillSpy.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-heartbeat.test.ts b/tests/helpers/refresh-lock-heartbeat.test.ts new file mode 100644 index 00000000..9f985bd3 --- /dev/null +++ b/tests/helpers/refresh-lock-heartbeat.test.ts @@ -0,0 +1,177 @@ +// 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 }); + }, 500); + try { + const started = Date.now(); + await withRefreshLock( + async () => { + // 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: 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 >= 400)).toEqual([]); + } finally { + touchSpy.mockRestore(); + } + }); + + // 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) + 50) + ); + setTimeout(() => { + fs.rmSync(holder, { force: true }); + }, 500); + try { + await withRefreshLock( + async () => { + // 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: 400, 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; + 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 new file mode 100644 index 00000000..804deaf4 --- /dev/null +++ b/tests/helpers/refresh-lock.test.ts @@ -0,0 +1,457 @@ +// 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, + readFileSync, + utimesSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +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 lockDir: string; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "archgate-refresh-lock-")); + lockDir = join(tempDir, "refresh-lock"); + }); + + afterEach(() => { + safeRmSync(tempDir); + }); + + /** 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(path, then, then); + } + return path; + } + + test("files an entry while the task runs and removes it afterwards", async () => { + const result = await withRefreshLock(async () => { + const names = readdirSync(lockDir); + expect(names).toHaveLength(1); + expect(readFileSync(join(lockDir, names[0] ?? ""), "utf8")).toBe( + String(process.pid) + ); + return "done"; + }, lockDir); + + expect(result).toBe("done"); + expect(readdirSync(lockDir)).toEqual([]); + }); + + test("removes its entry when the task throws", async () => { + expect( + await rejectionMessage( + withRefreshLock(async () => { + throw new Error("renewal failed"); + }, lockDir) + ) + ).toBe("renewal failed"); + expect(readdirSync(lockDir)).toEqual([]); + }); + + // 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( + Array.from({ length: 6 }, async () => + withRefreshLock(task, lockDir, { pollMs: 2, staleMs: 1_000 }) + ) + ); + + expect(finished).toBe(6); + expect(peak).toBe(1); + expect(readdirSync(lockDir)).toEqual([]); + }); + + test("removes an entry abandoned by a dead process", async () => { + const abandoned = foreignEntry(entryName(1, "low"), 60_000); + + await withRefreshLock( + async () => { + expect(existsSync(abandoned)).toBe(false); + }, + lockDir, + { staleMs: 1_000 } + ); + }); + + // A slow holder must not block the git operation indefinitely. + 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")]); + }, + lockDir, + { waitMs: 20, pollMs: 5 } + ); + + expect(ran).toBe(true); + expect(existsSync(holder)).toBe(true); + }); + + test("is not held up by an entry filed after its own", async () => { + foreignEntry(entryName(Date.now() + 60_000, "low")); + + let entries = 0; + await withRefreshLock( + async () => { + entries = readdirSync(lockDir).length; + }, + lockDir, + { waitMs: 1_000 } + ); + + expect(entries).toBe(2); + }); + + // 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 { + await withRefreshLock( + async () => { + expect(readdirSync(lockDir)).toEqual([entryName(stamp, "low")]); + }, + lockDir, + { waitMs: 20, pollMs: 5 } + ); + } finally { + nowSpy.mockRestore(); + } + }); + + // 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); + }); + + // 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( + 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"), ""); + + let entries: string[] = []; + await withRefreshLock(async () => { + entries = readdirSync(lockDir); + }, lockDir); + + 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; + }, + join(blocker, "refresh-lock") + ); + + expect(ran).toBe(true); + }); + + 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; + }, lockDir); + expect(ran).toBe(true); + } finally { + readSpy.mockRestore(); + } + expect(readdirSync(lockDir)).toEqual([]); + }); + + test("skips an entry that vanishes while it is checked", async () => { + 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 () => { + ran = true; + }, + lockDir, + { 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(); + } + }); + + // 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 readSpy = spyOn(fs, "readdirSync").mockImplementationOnce((( + ...args: Parameters + ) => { + for (const name of original(lockDir)) { + if (name !== holderName) fs.rmSync(join(lockDir, name)); + } + return original(...args); + }) as typeof fs.readdirSync); + try { + let ran = false; + await withRefreshLock( + async () => { + ran = true; + }, + lockDir, + { waitMs: 60_000, pollMs: 5 } + ); + expect(ran).toBe(true); + } 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 archgate state directory", async () => { + const originalHome = Bun.env.HOME; + Bun.env.HOME = tempDir; + try { + await withRefreshLock(async () => { + expect( + readdirSync(join(tempDir, ".archgate", "refresh-lock")) + ).toHaveLength(1); + }); + } finally { + restoreEnv("HOME", originalHome); + } + }); +});