Skip to content

Commit 0871bf8

Browse files
adulbrichclaude
andauthored
Persist RAW-to-TIFF conversions across reloads (#243) (#249)
* test(perf): benchmark the web build against local and deployed hosts "The deployed site feels slower than the local build" is not a question inspection can answer: the static export and `out/` are the same bytes, so any difference has to be delivery, and sizing delivery against compute needs measurement. Measured on the CR2 fixture bracket, Chromium both sides: a full JPEG generate takes 26,963 ms local against 31,973 ms deployed, and a 3-frame RAW import 5968 ms against 5959 ms. The deployed penalty is a flat ~5 s of one-time tool fetches, not a multiplier -- RAW conversion shows none at all. Requests are counted through `page.on(...)` rather than Resource Timing, because the wasm is fetched from inside a Worker and a worker's entries never land on the page's performance timeline. The CR2 mode waits on the `<canvas>` each thumbnail appends, not on the container div. Mounting is not gated on conversion, so an earlier draft reported a 269 ms import of ten 21.7 MB frames with zero `/wasm/` requests -- it exited before conversion began. `pipeline.spec.ts` documents the same trap for the responsiveness test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: design for persisting RAW conversions across reloads Closes the gap #243 describes: the RAW-to-TIFF cache lives in memory and dies with the tab, so a reload reconverts every frame -- ~19 s for a 10-frame bracket, and a regression for desktop, which kept this on disk before the WebAssembly port. A persistent tier goes behind the session tier, inside the RAW worker. `createSyncAccessHandle()` is worker-only, and the worker already holds the source bytes, so it can hash them without a second read and the content hash never crosses back to the page. The `tiffFor` seam needs no change. Two design points beyond the issue. The key folds in a tool tag derived from the `dcraw_emu` commit and the `dcrawArgs` flags: without it, rebuilding the wasm would silently serve pixels from a different demosaic, undoing the byte-identical guarantee `raw-preview.ts` exists to hold. And the key must be a content hash rather than a path, because `registerSessionFile` mints `/session/${nextId()}/${name}` from a counter that restarts each session -- so the same string names different bytes across sessions. The OPFS-versus-IndexedDB choice is left to a probe rather than assumed. OPFS is used nowhere in the codebase today and has never been tested in the three Tauri webviews. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: implementation plan for the persistent RAW cache Ten tasks, probe first. The backend choice is deferred to Task 1's result rather than assumed, with the decision rule stated in advance so the outcome is not rationalised after the fact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: settle the OPFS write path by measurement, and tighten two tests The plan chose createWritable over createSyncAccessHandle by reasoning. The probe now times both per host and the choice follows a stated rule, since the sync handle is also the spec's reason for putting this tier in a worker -- a rationale that only holds if the sync path wins. Worker placement stands either way: the worker already holds the bytes, and hashing 22 MB on the main thread would jank the UI the RAW worker exists to protect. The probe now fails loudly where OPFS is absent rather than passing on a vacuous assertion, and the OPFS store gains a test for its use of the API -- in particular that the writable is closed on the failure path, which is what otherwise leaves a zero-length file that reads as a hit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(storage): probe OPFS and IndexedDB across every host #243 assumes OPFS works in the three Tauri webviews and in Safari. Nothing in the codebase uses OPFS today, so that was an assumption rather than a finding. This records what each engine actually does with a converted-frame-sized blob. * style: sort object keys in the benchmark report Repo-wide `npm run check` was failing on this file from 7949a51, which made every later task's lint gate ambiguous. * test(storage): probe chunked OPFS writes, and correct a confounded WebKit result A single 67 MB write() failing on WebKit could mean the call shape, not the backend, so the probe now also writes the same blob as 8 MB slices through createWritable and createSyncAccessHandle, on both write paths. Chromium round-trips cleanly at every combination. Chasing the WebKit chunked result led to a 4-byte control write failing identically on the same host, which means both the new chunked number and the single-shot one from the previous commit are confounded by host memory pressure rather than evidence about WebKit's OPFS implementation -- corrected in the design doc rather than left to read as a finding. * fix(storage): correct the WebKit finding and harden the probe per review Task 1 review returned five findings. The design doc claimed the brief's backend rule doesn't name WebKit -- it does, first in the list -- so the correct framing is that the row's evidence is invalid (host memory pressure, proven by a failing control write), not that the rule is exempt; corrected in place along with an opfsRoundTrips=false cell that read as corruption rather than "never compared," and two dangling references to sections that only exist in the brief. The WebdriverIO port passed an async callback to browser.execute, which classic WebDriver's execute-sync endpoint does not await -- silently producing no measurement on any driver that isn't BiDi-negotiated. Restructured to the suite's own convention: a synchronous callback launches the probe as a page-side IIFE that reports through a window global, polled with browser.waitUntil. The control write that caught the WebKit confound last round is now a permanent first step in both probes, asserted before the OPFS-specific checks so a thrashing host reports itself as inconclusive rather than as an OPFS verdict. WebKit was not rerun on this machine, which is still under memory pressure; its row is marked accordingly rather than re-measured. * refactor: extract sha256Hex so worker code can hash presets.ts imports a React config provider, so a worker importing it for one hash function would pull React into the worker bundle. * feat(storage): add updateDocument for atomic read-modify-write The RAW cache index is written by the worker on every conversion and cleared from the settings page. Two transactions leave a window where one loses the other's update, and a lost index entry is a blob nothing will evict. * feat(raw): add the persistent cache tier, storage injected Content-addressed with an LRU bound. Storage is a seam rather than OPFS directly: the eviction and index logic is the part worth testing, and navigator.storage does not exist under Jest. * feat(raw): reconcile the cache index against the blob store Two stores can disagree. A phantom index entry self-heals into a miss on read; an orphaned blob is invisible to eviction and would consume disk forever, so it is swept once per session. Also strengthens "never evicts the entry just added": the original two-entry version passed even with the just-added guard removed, since evicting the older entry alone already satisfied budget before the loop could reach the new one. The new version re-grows an already-present key so its lastUsed ties with a later entry's, making it the first eviction candidate by stable-sort order unless the guard excludes it -- verified failing with the guard removed and passing with it restored. * feat(raw): derive the cache key from content and tool identity The content hash is required for correctness in a browser, where session paths restart from a counter and name different bytes across visits. The tool tag stops a rebuilt dcraw_emu from silently serving the previous demosaic. * fix(raw): guard the tool tag against a dropped flag set and a missing commit Adds a test that mocks dcrawArgs so a future edit that drops it from the tag's hash (recreating the truncated-template-literal defect this module already fixed once) fails a test instead of passing silently. Also stops toolTag from substituting an "unknown" placeholder when versions.json omits the recorded commit -- two builds that both fail to report one would otherwise collide on the same tag and share a cache entry, each serving the other's pixels. Throwing instead is safe: the caller degrades to converting without a persistent-cache hit or write for the session, per the try/catch task 8 wraps key derivation in. * docs: the probe chose IndexedDB, so Task 7 becomes IndexedDB navigator.storage.getDirectory is absent in WebKit and in WebKitGTK 605.1.15, the webview Tauri uses on Linux -- not slow, not quota-limited, absent. An OPFS-backed cache would have silently never worked for Safari users or Linux desktop users. IndexedDB round-tripped 67 MB on every engine tested. The decision rule was written before the evidence and is applied as written: OPFS failing on any engine sends this to approach B. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(raw): back the persistent cache with IndexedDB #243 specified OPFS. The probe found navigator.storage.getDirectory absent in WebKit and in WebKitGTK 605.1.15, the webview Tauri uses on Linux -- not slow, not quota-limited, absent. An OPFS cache would have silently never worked for Safari or Linux desktop users. IndexedDB round-tripped 67 MB everywhere. * docs: point Tasks 8-10 at the IndexedDB blob store * feat(raw): answer conversions from the persistent cache The cache is consulted before converting and populated after, always before the caller transfers the buffer: postMessage detaches it, and a write after that persists a zero-byte file. Every cache failure falls through to conversion, so the cache can never be why a frame fails. * fix(raw): close the onblocked deadlock and a rejected-connection cache in the IndexedDB store Round-1 review on the blob store found two gaps. The view-vs-slice test compared read-back values, which cannot distinguish a stored slice from a stored view -- IndexedDB's structured clone preserves a view's offset and length either way, so the buggy and correct implementations read back identically. Rewritten to check the stored record's byteLength instead. Separately, DATABASE_VERSION had never changed before this task, so onblocked (fired when another tab holds an older version open) and a cached rejected connection promise were both unreachable in practice. Bumping the version made them reachable: a web user with two tabs open across a deploy could wedge storage -- presets and settings, not just this cache -- until reload. Added onversionchange handling so an old tab yields its connection, and stopped the connection cache from pinning a failure. * feat(settings): show and clear the RAW conversion cache A 2 GB cache that a user cannot see or reclaim short of clearing site data is not an honest default. Hidden entirely where there is no IndexedDB, since zero would claim an empty cache rather than no cache. * fix(raw): clear the cached connection before onversionchange closes it, and make the view-vs-slice test discriminate for real Round-2 review found F1 still open and a Critical regression (F4) from round 1's F2 fix. onversionchange closed the database but left kv.ts's module-level connection promise resolved to the closed handle, so every later getDocument/putDocument in that tab would fail with InvalidStateError permanently. app-storage.ts's readJson swallows read errors, so this would render as an empty app -- no presets, no settings, no run history -- rather than a visible error. Fixed by clearing connection before close(), with a test proving a call after the version change recovers by reopening. F1's checked stored?.byteLength === 3, which a stored *view* also satisfies (a view's byteLength reflects its own length, not its backing buffer's). Rewritten to check the stored record is a plain ArrayBuffer, using toString/isView rather than instanceof -- fake-indexeddb clones values into a different realm, so instanceof ArrayBuffer is unreliable here even for a correct record. * fix(raw): make clear() honest about what it actually reclaimed clear() removed each blob, swallowed every per-key failure, and then emptied the index unconditionally. A removal that failed left its blob on disk while usage() -- which only reads the index -- reported 0 B and the settings page toasted success. The Clear button existed to guarantee disk was reclaimed; this let it lie about the one thing it was for. Now clear() keeps in the index exactly the entries whose blobs could not be removed, so usage() still reflects what's really on disk, and it throws naming how many entries failed so the caller can report honestly instead of celebrating a partial no-op. page.tsx re-reads usage() in a finally so a partial failure doesn't leave a stale number next to the error toast, and no longer disables Clear on cacheBytes === 0 -- that reads indexed usage only, so a cache holding nothing but sweep-eligible orphans showed 0 B with Clear disabled and no way to reclaim them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(perf): measure that a reload reuses converted frames #243's acceptance criterion as a number rather than a claim: same frames, new tab, empty session tier, so any saving is the persistent tier's. * docs(perf): say what the control run does and does not settle The reload result stands on a real control: with the pre-cache worker, the second import is as slow as the first. What that control cannot settle is why the first import runs above the 5968 ms baseline measured earlier the same day, because it is a single sample and happens to be the fastest run recorded. Attributing the gap to environment slowness was stated more confidently than one data point supports, so it is now an open question with the run that would close it named. Also corrects the benchmark's comment: it is a reload, not a new tab. The reason the measurement is valid is that a reload discards the JS realm, so the session cache and the RAW worker both start fresh. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: apply final whole-branch review findings (F1-F3) before merge Three Important findings, addressed together since they touch shared files (kv.ts, init.tsx, the design doc): F1 -- a version downgrade (rolled-back deploy, stale HTTP cache, an older desktop reinstall) made kv.ts's open() fail with VersionError, which app-storage.ts's readJson swallowed into the empty-state fallback -- rendering as "you have no data" with the real data intact and no error anywhere. kv.ts now names this DatabaseVersionError; readJson rethrows it instead of falling back; init.tsx probes for it once at startup (mounted app-wide via the root layout) and shows a persistent toast. F2 -- the 2 GB RAW cache budget was never checked against the origin's real navigator.storage.estimate() quota, so on a smaller-quota host the eviction loop never triggered and writes just started failing once the real limit hit, wedging the cache permanently. raw-cache.ts now clamps the effective budget to min(2 GB, 50% of quota) via an injected estimator (raw-cache-quota.ts, kept out of raw-cache.ts itself so it stays testable under Jest), and a write failure now evicts LRU entries -- unconditionally, not gated on the same budget that just failed to predict the failure -- and retries once before giving up. Settings reports the effective budget, not the nominal one. navigator.storage.persist() is called once, best-effort, from both the worker and the page. F3 -- the cache key's tool tag hashed only the dcraw_emu commit, so a rebuild from the same LibRaw commit on a bumped Emscripten toolchain (what #244 automates) would produce an identical key over potentially different bytes, serving a stale TIFF as a valid hit. The tag now folds in versions.json's top-level emscripten field, symmetric with the existing missing-commit guard. Design doc updated to match: the cache key formula, the no-longer-fixed budget, the retry-on-write-failure, and the Settings effective-budget figure. Tests for all three, each verified to fail against the pre-fix code and pass after restoring -- including a targeted mutation on raw-cache.ts's eviction-on-failure path (temporarily making it budget-gated) to confirm the specific test that pins the unconditional-eviction behavior, since a budget-gated version also happens to pass the more obvious retry test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(storage): stop the probe failing on the backend it rejected storage-probe.spec.ts and its WebdriverIO port existed to pick between OPFS and IndexedDB for the persistent RAW cache. They found navigator.storage.getDirectory absent in WebKit and in WebKitGTK 605.1.15 (both opfsAvailable: false in CI), so the cache was built on IndexedDB (raw-cache-idb.ts). The probes kept asserting opfsAvailable === true anyway, which meant a decision the probe itself made permanently red-lines CI on the two engines it made the decision about -- OPFS is not a real dependency of the app that shipped. Both specs now assert idbRoundTrips/idbError, the app's actual dependency, unconditionally. opfsAvailable is recorded and printed but no longer asserted. Where OPFS is present, the control-write check still gates the rest of the OPFS numbers as before (a 4-byte failure under host memory pressure reads as inconclusive, not a negative finding), and where the control write succeeds and OPFS reports no error, the round-trip is still asserted -- an engine that claims OPFS and then corrupts data is still a build-breaking finding. Only OPFS's absence stopped being fatal. Design doc's "Probe results" section records why, alongside the existing WebKit-memory-pressure correction it doesn't contradict: CI's WebKit/WebKitGTK report the API absent; the macOS Playwright row already in the table reports it present but confounded by a loaded host, which is a different thing and still surfaces as inconclusive. Verified locally: chromium passes (idbRoundTrips: true, opfsRoundTrips: true). webkit fails on the control-write assertion here twice in a row with opfsAvailable: true, controlOk: false, under ~81 MB free of 16 GB (memory_pressure) -- the same confound already on record for this Playwright/macOS host, not a regression. CI's WebKit reports opfsAvailable: false, so that branch never runs there and IndexedDB carries the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(storage): match the design doc's WebKitGTK row to the CI run it already had The "Probe results" section's WebKitGTK table row still read "pending CI" and the "Still needed" paragraph still said the WDIO port "has not run anywhere" -- both stale against the same CI run named in the prior commit's own probe-repoint rationale (WebKitGTK 605.1.15, opfsAvailable: false, per eb0aec8). Left uncorrected this would have reproduced the exact table-vs-prose contradiction that commit just fixed for WebKit, one paragraph down. Table row and "Still needed" now say what actually ran: WebKitGTK on the Ubuntu runner, WKWebView and WebView2 still outstanding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent b533aca commit 0871bf8

35 files changed

Lines changed: 5558 additions & 26 deletions

__tests__/app-storage.test.ts

Lines changed: 43 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,23 +10,42 @@ import { describe, expect, it } from "@jest/globals";
1010

1111
const documents = new Map<string, unknown>();
1212

13-
jest.mock("../src/lib/storage/kv", () => ({
14-
getDocument: (key: string) => {
15-
const value = documents.get(key);
16-
if (value === "THROW") {
17-
return Promise.reject(new Error("store unavailable"));
13+
jest.mock("../src/lib/storage/kv", () => {
14+
// A real subclass, not a bare object shape: F1's fix to `readJson` tells
15+
// this class apart with `instanceof`, and a mock that only matched by
16+
// duck-typing would leave that check untested against the one thing it
17+
// actually has to distinguish. Assigned rather than declared with `class
18+
// DatabaseVersionError`, which would shadow the real export this file also
19+
// imports by that name for its own assertions.
20+
const MockDatabaseVersionError = class extends Error {
21+
constructor() {
22+
super("stored data is newer than this build can open");
23+
this.name = "DatabaseVersionError";
1824
}
19-
return Promise.resolve(value);
20-
},
21-
putDocument: (key: string, value: unknown) => {
22-
documents.set(key, value);
23-
return Promise.resolve();
24-
},
25-
}));
25+
};
26+
return {
27+
DatabaseVersionError: MockDatabaseVersionError,
28+
getDocument: (key: string) => {
29+
const value = documents.get(key);
30+
if (value === "THROW") {
31+
return Promise.reject(new Error("store unavailable"));
32+
}
33+
if (value === "VERSION_ERROR") {
34+
return Promise.reject(new MockDatabaseVersionError());
35+
}
36+
return Promise.resolve(value);
37+
},
38+
putDocument: (key: string, value: unknown) => {
39+
documents.set(key, value);
40+
return Promise.resolve();
41+
},
42+
};
43+
});
2644

2745
declare const jest: typeof import("@jest/globals").jest;
2846

2947
import { readJson, STORAGE_VERSION, writeJson } from "../src/lib/app-storage";
48+
import { DatabaseVersionError } from "../src/lib/storage/kv";
3049

3150
describe("app storage", () => {
3251
it("round-trips a value and stamps the version", async () => {
@@ -62,4 +81,16 @@ describe("app storage", () => {
6281
runs: [],
6382
});
6483
});
84+
85+
it("rethrows DatabaseVersionError instead of returning the fallback", async () => {
86+
// F1: a stored-data-is-newer-than-this-build condition is not "nothing
87+
// stored badly" -- the data is intact, and papering over it with the
88+
// fallback is what used to make a rolled-back deploy render as an empty
89+
// app with nothing to explain why.
90+
documents.set("history/newer.json", "VERSION_ERROR");
91+
92+
await expect(
93+
readJson("history/newer.json", { runs: [] })
94+
).rejects.toBeInstanceOf(DatabaseVersionError);
95+
});
6596
});

0 commit comments

Comments
 (0)