You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi Surge team! Thanks for building such an incredibly robust and fast engine. We are building a project that integrates with Surge's backend, and we love the direction it's heading. We'd like to contribute a small but impactful enhancement to the download request pipeline.
Motivation
When integrating with download engines via RPC, external orchestrators often require deterministic control over worker allocation and chunk sizes. Rather than relying solely on static file-size heuristics, client applications may need to dynamically allocate threads based on available bandwidth, specific server restrictions, or real-time network conditions.
Currently, Surge’s backend beautifully handles maximum global connections, but it strictly relies on a √size heuristic for per-task worker allocation. We've introduced a minimal, backward-compatible override that allows API callers to inject precise Workers and MinChunkSize parameters per download. This aligns Surge with universal daemon patterns (similar to aria2c's split and min-split-size RPC parameters) while seamlessly falling back to the default heuristic when unset.
Target vs Ceiling:
A key design decision here is that Workers specifies a precise target, not a ceiling. When set, the engine uses exactly that many connections (still safely clamped by MaxConnectionsPerDownload and MinChunkSize), rather than dynamically computing them via the √size heuristic. This allows external applications to strictly enforce thread counts without racing against global configuration changes.
Alignment with Engine Roadmap
Noting the discussion in #331 regarding the roadmap to eventually "extract surge core" for GUI wrappers, exposing these granular parameters is a foundational step.
For extracted engines: When an engine operates as an independent library or daemon, external GUIs need exact target values to implement predictable, deterministic scheduling, rather than just setting a ceiling and letting the engine guess.
For dynamic scaling: Laying the groundwork for per-task overrides opens the door for future dynamic parameter tuning (e.g., adaptive thread scaling mid-flight) by ensuring the underlying engine can accept and strictly respect external constraints.
Verification & Testing
Manual Debugging: We temporarily injected a probe into downloader.go immediately following getInitialConnections and ran the daemon in server mode. We dispatched a ~23MB download via the API with workers: 16 and min_chunk_size: 1048576. The engine successfully overrode the heuristic and spawned exactly 16 workers, completing the task at ~17.6MB/s.
Unit Tests: Added comprehensive test coverage for the heuristic override, bounds clamping, parameter threading, and configuration overlay.
Automated Tests: go test ./... and go test -race ./... both pass successfully for our modified packages. (Note: A pre-existing environment-specific failure exists in TestWindowsPathsIgnoreRelativeAppData on Windows, which is orthogonal to these core engine changes).
(The following technical summary was generated by AI)
Summary
Add optional per-task Workers and MinChunkSize fields to Surge's download pipeline, enabling external callers to specify exact worker counts and chunk sizes on a per-download basis. When unset (zero-value), behavior is unchanged.
engine/types: New Workers field on RuntimeConfig with zero-value sentinel for "use heuristic"
engine/concurrent: getInitialConnections() overrides √size heuristic when Workers > 0, still clamped by MaxConnectionsPerDownload and MinChunkSize
processing: DownloadRequest gains Workers/MinChunkSize fields; function signatures and Enqueue/EnqueueWithID thread them through
core: DownloadService interface updated; LocalDownloadService overlays non-zero values onto RuntimeConfig per task; RemoteDownloadService forwards them in HTTP payloads
cmd: HTTP API DownloadRequest gains workers/min_chunk_size JSON fields
tui: Legacy call sites pass zero values (no TUI support yet)
tests: All mocks/signatures updated; new unit tests cover √size override, clamping, override threading, and RuntimeConfig overlay
Greptile Summary
This PR adds optional per-task Workers and MinChunkSize overrides to Surge's download pipeline, enabling API callers to specify exact worker counts and chunk sizes on a per-download basis. When the fields are zero (the default), all existing heuristic behaviour is unchanged.
RuntimeConfig gains a Workers field (zero = use √size heuristic); getInitialConnections checks it before the heuristic path and still clamps by MaxConnectionsPerDownload and MinChunkSize.
DownloadRequest, DownloadState, DownloadEntry, and every event message in the lifecycle chain are extended with Workers/MinChunkSize; values are persisted through pause/resume via buildResumeConfig, which prefers savedState over the master-list entry.
The HTTP API, TUI, and LocalDownloadService/RemoteDownloadService are all updated; TUI call sites pass zero (no UI for these fields yet) while API-initiated paths forward the caller-provided values end-to-end.
Confidence Score: 5/5
Safe to merge; the feature is purely additive with zero-value fallback to existing behaviour, and the full lifecycle path (enqueue → start → pause → resume → complete) is covered by the new tests.
The override values are threaded consistently through all dispatch paths, persisted in both DownloadState and DownloadEntry, and correctly restored by buildResumeConfig. The download/manager.go correctly populates Workers/MinChunkSize in DownloadStartedMsg from cfg.Runtime, so the master list is never silently zeroed after start. All changed call sites compile with the new signatures, and the new test files cover the key correctness properties including clamping, zero-value passthrough, savedState priority, and entry fallback.
No files require special attention; the one minor observation (redundant maxConns clamp in local_service.go vs getInitialConnections) is non-blocking.
Important Files Changed
Filename
Overview
internal/engine/concurrent/downloader.go
Adds Workers override path in getInitialConnections; clamped by maxConns and minChunkSize. Also persists Workers/MinChunkSize to DownloadState on pause.
internal/engine/types/config.go
Adds Workers field to RuntimeConfig with zero-value-means-heuristic sentinel; GetWorkers() helper; DefaultRuntimeConfig sets Workers=0.
internal/engine/types/models.go
Adds Workers/MinChunkSize fields (with omitempty) to DownloadState and DownloadEntry for persistence and resume recovery.
internal/core/local_service.go
Threads workers/minChunkSize through the add() call chain; clamps workers against MaxConnectionsPerDownload before setting runtime.Workers.
internal/core/remote_service.go
Conditionally includes workers/min_chunk_size in HTTP payloads only when > 0; correct opt-in serialization.
internal/processing/events.go
Propagates Workers/MinChunkSize through all lifecycle event handlers (queued, started, paused, completed, finalization error).
internal/processing/pause_resume.go
buildResumeConfig restores Workers/MinChunkSize from savedState (preferred) or entry fallback, correctly re-applying overrides on resume.
internal/processing/manager.go
Extends AddDownloadFunc/AddDownloadWithIDFunc signatures; threads Workers/MinChunkSize through Enqueue and EnqueueWithID into the queue layer.
cmd/root_downloads.go
HTTP DownloadRequest gains workers/min_chunk_size JSON fields; forwarded through all dispatch paths (single, batch, approval, enqueue).
internal/tui/process.go
startDownload signature gains workers/minChunkSize parameters; TUI form calls pass 0,0 while API-initiated calls forward the received overrides.
internal/engine/state/state.go
SaveStateWithOptions now copies Workers/MinChunkSize from DownloadState into the master list entry when updating state.
New test file covering workers override, maxConns clamping, minChunkSize clamping, and heuristic fallback paths.
internal/processing/pause_resume_override_test.go
New test file verifying buildResumeConfig restores overrides from savedState (priority) or entry fallback, and defaults when neither is set.
internal/tui/override_threading_test.go
New test file (254 lines) covering end-to-end threading of override values through extension confirmation, duplicate-warning, and batch-confirm TUI flows.
Positional zero arguments reduce call-site legibility
Both TUI call sites now pass 0, 0, 0, false as the last four positional arguments (workers, minChunkSize, totalSize, supportsRange). The same pattern appears across several other stubs (e.g., connect_test.go, headless_approval_test.go). As the AddDownloadFunc/AddWithIDFunc types accumulate parameters, callers become hard to audit without consulting the definition.
Consider introducing a lightweight DownloadParams struct (or reusing processing.DownloadRequest) so call sites read as named fields rather than ordered literals — similar to how other parts of the engine use types.DownloadConfig.
This is a comment left during a code review.
Path: internal/tui/process.go
Line: 156-177
Comment:
**Positional zero arguments reduce call-site legibility**
Both TUI call sites now pass `0, 0, 0, false` as the last four positional arguments (`workers`, `minChunkSize`, `totalSize`, `supportsRange`). The same pattern appears across several other stubs (e.g., `connect_test.go`, `headless_approval_test.go`). As the `AddDownloadFunc`/`AddWithIDFunc` types accumulate parameters, callers become hard to audit without consulting the definition.
Consider introducing a lightweight `DownloadParams` struct (or reusing `processing.DownloadRequest`) so call sites read as named fields rather than ordered literals — similar to how other parts of the engine use `types.DownloadConfig`.
**Rule Used:** # Code Review Rule: Suggest Architectural Improvem... ([source](https://app.greptile.com/surge-org-3/-/custom-context?memory=4d82344e-33c4-4e14-bbc7-81d79af11b7e))
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!
internal/core/remote_service.go, line 448-460 (link)
Zero-value fields are always serialised in the HTTP payload
Both Add and AddWithID unconditionally include "workers": 0 and "min_chunk_size": 0 in the JSON map even when the caller provides no overrides. The server side is safe (the > 0 guard in local_service.go ignores zeros), but the payload carries unnecessary keys on every remote call. Conditional inclusion (only when > 0) or omitting them altogether when zero would keep the wire format minimal and prevent confusion if a future server version interprets an explicit 0 differently from an absent field.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/core/remote_service.go
Line: 448-460
Comment:
**Zero-value fields are always serialised in the HTTP payload**
Both `Add` and `AddWithID` unconditionally include `"workers": 0` and `"min_chunk_size": 0` in the JSON map even when the caller provides no overrides. The server side is safe (the `> 0` guard in `local_service.go` ignores zeros), but the payload carries unnecessary keys on every remote call. Conditional inclusion (only when `> 0`) or omitting them altogether when zero would keep the wire format minimal and prevent confusion if a future server version interprets an explicit `0` differently from an absent field.
How can I resolve this? If you propose a fix, please make it concise.
Hey @superGekFordJ I will take a look at the pr as soon as I can. The design choice seems okay to me if an external system wants to force use specific number of workers and a min chunk size, it should be fine as long as they are within a threshold as defined in surge core.
Thanks for building on top of surge.
May I ask what exactly are you building with surge backend? if you are open to sharing :)
The reason will be displayed to describe this comment to others. Learn more.
This is functionally fine but there is a hygiene issue
If the API puts worker count as a value which is beyond maxConnections then that is handled inside download where it is clamped to the maxConnections.
But the downloadConfig contains the raw unsanitised value which is a conflict.
Not a blocker but worth taking a look at
The maxWorkers and minChunkSize set by the API is lost during a cold resume (restart surge and restart download)
In pause_resume.go
saved.LoadState() loads a DownloadState which does not have workers and MinChunkSize params so they are lost on a cold resume
I do not think this should be intended behaviour because that defeats the purpose of having a way to populate these fields through the API.
The fields should ideally be in the DownloadEntry
Let me know your thoughts
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The pull request aims to introduce precise per-task worker and chunk size overrides; however, the provided metadata and analysis indicate that no files were actually included in the submission. Consequently, none of the acceptance criteria—such as bypassing the square-root heuristic or enforcing clamping constraints—could be verified. The absence of implementation code makes it impossible to confirm if the engine correctly falls back to defaults or if the HTTP API structure was successfully updated. Furthermore, the proposed changes to function signatures may negatively impact maintainability by introducing excessive positional arguments.
About this PR
The PR appears to contain no file changes according to the provided analysis tools. This makes it impossible to verify the implementation of worker overrides, clamping logic, or service-layer propagation.
Test suggestions
Verify the √size heuristic is used when overrides are zero or unset
Verify the 'Workers' override correctly bypasses the heuristic calculation
Verify the 'Workers' count is correctly clamped by 'MaxConnectionsPerDownload'
Verify the 'Workers' count is correctly limited by available chunks (TotalSize / MinChunkSize)
Verify 'getInitialConnections' returns a minimum floor of 1 connection
Verify 'LocalDownloadService' correctly merges per-task overrides into the task configuration
Verify the HTTP API correctly deserializes 'workers' and 'min_chunk_size' from JSON payloads
Verify 'RemoteDownloadService' conditionally serializes overrides (only when > 0) to keep payloads minimal
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify the √size heuristic is used when overrides are zero or unset
2. Verify the 'Workers' override correctly bypasses the heuristic calculation
3. Verify the 'Workers' count is correctly clamped by 'MaxConnectionsPerDownload'
4. Verify the 'Workers' count is correctly limited by available chunks (TotalSize / MinChunkSize)
5. Verify 'getInitialConnections' returns a minimum floor of 1 connection
6. Verify 'LocalDownloadService' correctly merges per-task overrides into the task configuration
7. Verify the HTTP API correctly deserializes 'workers' and 'min_chunk_size' from JSON payloads
8. Verify 'RemoteDownloadService' conditionally serializes overrides (only when > 0) to keep payloads minimal
Low confidence findings
Ensure that 'workers' and 'min_chunk_size' fields are conditionally serialized (e.g., using 'omitempty' in Go) to keep HTTP payloads minimal when overrides are not used.
Adding several positional arguments (workers, minChunkSize, totalSize, etc.) to function signatures reduces legibility and increases the risk of parameter-ordering bugs at the call-site. Consider using a structured configuration object.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the requested per-task overrides for worker counts and minimum chunk sizes, allowing for more granular control over download concurrency. The implementation includes the necessary fallback heuristics and safety clamping logic.
While the Codacy analysis indicates the code is up to standards, two primary concerns were identified. First, a potential integer overflow in the worker calculation on 32-bit systems could lead to degraded performance for large file downloads. Second, the expansion of core service methods to include up to 11 positional parameters has reached a level that significantly hinders maintainability and increases the risk of argument-order bugs.
About this PR
The addition of multiple positional arguments to the Add and AddWithID signatures (up to 10 parameters) across more than 20 files reduces code legibility and makes maintenance prone to errors; several call sites now use sequences of literals like '0, 0, 0, false' which are difficult to audit.
Test suggestions
Verify worker count calculation falls back to √size heuristic when Workers override is 0.
Verify Workers override correctly bypasses the heuristic calculation when provided as a positive integer.
Verify Workers override is capped at MaxConnectionsPerDownload when the requested value is higher.
Verify Workers override is limited based on fileSize and MinChunkSize to ensure valid chunking.
Verify MinChunkSize override is respected and correctly influences the max possible chunks calculation.
Verify LocalDownloadService correctly overlays per-task config parameters onto global settings.
Verify Enqueue and EnqueueWithID correctly propagate override parameters through the processing layer.
To answer your question: we're building GoAria, a Wails-based download manager. We are trying to use surge to handle HTTP(s) downloads and leave other protocols to aria2c. The reason we need this override is that our orchestrator calculates thread allocations dynamically per task based on real-time bandwidth and network congestion. It needs the underlying engine to strictly follow these targets. Surge's architecture is great for this! I'll be publishing the dual-engine version soon if you're curious to see it in action.
I just pushed some fixes for the issues you found:
For the config hygiene: workers is now clamped to MaxConnectionsPerDownload early before it hits RuntimeConfig. We keep the priority strict: API Workers -> clamped by MaxConnections -> clamped by MinChunkSize constraints. (Also note that the MinChunkSize clamp still happens later in getInitialConnections since it needs fileSize from the HEAD request).
About the cold resume, losing those fields on restart completely defeats the API's purpose. I added Workers and MinChunkSize to DownloadState and DownloadEntry so they are properly saved and loaded on a cold resume now.
@superGekFordJ this feature fails for batch downloads because events.DownloadRequestMsg lacks Workers and MinChunkSize fields. So the configured values are dropped and defaults are used
I think for your application you will need batch downloads as well? In that case this might be worthy of a look.
Let me know if you need more help to resolve this. I can help you with the appropiate fixes.
…te DownloadEntry writes
- AddToMasterList performs full replacement, not merge. The DownloadCompleteMsg
handler built a fresh DownloadEntry without copying Workers/MinChunkSize from
the existing entry, silently zeroing override metadata on completed and
finalization-error states. Since error entries are resumable, this caused
cold-resume to lose per-task worker/chunk config. Now both paths copy
Workers/MinChunkSize from the existing entry before AddToMasterList.
@junaid2005p good catch on the batch downloads. Since I embed Surge directly as an in-process engine and bypass the TUI entirely, those modal flows completely slipped under my radar. I just pushed a fix for this: I extended events.DownloadRequestMsg to carry the override fields and threaded them through the TUI confirmation state machine (batch approvals, dupe warnings, etc.).
Since I'm less familiar with the frontend TUI design and features, I leaned on AI to help cover those modal loops with some defensive regression tests. If you feel any of them are overkill or redundant, feel free to just drop them before merging.
Also, regarding the cold resume issue you caught earlier: I traced it to a bug in events.go. The completion handler was building a fresh DownloadEntry and accidentally dropping the override metadata beforedropping the override metadata before writing it to the master list. I added a fix there so the values properly survive restarts now.
While integrating, I noticed poorly behaved servers (e.g., missing FIN packets) causing the engine to hang until timeout. I built a "verified process" fix for us, but since it might introduce race conditions for your TUI, I'll hold off on upstreaming it for now.
However, I built a few other enhancements that I believe would cleanly fit into upstream Surge:
Tiered buffer pool with cap filter
End-game hedge poison defense & connection error detection: using recordHedgeError() / recordHedgeSuccess() and isConnLimitError() (handles 503/429/conn refused).
Tail-end chunk degradation: letting StealWork scale down MinChunkSize via alignedSplitSizeWithMin.
Since we just rolled out this major update, it might be a little while before I have the bandwidth to prepare these as clean PRs. Also, I noticed you guys just pulled off a massive architectural refactor (the new decoupled layers look amazing for external API consumers like us, by the way!), so I'll probably need to port our patches over to your new scheduler/strategy layers first.
In the meantime, feel free to poke around our fork and let me know if any of these catch your eye for merging!
Hey @superGekFordJ sorry I was busy with other stuff. This looks amazing. I will take a deeper look over the weekend and let you know.
The enhancements look good to me.
However I have some questions
Why do we need a tiered buffer pool? We need as many buffers as many workers. If there is a worker hedging another worker that means a buffer must be free? so what problem is a tiered buffer solving and it would also be nice if you had some numbers to show the difference!
letting StealWork scale down MinChunkSize via alignedSplitSizeWithMin
This seems like a good idea I will read your implementation but could you tell me if this yields better results on your benchmarks?
Nonetheless would appreciate those PRs if you have time and thanks for contributing. Please open a new issue if you would be kind enough to send a patch for these.
Benchmarking this perfectly is tough due to network fluctuations. However, I noticed Surge sometimes stalls at 99% longer than aria2 on bad nodes. To help with this, I found these two tweaks to be the simplest drop-in improvements:
When a download stalls near the end, it's usually 1-2 workers crawling on a bad connection. If the remaining bytes are smaller than MinChunkSize, idle workers can't steal them and just exit early.
You can see this exact 'tail drop' in our 538MB benchmark (Note: this was actually run on a highly-optimized server capable of saturating gigabit bandwidth): (Graph legend: Solid = Throughput MB/s; Dashed = Thread Count. Green = Surge, Blue = GoAria).
If you look at the green dashed line (Surge) toward the tail-end, threads start dropping aggressively. Here is the raw JSONL log for that exact moment (with 23MB remaining):
{"ts":"09:41:03.412...","completed_bytes":515056887,"current_threads":11}
{"ts":"09:41:03.473...","old_threads":11,"new_threads":10,"reason":"worker_exited"}
... [6 more workers exit over the next 2.5 seconds] ...
{"ts":"09:41:05.997...","old_threads":4,"new_threads":3,"reason":"worker_exited"}
{"ts":"09:41:06.262...","completed_bytes":523445495,"current_threads":3}
For 2.5 seconds, completed_bytes was completely frozen at 515MB, yet 8 workers exited. They checked out early because they couldn't steal the remaining 23MB (restricted by static chunk size).
By letting StealWork dynamically shrink that chunk floor, all workers stay alive (blue dashed line) and instantly swarm the last few megabytes to break the stall.
However, waking up 10 workers to grab tiny chunks normally wastes 5MB of RAM (with default 512KB buffers). Since these tail-end connections are slow anyway, my Tiered Buffer Pool catches this and drops them to 32KB buffers. This lets the engine handle huge tail-end swarms without blowing up memory on constrained devices. (The Cap Filter also prevents Go's Issue #23199 memory leak).
As I mentioned earlier, since you just completed a massive architectural refactor, I'll need some time to port my downstream logic over to your new scheduler before submitting a PR. Let me know what you think of these ideas!
This is so interesting! Thanks for bringing it up. I've always thought that waking up the workers just to reduce tail latency would be punishing because of the TCP setup cost. But the benchmarks say otherwise.
Hey @SuperCoolPencil, thanks for bringing up such an intriguing point.
I agree that the TTFB overhead is typically high, but the dynamics at the tail end are quite different.
From my observations, tail-end stalls usually happen when a few specific workers get stuck on a bad connection. Waking up fresh workers generally allows us to bypass the stall at normal speeds. Plus, since Surge allocates a higher number of concurrent workers than I initially expected, hitting a poorly behaved server node doesn't actually seem that rare.
Because of this, my intuition is that enabling this tail-end acceleration for Surge is well worth the handshake overhead.
Also, I noticed from the test data that a major factor in these tail-end stalls is actually workers failing to exit gracefully even after the data is fully received (e.g., hanging on missing FIN packets). Once our GoAria 'verified process' (VP) mechanism stabilizes, I'll definitely look into upstreaming it to address that side of the bottleneck as well!
I've been thinking lately to reduce the default workers to 12 or even 8. For me personally that seems to work better on most sites without a considerable drop in performance, maybe that's the way forward.
Lowering the default worker count is definitely the safest route for stability on strict sites.
But that's exactly the trade-off that led me to build this dynamic scheduling approach! By keeping the initial worker count high and only manipulating the tail-end chunks, we get to keep the peak throughput on good links without getting punished by the strict ones at the end.
If you ever have the time, feel free to take GoAria's scheduler for a spin to see how that balance plays out in practice! 😉
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
Hi Surge team! Thanks for building such an incredibly robust and fast engine. We are building a project that integrates with Surge's backend, and we love the direction it's heading. We'd like to contribute a small but impactful enhancement to the download request pipeline.
Motivation
When integrating with download engines via RPC, external orchestrators often require deterministic control over worker allocation and chunk sizes. Rather than relying solely on static file-size heuristics, client applications may need to dynamically allocate threads based on available bandwidth, specific server restrictions, or real-time network conditions.
Currently, Surge’s backend beautifully handles maximum global connections, but it strictly relies on a
√sizeheuristic for per-task worker allocation. We've introduced a minimal, backward-compatible override that allows API callers to inject preciseWorkersandMinChunkSizeparameters per download. This aligns Surge with universal daemon patterns (similar toaria2c'ssplitandmin-split-sizeRPC parameters) while seamlessly falling back to the default heuristic when unset.Target vs Ceiling:
A key design decision here is that
Workersspecifies a precise target, not a ceiling. When set, the engine uses exactly that many connections (still safely clamped byMaxConnectionsPerDownloadandMinChunkSize), rather than dynamically computing them via the√sizeheuristic. This allows external applications to strictly enforce thread counts without racing against global configuration changes.Alignment with Engine Roadmap
Noting the discussion in #331 regarding the roadmap to eventually "extract surge core" for GUI wrappers, exposing these granular parameters is a foundational step.
Verification & Testing
downloader.goimmediately followinggetInitialConnectionsand ran the daemon inservermode. We dispatched a ~23MB download via the API withworkers: 16andmin_chunk_size: 1048576. The engine successfully overrode the heuristic and spawned exactly 16 workers, completing the task at ~17.6MB/s.go test ./...andgo test -race ./...both pass successfully for our modified packages. (Note: A pre-existing environment-specific failure exists inTestWindowsPathsIgnoreRelativeAppDataon Windows, which is orthogonal to these core engine changes).(The following technical summary was generated by AI)
Summary
Add optional per-task
WorkersandMinChunkSizefields to Surge's download pipeline, enabling external callers to specify exact worker counts and chunk sizes on a per-download basis. When unset (zero-value), behavior is unchanged.Workersfield onRuntimeConfigwith zero-value sentinel for "use heuristic"getInitialConnections()overrides √size heuristic whenWorkers > 0, still clamped byMaxConnectionsPerDownloadandMinChunkSizeDownloadRequestgainsWorkers/MinChunkSizefields; function signatures andEnqueue/EnqueueWithIDthread them throughDownloadServiceinterface updated;LocalDownloadServiceoverlays non-zero values ontoRuntimeConfigper task;RemoteDownloadServiceforwards them in HTTP payloadsDownloadRequestgainsworkers/min_chunk_sizeJSON fieldsGreptile Summary
This PR adds optional per-task
WorkersandMinChunkSizeoverrides to Surge's download pipeline, enabling API callers to specify exact worker counts and chunk sizes on a per-download basis. When the fields are zero (the default), all existing heuristic behaviour is unchanged.RuntimeConfiggains aWorkersfield (zero = use √size heuristic);getInitialConnectionschecks it before the heuristic path and still clamps byMaxConnectionsPerDownloadandMinChunkSize.DownloadRequest,DownloadState,DownloadEntry, and every event message in the lifecycle chain are extended withWorkers/MinChunkSize; values are persisted through pause/resume viabuildResumeConfig, which preferssavedStateover the master-listentry.LocalDownloadService/RemoteDownloadServiceare all updated; TUI call sites pass zero (no UI for these fields yet) while API-initiated paths forward the caller-provided values end-to-end.Confidence Score: 5/5
Safe to merge; the feature is purely additive with zero-value fallback to existing behaviour, and the full lifecycle path (enqueue → start → pause → resume → complete) is covered by the new tests.
The override values are threaded consistently through all dispatch paths, persisted in both DownloadState and DownloadEntry, and correctly restored by buildResumeConfig. The download/manager.go correctly populates Workers/MinChunkSize in DownloadStartedMsg from cfg.Runtime, so the master list is never silently zeroed after start. All changed call sites compile with the new signatures, and the new test files cover the key correctness properties including clamping, zero-value passthrough, savedState priority, and entry fallback.
No files require special attention; the one minor observation (redundant maxConns clamp in local_service.go vs getInitialConnections) is non-blocking.
Important Files Changed
Comments Outside Diff (2)
internal/tui/process.go, line 156-177 (link)Both TUI call sites now pass
0, 0, 0, falseas the last four positional arguments (workers,minChunkSize,totalSize,supportsRange). The same pattern appears across several other stubs (e.g.,connect_test.go,headless_approval_test.go). As theAddDownloadFunc/AddWithIDFunctypes accumulate parameters, callers become hard to audit without consulting the definition.Consider introducing a lightweight
DownloadParamsstruct (or reusingprocessing.DownloadRequest) so call sites read as named fields rather than ordered literals — similar to how other parts of the engine usetypes.DownloadConfig.Rule Used: # Code Review Rule: Suggest Architectural Improvem... (source)
Prompt To Fix With AI
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!
internal/core/remote_service.go, line 448-460 (link)Both
AddandAddWithIDunconditionally include"workers": 0and"min_chunk_size": 0in the JSON map even when the caller provides no overrides. The server side is safe (the> 0guard inlocal_service.goignores zeros), but the payload carries unnecessary keys on every remote call. Conditional inclusion (only when> 0) or omitting them altogether when zero would keep the wire format minimal and prevent confusion if a future server version interprets an explicit0differently from an absent field.Prompt To Fix With AI
Reviews (4): Last reviewed commit: "test: add MinChunkSize savedState>entry ..." | Re-trigger Greptile
Context used: