Skip to content

fix: implement graceful download queue clearing and throttle concurrent server probes with a semaphore#339

Merged
SuperCoolPencil merged 6 commits intomainfrom
fix-multiple-downloads
Apr 10, 2026
Merged

fix: implement graceful download queue clearing and throttle concurrent server probes with a semaphore#339
SuperCoolPencil merged 6 commits intomainfrom
fix-multiple-downloads

Conversation

@SuperCoolPencil
Copy link
Copy Markdown
Member

@SuperCoolPencil SuperCoolPencil commented Apr 10, 2026 •
edited by greptile-apps Bot
Loading

Greptile Summary

This PR introduces two related improvements: WorkerPool.GracefulShutdown now explicitly discards the in-memory queue and drains taskChan so that buffered-but-unstarted downloads are not picked up by workers after shutdown is initiated; and LifecycleManager gains a probeSem channel semaphore that caps simultaneous server probes to MaxConcurrentProbes (default 3), aborting early when the caller's context is cancelled. A small integrity fix also ensures queued DB entries whose .surge files have disappeared are cleaned up on startup.

Confidence Score: 5/5

Safe to merge — both features are well-implemented and tested; only P2 style issues remain.

All findings are P2: a missing "Requires restart" note in the in-app UI description and a stale comment in a test. The core semaphore logic, queue-clearing, and channel lifecycle are correct. New tests thoroughly cover the added behavior.

internal/config/settings.go (missing "Requires restart" in ui_desc tag for max_concurrent_probes)

Important Files Changed

Filename Overview
internal/download/pool.go Added GracefulShutdown: clears the in-memory queued map, drains taskChan, waits for active pauses to complete, then closes channels — correct and well-guarded.
internal/processing/manager.go Adds probeSem channel semaphore to LifecycleManager; enqueueResolved acquires a slot before probing and releases via defer. Semaphore is correctly initialized from settings at construction time.
internal/config/settings.go Adds MaxConcurrentProbes to NetworkSettings with default 3; the ui_desc tag is missing the "Requires restart" note that SETTINGS.md and the adjacent max_concurrent_downloads field include.
docs/SETTINGS.md Documents max_concurrent_probes with "Requires restart" correctly in the Connection Settings table.
internal/engine/state/state.go ValidateIntegrity now correctly removes queued entries whose .surge file is absent, fixing a gap where orphaned DB rows could accumulate for queued-then-cancelled downloads.
internal/processing/manager_test.go Adds two probe-semaphore tests; TestLifecycleManager_ProbeSemaphore_LimitsInflight correctly uses a non-nil addFunc that returns an error, but the comment on line 1052 still incorrectly says "addFunc is nil".
internal/download/pool_test.go New tests cover GracefulShutdown queue clearing (ClearsQueuedMap, WorkerSkipsQueuedAfterShutdown) and the soft/hard timeout paths — good coverage of the new behavior.
internal/core/local_service_test.go Removed TestLocalDownloadService_Shutdown_PersistsQueuedState — intentional, as the new GracefulShutdown purposely discards the in-memory queue rather than persisting it.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant LifecycleManager
    participant probeSem
    participant ProbeServer
    participant WorkerPool

    Caller->>LifecycleManager: Enqueue(ctx, req)
    LifecycleManager->>probeSem: acquire slot (blocks if cap reached)
    alt ctx cancelled while waiting
        probeSem-->>LifecycleManager: ctx.Done()
        LifecycleManager-->>Caller: error: enqueue aborted
    else slot acquired
        probeSem-->>LifecycleManager: slot granted
        LifecycleManager->>ProbeServer: ProbeServerWithProxy()
        ProbeServer-->>LifecycleManager: ProbeResult (size, range support)
        LifecycleManager->>WorkerPool: dispatch(finalPath, finalFilename, probe)
        WorkerPool-->>LifecycleManager: downloadID
        LifecycleManager->>probeSem: release slot (defer)
        LifecycleManager-->>Caller: downloadID
    end

    Note over WorkerPool: On GracefulShutdown
    WorkerPool->>WorkerPool: PauseAll() active downloads
    WorkerPool->>WorkerPool: clear queued map
    WorkerPool->>WorkerPool: drain taskChan
    WorkerPool->>WorkerPool: wg.Wait() for pause completion
    WorkerPool->>WorkerPool: close(progressDone), close(taskChan)
Loading
Prompt To Fix All With AI 
This is a comment left during a code review.
Path: internal/config/settings.go
Line: 53

Comment:
**`ui_desc` missing "Requires restart" notice**

`max_concurrent_downloads` on the line above includes "Requires restart." in its `ui_desc` tag, and SETTINGS.md documents the same for `max_concurrent_probes`. The in-app UI description for `max_concurrent_probes` is missing this notice, so users who change the value through the settings UI won't know they need to restart for it to take effect.

```suggestion
	MaxConcurrentProbes    int    `json:"max_concurrent_probes" ui_label:"Max Concurrent Probes" ui_desc:"Maximum number of simultaneous server probes when adding many downloads at once (1-10). Requires restart."`
```

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/processing/manager_test.go
Line: 1052-1053

Comment:
**Stale comment contradicts the code**

The comment says "addFunc is nil so every enqueue fails after probe" but at line 1027 `mgr.addFunc` is explicitly set to a non-nil function that returns an error. The comment should describe why the dispatch is rejected, not misstate what `addFunc` is.

```suggestion
	// dispatch is intentionally rejected; we only care that the probe phase was throttled.
```

**Rule Used:** What: Comments must explain *why* code exists, not... ([source](https://app.greptile.com/review/custom-context?memory=4e45ef13-32c2-4753-8060-a44ef767144d))

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

Reviews (2): Last reviewed commit: "fix: remove wrong test" | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 10, 2026 •
edited
Loading

Binary Size Analysis

⚠️ Size Increased

Version Human Readable Raw Bytes
Main 18.94 MB 19861796
PR 18.95 MB 19865892
Difference 4.00 KB 4096

greptile-apps[bot]
greptile-apps Bot reviewed Apr 10, 2026
View reviewed changes
Comment thread internal/processing/manager_test.go Outdated
Comment thread docs/SETTINGS.md Outdated
@SuperCoolPencil SuperCoolPencil force-pushed the fix-multiple-downloads branch from 627a72e to b8f6c86 Compare April 10, 2026 18:31
@SuperCoolPencil SuperCoolPencil merged commit ab15dcc into main Apr 10, 2026
15 checks passed
@SuperCoolPencil SuperCoolPencil deleted the fix-multiple-downloads branch April 10, 2026 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@SuperCoolPencil