fix: normalize model download paths + handle download interupts#1655
fix: normalize model download paths + handle download interupts#1655aminya wants to merge 9 commits intoComfy-Org:mainfrom
Conversation
Electron can emit an updated event with state === 'interrupted' when the download is paused by the network (e.g. connection drop, timeout), not by the user. In that case the download can be resumed.
dosubot
bot
added
size:S
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds auto-resume for interrupted downloads with per-download Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI
participant DM as DownloadManager
participant NET as Network
participant FS as Filesystem
UI->>DM: startDownload(request with savePath)
DM->>DM: resolveSavePath -> create Download entry (localSavePath, tempPath, item=null, interruptResumeCount=0)
DM->>NET: begin transfer
NET-->>DM: interruption/error
DM->>DM: compute progress, map INTERRUPTED->PAUSED, set message (auto-resume eligible?)
DM->>UI: report PAUSED + progress + message
alt eligible to auto-resume
DM->>DM: schedule resume after 500ms
DM->>DM: increment interruptResumeCount
DM->>NET: resume transfer (verify item still interrupted & URL matches)
NET-->>DM: transfer continues/completes
DM->>FS: write temp/local files using resolved savePath
DM->>UI: report progress/completion
else not eligible or error
DM->>DM: delete download entry from internal map
DM->>UI: report final error/state
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/DownloadManager.ts`:
- Around line 72-79: The reportProgress call in DownloadManager
(reportProgress({..., status: DownloadStatus.PAUSED, message: 'Interrupted,
resuming…' })) is inconsistent with getAllDownloads() which currently normalizes
an 'interrupted' persisted status to DownloadStatus.ERROR; update the
normalization in getAllDownloads() so that when it encounters the persisted
'interrupted' state it maps to DownloadStatus.PAUSED (or the agreed
Interrupt/Paused enum value) instead of ERROR, ensuring both reportProgress and
getAllDownloads() use the same DownloadStatus for interrupted transfers.
- Around line 81-85: The timeout callback closes over the stale local variable
`download`, allowing a canceled or restarted download to be erroneously resumed;
change the guard inside the setTimeout in DownloadManager so it fetches the live
entry from the manager's map (e.g., look up the current download record by the
same key/id used in `cancelDownload`) and compare that liveEntry.item to `item`
and check `item.getState() === 'interrupted'` before calling `item.resume()`;
update the callback to use the live map lookup instead of `download.item ===
item`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34f689a6-c1f5-4ccd-bc0f-5fd6dab99b4a
📒 Files selected for processing (1)
src/models/DownloadManager.ts
dosubot
bot
added
size:M
aminya
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/DownloadManager.ts (1)
138-147:⚠️ Potential issue | 🟠 MajorGuard stale
donecallbacks before reporting failure.After
cancelDownload()removes the old entry, the same URL can be started again before the cancelled item'sdonecallback fires. This branch would then emitDownloadStatus.ERRORfor the stale item, anddelete(url)would remove the new live download. Re-check the current map entry againstitembefore reporting or deleting.Suggested fix
} else { + const liveEntry = this.downloads.get(url); + if (liveEntry?.item !== item) { + return; + } log.info(`Download failed: ${state}`); const progress = item.getReceivedBytes() / item.getTotalBytes(); this.reportProgress({ url, filename: download.filename, progress, status: DownloadStatus.ERROR, savePath: download.savePath, }); this.downloads.delete(url); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 138 - 147, Race condition: a stale item's done callback can report ERROR and delete a new download for the same URL. Before calling this.reportProgress(...) or this.downloads.delete(url) in the done/error branch, re-check that this.downloads.get(url) === item and abort if it differs; only emit DownloadStatus.ERROR and remove the entry when the current map value is the same item instance. Use strict equality on the stored entry (this.downloads.get(url)) to guard both the reportProgress call and the delete call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/DownloadManager.ts`:
- Around line 198-205: The downloads map is storing savePath as the resolved
file path (localSavePath) but resumeDownload() expects a directory/path segment
when it calls startDownload(); this causes resumed downloads to restart into the
wrong destination. Change the download record to keep the original directory
(e.g., saveDir or directoryPath) as savePath and keep the resolved file path
separately (e.g., fullPath or filename+tempPath) so resumeDownload() can pass
the directory into startDownload(); update the object created in the
downloads.set(...) (and any uses of download.savePath in resumeDownload(),
startDownload(), and canResume()) to use the directory field when restarting and
the full/resolved path when performing file-level operations.
---
Outside diff comments:
In `@src/models/DownloadManager.ts`:
- Around line 138-147: Race condition: a stale item's done callback can report
ERROR and delete a new download for the same URL. Before calling
this.reportProgress(...) or this.downloads.delete(url) in the done/error branch,
re-check that this.downloads.get(url) === item and abort if it differs; only
emit DownloadStatus.ERROR and remove the entry when the current map value is the
same item instance. Use strict equality on the stored entry
(this.downloads.get(url)) to guard both the reportProgress call and the delete
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bec166c9-7ef2-4b6f-bed5-2518e99e267b
📒 Files selected for processing (1)
src/models/DownloadManager.ts
aminya
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/DownloadManager.ts`:
- Around line 329-337: The path containment check using startsWith is unsafe and
can be bypassed; update resolveSavePath and isPathInModelsDirectory to compute
const rel = path.relative(this.modelsDirectory, resolved) and then treat the
path as inside only if rel === '' || (!rel.startsWith('..' + path.sep) && rel
!== '..' && !path.isAbsolute(rel)); use that boolean to decide behavior in
resolveSavePath (compute and return path.dirname(rel) or rel when inside) and in
isPathInModelsDirectory (return the boolean), which will secure getLocalSavePath
and getTempPath against sibling-prefix escapes for this.modelsDirectory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3270ed0-e116-43aa-a657-dc1aca63b1c8
📒 Files selected for processing (1)
src/models/DownloadManager.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/models/DownloadManager.ts (1)
322-330:⚠️ Potential issue | 🔴 Critical
startsWithcheck can be bypassed; usepath.relative()for safe containment.The
startsWithcomparison at line 325 (and line 340 inisPathInModelsDirectory) treats sibling directories as valid children. For example,/models_evil/foo.safetensorsstarts with/modelsbut is not inside it.Suggested fix using path.relative()
private resolveSavePath(savePath: string, filename: string): string { const base = path.resolve(this.modelsDirectory); const resolved = path.resolve(savePath); - if (resolved.startsWith(base)) { - const rel = path.relative(base, resolved); + const rel = path.relative(base, resolved); + const isInside = rel !== '' && rel !== '..' && !rel.startsWith(`..${path.sep}`) && !path.isAbsolute(rel); + if (isInside) { return rel.endsWith(filename) ? path.dirname(rel) : rel; } return savePath; } private isPathInModelsDirectory(filePath: string): boolean { const absoluteFilePath = path.resolve(filePath); const absoluteModelsDir = path.resolve(this.modelsDirectory); - return absoluteFilePath.startsWith(absoluteModelsDir); + const rel = path.relative(absoluteModelsDir, absoluteFilePath); + return rel !== '..' && !rel.startsWith(`..${path.sep}`) && !path.isAbsolute(rel); }Based on learnings: "Use
path.join()andpath.septo ensure file path handling works consistently across Windows, macOS, and Linux platforms."Also applies to: 337-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 322 - 330, The current startsWith-based containment checks in resolveSavePath and isPathInModelsDirectory are unsafe (e.g., "/models_evil" passes) — replace them with path.relative-based containment: compute const rel = path.relative(base, resolved) after resolving both base (this.modelsDirectory) and the target path, then treat the path as inside base only if rel is not empty, does not start with '..' and is not equal to '..' (use path.sep awareness for Windows), and normalize/trim separators as needed; update resolveSavePath to use that containment test before returning a relative path and update isPathInModelsDirectory to use the same path.relative-based logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/models/DownloadManager.ts`:
- Line 98: The progress calculation uses item.getReceivedBytes() /
item.getTotalBytes() without guarding against zero total; update both
occurrences where progress is computed (the expression using
item.getReceivedBytes() and item.getTotalBytes()) to check totalBytes > 0 first
(e.g., const total = item.getTotalBytes(); const progress = total > 0 ?
item.getReceivedBytes() / total : 0) so Infinity/NaN are never produced and the
UI always receives a valid number.
- Around line 87-96: The auto-resume scheduling in DownloadManager increments
interruptResumeCount only inside the setTimeout callback which allows multiple
timeouts to be scheduled and exceed MAX_AUTO_RESUME_ATTEMPTS; modify the
scheduling logic in the block where you check item.canResume() and
autoResumesLeft so you atomically reserve an auto-resume slot before calling
setTimeout — e.g., locate the entry via this.downloads.get(url), immediately
increment entry.interruptResumeCount (or set a boolean like
entry.autoResumeScheduled) and re-check autoResumesLeft, then schedule the
timeout that will call item.resume() only if entry.item === item and
item.getState() === 'interrupted'; ensure you decrement/clear the scheduled flag
only inside the callback so duplicate timeouts cannot be created.
---
Duplicate comments:
In `@src/models/DownloadManager.ts`:
- Around line 322-330: The current startsWith-based containment checks in
resolveSavePath and isPathInModelsDirectory are unsafe (e.g., "/models_evil"
passes) — replace them with path.relative-based containment: compute const rel =
path.relative(base, resolved) after resolving both base (this.modelsDirectory)
and the target path, then treat the path as inside base only if rel is not
empty, does not start with '..' and is not equal to '..' (use path.sep awareness
for Windows), and normalize/trim separators as needed; update resolveSavePath to
use that containment test before returning a relative path and update
isPathInModelsDirectory to use the same path.relative-based logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db230c03-ae3d-4e04-b1af-923c59d0e61f
📒 Files selected for processing (1)
src/models/DownloadManager.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/models/DownloadManager.ts (3)
139-139:⚠️ Potential issue | 🟡 MinorDivision by zero not guarded in done handler error path.
Same issue as line 98—apply the guard here as well.
Suggested fix
} else { log.info(`Download failed: ${state}`); - const progress = item.getReceivedBytes() / item.getTotalBytes(); + const totalBytes = item.getTotalBytes(); + const progress = totalBytes > 0 ? item.getReceivedBytes() / totalBytes : 0; this.reportProgress({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` at line 139, In the DownloadManager done handler compute of progress (const progress = item.getReceivedBytes() / item.getTotalBytes();) you must guard against totalBytes being zero to avoid division-by-zero; update the done handler in the DownloadManager (same pattern used at line 98) to check item.getTotalBytes() > 0 before dividing and fallback to a safe value (e.g., 0 or 1 as appropriate) when total is zero, ensuring progress is always a finite number.
98-98:⚠️ Potential issue | 🟡 MinorDivision by zero not guarded in progressing state.
Line 75 correctly guards against zero
totalBytes, but line 98 does not. IfgetTotalBytes()returns 0 (e.g., server didn't send Content-Length), this producesInfinityorNaN.Suggested fix
} else if (state === 'progressing') { - const progress = item.getReceivedBytes() / item.getTotalBytes(); + const totalBytes = item.getTotalBytes(); + const progress = totalBytes > 0 ? item.getReceivedBytes() / totalBytes : 0; if (item.isPaused()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` at line 98, The progress calculation uses item.getReceivedBytes() / item.getTotalBytes() without guarding against a zero total, causing Infinity/NaN; update the logic in the DownloadManager class/method where progress is computed (the line that assigns to progress) to check item.getTotalBytes() > 0 first and fall back to 0 (or the same safe behavior used earlier at line 75) when totalBytes is zero or undefined so progress never becomes Infinity/NaN.
87-96:⚠️ Potential issue | 🟡 MinorRace condition: multiple auto-resume timeouts can be scheduled.
If multiple
'interrupted'events fire within 500ms, each schedules asetTimeoutbeforeinterruptResumeCountis incremented (since increment happens inside the callback). This can exceedMAX_AUTO_RESUME_ATTEMPTS.Increment the count immediately when scheduling:
Suggested fix
if (item.canResume() && autoResumesLeft > 0) { + // Increment immediately to prevent multiple scheduled resumes + if (liveEntry) { + liveEntry.interruptResumeCount = (liveEntry.interruptResumeCount ?? 0) + 1; + } setTimeout(() => { const entry = this.downloads.get(url); if (entry?.item === item && item.getState() === 'interrupted') { - entry.interruptResumeCount = (entry.interruptResumeCount ?? 0) + 1; log.info('Auto-resuming interrupted download'); item.resume(); } }, 500); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/DownloadManager.ts` around lines 87 - 96, Race condition: multiple timeouts can be scheduled because interruptResumeCount is incremented only inside the setTimeout; increment the counter immediately when scheduling to prevent exceeding MAX_AUTO_RESUME_ATTEMPTS. Locate the block using item.canResume(), autoResumesLeft and this.downloads.get(url), and change the logic so you read the map entry, compute and increment entry.interruptResumeCount (entry.interruptResumeCount = (entry.interruptResumeCount ?? 0) + 1) before calling setTimeout, and only schedule the timeout if the new count is <= MAX_AUTO_RESUME_ATTEMPTS (or autoResumesLeft > 0); keep the existing inside-timeout checks (entry?.item === item && item.getState() === 'interrupted') but do not increment there again.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/models/DownloadManager.ts`:
- Line 139: In the DownloadManager done handler compute of progress (const
progress = item.getReceivedBytes() / item.getTotalBytes();) you must guard
against totalBytes being zero to avoid division-by-zero; update the done handler
in the DownloadManager (same pattern used at line 98) to check
item.getTotalBytes() > 0 before dividing and fallback to a safe value (e.g., 0
or 1 as appropriate) when total is zero, ensuring progress is always a finite
number.
- Line 98: The progress calculation uses item.getReceivedBytes() /
item.getTotalBytes() without guarding against a zero total, causing
Infinity/NaN; update the logic in the DownloadManager class/method where
progress is computed (the line that assigns to progress) to check
item.getTotalBytes() > 0 first and fall back to 0 (or the same safe behavior
used earlier at line 75) when totalBytes is zero or undefined so progress never
becomes Infinity/NaN.
- Around line 87-96: Race condition: multiple timeouts can be scheduled because
interruptResumeCount is incremented only inside the setTimeout; increment the
counter immediately when scheduling to prevent exceeding
MAX_AUTO_RESUME_ATTEMPTS. Locate the block using item.canResume(),
autoResumesLeft and this.downloads.get(url), and change the logic so you read
the map entry, compute and increment entry.interruptResumeCount
(entry.interruptResumeCount = (entry.interruptResumeCount ?? 0) + 1) before
calling setTimeout, and only schedule the timeout if the new count is <=
MAX_AUTO_RESUME_ATTEMPTS (or autoResumesLeft > 0); keep the existing
inside-timeout checks (entry?.item === item && item.getState() ===
'interrupted') but do not increment there again.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 568bfb63-d4f2-4ce3-a3d9-499baa2c38c8
📒 Files selected for processing (1)
src/models/DownloadManager.ts
|
Hello, Is there a pre-release version that addresses this issue? Thanks! Regards! |
Not yet until this is merged. Meanwhile, the only solution would be to build the code yourself, which is quite a task. |
|
Hi @aminya, thank you so much for this contribution! I wanted to apologize—I actually missed this PR while working on a fix for the same bug recently. Since I didn't see this was already in progress, I ended up implementing a separate fix that was merged a few days ago. That said, I see this PR includes great improvements for download recovery/resuming that weren't part of my change. We’d still love to get those features in! If you're interested in moving forward with the resuming logic, would you mind resolving the merge conflicts so we can take another look at those specific additions? |
@benceruleanlu Hi, you are welcome! That's totally fine. I have resolved the conflicts via a merge to preserve the review history. |
3 participants
The model paths had to be normalized for the downloads. This fixes the download issue. But I also added better handling of interrupts in case of external issues.
An Electron event can be emitted with state === 'interrupted' when the download is paused due to an issue or network condition (e.g., a connection drop or timeout), not by the user. In that case, the download can be resumed.
Fixes Comfy-Org/ComfyUI#12929
Now the downloads work:
In case of a network interruption, the status of the download is now shown correctly when paused
┆Issue is synchronized with this Notion page by Unito
Summary by CodeRabbit
New Features
Bug Fixes