Skip to content

Commit e3f5dfc

Browse files
ndycodendycode
authored
Merge pull request #320 from ndycode/fix/export-empty-storage-state
fix(storage): fail export when current pool is empty
2 parents 76e5216 + d381063 commit e3f5dfc

File tree

5 files changed

+936
-61
lines changed

5 files changed

+936
-61
lines changed

‎lib/storage.ts‎

Lines changed: 140 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -709,13 +709,17 @@ 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;
718721
}
722+
const currentStoragePath = state.currentStoragePath;
719723

720724
const candidatePaths = [
721725
state.currentLegacyWorktreeStoragePath,
@@ -740,10 +744,8 @@ async function migrateLegacyProjectStorageIfNeeded(
740744
return null;
741745
}
742746

743-
let targetStorage = await loadNormalizedStorageFromPath(
744-
state.currentStoragePath,
745-
"current account storage",
746-
{
747+
const loadCurrentStorageForMigration = async (): Promise<AccountStorageV3 | null> =>
748+
loadNormalizedStorageFromPath(currentStoragePath, "current account storage", {
747749
loadAccountsFromPath: (path) =>
748750
loadAccountsFromPath(path, {
749751
normalizeAccountStorage,
@@ -752,11 +754,50 @@ async function migrateLegacyProjectStorageIfNeeded(
752754
logWarn: (message, details) => {
753755
log.warn(message, details);
754756
},
755-
},
756-
);
757+
});
758+
const readLiveCurrentStorageIfExportMode = async (): Promise<{
759+
exists: boolean;
760+
storage: AccountStorageV3 | null;
761+
}> => {
762+
if (commit || !existsSync(currentStoragePath)) {
763+
return { exists: false, storage: null };
764+
}
765+
try {
766+
const { normalized, schemaErrors } = await loadAccountsFromPath(
767+
currentStoragePath,
768+
{
769+
normalizeAccountStorage,
770+
isRecord,
771+
},
772+
);
773+
if (schemaErrors.length > 0) {
774+
log.warn("current account storage schema validation warnings", {
775+
path: currentStoragePath,
776+
errors: schemaErrors.slice(0, 5),
777+
});
778+
}
779+
return {
780+
exists: true,
781+
storage: normalized,
782+
};
783+
} catch (error) {
784+
if ((error as NodeJS.ErrnoException).code === "ENOENT") {
785+
return { exists: false, storage: null };
786+
}
787+
throw error;
788+
}
789+
};
790+
791+
let targetStorage = await loadCurrentStorageForMigration();
757792
let migrated = false;
758793

759794
for (const legacyPath of existingCandidatePaths) {
795+
const liveCurrentStorageBeforeMerge =
796+
await readLiveCurrentStorageIfExportMode();
797+
if (liveCurrentStorageBeforeMerge.exists) {
798+
return liveCurrentStorageBeforeMerge.storage;
799+
}
800+
760801
const legacyStorage = await loadNormalizedStorageFromPath(
761802
legacyPath,
762803
"legacy account storage",
@@ -775,56 +816,68 @@ async function migrateLegacyProjectStorageIfNeeded(
775816
continue;
776817
}
777818

819+
const liveCurrentStorageAfterLegacyRead =
820+
await readLiveCurrentStorageIfExportMode();
821+
if (liveCurrentStorageAfterLegacyRead.exists) {
822+
return liveCurrentStorageAfterLegacyRead.storage;
823+
}
824+
778825
const mergedStorage = mergeStorageForMigration(
779826
targetStorage,
780827
legacyStorage,
781828
normalizeAccountStorage,
782829
);
783830
const fallbackStorage = targetStorage ?? legacyStorage;
784831

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", {
832+
if (commit) {
833+
try {
834+
await persist(mergedStorage);
835+
targetStorage = mergedStorage;
836+
migrated = true;
837+
} catch (error) {
838+
targetStorage = fallbackStorage;
839+
log.warn("Failed to persist migrated account storage", {
840+
from: legacyPath,
841+
to: currentStoragePath,
842+
error: String(error),
843+
});
844+
continue;
845+
}
846+
847+
try {
848+
await fs.unlink(legacyPath);
849+
log.info("Removed legacy account storage file after migration", {
850+
path: legacyPath,
851+
});
852+
} catch (unlinkError) {
853+
const code = (unlinkError as NodeJS.ErrnoException).code;
854+
if (code !== "ENOENT") {
855+
log.warn(
856+
"Failed to remove legacy account storage file after migration",
857+
{
858+
path: legacyPath,
859+
error: String(unlinkError),
860+
},
861+
);
862+
}
863+
}
864+
865+
log.info("Migrated legacy project account storage", {
792866
from: legacyPath,
793-
to: state.currentStoragePath,
794-
error: String(error),
867+
to: currentStoragePath,
868+
accounts: mergedStorage.accounts.length,
795869
});
796870
continue;
797871
}
798872

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-
});
873+
targetStorage = mergedStorage;
874+
migrated = true;
822875
}
823876

824877
if (migrated) {
825878
return targetStorage;
826879
}
827-
if (targetStorage && !existsSync(state.currentStoragePath)) {
880+
if (targetStorage && !existsSync(currentStoragePath)) {
828881
return targetStorage;
829882
}
830883
return null;
@@ -1263,7 +1316,7 @@ async function loadAccountsInternal(
12631316
const resetMarkerPath = getIntentionalResetMarkerPath(path);
12641317
await cleanupStaleRotatingBackupArtifacts(path);
12651318
const migratedLegacyStorage = persistMigration
1266-
? await migrateLegacyProjectStorageIfNeeded(persistMigration)
1319+
? await migrateLegacyProjectStorageIfNeeded({ persist: persistMigration })
12671320
: null;
12681321

12691322
try {
@@ -1432,6 +1485,48 @@ async function loadAccountsInternal(
14321485
}
14331486
}
14341487

1488+
async function loadAccountsForExport(): Promise<AccountStorageV3 | null> {
1489+
// Export reuses this helper from both paths in `exportAccounts()`. Keep the
1490+
// read side effect free so export never clears a reset marker or races with
1491+
// concurrent writers while normalizing legacy storage for the snapshot.
1492+
const path = getStoragePath();
1493+
const resetMarkerPath = getIntentionalResetMarkerPath(path);
1494+
1495+
if (existsSync(resetMarkerPath)) {
1496+
return createEmptyStorageWithMetadata(false, "intentional-reset");
1497+
}
1498+
1499+
try {
1500+
const { normalized, schemaErrors } = await loadAccountsFromPath(path, {
1501+
normalizeAccountStorage,
1502+
isRecord,
1503+
});
1504+
if (schemaErrors.length > 0) {
1505+
log.warn("Account storage schema validation warnings", {
1506+
errors: schemaErrors.slice(0, 5),
1507+
});
1508+
}
1509+
if (existsSync(resetMarkerPath)) {
1510+
return createEmptyStorageWithMetadata(false, "intentional-reset");
1511+
}
1512+
return normalized;
1513+
} catch (error) {
1514+
const code = (error as NodeJS.ErrnoException).code;
1515+
if (existsSync(resetMarkerPath)) {
1516+
return createEmptyStorageWithMetadata(false, "intentional-reset");
1517+
}
1518+
if (code === "ENOENT") {
1519+
const migratedLegacyStorage =
1520+
await migrateLegacyProjectStorageIfNeeded({ commit: false });
1521+
if (existsSync(resetMarkerPath)) {
1522+
return createEmptyStorageWithMetadata(false, "intentional-reset");
1523+
}
1524+
return migratedLegacyStorage;
1525+
}
1526+
throw error;
1527+
}
1528+
}
1529+
14351530
async function saveAccountsUnlocked(storage: AccountStorageV3): Promise<void> {
14361531
const path = getStoragePath();
14371532
const resetMarkerPath = getIntentionalResetMarkerPath(path);
@@ -1788,9 +1883,8 @@ export async function exportAccounts(
17881883
force,
17891884
currentStoragePath,
17901885
transactionState: getTransactionSnapshotState(),
1791-
loadAccountsInternal: () => loadAccountsInternal(saveAccountsUnlocked),
1792-
readCurrentStorage: () =>
1793-
withAccountStorageTransaction((current) => Promise.resolve(current)),
1886+
readCurrentStorageUnlocked: () => loadAccountsForExport(),
1887+
readCurrentStorage: () => withStorageLock(() => loadAccountsForExport()),
17941888
exportAccountsToFile,
17951889
beforeCommit,
17961890
logInfo: (message, details) => {

‎lib/storage/account-port.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export async function exportAccountsSnapshot(params: {
1111
snapshot: AccountStorageV3 | null;
1212
}
1313
| undefined;
14-
loadAccountsInternal: () => Promise<AccountStorageV3 | null>;
14+
readCurrentStorageUnlocked: () => Promise<AccountStorageV3 | null>;
1515
readCurrentStorage: () => Promise<AccountStorageV3 | null>;
1616
exportAccountsToFile: (args: {
1717
resolvedPath: string;
@@ -28,7 +28,7 @@ export async function exportAccountsSnapshot(params: {
2828
params.transactionState.storagePath === params.currentStoragePath
2929
? params.transactionState.snapshot
3030
: params.transactionState?.active
31-
? await params.loadAccountsInternal()
31+
? await params.readCurrentStorageUnlocked()
3232
: await params.readCurrentStorage();
3333

3434
await params.exportAccountsToFile({

‎test/account-port.test.ts‎

Lines changed: 88 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,28 +5,111 @@ import {
55
} from "../lib/storage/account-port.js";
66

77
describe("account port helpers", () => {
8-
it("exports transaction snapshot when active", async () => {
8+
it("exports transaction snapshot when active for the current storage path", async () => {
99
const exportAccountsToFile = vi.fn(async () => undefined);
10+
const snapshot = {
11+
version: 3 as const,
12+
accounts: [{ refreshToken: "snapshot-token" }],
13+
activeIndex: 0,
14+
activeIndexByFamily: {},
15+
};
16+
const readCurrentStorageUnlocked = vi.fn();
17+
const readCurrentStorage = vi.fn();
18+
1019
await exportAccountsSnapshot({
1120
resolvedPath: "/tmp/out.json",
1221
force: true,
1322
currentStoragePath: "/tmp/accounts.json",
1423
transactionState: {
1524
active: true,
1625
storagePath: "/tmp/accounts.json",
26+
snapshot,
27+
},
28+
readCurrentStorageUnlocked,
29+
readCurrentStorage,
30+
exportAccountsToFile,
31+
logInfo: vi.fn(),
32+
});
33+
expect(readCurrentStorageUnlocked).not.toHaveBeenCalled();
34+
expect(readCurrentStorage).not.toHaveBeenCalled();
35+
expect(exportAccountsToFile).toHaveBeenCalledWith(
36+
expect.objectContaining({
37+
storage: snapshot,
38+
}),
39+
);
40+
});
41+
42+
it("reads current storage without reusing a stale transaction snapshot from another path", async () => {
43+
const exportAccountsToFile = vi.fn(async () => undefined);
44+
const readCurrentStorageUnlocked = vi.fn(async () => ({
45+
version: 3 as const,
46+
accounts: [{ refreshToken: "live-token" }],
47+
activeIndex: 0,
48+
activeIndexByFamily: {},
49+
}));
50+
const readCurrentStorage = vi.fn();
51+
52+
await exportAccountsSnapshot({
53+
resolvedPath: "/tmp/out.json",
54+
force: true,
55+
currentStoragePath: "/tmp/accounts.json",
56+
transactionState: {
57+
active: true,
58+
storagePath: "/tmp/other.json",
1759
snapshot: {
1860
version: 3,
19-
accounts: [],
61+
accounts: [{ refreshToken: "stale-token" }],
2062
activeIndex: 0,
2163
activeIndexByFamily: {},
2264
},
2365
},
24-
loadAccountsInternal: vi.fn(),
25-
readCurrentStorage: vi.fn(),
66+
readCurrentStorageUnlocked,
67+
readCurrentStorage,
2668
exportAccountsToFile,
2769
logInfo: vi.fn(),
2870
});
29-
expect(exportAccountsToFile).toHaveBeenCalled();
71+
72+
expect(readCurrentStorageUnlocked).toHaveBeenCalledTimes(1);
73+
expect(readCurrentStorage).not.toHaveBeenCalled();
74+
expect(exportAccountsToFile).toHaveBeenCalledWith(
75+
expect.objectContaining({
76+
storage: expect.objectContaining({
77+
accounts: [{ refreshToken: "live-token" }],
78+
}),
79+
}),
80+
);
81+
});
82+
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+
);
30113
});
31114

32115
it("imports through transaction helper and logs result", async () => {

0 commit comments

Comments
 (0)