Skip to content

Commit 90d02ae

Browse files
TheGreatAxiosTheGreatAxios
authored
fix(auth): recover stale credential file locks via PID liveness (#1194)
* fix(auth): recover stale credential file locks via PID liveness * fix(auth): verify lock claim after steal and pace contention The steal re-read narrows but does not close the unlink window, so a loser that deletes a winner's live lock could run two holders. Claims are now unique per waiter and re-read after create; a mismatch closes and re-contends. Steal contention also falls through to the deadline and sleep path instead of hot-spinning. * test(auth): cover pid:counter lock claims with keepers * style(auth): format credential lock tests * fix(auth): preserve credential cleanup error precedence
1 parent 9758a42 commit 90d02ae

2 files changed

Lines changed: 446 additions & 16 deletions

File tree

‎src/auth/store.test.ts‎

Lines changed: 276 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,32 @@
11
import { describe, expect, test } from "bun:test";
2-
import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
2+
import {
3+
mkdir,
4+
mkdtemp,
5+
readFile,
6+
rm,
7+
utimes,
8+
writeFile,
9+
} from "node:fs/promises";
310
import { tmpdir } from "node:os";
411
import { join } from "node:path";
512
import { type } from "arktype";
613

7-
import { createAuthStore, type BaseTokens } from "./store.js";
14+
import { withMockedModule } from "../../tests/helpers/mock-module.js";
15+
import type { BaseTokens } from "./store.js";
16+
17+
let failingUnlinkPath: string | undefined;
18+
await withMockedModule(
19+
import.meta.resolve("node:fs/promises"),
20+
(real: typeof import("node:fs/promises")) => ({
21+
...real,
22+
unlink: async (...args: Parameters<typeof real.unlink>) => {
23+
if (args[0] === failingUnlinkPath) throw new Error("lock release failed");
24+
return real.unlink(...args);
25+
},
26+
}),
27+
);
28+
29+
const { createAuthStore } = await import("./store.js");
830

931
type TestTokens = BaseTokens & { accountId?: string };
1032

@@ -294,6 +316,64 @@ describe("createAuthStore", () => {
294316
}
295317
});
296318

319+
test("surfaces a credential lock release failure after a successful write", async () => {
320+
const home = await mkdtemp(join(tmpdir(), "oauth-store-release-error-"));
321+
try {
322+
const store = createAuthStore<TestTokens>({
323+
filename: "test-auth.json",
324+
settingsDirName: TEST_SETTINGS_DIR,
325+
isTokens: isTestTokens,
326+
});
327+
failingUnlinkPath = `${store.authPath(home)}.lock`;
328+
329+
await expect(
330+
store.saveProfile(
331+
{
332+
name: "work",
333+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
334+
createdAt: 1,
335+
},
336+
home,
337+
),
338+
).rejects.toThrow("lock release failed");
339+
} finally {
340+
failingUnlinkPath = undefined;
341+
await rm(home, { recursive: true, force: true });
342+
}
343+
});
344+
345+
test("does not mask a read-modify-write callback failure during release", async () => {
346+
const home = await mkdtemp(join(tmpdir(), "oauth-store-error-precedence-"));
347+
try {
348+
const store = createAuthStore<TestTokens>({
349+
filename: "test-auth.json",
350+
settingsDirName: TEST_SETTINGS_DIR,
351+
isTokens: isTestTokens,
352+
});
353+
const profile = {
354+
name: "work",
355+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
356+
createdAt: 1,
357+
};
358+
await store.saveProfile(profile, home);
359+
360+
const failingStore = createAuthStore<TestTokens>({
361+
filename: "test-auth.json",
362+
settingsDirName: TEST_SETTINGS_DIR,
363+
isTokens: (_value: unknown): _value is TestTokens => {
364+
throw new Error("validator failed");
365+
},
366+
});
367+
failingUnlinkPath = `${store.authPath(home)}.lock`;
368+
await expect(failingStore.saveProfile(profile, home)).rejects.toThrow(
369+
"validator failed",
370+
);
371+
} finally {
372+
failingUnlinkPath = undefined;
373+
await rm(home, { recursive: true, force: true });
374+
}
375+
});
376+
297377
test("releases the credential lock when a read-modify-write callback fails", async () => {
298378
const home = await mkdtemp(join(tmpdir(), "oauth-store-error-"));
299379
try {
@@ -328,6 +408,200 @@ describe("createAuthStore", () => {
328408
}
329409
});
330410

411+
test("takes over a dead-holder lock instead of timing out", async () => {
412+
const home = await mkdtemp(join(tmpdir(), "oauth-store-takeover-"));
413+
try {
414+
const store = createAuthStore<TestTokens>({
415+
filename: "test-auth.json",
416+
settingsDirName: TEST_SETTINGS_DIR,
417+
isTokens: isTestTokens,
418+
});
419+
const lockPath = `${store.authPath(home)}.lock`;
420+
await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
421+
// The maximum pid_t can never be a live holder: kill(pid, 0) answers
422+
// ESRCH (or EINVAL), both of which read as dead.
423+
await writeFile(lockPath, `${2_147_483_647}`, { mode: 0o600 });
424+
425+
const profile = {
426+
name: "work",
427+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
428+
createdAt: 1,
429+
};
430+
await expect(store.saveProfile(profile, home)).resolves.toBeUndefined();
431+
expect(await store.loadProfile("work", home)).toEqual(profile);
432+
await expect(readFile(lockPath, "utf8")).rejects.toThrow("ENOENT");
433+
} finally {
434+
await rm(home, { recursive: true, force: true });
435+
}
436+
});
437+
438+
test("takes over a dead-holder pid:counter claim instead of timing out", async () => {
439+
const home = await mkdtemp(join(tmpdir(), "oauth-store-takeover-claim-"));
440+
try {
441+
const store = createAuthStore<TestTokens>({
442+
filename: "test-auth.json",
443+
settingsDirName: TEST_SETTINGS_DIR,
444+
isTokens: isTestTokens,
445+
});
446+
const lockPath = `${store.authPath(home)}.lock`;
447+
await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
448+
// The NEW pid:counter claim format with a certainly-dead PID: kill(pid, 0)
449+
// answers ESRCH (or EINVAL), both of which read as dead. The counter leg
450+
// must not stop holderPid from reading the pid leg.
451+
await writeFile(lockPath, `${2_147_483_647}:99`, { mode: 0o600 });
452+
453+
const profile = {
454+
name: "work",
455+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
456+
createdAt: 1,
457+
};
458+
await expect(store.saveProfile(profile, home)).resolves.toBeUndefined();
459+
expect(await store.loadProfile("work", home)).toEqual(profile);
460+
await expect(readFile(lockPath, "utf8")).rejects.toThrow("ENOENT");
461+
} finally {
462+
await rm(home, { recursive: true, force: true });
463+
}
464+
});
465+
466+
test("waits on a live-holder lock and times out without touching it", async () => {
467+
const home = await mkdtemp(join(tmpdir(), "oauth-store-live-"));
468+
try {
469+
const store = createAuthStore<TestTokens>({
470+
filename: "test-auth.json",
471+
settingsDirName: TEST_SETTINGS_DIR,
472+
isTokens: isTestTokens,
473+
lockTimeoutMs: 100,
474+
});
475+
const lockPath = `${store.authPath(home)}.lock`;
476+
await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
477+
await writeFile(lockPath, `${process.pid}`, { mode: 0o600 });
478+
479+
await expect(
480+
store.saveProfile(
481+
{
482+
name: "work",
483+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
484+
createdAt: 1,
485+
},
486+
home,
487+
),
488+
).rejects.toThrow(
489+
`Timed out waiting for OAuth credential lock ${lockPath}. ` +
490+
"If no Corbits process is running, remove this lock file manually and retry.",
491+
);
492+
expect(await readFile(lockPath, "utf8")).toBe(`${process.pid}`);
493+
} finally {
494+
await rm(home, { recursive: true, force: true });
495+
}
496+
});
497+
498+
test("waits on a live-holder pid:counter claim and times out without touching it", async () => {
499+
const home = await mkdtemp(join(tmpdir(), "oauth-store-live-claim-"));
500+
try {
501+
const store = createAuthStore<TestTokens>({
502+
filename: "test-auth.json",
503+
settingsDirName: TEST_SETTINGS_DIR,
504+
isTokens: isTestTokens,
505+
lockTimeoutMs: 100,
506+
});
507+
const lockPath = `${store.authPath(home)}.lock`;
508+
await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
509+
// The NEW pid:counter claim format held by this live process. Never
510+
// signal it; the waiter must read the pid leg as alive, time out, and
511+
// leave the claim byte-identical.
512+
const claim = `${process.pid}:42`;
513+
await writeFile(lockPath, claim, { mode: 0o600 });
514+
515+
await expect(
516+
store.saveProfile(
517+
{
518+
name: "work",
519+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
520+
createdAt: 1,
521+
},
522+
home,
523+
),
524+
).rejects.toThrow(
525+
`Timed out waiting for OAuth credential lock ${lockPath}. ` +
526+
"If no Corbits process is running, remove this lock file manually and retry.",
527+
);
528+
expect(await readFile(lockPath, "utf8")).toBe(claim);
529+
} finally {
530+
await rm(home, { recursive: true, force: true });
531+
}
532+
});
533+
534+
test("takes over a stale legacy lock but waits on a fresh one", async () => {
535+
const home = await mkdtemp(join(tmpdir(), "oauth-store-legacy-"));
536+
try {
537+
const staleStore = createAuthStore<TestTokens>({
538+
filename: "stale-auth.json",
539+
settingsDirName: TEST_SETTINGS_DIR,
540+
isTokens: isTestTokens,
541+
});
542+
const staleLockPath = `${staleStore.authPath(home)}.lock`;
543+
await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
544+
await writeFile(staleLockPath, "legacy-orphan", { mode: 0o600 });
545+
await utimes(staleLockPath, new Date(), new Date(Date.now() - 60_000));
546+
547+
const profile = {
548+
name: "work",
549+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
550+
createdAt: 1,
551+
};
552+
await expect(
553+
staleStore.saveProfile(profile, home),
554+
).resolves.toBeUndefined();
555+
expect(await staleStore.loadProfile("work", home)).toEqual(profile);
556+
557+
const freshStore = createAuthStore<TestTokens>({
558+
filename: "fresh-auth.json",
559+
settingsDirName: TEST_SETTINGS_DIR,
560+
isTokens: isTestTokens,
561+
lockTimeoutMs: 100,
562+
});
563+
const freshLockPath = `${freshStore.authPath(home)}.lock`;
564+
await writeFile(freshLockPath, "legacy-orphan", { mode: 0o600 });
565+
await expect(freshStore.saveProfile(profile, home)).rejects.toThrow(
566+
"Timed out waiting for OAuth credential lock",
567+
);
568+
expect(await readFile(freshLockPath, "utf8")).toBe("legacy-orphan");
569+
} finally {
570+
await rm(home, { recursive: true, force: true });
571+
}
572+
});
573+
574+
test("saves when a contended lock vanishes mid-wait", async () => {
575+
const home = await mkdtemp(join(tmpdir(), "oauth-store-vanish-"));
576+
try {
577+
const store = createAuthStore<TestTokens>({
578+
filename: "test-auth.json",
579+
settingsDirName: TEST_SETTINGS_DIR,
580+
isTokens: isTestTokens,
581+
});
582+
const lockPath = `${store.authPath(home)}.lock`;
583+
await mkdir(join(home, TEST_SETTINGS_DIR), { recursive: true });
584+
await writeFile(lockPath, `${2_147_483_647}`, { mode: 0o600 });
585+
586+
// Yank the stale lock out from under the waiter: whether the waiter
587+
// observes the dead PID, an ENOENT read, or an ENOENT unlink, it must
588+
// retry the exclusive create and land the save — never throw ENOENT.
589+
const pending = store.saveProfile(
590+
{
591+
name: "work",
592+
tokens: { access: "a", refresh: "r", expiresAt: 1 },
593+
createdAt: 1,
594+
},
595+
home,
596+
);
597+
await rm(lockPath, { force: true });
598+
await expect(pending).resolves.toBeUndefined();
599+
expect((await store.loadProfile("work", home))?.tokens.access).toBe("a");
600+
} finally {
601+
await rm(home, { recursive: true, force: true });
602+
}
603+
});
604+
331605
test("fails closed with manual recovery guidance when an orphan lock exists", async () => {
332606
const home = await mkdtemp(join(tmpdir(), "oauth-store-orphan-"));
333607
try {

0 commit comments

Comments
 (0)