Skip to content

Commit e5b5b36

Browse files
ndycodendycode
committed
Fix backup pruning review follow-ups
1 parent bbadd2d commit e5b5b36

File tree

5 files changed

+300
-19
lines changed

5 files changed

+300
-19
lines changed

‎index.ts‎

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3444,9 +3444,17 @@ while (attempted.size < Math.max(1, accountCount)) {
34443444
}
34453445
};
34463446

3447-
const pruneOldSyncPruneBackups = async (
3447+
const pruneTimestampedBackups = async (
34483448
backupDir: string,
3449-
keepPaths: string[] = [],
3449+
{
3450+
prefix,
3451+
keepPaths = [],
3452+
logLabel,
3453+
}: {
3454+
prefix: string;
3455+
keepPaths?: string[];
3456+
logLabel: string;
3457+
},
34503458
): Promise<void> => {
34513459
const entries = await fsPromises.readdir(backupDir, { withFileTypes: true }).catch((error) => {
34523460
const code = (error as NodeJS.ErrnoException).code;
@@ -3465,7 +3473,7 @@ while (attempted.size < Math.max(1, accountCount)) {
34653473
.filter(
34663474
(entry) =>
34673475
entry.isFile() &&
3468-
entry.name.startsWith(`${SYNC_PRUNE_BACKUP_PREFIX}-`) &&
3476+
entry.name.startsWith(`${prefix}-`) &&
34693477
entry.name.endsWith(".json"),
34703478
)
34713479
.map(async (entry) => {
@@ -3491,7 +3499,12 @@ while (attempted.size < Math.max(1, accountCount)) {
34913499
)
34923500
)
34933501
.filter((candidate): candidate is { path: string; mtimeMs: number } => candidate !== null)
3494-
.sort((left, right) => right.path.localeCompare(left.path));
3502+
.sort((left, right) => {
3503+
if (right.mtimeMs !== left.mtimeMs) {
3504+
return right.mtimeMs - left.mtimeMs;
3505+
}
3506+
return left.path.localeCompare(right.path);
3507+
});
34953508
const staleBackupPaths = [
34963509
...backupCandidates
34973510
.filter((candidate) => now - candidate.mtimeMs > SYNC_PRUNE_BACKUP_MAX_AGE_MS)
@@ -3512,12 +3525,32 @@ while (attempted.size < Math.max(1, accountCount)) {
35123525
}
35133526
const message = error instanceof Error ? error.message : String(error);
35143527
logWarn(
3515-
`[${PLUGIN_NAME}] Failed to prune stale sync prune backup ${staleBackupPath}: ${message}`,
3528+
`[${PLUGIN_NAME}] Failed to prune stale ${logLabel} ${staleBackupPath}: ${message}`,
35163529
);
35173530
}
35183531
}
35193532
};
35203533

3534+
const pruneOldSyncPruneBackups = async (
3535+
backupDir: string,
3536+
keepPaths: string[] = [],
3537+
): Promise<void> =>
3538+
pruneTimestampedBackups(backupDir, {
3539+
prefix: SYNC_PRUNE_BACKUP_PREFIX,
3540+
keepPaths,
3541+
logLabel: "sync prune backup",
3542+
});
3543+
3544+
const pruneOldOverlapCleanupBackups = async (
3545+
backupDir: string,
3546+
keepPaths: string[] = [],
3547+
): Promise<void> =>
3548+
pruneTimestampedBackups(backupDir, {
3549+
prefix: "codex-maintenance-overlap-backup",
3550+
keepPaths,
3551+
logLabel: "overlap cleanup backup",
3552+
});
3553+
35213554
const createSyncPruneBackup = async (): Promise<{
35223555
backupPath: string;
35233556
restore: () => Promise<void>;
@@ -3974,6 +4007,16 @@ while (attempted.size < Math.max(1, accountCount)) {
39744007
backupPath = createTimestampedBackupPath("codex-maintenance-overlap-backup");
39754008
const result = await cleanupCodexMultiAuthSyncedOverlaps(backupPath);
39764009
invalidateAccountManagerCache();
4010+
if (backupPath) {
4011+
const backupDir = dirname(backupPath);
4012+
await pruneOldOverlapCleanupBackups(backupDir, [backupPath]).catch((cleanupError) => {
4013+
const cleanupMessage =
4014+
cleanupError instanceof Error ? cleanupError.message : String(cleanupError);
4015+
logWarn(
4016+
`[${PLUGIN_NAME}] Failed to prune old overlap cleanup backups in ${backupDir}: ${cleanupMessage}`,
4017+
);
4018+
});
4019+
}
39774020
console.log("");
39784021
console.log("Cleanup complete.");
39794022
console.log(`Before: ${result.before}`);

‎lib/sync-prune-backup.ts‎

Lines changed: 59 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,33 @@ type TokenBearingRecord = {
1515
idToken?: string;
1616
};
1717

18-
function redactCredentialRecord<TAccount extends object>(account: TAccount): TAccount {
18+
type ReplaceTokenField<
19+
TAccount extends object,
20+
TKey extends keyof TokenBearingRecord,
21+
TValue,
22+
> = TKey extends keyof TAccount
23+
? undefined extends TAccount[TKey]
24+
? Omit<TAccount, TKey> & { [K in TKey]?: TValue }
25+
: Omit<TAccount, TKey> & { [K in TKey]: TValue }
26+
: TAccount;
27+
28+
export type TokenRedacted<TAccount extends object> = ReplaceTokenField<
29+
ReplaceTokenField<ReplaceTokenField<TAccount, "refreshToken", "__redacted__">, "accessToken", undefined>,
30+
"idToken",
31+
undefined
32+
>;
33+
34+
type RedactedAccountStorage = Omit<AccountStorageV3, "accounts"> & {
35+
accounts: Array<TokenRedacted<AccountStorageV3["accounts"][number]>>;
36+
};
37+
38+
type SyncPruneBackupPayload<TAccountsStorage extends AccountStorageV3, TFlaggedAccount extends object> = {
39+
version: 1;
40+
accounts: TAccountsStorage;
41+
flagged: FlaggedSnapshot<TFlaggedAccount>;
42+
};
43+
44+
function redactCredentialRecord<TAccount extends object>(account: TAccount): TokenRedacted<TAccount> {
1945
const clone = structuredClone(account) as TAccount & TokenBearingRecord;
2046
if (typeof clone.refreshToken === "string") {
2147
clone.refreshToken = "__redacted__";
@@ -26,30 +52,51 @@ function redactCredentialRecord<TAccount extends object>(account: TAccount): TAc
2652
if ("idToken" in clone) {
2753
clone.idToken = undefined;
2854
}
29-
return clone;
55+
return clone as TokenRedacted<TAccount>;
3056
}
3157

58+
export function createSyncPruneBackupPayload<TFlaggedAccount extends object>(
59+
currentAccountsStorage: AccountStorageV3,
60+
currentFlaggedStorage: FlaggedSnapshot<TFlaggedAccount>,
61+
options?: { includeLiveTokens?: false | undefined },
62+
): SyncPruneBackupPayload<RedactedAccountStorage, TokenRedacted<TFlaggedAccount>>;
63+
export function createSyncPruneBackupPayload<TFlaggedAccount extends object>(
64+
currentAccountsStorage: AccountStorageV3,
65+
currentFlaggedStorage: FlaggedSnapshot<TFlaggedAccount>,
66+
options: { includeLiveTokens: true },
67+
): SyncPruneBackupPayload<AccountStorageV3, TFlaggedAccount>;
3268
export function createSyncPruneBackupPayload<TFlaggedAccount extends object>(
3369
currentAccountsStorage: AccountStorageV3,
3470
currentFlaggedStorage: FlaggedSnapshot<TFlaggedAccount>,
3571
options: SyncPruneBackupPayloadOptions = {},
36-
): {
37-
version: 1;
38-
accounts: AccountStorageV3;
39-
flagged: FlaggedSnapshot<TFlaggedAccount>;
40-
} {
72+
):
73+
| SyncPruneBackupPayload<RedactedAccountStorage, TokenRedacted<TFlaggedAccount>>
74+
| SyncPruneBackupPayload<AccountStorageV3, TFlaggedAccount> {
4175
const accounts = structuredClone(currentAccountsStorage);
4276
const flagged = structuredClone(currentFlaggedStorage);
43-
if (!options.includeLiveTokens) {
44-
accounts.accounts = accounts.accounts.map((account) => redactCredentialRecord(account));
45-
flagged.accounts = flagged.accounts.map((account) => redactCredentialRecord(account));
77+
if (options.includeLiveTokens) {
78+
return {
79+
version: 1,
80+
accounts,
81+
flagged,
82+
};
4683
}
84+
85+
const redactedAccounts: RedactedAccountStorage = {
86+
...accounts,
87+
accounts: accounts.accounts.map((account) => redactCredentialRecord(account)),
88+
};
89+
const redactedFlagged: FlaggedSnapshot<TokenRedacted<TFlaggedAccount>> = {
90+
...flagged,
91+
accounts: flagged.accounts.map((account) => redactCredentialRecord(account)),
92+
};
93+
4794
// Callers opt into live tokens only when crash recovery must fully restore pruned accounts.
4895
// On Windows the eventual file write still relies on config-home ACLs because `mode: 0o600`
4996
// is only a best-effort hint there.
5097
return {
5198
version: 1,
52-
accounts,
53-
flagged,
99+
accounts: redactedAccounts,
100+
flagged: redactedFlagged,
54101
};
55102
}

‎test/index.test.ts‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2167,11 +2167,89 @@ describe("OpenAIOAuthPlugin", () => {
21672167
}
21682168
});
21692169

2170+
it("prunes stale overlap cleanup backups after a successful cleanup", async () => {
2171+
const cliModule = await import("../lib/cli.js");
2172+
const confirmModule = await import("../lib/ui/confirm.js");
2173+
const syncModule = await import("../lib/codex-multi-auth-sync.js");
2174+
const { promises: nodeFsPromises } = await import("node:fs");
2175+
const logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
2176+
2177+
mockStorage.accounts = [
2178+
{
2179+
accountId: "existing-account",
2180+
email: "existing@example.com",
2181+
refreshToken: "existing-refresh",
2182+
addedAt: 1,
2183+
lastUsed: 1,
2184+
},
2185+
];
2186+
2187+
vi.mocked(cliModule.promptLoginMode)
2188+
.mockResolvedValueOnce({ mode: "experimental-cleanup-overlaps" })
2189+
.mockResolvedValueOnce({ mode: "cancel" })
2190+
.mockResolvedValue({ mode: "cancel" });
2191+
vi.mocked(confirmModule.confirm).mockResolvedValueOnce(true);
2192+
vi.mocked(syncModule.previewCodexMultiAuthSyncedOverlapCleanup).mockResolvedValueOnce({
2193+
before: 3,
2194+
after: 2,
2195+
removed: 1,
2196+
updated: 0,
2197+
});
2198+
vi.mocked(syncModule.cleanupCodexMultiAuthSyncedOverlaps).mockResolvedValueOnce({
2199+
before: 3,
2200+
after: 2,
2201+
removed: 1,
2202+
updated: 0,
2203+
});
2204+
2205+
const staleBackupPath = "/tmp/codex-maintenance-overlap-backup-20240201-000000.json";
2206+
const normalizePath = (value: string) => value.replace(/\\/g, "/");
2207+
const readdirSpy = vi.spyOn(nodeFsPromises, "readdir").mockResolvedValue([
2208+
{
2209+
name: "codex-maintenance-overlap-backup-20260101-000000.json",
2210+
isFile: () => true,
2211+
},
2212+
{
2213+
name: "codex-maintenance-overlap-backup-20240201-000000.json",
2214+
isFile: () => true,
2215+
},
2216+
] as never);
2217+
const statSpy = vi.spyOn(nodeFsPromises, "stat").mockImplementation(async (path) => {
2218+
return {
2219+
mtimeMs:
2220+
normalizePath(String(path)) === staleBackupPath ? Date.now() - 8 * 24 * 60 * 60 * 1000 : Date.now(),
2221+
} as never;
2222+
});
2223+
const unlinkSpy = vi.spyOn(nodeFsPromises, "unlink").mockResolvedValue(undefined);
2224+
2225+
try {
2226+
const autoMethod = plugin.auth.methods[0] as unknown as {
2227+
authorize: (inputs?: Record<string, string>) => Promise<{ instructions: string }>;
2228+
};
2229+
2230+
const result = await autoMethod.authorize();
2231+
expect(result.instructions).toBe("Authentication cancelled");
2232+
expect(vi.mocked(syncModule.cleanupCodexMultiAuthSyncedOverlaps)).toHaveBeenCalledWith(
2233+
"/tmp/codex-maintenance-overlap-backup-20260101-000000.json",
2234+
);
2235+
expect(unlinkSpy.mock.calls.map(([path]) => normalizePath(String(path)))).toEqual([staleBackupPath]);
2236+
2237+
const output = logSpy.mock.calls.flat().join("\n");
2238+
expect(output).toContain("Cleanup complete.");
2239+
} finally {
2240+
logSpy.mockRestore();
2241+
readdirSpy.mockRestore();
2242+
statSpy.mockRestore();
2243+
unlinkSpy.mockRestore();
2244+
}
2245+
});
2246+
21702247
it("writes a restorable sync prune backup before removing accounts", async () => {
21712248
const cliModule = await import("../lib/cli.js");
21722249
const confirmModule = await import("../lib/ui/confirm.js");
21732250
const configModule = await import("../lib/config.js");
21742251
const syncModule = await import("../lib/codex-multi-auth-sync.js");
2252+
const storageModule = await import("../lib/storage.js");
21752253
const { promises: nodeFsPromises } = await import("node:fs");
21762254

21772255
mockStorage.accounts = [
@@ -2218,6 +2296,9 @@ describe("OpenAIOAuthPlugin", () => {
22182296
.mockResolvedValue({ mode: "cancel" });
22192297
vi.mocked(cliModule.promptCodexMultiAuthSyncPrune).mockResolvedValueOnce([0]);
22202298
vi.mocked(confirmModule.confirm).mockResolvedValue(true);
2299+
vi.mocked(storageModule.createTimestampedBackupPath).mockImplementation(
2300+
(prefix?: string) => `\\tmp\\${prefix ?? "codex-backup"}-20260101-000000.json`,
2301+
);
22212302
vi.mocked(syncModule.previewSyncFromCodexMultiAuth)
22222303
.mockRejectedValueOnce(
22232304
new CodexMultiAuthSyncCapacityError({
@@ -2262,6 +2343,53 @@ describe("OpenAIOAuthPlugin", () => {
22622343
.spyOn(nodeFsPromises, "rename")
22632344
.mockRejectedValueOnce(Object.assign(new Error("rename locked"), { code: "EPERM" }))
22642345
.mockResolvedValueOnce(undefined);
2346+
const normalizePath = (value: string) => value.replace(/\\/g, "/");
2347+
const now = Date.now();
2348+
const statTimes = new Map<string, number>([
2349+
["/tmp/codex-sync-prune-backup-z.json", now - 3_000],
2350+
["/tmp/codex-sync-prune-backup-y.json", now - 2_000],
2351+
["/tmp/codex-sync-prune-backup-a.json", now - 1_000],
2352+
]);
2353+
const readdirSpy = vi
2354+
.spyOn(nodeFsPromises, "readdir")
2355+
.mockResolvedValueOnce([
2356+
{
2357+
name: "codex-sync-prune-backup-20260101-000000.json",
2358+
isFile: () => true,
2359+
},
2360+
{
2361+
name: "codex-sync-prune-backup-z.json",
2362+
isFile: () => true,
2363+
},
2364+
{
2365+
name: "codex-sync-prune-backup-y.json",
2366+
isFile: () => true,
2367+
},
2368+
{
2369+
name: "codex-sync-prune-backup-a.json",
2370+
isFile: () => true,
2371+
},
2372+
] as never)
2373+
.mockResolvedValueOnce([
2374+
{
2375+
name: "codex-sync-prune-backup-z.json",
2376+
isFile: () => true,
2377+
},
2378+
{
2379+
name: "codex-sync-prune-backup-y.json",
2380+
isFile: () => true,
2381+
},
2382+
{
2383+
name: "codex-sync-prune-backup-a.json",
2384+
isFile: () => true,
2385+
},
2386+
] as never);
2387+
const statSpy = vi.spyOn(nodeFsPromises, "stat").mockImplementation(async (path) => {
2388+
return {
2389+
mtimeMs: statTimes.get(normalizePath(String(path))) ?? Date.now(),
2390+
} as never;
2391+
});
2392+
const unlinkSpy = vi.spyOn(nodeFsPromises, "unlink").mockResolvedValue(undefined);
22652393

22662394
try {
22672395
const autoMethod = plugin.auth.methods[0] as unknown as {
@@ -2285,10 +2413,18 @@ describe("OpenAIOAuthPlugin", () => {
22852413
expect(mockFlaggedStorage.accounts).toHaveLength(0);
22862414
expect(renameSpy).toHaveBeenCalledTimes(2);
22872415
expect(mkdirSpy).toHaveBeenCalled();
2416+
const normalizedUnlinks = unlinkSpy.mock.calls.map(([path]) => normalizePath(String(path)));
2417+
expect(normalizedUnlinks).toContain("/tmp/codex-sync-prune-backup-20260101-000000.json");
2418+
expect(normalizedUnlinks.filter((path) => path.endsWith("codex-sync-prune-backup-z.json"))).toHaveLength(2);
2419+
expect(normalizedUnlinks.some((path) => path.endsWith("codex-sync-prune-backup-a.json"))).toBe(false);
2420+
expect(normalizedUnlinks.some((path) => path.endsWith("codex-sync-prune-backup-y.json"))).toBe(false);
22882421
} finally {
22892422
mkdirSpy.mockRestore();
22902423
writeSpy.mockRestore();
22912424
renameSpy.mockRestore();
2425+
readdirSpy.mockRestore();
2426+
statSpy.mockRestore();
2427+
unlinkSpy.mockRestore();
22922428
}
22932429
});
22942430

‎test/storage.test.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1986,6 +1986,10 @@ describe("storage", () => {
19861986
firstTransactionReady = resolve;
19871987
});
19881988
let secondEntered = false;
1989+
let resolveSecondStarted!: () => void;
1990+
const secondStarted = new Promise<void>((resolve) => {
1991+
resolveSecondStarted = resolve;
1992+
});
19891993

19901994
const firstTransaction = withAccountAndFlaggedStorageTransaction(async (current, persist) => {
19911995
events.push("first:start");
@@ -2026,9 +2030,17 @@ describe("storage", () => {
20262030
const secondTransaction = withAccountAndFlaggedStorageTransaction(async () => {
20272031
secondEntered = true;
20282032
events.push("second:start");
2033+
resolveSecondStarted();
20292034
});
20302035

2031-
await new Promise((resolve) => setTimeout(resolve, 25));
2036+
const secondProgress = await Promise.race([
2037+
secondStarted.then(() => "started" as const),
2038+
Promise.resolve()
2039+
.then(() => Promise.resolve())
2040+
.then(() => "waiting" as const),
2041+
]);
2042+
2043+
expect(secondProgress).toBe("waiting");
20322044
expect(secondEntered).toBe(false);
20332045
expect(events).toEqual(["first:start", "first:after-flagged"]);
20342046

0 commit comments

Comments
 (0)