feat(network): add resume support to network.download#517
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for resuming file downloads by adding a resume parameter to the plugin bridge adapter and file download service. When enabled, the service checks for an existing partial file and requests the remaining bytes using the Range header. The review feedback highlights two important improvements: handling the 416 Range Not Satisfiable status code gracefully when a download is already complete, and safely parsing the resume argument as a boolean to prevent potential runtime type errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| final response = await _fetchFollowingAllowedRedirects( | ||
| downloadUri, | ||
| isAllowed, | ||
| isRedirectAllowed, | ||
| rangeStart: existingBytes, | ||
| ); | ||
|
|
||
| final outFile = File(destPath); | ||
| await outFile.parent.create(recursive: true); | ||
| // 206 = השרת כיבד את ה-Range → צרף לקובץ קיים; 200 = כתוב מחדש | ||
| final shouldAppend = existingBytes > 0 && response.statusCode == 206; | ||
| final sink = outFile.openWrite( | ||
| mode: shouldAppend ? FileMode.append : FileMode.write, | ||
| ); |
There was a problem hiding this comment.
When resuming a download that is already fully completed (i.e., existingBytes is exactly equal to the total file size on the server), sending a Range: bytes=N- request will cause compliant servers to return a 416 Range Not Satisfiable status code.
Currently, this will result in a failed download exception. We can make this much more robust by intercepting the 416 response, parsing the Content-Range header to check the total file size, and if it matches existingBytes, gracefully treating the download as successfully completed.
final response = await _fetchFollowingAllowedRedirects(
downloadUri,
isAllowed,
isRedirectAllowed,
rangeStart: existingBytes,
);
if (response.statusCode == 416) {
final contentRange = response.headers['content-range'];
if (contentRange != null) {
final match = RegExp(r'bytes\s+(?:\*|\d+-\d+)/(\d+)').firstMatch(contentRange);
if (match != null) {
final totalLength = int.tryParse(match.group(1) ?? '');
if (totalLength == existingBytes) {
await response.stream.timeout(_stallTimeout).drain<void>();
return PluginFileDownloadResult(outFile.path, p.basename(outFile.path));
}
}
}
await response.stream.timeout(_stallTimeout).drain<void>();
throw Exception('שגיאה בהורדת הקובץ (416)');
}
// 206 = השרת כיבד את ה-Range → צרף לקובץ קיים; 200 = כתוב מחדש
final shouldAppend = existingBytes > 0 && response.statusCode == 206;
final sink = outFile.openWrite(
mode: shouldAppend ? FileMode.append : FileMode.write,
);| // תיקיית ההורדות. הנתיב חייב להיות בתוך תיקייה שהמשתמש אישר דרך | ||
| // ui.pickFolder — אותו גבול אבטחה של פעולות ה-fs. | ||
| final destPath = args['destPath'] as String?; | ||
| final resume = args['resume'] as bool? ?? false; |
There was a problem hiding this comment.
Using args['resume'] as bool? can throw a TypeError if the plugin passes a non-boolean value (e.g., a string or integer) for the resume parameter. It is safer to use args['resume'] == true to avoid runtime crashes.
| final resume = args['resume'] as bool? ?? false; | |
| final resume = args['resume'] == true; |
|
תודה על ה-PR! הכיוון נכון והצורך אמיתי (קבצי ZIP גדולים שנתקעים באמצע הורדה). עברתי על הקוד לעומק, וכרגע יש כמה נקודות חוסמות לפני מיזוג — בעיקר משום שכבר קיים בריפו מימוש resume בוגר שמטפל נכון בכל הקצוות שה-PR מפספס. 🔴 קריטי1. אין אימות
|
| מקרה | _downloadAsset הקיים |
ה-PR |
|---|---|---|
| 416 (קובץ שלם) | return; — מזהה שהקובץ שלם |
throw — כשל ❌ |
| 200 (אין Range) | מוחק חלקי ומתחיל מחדש | FileMode.write ✓ |
| 206 + Content-Range | מאמת serverStart == alreadyDownloaded |
append עיוור ❌ |
| בדיקת שלמות | alreadyDownloaded >= compressedSize |
אין ❌ |
עדיף לחלץ את הלוגיקה הקיימת לפונקציה משותפת, או לפחות להעתיק ממנה את הטיפול ב-416 וב-Content-Range.
🟡 נוסף
- אין טסטים — חובה בפרויקט. תשתית ה-
MockClientב-test/plugins/services/plugin_file_download_service_test.dartכבר מוכנה ומאפשרת לבדוק בקלות 206/200/416/Content-Range לא תואם. - footgun ב-destPath:
downloadToPathרגיל דורס; עםresume:trueהוא מצרף. אם נשאר בנתיב קובץ ישן/שונה → צירוף עליו = שחיתות. איןIf-Range/ETag שמקשר את החלקי ל-URL. - קל: ה-
Rangeנשלח גם לבקשות ה-redirect הביניים (מיותר, עדיף רק לבקשה הסופית); התיעוד בשורה ~117 ("קובץ קיים נדרס") כבר לא מדויק במצב resume.
מבחינת אבטחה אין רגרסיה — בדיקות ה-allowlist, ה-redirects וה-path validation לא נפגעו.
לסיכום: הרעיון טוב, אבל לפני מיזוג נדרש לפחות (1) טיפול ב-416 כהצלחה, (2) אימות Content-Range לפני append, (3) טסטים. ורצוי לאחד עם המימוש הקיים ב-empty_library_bloc במקום לשכפל. 🙏
Add esume parameter to downloadToPath and _fetchFollowingAllowedRedirects to support resuming interrupted downloads via HTTP Range requests. When esume: true is passed and a partial file exists at destPath: - Sends Range: bytes=N- header (N = existing file size) - If server returns 206 Partial Content: appends to existing file - If server returns 200 (Range ignored): overwrites from scratch - On failure: partial file is preserved for the next attempt Also update plugin_bridge_adapter.dart to extract esume from RPC args and forward it to downloadToPath. Motivation: large files (e.g. 164 MB ZIPs from GitHub Releases) can stall mid-download. Without resume, each retry starts from byte 0. With resume, retries continue from the last received byte, reducing total transfer time and making large-file downloads reliable despite transient CDN stalls.
palmoni5
force-pushed
the
feature/resume-download
branch
from
957c9ee to
cc04f2f
Compare
2 participants
Add
esume parameter to downloadToPath and _fetchFollowingAllowedRedirects to support resuming interrupted downloads via HTTP Range requests.
When
esume: true is passed and a partial file exists at destPath:
Also update plugin_bridge_adapter.dart to extract
esume from RPC args and forward it to downloadToPath.
Motivation: large files (e.g. 164 MB ZIPs from GitHub Releases) can stall mid-download. Without resume, each retry starts from byte 0. With resume, retries continue from the last received byte, reducing total transfer time and making large-file downloads reliable despite transient CDN stalls.