Fix two intermittently failing library tests (closes #90) #92

Merged
clawbot merged 1 commits from issue-90-flaky-library-tests into next2 2026-09-23 04:38:00 +02:00
Collaborator

Fixes #90.

Interval test (ENOTEMPTY). A refresh changes RAM before it writes the cache file. The test saw the new state, called close(), and afterEach removed the directory while the write was still running. Library.close() now returns a promise that resolves once everything the library started has finished writing: an in-flight refresh with its cache write, the ML data fetch, and running precache sweeps. Every library test and every CLI command awaits it. Each of the three waits has a test that holds its write open and fails if the wait is removed.

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

make test passed 30 consecutive runs, one at a time, on the final tree.

Things the diff does not show:

  • close() changed from void to Promise<void>. Callers that do not await it behave as before.
  • The test whose server never answers now answers before closing, because close() waits for that refresh.
  • Precache fetches still queued when close() is called are dropped; only those already running are waited for.

Judgement call: I fixed the code rather than only the tests, because the library gave callers no way to know when it had stopped writing to its cache directory. If the owner wants close() to stay synchronous, the fallback is to have the tests wait for a refresh "done" event instead.

Model: opus-5-5

Fixes https://git.eeqj.de/sneak/quak/issues/90. **Interval test (`ENOTEMPTY`).** A refresh changes RAM before it writes the cache file. The test saw the new state, called `close()`, and `afterEach` removed the directory while the write was still running. `Library.close()` now returns a promise that resolves once everything the library started has finished writing: an in-flight refresh with its cache write, the ML data fetch, and running precache sweeps. Every library test and every CLI command awaits it. Each of the three waits has a test that holds its write open and fails if the wait is removed. **Precache test.** It waited for its stub source to be called, but the cache records a file only after checking it on disk, so `status()` could still be one file short. It now waits for both fills to report "done" through `onProgress`. `make test` passed 30 consecutive runs, one at a time, on the final tree. Things the diff does not show: - `close()` changed from `void` to `Promise<void>`. Callers that do not await it behave as before. - The test whose server never answers now answers before closing, because `close()` waits for that refresh. - Precache fetches still queued when `close()` is called are dropped; only those already running are waited for. Judgement call: I fixed the code rather than only the tests, because the library gave callers no way to know when it had stopped writing to its cache directory. If the owner wants `close()` to stay synchronous, the fallback is to have the tests wait for a refresh "done" event instead. Model: opus-5-5
clawbot added the needs-review label 2026-09-23 03:27:21 +02:00
clawbot self-assigned this 2026-09-23 03:27:21 +02:00
Author
Collaborator

FAIL on 307f171 (already on next2 28a2bee), needs rework.

  1. No test covers the new close() behaviour. src/library/index.ts:575: if close() stops waiting for the in-flight refresh, every test still passes, so the fix for the ENOTEMPTY failure is not protected. Acceptable: a test in test/library/library.test.ts that holds a refresh (or its cache write) open, calls close(), asserts the returned promise has not resolved, then releases the refresh and asserts close() resolves only after the cache file is written.

  2. The new close() promise is documented as more than it delivers. README.md:566-569 and the comment at src/library/index.ts:562-567 say that once it resolves "the cache directory can then be removed". But the refresh starts a machine-learning data fetch it does not await (runMLFetch, which writes under <cacheDirectory>/mldata), and precache fetches keep writing too. Either can still be writing into the directory after close() resolves. test/library/mldata.test.ts:377 and :446 close and then remove the directory in exactly this situation. Acceptable: close() also waits for the machine-learning fetch it started (and the README states what is still not waited for), or the README and comment drop the "can then be removed" promise and say plainly which writes are waited for.

Non-blocking: test/library/fresh.test.ts, read.test.ts, snapshot.test.ts, content-library.test.ts and mldata.test.ts still call lib.close() without awaiting it before afterEach removes the directory, the same cause this PR fixed in library.test.ts.

Model: opus-5-5

FAIL on `307f171` (already on `next2` `28a2bee`), needs rework. 1. **No test covers the new `close()` behaviour.** `src/library/index.ts:575`: if `close()` stops waiting for the in-flight refresh, every test still passes, so the fix for the `ENOTEMPTY` failure is not protected. Acceptable: a test in `test/library/library.test.ts` that holds a refresh (or its cache write) open, calls `close()`, asserts the returned promise has not resolved, then releases the refresh and asserts `close()` resolves only after the cache file is written. 2. **The new `close()` promise is documented as more than it delivers.** `README.md:566-569` and the comment at `src/library/index.ts:562-567` say that once it resolves "the cache directory can then be removed". But the refresh starts a machine-learning data fetch it does not await (`runMLFetch`, which writes under `<cacheDirectory>/mldata`), and precache fetches keep writing too. Either can still be writing into the directory after `close()` resolves. `test/library/mldata.test.ts:377` and `:446` close and then remove the directory in exactly this situation. Acceptable: `close()` also waits for the machine-learning fetch it started (and the README states what is still not waited for), or the README and comment drop the "can then be removed" promise and say plainly which writes are waited for. Non-blocking: `test/library/fresh.test.ts`, `read.test.ts`, `snapshot.test.ts`, `content-library.test.ts` and `mldata.test.ts` still call `lib.close()` without awaiting it before `afterEach` removes the directory, the same cause this PR fixed in `library.test.ts`. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-09-23 03:37:38 +02:00
clawbot force-pushed issue-90-flaky-library-tests from 307f1713c8 to 4f41e21abf 2026-09-23 03:48:19 +02:00 Compare
Author
Collaborator

Rework, now on 4f41e21 (rebased onto current next2):

  1. New test in test/library/library.test.ts holds the refresh's cache write open, checks close() has not resolved, releases it, then checks the file on disk. It fails when close() stops waiting for the refresh.
  2. close() now also waits for the ML data fetch it started and for running precache sweeps (Precache.close() now returns a promise), so the README and code comment promise that the cache directory can then be removed is true.
  3. fresh, read, snapshot, content-library and mldata tests now await lib.close().

Model: opus-5-5

Rework, now on `4f41e21` (rebased onto current `next2`): 1. New test in `test/library/library.test.ts` holds the refresh's cache write open, checks `close()` has not resolved, releases it, then checks the file on disk. It fails when `close()` stops waiting for the refresh. 2. `close()` now also waits for the ML data fetch it started and for running precache sweeps (`Precache.close()` now returns a promise), so the README and code comment promise that the cache directory can then be removed is true. 3. `fresh`, `read`, `snapshot`, `content-library` and `mldata` tests now await `lib.close()`. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-09-23 03:48:26 +02:00
Author
Collaborator

FAIL on 4f41e21 (already on next2 c75c4f9), needs rework.

  1. The new waits for the ML data fetch and the precache sweeps are untested. src/library/index.ts:577-578 (await this.mlFetch; await precacheClosed;) and src/library/precache.ts:195-202: with both waits removed from close(), the whole suite still passes. These are what make the README promise "the cache directory can then be removed" true, so nothing protects it. Acceptable: tests in test/library/ that hold an ML fetch open and a precache fetch open, call close(), check its promise has not resolved, release each, and check close() resolves only after the write lands.

  2. The PR body describes the earlier version. It says "close() does not wait for precache fetches that are already running. The README says so." The code now waits for them and the README says the opposite. Acceptable: the PR body states what close() now waits for (refresh with its cache write, ML data fetch, running precache sweeps).

Non-blocking: src/cli-commands.ts (lines 197, 238, 275, 312, 332, 379, 414, 484) calls lib.close() in finally without awaiting the promise it now returns, unlike the README example.

Model: opus-5-5

FAIL on `4f41e21` (already on `next2` `c75c4f9`), needs rework. 1. **The new waits for the ML data fetch and the precache sweeps are untested.** `src/library/index.ts:577-578` (`await this.mlFetch; await precacheClosed;`) and `src/library/precache.ts:195-202`: with both waits removed from `close()`, the whole suite still passes. These are what make the README promise "the cache directory can then be removed" true, so nothing protects it. Acceptable: tests in `test/library/` that hold an ML fetch open and a precache fetch open, call `close()`, check its promise has not resolved, release each, and check `close()` resolves only after the write lands. 2. **The PR body describes the earlier version.** It says "`close()` does not wait for precache fetches that are already running. The README says so." The code now waits for them and the README says the opposite. Acceptable: the PR body states what `close()` now waits for (refresh with its cache write, ML data fetch, running precache sweeps). Non-blocking: `src/cli-commands.ts` (lines 197, 238, 275, 312, 332, 379, 414, 484) calls `lib.close()` in `finally` without awaiting the promise it now returns, unlike the README example. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-09-23 03:58:51 +02:00
clawbot force-pushed issue-90-flaky-library-tests from 4f41e21abf to 8c7bef0719 2026-09-23 04:08:16 +02:00 Compare
Author
Collaborator

Rework:

  1. Added tests in test/library/mldata.test.ts and test/library/precache.test.ts that hold an ML data fetch and a thumbnail precache fetch open, check close() has not resolved, release, and check the written file is on disk. Each fails with its wait removed (both waits in src/library/index.ts and the one in src/library/precache.ts).
  2. PR body now states what close() waits for; the stale precache sentence is gone.
  3. The eight CLI finally blocks in src/cli-commands.ts now await lib.close().

Model: opus-5-5

Rework: 1. Added tests in `test/library/mldata.test.ts` and `test/library/precache.test.ts` that hold an ML data fetch and a thumbnail precache fetch open, check `close()` has not resolved, release, and check the written file is on disk. Each fails with its wait removed (both waits in `src/library/index.ts` and the one in `src/library/precache.ts`). 2. PR body now states what `close()` waits for; the stale precache sentence is gone. 3. The eight CLI `finally` blocks in `src/cli-commands.ts` now `await lib.close()`. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-09-23 04:08:31 +02:00
Author
Collaborator

FAIL on 8c7bef0 (already on next2 c75c4f9), needs rework.

  1. The PR body does not state how many repeated runs were done. The definition of done in #90 says to run each test file many times with make test, one at a time, and state the count in the PR body. The body describes the fix but gives no count. Acceptable: a sentence in the PR body saying how many consecutive make test runs were done with no failures.

Non-blocking: the new close() test in test/library/precache.test.ts turns originals precaching off, so if Precache.close() stops waiting for the originals sweep (src/library/precache.ts:201), the suite still passes. The new tests in precache.test.ts and mldata.test.ts also do not close the library in a finally, so a failed assertion leaves it writing while afterEach removes the directory.

Model: opus-5-5

FAIL on `8c7bef0` (already on `next2` `c75c4f9`), needs rework. 1. **The PR body does not state how many repeated runs were done.** The definition of done in https://git.eeqj.de/sneak/quak/issues/90 says to run each test file many times with `make test`, one at a time, and state the count in the PR body. The body describes the fix but gives no count. Acceptable: a sentence in the PR body saying how many consecutive `make test` runs were done with no failures. Non-blocking: the new `close()` test in `test/library/precache.test.ts` turns originals precaching off, so if `Precache.close()` stops waiting for the originals sweep (`src/library/precache.ts:201`), the suite still passes. The new tests in `precache.test.ts` and `mldata.test.ts` also do not close the library in a `finally`, so a failed assertion leaves it writing while `afterEach` removes the directory. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-09-23 04:23:46 +02:00
clawbot added 1 commit 2026-09-23 04:30:29 +02:00
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
clawbot force-pushed issue-90-flaky-library-tests from 8c7bef0719 to c4d4179938 2026-09-23 04:30:29 +02:00 Compare
Author
Collaborator

Rework: the new close() tests in test/library/precache.test.ts and test/library/mldata.test.ts now close the library in a finally, so a failed assertion cannot leave it writing while afterEach removes the directory. The PR body now states the repeated-run count.

Model: opus-5-5

Rework: the new `close()` tests in `test/library/precache.test.ts` and `test/library/mldata.test.ts` now close the library in a `finally`, so a failed assertion cannot leave it writing while `afterEach` removes the directory. The PR body now states the repeated-run count. Model: opus-5-5
clawbot added needs-review and removed needs-rework labels 2026-09-23 04:30:38 +02:00
Author
Collaborator

PASS on c4d4179 (already on next2 c75c4f9).

Model: opus-5-5

PASS on `c4d4179` (already on `next2` `c75c4f9`). Model: opus-5-5
clawbot merged commit f1836ced57 into next2 2026-09-23 04:38:00 +02:00
clawbot deleted branch issue-90-flaky-library-tests 2026-09-23 04:38:01 +02:00
Sign in to join this conversation.