Skip to content

Conversation

gau-nernst
Copy link
Contributor

@gau-nernst gau-nernst commented May 27, 2025

Describe Your Changes

Add support for resuming download.

Outline of logic: Given that we want to download from URL url and save to save_path

  • We will write to save_path.tmp first, and save download url to save_path.url
  • When trying to resume download, we will check if save_path.tmp exists, and whether the saved URL in save_path.url matches the provided url
    • The saved URL provides a simple safety check against resuming with a different URL, though it is not yet fool-proof. Perhaps we can also save Etag to check?
  • If we resume download, we will use Range header to request the starting bytes. The starting bytes is the filesize of save_path.tmp
    • If server supports Range header, it will respond with 206 Partial Content. We will check for this, and fallback to normal download otherwise
    • We will open the file with append mode instead of creating a new file
  • When download finish, we will rename save_path.tmp to save_path, and remove save_path.url

Note: we really should have file checksum check to ensure resume logic did not go wrong. But it can be a bit tricky about who should provide the checksum.

Other misc changes

  • Use buffered writer
  • Change update interval to every 10MB
download_resume.mp4

Fixes Issues

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

Important

Adds support for resuming downloads with partial content requests and temporary file management in download.rs.

  • Behavior:
    • Adds download resume support in _download_files_internal() by checking for existing .tmp and .url files.
    • Uses Range header to request partial content if resuming, falling back to full download if not supported.
    • Renames .tmp file to final file upon completion and removes .url file.
  • Functions:
    • Adds _get_maybe_resume() to handle HTTP requests with Range header.
  • Misc:
    • Uses tokio::io::BufWriter for buffered writing in _download_files_internal().
    • Changes update interval to every 10MB in _download_files_internal().

This description was created by Ellipsis for 3bb7724. You can customize this summary. It will automatically update as commits are pushed.

@gau-nernst gau-nernst marked this pull request as ready for review May 27, 2025 01:31
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to 3bb7724 in 2 minutes and 8 seconds. Click for details.
  • Reviewed 164 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/core/utils/download.rs:196
  • Draft comment:
    Prefer using an asynchronous file existence check (e.g. tokio::fs::metadata) instead of the synchronous tmp_save_path.exists(), to avoid blocking the async runtime.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The code is in an async context and mixing sync and async file operations. The exists() call is synchronous and could potentially block. However, this is just a quick metadata check that is unlikely to cause significant blocking. The code already uses async operations for the more intensive file operations like reading and writing. The performance impact of a single synchronous exists() call is likely minimal in practice. The code is already working and the change adds complexity for questionable benefit. While the impact may be small, following consistent async patterns is good practice in async code to avoid any potential blocking operations. The suggestion is technically correct but the benefit is minimal compared to the added complexity. This is more of a style preference than a significant issue.
2. src-tauri/src/core/utils/download.rs:220
  • Draft comment:
    In the fallback branch when resume fails, the partial file is not explicitly cleared before starting a full download. Confirm that truncating the file via File::create (thereby discarding the partial data) is the desired behavior.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. src-tauri/src/core/utils/download.rs:288
  • Draft comment:
    When making a range request in _get_maybe_resume, the code strictly expects a 206 Partial Content response. Some servers might return 200 OK even when a Range header is sent. Consider handling such cases or documenting this limitation.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment points out a real edge case in HTTP range requests. Some servers might not properly implement range requests and return 200 OK with full content instead of 206 Partial Content. However, this would likely be rare with modern servers. The current behavior of failing fast is actually safer than trying to handle 200 responses, which could lead to corrupted downloads if we tried to append full content to a partial file. I might be underestimating how common non-compliant servers are. The current error handling could lead to failed downloads that could have succeeded with more lenient status code handling. While non-compliant servers exist, failing fast is better than risking corrupted downloads. The current implementation follows the HTTP spec correctly and has clear error messages. Delete the comment. The current strict behavior is actually better for reliability and correctness.
4. src-tauri/src/core/utils/download.rs:258
  • Draft comment:
    The progress update interval has been increased to 10 MB. Verify that this interval meets user expectations, particularly for smaller downloads where progress feedback might be less frequent.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_FjUOGGKMTZFwS1Cn

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@louis-menlo louis-menlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gau-nernst gau-nernst merged commit 6415be9 into release/v0.5.18 May 27, 2025
17 checks passed
@gau-nernst gau-nernst deleted the thien/download_resume branch May 27, 2025 02:46
@github-project-automation github-project-automation bot moved this to QA in Jan May 27, 2025
@github-actions github-actions bot added this to the v0.5.18 milestone May 27, 2025
@david-menloai david-menloai moved this from QA to Done in Jan Jun 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants