fix(layers): stop the layer cache from disposing the source image

src/components/ImageDataBrowser.vue

@@ -288,11 +288,18 @@ export default defineComponent({
                 Spacing: ({{ image.spacing.join(', ') }})
               </div>
             </div>
-            <v-btn icon variant="plain" size="x-small" class="dataset-menu">
+            <v-btn
+              icon
+              variant="plain"
+              size="x-small"
+              class="dataset-menu"
+              data-testid="dataset-menu-button"
+            >
               <v-menu activator="parent">
                 <v-list>
                   <v-list-item
                     v-if="image.layerable"
+                    data-testid="dataset-menu-layer-item"
                     @click.stop="image.layerHandler()"
                   >
                     <template v-if="image.layerLoading">

src/core/streaming/dicomChunkImage.ts

@@ -329,8 +329,11 @@ export default class DicomChunkImage
   }
 
   private reallocateImage() {
-    this.vtkImageData.value.delete();
+    // Allocation can throw, and the cache keeps this image either way, so the
+    // old buffer is only safe to release once its replacement exists.
+    const previous = this.vtkImageData.value;
     this.vtkImageData.value = allocateImageFromChunks(this.chunks);
+    previous.delete();
     this.applyUltrasoundSpacing();
   }
 

src/io/resample/resample.ts

@@ -2,6 +2,7 @@ import { Image } from 'itk-wasm';
 import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData';
 import vtkITKHelper from '@kitware/vtk.js/Common/DataModel/ITKHelper';
 import { compareImageIndexGrids } from '@/src/utils/imageSpace';
+import { shallowCopyImageData } from '@/src/utils/vtk-helpers';
 import { runWasm } from './itkWasmUtils';
 
 export async function resample(fixed: Image, moving: Image, label = false) {
@@ -27,8 +28,10 @@ export async function ensureSameSpace(
   resampleCandidate: vtkImageData,
   label = false
 ) {
+  // Callers own what they get back and may hand it to something that disposes
+  // it, so never return the candidate itself.
   if (compareImageIndexGrids(target, resampleCandidate)) {
-    return resampleCandidate;
+    return shallowCopyImageData(resampleCandidate);
   }
   const itkImage = await resample(
     vtkITKHelper.convertVtkToItkImage(target),

src/store/__tests__/image-cache.spec.ts

@@ -33,4 +33,13 @@ describe('image cache lifecycle', () => {
 
     expect(store.getVtkImageData('img-1')).toBeNull();
   });
+
+  it('treats a disposed image still in the cache as absent', () => {
+    const store = seatImage();
+    // vtk.js delete() strips the model but keeps the closures, so the getters
+    // return undefined instead of throwing.
+    store.imageById['img-1'].dispose();
+
+    expect(store.getVtkImageData('img-1')).toBeNull();
+  });
 });

src/store/__tests__/layerImageOwnership.spec.ts

@@ -0,0 +1,70 @@
+import { describe, it, expect, beforeEach } from 'vitest';
+import { setActivePinia, createPinia } from 'pinia';
+import { nextTick } from 'vue';
+import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData';
+import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray';
+
+// The real `ensureSameSpace` runs here. Matching grids take its fast path, so
+// no wasm resampling is involved.
+import { useLayersStore } from '@/src/store/datasets-layers';
+import { useImageCacheStore } from '@/src/store/image-cache';
+
+const SIZE = 3;
+
+const seatImage = (id: string) => {
+  const image = vtkImageData.newInstance();
+  image.setDimensions(SIZE, SIZE, SIZE);
+  image.getPointData().setScalars(
+    vtkDataArray.newInstance({
+      name: 'scalars',
+      numberOfComponents: 1,
+      values: new Uint8Array(SIZE ** 3),
+    })
+  );
+  return useImageCacheStore().addVTKImageData(image, id, { id });
+};
+
+const sourceScalars = () =>
+  useImageCacheStore()
+    .getVtkImageData('source')
+    ?.getPointData()
+    .getScalars()
+    ?.getData();
+
+beforeEach(() => {
+  setActivePinia(createPinia());
+  seatImage('parent');
+  seatImage('source');
+});
+
+describe('layer image ownership', () => {
+  it('caches a distinct image from the source it was built from', async () => {
+    const imageCache = useImageCacheStore();
+
+    await useLayersStore().addLayer('parent', 'source');
+
+    expect(imageCache.getVtkImageData('parent::source')).not.toBe(
+      imageCache.getVtkImageData('source')
+    );
+  });
+
+  it('leaves the source image readable after its parent is removed', async () => {
+    const store = useLayersStore();
+    await store.addLayer('parent', 'source');
+
+    store.remove('parent');
+    await nextTick();
+
+    expect(sourceScalars()).toHaveLength(SIZE ** 3);
+  });
+
+  it('leaves the source image readable after the layer is deleted', async () => {
+    const store = useLayersStore();
+    await store.addLayer('parent', 'source');
+
+    store.deleteLayer('parent', 'source');
+    await nextTick();
+
+    expect(sourceScalars()).toHaveLength(SIZE ** 3);
+  });
+});

src/store/__tests__/messages.spec.ts

@@ -1,5 +1,10 @@
 import { describe, it, beforeEach, expect } from 'vitest';
 import { setActivePinia, createPinia } from 'pinia';
+import { nextTick } from 'vue';
+import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray';
+import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData';
+import { useImageStore } from '@/src/store/datasets-images';
+import { useImageCacheStore } from '@/src/store/image-cache';
 import { MessageType, useMessageStore } from '@/src/store/messages';
 
 describe('Message store', () => {
@@ -66,4 +71,27 @@ describe('Message store', () => {
     messageStore.clearAll();
     expect(messageStore.messages).to.be.empty;
   });
+
+  it('still reports an error when a dataset image was disposed', async () => {
+    // addError builds a bug report from the image cache, so a dataset the
+    // cache can no longer read must not cost the user the message itself.
+    const image = vtkImageData.newInstance();
+    image.setDimensions(2, 2, 2);
+    image.getPointData().setScalars(
+      vtkDataArray.newInstance({
+        name: 'scalars',
+        numberOfComponents: 1,
+        values: new Uint8Array(8),
+      })
+    );
+    useImageStore().addVTKImageData('CT', image, { id: 'img-1' });
+    await nextTick();
+    useImageCacheStore().imageById['img-1'].dispose();
+
+    const messageStore = useMessageStore();
+    messageStore.addError('an error', { error: new Error('boom') });
+
+    expect(messageStore.messages).toHaveLength(1);
+    expect(messageStore.messages[0].title).toBe('an error');
+  });
 });

src/store/__tests__/segmentImportPath.spec.ts

@@ -44,7 +44,12 @@ describe('labelmap import index alignment', () => {
     const runWasm = useNodeResampling();
     const parent = makeImage();
     const child = makeImage();
-    expect(await ensureSameSpace(parent, child, true)).toBe(child);
+    const result = await ensureSameSpace(parent, child, true);
+    // The contract is no resampling and no voxel copy. The wrapper is the
+    // caller's to own, so it is not the candidate itself.
+    expect(result.getPointData().getScalars().getData()).toBe(
+      child.getPointData().getScalars().getData()
+    );
     expect(runWasm).not.toHaveBeenCalled();
   });
 

src/store/image-cache.ts

@@ -35,9 +35,12 @@ export const useImageCacheStore = defineStore('image-cache', () => {
     const image = imageById[id];
     if (!image) return null;
     const data = image.getVtkImageData();
+    // A deleted vtkImageData keeps its closures but loses its model, so every
+    // getter returns undefined rather than throwing on access.
+    if (!data || data.isDeleted()) return null;
     // ProgressiveImage initializes with empty vtkImageData before actual data loads.
     // VTK.js volume renderer crashes on empty data (null scalar texture).
-    if (!data?.getPointData().getScalars()?.getData()?.length) return null;
+    if (!data.getPointData().getScalars()?.getData()?.length) return null;
     return data;
   }
 

src/store/messages.ts

@@ -37,6 +37,17 @@ interface State {
   msgList: string[];
 }
 
+// The reporter reads application state that an error may have just corrupted,
+// and it runs before the message exists, so a throw here costs the user the
+// message itself.
+function describeBug(error?: Error) {
+  try {
+    return generateBugReport(error);
+  } catch {
+    return 'Bug report unavailable';
+  }
+}
+
 export const useMessageStore = defineStore('message', {
   state: (): State => ({
     _nextID: 1,
@@ -60,7 +71,7 @@ export const useMessageStore = defineStore('message', {
         {
           type: MessageType.Error,
           title,
-          bugReport: generateBugReport(opts?.error),
+          bugReport: describeBug(opts?.error),
         },
         {
           details: opts?.details ?? opts?.error?.stack,

src/utils/__tests__/bugReport.spec.ts

@@ -0,0 +1,51 @@
+import { beforeEach, describe, expect, it } from 'vitest';
+import { createPinia, setActivePinia } from 'pinia';
+import { nextTick } from 'vue';
+import vtkDataArray from '@kitware/vtk.js/Common/Core/DataArray';
+import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData';
+
+import { useImageStore } from '@/src/store/datasets-images';
+import { generateBugReport } from '@/src/utils/bugReport';
+
+const seatImage = (id = 'img-1') => {
+  const data = vtkImageData.newInstance();
+  data.setDimensions(2, 2, 2);
+  data.getPointData().setScalars(
+    vtkDataArray.newInstance({
+      name: 'scalars',
+      numberOfComponents: 1,
+      values: new Uint8Array(8),
+    })
+  );
+  useImageStore().addVTKImageData('CT', data, { id });
+};
+
+describe('generateBugReport', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia());
+  });
+
+  it('describes a healthy dataset', async () => {
+    seatImage();
+    // Image metadata is a Vue watcher, so it lands on the next flush.
+    await nextTick();
+
+    const report = generateBugReport(new Error('boom'));
+
+    expect(report).toContain('Datasets: 1');
+    expect(report).toContain('2×2×2 Uint8');
+    expect(report).toContain('boom');
+  });
+
+  it('counts every dataset it describes', async () => {
+    seatImage('img-1');
+    seatImage('img-2');
+    await nextTick();
+
+    const report = generateBugReport(new Error('boom'));
+
+    expect(report).toContain('Datasets: 2');
+    expect(report).toContain('[0]');
+    expect(report).toContain('[1]');
+  });
+});

src/utils/vtk-helpers.ts

@@ -12,6 +12,7 @@ import {
   OpacityNodes,
 } from '@/src/types/views';
 import vtkFieldData from '@kitware/vtk.js/Common/DataModel/DataSetAttributes/FieldData';
+import vtkImageData from '@kitware/vtk.js/Common/DataModel/ImageData';
 import { Maybe } from '@/src/types';
 
 export function computeWorldToDisplay(
@@ -290,3 +291,24 @@ export function getDataArray(
 export function isZeroWidthRange(range: [number, number] | number[]) {
   return range[0] === range[1];
 }
+
+/**
+ * Copies an image's geometry into a new vtkImageData sharing the same scalars.
+ *
+ * vtk.js `delete()` wipes only the object's own model, so the shared scalar
+ * array outlives either copy. Point data beyond the active scalars, cell data
+ * and field data are not copied.
+ *
+ * vtkImageData.shallowCopy assigns the index/world matrices by reference and
+ * computeTransforms rewrites them in place, so a later setSpacing on either
+ * image would corrupt the other.
+ */
+export function shallowCopyImageData(image: vtkImageData) {
+  const copy = vtkImageData.newInstance();
+  copy.setExtent(image.getExtent());
+  copy.setOrigin(image.getOrigin());
+  copy.setSpacing(image.getSpacing());
+  copy.setDirection(image.getDirection());
+  copy.getPointData().setScalars(image.getPointData().getScalars());
+  return copy;
+}

tests/specs/layer-source-lifetime.e2e.ts

@@ -0,0 +1,74 @@
+import { volViewPage } from '../pageobjects/volview.page';
+import { openVolViewPage, writeManifestToFile, writeMetaImage } from './utils';
+
+const MENU_BUTTON = '[data-testid="dataset-menu-button"]';
+const MENU_ITEM = '[data-testid="dataset-menu-layer-item"]';
+const WAIT = { timeout: 20_000 };
+
+const cardFor = async (name: string) => {
+  const card = $(`.image-list-card*=${name}`);
+  await card.waitForClickable(WAIT);
+  return card;
+};
+
+// The menu stays open after an item is picked, so opening one requires
+// closing whatever is already on screen.
+const clickDatasetMenuItem = async (name: string, itemText: string) => {
+  await browser.keys('Escape');
+  await browser.waitUntil(async () => (await $$(MENU_ITEM).length) === 0, {
+    ...WAIT,
+    timeoutMsg: 'Expected any open dataset menu to close',
+  });
+
+  await (await cardFor(name)).$(MENU_BUTTON).click();
+  // While a layer builds, the item renders a spinner instead of its label.
+  const item = $(`${MENU_ITEM}*=${itemText}`);
+  await item.waitForClickable(WAIT);
+  await item.click();
+};
+
+// The opacity slider appears once the layer image is in the cache, and the
+// dataset menus live on the Data tab, so step back after checking.
+const waitForLayerBuilt = async () => {
+  await volViewPage.renderingModuleTab.click();
+  await browser.waitUntil(
+    async () => (await volViewPage.layerOpacitySliders.length) > 0,
+    { ...WAIT, timeoutMsg: 'Expected the layer to finish building' }
+  );
+  await $('button[data-testid="module-tab-Data"]').click();
+};
+
+describe('An image used as a layer of another image', () => {
+  it('can be layered again after the layer is removed', async () => {
+    // Matching grids, so building the layer reuses the source image rather
+    // than resampling it.
+    const parentName = writeMetaImage(`layer-parent-${Date.now()}.mha`);
+    const sourceName = writeMetaImage(`layer-source-${Date.now()}.mha`, {
+      step: 29,
+    });
+    const manifestName = `layer-source-lifetime-${Date.now()}.json`;
+    await writeManifestToFile(
+      {
+        resources: [
+          { url: `/tmp/${parentName}`, name: parentName },
+          { url: `/tmp/${sourceName}`, name: sourceName },
+        ],
+      },
+      manifestName
+    );
+
+    await openVolViewPage(manifestName);
+    await (await cardFor(parentName)).click();
+
+    await clickDatasetMenuItem(sourceName, 'Add as layer');
+    await waitForLayerBuilt();
+    await clickDatasetMenuItem(sourceName, 'Remove as layer');
+
+    // Rebuilding reads the source image again, which fails once removing the
+    // layer has released the image the source dataset still owns.
+    await clickDatasetMenuItem(sourceName, 'Add as layer');
+
+    await waitForLayerBuilt();
+    expect(await volViewPage.getNotificationsCount()).toEqual(0);
+  });
+});