Skip to content

Commit ebb5ebc

Browse files
adulbrichclaude
andcommitted
test(raw): pin the drop-and-instantly-re-add case, and fix two comment drifts
Review found the drop-cancellation implementation correct but the test surface incomplete: no test exercised re-adding a frame in the same turn it was dropped, which is the only way to observe `forget`'s identity check and `dropRawConversions`'s eager `forget` actually doing their jobs -- the existing "forgets a frame dropped before it started" test awaits the rejection first, giving the ordinary catch -> forget path time to clean up before the re-add ever runs. Adds one test that drops and re-adds with no await between, confirmed by mutation to fail both ways: deleting `dropRawConversions`'s `forget` call lets the re-added promise inherit the dropped entry's AbortError, and reverting `forget`'s identity check lets the dropped entry's late, post-rejection forget delete the re-added entry's cache slot, surfacing as a third conversion of bytes already held. Also corrects two doc comments that had drifted from the code they describe: `tiffFor`'s claim that a converter skipping `onStart` leaves frames droppable "for their whole life" (false since the `started || done` widening -- droppable only until they finish), and `dropRawConversions`'s docstring naming the test fixture `countingIo` where it should state the underlying contract instead. Refs #248 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 645e432 commit ebb5ebc

2 files changed

Lines changed: 45 additions & 5 deletions

File tree

src/lib/raw-preview.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,43 @@ describe("shared RAW conversion", () => {
233233
expect(source.converted).toHaveLength(1);
234234
});
235235

236+
it("does not let a frame dropped and instantly re-added inherit the dropped entry's rejection or lose its own cache slot", async () => {
237+
const io = deferredIo();
238+
const path = "/in/capt01.CR2";
239+
240+
const dropped = rawToTiff(path, io);
241+
// Two turns so `tiffFor` has run and the abort listener is attached
242+
// before the drop, exactly as in the "forgets a frame dropped before it
243+
// started" case above.
244+
await Promise.resolve();
245+
await Promise.resolve();
246+
dropRawConversions([path]);
247+
// No await between the drop and the re-add: that gap is what the earlier
248+
// "forgets a frame dropped..." test could not exercise, because it
249+
// awaited `dropped`'s rejection first, giving `rawToTiff`'s own
250+
// `catch -> forget` time to clear the entry before the re-add ever ran.
251+
const again = rawToTiff(path, io);
252+
await expect(dropped).rejects.toThrow(ABORT_MESSAGE);
253+
254+
await Promise.resolve();
255+
await Promise.resolve();
256+
io.start(path);
257+
io.finish(path);
258+
await expect(again).resolves.toHaveLength(1024);
259+
expect(io.converted).toHaveLength(2);
260+
261+
// The replacement entry `again` created must still be the live cache
262+
// entry for this key. Deliberately not awaited: if it were lost -- the
263+
// dropped entry's late `forget` deleting it regardless of identity --
264+
// this call starts a third, real conversion that nothing here ever
265+
// finishes, and awaiting it would hang for 5 s instead of failing
266+
// cleanly on `io.converted`'s length.
267+
rawToTiff(path, io);
268+
await Promise.resolve();
269+
await Promise.resolve();
270+
expect(io.converted).toHaveLength(2);
271+
});
272+
236273
it("does not count a conversion that finished after its entry was gone", async () => {
237274
const io = deferredIo();
238275

src/lib/raw-preview.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,9 @@ export interface RawSourceIo {
100100
* `options.onStart` is the converter's half of that bargain: it says the
101101
* frame can no longer be skipped, which is the only way this module can
102102
* tell a queued frame from one already converting. A converter that never
103-
* calls it leaves its frames droppable for their whole life.
103+
* calls it leaves its frames droppable until they finish -- see
104+
* `dropRawConversions`, which keeps a settled entry by its `done` flag
105+
* regardless of `started`.
104106
*/
105107
tiffFor?: (
106108
path: string,
@@ -276,10 +278,11 @@ async function convert(
276278
* because forgetting it would make a re-added set a cache miss and convert
277279
* the same bytes a second time. A finished frame is kept too -- it costs
278280
* nothing the LRU budget does not already govern, and it makes re-adding the
279-
* same file instant. `flags.done` is what recognizes it: a converter that
280-
* resolves without ever calling `onStart` -- `countingIo` in the tests --
281-
* would otherwise leave `flags.started` false on a completed entry,
282-
* indistinguishable from one still queued.
281+
* same file instant. `flags.done` is what recognizes it: a converter is free
282+
* to resolve without ever calling `onStart` -- #243's OPFS path will do
283+
* exactly that when it answers from a cached TIFF instead of converting --
284+
* and such a converter would otherwise leave `flags.started` false on a
285+
* completed entry, indistinguishable from one still queued.
283286
*
284287
* Paths that were never converted, including every non-RAW one, match no key
285288
* and cost a scan.

0 commit comments

Comments
 (0)