Skip to content

fix: broken fallback to single downloader - #519

Merged
SuperCoolPencil merged 8 commits into
mainfrom
fix/fallback
Jun 26, 2026
Merged

fix: broken fallback to single downloader#519
SuperCoolPencil merged 8 commits into
mainfrom
fix/fallback

Conversation

@SuperCoolPencil

@SuperCoolPencil SuperCoolPencil commented Jun 26, 2026

Copy link
Copy Markdown
Member

Greptile Summary

This PR fixes two related gaps in the single-threaded download path: DownloadPausedMsg was never emitted when a single-threaded download was paused (the pool now sends it when isPaused && err != nil), and ActiveWorkers was never set or cleared, leaving progress state inconsistent. It also adds rate-limit retry logic (429/503 Retry-After) to the single downloader and wires UpdateChunkStatus into progressReader so the chunk bitmap is populated during single-threaded transfers.

  • Pool pause fix (pool.go): The discriminating condition isPaused && err != nil correctly identifies single-threaded paused downloads (which return a non-nil error) vs. concurrent ones (which send the message themselves and return nil).
  • Single downloader enhancements (single/downloader.go): Introduces a child context (dlCtx) for pause/cancel, deferred ActiveWorkers lifecycle management, 429/503 retry-after loop, and chunk-bitmap updates via progressReader.flushWithTime.
  • State cleanup (types/progress.go): SessionReset now zeroes ActiveWorkers; the noisy UpdateChunkStatus skipped debug log is removed since single-threaded downloads call it with no bitmap.

Confidence Score: 5/5

The core bug fixes are correct and well-scoped; the two findings are minor quality concerns that do not affect correctness of the main fix.

The pause-message fix and ActiveWorkers lifecycle management are correct. The throttledReader context mismatch only causes marginal pause-response delay under rate limiting, and the missing retry tests do not affect current runtime behavior.

internal/engine/single/downloader.go — the rate-limit retry loop and the throttledReader context are the areas worth a second look

Important Files Changed

Filename Overview
internal/download/pool.go Adds DownloadPausedMsg emission for single-threaded paused downloads; the discriminating condition (isPaused && err != nil) correctly distinguishes single-threaded (returns non-nil error on pause) from concurrent (returns nil, sends message itself)
internal/engine/single/downloader.go Adds ActiveWorkers lifecycle, a child context for cancellation, rate-limit retry loop, and UpdateChunkStatus calls to progressReader; throttledReader still uses parent ctx instead of dlCtx, and the retry loop has no test coverage
internal/engine/types/progress.go SessionReset now zeroes ActiveWorkers and removes a noisy debug log from UpdateChunkStatus
internal/engine/single/downloader_test.go Adds ActiveWorkers post-completion assertions; does not cover the new rate-limit retry code path
internal/engine/types/progress_test.go Extends SessionReset test to verify ActiveWorkers is zeroed; straightforward and correct
internal/download/manager.go Adds debug logging around single-threaded download outcome; minimal, correct change
cmd/root_startup.go Logs version and commit at startup for easier debugging; safe one-liner addition

Reviews (4): Last reviewed commit: "feat: single downloader Retry-After" | Re-trigger Greptile

Context used:

  • Rule used - What: All code changes must include tests for edge... (source)

Comment thread internal/engine/single/downloader.go
@github-actions

Copy link
Copy Markdown

Binary Size Analysis

⚠️ Size Increased

Version Human Readable Raw Bytes
Main 17.00 MB 17821988
PR 17.00 MB 17826084
Difference 4.00 KB 4096

@github-actions

Copy link
Copy Markdown

❌ Test Failures on windows-latest

  • github.com/SurgeDM/Surge/cmd: TestRmClean_Offline_Works

@SuperCoolPencil
SuperCoolPencil merged commit 082ad61 into main Jun 26, 2026
13 of 14 checks passed
@SuperCoolPencil
SuperCoolPencil deleted the fix/fallback branch June 26, 2026 10:07
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.

1 participant