fix(dicom): preserve slices across incremental imports - #935
Open
PaulHax wants to merge 1 commit into
Open
Conversation
Adding chunks to an existing DICOM image re-sorts the list and reallocates the volume buffer. Slices already decoded were zeroed by the reallocation and never rewritten while their status still said Loaded. A decode in flight across that reallocation wrote through the slot it captured before the sort, so its pixels or its error could land on another chunk's slice. Two overlapping additions each awaited a sort of the shared list, and the sort that settled last won, dropping the newer call's chunks. The image cache also starts a load only when it first registers an image, so chunks added on a re-import were allocated for but never asked to load. Redecode every chunk that holds data after a reallocation and report those slices as Loading until the rewrite lands. Resolve a chunk's slot after its decode settles, and discard any result from a previous allocation generation, so completion order cannot overwrite current state. Address failures by chunk rather than by captured index. Serialize additions through a promise queue. Ask an already-registered image to start loading after a re-import adds to it.
✅ Deploy Preview for volview-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #934. This PR targets
dicom-volume-bufferso its diff contains only the incremental-import changes.Problem
addChunksappends the new chunks, re-sorts the whole list, and reallocates thevolume buffer. A second call is reachable when importing more slices of an
already-open series, and it discarded pixels that had already loaded:
yet
chunkStatusstill reported themLoaded.A related race sits inside the decode itself.
onChunkHasDatacaptures itsindex before awaiting the decode, and
addChunkscan re-sort and reallocatewhile that await is outstanding, so the index it writes through may no longer
belong to the chunk it decoded.
The old
processNewChunkspath also indexed already-loaded incoming chunks bytheir position in the arrival array rather than the sorted volume. Current UI
imports hand over metadata-only chunks, so that particular case was a latent API
trap rather than a user-reachable path. The reallocation loss and in-flight
decode race are reachable through the current incremental-import path.
The import flow had a gap of its own: the image cache only calls
startLoadwhen it first registers an image, so chunks added to an already-registered
image were sorted and allocated for but never asked to load their data. Their
slices stayed zero until something else triggered the load.
These failures were identified through code inspection and reproduced with
controlled unit tests; they were not traced to a reported user dataset.
Change
processNewChunksbecomesprocessLoadedChunks, which walksthis.chunksinsorted order after the reallocation and redecodes every chunk that already holds
data. Chunks that arrive later still emit
doneDataand take the normal path;chunks whose data is already in memory never will, so their slices are rewritten
here.
onChunkHasDatarecomputes the chunk's slot fromthis.chunksafter the decoderesolves, and uses that for the write offset, the status update, and the emitted
extent. A chunk no longer in the list belongs to a disposed or superseded image
and its result is dropped rather than written into someone else's slice.
addChunksmarked every chunk holding data asLoadedbefore reallocating, butreallocating zeroes their slices and only the redecode that follows rewrites
them. Between the two the image reported complete over an empty buffer. Those
chunks are
Loadinguntil their slice lands.A decode that settles after a reallocation belongs to the old buffer generation.
Its result is now discarded rather than allowed to overwrite the replacement
attempt's status or emit an obsolete error. Current-generation failures address
chunks rather than captured indices and resolve the slot when the result lands,
so a failure cannot mark a healthy slice errored.
addChunkscalls are serialized through a promise queue. Two overlapping callseach pushed their chunks and awaited a sort of the shared list, and whichever
sort settled last won, so an older sort could drop the chunks a newer call had
added. Each addition now waits for the one before it, and a failed addition
does not block the next.
importChunksin the DICOM store asks an already-registered image tostartLoadafter adding to it, so the new chunks load.Chunk.loadDatais ano-op for chunks that already hold data, so the chunks kept across the
re-import are not fetched again.
The constructor seam for
splitAndSortand the ITK read, introduced with thebuffer-type change, lets a unit test drive
addChunksin two batches witharrival order differing from sorted order and assert on decoded pixel
placement and the emitted extents.