Skip to content

Commit fcd6be3

Browse files
ndycodendycode
committed
fix(storage): keep export reads side-effect free
1 parent 49839a9 commit fcd6be3

2 files changed

Lines changed: 174 additions & 34 deletions

File tree

‎lib/storage.ts‎

Lines changed: 11 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1441,49 +1441,27 @@ async function loadAccountsInternal(
14411441
}
14421442
}
14431443

1444-
async function loadAccountsForExport(
1445-
mode: "locked" | "unlocked" = "locked",
1446-
): Promise<AccountStorageV3 | null> {
1447-
// Export reuses this helper from both paths in `exportAccounts()`. Only the
1448-
// locked path may persist migrations; unlocked reads must remain side-effect
1449-
// free while another storage transaction holds the mutex for a different file.
1450-
const persistMigrations = mode === "locked";
1444+
async function loadAccountsForExport(): Promise<AccountStorageV3 | null> {
1445+
// Export reuses this helper from both paths in `exportAccounts()`. Keep the
1446+
// read side effect free so export never clears a reset marker or races with
1447+
// concurrent writers while normalizing legacy storage for the snapshot.
14511448
const path = getStoragePath();
14521449
const resetMarkerPath = getIntentionalResetMarkerPath(path);
1453-
const migrationOptions = persistMigrations
1454-
? { persist: saveAccountsUnlocked }
1455-
: { commit: false };
14561450

14571451
if (existsSync(resetMarkerPath)) {
14581452
return createEmptyStorageWithMetadata(false, "intentional-reset");
14591453
}
14601454

14611455
try {
1462-
const { normalized, storedVersion, schemaErrors } = await loadAccountsFromPath(
1463-
path,
1464-
{
1465-
normalizeAccountStorage,
1466-
isRecord,
1467-
},
1468-
);
1456+
const { normalized, schemaErrors } = await loadAccountsFromPath(path, {
1457+
normalizeAccountStorage,
1458+
isRecord,
1459+
});
14691460
if (schemaErrors.length > 0) {
14701461
log.warn("Account storage schema validation warnings", {
14711462
errors: schemaErrors.slice(0, 5),
14721463
});
14731464
}
1474-
if (persistMigrations && normalized && storedVersion !== normalized.version) {
1475-
log.info("Migrating account storage to v3", {
1476-
from: storedVersion,
1477-
to: normalized.version,
1478-
});
1479-
try {
1480-
await saveAccountsUnlocked(normalized);
1481-
} catch (saveError) {
1482-
log.warn("Failed to persist migrated storage", {
1483-
error: String(saveError),
1484-
});
1485-
}
1486-
}
14871465
if (existsSync(resetMarkerPath)) {
14881466
return createEmptyStorageWithMetadata(false, "intentional-reset");
14891467
}
@@ -1495,7 +1473,7 @@ async function loadAccountsForExport(
14951473
}
14961474
if (code === "ENOENT") {
14971475
const migratedLegacyStorage =
1498-
await migrateLegacyProjectStorageIfNeeded(migrationOptions);
1476+
await migrateLegacyProjectStorageIfNeeded({ commit: false });
14991477
if (existsSync(resetMarkerPath)) {
15001478
return createEmptyStorageWithMetadata(false, "intentional-reset");
15011479
}
@@ -1861,9 +1839,8 @@ export async function exportAccounts(
18611839
force,
18621840
currentStoragePath,
18631841
transactionState: getTransactionSnapshotState(),
1864-
readCurrentStorageUnlocked: () => loadAccountsForExport("unlocked"),
1865-
readCurrentStorage: () =>
1866-
withStorageLock(() => loadAccountsForExport("locked")),
1842+
readCurrentStorageUnlocked: () => loadAccountsForExport(),
1843+
readCurrentStorage: () => withStorageLock(() => loadAccountsForExport()),
18671844
exportAccountsToFile,
18681845
beforeCommit,
18691846
logInfo: (message, details) => {

‎test/storage.test.ts‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1407,6 +1407,41 @@ describe("storage", () => {
14071407
}
14081408
});
14091409

1410+
it("does not persist v3 normalization while export reads storage with the lock", async () => {
1411+
const currentStoragePath = join(testWorkDir, "accounts-v1-locked.json");
1412+
await fs.writeFile(
1413+
currentStoragePath,
1414+
JSON.stringify({
1415+
version: 1,
1416+
activeIndex: 0,
1417+
accounts: [
1418+
{
1419+
refreshToken: "legacy-token",
1420+
addedAt: 1,
1421+
lastUsed: 1,
1422+
},
1423+
],
1424+
}),
1425+
);
1426+
1427+
setStoragePathDirect(currentStoragePath);
1428+
try {
1429+
await exportAccounts(exportPath);
1430+
1431+
const onDisk = JSON.parse(
1432+
await fs.readFile(currentStoragePath, "utf-8"),
1433+
);
1434+
const exported = JSON.parse(await fs.readFile(exportPath, "utf-8"));
1435+
expect(onDisk.version).toBe(1);
1436+
expect(exported.version).toBe(3);
1437+
expect(exported.accounts).toEqual([
1438+
expect.objectContaining({ refreshToken: "legacy-token" }),
1439+
]);
1440+
} finally {
1441+
setStoragePathDirect(testStoragePath);
1442+
}
1443+
});
1444+
14101445
it.each(["EBUSY", "EPERM"] as const)(
14111446
"rethrows %s when export cannot read the current storage file",
14121447
async (code) => {
@@ -1456,6 +1491,92 @@ describe("storage", () => {
14561491
},
14571492
);
14581493

1494+
it.each(["EBUSY", "EPERM"] as const)(
1495+
"does not write an export file when %s happens while reading another storage path during a transaction",
1496+
async (code) => {
1497+
const transactionStoragePath = join(
1498+
testWorkDir,
1499+
`accounts-transaction-${code}.json`,
1500+
);
1501+
const currentStoragePath = join(testWorkDir, `accounts-live-${code}.json`);
1502+
await fs.writeFile(
1503+
transactionStoragePath,
1504+
JSON.stringify({
1505+
version: 3,
1506+
activeIndex: 0,
1507+
activeIndexByFamily: {},
1508+
accounts: [
1509+
{
1510+
accountId: "transaction",
1511+
refreshToken: "transaction-token",
1512+
addedAt: 1,
1513+
lastUsed: 1,
1514+
},
1515+
],
1516+
}),
1517+
);
1518+
await fs.writeFile(
1519+
currentStoragePath,
1520+
JSON.stringify({
1521+
version: 3,
1522+
activeIndex: 0,
1523+
activeIndexByFamily: {},
1524+
accounts: [
1525+
{
1526+
accountId: "live",
1527+
refreshToken: "live-token",
1528+
addedAt: 1,
1529+
lastUsed: 1,
1530+
},
1531+
],
1532+
}),
1533+
);
1534+
1535+
const actualStorageParser = await vi.importActual<
1536+
typeof import("../lib/storage/storage-parser.js")
1537+
>("../lib/storage/storage-parser.js");
1538+
vi.resetModules();
1539+
vi.doMock("../lib/storage/storage-parser.js", () => ({
1540+
...actualStorageParser,
1541+
loadAccountsFromPath: vi.fn(async (path, deps) => {
1542+
if (path === currentStoragePath) {
1543+
throw Object.assign(new Error(`locked ${code}`), { code });
1544+
}
1545+
return actualStorageParser.loadAccountsFromPath(path, deps);
1546+
}),
1547+
}));
1548+
1549+
try {
1550+
const isolatedStorageModule = await import("../lib/storage.js");
1551+
const isolatedPathState = await import("../lib/storage/path-state.js");
1552+
isolatedStorageModule.setStoragePathDirect(transactionStoragePath);
1553+
await expect(
1554+
isolatedStorageModule.withAccountStorageTransaction(async () => {
1555+
isolatedPathState.setStoragePathState({
1556+
currentStoragePath,
1557+
currentLegacyProjectStoragePath: null,
1558+
currentLegacyWorktreeStoragePath: null,
1559+
currentProjectRoot: null,
1560+
});
1561+
await isolatedStorageModule.exportAccounts(exportPath);
1562+
}),
1563+
).rejects.toMatchObject({ code });
1564+
1565+
const transactionStorage = JSON.parse(
1566+
await fs.readFile(transactionStoragePath, "utf-8"),
1567+
);
1568+
expect(transactionStorage.accounts).toEqual([
1569+
expect.objectContaining({ refreshToken: "transaction-token" }),
1570+
]);
1571+
expect(existsSync(exportPath)).toBe(false);
1572+
} finally {
1573+
vi.doUnmock("../lib/storage/storage-parser.js");
1574+
vi.resetModules();
1575+
setStoragePathDirect(testStoragePath);
1576+
}
1577+
},
1578+
);
1579+
14591580
it("does not revive legacy accounts when the current storage exists but is empty", async () => {
14601581
const currentStoragePath = join(testWorkDir, "accounts-empty-current.json");
14611582
const legacyStoragePath = join(testWorkDir, "accounts-empty-legacy.json");
@@ -1509,6 +1630,48 @@ describe("storage", () => {
15091630
}
15101631
});
15111632

1633+
it("exports legacy storage without persisting it when current storage is missing", async () => {
1634+
const currentStoragePath = join(testWorkDir, "accounts-missing-current.json");
1635+
const legacyStoragePath = join(testWorkDir, "accounts-missing-legacy.json");
1636+
await fs.writeFile(
1637+
legacyStoragePath,
1638+
JSON.stringify({
1639+
version: 3,
1640+
activeIndex: 0,
1641+
activeIndexByFamily: {},
1642+
accounts: [
1643+
{
1644+
accountId: "legacy",
1645+
refreshToken: "legacy-token",
1646+
addedAt: 1,
1647+
lastUsed: 1,
1648+
},
1649+
],
1650+
}),
1651+
);
1652+
1653+
setStoragePathDirect(currentStoragePath);
1654+
try {
1655+
setStoragePathState({
1656+
currentStoragePath,
1657+
currentLegacyProjectStoragePath: legacyStoragePath,
1658+
currentLegacyWorktreeStoragePath: null,
1659+
currentProjectRoot: null,
1660+
});
1661+
1662+
await exportAccounts(exportPath);
1663+
1664+
const exported = JSON.parse(await fs.readFile(exportPath, "utf-8"));
1665+
expect(exported.accounts).toEqual([
1666+
expect.objectContaining({ refreshToken: "legacy-token" }),
1667+
]);
1668+
expect(existsSync(currentStoragePath)).toBe(false);
1669+
expect(existsSync(legacyStoragePath)).toBe(true);
1670+
} finally {
1671+
setStoragePathDirect(testStoragePath);
1672+
}
1673+
});
1674+
15121675
it("does not revive legacy accounts when the current storage has an intentional reset marker", async () => {
15131676
const currentStoragePath = join(testWorkDir, "accounts-reset-current.json");
15141677
const legacyStoragePath = join(testWorkDir, "accounts-reset-legacy.json");

0 commit comments

Comments
 (0)