fix(engine): preserve multi-segment progress when resume collapses to 1 connection - #240
Merged
Merged
Conversation
… 1 connection `set_task_segments` documents that changed segment counts fully preserve downloaded progress, including the "reduce concurrency" case. But when the effective segment count collapsed to exactly 1 — via the persisted domain single-connection cache (`is_single_conn_domain`, survives an app/PC restart) or a direct user request — `downloader.rs`'s `use_segments` gate required `segments > 1`, so the resume always took the true single-stream path. That path's F025 resume branch unconditionally deletes the DB segment rows and the pre-allocated temp file and resets progress to 0 before re-downloading from byte 0, discarding gigabytes of already downloaded data. The wipe was unnecessary: `segment_coordinator.rs` already unconditionally restores the full existing segment layout from the DB whenever rows exist, regardless of the requested worker count, and its worker_cap is independently clamped to the same domain-cap value — the coordinator already supports a worker_cap==1 resume of an existing multi-segment download without any data loss. Widen the `use_segments` gate to also route through the coordinator when a resume already has a multi-segment layout on disk, so a forced single-connection resume degrades to one active connection instead of discarding progress. Fixes #239
Closed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Repro
Reporter's client log (task
a6fd8a93, an 11.66 GB file fromcas-bridge.xethub.hf.co) shows repeated segment-count changes and app-persisted domain single-connection learning colliding: at 22:10:42 the log shows分段数已改为 0(进度保留,was_active=true)followed immediately by域名命中单连接缓存,强制 segments=1andswitching multi-segment → single-stream on resume; clearing 9 stale segment(s) and pre-allocated temp file— the download restarts from byte 0 despite having significant prior progress. The same persisted domain cap (24h TTL,config.domain_conn_caps) means this also fires on ordinary app-restart resumes (e.g. after a PC shutdown mid-download), matching both reported symptoms (关闭电脑重下失败,修改线程下载失败). No build/run was possible in this bot environment; verified by full static trace throughset_task_segments→do_start_task/do_resume_task→downloader.rs→segment_coordinator.rs(see repro comment on the issue for line-by-line citations).Cause
is_single_conn_domain(native/engine/src/download_manager.rs:4520,:5437) forces a task's effective segment count to literal1whenever the domain was previously observed to reject multi-connection.downloader.rs'suse_segmentsgate requiredsegments > 1, so any resume that collapsed tosegments == 1always took the true single-stream path (download_single) instead of the multi-segment coordinator. That path's resume branch (taggedF025,downloader.rs~3006-3031) unconditionally deletes the DB segment rows and the pre-allocated temp file and resetsdownloaded_bytesto 0 before re-downloading from scratch — a correctness necessity fordownload_singlespecifically (its "resume fromexisting_len" logic is unsafe against a pre-allocated sparse file with scattered segment data), but not a necessity for the coordinator path, which was simply never reachable atsegments == 1.segment_coordinator.rs:1324-1370already unconditionally restores the entire prior segment layout from the DB whenever rows exist, regardless of theinitial_segment_count/worker_capargument, andworker_capis independently clamped to the same domain-cap value mid-flight on already-running downloads (segment_coordinator.rs:1609-1611). So asegments == 1resume of an existing multi-segment task can safely go through the coordinator (worker_cap=1, processing all existing segments serially) with zero data loss — thesegments > 1threshold indownloader.rswas simply too strict for the resume-with-existing-progress case.Fix
native/engine/src/downloader.rs: hoist the DB segment-row lookup that resume already performed for "auto" segment counts so it's available regardless of whether the segment count came from auto-reuse or an explicit/forced value.resume_has_segments(true when the resume already has a non-empty multi-segment layout in the DB).use_segmentsto... && (segments > 1 || resume_has_segments) && ..., so a resume that already has multi-segment progress stays on the coordinator path even when the effective concurrency was forced down to 1, instead of falling into the destructive single-stream wipe. Fresh downloads and genuinely non-resumable cases (range unsupported, non-GET, tiny file) are unaffected —resume_has_segmentsis only true whenp.is_resumeand DB rows exist.Verification
Not built or run in this bot environment — the container cannot host the Rust workspace (see repo
AGENTS.mdbot operating rules). Verified entirely by static trace:segment_coordinator.rs's initial segment-map construction (existing.is_empty()branch vs DB-restore branch, lines 1328-1370) ignores theinitial_segment_countparameter whenever DB rows exist, and separately clampsworker_capto the domain cap (lines 1609-1611) — so passingsegments=1intodownload_multi_segmentwhen 32 DB rows exist correctly restores all 32 segments and runs them with 1 concurrent connection, not 1 segment.resume_has_segmentsgate only changes behavior forp.is_resume && existing DB rows non-empty; all other call sites (fresh downloads, non-GET, range-unsupported, sub-1MB files) are untouched since they still faileffective_supports_range/effective_total_bytes > 1MB/is_get_like()or haveresume_has_segments == false.RangeNotSupported/tiny-file/non-GET wipe paths are untouched — they still correctly discard segment data when the server-side situation genuinely changed.Please run:
cargo check -p fluxdown_engine --libcargo clippy -p fluxdown_engine -- -D warningscargo nextest run -p fluxdown_engine downloader(and any existingsegment_coordinator/resume-focused tests)Known not covered: no new automated test was added for this specific interaction (domain-cap-forced resume of a partially-downloaded multi-segment task); writing a deterministic test would require mocking the HTTP server's Range behavior across a resume boundary, which this bot cannot validate without running the suite. A maintainer-added integration test exercising
set_task_segments/resume with a pre-existing segment layout and a domain cap of 1 would close that gap.Fixes #239