Skip to content

fix: Honour write back pressure so large multi-frame instances don't exhaust file handles - #129

Merged
wayfarer3130 merged 2 commits into
masterfrom
fix/wsi-open-file-backpressure
Aug 11, 2026
Merged

fix: Honour write back pressure so large multi-frame instances don't exhaust file handles#129
wayfarer3130 merged 2 commits into
masterfrom
fix/wsi-open-file-backpressure

Conversation

@wayfarer3130

Copy link
Copy Markdown
Collaborator

Writing a whole slide image opened one file per frame with no effective limit: "[StreamInfo] open file count exceeded 500 by 0: totalOpen=500 totalClosed=107", climbing from there until the process ran out of handles.

The drain hook was already wired to a promise tracker, but instanceFromStream created that tracker after the writer and never passed it to the writer. The writer is what registers each stream's completion promise, so the tracker the drain waited on was always empty and limitUnsettled returned immediately. Create the tracker first and pass it to the writer, so back pressure actually engages.

Also bound and release handles directly:

  • DicomWebWriter.awaitOpenStreamLimit waits until fewer than maxOpenStreams (default 32, --max-open-files) streams are open, yielding to the event loop so stream finish callbacks are delivered instead of only draining microtasks. The listener drain awaits it before emitting more frame/bulkdata values.
  • DicomWebWriter.drainOpenStreams flushes the in-flight closes at the end of an instance (the filters close frames without awaiting) and destroys anything that was never closed rather than leaking its handle.
  • openStream no longer replaces an open stream when a streamKey repeats; the replaced stream was orphaned, so it never closed and its tracked promise never settled. The filters close the key the writer assigned.
  • StreamInfo removes its 'drain' listeners when the wait times out; they used to accumulate one per timed-out write.
  • StreamInfo records stream errors as failures, so an error before end() attaches its handlers no longer crashes the process or leaves the handle open forever.

Verified on 22MB, 99MB and 1.3GB WSI instances (1292, 1586 and 24617 frames): no open file count warnings, no temp files left behind, and for the 22MB instance output byte-identical to before apart from the random multipart boundaries, with unchanged runtime.

…exhaust file handles

Writing a whole slide image opened one file per frame with no effective limit:
"[StreamInfo] open file count exceeded 500 by 0: totalOpen=500 totalClosed=107",
climbing from there until the process ran out of handles.

The drain hook was already wired to a promise tracker, but instanceFromStream
created that tracker *after* the writer and never passed it to the writer. The
writer is what registers each stream's completion promise, so the tracker the
drain waited on was always empty and limitUnsettled returned immediately. Create
the tracker first and pass it to the writer, so back pressure actually engages.

Also bound and release handles directly:

- DicomWebWriter.awaitOpenStreamLimit waits until fewer than maxOpenStreams
  (default 32, --max-open-files) streams are open, yielding to the event loop so
  stream finish callbacks are delivered instead of only draining microtasks. The
  listener drain awaits it before emitting more frame/bulkdata values.
- DicomWebWriter.drainOpenStreams flushes the in-flight closes at the end of an
  instance (the filters close frames without awaiting) and destroys anything that
  was never closed rather than leaking its handle.
- openStream no longer replaces an open stream when a streamKey repeats; the
  replaced stream was orphaned, so it never closed and its tracked promise never
  settled. The filters close the key the writer assigned.
- StreamInfo removes its 'drain' listeners when the wait times out; they used to
  accumulate one per timed-out write.
- StreamInfo records stream errors as failures, so an error before end() attaches
  its handlers no longer crashes the process or leaves the handle open forever.

Verified on 22MB, 99MB and 1.3GB WSI instances (1292, 1586 and 24617 frames): no
open file count warnings, no temp files left behind, and for the 22MB instance
output byte-identical to before apart from the random multipart boundaries, with
unchanged runtime.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@wayfarer3130
wayfarer3130 requested a review from rleisti August 4, 2026 15:58
@wayfarer3130

Copy link
Copy Markdown
Collaborator Author

@rleisti - I found this while ingesting a really large study for testing.

@wayfarer3130

Copy link
Copy Markdown
Collaborator Author

@awatson1978 - can you review this - it fixes the backpressure check on converting large WSI files using createdicomweb command - test it with the WSI files I sent you and:
createdicomweb create WSI-DIR
Note it takes a really long time to convert, so you might want to try it on just the big file and not on everything.

@wayfarer3130

Copy link
Copy Markdown
Collaborator Author

Clod comments are:
Overall the design holds up. awaitOpenStreamLimit races every non-ended stream plus a removal signal, so it cannot deadlock against the frame stream its own caller is holding open; _deleteOpenStream is genuinely the only removal path (verified by grep); StreamInfo's new error handlers plus the completion-promise .catch remove a real unhandledRejection crash; and drainOpenStreams replaces an awaitAllStreams() that would have hung forever on a never-closed stream. I confirmed via the installed dcmjs build that listener.awaitDrain() is actually invoked (_emitSplitValues, three call sites), so the gate is wired to something real. All 59 create-dicomweb tests and the webserver suite pass.

The issues are concentrated on the failure paths the new code introduces:

packages/create-dicomweb/lib/instance/DicomWebWriter.mjs:763 — drain's closeStream catch only warns; _moveTempToFinal deletes the temp file and rethrows on a failed rename, so a lost frame is reported as a successful STOW store. The lingering branch 10 lines below does call recordStreamError.
packages/create-dicomweb/lib/instance/instanceFromStream.mjs:364 — the "fails the instance" guarantee only holds for STOW; createMain/instanceMain never check hasStreamErrors(), so a stalled drain yields exit 0, a full study index, and missing frames.
packages/create-dicomweb/lib/instance/DicomWebWriter.mjs:693 — the empty-map early return skips _onAllStreamsDrained(); since recordStreamError/abort empty the map without finalizing, a multipart response can be left unended.
packages/create-dicomweb/lib/instance/MultipartResponseDicomWebWriter.mjs:121 — the old error path always called response.end(); the refactor rejects instead, and _responseFinalized blocks any retry.
packages/create-dicomweb/lib/instance/DicomWebWriter.mjs:126 — --max-open-files 1..3 is silently clamped to 4 while the new README tells users to lower it.

Given that, merging this change

@wayfarer3130
wayfarer3130 merged commit 54192d3 into master Aug 11, 2026
2 checks passed
@wayfarer3130
wayfarer3130 deleted the fix/wsi-open-file-backpressure branch August 11, 2026 19:10
@awatson1978

Copy link
Copy Markdown
Collaborator

This fix was verified against a real whole-slide study, extending the instance sizes in the PR description (22 MB, 99 MB, 1.3 GB / 24,617 frames) by an order of magnitude: a 5 GB pyramid from the CMB-MML collection with 394,830 frames, run through mkdicomweb create.

The back pressure engages, at scale. The description's original symptom — "open file count exceeded 500 ... climbing from there" — never appeared: the run produced zero StreamInfo warnings across all 394,830 frames. A file-handle monitor sampling the process every 2 seconds never saw more than 7 files open at once, comfortably inside the default maxOpenStreams of 32. So the tracker is genuinely wired to the writer now — with one file per frame and no working limit, this run would have needed hundreds of thousands of handles.

The cleanup paths hold too. No tempFile-* files were left behind in the output tree, and all frames closed out (the full extraction completed with exit 0), matching the "no temp files, everything closed" behavior the description verified at smaller sizes.

One caveat about the test file, which surfaced a separate bug. The 5 GB pyramid (like 3 of its 4 sibling files) carries an Extended Offset Table, whose value type (OV) dicom-parser 1.8.21 doesn't recognize — its long-VR list has no OV, so parsing misreads the element. Three siblings fail loudly with "buffer overrun"; the 5 GB file fails silently — metadata written, zero frames extracted, no error. The verification above therefore used a copy with the offset table spliced out (which converts perfectly and proves the diagnosis). Worth an upstream issue on dicom-parser; the silent-failure mode may also be worth a guard here.

One caution on memory: converting the 5 GB instance peaked at about 4.5 GB of memory. Handles are now bounded, but memory still scales with the size of the whole instance.

@awatson1978

Copy link
Copy Markdown
Collaborator

5 GB WSI verification — commands used

Testing Static-DICOMWeb PR #129 against a real 5 GB / 394,830-frame slide. Source data per dicom-large-data-test-sources.md.

1. Get the study (TCIA CMB-MML, CC BY 4.0)

python -m pip install --upgrade idc-index
idc download 1.3.6.1.4.1.5962.99.1.1152570677.393312001.1714844554549.4.0 \
  --download-dir ./cmb-mml-wsi

Five instances, ~6.1 GB; the largest (17a26857….dcm) is 5,012,980,492 bytes.

2. Build the converter (master with #129 merged)

PR #129's source branch is fix/wsi-open-file-backpressure (head e897f2e); it was squash-merged into master as 54192d3. Test that merged commit via a worktree of the existing Static-DICOMWeb checkout:

cd Static-DICOMWeb
git worktree add ../static-dicomweb-verify-129 54192d3 --detach
cd ../static-dicomweb-verify-129 && bun install

3. Convert the study, monitoring handles and memory

ulimit -Sn 10240
/usr/bin/time -l bun packages/static-wado-creator/bin/mkdicomweb.mjs create \
  -o ../large-files/dicomweb-wsi-verify ../large-files/cmb-mml-wsi \
  > wsi-verify-run.log 2>&1

In a second shell, sample open fds + RSS:

while PID=$(pgrep -f "mkdicomweb.mjs create" | head -1); [ -n "$PID" ]; do
  echo "$(date +%H:%M:%S) fds=$(lsof -p $PID | wc -l) rss_kb=$(ps -o rss= -p $PID)"
  sleep 20
done >> wsi-verify-monitor.log

Result: peak 7–9 fds, zero StreamInfo warnings — but 3 of 5 files fail with buffer overrun, and the 5 GB file writes metadata with zero frames (silent). Caution: lsof polling can interrupt an open() with EINTR, which Bun doesn't retry — poll sparingly or monitor RSS only.

4. Diagnose: parse each file directly with dicom-parser 1.8.21

// direct-parse-test.mjs
import fs from 'fs';
import dicomParser from 'dicom-parser';
try { dicomParser.parseDicom(fs.readFileSync(process.argv[2])); console.log('PARSE OK'); }
catch (e) { console.log('PARSE FAILED:', e.exception || e); }

The 4 files containing an Extended Offset Table (7FE0,0001, VR OV) fail; the 1 without it parses. Root cause: OV missing from dicom-parser's long-VR list (see dicom-parser-ov-issue.md).

5. Splice the EOT out of the 5 GB file

Locate the element boundaries, then copy everything except bytes [EOT_START, EOT_END):

# find offsets: EOT tag = e0 7f 01 00 + 'OV'; PixelData = e0 7f 10 00
data = open(src,'rb').read(16*1024*1024)
EOT_START = data.find(b'\xe0\x7f\x01\x00OV')          # 1694034
EOT_END   = data.find(b'\xe0\x7f\x10\x00')             # 8011338 (PixelData start)

# splice (streamed, 64 MiB chunks); assert the boundary bytes first
with open(src,'rb') as f, open('wsi-5gb-no-eot.dcm','wb') as o:
    ...  # copy [0, EOT_START), skip to EOT_END, copy rest

6. Convert the spliced file

/usr/bin/time -l bun packages/static-wado-creator/bin/mkdicomweb.mjs create \
  -o ../large-files/dicomweb-wsi-verify-noeot ../large-files/wsi-5gb-no-eot.dcm

Result: exit 0, all 394,830 frames extracted, peak RSS ~4.5 GB.

7. Verify

grep -c 'Create failure' wsi-verify-noeot-run.log            # 0
find dicomweb-wsi-verify-noeot -name 'tempFile-*' | wc -l    # 0 (no leaked streams)
find dicomweb-wsi-verify-noeot -type f | wc -l               # 394,847

Optional: pre-#129 baseline

git worktree add ../static-dicomweb-pre129 b4930ab --detach
cd ../static-dicomweb-pre129 && bun install
# rerun step 3 to observe the "open file count exceeded" warnings the fix removes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants