Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions src/auth/store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,101 @@ describe("createAuthStore", () => {
}
});

test("queues same-process profile writes so neither save is lost", async () => {
const home = await mkdtemp(join(tmpdir(), "oauth-store-same-process-"));
try {
const store = createAuthStore<TestTokens>({
filename: "test-auth.json",
settingsDirName: TEST_SETTINGS_DIR,
isTokens: isTestTokens,
});

await Promise.all([
store.saveProfile(
{
name: "personal",
tokens: { access: "p", refresh: "pr", expiresAt: 1 },
createdAt: 1,
},
home,
),
store.saveProfile(
{
name: "work",
tokens: { access: "w", refresh: "wr", expiresAt: 2 },
createdAt: 2,
},
home,
),
]);

const profiles = await store.listProfiles(home);
expect(profiles.map((profile) => profile.name)).toEqual([
"personal",
"work",
]);
expect(profiles.find((profile) => profile.name === "personal")).toEqual({
name: "personal",
tokens: { access: "p", refresh: "pr", expiresAt: 1 },
createdAt: 1,
});
expect(profiles.find((profile) => profile.name === "work")).toEqual({
name: "work",
tokens: { access: "w", refresh: "wr", expiresAt: 2 },
createdAt: 2,
});
} finally {
await rm(home, { recursive: true, force: true });
}
});

test("gives queued same-process writes their own lock window", async () => {
const home = await mkdtemp(join(tmpdir(), "oauth-store-queue-"));
try {
const store = createAuthStore<TestTokens>({
filename: "test-auth.json",
settingsDirName: TEST_SETTINGS_DIR,
isTokens: isTestTokens,
});
await store.saveProfile(
{
name: "work",
tokens: { access: "a", refresh: "r", expiresAt: 1 },
createdAt: 1,
},
home,
);

// Hold the lock until the head of the same-process queue times out; the
// queued write must still get its own lock window after we release.
const lockPath = `${store.authPath(home)}.lock`;
await writeFile(lockPath, "foreign", { mode: 0o600 });

const first = store.updateTokens(
"work",
{ access: "first", refresh: "r1", expiresAt: 2 },
home,
);
const second = store.updateTokens(
"work",
{ access: "second", refresh: "r2", expiresAt: 3 },
home,
);

await expect(first).rejects.toThrow(
"Timed out waiting for OAuth credential lock",
);
await rm(lockPath, { force: true });
await expect(second).resolves.toBeUndefined();

expect((await store.loadProfile("work", home))?.tokens.access).toBe(
"second",
);
} finally {
await rm(home, { recursive: true, force: true });
}
});

test("round-trips profiles under an injected home and survives corrupt files", async () => {
const home = await mkdtemp(join(tmpdir(), "oauth-store-"));
try {
Expand Down
37 changes: 33 additions & 4 deletions src/auth/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ interface AuthFile<TTokens extends BaseTokens> {
const LOCK_RETRY_MS = 25;
const LOCK_TIMEOUT_MS = 1_000;

// pid alone is not unique per call — concurrent saves in one process must not
// share a temp path or the second rename hits ENOENT after the first moves it.
let tmpWriteCounter = 0;

// Same-process ops on one auth file queue here so a caller's lock deadline
// starts when it actually runs, not when it was invoked — otherwise one lock
// held past LOCK_TIMEOUT_MS fails the whole burst, not just the first waiter.
const updateChains = new Map<string, Promise<unknown>>();

const AuthFileShape = type({
profiles: "Record<string, unknown>",
});
Expand Down Expand Up @@ -115,7 +124,7 @@ export function createAuthStore<TTokens extends BaseTokens>(
): Promise<void> {
const path = authPath(home);
await mkdir(dirname(path), { recursive: true, mode: 0o700 });
const tmp = `${path}.${String(process.pid)}.tmp`;
const tmp = `${path}.${process.pid}.${(tmpWriteCounter += 1)}.tmp`;
await writeFile(tmp, JSON.stringify(file, null, 2), { mode: 0o600 });
await rename(tmp, path);
}
Expand Down Expand Up @@ -158,6 +167,26 @@ export function createAuthStore<TTokens extends BaseTokens>(
}
}

function enqueueAuthFileOp<TResult>(
home: string,
op: () => Promise<TResult>,
): Promise<TResult> {
const path = authPath(home);
const previous = updateChains.get(path) ?? Promise.resolve();
const run = previous.then(
() => withAuthFileLock(home, op),
() => withAuthFileLock(home, op),
);
updateChains.set(
path,
run.then(
() => undefined,
() => undefined,
),
);
return run;
}

return {
authPath,
async listProfiles(
Expand All @@ -179,7 +208,7 @@ export function createAuthStore<TTokens extends BaseTokens>(
profile: AuthProfile<TTokens>,
home: string = homedir(),
): Promise<void> {
await withAuthFileLock(home, async () => {
await enqueueAuthFileOp(home, async () => {
const file = await readAuthFile(home);
file.profiles[profile.name] = profile;
await writeAuthFile(file, home);
Expand All @@ -192,7 +221,7 @@ export function createAuthStore<TTokens extends BaseTokens>(
tokens: TTokens,
home: string = homedir(),
): Promise<void> {
await withAuthFileLock(home, async () => {
await enqueueAuthFileOp(home, async () => {
const file = await readAuthFile(home);
const existing = file.profiles[name];
if (existing === undefined) return;
Expand All @@ -204,7 +233,7 @@ export function createAuthStore<TTokens extends BaseTokens>(
name: string | undefined,
home: string = homedir(),
): Promise<string[]> {
return withAuthFileLock(home, async () => {
return enqueueAuthFileOp(home, async () => {
const file = await readAuthFile(home);
if (name === undefined) {
const removed = Object.keys(file.profiles);
Expand Down
Loading