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__/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(); }); 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/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({ 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 ( 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__/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/__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/src/store/datasets-layers.ts b/src/store/datasets-layers.ts index 60138fa3d..d2edb7783 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; } }); @@ -135,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); }); } 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); }); } 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); 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)); 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',