feat: apply worker size heuristic to resumed download - #512
Conversation
…maining file size during resume
…cancellation instead of re-queueing failed tasks
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| if lastErr != nil { | ||
| // Log failed task but continue with next task | ||
| // If we modified StopAt we should probably reset it or push the remaining part? | ||
| // TODO: Could optimize by pushing only remaining part if we track that. | ||
| queue.Push(task) | ||
| utils.Debug("task at offset %d failed after %d retries: %v", task.Offset, maxRetries, lastErr) | ||
| utils.Debug("Worker %d: task at offset %d failed after %d retries: %v", id, task.Offset, maxRetries, lastErr) | ||
| return lastErr | ||
| } |
There was a problem hiding this comment.
The change from re-queuing failed tasks to returning the error (and cancelling all workers) is the most significant behavioral shift in this PR. Previously a task that exhausted all retries was silently pushed back to the queue; now it causes the entire download to abort. There are no tests covering this new code path — no test verifies that a worker error surfaces in the returned error from executeWorkers, cancels the context, and terminates sibling workers. Per the project's testing rule, edge cases and integration points between components should have explicit coverage.
Rule Used: What: All code changes must include tests for edge... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/engine/concurrent/worker.go
Line: 173-178
Comment:
**Missing test for fail-fast worker behavior**
The change from re-queuing failed tasks to returning the error (and cancelling all workers) is the most significant behavioral shift in this PR. Previously a task that exhausted all retries was silently pushed back to the queue; now it causes the entire download to abort. There are no tests covering this new code path — no test verifies that a worker error surfaces in the returned `error` from `executeWorkers`, cancels the context, and terminates sibling workers. Per the project's testing rule, edge cases and integration points between components should have explicit coverage.
**Rule Used:** What: All code changes must include tests for edge... ([source](https://app.greptile.com/surge-org-3/-/custom-context?memory=2b22782d-3452-4d55-b059-e631b2540ce8))
How can I resolve this? If you propose a fix, please make it concise.| @@ -285,7 +291,7 @@ func (d *ConcurrentDownloader) Download(ctx context.Context, rawurl string, cand | |||
| d.State.InitBitmap(fileSize, chunkSize) | |||
| } | |||
|
|
|||
| tasks, err := d.setupTasks(destPath, fileSize, chunkSize, outFile) | |||
| tasks, err := d.setupTasks(destPath, fileSize, chunkSize, outFile, savedState, isResume) | |||
There was a problem hiding this comment.
This PR bundles two independent changes: (1) the resumed-download size heuristic (getEffectiveSizeForWorkers, early LoadState) and (2) a significant error-propagation overhaul (cancel() threaded into executeWorkers, workers now returning errors instead of re-queuing). The error-propagation change is self-contained and can be reasoned about separately; if either half needs to be reverted it requires touching both. Splitting them into separate commits or PRs would make each easier to review, test, and revert independently.
Rule Used: What: Flag commits that bundle unnecessary changes... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/engine/concurrent/downloader.go
Line: 261-294
Comment:
**Mixed concerns: resume heuristic and worker error propagation**
This PR bundles two independent changes: (1) the resumed-download size heuristic (`getEffectiveSizeForWorkers`, early `LoadState`) and (2) a significant error-propagation overhaul (`cancel()` threaded into `executeWorkers`, workers now returning errors instead of re-queuing). The error-propagation change is self-contained and can be reasoned about separately; if either half needs to be reverted it requires touching both. Splitting them into separate commits or PRs would make each easier to review, test, and revert independently.
**Rule Used:** What: Flag commits that bundle unnecessary changes... ([source](https://app.greptile.com/surge-org-3/-/custom-context?memory=74a28159-60fc-4201-a661-331594ebaf90))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| func (d *ConcurrentDownloader) getEffectiveSizeForWorkers(fileSize int64, savedState *types.DownloadState, isResume bool) int64 { | ||
| if isResume && savedState != nil && savedState.TotalSize > 0 { | ||
| eff := savedState.TotalSize - savedState.Downloaded | ||
| if eff < 0 { | ||
| return 0 | ||
| } | ||
| return eff | ||
| } | ||
| return fileSize | ||
| } |
There was a problem hiding this comment.
fileSize parameter is unused when isResume is true
getEffectiveSizeForWorkers accepts fileSize but never reads it in the isResume branch — it derives the remaining size solely from savedState.TotalSize - savedState.Downloaded. If the live server reports a different file size than what was saved (e.g., a CMS updated the asset between pause and resume), the caller's freshly-probed fileSize is silently ignored for the heuristic, which may cause a surprising mismatch with the chunkSize calculation that still uses the live fileSize. Consider at least a debug log when savedState.TotalSize != fileSize.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/engine/concurrent/downloader.go
Line: 384-393
Comment:
**`fileSize` parameter is unused when `isResume` is true**
`getEffectiveSizeForWorkers` accepts `fileSize` but never reads it in the `isResume` branch — it derives the remaining size solely from `savedState.TotalSize - savedState.Downloaded`. If the live server reports a different file size than what was saved (e.g., a CMS updated the asset between pause and resume), the caller's freshly-probed `fileSize` is silently ignored for the heuristic, which may cause a surprising mismatch with the `chunkSize` calculation that still uses the live `fileSize`. Consider at least a debug log when `savedState.TotalSize != fileSize`.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Greptile Summary
This PR improves resumed downloads by computing the worker count heuristic against the remaining bytes rather than the full file size, preventing over-spawning workers for a nearly-complete download. It also fixes a latent bug where a task that exhausted all per-worker retries was silently re-queued indefinitely; workers now return the error, which triggers cancellation of the entire download.
state.LoadStateis called earlier inDownload()to deriveeffectiveSizeForWorkers(remaining bytes), which is fed intogetInitialConnectionsinstead of the total file size. ThesetupTaskssignature is updated to accept the pre-loaded state, removing a redundantLoadStatecall.worker()now returnslastErrafter exhausting retries.executeWorkersreceives acancelfunc and calls it on the first non-cancellation worker error, stopping all sibling workers promptly.executeWorkerschecks now prioritizedownloadErroverdownloadCtx.Err(), which is correct now thatcancel()is called internally on failure.Confidence Score: 4/5
Safe to merge; no correctness regressions were found. The resume heuristic and the fail-fast error propagation are both logically sound.
The resume heuristic correctly computes remaining bytes and the worker-count decision is purely advisory. The fail-fast change is a real improvement over the old infinite-requeue pattern. The main gaps are the absence of integration tests for the new worker-failure code path and the silent discard of the live
fileSizeduring a resume if the server-reported size differs from the saved state.worker.go— the behavioral shift from re-queuing to returning errors is the highest-impact change and currently has no test coverage.downloader.go(getEffectiveSizeForWorkers) silently ignores the freshly-probed file size when resuming, which could obscure a stale or corrupt state file.Important Files Changed
getEffectiveSizeForWorkersheuristic; threadscancelintoexecuteWorkers; improves context-error propagation ordering. Two P2 issues: bundled concerns andfileSizebeing silently unused in the resume branch.getEffectiveSizeForWorkerscovering fresh download, normal resume, and the negative-effective-size guard. Coverage is adequate for the new function itself.setupTaskscall sites to pass the now-explicitsavedStateandisResumeparameters; no logic changes.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor: propagate worker errors to dow..." | Re-trigger Greptile
Context used: