Skip to content

Commit c95016d

Browse files
ndycodendycode
committed
fix(storage): harden export review followups
1 parent cdb4a37 commit c95016d

4 files changed

Lines changed: 283 additions & 48 deletions

File tree

‎lib/storage.ts‎

Lines changed: 57 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -709,9 +709,12 @@ function getLegacyFlaggedAccountsPath(): string {
709709
);
710710
}
711711

712-
async function migrateLegacyProjectStorageIfNeeded(
713-
persist: (storage: AccountStorageV3) => Promise<void> = saveAccounts,
714-
): Promise<AccountStorageV3 | null> {
712+
async function migrateLegacyProjectStorageIfNeeded(options?: {
713+
persist?: (storage: AccountStorageV3) => Promise<void>;
714+
commit?: boolean;
715+
}): Promise<AccountStorageV3 | null> {
716+
const persist = options?.persist ?? saveAccounts;
717+
const commit = options?.commit ?? true;
715718
const state = getStoragePathState();
716719
if (!state.currentStoragePath) {
717720
return null;
@@ -782,43 +785,49 @@ async function migrateLegacyProjectStorageIfNeeded(
782785
);
783786
const fallbackStorage = targetStorage ?? legacyStorage;
784787

785-
try {
786-
await persist(mergedStorage);
787-
targetStorage = mergedStorage;
788-
migrated = true;
789-
} catch (error) {
790-
targetStorage = fallbackStorage;
791-
log.warn("Failed to persist migrated account storage", {
788+
if (commit) {
789+
try {
790+
await persist(mergedStorage);
791+
targetStorage = mergedStorage;
792+
migrated = true;
793+
} catch (error) {
794+
targetStorage = fallbackStorage;
795+
log.warn("Failed to persist migrated account storage", {
796+
from: legacyPath,
797+
to: state.currentStoragePath,
798+
error: String(error),
799+
});
800+
continue;
801+
}
802+
803+
try {
804+
await fs.unlink(legacyPath);
805+
log.info("Removed legacy account storage file after migration", {
806+
path: legacyPath,
807+
});
808+
} catch (unlinkError) {
809+
const code = (unlinkError as NodeJS.ErrnoException).code;
810+
if (code !== "ENOENT") {
811+
log.warn(
812+
"Failed to remove legacy account storage file after migration",
813+
{
814+
path: legacyPath,
815+
error: String(unlinkError),
816+
},
817+
);
818+
}
819+
}
820+
821+
log.info("Migrated legacy project account storage", {
792822
from: legacyPath,
793823
to: state.currentStoragePath,
794-
error: String(error),
824+
accounts: mergedStorage.accounts.length,
795825
});
796826
continue;
797827
}
798828

799-
try {
800-
await fs.unlink(legacyPath);
801-
log.info("Removed legacy account storage file after migration", {
802-
path: legacyPath,
803-
});
804-
} catch (unlinkError) {
805-
const code = (unlinkError as NodeJS.ErrnoException).code;
806-
if (code !== "ENOENT") {
807-
log.warn(
808-
"Failed to remove legacy account storage file after migration",
809-
{
810-
path: legacyPath,
811-
error: String(unlinkError),
812-
},
813-
);
814-
}
815-
}
816-
817-
log.info("Migrated legacy project account storage", {
818-
from: legacyPath,
819-
to: state.currentStoragePath,
820-
accounts: mergedStorage.accounts.length,
821-
});
829+
targetStorage = mergedStorage;
830+
migrated = true;
822831
}
823832

824833
if (migrated) {
@@ -1263,7 +1272,7 @@ async function loadAccountsInternal(
12631272
const resetMarkerPath = getIntentionalResetMarkerPath(path);
12641273
await cleanupStaleRotatingBackupArtifacts(path);
12651274
const migratedLegacyStorage = persistMigration
1266-
? await migrateLegacyProjectStorageIfNeeded(persistMigration)
1275+
? await migrateLegacyProjectStorageIfNeeded({ persist: persistMigration })
12671276
: null;
12681277

12691278
try {
@@ -1432,11 +1441,17 @@ async function loadAccountsInternal(
14321441
}
14331442
}
14341443

1435-
async function loadAccountsForExport(): Promise<AccountStorageV3 | null> {
1444+
async function loadAccountsForExport(options?: {
1445+
persistMigrations?: boolean;
1446+
}): Promise<AccountStorageV3 | null> {
1447+
const persistMigrations = options?.persistMigrations ?? true;
14361448
const path = getStoragePath();
14371449
const resetMarkerPath = getIntentionalResetMarkerPath(path);
1438-
const migratedLegacyStorage =
1439-
await migrateLegacyProjectStorageIfNeeded(saveAccountsUnlocked);
1450+
const migratedLegacyStorage = await migrateLegacyProjectStorageIfNeeded(
1451+
persistMigrations
1452+
? { persist: saveAccountsUnlocked }
1453+
: { commit: false },
1454+
);
14401455

14411456
if (existsSync(resetMarkerPath)) {
14421457
return createEmptyStorageWithMetadata(false, "intentional-reset");
@@ -1455,7 +1470,7 @@ async function loadAccountsForExport(): Promise<AccountStorageV3 | null> {
14551470
errors: schemaErrors.slice(0, 5),
14561471
});
14571472
}
1458-
if (normalized && storedVersion !== normalized.version) {
1473+
if (persistMigrations && normalized && storedVersion !== normalized.version) {
14591474
log.info("Migrating account storage to v3", {
14601475
from: storedVersion,
14611476
to: normalized.version,
@@ -1840,8 +1855,9 @@ export async function exportAccounts(
18401855
force,
18411856
currentStoragePath,
18421857
transactionState: getTransactionSnapshotState(),
1843-
readCurrentStorageUnlocked: loadAccountsForExport,
1844-
readCurrentStorage: () => withStorageLock(loadAccountsForExport),
1858+
readCurrentStorageUnlocked: () =>
1859+
loadAccountsForExport({ persistMigrations: false }),
1860+
readCurrentStorage: () => withStorageLock(() => loadAccountsForExport()),
18451861
exportAccountsToFile,
18461862
beforeCommit,
18471863
logInfo: (message, details) => {

‎test/account-port.test.ts‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,38 @@ describe("account port helpers", () => {
8080
);
8181
});
8282

83+
it("reads current storage via the locked reader when no transaction is active", async () => {
84+
const exportAccountsToFile = vi.fn(async () => undefined);
85+
const readCurrentStorageUnlocked = vi.fn();
86+
const readCurrentStorage = vi.fn(async () => ({
87+
version: 3 as const,
88+
accounts: [{ refreshToken: "locked-read-token" }],
89+
activeIndex: 0,
90+
activeIndexByFamily: {},
91+
}));
92+
93+
await exportAccountsSnapshot({
94+
resolvedPath: "/tmp/out.json",
95+
force: true,
96+
currentStoragePath: "/tmp/accounts.json",
97+
transactionState: undefined,
98+
readCurrentStorageUnlocked,
99+
readCurrentStorage,
100+
exportAccountsToFile,
101+
logInfo: vi.fn(),
102+
});
103+
104+
expect(readCurrentStorageUnlocked).not.toHaveBeenCalled();
105+
expect(readCurrentStorage).toHaveBeenCalledTimes(1);
106+
expect(exportAccountsToFile).toHaveBeenCalledWith(
107+
expect.objectContaining({
108+
storage: expect.objectContaining({
109+
accounts: [{ refreshToken: "locked-read-token" }],
110+
}),
111+
}),
112+
);
113+
});
114+
83115
it("imports through transaction helper and logs result", async () => {
84116
const result = await importAccountsSnapshot({
85117
resolvedPath: "/tmp/in.json",

‎test/experimental-sync-target-entry.test.ts‎

Lines changed: 56 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,27 @@ describe("experimental sync target entry", () => {
4040
});
4141
});
4242

43-
it("wires windows-safe retry options through readJson", async () => {
43+
it("provides windows lock retry policy to the injected reader", async () => {
4444
const sleep = vi.fn(async () => undefined);
45-
const readFileWithRetry = vi.fn(async () => '{"hello":"world"}');
45+
const retryAttempts: string[] = [];
46+
const readFileWithRetry = vi.fn(async (_path, options) => {
47+
expect(options.retryableCodes.has("EBUSY")).toBe(true);
48+
expect(options.retryableCodes.has("EPERM")).toBe(true);
49+
expect(options.retryableCodes.has("EAGAIN")).toBe(true);
50+
expect(options.retryableCodes.has("EACCES")).toBe(false);
51+
expect(options.retryableCodes.has("ENOTEMPTY")).toBe(false);
52+
expect(options.maxAttempts).toBe(4);
53+
54+
while (retryAttempts.length < 2) {
55+
retryAttempts.push("EBUSY");
56+
if (!options.retryableCodes.has("EBUSY")) {
57+
throw Object.assign(new Error("busy"), { code: "EBUSY" });
58+
}
59+
await options.sleep(25 * retryAttempts.length);
60+
}
61+
62+
return '{"hello":"world"}';
63+
});
4664
const normalizeAccountStorage = vi.fn(() => null);
4765
let capturedReadJson: ((path: string) => Promise<unknown>) | undefined;
4866

@@ -66,14 +84,45 @@ describe("experimental sync target entry", () => {
6684
});
6785

6886
expect(capturedReadJson).toBeDefined();
69-
expect(readFileWithRetry).toHaveBeenCalledWith("C:\\state.json", {
70-
retryableCodes: new Set(["EBUSY", "EPERM", "EAGAIN"]),
71-
maxAttempts: 4,
72-
sleep,
73-
});
87+
expect(readFileWithRetry).toHaveBeenCalledTimes(1);
88+
expect(sleep).toHaveBeenNthCalledWith(1, 25);
89+
expect(sleep).toHaveBeenNthCalledWith(2, 50);
7490
expect(normalizeAccountStorage).toHaveBeenCalledWith({ hello: "world" });
7591
});
7692

93+
it("propagates fail-fast lock errors that are outside the retryable set", async () => {
94+
const sleep = vi.fn(async () => undefined);
95+
const readFileWithRetry = vi.fn(async (_path, options) => {
96+
expect(options.retryableCodes.has("EACCES")).toBe(false);
97+
expect(options.retryableCodes.has("ENOTEMPTY")).toBe(false);
98+
throw Object.assign(new Error("denied"), { code: "EACCES" });
99+
});
100+
const normalizeAccountStorage = vi.fn(() => null);
101+
102+
const loadExperimentalSyncTargetState = vi.fn(async (args) => {
103+
await args.readJson("C:\\state.json");
104+
return {
105+
kind: "target" as const,
106+
detection: { kind: "target" as const },
107+
destination: null,
108+
};
109+
});
110+
111+
await expect(
112+
loadExperimentalSyncTargetEntry({
113+
loadExperimentalSyncTargetState,
114+
detectTarget: createDetectedTarget,
115+
readFileWithRetry,
116+
normalizeAccountStorage,
117+
sleep,
118+
}),
119+
).rejects.toMatchObject({ code: "EACCES" });
120+
121+
expect(readFileWithRetry).toHaveBeenCalledTimes(1);
122+
expect(sleep).not.toHaveBeenCalled();
123+
expect(normalizeAccountStorage).not.toHaveBeenCalled();
124+
});
125+
77126
it("propagates malformed json parse failures to the caller", async () => {
78127
const readFileWithRetry = vi.fn(async () => "not-valid-json{{{");
79128
const normalizeAccountStorage = vi.fn(() => null);

0 commit comments

Comments
 (0)