Fix two intermittently failing library tests (closes #90)
check / check (push) Successful in 26s

Library.close() now returns a promise that resolves once the work it
started has finished: an in-flight refresh with its cache write, the ML
data fetch, and running precache sweeps. The interval test could see a
refresh's new state, close, and remove the directory while the write was
still running. Every library test and CLI command now awaits close(), and
tests hold each of the three writes open to prove close() waits for it.

The precache test waited for its stub source to be called, but the cache
records a file only after checking it on disk, so status() could lag. It
now waits for both fills to report "done".

Model: opus-5-5
This commit is contained in:
2026-09-23 02:08:14 +00:00
parent c75c4f987c
commit 8c7bef0719
12 changed files with 268 additions and 77 deletions
+5 -2
View File
@@ -73,7 +73,7 @@ if (photo) {
console.log(`original at ${path}`);
}
lib.close();
await lib.close();
```
The lower-level `Client` (login, session serialization, and the raw
@@ -563,7 +563,10 @@ and pass it. The three pools default to 10 / 5 / 25 (see Request pools below).
`lib.status()` returns a `LibraryStatus` (collection/file counts, last
refresh/ML times and errors, originals usage and effective limit, precache
progress, and `closed`). `lib.close()` stops the background timer; it is
idempotent, and an in-flight refresh is left to finish.
idempotent, and an in-flight refresh is left to finish. The promise it returns
resolves once that refresh (including its cache write), the ML data fetch and
the precache fetches already running have all finished, so the cache directory
can then be removed.
### Default reads vs. fresh reads
+7
View File
@@ -18,6 +18,13 @@ Tag v1.0.0.
# Completed Steps
- 2026-09-23: Fixed two intermittently failing library tests (issue 90).
`Library.close()` now returns a promise that resolves once an in-flight
refresh (including its cache write), the ML data fetch and running precache
sweeps have finished; the library tests await it, so `afterEach` no longer
removes the cache directory while something is still writing into it. The
precache test waits for both fills to report "done" instead of for its stub
source to be called, which happened before the cache recorded the file.
- 2026-09-23: Pinned three guards reviewers found untested (issue 89). The
download idle deadline's timer is unref'd, so it can never keep the process
alive, and a test checks no timer is left after a download completes or fails.
+8 -8
View File
@@ -194,7 +194,7 @@ export const collectionsCommand = async (
}
return 0;
} finally {
lib.close();
await lib.close();
}
};
@@ -235,7 +235,7 @@ export const filesCommand = async (
}
return 0;
} finally {
lib.close();
await lib.close();
}
};
@@ -272,7 +272,7 @@ export const getCommand = async (
ctx.stderr.write(`${result.bytes} bytes -> ${outPath}\n`);
return 0;
} finally {
lib.close();
await lib.close();
}
};
@@ -309,7 +309,7 @@ export const getThumbCommand = async (
ctx.stderr.write(`${result.bytes} bytes -> ${outPath}\n`);
return 0;
} finally {
lib.close();
await lib.close();
}
};
@@ -329,7 +329,7 @@ export const backupMetadataCommand = async (
});
return 0;
} finally {
lib.close();
await lib.close();
}
};
@@ -376,7 +376,7 @@ export const backupCommand = async (
return result.failed > 0 ? 1 : 0;
} finally {
lib.close();
await lib.close();
}
};
@@ -411,7 +411,7 @@ export const listMissingThumbnailsCommand = async (
}
return 0;
} finally {
lib.close();
await lib.close();
}
};
@@ -481,6 +481,6 @@ export const fixMissingThumbnailsCommand = async (
return results.some((r) => r.status === "failed") ? 1 : 0;
} finally {
lib.close();
await lib.close();
}
};
+17 -10
View File
@@ -266,8 +266,9 @@ export class Library {
// that, a fresh read propagates it.
private cycle?: Promise<void>;
// Guards the ML fetch pass so a slow backfill never runs twice at once; a
// refresh whose pass is still running kicks nothing new.
private mlFetching = false;
// refresh whose pass is still running kicks nothing new. Holds the running
// pass, so `close()` can wait for it.
private mlFetch?: Promise<void>;
private closed = false;
private lastRefreshAt?: number;
private lastError?: string;
@@ -560,14 +561,21 @@ export class Library {
}
// Stop the background timer. Idempotent. An in-flight refresh is left to
// finish; it will not schedule another cycle once closed.
close(): void {
// finish; it will not schedule another cycle once closed. The returned
// promise resolves once that refresh (including its cache write), the ML
// fetch pass and the precache fetches already running have all finished,
// so a caller can then remove the cache directory. A refresh failure is
// reported through `status()`, not thrown here.
async close(): Promise<void> {
this.closed = true;
this.precache?.close();
const precacheClosed = this.precache?.close();
if (this.timer !== undefined) {
clearTimeout(this.timer);
this.timer = undefined;
}
await this.cycle?.catch(() => {});
await this.mlFetch;
await precacheClosed;
}
private scheduleNext(): void {
@@ -631,7 +639,9 @@ export class Library {
// outside the refresh's success/failure so a fetch or disk problem
// there never marks the metadata refresh failed, and it is not
// awaited so it never stalls the refresh interval.
void this.runMLFetch();
this.mlFetch ??= this.runMLFetch().finally(() => {
this.mlFetch = undefined;
});
} catch (err) {
const error = err instanceof Error ? err.message : String(err);
this.lastError = error;
@@ -745,13 +755,12 @@ export class Library {
// Bind so the call keeps the client as its receiver when invoked
// through the pool below.
const fetchMLData = this.client.fetchMLData?.bind(this.client);
if (!mldata || !fetchMLData || this.closed || this.mlFetching) return;
if (!mldata || !fetchMLData || this.closed) return;
const files = this.uniqueFiles();
const needed = mldata.neededFor(files);
if (needed.length === 0) return;
this.mlFetching = true;
this.emit({ operation: "fetchMLData", status: "started" });
try {
const fileKeys = new Map<number, Uint8Array>();
@@ -784,8 +793,6 @@ export class Library {
const error = err instanceof Error ? err.message : String(err);
this.lastMLError = error;
this.emit({ operation: "fetchMLData", status: "failed", error });
} finally {
this.mlFetching = false;
}
}
+16 -12
View File
@@ -92,9 +92,10 @@ export class Precache {
private pinned = new Set<number>();
// A sweep runs at most once per fill at a time; a re-kick while one runs is
// a no-op, and the next refresh re-kicks after it finishes.
private thumbRunning = false;
private originalsRunning = false;
// a no-op, and the next refresh re-kicks after it finishes. Each holds the
// running sweep, so `close()` can wait for it.
private thumbSweep?: Promise<void>;
private originalsSweep?: Promise<void>;
private readonly aborter = new AbortController();
private closed = false;
@@ -191,15 +192,19 @@ export class Precache {
}
// Stop the fills. In-flight fetches are left to settle; queued ones drop.
close(): void {
// Resolves once both sweeps have finished, so nothing is still writing.
async close(): Promise<void> {
this.closed = true;
this.aborter.abort();
await Promise.all([
this.thumbSweep?.catch(() => {}),
this.originalsSweep?.catch(() => {}),
]);
}
private kickThumbnails(): void {
if (this.thumbRunning) return;
this.thumbRunning = true;
void this.sweep(
if (this.thumbSweep) return;
this.thumbSweep = this.sweep(
"precacheThumbnails",
() => this.thumbOrder,
(id) => this.cache!.pathsFor(id).thumbnailPath !== undefined,
@@ -211,14 +216,13 @@ export class Precache {
signal: this.aborter.signal,
}),
).finally(() => {
this.thumbRunning = false;
this.thumbSweep = undefined;
});
}
private kickOriginals(): void {
if (this.originalsRunning) return;
this.originalsRunning = true;
void this.sweep(
if (this.originalsSweep) return;
this.originalsSweep = this.sweep(
"precacheOriginals",
() => this.originalsOrder,
(id) => this.cache!.pathsFor(id).originalPath !== undefined,
@@ -229,7 +233,7 @@ export class Precache {
signal: this.aborter.signal,
}),
).finally(() => {
this.originalsRunning = false;
this.originalsSweep = undefined;
});
}
+3 -3
View File
@@ -116,7 +116,7 @@ describe("Library content wiring", () => {
expect(lib.photos.byID({ fileID: 1 })!.record().thumbnailPath).toBe(
result.path,
);
lib.close();
await lib.close();
});
it("drives thumbnails.ensure through the cache", async () => {
@@ -139,7 +139,7 @@ describe("Library content wiring", () => {
expect(results).toEqual([
{ fileID: 1, path: join(root, "cache", "thumbnails", "1.jpg") },
]);
lib.close();
await lib.close();
});
it("throws from content methods when opened without a content source", async () => {
@@ -155,6 +155,6 @@ describe("Library content wiring", () => {
await expect(
lib.thumbnails.ensure({ fileIDs: [1], priority: "visible" }),
).rejects.toThrow(/content cache/i);
lib.close();
await lib.close();
});
});
+3 -3
View File
@@ -179,7 +179,7 @@ describe("Library.fresh", () => {
// And the change is now live for the default namespaces too.
expect(lib.photos.byID({ fileID: 1002 })?.fileID).toBe(1002);
} finally {
lib.close();
await lib.close();
}
});
@@ -228,7 +228,7 @@ describe("Library.fresh", () => {
expect(reads.photos.byID({ fileID: 1002 })?.fileID).toBe(1002);
}
} finally {
lib.close();
await lib.close();
}
});
@@ -266,7 +266,7 @@ describe("Library.fresh", () => {
);
expect(lib.status().lastError).toBeUndefined();
} finally {
lib.close();
await lib.close();
}
});
});
+89 -17
View File
@@ -17,7 +17,8 @@
* failure surfaces via `onProgress` ("failed") and `status()`, and a later
* success clears the error. `open()` itself resolves even when the first
* refresh fails (offline start from cache).
* 5. `close()` stops the timer and is idempotent.
* 5. `close()` stops the timer and is idempotent, and its promise resolves
* only once an in-flight refresh has written the cache file.
* 6. `cacheDirectory` defaults to the env-paths cache dir plus the user id.
* 7. `open()` branches on the cache: an empty cache awaits the first refresh
* (it has nothing to serve yet); an existing cache serves its copy at once
@@ -37,6 +38,11 @@
* short interval and `vi.waitFor`: a fake clock cannot settle the real
* fsync-and-rename cache write, and empty diffs never write, so the eventual
* state is stable to poll for.
*
* A refresh changes RAM before it writes the cache file, so a polled state can
* be visible while that write is still running. Every test therefore awaits
* `close()`, which waits for the in-flight refresh, before `afterEach` removes
* the directory.
*/
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
@@ -193,7 +199,7 @@ describe("Library.open and background refresh", () => {
expect(reloaded.getFile(1, 1001)?.id).toBe(1001);
expect(reloaded.collectionsSinceTime).toBe(100);
} finally {
lib.close();
await lib.close();
}
});
@@ -224,7 +230,7 @@ describe("Library.open and background refresh", () => {
expect(client.collectionsSinceTimes.length).toBe(collectionCalls);
expect(client.filesCalls.length).toBe(fileCalls);
} finally {
lib.close();
await lib.close();
}
});
@@ -271,7 +277,7 @@ describe("Library.open and background refresh", () => {
{ timeout: 2000, interval: 5 },
);
} finally {
lib.close();
await lib.close();
}
});
@@ -303,7 +309,7 @@ describe("Library.open and background refresh", () => {
// never re-fetched.
expect(client.filesCalls).toEqual([]);
} finally {
lib.close();
await lib.close();
}
});
@@ -357,7 +363,7 @@ describe("Library.open and background refresh", () => {
{ timeout: 2000, interval: 5 },
);
} finally {
lib.close();
await lib.close();
}
});
@@ -402,7 +408,7 @@ describe("Library.open and background refresh", () => {
await new Promise((r) => setTimeout(r, FAST_INTERVAL * 1000 * 4));
expect(saveSpy).toHaveBeenCalledTimes(2);
} finally {
lib.close();
await lib.close();
saveSpy.mockRestore();
}
});
@@ -462,7 +468,7 @@ describe("Library.open and background refresh", () => {
{ timeout: 2000, interval: 5 },
);
} finally {
lib.close();
await lib.close();
}
});
@@ -488,7 +494,7 @@ describe("Library.open and background refresh", () => {
),
).toBe(true);
} finally {
lib.close();
await lib.close();
}
});
@@ -502,9 +508,14 @@ describe("Library.open and background refresh", () => {
seed.putFile(file(1001, 1, 400));
await seed.save();
// The server never answers this run's first refresh.
// The server does not answer this run's first refresh until the test
// is done with it.
let answerFirstFetch: (page: CollectionsPage) => void = () => {};
const client = new MockClient();
client.collectionsSince = () => new Promise<CollectionsPage>(() => {});
client.collectionsSince = () =>
new Promise<CollectionsPage>((resolve) => {
answerFirstFetch = resolve;
});
// open() must resolve from the cache without blocking on the network,
// and reads must serve the seeded copy.
@@ -517,7 +528,9 @@ describe("Library.open and background refresh", () => {
expect(lib.status().lastRefreshAt).toBeUndefined();
expect(lib.status().lastError).toBeUndefined();
} finally {
lib.close();
// close() waits for the outstanding refresh, so let it finish.
answerFirstFetch({ collections: [], deleted: [], cursor: 500 });
await lib.close();
}
});
@@ -564,7 +577,7 @@ describe("Library.open and background refresh", () => {
expect(lib.listFiles(1).map((f) => f.id)).toEqual([1001]);
expect(lib.status().lastRefreshAt).toBeGreaterThan(0);
} finally {
lib.close();
await lib.close();
}
});
@@ -619,7 +632,7 @@ describe("Library.open and background refresh", () => {
);
expect(reloaded.getFile(1, 1001)?.id).toBe(1001);
} finally {
lib.close();
await lib.close();
saveSpy.mockRestore();
}
});
@@ -634,8 +647,8 @@ describe("Library.open and background refresh", () => {
});
const callsAfterOpen = client.collectionsSinceTimes.length;
lib.close();
lib.close(); // second close must not throw
await lib.close();
await lib.close(); // second close must not throw
expect(lib.status().closed).toBe(true);
// No further refreshes fire once closed.
@@ -643,6 +656,65 @@ describe("Library.open and background refresh", () => {
expect(client.collectionsSinceTimes.length).toBe(callsAfterOpen);
});
it("close() resolves only after an in-flight refresh has written the cache", async () => {
const path = join(cacheDirectory, "metadata.json");
const seed = await MetadataStore.load(path);
seed.userID = USER_ID;
seed.collectionsSinceTime = 500;
seed.putCollection(collection(1, 400));
seed.putFile(file(1001, 1, 400));
await seed.save();
const client = new MockClient();
client.collectionsQueue.push({
collections: [collection(1, 600)],
deleted: [],
cursor: 600,
});
client.filesFor(1, {
files: [file(1002, 1, 600)],
deleted: [],
cursor: 600,
});
// Hold the refresh's cache write until the test releases it.
const realSave = MetadataStore.prototype.save;
let releaseSave: () => void = () => {};
const saveHeld = new Promise<void>((resolve) => {
releaseSave = resolve;
});
const saveSpy = vi
.spyOn(MetadataStore.prototype, "save")
.mockImplementation(async function (this: MetadataStore) {
await saveHeld;
return realSave.call(this);
});
const lib = await Library.open({ client, cacheDirectory });
try {
await vi.waitFor(() => expect(saveSpy).toHaveBeenCalled(), {
timeout: 2000,
interval: 5,
});
let closed = false;
const closing = lib.close().then(() => {
closed = true;
});
await new Promise((r) => setTimeout(r, 50));
expect(closed).toBe(false);
releaseSave();
await closing;
const reloaded = await MetadataStore.load(path);
expect(reloaded.getFile(1, 1002)?.id).toBe(1002);
} finally {
releaseSave();
await lib.close();
saveSpy.mockRestore();
}
});
it("defaults cacheDirectory to the env-paths cache dir plus user id", async () => {
const xdg = join(dir, "xdg-cache");
const prev = process.env.XDG_CACHE_HOME;
@@ -659,7 +731,7 @@ describe("Library.open and background refresh", () => {
expect(lib.cacheDirectory.startsWith(xdg)).toBe(true);
expect(lib.cacheDirectory.endsWith(String(USER_ID))).toBe(true);
} finally {
lib.close();
await lib.close();
}
} finally {
if (prev === undefined) delete process.env.XDG_CACHE_HOME;
+49 -2
View File
@@ -374,7 +374,7 @@ describe("Library ML-data fetch on refresh", () => {
await new Promise((r) => setTimeout(r, FAST_INTERVAL * 1000 * 5));
expect(client.mlFetchCalls.length).toBe(callsAfterFirst);
} finally {
lib.close();
await lib.close();
}
});
@@ -443,7 +443,54 @@ describe("Library ML-data fetch on refresh", () => {
expect(client.mlFetchCalls.length).toBeGreaterThan(callsBefore);
expect(client.mlFetchCalls.flat()).toContain(1001);
} finally {
lib.close();
await lib.close();
}
});
it("close() resolves only after a running ML data fetch has stored its payloads", async () => {
const client = new MLMockClient();
client.collectionsQueue.push({
collections: [collection(1, 100)],
deleted: [],
cursor: 100,
});
client.filesFor(1, {
files: [file(1001, 1, 90)],
deleted: [],
cursor: 90,
});
client.mlByFile.set(1001, payload([0.5, 0.25, 0.75]));
// Hold the ML data fetch open until the test releases it.
let release!: () => void;
const held = new Promise<void>((r) => (release = r));
let fetchStarted!: () => void;
const started = new Promise<void>((r) => (fetchStarted = r));
const realFetch = client.fetchMLData.bind(client);
client.fetchMLData = async (args) => {
fetchStarted();
await held;
return realFetch(args);
};
const lib = await Library.open({
client,
cacheDirectory,
refreshIntervalSeconds: 3600,
});
await started;
let closed = false;
const closing = lib.close().then(() => {
closed = true;
});
await new Promise((r) => setTimeout(r, 20));
expect(closed).toBe(false);
release();
await closing;
expect(existsSync(join(cacheDirectory, "mldata", "1001.json"))).toBe(
true,
);
});
});
+66 -15
View File
@@ -402,16 +402,64 @@ describe("Precache through Library.open", () => {
}
it("starts both precaches from open() and reports them in status()", async () => {
const thumbFetched = new Set<number>();
const origFetched = new Set<number>();
const source: ContentSource = {
original: async ({ file: f, destination }) => {
origFetched.add(f.id);
original: async ({ destination }) => {
await writeFile(destination, Buffer.alloc(10, 1));
return { bytesWritten: 10 };
},
thumbnail: async ({ file: f, destination }) => {
thumbFetched.add(f.id);
thumbnail: async ({ destination }) => {
await writeFile(destination, Buffer.alloc(10, 1));
return { bytesWritten: 10 };
},
};
// Each fill reports "done" once the cache has recorded its files. The
// source returning is not enough: the cache records a file only after
// it has checked it on disk.
const finished = new Set<string>();
let bothFinished!: () => void;
const precached = new Promise<void>((r) => (bothFinished = r));
const lib = await Library.open({
client: new MockClient(),
cacheDirectory: join(root, "cache"),
contentSource: source,
refreshIntervalSeconds: 3600,
onProgress: (e) => {
if (
e.status === "done" &&
(e.operation === "precacheThumbnails" ||
e.operation === "precacheOriginals")
) {
finished.add(e.operation);
if (finished.size === 2) bothFinished();
}
},
});
// Every file's thumbnail is precached; the favorite (file 3) and the
// week's files (1, 2) all have their originals precached.
await precached;
const status = lib.status();
expect(status.thumbnailsTotal).toBe(3);
expect(status.thumbnailsCached).toBe(3);
expect(status.originalsPinned).toBe(3);
expect(status.originalsCached).toBe(3);
await lib.close();
});
it("close() resolves only after a running precache fetch has written its file", async () => {
// Every thumbnail fetch waits until the test releases it.
let release!: () => void;
const held = new Promise<void>((r) => (release = r));
let fetchStarted!: (destination: string) => void;
const started = new Promise<string>((r) => (fetchStarted = r));
const source: ContentSource = {
original: async ({ destination }) => {
await writeFile(destination, Buffer.alloc(10, 1));
return { bytesWritten: 10 };
},
thumbnail: async ({ destination }) => {
fetchStarted(destination);
await held;
await writeFile(destination, Buffer.alloc(10, 1));
return { bytesWritten: 10 };
},
@@ -421,16 +469,19 @@ describe("Precache through Library.open", () => {
cacheDirectory: join(root, "cache"),
contentSource: source,
refreshIntervalSeconds: 3600,
precacheOriginals: false,
});
const destination = await started;
// Every file's thumbnail is precached; the favorite (file 3) and the
// week's files (1, 2) all have their originals precached.
await until(() => thumbFetched.size === 3 && origFetched.size === 3);
const status = lib.status();
expect(status.thumbnailsTotal).toBe(3);
expect(status.thumbnailsCached).toBe(3);
expect(status.originalsPinned).toBe(3);
expect(status.originalsCached).toBe(3);
lib.close();
let closed = false;
const closing = lib.close().then(() => {
closed = true;
});
await new Promise((r) => setTimeout(r, 20));
expect(closed).toBe(false);
release();
await closing;
expect(existsSync(destination)).toBe(true);
});
});
+1 -1
View File
@@ -531,7 +531,7 @@ describe("Library exposes the read surface over its live store", () => {
expect(client.collectionsCalls).toBe(collectionsBefore);
expect(client.filesCalls).toBe(filesBefore);
} finally {
lib.close();
await lib.close();
}
});
});
+4 -4
View File
@@ -159,7 +159,7 @@ describe("Library.snapshot and Library.subscribe", () => {
for (const p of snap.photos) expect("key" in p).toBe(false);
for (const a of snap.albums) expect("key" in a).toBe(false);
} finally {
lib.close();
await lib.close();
}
});
@@ -219,7 +219,7 @@ describe("Library.snapshot and Library.subscribe", () => {
expect(change.refreshedAt).toBeGreaterThan(0);
} finally {
unsubscribe();
lib.close();
await lib.close();
}
});
@@ -251,7 +251,7 @@ describe("Library.snapshot and Library.subscribe", () => {
expect(changes).toEqual([]);
} finally {
unsubscribe();
lib.close();
await lib.close();
}
});
@@ -297,7 +297,7 @@ describe("Library.snapshot and Library.subscribe", () => {
);
expect(changes).toEqual([]);
} finally {
lib.close();
await lib.close();
}
});
});