fix: deduplicate concurrent downloads in resolveModelFile#570
Open
graciegould wants to merge 2 commits intowithcatai:masterfrom
Open
fix: deduplicate concurrent downloads in resolveModelFile#570graciegould wants to merge 2 commits intowithcatai:masterfrom
graciegould wants to merge 2 commits intowithcatai:masterfrom
Conversation
When multiple concurrent calls to resolveModelFile target the same model, each call independently starts a download via ipull. Multiple ipull instances writing to the same .ipull temp file causes orphaned FileHandle objects when the first download completes and renames the file. On Node 22+, garbage-collected FileHandles are a fatal error. Add a module-level Map to track in-flight downloads by entrypoint file path. When a concurrent call detects an existing download for the same file, it cancels its own downloader and awaits the existing promise instead of starting a duplicate download. Closes withcatai#569
5 tasks
giladgd
reviewed
Mar 6, 2026
src/utils/resolveModelFile.ts
Outdated
| ? chalk.gray(` (combining ${downloader.splitBinaryParts} parts into a single file)`) | ||
| : "" | ||
| }`); | ||
| const downloadKey = downloader.entrypointFilePath; |
Member
There was a problem hiding this comment.
The issue with this approach is that the first call to resolveModelFile may pass a signal and cancel it and thus cause other downloads to be cancelled as well.
I think we can take a different approach by doing something like this:
import {acquireLock} from "lifecycle-utils";
...
const downloader = await createModelDownloader({
...
});
const lock = await acquireLock([resolveModelFile, "download", downloader.entrypointFilePath]);
try {
if (await fs.pathExists(downloader.entrypointFilePath)) {
await downloader.cancel({deleteTempFile: false});
return downloader.entrypointFilePath;
}
...
return downloader.entrypointFilePath;
} finally {
lock.dispose();
}In this case if the first download gets cancelled, the next call to resolveModelFile will redownload (and use the headers, tokens and other parameters passed to that resolveModelFile call).
Author
There was a problem hiding this comment.
Good call! I will update this.
5334f6b to
b5c177b
Compare
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.
Description of change
Current behavior: Concurrent
resolveModelFilecalls each start separate ipull downloads to the same.ipulltemp file. When the first completes and renames the file, the other instances' file handles become orphaned. On Node 22+, garbage-collectedFileHandleobjects are a fatal error that crashes the process.New behavior: A module-level
Maptracks in-flight downloads by entrypoint path. Concurrent calls detect the existing download, cancel their own downloader, and await the shared promise.Fixes #569
Pull-Request Checklist
masterbranchnpm run formatto apply eslint formattingnpm run testpasses with this changeFixes #569