You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixes EMFILE: too many open files when reading blobs that are spread across many extents (issue #1967).
FSExtentStore.readExtents previously opened a read stream — and therefore a file handle — for every extent up front before handing the array to multistream. For a blob composed of many extents (e.g. a block blob with many committed blocks, or a large append blob), this opened one file descriptor per extent simultaneously and could exceed the OS file-handle limit, failing the download with EMFILE.
As discussed on the original PR #2267, the maintainer's suggested fix was to open each extent only when it is about to be read and close it before opening the next. This PR does exactly that.
How
readExtents now:
Resolves which extent sub-chunks to read (pure offset/count arithmetic — no file handles opened), keeping the existing RangeError validation unchanged.
Hands multistream a lazy FactoryStream. multistream invokes the factory from its constructor, so the first extent begins opening as the merged stream is constructed; every subsequent extent is opened only after the previous stream has emitted close (its file descriptor is released). The result is at most one extent file descriptor open at a time, regardless of blob size.
If the merged stream is destroyed (a programmatic teardown) while an extent is still opening, the freshly opened extent is destroyed instead of being installed, so its descriptor is not leaked.
Because the fix lives in the shared FSExtentStore, it also benefits queue and table reads, without any per-request stream bookkeeping or interface changes.
Scope / known follow-up
This PR eliminates the up-front N-descriptor spike and the normal-completion leak from #1967. It does not fully cover a raw client disconnect: the response pipe (serializer.ts) only unpipes the body on disconnect rather than destroying it, so the current extent's descriptor can still be held. That reduces the pre-existing disconnect leak from N descriptors to 1, and destroying the body stream on response close/abort is tracked as a focused follow-up in #2804.
Tests
Coverage lives in tests/blob/fsStore.test.ts, exercising FSExtentStore directly:
merges multiple extents into a single readable stream (full read), and a range that spans multiple extent boundaries;
asserts serial extent creation — only the first extent may begin opening before consumption, subsequent extents are deferred, and at most one extent stream is active at a time (the active-stream counter is decremented only on close/error, so it fails if the next extent opens before the previous descriptor is released);
asserts, deterministically via deferred signals, that an extent opened after the merged stream is destroyed mid-open is itself destroyed rather than leaked.
Each new assertion was verified to fail against the previous behaviour. Existing blockblob and appendblob suites also pass, and the full tsc build and eslint pass.
Notes
Supersedes Adding initial fix for bug issue #1967 #2267, which tracked/destroyed streams on res.on("close"); that approach didn't compile against the current interfaces, leaked its tracking Map, and didn't prevent a single large download from exhausting FDs up front. This PR addresses the root cause instead.
FSExtentStore.readExtents opened a read stream (and thus a file handle)
for every extent up front, so downloading a blob spread across many
extents could exceed the OS file-handle limit and fail with EMFILE.
It now hands multistream a lazy factory that opens each extent's read
stream only after the previous one has ended, keeping at most one extent
file handle open at a time regardless of blob size. This also covers
queue and table reads, which share FSExtentStore.
Adds a regression test that downloads a blob spanning many extents (full
blob and a cross-extent-boundary range) and asserts byte-exact content.
Supersedes #2267.
An fs.ReadStream releases its file descriptor asynchronously, emitting
"close" after "end". multistream advances on "end", so the previous
implementation could open the next extent before the prior descriptor
was released, briefly holding two handles and still risking EMFILE near
the descriptor limit (issue #1967).
readExtents now waits for the previous extent stream's "close" before
opening the next, guaranteeing at most one extent file descriptor is
open at a time. The fsStore regression test now decrements its active
stream counter only on "close"/"error" (not "end"), so it fails if the
next extent opens before the previous one is closed.
If the merged stream is destroyed (e.g. the client aborts) while the
factory is waiting for the previous extent's close or for readExtent to
resolve, multistream's _destroy only tears down its current stream. An
extent opened after that point was handed back but never consumed or
destroyed, leaking its file descriptor.
The factory now checks whether the merged stream has been destroyed once
readExtent resolves and, if so, destroys the freshly opened extent
instead of installing it. Adds an abort-while-opening regression test
that fails against the previous behaviour.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The abort test is timing-dependent, and the claimed first-read laziness does not match actual multistream behavior.
Review effort: Balanced Findings: None
Previously missed (2)
In code that hasn't changed since last review
Fixed delay races extent stream close event
tests/blob/fsStore.test.ts:185
This fixed delay does not synchronize with the extent stream's close event. If the event loop is stalled long enough for both timers to expire in one timers phase, this assertion can run before close callbacks and fail intermittently. Await a promise resolved by the opened stream's close event, and use a deferred signal to ensure opening has started before destroying the merged stream.
First extent opens before merged stream consumption
src/common/persistence/FSExtentStore.ts:491
multistream@4.1.0 invokes the factory from its constructor, so constructing it here starts opening the first extent before the returned stream is consumed (the test's extentReadCalls <= 1 also permits this). This contradicts the PR description's claim that the first extent is not opened until consumption. Either update the description to clarify that only subsequent extents are deferred, or add an outer lazy readable if zero pre-consumption opens is required.
The abort-while-opening test relied on fixed setTimeout delays that did
not synchronize with the extent stream's close event and could assert
before the close callback ran. It now uses deferred signals: it waits
until the first extent has started opening, destroys the merged stream,
releases the open, then awaits the opened stream's close before
asserting no descriptor leaked.
Also reworded the merge test's first-read assertion: multistream invokes
the factory from its constructor, so the first extent may begin opening
before consumption; only subsequent extents are deferred.
Thanks — both "previously missed" findings are addressed in 26987b3:
Abort test timing dependence (fsStore.test.ts): the test no longer uses fixed setTimeout delays. It uses deferred signals — it waits until the first extent has started opening, destroys the merged stream, releases the open, then awaits the opened stream's close event before asserting no descriptor leaked. Ran deterministically green across repeated runs.
First-extent laziness (FSExtentStore.ts): correct — multistream@4.1.0 invokes the factory from its constructor, so the first extent begins opening at construction, not on first read. Only subsequent extents are deferred. The EMFILE guarantee (at most one extent descriptor open at a time) is unaffected, so rather than adding an outer lazy readable I updated the PR description and the test assertion wording to state this accurately.
The abort guard releases an extent opened after the merged stream is
destroyed. Reworded the comment (and the test) to make clear this covers
a programmatic teardown; a raw HTTP client disconnect does not currently
destroy the merged stream (the response pipe only unpipes it), which is
tracked separately in issue #2804.
Advance occurs before failed stream closes, allowing descriptor overlap
src/common/persistence/FSExtentStore.ts:486
Resolving this gate on error can open the next extent before the failed fs.ReadStream has emitted close and released its descriptor. multistream 4.1.0 destroys the merged stream synchronously on a source error, but this promise is already resolved, so the next readExtent can still run during the error-to-close window and violate the one-descriptor guarantee. Wait only for close; the default fs.ReadStream auto-close path emits it after errors too.
Test stops tracking descriptor activity on error instead of close
tests/blob/fsStore.test.ts:111
Deactivating on error makes this regression test stop counting the descriptor before it is actually closed, so it can miss the error-to-close overlap that the production code currently permits. Track activity until close only, matching the test's own stated invariant that descriptor release is what matters.
Address the error-to-close descriptor overlap: the close gate no longer
resolves on "error". fs.ReadStream auto-close emits "close" after an
error too, so waiting on "close" alone still covers the error path
without opening the next extent during the error-to-close window. The
merge test likewise tracks descriptor activity until "close" only.
Also simplify the abort regression test: it now gates the first open,
destroys the merged stream, releases the open, and awaits the opened
stream's close, then asserts that stream was destroyed - no manual
active-stream counters or extra signals.
Addressed the two "previously missed" findings from the latest review in 232ca08:
Advance occurs before failed stream closes (FSExtentStore.ts) — the close gate no longer resolves on error. It waits for close only; fs.ReadStream auto-close emits close after an error as well, so the error path is still covered without opening the next extent during the error-to-close window. (If the current stream errors, multistream destroys the merged stream and the factory is not called again, so there is no hang.)
Test stops tracking on error instead of close (fsStore.test.ts) — the merge test now decrements the active-descriptor counter only on close, matching the invariant that descriptor release is what matters.
Separately, I simplified the abort regression test per reviewer/maintainer feedback: it now gates the first open, destroys the merged stream, releases the open, awaits the opened stream's close, and asserts that stream was destroyed — no manual active-stream counters or extra deferred signals. Build, lint, and the fsStore suite (including the multi-extent merge and ranged reads) pass; the abort test is deterministic across repeated runs.
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
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.
What & why
Fixes
EMFILE: too many open fileswhen reading blobs that are spread across many extents (issue #1967).FSExtentStore.readExtentspreviously opened a read stream — and therefore a file handle — for every extent up front before handing the array tomultistream. For a blob composed of many extents (e.g. a block blob with many committed blocks, or a large append blob), this opened one file descriptor per extent simultaneously and could exceed the OS file-handle limit, failing the download withEMFILE.As discussed on the original PR #2267, the maintainer's suggested fix was to open each extent only when it is about to be read and close it before opening the next. This PR does exactly that.
How
readExtentsnow:RangeErrorvalidation unchanged.multistreama lazyFactoryStream.multistreaminvokes the factory from its constructor, so the first extent begins opening as the merged stream is constructed; every subsequent extent is opened only after the previous stream has emittedclose(its file descriptor is released). The result is at most one extent file descriptor open at a time, regardless of blob size.Because the fix lives in the shared
FSExtentStore, it also benefits queue and table reads, without any per-request stream bookkeeping or interface changes.Scope / known follow-up
This PR eliminates the up-front
N-descriptor spike and the normal-completion leak from #1967. It does not fully cover a raw client disconnect: the response pipe (serializer.ts) only unpipes the body on disconnect rather than destroying it, so the current extent's descriptor can still be held. That reduces the pre-existing disconnect leak fromNdescriptors to1, and destroying the body stream on response close/abort is tracked as a focused follow-up in #2804.Tests
Coverage lives in
tests/blob/fsStore.test.ts, exercisingFSExtentStoredirectly:close/error, so it fails if the next extent opens before the previous descriptor is released);Each new assertion was verified to fail against the previous behaviour. Existing
blockblobandappendblobsuites also pass, and the fulltscbuild andeslintpass.Notes
res.on("close"); that approach didn't compile against the current interfaces, leaked its trackingMap, and didn't prevent a single large download from exhausting FDs up front. This PR addresses the root cause instead.Closes #1967