From 0c5d43b0a52eedd6ec7e00973429c62bbf49fab0 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 12:00:59 +0000 Subject: [PATCH 1/9] test(dicom): fold the new spec clones and cut the chunk handler complexity The duplication ratchet found four clones inside the DICOM chunk image spec, one shared by the two DICOM store specs, and one shared by the two new DICOM end-to-end specs. The complexity ratchet found onRegularChunkHasData at 18. The chunk image spec builds its held-open decodes with one deferredDecoder helper, shares the re-sorted chunk setup the three staleness tests need, and asserts slice contents through expectSliceValues. The two store specs build their instance tags from a shared fixture. The end-to-end specs make their scratch directory with a makeTempDir helper in the existing test utils. No assertion changed. onRegularChunkHasData delegates its three rejections to assertSingleFramePerFile, assertChunkFitsSlot and assertSamplesRepresentable, which also removes the repeated "File X (chunk N)" prefix. Behaviour and error messages are unchanged. --- eslint.config.js | 2 +- .../__tests__/dicomChunkImage.spec.ts | 244 ++++++------------ src/core/streaming/dicomChunkImage.ts | 142 ++++++---- .../__tests__/datasets-dicom-cine.spec.ts | 34 +-- .../__tests__/datasets-dicom-reimport.spec.ts | 30 +-- src/store/__tests__/dicomTagFixtures.ts | 43 +++ tests/specs/dicom-dimension-mismatch.e2e.ts | 10 +- tests/specs/dicom-modality-rescale.e2e.ts | 10 +- tests/specs/utils.ts | 13 + 9 files changed, 252 insertions(+), 276 deletions(-) create mode 100644 src/store/__tests__/dicomTagFixtures.ts diff --git a/eslint.config.js b/eslint.config.js index 78aaaeafc..36dda8865 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -60,7 +60,7 @@ const featureBoundaries = (features) => { { paths: ['pinia', 'vue'].map((name) => ({ name, - message: `The ${feature.dir} pure layer must stay framework-free — no ${name}.`, + message: `The ${feature.dir} pure layer must stay framework-free: no ${name}.`, })), patterns: [ { diff --git a/src/core/streaming/__tests__/dicomChunkImage.spec.ts b/src/core/streaming/__tests__/dicomChunkImage.spec.ts index 8d7c77764..ee6f66161 100644 --- a/src/core/streaming/__tests__/dicomChunkImage.spec.ts +++ b/src/core/streaming/__tests__/dicomChunkImage.spec.ts @@ -102,6 +102,72 @@ function sliceOf(image: DicomChunkImage, index: number) { ); } +// A decode held open per pixel value, so a test settles each attempt in the +// order it chooses. A chunk redecoded after a re-sort has one settler per +// attempt, oldest first; a settler given an error rejects instead of resolving. +function deferredDecoder() { + const pending = new Map void>>(); + const read: DicomChunkImageInit['readDicomImage'] = async (file) => { + const value = Number(await file.text()); + return new Promise((resolve, reject) => { + const settlers = pending.get(value) ?? []; + settlers.push((err) => { + if (err) reject(err); + else + resolve({ + image: { + size: [COLUMNS, ROWS, 1], + data: new Uint16Array(PIXELS_PER_SLICE).fill(value), + imageType: { components: 1 }, + }, + }); + }); + pending.set(value, settlers); + }); + }; + return { pending, read }; +} + +// Chunk 3 starts alone in slot 0, then chunks 1 and 2 arrive and re-sort it +// into slot 2 while its first decode is still outstanding. `pending.get(3)` +// then holds the stale attempt at index 0 and the current one at index 1. +async function imageWithResortedChunk() { + const { pending, read } = deferredDecoder(); + const image = new DicomChunkImage({ + splitAndSort: splitAndSortByPosition, + readDicomImage: read, + }); + + const errors: number[] = []; + image.addEventListener('chunkError', ({ chunk }) => { + errors.push(zOf(chunk)); + }); + + const [first, second, third] = await Promise.all([ + makeLoadedChunk(1), + makeLoadedChunk(2), + makeLoadedChunk(3), + ]); + + // Start chunk 3 in slot 0, then move it to slot 2 while decoding. + await image.addChunks([third]); + await vi.waitFor(() => expect(pending.get(3)).toHaveLength(1)); + + await image.addChunks([first, second]); + await vi.waitFor(() => expect(pending.get(3)).toHaveLength(2)); + + return { image, pending, errors }; +} + +// Asserts the volume's leading slices hold the pixels of the chunks that +// belong in them; a chunk's z position is also its pixel value, and 0 is a +// slice no chunk has written. +function expectSliceValues(image: DicomChunkImage, values: number[]) { + values.forEach((value, index) => + expect(sliceOf(image, index)).toEqual(Array(PIXELS_PER_SLICE).fill(value)) + ); +} + async function loadRejectingSeries( read: DicomChunkImageInit['readDicomImage'] ) { @@ -128,8 +194,7 @@ async function loadRejectingSeries( expect(image.status.value).toBe('complete'); expect(errors).toHaveLength(1); - expect(sliceOf(image, 0)).toEqual(Array(PIXELS_PER_SLICE).fill(1)); - expect(sliceOf(image, 1)).toEqual(Array(PIXELS_PER_SLICE).fill(0)); + expectSliceValues(image, [1, 0]); image.dispose(); return String(errors[0]); } @@ -275,8 +340,7 @@ describe('DicomChunkImage', () => { ); expect(image.getChunks().map(zOf)).toEqual([1, 2]); - expect(sliceOf(image, 0)).toEqual(Array(PIXELS_PER_SLICE).fill(1)); - expect(sliceOf(image, 1)).toEqual(Array(PIXELS_PER_SLICE).fill(2)); + expectSliceValues(image, [1, 2]); image.dispose(); }); @@ -313,60 +377,22 @@ describe('DicomChunkImage', () => { { z: 2, zRange: [1, 1] }, { z: 3, zRange: [2, 2] }, ]); - expect(sliceOf(image, 0)).toEqual(Array(PIXELS_PER_SLICE).fill(1)); - expect(sliceOf(image, 1)).toEqual(Array(PIXELS_PER_SLICE).fill(2)); - expect(sliceOf(image, 2)).toEqual(Array(PIXELS_PER_SLICE).fill(3)); + expectSliceValues(image, [1, 2, 3]); image.dispose(); }); it('keeps a stale in-flight decode from clobbering the re-sorted volume', async () => { - // Hold each decode independently by its pixel value. - const pending = new Map void>>(); - const deferredRead: DicomChunkImageInit['readDicomImage'] = async ( - file - ) => { - const value = Number(await file.text()); - return new Promise((resolve) => { - const resolvers = pending.get(value) ?? []; - resolvers.push(() => - resolve({ - image: { - size: [COLUMNS, ROWS, 1], - data: new Uint16Array(PIXELS_PER_SLICE).fill(value), - imageType: { components: 1 }, - }, - }) - ); - pending.set(value, resolvers); - }); - }; - - const image = new DicomChunkImage({ - splitAndSort: splitAndSortByPosition, - readDicomImage: deferredRead, - }); + const { image, pending } = await imageWithResortedChunk(); let loads = 0; image.addEventListener('chunkLoad', () => { loads += 1; }); - const [first, second, third] = await Promise.all([ - makeLoadedChunk(1), - makeLoadedChunk(2), - makeLoadedChunk(3), - ]); - - // Start chunk 3 in slot 0, then move it to slot 2 while decoding. - await image.addChunks([third]); - await vi.waitFor(() => expect(pending.get(3)).toHaveLength(1)); - - await image.addChunks([first, second]); await vi.waitFor(() => { expect(pending.get(1)).toHaveLength(1); expect(pending.get(2)).toHaveLength(1); - expect(pending.get(3)).toHaveLength(2); }); // Complete the current decodes before the stale attempt. @@ -380,59 +406,13 @@ describe('DicomChunkImage', () => { await Promise.resolve(); expect(loads).toBe(3); - expect(sliceOf(image, 0)).toEqual(Array(PIXELS_PER_SLICE).fill(1)); - expect(sliceOf(image, 1)).toEqual(Array(PIXELS_PER_SLICE).fill(2)); - expect(sliceOf(image, 2)).toEqual(Array(PIXELS_PER_SLICE).fill(3)); + expectSliceValues(image, [1, 2, 3]); image.dispose(); }); it('does not let a stale success overwrite a replacement failure', async () => { - // Hold each decode independently by its pixel value. - const pending = new Map void>>(); - const deferredRead: DicomChunkImageInit['readDicomImage'] = async ( - file - ) => { - const value = Number(await file.text()); - return new Promise((resolve, reject) => { - const settlers = pending.get(value) ?? []; - settlers.push((err) => { - if (err) reject(err); - else - resolve({ - image: { - size: [COLUMNS, ROWS, 1], - data: new Uint16Array(PIXELS_PER_SLICE).fill(value), - imageType: { components: 1 }, - }, - }); - }); - pending.set(value, settlers); - }); - }; - - const image = new DicomChunkImage({ - splitAndSort: splitAndSortByPosition, - readDicomImage: deferredRead, - }); - - const errors: number[] = []; - image.addEventListener('chunkError', ({ chunk }) => { - errors.push(zOf(chunk)); - }); - - const [first, second, third] = await Promise.all([ - makeLoadedChunk(1), - makeLoadedChunk(2), - makeLoadedChunk(3), - ]); - - // Start chunk 3 in slot 0, then move it to slot 2 while decoding. - await image.addChunks([third]); - await vi.waitFor(() => expect(pending.get(3)).toHaveLength(1)); - - await image.addChunks([first, second]); - await vi.waitFor(() => expect(pending.get(3)).toHaveLength(2)); + const { image, pending, errors } = await imageWithResortedChunk(); pending.get(1)![0](); pending.get(2)![0](); @@ -462,49 +442,7 @@ describe('DicomChunkImage', () => { }); it('does not let a stale failure overwrite a replacement success', async () => { - const pending = new Map void>>(); - const deferredRead: DicomChunkImageInit['readDicomImage'] = async ( - file - ) => { - const value = Number(await file.text()); - return new Promise((resolve, reject) => { - const settlers = pending.get(value) ?? []; - settlers.push((err) => { - if (err) reject(err); - else - resolve({ - image: { - size: [COLUMNS, ROWS, 1], - data: new Uint16Array(PIXELS_PER_SLICE).fill(value), - imageType: { components: 1 }, - }, - }); - }); - pending.set(value, settlers); - }); - }; - - const image = new DicomChunkImage({ - splitAndSort: splitAndSortByPosition, - readDicomImage: deferredRead, - }); - - const errors: number[] = []; - image.addEventListener('chunkError', ({ chunk }) => { - errors.push(zOf(chunk)); - }); - - const [first, second, third] = await Promise.all([ - makeLoadedChunk(1), - makeLoadedChunk(2), - makeLoadedChunk(3), - ]); - - await image.addChunks([third]); - await vi.waitFor(() => expect(pending.get(3)).toHaveLength(1)); - - await image.addChunks([first, second]); - await vi.waitFor(() => expect(pending.get(3)).toHaveLength(2)); + const { image, pending, errors } = await imageWithResortedChunk(); pending.get(1)![0](); pending.get(2)![0](); @@ -529,27 +467,10 @@ describe('DicomChunkImage', () => { }); it('reports a reallocated chunk as loading until its slice is rewritten', async () => { - const pending: Array<() => void> = []; - const deferredRead: DicomChunkImageInit['readDicomImage'] = async ( - file - ) => { - const value = Number(await file.text()); - return new Promise((resolve) => { - pending.push(() => - resolve({ - image: { - size: [COLUMNS, ROWS, 1], - data: new Uint16Array(PIXELS_PER_SLICE).fill(value), - imageType: { components: 1 }, - }, - }) - ); - }); - }; - + const { pending, read } = deferredDecoder(); const image = new DicomChunkImage({ splitAndSort: splitAndSortByPosition, - readDicomImage: deferredRead, + readDicomImage: read, }); const [first, second] = await Promise.all([ @@ -558,8 +479,8 @@ describe('DicomChunkImage', () => { ]); await image.addChunks([first]); - await vi.waitFor(() => expect(pending).toHaveLength(1)); - pending[0](); + await vi.waitFor(() => expect(pending.get(1)).toHaveLength(1)); + pending.get(1)![0](); await vi.waitFor(() => expect(image.status.value).toBe('complete')); // Reallocation cleared chunk 1, and neither replacement decode has run. @@ -572,11 +493,14 @@ describe('DicomChunkImage', () => { expect(image.status.value).toBe('incomplete'); expect(sliceOf(image, 0)).toEqual(Array(PIXELS_PER_SLICE).fill(0)); - await vi.waitFor(() => expect(pending).toHaveLength(3)); - pending.slice(1).forEach((settle) => settle()); + await vi.waitFor(() => { + expect(pending.get(1)).toHaveLength(2); + expect(pending.get(2)).toHaveLength(1); + }); + pending.get(1)![1](); + pending.get(2)![0](); await vi.waitFor(() => expect(image.status.value).toBe('complete')); - expect(sliceOf(image, 0)).toEqual(Array(PIXELS_PER_SLICE).fill(1)); - expect(sliceOf(image, 1)).toEqual(Array(PIXELS_PER_SLICE).fill(2)); + expectSliceValues(image, [1, 2]); image.dispose(); }); diff --git a/src/core/streaming/dicomChunkImage.ts b/src/core/streaming/dicomChunkImage.ts index 3d2c151ba..d327d3be9 100644 --- a/src/core/streaming/dicomChunkImage.ts +++ b/src/core/streaming/dicomChunkImage.ts @@ -99,6 +99,39 @@ export interface DicomChunkImageInit { warn: (title: string, details: string) => void; } +type DecodedChunkImage = Awaited< + ReturnType +>['image']; + +// The buffer is allocated for the range every chunk's tags declare, so a chunk +// only fails here when its decoded values disagree with its tags. +// TypedArray.set raises nothing for such values: integers wrap and fractions +// truncate. +function assertSamplesRepresentable( + decoded: ArrayLike, + range: { min: number; max: number }, + buffer: TypedArray, + where: string +) { + if (!valuesFitBuffer(range, buffer)) { + const bufferRange = getBufferValueRange(buffer)!; + throw new Error( + `${where} has pixel values the volume it belongs to cannot represent. ` + + `Its pixel values run from ${range.min} to ${range.max}, but the volume's buffer is ` + + `${buffer.constructor.name}, holding values from ${bufferRange.min} to ${bufferRange.max}. ` + + `Every file in a volume must decode to values its buffer can hold without conversion.` + ); + } + if (!samplesAreIntegral(decoded, buffer)) { + throw new Error( + `${where} has fractional pixel values the volume it belongs to cannot represent. ` + + `Its pixel values run from ${range.min} to ${range.max}, but the volume's buffer is ` + + `${buffer.constructor.name}, which holds only whole numbers. ` + + `Every file in a volume must decode to values its buffer can hold without conversion.` + ); + } +} + export default class DicomChunkImage extends BaseProgressiveImage implements ChunkImage @@ -444,6 +477,52 @@ export default class DicomChunkImage this.onChunksUpdated(); } + // The slot layout gives one frame per file, so several files that each carry + // several frames have nowhere to go. + private assertSingleFramePerFile(frames: number, where: string) { + if (frames > 1 && this.chunks.length > 1) { + // we're trying to load multiple chunks where individual chunks have multiple frames + throw new Error( + `Loading a single volume from multiple DICOM files where individual files contain multiple frames is not supported. ` + + `${where} contains ${frames} frames.` + ); + } + } + + // Each chunk gets a fixed slot: one frame per chunk in a multi-file volume, + // or the whole volume when a single multi-frame chunk fills it. A decoded + // chunk has to fill its slot exactly. + private assertChunkFitsSlot( + image: DecodedChunkImage, + volume: { dims: number[]; componentCount: number }, + where: string + ) { + const { dims, componentCount } = volume; + const multiFile = this.chunks.length > 1; + const framesPerChunk = multiFile ? 1 : dims[2]; + const [chunkWidth, chunkHeight] = image.size; + const chunkFrames = image.size[2] ?? 1; + const chunkComponents = image.imageType.components; + if ( + chunkWidth !== dims[0] || + chunkHeight !== dims[1] || + chunkFrames !== framesPerChunk || + chunkComponents !== componentCount + ) { + // A lone chunk defines the volume it fails to fit, so advice about + // agreeing with the other files only makes sense for a multi-file volume. + const advice = multiFile + ? ' Every file in a volume must have the same Rows, Columns, and SamplesPerPixel.' + : ''; + throw new Error( + `${where} does not fit the volume it belongs to. ` + + `It decoded to ${chunkWidth}x${chunkHeight}x${chunkFrames} with ${chunkComponents} component(s), ` + + `but the volume has room for ${dims[0]}x${dims[1]}x${framesPerChunk} with ${componentCount} component(s).` + + advice + ); + } + } + private async onRegularChunkHasData(chunk: Chunk, generation: number) { const chunkIndex = this.chunks.indexOf(chunk); if (!chunk.dataBlob) @@ -464,13 +543,8 @@ export default class DicomChunkImage const sliceIndex = this.chunks.indexOf(chunk); if (sliceIndex === -1) return; - if (result.image.size[2] > 1 && this.chunks.length > 1) { - // we're trying to load multiple chunks where individual chunks have multiple frames - throw new Error( - `Loading a single volume from multiple DICOM files where individual files contain multiple frames is not supported. ` + - `File ${chunkId} (chunk ${sliceIndex}) contains ${result.image.size[2]} frames.` - ); - } + const where = `File ${chunkId} (chunk ${sliceIndex})`; + this.assertSingleFramePerFile(result.image.size[2], where); const scalars = this.vtkImageData.value.getPointData().getScalars(); const pixelData = scalars.getData() as TypedArray; @@ -478,31 +552,7 @@ export default class DicomChunkImage const dims = this.vtkImageData.value.getDimensions(); - // Each chunk gets a fixed slot: one frame per chunk in a multi-file - // volume, or the whole volume when a single multi-frame chunk fills it. - const framesPerChunk = this.chunks.length > 1 ? 1 : dims[2]; - const [chunkWidth, chunkHeight] = result.image.size; - const chunkFrames = result.image.size[2] ?? 1; - const chunkComponents = result.image.imageType.components; - if ( - chunkWidth !== dims[0] || - chunkHeight !== dims[1] || - chunkFrames !== framesPerChunk || - chunkComponents !== componentCount - ) { - // A lone chunk defines the volume it fails to fit, so advice about - // agreeing with the other files only makes sense for a multi-file volume. - const advice = - this.chunks.length > 1 - ? ' Every file in a volume must have the same Rows, Columns, and SamplesPerPixel.' - : ''; - throw new Error( - `File ${chunkId} (chunk ${sliceIndex}) does not fit the volume it belongs to. ` + - `It decoded to ${chunkWidth}x${chunkHeight}x${chunkFrames} with ${chunkComponents} component(s), ` + - `but the volume has room for ${dims[0]}x${dims[1]}x${framesPerChunk} with ${componentCount} component(s).` + - advice - ); - } + this.assertChunkFitsSlot(result.image, { dims, componentCount }, where); const chunkDataRange: Array<[number, number]> = []; for (let comp = 0; comp < componentCount; comp++) { @@ -514,30 +564,14 @@ export default class DicomChunkImage chunkDataRange.push([min, max]); } - // The buffer is allocated for the range every chunk's tags declare, so a - // chunk only fails here when its decoded values disagree with its tags. - // TypedArray.set raises nothing for such values: integers wrap and - // fractions truncate. const chunkMin = Math.min(...chunkDataRange.map(([min]) => min)); const chunkMax = Math.max(...chunkDataRange.map(([, max]) => max)); - const decoded = result.image.data as unknown as ArrayLike; - if (!valuesFitBuffer({ min: chunkMin, max: chunkMax }, pixelData)) { - const bufferRange = getBufferValueRange(pixelData)!; - throw new Error( - `File ${chunkId} (chunk ${sliceIndex}) has pixel values the volume it belongs to cannot represent. ` + - `Its pixel values run from ${chunkMin} to ${chunkMax}, but the volume's buffer is ` + - `${pixelData.constructor.name}, holding values from ${bufferRange.min} to ${bufferRange.max}. ` + - `Every file in a volume must decode to values its buffer can hold without conversion.` - ); - } - if (!samplesAreIntegral(decoded, pixelData)) { - throw new Error( - `File ${chunkId} (chunk ${sliceIndex}) has fractional pixel values the volume it belongs to cannot represent. ` + - `Its pixel values run from ${chunkMin} to ${chunkMax}, but the volume's buffer is ` + - `${pixelData.constructor.name}, which holds only whole numbers. ` + - `Every file in a volume must decode to values its buffer can hold without conversion.` - ); - } + assertSamplesRepresentable( + result.image.data as unknown as ArrayLike, + { min: chunkMin, max: chunkMax }, + pixelData, + where + ); const offset = dims[0] * dims[1] * componentCount * sliceIndex; pixelData.set(result.image.data as TypedArray, offset); diff --git a/src/store/__tests__/datasets-dicom-cine.spec.ts b/src/store/__tests__/datasets-dicom-cine.spec.ts index 2d9d424b2..2f1c4fc97 100644 --- a/src/store/__tests__/datasets-dicom-cine.spec.ts +++ b/src/store/__tests__/datasets-dicom-cine.spec.ts @@ -12,6 +12,7 @@ import type { CineParseResult, } from '@/src/core/cine/parseCineDicom'; import { useImageCacheStore } from '@/src/store/image-cache'; +import { instanceTags } from '@/src/store/__tests__/dicomTagFixtures'; import { isCineChunkGroup, useDICOMStore } from '@/src/store/datasets-dicom'; const mocks = vi.hoisted(() => { @@ -87,29 +88,16 @@ vi.mock('@/src/core/streaming/dicomChunkImage', () => ({ })); function metadata(overrides: Record = {}) { - return ( - [ - [Tags.SOPClassUID, SOP_CLASS_ULTRASOUND_MULTIFRAME], - [Tags.NumberOfFrames, '2'], - [Tags.SOPInstanceUID, 'sop-uid'], - [Tags.PatientID, 'patient-1'], - [Tags.PatientName, 'Test Patient'], - [Tags.PatientBirthDate, ''], - [Tags.PatientSex, ''], - [Tags.StudyID, 'study-1'], - [Tags.StudyInstanceUID, 'study-uid'], - [Tags.StudyDate, ''], - [Tags.StudyTime, ''], - [Tags.AccessionNumber, ''], - [Tags.StudyDescription, ''], - [Tags.Modality, 'US'], - [Tags.SeriesInstanceUID, 'series-uid'], - [Tags.SeriesNumber, '7'], - [Tags.SeriesDescription, 'Unsupported native cine'], - [Tags.WindowLevel, ''], - [Tags.WindowWidth, ''], - ] as [string, string][] - ).map(([tag, value]) => [tag, overrides[tag] ?? value]) as [string, string][]; + return instanceTags({ + sopClassUid: SOP_CLASS_ULTRASOUND_MULTIFRAME, + numberOfFrames: '2', + sopInstanceUid: 'sop-uid', + modality: 'US', + seriesDescription: 'Unsupported native cine', + }).map(([tag, value]) => [tag, overrides[tag] ?? value]) as [ + string, + string, + ][]; } function cineHeader(overrides: Partial = {}): CineHeader { diff --git a/src/store/__tests__/datasets-dicom-reimport.spec.ts b/src/store/__tests__/datasets-dicom-reimport.spec.ts index df7fa9ed7..b60cffc94 100644 --- a/src/store/__tests__/datasets-dicom-reimport.spec.ts +++ b/src/store/__tests__/datasets-dicom-reimport.spec.ts @@ -2,8 +2,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { createPinia, setActivePinia } from 'pinia'; import type { Chunk } from '@/src/core/streaming/chunk'; -import { Tags } from '@/src/core/dicomTags'; import { useImageCacheStore } from '@/src/store/image-cache'; +import { instanceTags } from '@/src/store/__tests__/dicomTagFixtures'; import { useDICOMStore } from '@/src/store/datasets-dicom'; const mocks = vi.hoisted(() => { @@ -67,27 +67,13 @@ vi.mock('@/src/core/streaming/dicomChunkImage', () => ({ })); function chunk(sopInstanceUid: string) { - const metadata = [ - [Tags.SOPClassUID, '1.2.840.10008.5.1.4.1.1.2'], - [Tags.NumberOfFrames, '1'], - [Tags.SOPInstanceUID, sopInstanceUid], - [Tags.PatientID, 'patient-1'], - [Tags.PatientName, 'Test Patient'], - [Tags.PatientBirthDate, ''], - [Tags.PatientSex, ''], - [Tags.StudyID, 'study-1'], - [Tags.StudyInstanceUID, 'study-uid'], - [Tags.StudyDate, ''], - [Tags.StudyTime, ''], - [Tags.AccessionNumber, ''], - [Tags.StudyDescription, ''], - [Tags.Modality, 'CT'], - [Tags.SeriesInstanceUID, 'series-uid'], - [Tags.SeriesNumber, '7'], - [Tags.SeriesDescription, 'Incremental series'], - [Tags.WindowLevel, ''], - [Tags.WindowWidth, ''], - ] as [string, string][]; + const metadata = instanceTags({ + sopClassUid: '1.2.840.10008.5.1.4.1.1.2', + numberOfFrames: '1', + sopInstanceUid, + modality: 'CT', + seriesDescription: 'Incremental series', + }); return { metadata, metaBlob: new Blob([new Uint8Array([1])]), diff --git a/src/store/__tests__/dicomTagFixtures.ts b/src/store/__tests__/dicomTagFixtures.ts new file mode 100644 index 000000000..fabacdaa7 --- /dev/null +++ b/src/store/__tests__/dicomTagFixtures.ts @@ -0,0 +1,43 @@ +import { Tags } from '@/src/core/dicomTags'; + +/** + * The tags one synthetic instance carries, in the order the DICOM store reads + * them. Everything a case does not vary (patient, study, and the series + * identity the store groups on) is fixed here so specs only state what their + * case is about. + */ +export type InstanceTags = { + sopClassUid: string; + numberOfFrames: string; + sopInstanceUid: string; + modality: string; + seriesDescription: string; +}; + +export const instanceTags = ({ + sopClassUid, + numberOfFrames, + sopInstanceUid, + modality, + seriesDescription, +}: InstanceTags): Array<[string, string]> => [ + [Tags.SOPClassUID, sopClassUid], + [Tags.NumberOfFrames, numberOfFrames], + [Tags.SOPInstanceUID, sopInstanceUid], + [Tags.PatientID, 'patient-1'], + [Tags.PatientName, 'Test Patient'], + [Tags.PatientBirthDate, ''], + [Tags.PatientSex, ''], + [Tags.StudyID, 'study-1'], + [Tags.StudyInstanceUID, 'study-uid'], + [Tags.StudyDate, ''], + [Tags.StudyTime, ''], + [Tags.AccessionNumber, ''], + [Tags.StudyDescription, ''], + [Tags.Modality, modality], + [Tags.SeriesInstanceUID, 'series-uid'], + [Tags.SeriesNumber, '7'], + [Tags.SeriesDescription, seriesDescription], + [Tags.WindowLevel, ''], + [Tags.WindowWidth, ''], +]; diff --git a/tests/specs/dicom-dimension-mismatch.e2e.ts b/tests/specs/dicom-dimension-mismatch.e2e.ts index 657ed0505..e0838dd62 100644 --- a/tests/specs/dicom-dimension-mismatch.e2e.ts +++ b/tests/specs/dicom-dimension-mismatch.e2e.ts @@ -5,11 +5,9 @@ // the series has to reach a usable state anyway. import * as path from 'path'; import * as fs from 'fs'; -import { cleanuptotal } from 'wdio-cleanuptotal-service'; import { volViewPage } from '../pageobjects/volview.page'; -import { TEMP_DIR } from '../../wdio.shared.conf'; import { buildSyntheticDicom, newUid } from './syntheticDicom'; -import { writeManifestToFile } from './utils'; +import { makeTempDir, writeManifestToFile } from './utils'; const IMAGE_ORIENTATION_PATIENT = [1, 0, 0, 0, 1, 0] as const; const SLICE_COUNT = 5; @@ -29,11 +27,7 @@ async function writeSeries( outlierSlice: number, manifestName: string ) { - const dir = path.join(TEMP_DIR, dirName); - fs.mkdirSync(dir, { recursive: true }); - cleanuptotal.addCleanup(async () => { - fs.rmSync(dir, { recursive: true, force: true }); - }); + const dir = makeTempDir(dirName); const studyUid = newUid(); const seriesUid = newUid(); diff --git a/tests/specs/dicom-modality-rescale.e2e.ts b/tests/specs/dicom-modality-rescale.e2e.ts index 9b7225ee2..fbc261b43 100644 --- a/tests/specs/dicom-modality-rescale.e2e.ts +++ b/tests/specs/dicom-modality-rescale.e2e.ts @@ -1,12 +1,10 @@ import * as fs from 'fs'; import * as path from 'path'; -import { cleanuptotal } from 'wdio-cleanuptotal-service'; -import { TEMP_DIR } from '../../wdio.shared.conf'; import { volViewPage } from '../pageobjects/volview.page'; import { buildSyntheticDicom, newUid } from './syntheticDicom'; import { waitForFirstCompleteCachedImageScalars } from './imageCacheUtils'; -import { writeManifestToFile } from './utils'; +import { makeTempDir, writeManifestToFile } from './utils'; const PUBLIC_DSC_SERIES_UID = '1.3.6.1.4.1.9590.100.1.2.284777661700890778225181143863199482857'; @@ -17,11 +15,7 @@ const COLUMNS = 4; async function writeRescaledSeries() { const dirName = `modality-rescale-${Date.now()}`; - const dir = path.join(TEMP_DIR, dirName); - fs.mkdirSync(dir, { recursive: true }); - cleanuptotal.addCleanup(async () => { - fs.rmSync(dir, { recursive: true, force: true }); - }); + const dir = makeTempDir(dirName); const studyUid = newUid(); const resources = STORED_VALUES.map((pixelValue, index) => { diff --git a/tests/specs/utils.ts b/tests/specs/utils.ts index 184b74614..4125b845e 100644 --- a/tests/specs/utils.ts +++ b/tests/specs/utils.ts @@ -33,6 +33,19 @@ export function writeMetaImage( return fileName; } +/** + * A directory under TEMP_DIR for one spec's generated files, removed with + * everything in it once the run finishes. + */ +export function makeTempDir(dirName: string) { + const dir = path.join(TEMP_DIR, dirName); + fs.mkdirSync(dir, { recursive: true }); + cleanuptotal.addCleanup(async () => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + return dir; +} + export async function writeManifestToFile(manifest: unknown, fileName: string) { const filePath = path.join(TEMP_DIR, fileName); await fs.promises.writeFile(filePath, JSON.stringify(manifest)); From 41880d1f8a7fb430399598f1b57849344aeb9834 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 21:03:17 -0400 Subject: [PATCH 2/9] fix(layers): leave no undefined layer list behind a failed build --- src/store/datasets-layers.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/store/datasets-layers.ts b/src/store/datasets-layers.ts index 60138fa3d..44fa68590 100644 --- a/src/store/datasets-layers.ts +++ b/src/store/datasets-layers.ts @@ -70,9 +70,11 @@ export const useLayersStore = defineStore('layer', () => { return await _addLayer(parent, source); } catch (error) { // remove failed layer from parent's layer list - parentToLayers[parent] = parentToLayers[parent]?.filter( - ({ selection }) => selection !== source - ); + const layers = parentToLayers[parent]; + if (layers) + parentToLayers[parent] = layers.filter( + ({ selection }) => selection !== source + ); throw error; } }); From 6eb88d839548817c1953d2f040c947dd327cfbbf Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 21:03:21 -0400 Subject: [PATCH 3/9] fix(state): skip view configs of datasets that did not restore --- src/store/view-configs.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/store/view-configs.ts b/src/store/view-configs.ts index e75598ac8..7dc0e9ac0 100644 --- a/src/store/view-configs.ts +++ b/src/store/view-configs.ts @@ -58,7 +58,8 @@ export const useViewConfigStore = defineStore('viewConfig', () => { const updatedConfig: Record = {}; Object.entries(config).forEach(([dataID, viewConfig]) => { const newDataID = dataIDMap[dataID]; - updatedConfig[newDataID] = viewConfig; + // A dataset that failed to restore has no new id to carry its config. + if (newDataID) updatedConfig[newDataID] = viewConfig; }); viewSliceStore.deserialize(viewID, updatedConfig); From efb1ef3a956ab1db7deab1a808746de7c0920d71 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 21:15:38 -0400 Subject: [PATCH 4/9] test(e2e): fail on a missing screenshot baseline in CI --- wdio.shared.conf.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/wdio.shared.conf.ts b/wdio.shared.conf.ts index d4aecac44..e042de090 100644 --- a/wdio.shared.conf.ts +++ b/wdio.shared.conf.ts @@ -143,7 +143,10 @@ export const config: Options.Testrunner = { // Pinned geometry, so no {platformName}/{width}x{height}; one shared baseline. formatImageName: '{tag}-{browserName}-{dpr}', screenshotPath: TEMP_DIR, - autoSaveBaseline: true, + // A missing baseline is written and passes, which suits a local run + // adding a screenshot. On CI it would turn a renamed tag into a test + // that compares nothing. + autoSaveBaseline: !IS_CI, }, ], 'cleanuptotal', From 35fd1fbccd1c9230e69555e8f4076f6d6586857d Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 21:16:21 -0400 Subject: [PATCH 5/9] refactor(state): drop the paint labelmapOpacity field nothing reads or writes --- src/io/state-file/schema.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/io/state-file/schema.ts b/src/io/state-file/schema.ts index 9be42ba9c..558bf6c0f 100644 --- a/src/io/state-file/schema.ts +++ b/src/io/state-file/schema.ts @@ -414,7 +414,6 @@ const Paint = z.object({ activeSegment: z.number().nullish(), brushSize: z.number().optional(), crossPlaneSync: z.boolean().optional(), - labelmapOpacity: z.number().optional(), }); const LPSCroppingPlanes = z.object({ From 4362d72e27272ec7f8e976cdae960e28f9b2e33c Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sat, 19 Sep 2026 21:01:32 +0000 Subject: [PATCH 6/9] fix(crop): skip restored crops whose image did not load Restoring a state file mapped each saved crop entry's image id through the restore dataIDMap without checking that the image had actually loaded. A dataset that could not be reached is absent from that map, so its saved planes were seated under the key `undefined`, clamped against an empty extent to a degenerate zero-width crop. No image with that id is ever deleted, so the store's onImageDeleted cascade could never drop the entry: it survived in `tools.crop` and was written back out on every later save, leaving the saved manifest with a crop keyed by an image no dataset in the file describes, which the dev-only save backstop reports as a dangling reference. Deserialization now skips a saved crop whose image did not resolve, so the crops of resolved images survive a save unchanged. --- .../cropRestoreSkipsUnloadedImage.spec.ts | 105 ++++++++++++++++++ src/store/tools/crop.ts | 5 + 2 files changed, 110 insertions(+) create mode 100644 src/store/tools/__tests__/cropRestoreSkipsUnloadedImage.spec.ts diff --git a/src/store/tools/__tests__/cropRestoreSkipsUnloadedImage.spec.ts b/src/store/tools/__tests__/cropRestoreSkipsUnloadedImage.spec.ts new file mode 100644 index 000000000..2e5ac489e --- /dev/null +++ b/src/store/tools/__tests__/cropRestoreSkipsUnloadedImage.spec.ts @@ -0,0 +1,105 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { setActivePinia, createPinia } from 'pinia'; +import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData'; +import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray'; + +import { useImageCacheStore } from '@/src/store/image-cache'; +import { collectManifestRefs } from '@/src/core/manifestRefs'; +import { + ManifestSchema, + type Manifest, + type StateFile, +} from '@/src/io/state-file/schema'; +import { useCropStore } from '@/src/store/tools/crop'; + +// --------------------------------------------------------------------------- +// Regression: the same unguarded dataIDMap lookup de729cfa fixed for +// annotations. A dataset that could not be loaded is absent from the restore +// map, so its saved crop planes used to be seated under the key `undefined`. +// Nothing ever deletes that image, so the crop store's onImageDeleted cascade +// cannot drop the entry and every later save carries it: a `tools.crop` key no +// dataset in the file describes, which the save-time reference backstop reports +// as dangling. +// --------------------------------------------------------------------------- + +const LOADED = 'img-loaded'; +const MISSING = 'img-missing'; + +/** A unit-spacing cube wide enough that the planes below need no clamping. */ +const seat = (id: string) => { + const image = vtkImageData.newInstance(); + image.setDimensions(8, 8, 8); + image.getPointData().setScalars( + vtkDataArray.newInstance({ + name: 'scalars', + numberOfComponents: 1, + values: new Uint8Array(8 ** 3), + }) + ); + return useImageCacheStore().addVTKImageData(image, id, { id }); +}; + +const planes = (lower: number, upper: number) => ({ + Sagittal: [lower, upper] as [number, number], + Coronal: [lower, upper] as [number, number], + Axial: [lower, upper] as [number, number], +}); + +/** Crop planes on two images, shaped the way a save writes them. */ +const savedCrop = (): Manifest => ({ + version: '1.0.0', + dataSources: [], + tools: { crop: { [LOADED]: planes(1, 4), [MISSING]: planes(2, 5) } }, +}); + +/** Serialize the store the way `serialize` does, into a bare manifest. */ +const resave = () => { + const stateFile = { manifest: { tools: {} } } as unknown as StateFile; + useCropStore().serialize(stateFile); + return stateFile.manifest; +}; + +describe('restoring crop planes whose image did not load', () => { + beforeEach(() => { + setActivePinia(createPinia()); + }); + + it('skips the planes of the image that is missing', () => { + seat(LOADED); + useCropStore().deserialize(savedCrop(), { [LOADED]: LOADED }); + + const cropping = useCropStore().croppingByImageID; + expect(Object.keys(cropping)).toEqual([LOADED]); + expect(cropping[LOADED]).toEqual(planes(1, 4)); + }); + + it('keeps the surviving planes saveable and free of dangling references', () => { + seat(LOADED); + useCropStore().deserialize(savedCrop(), { [LOADED]: LOADED }); + + const manifest = resave(); + expect(Object.keys(manifest.tools!.crop!)).toEqual([LOADED]); + expect(ManifestSchema.shape.tools.safeParse(manifest.tools).success).toBe( + true + ); + expect( + collectManifestRefs(manifest as unknown as Record).map( + (ref) => ref.where + ) + ).toEqual([`tools.crop[${LOADED}]`]); + }); + + it('follows the image the planes were remapped onto', () => { + seat('new-id'); + useCropStore().deserialize(savedCrop(), { [LOADED]: 'new-id' }); + + expect(Object.keys(useCropStore().croppingByImageID)).toEqual(['new-id']); + }); + + it('restores nothing when no image came back', () => { + useCropStore().deserialize(savedCrop(), {}); + + expect(useCropStore().croppingByImageID).toEqual({}); + expect(resave().tools!.crop).toEqual({}); + }); +}); diff --git a/src/store/tools/crop.ts b/src/store/tools/crop.ts index 2aa6c3fdf..9382f62b4 100644 --- a/src/store/tools/crop.ts +++ b/src/store/tools/crop.ts @@ -195,6 +195,11 @@ export const useCropStore = defineStore('crop', () => { Object.entries(cropping).forEach(([imageID, planes]) => { const newImageID = dataIDMap[imageID]; + // An image that did not load has no extent to clamp against and no view + // to crop. Seating its planes anyway keys them by a missing id, and the + // cascade above can never drop that entry because no such image is ever + // deleted: it is written back out on every later save. + if (newImageID === undefined) return; setCropping(newImageID, planes); }); } From 049b0bd708327510dc303cf8833e2191f2749e59 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sat, 19 Sep 2026 21:03:48 +0000 Subject: [PATCH 7/9] fix(layers): skip restored layers whose image did not load Restoring a state file mapped each saved layer relationship's parent and source through the restore dataIDMap without checking that either image had actually loaded. A dataset that could not be reached is absent from that map, so addLayer was handed a missing id and only failed once the build was under way. A missing parent was the worse of the two. addLayer's failure path assigns `parentToLayers[parent]` the result of filtering a list that is not there, which seats the key `undefined` with the value `undefined`. Serialize maps over every entry, so the next save threw a TypeError before it could write anything, and the user saw only a failed save. A missing source instead left a layer whose selection was undefined hanging off a real parent, and the save wrote that parent's whole relationship with an undefined source key, which the manifest schema rejects: the parent's valid layers were dropped from the file with it. Deserialization now skips a saved relationship whose parent did not resolve and each source that did not resolve, so the layers of resolved images are built and saved unchanged. --- src/store/__tests__/datasets-layers.spec.ts | 84 +++++++++++++++++++++ src/store/datasets-layers.ts | 6 ++ 2 files changed, 90 insertions(+) diff --git a/src/store/__tests__/datasets-layers.spec.ts b/src/store/__tests__/datasets-layers.spec.ts index 1e73059ef..53f2736e5 100644 --- a/src/store/__tests__/datasets-layers.spec.ts +++ b/src/store/__tests__/datasets-layers.spec.ts @@ -14,6 +14,11 @@ vi.mock('@/src/io/resample/resample', () => ({ ensureSameSpace })); import { useLayersStore } from '@/src/store/datasets-layers'; import { useImageCacheStore } from '@/src/store/image-cache'; import { useMessageStore } from '@/src/store/messages'; +import { + ParentToLayers, + type Manifest, + type StateFile, +} from '@/src/io/state-file/schema'; // A unit-spacing cube at `origin`, so its bounds are the numbers the overlap // check reads: an n-wide cube at o spans [o, o + n - 1] on every axis. @@ -104,3 +109,82 @@ describe('useLayersStore.remove', () => { expect(cached('parent::source')).toBe(false); }); }); + +// --------------------------------------------------------------------------- +// Regression: the same unguarded dataIDMap lookup de729cfa fixed for +// annotations. A dataset that could not be loaded is absent from the restore +// map, so a saved layer relationship naming it used to reach `addLayer` with a +// missing id. That only fails once the build is already under way, after the +// relationship has been written into `parentToLayers`, keyed by, or pointing +// at, an id no image has. +// --------------------------------------------------------------------------- + +const savedLayers = ( + selectionKey: string, + sourceSelectionKeys: string[] +): Manifest => ({ + version: '1.0.0', + dataSources: [], + parentToLayers: [{ selectionKey, sourceSelectionKeys }], +}); + +/** Serialize the store the way `serialize` does, into a bare manifest. */ +const resave = () => { + const stateFile = { manifest: {} } as unknown as StateFile; + useLayersStore().serialize(stateFile); + return stateFile.manifest.parentToLayers; +}; + +/** Lets every pending layer build settle, successfully or not. */ +const settle = () => + new Promise((resolve) => { + setTimeout(resolve); + }); + +describe('useLayersStore.deserialize with an image that did not load', () => { + it('restores nothing when the layer parent did not load', async () => { + seatImage('source', 0); + const store = useLayersStore(); + + store.deserialize(savedLayers('parent', ['source']), { + source: 'source', + }); + await settle(); + + expect(Object.keys(store.parentToLayers)).toEqual([]); + expect(useMessageStore().messages).toHaveLength(0); + // Before the guard this threw: the failed build left `parentToLayers` with + // a key whose value was `undefined`, and serialize mapped over it. + expect(resave()).toEqual([]); + }); + + it('restores nothing when the layer source did not load', async () => { + seatImage('parent', 0); + const store = useLayersStore(); + + store.deserialize(savedLayers('parent', ['source']), { + parent: 'parent', + }); + await settle(); + + expect(store.getLayers('parent')).toHaveLength(0); + expect(useMessageStore().messages).toHaveLength(0); + // Before the guard the parent's whole relationship was saved with an + // `undefined` source key, which the save-time schema rejects outright. + expect(ParentToLayers.safeParse(resave()).success).toBe(true); + }); + + it('still builds a relationship whose images both came back', () => { + seatOverlappingPair(); + const store = useLayersStore(); + + store.deserialize(savedLayers('saved-parent', ['saved-source']), { + 'saved-parent': 'parent', + 'saved-source': 'source', + }); + + expect(store.getLayers('parent').map(({ id }) => id)).toEqual([ + 'parent::source', + ]); + }); +}); diff --git a/src/store/datasets-layers.ts b/src/store/datasets-layers.ts index 44fa68590..d2edb7783 100644 --- a/src/store/datasets-layers.ts +++ b/src/store/datasets-layers.ts @@ -137,8 +137,14 @@ export const useLayersStore = defineStore('layer', () => { parentToLayersSerialized.forEach( ({ selectionKey, sourceSelectionKeys }) => { const parent = remapSelection(selectionKey); + // An image that did not load cannot be a layer parent or a layer + // source. Handing `addLayer` a missing id only fails later, after it + // has already written the relationship into `parentToLayers` under + // that missing id, where the next serialize trips over it. + if (parent === undefined) return; sourceSelectionKeys.forEach((sourceKey) => { const source = remapSelection(sourceKey); + if (source === undefined) return; addLayer(parent, source); }); } From d51bbcedd01be7db592873d055332db44ca8028b Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Sun, 20 Sep 2026 19:01:49 +0000 Subject: [PATCH 8/9] fix(processing): expire the session when the job history load gets a 401 Polling, cancel and result loading all classify their errors and hand a 401 to markSessionExpired. The job-history load did not: it logged and recorded a per-provider string, so the session stayed live in the store, the persistent reload notice never appeared, and the panel offered a Retry that could only 401 again. Its catch now goes through expireSessionIf first, and both the history request and the page loop stop once the session is expired, so nothing re-issues a request that cannot succeed. The two catches build their detail string in a local, which keeps the file inside the line ratchet. A case in the store spec drives a 401 listing through adoption and asserts the notice, the empty history error, and that a retry sends nothing. --- src/processing/__tests__/store.spec.ts | 21 +++++++++++++++++++++ src/processing/store.ts | 19 +++++++++---------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/processing/__tests__/store.spec.ts b/src/processing/__tests__/store.spec.ts index aecfca1fe..fd0e020d0 100644 --- a/src/processing/__tests__/store.spec.ts +++ b/src/processing/__tests__/store.spec.ts @@ -1531,6 +1531,27 @@ describe('Providers store — re-discovered job history: slim observability adop expect(provider.getResults).toHaveBeenCalledTimes(1); }); + // Every other request path routes a 401 through classifyError to + // markSessionExpired; the history load used to record a per-provider error + // string instead, leaving the panel with a Retry that could only 401 again. + it('marks the session expired when the job history load 401s', async () => { + const listJobHistory = vi.fn().mockRejectedValue(httpError(401)); + const store = arrange(makeProvider({ listJobHistory })); + + await store.adoptJobHistory(); + + expect(store.sessionExpired).toBe(true); + const expiry = useMessageStore().messages.find((m) => + /session has expired/i.test(m.title) + ); + expect(expiry?.options.persist).toBe(true); + expect(store.jobHistoryError).toBeNull(); + + await store.loadAllJobHistory(); + + expect(listJobHistory).toHaveBeenCalledTimes(1); + }); + it('a re-discovery listing failure is not fatal (logged, degrades)', async () => { const err = vi.spyOn(console, 'error').mockImplementation(() => {}); const provider = makeProvider({ diff --git a/src/processing/store.ts b/src/processing/store.ts index 13734f114..f8742967d 100644 --- a/src/processing/store.ts +++ b/src/processing/store.ts @@ -930,10 +930,8 @@ export const useProcessingJobsStore = defineStore('processingJobs', () => { try { provider = await getProvider(providerId); } catch (err) { - jobHistoryErrors.set( - providerId, - getErrorDetail(err, 'Failed to load job history') - ); + const detail = getErrorDetail(err, 'Failed to load job history'); + jobHistoryErrors.set(providerId, detail); return; } const cursor = jobHistoryCursors.get(providerId) ?? undefined; @@ -956,16 +954,16 @@ export const useProcessingJobsStore = defineStore('processingJobs', () => { ); } } catch (err) { + // A 401 is the session, not this page, so it is reported as one. + if (expireSessionIf(err)) return; console.error('Job re-discovery failed', err); - jobHistoryErrors.set( - providerId, - getErrorDetail(err, 'Failed to load job history') - ); + const detail = getErrorDetail(err, 'Failed to load job history'); + jobHistoryErrors.set(providerId, detail); } } async function loadMoreJobHistory() { - if (jobHistoryComplete.value) return; + if (sessionExpired.value || jobHistoryComplete.value) return; if (jobHistoryRequest) return jobHistoryRequest; jobHistoryRequest = (async () => { jobHistoryLoading.value = true; @@ -994,7 +992,8 @@ export const useProcessingJobsStore = defineStore('processingJobs', () => { } while ( pageCount < MAX_JOB_HISTORY_PAGES && !jobHistoryComplete.value && - jobHistoryError.value == null + jobHistoryError.value == null && + !sessionExpired.value ); if ( From c3df55b1649a8be3daf940a5c26fcfef31e39960 Mon Sep 17 00:00:00 2001 From: Paul Elliott Date: Wed, 16 Sep 2026 01:38:38 -0400 Subject: [PATCH 9/9] test: make stream resume coverage independent of network timing --- .../__tests__/cachedStreamFetcher.spec.ts | 110 +++++++++++++----- 1 file changed, 78 insertions(+), 32 deletions(-) diff --git a/src/core/streaming/__tests__/cachedStreamFetcher.spec.ts b/src/core/streaming/__tests__/cachedStreamFetcher.spec.ts index 8626fcbc6..d7eb46216 100644 --- a/src/core/streaming/__tests__/cachedStreamFetcher.spec.ts +++ b/src/core/streaming/__tests__/cachedStreamFetcher.spec.ts @@ -2,51 +2,97 @@ import { RequestPool } from '@/src/core/streaming/requestPool'; import { CachedStreamFetcher, sliceChunks, - StopSignal, } from '@/src/core/streaming/cachedStreamFetcher'; import { describe, expect, it } from 'vitest'; +const readStream = async ( + stream: ReadableStream, + stopAfter = Infinity +) => { + const reader = stream.getReader(); + const chunks: Uint8Array[] = []; + let size = 0; + let completed = false; + try { + while (size <= stopAfter) { + const result = await reader.read(); + if (result.done) { + completed = true; + break; + } + chunks.push(result.value); + size += result.value.length; + } + } finally { + if (!completed) await reader.cancel(); + reader.releaseLock(); + } + + const bytes = new Uint8Array(size); + let offset = 0; + chunks.forEach((chunk) => { + bytes.set(chunk, offset); + offset += chunk.length; + }); + return bytes; +}; + describe('CachedStreamFetcher', () => { it('should support stopping and resuming', async () => { - const pool = new RequestPool(); - const fetcher = new CachedStreamFetcher( - 'https://data.kitware.com/api/v1/file/57b5d4648d777f10f2693e7e/download', - { - fetch: pool.fetch, - } + const source = Uint8Array.from( + { length: 32 * 1024 + 123 }, + (_, index) => (index * 31) % 251 ); + const requestedRanges: Array = []; + const fetchRange: typeof fetch = async (_input, init) => { + const range = new Headers(init?.headers).get('Range'); + requestedRanges.push(range); + const start = range ? Number(range.match(/^bytes=(\d+)-$/)?.[1]) : 0; + let offset = start; + const body = new ReadableStream({ + pull(controller) { + if (offset === source.length) { + controller.close(); + return; + } + const end = Math.min(offset + 4096, source.length); + controller.enqueue(source.slice(offset, end)); + offset = end; + }, + }); + return new Response(body, { + status: start === 0 ? 200 : 206, + headers: { + 'content-length': String(source.length - start), + ...(start === 0 + ? {} + : { + 'content-range': `bytes ${start}-${source.length - 1}/${source.length}`, + }), + }, + }); + }; + const pool = new RequestPool(1, fetchRange); + const fetcher = new CachedStreamFetcher('https://example.test/data', { + fetch: pool.fetch, + }); await fetcher.connect(); - let stream = fetcher.getStream(); - let size = 0; - try { - // @ts-ignore - for await (const chunk of stream) { - size += chunk.length; - if (size > 4096 * 3) { - break; - } - } - } catch (err) { - if (err !== StopSignal) throw err; - } finally { - fetcher.close(); - } + const partial = await readStream(fetcher.getStream(), 4096 * 3); + expect(partial).toEqual(source.slice(0, partial.length)); + const resumeAt = fetcher.size; + expect(resumeAt).toBeGreaterThanOrEqual(partial.length); + expect(resumeAt).toBeLessThan(source.length); + fetcher.close(); await fetcher.connect(); + expect(requestedRanges).toEqual([null, `bytes=${resumeAt}-`]); - // ensure we can read the stream multiple times for (let i = 0; i < 2; i++) { - stream = fetcher.getStream(); - size = 0; - // @ts-ignore - - for await (const chunk of stream) { - size += chunk.length; - } - - expect(size).to.equal(fetcher.size); + expect(await readStream(fetcher.getStream())).toEqual(source); } + expect(fetcher.size).toBe(source.length); + expect(requestedRanges).toHaveLength(2); fetcher.close(); });