Library.open with a transparent background refresh loop #60

Merged
clawbot merged 1 commits from issue-42-library-open-refresh into next 2026-09-22 14:52:02 +02:00
Collaborator

Implements Library.open() and the transparent background refresh loop over the local cache.

open() loads the metadata store, runs one refresh, then a background timer refreshes every refreshIntervalSeconds (default 3). Every read is answered from RAM and never triggers a network call. Per the settled design there is no sync(), no refresh(), no serverReachable flag, and no before-each-read mode.

A refresh does all network reads first (the collections diff since the stored cursor, then per-collection file diffs) and commits to the store only after every fetch succeeds, so a failed refresh is invisible to reads — the last good snapshot stays and the failure surfaces via onProgress ("failed") and status(). The cache is rewritten only when something changed.

Builds on the metadata store (#41) and the resumable, tombstone-aware enumerators (#38); neither is reimplemented.

Not obvious from the diff:

  • A collection's files are re-enumerated only when its updationTime advances past the cached copy, using that cached updationTime as the per-collection file cursor — so no store schema change was needed.
  • The read surface is intentionally minimal (collections and file memberships); the album/photo/timeline surface belongs to later phases.
  • The interval tests use real timers with a short interval: 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.

make check passes (tests + Docker lint).

Model: opus-4-8

Implements `Library.open()` and the transparent background refresh loop over the local cache. `open()` loads the metadata store, runs one refresh, then a background timer refreshes every `refreshIntervalSeconds` (default 3). Every read is answered from RAM and never triggers a network call. Per the settled design there is no `sync()`, no `refresh()`, no `serverReachable` flag, and no before-each-read mode. A refresh does all network reads first (the collections diff since the stored cursor, then per-collection file diffs) and commits to the store only after every fetch succeeds, so a failed refresh is invisible to reads — the last good snapshot stays and the failure surfaces via `onProgress` ("failed") and `status()`. The cache is rewritten only when something changed. Builds on the metadata store (https://git.eeqj.de/sneak/quak/issues/41) and the resumable, tombstone-aware enumerators (https://git.eeqj.de/sneak/quak/issues/38); neither is reimplemented. Not obvious from the diff: - A collection's files are re-enumerated only when its `updationTime` advances past the cached copy, using that cached `updationTime` as the per-collection file cursor — so no store schema change was needed. - The read surface is intentionally minimal (collections and file memberships); the album/photo/timeline surface belongs to later phases. - The interval tests use real timers with a short interval: 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. `make check` passes (tests + Docker lint). Model: opus-4-8
clawbot added the needs-review label 2026-09-22 14:01:58 +02:00
clawbot self-assigned this 2026-09-22 14:01:58 +02:00
Author
Collaborator

FAIL — needs-rework.

1. open() always blocks on the first network refresh; a later run must not.
src/library/index.ts, Library.open() (the await lib.runRefresh(); lib.scheduleNext(); at the end) awaits the first refresh unconditionally, regardless of whether an existing local copy was loaded. Issue #42 requires: a first run with no local copy resolves only when the first fetch completes, but a later run resolves as soon as the local copy is loaded and the first refresh starts — it must not block on the network. As written, opening against an existing cache with a slow or unreachable server hangs until the network responds or fails, which defeats the phase's whole point (reads served from the local copy with no network in front of a caller) at startup for the common case. Acceptable: branch on whether the store loaded an existing copy — empty cache awaits the first refresh; existing cache kicks the first refresh off in the background (not awaited) and returns once the local copy is loaded. The required first-run-vs-later-run test is effectively missing: no test proves a later-run open() resolves before a slow refresh completes. Add one where collectionsSince never resolves yet open() still resolves and reads serve the cached data, alongside a first-run case that does await.

2. A failed save() leaves RAM ahead of disk and then silently clears the error. (lower severity)
src/library/index.ts, refreshOnce() mutates the in-memory store before await this.store.save(); runRefresh() clears lastError on the next successful tick. If every fetch succeeds but the persist fails, the in-memory store already holds the new snapshot and advanced cursor while metadata.json keeps the old one. The next tick fetches from the advanced in-RAM cursor, sees nothing changed, does not re-save, and clears lastError — so the on-disk cache stays stale indefinitely and the persist failure disappears from status(). Reads stay correct from RAM and the cache self-heals on restart, so this is minor, but it diverges from "commits to the store only once every fetch succeeds" and hides a persistent write failure. Acceptable: keep lastError set until a save actually succeeds, or roll the in-memory mutation back on save failure so RAM and disk stay in lockstep.

Everything else gated cleanly: base is current next and rebases with no conflict; make check is green here (tests plus a fresh Docker eslint/prettier run); make fmt clean; reads are answered from RAM with no network; the store (#41) and enumerators (#38) are reused, not reimplemented; tombstones, cursor threading, and rewrite-only-on-change behave; close()/status() behave; no sync()/refresh()/serverReachable/before-each-read surface; TODO.md untouched; squash subject carries (closes #42); the PR body and commit both carry a Model: line with no unwanted attribution.

Model: opus-4-8

**FAIL** — needs-rework. **1. `open()` always blocks on the first network refresh; a later run must not.** `src/library/index.ts`, `Library.open()` (the `await lib.runRefresh(); lib.scheduleNext();` at the end) awaits the first refresh unconditionally, regardless of whether an existing local copy was loaded. Issue #42 requires: a first run with no local copy resolves only when the first fetch completes, but a later run resolves as soon as the local copy is loaded and the first refresh *starts* — it must not block on the network. As written, opening against an existing cache with a slow or unreachable server hangs until the network responds or fails, which defeats the phase's whole point (reads served from the local copy with no network in front of a caller) at startup for the common case. Acceptable: branch on whether the store loaded an existing copy — empty cache awaits the first refresh; existing cache kicks the first refresh off in the background (not awaited) and returns once the local copy is loaded. The required first-run-vs-later-run test is effectively missing: no test proves a later-run `open()` resolves before a slow refresh completes. Add one where `collectionsSince` never resolves yet `open()` still resolves and reads serve the cached data, alongside a first-run case that does await. **2. A failed `save()` leaves RAM ahead of disk and then silently clears the error.** (lower severity) `src/library/index.ts`, `refreshOnce()` mutates the in-memory store before `await this.store.save()`; `runRefresh()` clears `lastError` on the next successful tick. If every fetch succeeds but the persist fails, the in-memory store already holds the new snapshot and advanced cursor while `metadata.json` keeps the old one. The next tick fetches from the advanced in-RAM cursor, sees nothing changed, does not re-save, and clears `lastError` — so the on-disk cache stays stale indefinitely and the persist failure disappears from `status()`. Reads stay correct from RAM and the cache self-heals on restart, so this is minor, but it diverges from "commits to the store only once every fetch succeeds" and hides a persistent write failure. Acceptable: keep `lastError` set until a save actually succeeds, or roll the in-memory mutation back on save failure so RAM and disk stay in lockstep. Everything else gated cleanly: base is current `next` and rebases with no conflict; `make check` is green here (tests plus a fresh Docker eslint/prettier run); `make fmt` clean; reads are answered from RAM with no network; the store (#41) and enumerators (#38) are reused, not reimplemented; tombstones, cursor threading, and rewrite-only-on-change behave; `close()`/`status()` behave; no `sync()`/`refresh()`/`serverReachable`/before-each-read surface; `TODO.md` untouched; squash subject carries `(closes #42)`; the PR body and commit both carry a `Model:` line with no unwanted attribution. Model: opus-4-8
clawbot added needs-rework and removed needs-review labels 2026-09-22 14:15:16 +02:00
clawbot added 1 commit 2026-09-22 14:31:44 +02:00
Add the Library surface over the on-disk metadata store. open() loads the
cache, then starts the refresh loop, branching on what was cached: an empty
cache awaits the first refresh so open() resolves onto populated data, while an
existing cache serves its copy immediately and refreshes in the background, so a
slow or unreachable server never stalls opening. A background timer then
refreshes every refreshIntervalSeconds (default 3). Reads are answered from RAM
and never touch the network; there is no sync(), no refresh(), no
serverReachable flag, and no before-each-read mode.

A refresh does all its network reads first and commits to the store only once
every fetch succeeds, so a failed refresh is invisible to reads: the last good
snapshot stays and the failure surfaces via onProgress ("failed") and status().
The cache is rewritten only when something actually changed. A commit that
mutates RAM but then fails to persist keeps status().lastError set and keeps
retrying the write until a save lands, so RAM never runs ahead of disk with the
failure masked by a later empty refresh.

A collection's files are re-enumerated only when its updationTime advances past
the cached copy, using that cached updationTime as the per-collection file
cursor, so no store schema change is needed. Reads expose only what this phase
needs (collections and file memberships); the album/photo/timeline surface is
later phases. Interval tests use real timers with a short interval because a
fake clock cannot settle the real fsync-and-rename cache write.

Model: opus-4-8
clawbot force-pushed issue-42-library-open-refresh from 501d623985 to 83878e1898 2026-09-22 14:31:44 +02:00 Compare
Author
Collaborator

Reworked; head now 83878e18984edcc04a8e835541386ebf80a53cf8.

  • Finding 1: open() now branches on the loaded cache — an empty cache still awaits the first refresh, an existing cache serves its copy at once and refreshes in the background, so an unreachable server no longer stalls opening. Added a later-run test (unresolved collectionsSince, open() still resolves and reads serve the cache) and a first-run test that proves the initial refresh is awaited.
  • Finding 2: a failed save() now keeps the store marked unsaved and keeps retrying on later refreshes, so status().lastError stays set until a save actually lands and a stale disk is never masked. Added a test that a persistent save failure keeps the error and clock from advancing, then clears only once the write succeeds.

Model: opus-4-8

Reworked; head now `83878e18984edcc04a8e835541386ebf80a53cf8`. - Finding 1: `open()` now branches on the loaded cache — an empty cache still awaits the first refresh, an existing cache serves its copy at once and refreshes in the background, so an unreachable server no longer stalls opening. Added a later-run test (unresolved `collectionsSince`, `open()` still resolves and reads serve the cache) and a first-run test that proves the initial refresh is awaited. - Finding 2: a failed `save()` now keeps the store marked unsaved and keeps retrying on later refreshes, so `status().lastError` stays set until a save actually lands and a stale disk is never masked. Added a test that a persistent save failure keeps the error and clock from advancing, then clears only once the write succeeds. Model: opus-4-8
clawbot added needs-review and removed needs-rework labels 2026-09-22 14:38:53 +02:00
Author
Collaborator

FAIL — needs-rework.

Both prior findings are genuinely fixed, each with a meaningful mock-Client test: open() now awaits the first refresh only on an empty cache and kicks it off in the background on an existing one (a never-resolving collectionsSince still lets open() resolve and serve the cached copy), and a failed save() keeps status().lastError set and keeps retrying the write until one lands, so a stale disk is never masked. The rest of the #42 contract and the gate hold: reads served from RAM with no network surface; the store (#41) and enumerators (#38) are reused, not reimplemented; changed-album-only diffs, tombstones, cursor threading, and rewrite-only-on-change behave; status()/close() behave; base is current next and rebases cleanly; make check is green here; make fmt clean; TODO.md untouched; subject carries (closes #42); Model: lines present with no unwanted attribution.

One finding:

Commit body exceeds the length limit. The landing commit 83878e1 has a ~244-word, three-paragraph body — roughly double the ~120-word limit for a commit message body. It largely restates the PR body and the source-file header comments. Acceptable: trim to ~120 words. The branch/await split, the stage-all-fetches-then-commit refresh, and the unsaved-and-retry behavior can be one sentence each; the fuller explanation already lives in the code comments and the PR description.

Model: opus-4-8

**FAIL** — needs-rework. Both prior findings are genuinely fixed, each with a meaningful mock-`Client` test: `open()` now awaits the first refresh only on an empty cache and kicks it off in the background on an existing one (a never-resolving `collectionsSince` still lets `open()` resolve and serve the cached copy), and a failed `save()` keeps `status().lastError` set and keeps retrying the write until one lands, so a stale disk is never masked. The rest of the #42 contract and the gate hold: reads served from RAM with no network surface; the store (#41) and enumerators (#38) are reused, not reimplemented; changed-album-only diffs, tombstones, cursor threading, and rewrite-only-on-change behave; `status()`/`close()` behave; base is current `next` and rebases cleanly; `make check` is green here; `make fmt` clean; `TODO.md` untouched; subject carries `(closes #42)`; `Model:` lines present with no unwanted attribution. One finding: **Commit body exceeds the length limit.** The landing commit `83878e1` has a ~244-word, three-paragraph body — roughly double the ~120-word limit for a commit message body. It largely restates the PR body and the source-file header comments. Acceptable: trim to ~120 words. The branch/await split, the stage-all-fetches-then-commit refresh, and the unsaved-and-retry behavior can be one sentence each; the fuller explanation already lives in the code comments and the PR description. Model: opus-4-8
Author
Collaborator

Sole finding is commit-body length. The landing commit is the squash commit, whose message the manager writes at merge time — using a ~120-word body, so nothing over-length enters history. Both code findings from the first pass were verified fixed and the gate is green; merging.

Model: opus-4-8

Sole finding is commit-body length. The landing commit is the squash commit, whose message the manager writes at merge time — using a ~120-word body, so nothing over-length enters history. Both code findings from the first pass were verified fixed and the gate is green; merging. Model: opus-4-8
clawbot merged commit 7570055a5b into next 2026-09-22 14:52:02 +02:00
clawbot deleted branch issue-42-library-open-refresh 2026-09-22 14:52:03 +02:00
Sign in to join this conversation.