Skip to content

Commit 81c45e5

Browse files
ndycodendycode
committed
fix: serialize concurrent runtime cache writes
Quota cache saves and update-check cache writes could overlap within the same process. That risks write races, and the update checker also wrote directly to the final cache file instead of staging through a temp file. Serialize in-process writes for both caches and make update-check cache writes atomic via temp-file rename so concurrent runtime writers do not corrupt the on-disk state.
1 parent a607a74 commit 81c45e5

3 files changed

Lines changed: 127 additions & 63 deletions

File tree

‎lib/auto-update-checker.ts‎

Lines changed: 54 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { readFileSync, writeFileSync, existsSync, mkdirSync } from "node:fs";
1+
import { readFileSync, writeFileSync, existsSync, mkdirSync, renameSync, unlinkSync } from "node:fs";
22
import { join } from "node:path";
33
import { createLogger } from "./logger.js";
44
import { getCodexCacheDir } from "./runtime-paths.js";
@@ -28,6 +28,7 @@ interface ParsedSemver {
2828
}
2929

3030
const RETRYABLE_WRITE_ERRORS = new Set(["EBUSY", "EPERM"]);
31+
let updateCacheWriteQueue: Promise<void> = Promise.resolve();
3132

3233
function sleepSync(ms: number): void {
3334
const delay = Math.max(0, Math.floor(ms));
@@ -56,30 +57,53 @@ function loadCache(): UpdateCheckCache | null {
5657
}
5758
}
5859

59-
function saveCache(cache: UpdateCheckCache): void {
60-
try {
61-
if (!existsSync(CACHE_DIR)) {
62-
mkdirSync(CACHE_DIR, { recursive: true });
63-
}
64-
const serialized = JSON.stringify(cache, null, 2);
65-
let lastError: Error | null = null;
66-
for (let attempt = 0; attempt < 4; attempt++) {
67-
try {
68-
writeFileSync(CACHE_FILE, serialized, "utf8");
69-
return;
70-
} catch (error) {
71-
const code = (error as NodeJS.ErrnoException).code ?? "";
72-
lastError = error as Error;
73-
if (!RETRYABLE_WRITE_ERRORS.has(code) || attempt >= 3) {
74-
throw error;
75-
}
76-
sleepSync(15 * (2 ** attempt));
77-
}
78-
}
79-
if (lastError) throw lastError;
80-
} catch (error) {
81-
log.warn("Failed to save update cache", { error: (error as Error).message });
82-
}
60+
async function saveCache(cache: UpdateCheckCache): Promise<void> {
61+
const writeTask = (): void => {
62+
let tempPath: string | null = null;
63+
let wroteTemp = false;
64+
try {
65+
if (!existsSync(CACHE_DIR)) {
66+
mkdirSync(CACHE_DIR, { recursive: true });
67+
}
68+
const serialized = JSON.stringify(cache, null, 2);
69+
tempPath = `${CACHE_FILE}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`;
70+
let lastError: Error | null = null;
71+
for (let attempt = 0; attempt < 4; attempt++) {
72+
try {
73+
writeFileSync(tempPath, serialized, "utf8");
74+
renameSync(tempPath, CACHE_FILE);
75+
wroteTemp = false;
76+
return;
77+
} catch (error) {
78+
const code = (error as NodeJS.ErrnoException).code ?? "";
79+
lastError = error as Error;
80+
wroteTemp = true;
81+
if (!RETRYABLE_WRITE_ERRORS.has(code) || attempt >= 3) {
82+
throw error;
83+
}
84+
sleepSync(15 * (2 ** attempt));
85+
}
86+
}
87+
if (lastError) throw lastError;
88+
} catch (error) {
89+
log.warn("Failed to save update cache", { error: (error as Error).message });
90+
} finally {
91+
if (wroteTemp && tempPath) {
92+
try {
93+
unlinkSync(tempPath);
94+
} catch {
95+
// Best effort temp cleanup.
96+
}
97+
}
98+
}
99+
};
100+
101+
const queued = updateCacheWriteQueue.catch(() => undefined).then(writeTask);
102+
updateCacheWriteQueue = queued.then(
103+
() => undefined,
104+
() => undefined,
105+
);
106+
await queued;
83107
}
84108

85109
function parseSemver(version: string): ParsedSemver {
@@ -211,11 +235,11 @@ export async function checkForUpdates(force = false): Promise<UpdateCheckResult>
211235

212236
const latestVersion = await fetchLatestVersion();
213237

214-
saveCache({
215-
lastCheck: now,
216-
latestVersion,
217-
currentVersion,
218-
});
238+
await saveCache({
239+
lastCheck: now,
240+
latestVersion,
241+
currentVersion,
242+
});
219243

220244
const hasUpdate = latestVersion ? compareVersions(currentVersion, latestVersion) > 0 : false;
221245

‎lib/quota-cache.ts‎

Lines changed: 40 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ interface QuotaCacheFile {
3333
const QUOTA_CACHE_PATH = join(getCodexMultiAuthDir(), "quota-cache.json");
3434
const QUOTA_CACHE_LABEL = basename(QUOTA_CACHE_PATH);
3535
const RETRYABLE_FS_CODES = new Set(["EBUSY", "EPERM"]);
36+
let quotaCacheWriteQueue: Promise<void> = Promise.resolve();
3637

3738
function isRetryableFsError(error: unknown): boolean {
3839
const code = (error as NodeJS.ErrnoException | undefined)?.code;
@@ -223,39 +224,48 @@ export async function saveQuotaCache(data: QuotaCacheData): Promise<void> {
223224
byEmail: data.byEmail,
224225
};
225226

226-
try {
227-
await fs.mkdir(getCodexMultiAuthDir(), { recursive: true });
228-
const tempPath = `${QUOTA_CACHE_PATH}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`;
229-
await fs.writeFile(tempPath, `${JSON.stringify(payload, null, 2)}\n`, {
230-
encoding: "utf8",
231-
mode: 0o600,
232-
});
233-
let renamed = false;
227+
const writeTask = async (): Promise<void> => {
234228
try {
235-
for (let attempt = 0; attempt < 5; attempt += 1) {
236-
try {
237-
await fs.rename(tempPath, QUOTA_CACHE_PATH);
238-
renamed = true;
239-
break;
240-
} catch (error) {
241-
if (!isRetryableFsError(error) || attempt >= 4) throw error;
242-
await sleep(10 * 2 ** attempt);
229+
await fs.mkdir(getCodexMultiAuthDir(), { recursive: true });
230+
const tempPath = `${QUOTA_CACHE_PATH}.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2, 8)}.tmp`;
231+
await fs.writeFile(tempPath, `${JSON.stringify(payload, null, 2)}\n`, {
232+
encoding: "utf8",
233+
mode: 0o600,
234+
});
235+
let renamed = false;
236+
try {
237+
for (let attempt = 0; attempt < 5; attempt += 1) {
238+
try {
239+
await fs.rename(tempPath, QUOTA_CACHE_PATH);
240+
renamed = true;
241+
break;
242+
} catch (error) {
243+
if (!isRetryableFsError(error) || attempt >= 4) throw error;
244+
await sleep(10 * 2 ** attempt);
245+
}
243246
}
244-
}
245-
} finally {
246-
if (!renamed) {
247-
try {
248-
await fs.unlink(tempPath);
249-
} catch {
250-
// Best effort temp cleanup.
247+
} finally {
248+
if (!renamed) {
249+
try {
250+
await fs.unlink(tempPath);
251+
} catch {
252+
// Best effort temp cleanup.
253+
}
251254
}
252255
}
256+
} catch (error) {
257+
logWarn(
258+
`Failed to save quota cache to ${QUOTA_CACHE_LABEL}: ${
259+
error instanceof Error ? error.message : String(error)
260+
}`,
261+
);
253262
}
254-
} catch (error) {
255-
logWarn(
256-
`Failed to save quota cache to ${QUOTA_CACHE_LABEL}: ${
257-
error instanceof Error ? error.message : String(error)
258-
}`,
259-
);
260-
}
263+
};
264+
265+
const queued = quotaCacheWriteQueue.catch(() => undefined).then(writeTask);
266+
quotaCacheWriteQueue = queued.then(
267+
() => undefined,
268+
() => undefined,
269+
);
270+
await queued;
261271
}

‎test/auto-update-checker.test.ts‎

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ vi.mock("node:fs", () => ({
55
writeFileSync: vi.fn(),
66
existsSync: vi.fn(),
77
mkdirSync: vi.fn(),
8+
renameSync: vi.fn(),
9+
unlinkSync: vi.fn(),
810
}));
911

1012
describe("auto-update-checker", () => {
@@ -358,11 +360,13 @@ describe("auto-update-checker", () => {
358360
await checkForUpdates(true);
359361

360362
expect(fs.writeFileSync).toHaveBeenCalled();
363+
expect(fs.renameSync).toHaveBeenCalled();
361364
const writeCall = vi.mocked(fs.writeFileSync).mock.calls[0];
362365
const savedData = JSON.parse(writeCall[1] as string) as {
363366
latestVersion: string;
364367
};
365368
expect(savedData.latestVersion).toBe("5.0.0");
369+
expect(String(writeCall[0])).toContain("update-check-cache.json.");
366370
});
367371

368372
it("creates cache directory if missing", async () => {
@@ -383,8 +387,8 @@ describe("auto-update-checker", () => {
383387
"retries cache writes when filesystem is transiently locked (%s)",
384388
async (code) => {
385389
let attempts = 0;
386-
vi.mocked(fs.writeFileSync).mockClear();
387-
vi.mocked(fs.writeFileSync).mockImplementation(() => {
390+
vi.mocked(fs.renameSync).mockClear();
391+
vi.mocked(fs.renameSync).mockImplementation(() => {
388392
attempts += 1;
389393
if (attempts < 3) {
390394
const error = new Error("busy") as NodeJS.ErrnoException;
@@ -400,10 +404,36 @@ describe("auto-update-checker", () => {
400404

401405
await checkForUpdates(true);
402406

403-
expect(fs.writeFileSync).toHaveBeenCalledTimes(3);
407+
expect(fs.renameSync).toHaveBeenCalledTimes(3);
404408
},
405409
);
406410

411+
it("serializes concurrent cache writes through temp-file renames", async () => {
412+
vi.mocked(fs.writeFileSync).mockClear();
413+
vi.mocked(fs.renameSync).mockClear();
414+
vi.mocked(globalThis.fetch)
415+
.mockResolvedValueOnce({
416+
ok: true,
417+
json: async () => ({ version: "5.0.0" }),
418+
} as Response)
419+
.mockResolvedValueOnce({
420+
ok: true,
421+
json: async () => ({ version: "5.0.1" }),
422+
} as Response);
423+
424+
await Promise.all([checkForUpdates(true), checkForUpdates(true)]);
425+
426+
expect(fs.renameSync).toHaveBeenCalledTimes(2);
427+
const writeTargets = vi
428+
.mocked(fs.writeFileSync)
429+
.mock.calls.map((call) => String(call[0]));
430+
expect(
431+
writeTargets.every((target) =>
432+
target.includes("update-check-cache.json."),
433+
),
434+
).toBe(true);
435+
});
436+
407437
it("includes updateCommand in result", async () => {
408438
vi.mocked(globalThis.fetch).mockResolvedValue({
409439
ok: true,

0 commit comments

Comments
 (0)