fix: Download of binary DICOM and study index writes wrong files (rebase of #127 onto current master) - #131
Open
awatson1978 wants to merge 6 commits into
Open
fix: Download of binary DICOM and study index writes wrong files (rebase of #127 onto current master)#131awatson1978 wants to merge 6 commits into
awatson1978 wants to merge 6 commits into
Conversation
…er bun - Declare <studyUIDs...> as a required positional on the studies command so commander reports a missing-argument error instead of exiting silently (previously the variadic -d option could swallow the study UID, leaving the command with no arguments and studiesMain returned without a message). - Import uids via its subpath in static-wado-creator index.mjs; mixing an ESM import of the static-wado-util package root with the CJS require() calls elsewhere in the package made bun treat the root as an async module and fail with "require() async module is unsupported". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On upload, fileToKey strips "/index.json.gz" so the study singleton index is stored at the S3 key "studies/<uid>". On retrieve, contentItemToFileName had no reverse mapping for that key shape and fell through to "studies/<uid>.gz", so the study index never landed at studies/<uid>/index.json.gz and mkdicomweb index could not add the study to the overall studies index. dir() now detects directory-style index objects generically: any listed key that also has child keys maps back to <key>/index.json.gz. This covers the study singleton, the overall studies index, and instance-level query indexes, without hardcoding path names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fileToKey strips the .mht extension on upload, so multipart objects live at extension-less keys (frames/1, bulkdata/xx/yy/zzzz). The directory listing can only guess "<key>.gz" for those, which stored retrieved frames as 1.gz instead of 1.mht / 1.mht.gz. - S3Ops.retrieve now corrects the destination name from the object's ContentType/ContentEncoding: multipart/related objects are written as <key>.mht, or <key>.mht.gz when gzip encoded. - S3Ops.localCandidates lists the possible local names for a listed object so DeployGroup.retrieve can recognize already-retrieved .mht files and skip re-downloading them. - DeployGroup renames files stored under the generic ".gz" name by older retrieves to their correct multipart name, sniffing the leading bytes (multipart boundary, or gzip whose content starts with a boundary) so gzipped JSON like metadata.gz is left alone. Skipped on --dry-run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tream writeRawPart10 re-read the received bytes from the parse stream, which only worked because the STOW upload stream never released its buffers - so it silently skipped (or threw on an undefined buffer) on the file-stream path, and kept a whole instance resident per concurrent upload on the STOW path. Regenerate the rendition from the parsed dataset via generatePart10ForInstance instead, re-reading the frame and bulkdata files just written, and turn on clearBuffers for the STOW stream now that nothing needs the received bytes. The rendition write moves after commitPendingMoves, since the files it reads back are at temp paths until then, and is written in 1 MiB chunks so it honours the stream highWaterMark. Also fix readBulkData ignoring byteOffset: Node pools Buffers under 4k, so data.buffer.slice(start, end) read unrelated pool memory for most frame and bulkdata files. This corrupted the part10 controller generation fallback and the part10Main CLI export as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
part10Controller: a multipart/related body cannot be parsed without its boundary, but neither stored-rendition branch sent one. Read it from the stored wrapper's delimiter line and advertise it, and stop dicomMap replacing a fuller multipart/related type with its boundary-less default. That same read tells us whether the file is a usable wrapper at all, so an unusable one falls through to generation rather than being served broken. part10Controller: stream application/dicom out of the wrapper (file -> gunzip -> payload transform -> res) rather than readFileSync + gunzipSync of the whole rendition, which blocked the event loop and held three copies of a large SEG. The transform withholds a 1k tail so the closing boundary is never emitted. zip still buffers - adm-zip needs the whole member - but reads asynchronously. DeployGroup: await ops.shouldSkip. The un-awaited Promise was always truthy, so every existing local file was skipped regardless of size or ETag and a truncated file could never be repaired. Multipart sniffing: require a boundary delimiter line followed by a MIME header instead of a bare leading "--". Bulkdata is stored under extension-less keys and is itself multipart, so the discrimination has to come from the content, not the path. Moved to static-wado-util/multipartFile.mjs so s3-deploy shares it. S3Ops: check every localCandidates name before skipping a retrieve, and correct a multipart file left at the guessed name instead of stranding it there. Derive retrieveFileName and localCandidates from one multipartBase helper so they cannot disagree - localCandidates split on "/" only, so it produced no candidates for Windows paths. S3Ops: only extension-less frame files are multipart wrappers. Raw codestreams take their content type from the transfer syntax table's extensions, instead of being labelled multipart/related whenever mime.lookup fails. Tests for all of the above, including the first suite in s3-deploy - its .gitignore excluded the whole test directory, now narrowed to the compiled output it meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
This is #127, rebased onto current master so it applies cleanly again after #128, #129, and the ESM/CI updates landed. All four commits are unchanged and still carry original authorship.
What the original PR does (unchanged)
.mht/.mht.gznames, and study-levelindex.json.gzfiles landed in the wrong place. The downloader now uses the object's content type and encoding, plus whether a key has children in the listing, to restore the correct local names.require()calls;uidsis now imported via its subpath.instances/<sop>/index.mht.gzat ingest time, so instance retrieval can serve the file as received instead of reconstructing it.The one conflict, and how it was resolved
instanceFromStream.mjs: this branch adds the raw Part 10 write just before the end-of-instance wait, but #129 replaced that wait (awaitAllStreams) withdrainOpenStreams. Resolution: the raw Part 10 write runs first, thendrainOpenStreams. That order matters — the raw write opens a stream through the writer, and the drain is what guarantees it (and any still-closing frame/bulkdata streams) is flushed, closed, and released before the instance is committed. Everything else in the file (the tracker now being created before the writer, the open-stream limit in the listener drain) merged without conflict.Testing
create-dicomweb: 59/59 pass, including the full back-pressure suite from fix: Honour write back pressure so large multi-frame instances don't exhaust file handles #129 running on top of these changesstatic-wado-webserverandstatic-wado-deploy: passs3-deploy: has no tests — noting because that's where the biggest chunk of this PR lives, so nothing automated guards the filename mapping. Even one round-trip test forretrieveFileName/localCandidateswould earn its keep.One thing worth a second look before merge
writeRawPart10re-reads the whole instance in one go viastream.getBuffer(0, rawSize). For typical SEG/SR that's fine, but SR instances can get genuinely large, and a single whole-instance buffer is the same allocation pattern #129 just worked to keep bounded on the write side. A size cap (skip raw storage above N bytes, log it) or a chunked copy from the retained parse buffers would close that off.🤖 Generated with Claude Code