feat: implement global and per-task download rate limiting

[SuperCoolPencil]

Greptile Summary

This PR adds global and per-task download rate limiting backed by golang.org/x/time/rate, wired through a new RateLimiter interface on ProgressState and a GlobalRateLimiter singleton. The integration into both the single and concurrent download engines is clean, and context-cancellation errors from WaitN are now properly propagated.

The main concern is that persistSettings() triggers both ReloadSettings() and ApplySettings(), causing every rate-limit field in active downloads to be updated twice; the update logic should be consolidated into one path.

Confidence Score: 5/5

Safe to merge — only P2 style and test-coverage findings remain; no new blocking defects introduced.

All P0/P1 concerns from prior review rounds are resolved: WaitN errors are now propagated, duplicate downloader branches were collapsed, and the custom token bucket was replaced with golang.org/x/time/rate. The two new findings are P2 only: duplicate rate-update calls on settings save (idempotent, no incorrect behaviour) and missing combined-limiter test coverage.

internal/core/local_service.go and internal/processing/manager.go — redundant rate-update logic that should be consolidated.

Important Files Changed

Filename	Overview
internal/utils/ratelimit.go	New TokenBucket wrapping golang.org/x/time/rate; WaitN loop handles burst-reduction races correctly via retry logic.
internal/core/local_service.go	ReloadSettings now updates global rate AND per-task limiters, duplicating the same logic that ApplySettings/UpdateActiveRates performs; both paths fire on every TUI settings save.
internal/processing/manager.go	ApplySettings updates global rate limiter and per-task limiters via the UpdateActiveRates hook — same operations already done by ReloadSettings, causing double-updates on every settings save.
internal/engine/single/downloader.go	progressReader now carries ctx and applies per-task + global limiters before advancing progress; duplicate branches collapsed; limiter errors are propagated correctly.
internal/engine/concurrent/worker.go	Rate limit calls inserted after resp.Body.Read returns n>0; per-task and global limiter errors correctly break the inner loop before readSoFar is incremented.
internal/engine/single/ratelimit_test.go	Covers global-only and per-task-only cases; missing a test where both limiters are active simultaneously.
internal/engine/concurrent/ratelimit_test.go	Same coverage gap as single/ratelimit_test.go — no test exercises both limiters concurrently.
cmd/root.go	UpdateActiveRates closure wired into EngineHooks iterates GlobalPool to propagate per-task rate changes — logic already present in local_service.go:ReloadSettings, creating a redundant update path.

Sequence Diagram

sequenceDiagram
    participant TUI as TUI (view_settings)
    participant LS as LocalDownloadService
    participant MGR as LifecycleManager
    participant GRL as GlobalRateLimiter
    participant Pool as WorkerPool

    TUI->>TUI: persistSettings()
    TUI->>LS: ReloadSettings()
    LS->>GRL: SetRate(globalRate * 1024)  [update #1]
    LS->>Pool: GetAll() → SetRate(perTaskRate) on each  [update #1]
    LS-->>TUI: ok

    TUI->>MGR: ApplySettings(settings)
    MGR->>GRL: SetRate(globalRate * 1024)  [update #2 – duplicate]
    MGR->>MGR: getEngineHooks().UpdateActiveRates(perTaskRate)
    MGR->>Pool: GetAll() → SetRate(perTaskRate) on each  [update #2 – duplicate]
    MGR-->>TUI: ok

    note over GRL,Pool: Both limiters updated twice per save

Loading

Comments Outside Diff (1)

1. internal/processing/manager.go, line 81-111 (link)

   GlobalRateLimiter never seeded from settings at startup

   NewLifecycleManager loads settings from disk (line 84-88) but never calls ApplySettings or utils.GlobalRateLimiter.SetRate. The singleton is initialized to NewTokenBucket(0) in utils/ratelimit.go and is only updated when the user explicitly saves settings through the TUI (ApplySettings → SaveSettings). Any global_rate_limit configured in the settings file will be silently ignored on every restart until the user opens and re-saves the settings screen.

   Fix: add one line after settings are loaded in NewLifecycleManager:

   settings, err := config.LoadSettings()
   if err != nil {
       settings = config.DefaultSettings()
   }
   // Seed global rate limiter from persisted config
   utils.GlobalRateLimiter.SetRate(settings.Network.GlobalRateLimit * 1024)

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/processing/manager.go
   Line: 81-111
   
   Comment:
   **GlobalRateLimiter never seeded from settings at startup**
   
   `NewLifecycleManager` loads settings from disk (line 84-88) but never calls `ApplySettings` or `utils.GlobalRateLimiter.SetRate`. The singleton is initialized to `NewTokenBucket(0)` in `utils/ratelimit.go` and is only updated when the user explicitly saves settings through the TUI (`ApplySettings` → `SaveSettings`). Any `global_rate_limit` configured in the settings file will be silently ignored on every restart until the user opens and re-saves the settings screen.
   
   Fix: add one line after settings are loaded in `NewLifecycleManager`:
   
   ```go
   settings, err := config.LoadSettings()
   if err != nil {
       settings = config.DefaultSettings()
   }
   // Seed global rate limiter from persisted config
   utils.GlobalRateLimiter.SetRate(settings.Network.GlobalRateLimit * 1024)
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: internal/core/local_service.go
Line: 43-55

Comment:
**Duplicate rate update — fires twice per settings save**

`persistSettings()` in `view_settings.go` calls both `ReloadSettings()` and then `Orchestrator.ApplySettings()` unconditionally. Both paths update `utils.GlobalRateLimiter` and iterate the pool to call `SetRate` on every active per-task limiter. The operations are idempotent, so there's no correctness bug today, but the duplicated logic means any future divergence (e.g. adding a callback or log in one branch) will produce subtle inconsistencies, and it violates the eliminate-duplicate-logic guideline.

The canonical update path is `ApplySettings` (it receives the in-memory settings directly). Consider removing the rate-update block from `ReloadSettings` and letting `ApplySettings` remain the single source of truth, or extract a shared `applyRateLimits(s *config.Settings)` helper called by both.

**Rule Used:** What: Eliminate duplicate logic, functions, or cod... ([source](https://app.greptile.com/review/custom-context?memory=f0a796a9-684f-4dfb-a5cf-8c99c16b63a1))

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/engine/single/ratelimit_test.go
Line: 1-92

Comment:
**No test for concurrent global + per-task limits**

All four new test cases (`TestSingleDownloader_GlobalRateLimit`, `TestSingleDownloader_PerTaskRateLimit`, `TestConcurrentDownloader_GlobalRateLimit`, `TestConcurrentDownloader_PerTaskRateLimit`) exercise each limiter in isolation. There is no test where both `utils.GlobalRateLimiter` and `state.Limiter` are non-zero simultaneously.

When both are active, `progressReader.Read` (and the concurrent worker loop) applies them sequentially: per-task wait, then global wait. Because `golang.org/x/time/rate` tracks time continuously, the global limiter accumulates tokens during the per-task wait, so the stacking effect is small in practice. However, a test that sets `per_task = 64 KB/s` and `global = 128 KB/s` and asserts the download completes in roughly the per-task window (not 2× longer) would guard against regressions in the ordering or any future changes to how the two limiters interact.

**Rule Used:** What: All code changes must include tests for edge... ([source](https://app.greptile.com/review/custom-context?memory=2b22782d-3452-4d55-b059-e631b2540ce8))

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (5): Last reviewed commit: "feat: implement global and per-task down..." | Re-trigger Greptile}

Context used:

• Rule used - What: Eliminate duplicate logic, functions, or cod... (source)
• Rule used - What: All code changes must include tests for edge... (source)

[github-actions]

Binary Size Analysis

⚠️ Size Increased

Version	Human Readable	Raw Bytes
Main	18.98 MB	19898660
PR	19.00 MB	19919140
Difference	20.00 KB	20480