From c4d41799389e8d887e8a47056afe4cdd4037acd6 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 23 Sep 2026 01:26:12 +0000 Subject: [PATCH] Fix two intermittently failing library tests (closes #90) 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 --- README.md | 7 +- TODO.md | 7 ++ src/cli-commands.ts | 16 ++-- src/library/index.ts | 27 ++++--- src/library/precache.ts | 28 ++++--- test/library/content-library.test.ts | 6 +- test/library/fresh.test.ts | 6 +- test/library/library.test.ts | 106 ++++++++++++++++++++++----- test/library/mldata.test.ts | 56 +++++++++++++- test/library/precache.test.ts | 86 ++++++++++++++++++---- test/library/read.test.ts | 2 +- test/library/snapshot.test.ts | 8 +- 12 files changed, 278 insertions(+), 77 deletions(-) diff --git a/README.md b/README.md index 2fd00d2..b98882b 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/TODO.md b/TODO.md index 29a1c3f..5b17221 100644 --- a/TODO.md +++ b/TODO.md @@ -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. diff --git a/src/cli-commands.ts b/src/cli-commands.ts index fc02d40..d72b607 100644 --- a/src/cli-commands.ts +++ b/src/cli-commands.ts @@ -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(); } }; diff --git a/src/library/index.ts b/src/library/index.ts index 3b184cb..df1199c 100644 --- a/src/library/index.ts +++ b/src/library/index.ts @@ -266,8 +266,9 @@ export class Library { // that, a fresh read propagates it. private cycle?: Promise; // 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; 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 { 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(); @@ -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; } } diff --git a/src/library/precache.ts b/src/library/precache.ts index f604eaa..472d627 100644 --- a/src/library/precache.ts +++ b/src/library/precache.ts @@ -92,9 +92,10 @@ export class Precache { private pinned = new Set(); // 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; + private originalsSweep?: Promise; 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 { 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; }); } diff --git a/test/library/content-library.test.ts b/test/library/content-library.test.ts index a89f47c..fe60236 100644 --- a/test/library/content-library.test.ts +++ b/test/library/content-library.test.ts @@ -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(); }); }); diff --git a/test/library/fresh.test.ts b/test/library/fresh.test.ts index 15b71b7..10b8ee4 100644 --- a/test/library/fresh.test.ts +++ b/test/library/fresh.test.ts @@ -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(); } }); }); diff --git a/test/library/library.test.ts b/test/library/library.test.ts index fe951c3..2669871 100644 --- a/test/library/library.test.ts +++ b/test/library/library.test.ts @@ -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(() => {}); + client.collectionsSince = () => + new Promise((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((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; diff --git a/test/library/mldata.test.ts b/test/library/mldata.test.ts index 6022db7..1dfa0c4 100644 --- a/test/library/mldata.test.ts +++ b/test/library/mldata.test.ts @@ -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,59 @@ 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((r) => (release = r)); + let fetchStarted!: () => void; + const started = new Promise((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, + }); + try { + 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); + } finally { + release(); + await lib.close(); } }); }); diff --git a/test/library/precache.test.ts b/test/library/precache.test.ts index 2580579..0c0e777 100644 --- a/test/library/precache.test.ts +++ b/test/library/precache.test.ts @@ -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(); - const origFetched = new Set(); 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(); + let bothFinished!: () => void; + const precached = new Promise((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((r) => (release = r)); + let fetchStarted!: (destination: string) => void; + const started = new Promise((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,24 @@ describe("Precache through Library.open", () => { cacheDirectory: join(root, "cache"), contentSource: source, refreshIntervalSeconds: 3600, + precacheOriginals: false, }); + try { + 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); + } finally { + release(); + await lib.close(); + } }); }); diff --git a/test/library/read.test.ts b/test/library/read.test.ts index 102bdee..2fe394a 100644 --- a/test/library/read.test.ts +++ b/test/library/read.test.ts @@ -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(); } }); }); diff --git a/test/library/snapshot.test.ts b/test/library/snapshot.test.ts index 511b4c4..c0f306c 100644 --- a/test/library/snapshot.test.ts +++ b/test/library/snapshot.test.ts @@ -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(); } }); });