Skip to content

Commit 367d845

Browse files
committed
Address CodeRabbit review feedback
- Serialize snapshot writes per slot. Callers fire these off without awaiting them, alongside opening or saving a project, so two projects can race for the same slot and whichever write landed last would win it. The recent list is updated synchronously as each project opens, so a slow first write finishing last would leave the slot holding a project the preference no longer resolves to, and the next cold start would find no copy matching the path it asks for. Chaining per slot makes the last write started the one that wins, which is the one the preference agrees with. The stored link swallows rejections so one failed copy cannot strand every later one behind it, while the caller still sees the real result. Covered by a test where the first write is delayed past the second.
1 parent d2474c9 commit 367d845

2 files changed

Lines changed: 78 additions & 16 deletions

File tree

apps/geolibre-desktop/src/lib/startup-project-snapshot.ts

Lines changed: 45 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,33 @@ function writeStartupSnapshotIndex(
169169
}
170170
}
171171

172+
/**
173+
* The write in flight for each slot, so copies land in the order they were
174+
* asked for.
175+
*
176+
* Callers fire these off without awaiting them, alongside opening or saving a
177+
* project. Two projects can therefore be racing for the same slot -- open one
178+
* project, open another before the first copy has landed -- and whichever write
179+
* finished last would win the slot. The recent list is updated synchronously as
180+
* each project opens, so a slow first write finishing last would leave the slot
181+
* holding a project the preference no longer resolves to, and the next cold
182+
* start would find no copy matching the path it asks for. Chaining per slot
183+
* makes the last write *started* the one that wins, which is the one the
184+
* preference agrees with.
185+
*/
186+
const slotWrites = new Map<StartupSnapshotSlot, Promise<unknown>>();
187+
188+
function queueSlotWrite<T>(slot: StartupSnapshotSlot, task: () => Promise<T>): Promise<T> {
189+
const next = (slotWrites.get(slot) ?? Promise.resolve()).then(task);
190+
// The stored link never rejects, so one failed copy cannot strand every later
191+
// one behind it; the caller still sees the real result through `next`.
192+
slotWrites.set(
193+
slot,
194+
next.catch(() => undefined),
195+
);
196+
return next;
197+
}
198+
172199
/**
173200
* The slot a project should be copied into, or null when it is not one the
174201
* startup preference would reopen.
@@ -222,23 +249,25 @@ export async function writeStartupSnapshot(
222249
return null;
223250
}
224251

225-
const file = startupSnapshotFile(slot);
226-
try {
227-
await io.write(file, text);
228-
} catch (error) {
229-
console.warn("Could not keep a restorable copy of the startup project.", error);
230-
return null;
231-
}
252+
return queueSlotWrite(slot, async () => {
253+
const file = startupSnapshotFile(slot);
254+
try {
255+
await io.write(file, text);
256+
} catch (error) {
257+
console.warn("Could not keep a restorable copy of the startup project.", error);
258+
return null;
259+
}
232260

233-
const storage = options?.storage === undefined ? defaultStorage() : options.storage;
234-
writeStartupSnapshotIndex(
235-
{
236-
...readStartupSnapshotIndex(storage),
237-
[slot]: { sourcePath: path, file, savedAt: new Date().toISOString() },
238-
},
239-
storage,
240-
);
241-
return slot;
261+
const storage = options?.storage === undefined ? defaultStorage() : options.storage;
262+
writeStartupSnapshotIndex(
263+
{
264+
...readStartupSnapshotIndex(storage),
265+
[slot]: { sourcePath: path, file, savedAt: new Date().toISOString() },
266+
},
267+
storage,
268+
);
269+
return slot;
270+
});
242271
}
243272

244273
/**

tests/startup-project-snapshot.test.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,39 @@ describe("writeStartupSnapshot", () => {
215215
assert.equal(warnings.length, 1);
216216
});
217217

218+
it("lets the last copy asked for win the slot, not the last one to land", async () => {
219+
// Open one project, open another before the first copy has landed. The
220+
// recent list already says the second is the most recent, so the slot has to
221+
// agree with it however slowly the first write finishes.
222+
const storage = makeStorage();
223+
const files = new Map<string, string>();
224+
const delays = [40, 0];
225+
const io = {
226+
write: async (file: string, content: string) => {
227+
await new Promise((resolve) => setTimeout(resolve, delays.shift() ?? 0));
228+
files.set(file, content);
229+
},
230+
read: async (file: string) => files.get(file) ?? Promise.reject(new Error("missing")),
231+
};
232+
233+
const first = writeStartupSnapshot(CONTENT_URI, "first", settings({ mode: "last" }), io, {
234+
storage,
235+
});
236+
const second = writeStartupSnapshot(
237+
OTHER_CONTENT_URI,
238+
"second",
239+
settings({ mode: "last" }),
240+
io,
241+
{
242+
storage,
243+
},
244+
);
245+
await Promise.all([first, second]);
246+
247+
assert.equal(files.get(startupSnapshotFile("last")), "second");
248+
assert.equal(readStartupSnapshotIndex(storage).last?.sourcePath, OTHER_CONTENT_URI);
249+
});
250+
218251
it("swallows a write failure so it cannot fail the save that triggered it", async () => {
219252
const storage = makeStorage();
220253
const { io } = makeIo({ writeError: new Error("No space left on device") });

0 commit comments

Comments
 (0)