Skip to content

Commit b213a60

Browse files
ndycodendycode
committed
fix: address final sync review comments
1 parent 8bebcdf commit b213a60

5 files changed

Lines changed: 248 additions & 133 deletions

File tree

‎index.ts‎

Lines changed: 115 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ import {
126126
clearFlaggedAccounts,
127127
StorageError,
128128
formatStorageErrorHint,
129+
withAccountAndFlaggedStorageTransaction,
129130
withFlaggedAccountsTransaction,
130131
type AccountStorageV3,
131132
type FlaggedAccountMetadataV1,
@@ -3444,12 +3445,12 @@ while (attempted.size < Math.max(1, accountCount)) {
34443445
if (!normalizedAccounts) {
34453446
throw new Error("Prune backup account snapshot failed validation.");
34463447
}
3447-
await withAccountStorageTransaction(async (_current, persist) => {
3448-
await persist(normalizedAccounts);
3448+
await withAccountAndFlaggedStorageTransaction(async (_current, persist) => {
3449+
await persist.accounts(normalizedAccounts);
3450+
await persist.flagged(
3451+
restoreFlaggedSnapshot as { version: 1; accounts: FlaggedAccountMetadataV1[] },
3452+
);
34493453
});
3450-
await saveFlaggedAccounts(
3451-
restoreFlaggedSnapshot as { version: 1; accounts: FlaggedAccountMetadataV1[] },
3452-
);
34533454
invalidateAccountManagerCache();
34543455
},
34553456
};
@@ -3465,132 +3466,137 @@ while (attempted.size < Math.max(1, accountCount)) {
34653466
index: number;
34663467
account: AccountStorageV3["accounts"][number];
34673468
}> = [];
3468-
let rollbackStorage: AccountStorageV3 | null = null;
3469-
await withAccountStorageTransaction(async (loadedStorage, persist) => {
3470-
const currentStorage =
3471-
loadedStorage ??
3469+
await withAccountAndFlaggedStorageTransaction(
3470+
async ({ accounts: loadedStorage, flagged: currentFlaggedStorage }, persist) => {
3471+
const currentStorage =
3472+
loadedStorage ??
34723473
({
34733474
version: 3,
34743475
accounts: [],
34753476
activeIndex: 0,
34763477
activeIndexByFamily: {},
34773478
} satisfies AccountStorageV3);
3478-
removedTargets = currentStorage.accounts
3479-
.map((account, index) => ({ index, account }))
3480-
.filter((entry) =>
3481-
targetKeySet.has(
3479+
removedTargets = currentStorage.accounts
3480+
.map((account, index) => ({ index, account }))
3481+
.filter((entry) =>
3482+
targetKeySet.has(
3483+
getSyncRemovalTargetKey({
3484+
refreshToken: entry.account.refreshToken,
3485+
organizationId: entry.account.organizationId,
3486+
accountId: entry.account.accountId,
3487+
}),
3488+
),
3489+
);
3490+
if (removedTargets.length === 0) {
3491+
return;
3492+
}
3493+
3494+
const removedFlaggedKeys = new Set(
3495+
removedTargets.map((entry) =>
34823496
getSyncRemovalTargetKey({
34833497
refreshToken: entry.account.refreshToken,
34843498
organizationId: entry.account.organizationId,
34853499
accountId: entry.account.accountId,
34863500
}),
34873501
),
34883502
);
3489-
if (removedTargets.length === 0) {
3490-
return;
3491-
}
3492-
rollbackStorage = structuredClone(currentStorage);
3493-
3494-
const activeAccountIdentity = {
3495-
refreshToken:
3496-
currentStorage.accounts[currentStorage.activeIndex]?.refreshToken ?? "",
3497-
organizationId:
3498-
currentStorage.accounts[currentStorage.activeIndex]?.organizationId,
3499-
accountId: currentStorage.accounts[currentStorage.activeIndex]?.accountId,
3500-
} satisfies SyncRemovalTarget;
3501-
const familyActiveIdentities = Object.fromEntries(
3502-
MODEL_FAMILIES.map((family) => {
3503-
const familyIndex = currentStorage.activeIndexByFamily?.[family] ?? currentStorage.activeIndex;
3504-
const familyAccount = currentStorage.accounts[familyIndex];
3505-
return [
3506-
family,
3507-
familyAccount
3508-
? ({
3509-
refreshToken: familyAccount.refreshToken,
3510-
organizationId: familyAccount.organizationId,
3511-
accountId: familyAccount.accountId,
3512-
} satisfies SyncRemovalTarget)
3513-
: null,
3514-
];
3515-
}),
3516-
) as Partial<Record<ModelFamily, SyncRemovalTarget | null>>;
3517-
3518-
currentStorage.accounts = currentStorage.accounts.filter(
3519-
(account) =>
3520-
!targetKeySet.has(
3521-
getSyncRemovalTargetKey({
3522-
refreshToken: account.refreshToken,
3523-
organizationId: account.organizationId,
3524-
accountId: account.accountId,
3525-
}),
3503+
const nextFlaggedStorage = {
3504+
version: 1 as const,
3505+
accounts: currentFlaggedStorage.accounts.filter(
3506+
(flagged) =>
3507+
!removedFlaggedKeys.has(
3508+
getSyncRemovalTargetKey({
3509+
refreshToken: flagged.refreshToken,
3510+
organizationId: flagged.organizationId,
3511+
accountId: flagged.accountId,
3512+
}),
3513+
),
35263514
),
3527-
);
3528-
const remappedActiveIndex = findAccountIndexByExactIdentity(
3529-
currentStorage.accounts,
3530-
activeAccountIdentity,
3531-
);
3532-
currentStorage.activeIndex =
3533-
remappedActiveIndex >= 0
3534-
? remappedActiveIndex
3535-
: Math.min(currentStorage.activeIndex, Math.max(0, currentStorage.accounts.length - 1));
3536-
currentStorage.activeIndexByFamily = currentStorage.activeIndexByFamily ?? {};
3537-
for (const family of MODEL_FAMILIES) {
3538-
const remappedFamilyIndex = findAccountIndexByExactIdentity(
3539-
currentStorage.accounts,
3540-
familyActiveIdentities[family] ?? null,
3541-
);
3542-
currentStorage.activeIndexByFamily[family] =
3543-
remappedFamilyIndex >= 0 ? remappedFamilyIndex : currentStorage.activeIndex;
3544-
}
3545-
clampActiveIndices(currentStorage);
3546-
await persist(currentStorage);
3547-
});
3515+
};
35483516

3549-
if (removedTargets.length > 0) {
3550-
const removedFlaggedKeys = new Set(
3551-
removedTargets.map((entry) =>
3552-
getSyncRemovalTargetKey({
3553-
refreshToken: entry.account.refreshToken,
3554-
organizationId: entry.account.organizationId,
3555-
accountId: entry.account.accountId,
3517+
const activeAccountIdentity = {
3518+
refreshToken:
3519+
currentStorage.accounts[currentStorage.activeIndex]?.refreshToken ?? "",
3520+
organizationId:
3521+
currentStorage.accounts[currentStorage.activeIndex]?.organizationId,
3522+
accountId: currentStorage.accounts[currentStorage.activeIndex]?.accountId,
3523+
} satisfies SyncRemovalTarget;
3524+
const familyActiveIdentities = Object.fromEntries(
3525+
MODEL_FAMILIES.map((family) => {
3526+
const familyIndex =
3527+
currentStorage.activeIndexByFamily?.[family] ?? currentStorage.activeIndex;
3528+
const familyAccount = currentStorage.accounts[familyIndex];
3529+
return [
3530+
family,
3531+
familyAccount
3532+
? ({
3533+
refreshToken: familyAccount.refreshToken,
3534+
organizationId: familyAccount.organizationId,
3535+
accountId: familyAccount.accountId,
3536+
} satisfies SyncRemovalTarget)
3537+
: null,
3538+
];
35563539
}),
3557-
),
3558-
);
3559-
try {
3560-
await withFlaggedAccountsTransaction(async (currentFlaggedStorage, persist) => {
3561-
await persist({
3562-
version: 1,
3563-
accounts: currentFlaggedStorage.accounts.filter(
3564-
(flagged) =>
3565-
!removedFlaggedKeys.has(
3566-
getSyncRemovalTargetKey({
3567-
refreshToken: flagged.refreshToken,
3568-
organizationId: flagged.organizationId,
3569-
accountId: flagged.accountId,
3570-
}),
3571-
),
3540+
) as Partial<Record<ModelFamily, SyncRemovalTarget | null>>;
3541+
3542+
currentStorage.accounts = currentStorage.accounts.filter(
3543+
(account) =>
3544+
!targetKeySet.has(
3545+
getSyncRemovalTargetKey({
3546+
refreshToken: account.refreshToken,
3547+
organizationId: account.organizationId,
3548+
accountId: account.accountId,
3549+
}),
35723550
),
3573-
});
3574-
});
3575-
} catch (flaggedError) {
3576-
if (rollbackStorage) {
3551+
);
3552+
const remappedActiveIndex = findAccountIndexByExactIdentity(
3553+
currentStorage.accounts,
3554+
activeAccountIdentity,
3555+
);
3556+
currentStorage.activeIndex =
3557+
remappedActiveIndex >= 0
3558+
? remappedActiveIndex
3559+
: Math.min(
3560+
currentStorage.activeIndex,
3561+
Math.max(0, currentStorage.accounts.length - 1),
3562+
);
3563+
currentStorage.activeIndexByFamily = currentStorage.activeIndexByFamily ?? {};
3564+
for (const family of MODEL_FAMILIES) {
3565+
const remappedFamilyIndex = findAccountIndexByExactIdentity(
3566+
currentStorage.accounts,
3567+
familyActiveIdentities[family] ?? null,
3568+
);
3569+
currentStorage.activeIndexByFamily[family] =
3570+
remappedFamilyIndex >= 0 ? remappedFamilyIndex : currentStorage.activeIndex;
3571+
}
3572+
clampActiveIndices(currentStorage);
3573+
3574+
// Persist flagged cleanup before account removal so a crash cannot leave
3575+
// flagged entries pointing at deleted accounts.
3576+
await persist.flagged(nextFlaggedStorage);
3577+
try {
3578+
await persist.accounts(currentStorage);
3579+
} catch (accountsError) {
35773580
try {
3578-
await withAccountStorageTransaction(async (_current, persist) => {
3579-
await persist(rollbackStorage as AccountStorageV3);
3580-
});
3581-
} catch (restoreError) {
3582-
const flaggedMessage =
3583-
flaggedError instanceof Error ? flaggedError.message : String(flaggedError);
3584-
const restoreMessage =
3585-
restoreError instanceof Error ? restoreError.message : String(restoreError);
3581+
await persist.flagged(currentFlaggedStorage);
3582+
} catch (restoreFlaggedError) {
3583+
const accountsMessage =
3584+
accountsError instanceof Error ? accountsError.message : String(accountsError);
3585+
const restoreFlaggedMessage =
3586+
restoreFlaggedError instanceof Error
3587+
? restoreFlaggedError.message
3588+
: String(restoreFlaggedError);
35863589
throw new Error(
3587-
`Failed to remove flagged sync entries after account removal: ${flaggedMessage}; ` +
3588-
`failed to restore removed accounts: ${restoreMessage}`,
3590+
`Failed to remove sync accounts after flagged cleanup: ${accountsMessage}; ` +
3591+
`failed to restore flagged storage: ${restoreFlaggedMessage}`,
35893592
);
35903593
}
3594+
throw accountsError;
35913595
}
3592-
throw flaggedError;
3593-
}
3596+
},
3597+
);
3598+
3599+
if (removedTargets.length > 0) {
35943600
invalidateAccountManagerCache();
35953601
}
35963602
};

‎lib/cli.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -229,15 +229,15 @@ async function promptLoginModeFallback(
229229

230230
while (true) {
231231
const answer = await rl.question(
232-
"(a)dd, (f)resh, (c)heck, (d)eep, (v)erify flagged, s(y)nc tools, or (q)uit? [a/f/c/d/v/s/y/q]: ",
232+
"(a)dd, (f)resh, (c)heck, (d)eep, (v)erify flagged, (s)ync tools, or (q)uit? [a/f/c/d/v/s/q]: ",
233233
);
234234
const normalized = answer.trim().toLowerCase();
235235
if (normalized === "a" || normalized === "add") return { mode: "add" };
236236
if (normalized === "f" || normalized === "fresh") return { mode: "fresh", deleteAll: true };
237237
if (normalized === "c" || normalized === "check") return { mode: "check" };
238238
if (normalized === "d" || normalized === "deep") return { mode: "deep-check" };
239239
if (normalized === "v" || normalized === "verify") return { mode: "verify-flagged" };
240-
if (normalized === "s" || normalized === "sync" || normalized === "y") {
240+
if (normalized === "s" || normalized === "sync") {
241241
const syncAction = await promptSyncToolsFallback(
242242
rl,
243243
options.syncFromCodexMultiAuthEnabled === true,
@@ -246,7 +246,7 @@ async function promptLoginModeFallback(
246246
continue;
247247
}
248248
if (normalized === "q" || normalized === "quit") return { mode: "cancel" };
249-
console.log("Please enter one of: a, f, c, d, v, s, y, q.");
249+
console.log("Please enter one of: a, f, c, d, v, s, q.");
250250
}
251251
} finally {
252252
rl.close();

‎lib/codex-multi-auth-sync.ts‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -269,12 +269,7 @@ async function withNormalizedImportFile<T>(
269269
return result;
270270
} catch (error) {
271271
await redactNormalizedImportTempFile(tempPath, storage);
272-
try {
273-
await removeNormalizedImportTempDir(tempDir, tempPath, { postSuccessCleanupFailureMode: "warn" });
274-
} catch (cleanupError) {
275-
const message = cleanupError instanceof Error ? cleanupError.message : String(cleanupError);
276-
logWarn(`Failed to remove temporary codex sync directory ${tempDir}: ${message}`);
277-
}
272+
await removeNormalizedImportTempDir(tempDir, tempPath, { postSuccessCleanupFailureMode: "warn" });
278273
throw error;
279274
}
280275
};

‎lib/storage.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,6 +1157,37 @@ export async function withFlaggedAccountsTransaction<T>(
11571157
});
11581158
}
11591159

1160+
/**
1161+
* Runs `handler` while the shared storage lock is held and exposes unlocked
1162+
* persist callbacks for both account files. Callers are responsible for
1163+
* choosing a write order that preserves invariants if the process crashes
1164+
* between the two atomic file writes.
1165+
*/
1166+
export async function withAccountAndFlaggedStorageTransaction<T>(
1167+
handler: (
1168+
current: {
1169+
accounts: AccountStorageV3 | null;
1170+
flagged: FlaggedAccountStorageV1;
1171+
},
1172+
persist: {
1173+
accounts: (storage: AccountStorageV3) => Promise<void>;
1174+
flagged: (storage: FlaggedAccountStorageV1) => Promise<void>;
1175+
},
1176+
) => Promise<T>,
1177+
): Promise<T> {
1178+
return withStorageLock(async () => {
1179+
const accounts = await loadAccountsInternal(saveAccountsUnlocked);
1180+
const flagged = await loadFlaggedAccountsUnlocked(accounts);
1181+
return handler(
1182+
{ accounts, flagged },
1183+
{
1184+
accounts: saveAccountsUnlocked,
1185+
flagged: saveFlaggedAccountsUnlocked,
1186+
},
1187+
);
1188+
});
1189+
}
1190+
11601191
export async function loadAccountAndFlaggedStorageSnapshot(): Promise<{
11611192
accounts: AccountStorageV3 | null;
11621193
flagged: FlaggedAccountStorageV1;

0 commit comments

Comments
 (0)