From 8d00e88167dccf45db1e2e92d2067bf09133a4ab Mon Sep 17 00:00:00 2001 From: zackees Date: Sun, 23 Aug 2026 13:00:30 -0700 Subject: [PATCH] fix(fetch): flush partial progress when a download connection drops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FastLED/fbuild#1370's resume path could lose the bytes it had already received, on the one path where keeping them is the entire point. `fetch_into_part` writes chunks to a `tokio::fs::File` and flushes after the read loop ends — but a dropped connection returned from *inside* the loop, skipping the flush. `tokio::fs::File` makes no promise to flush on drop, so the delivered bytes could go nowhere. The retry loop then stats a short `.part`, computes a stale `resume_from`, and asks the server for a range it already holds. The download still completes, so nothing looks wrong — it just costs a full extra round trip and re-transfers bytes the feature was written to avoid re-transferring. The body error is now recorded and returned after the flush rather than instead of it. This surfaced as `streaming_download_resumes_from_the_byte_offset_after_a_drop` failing on macOS runners with `left: 3, right: 2`. The exact count is the only assertion that can detect this: the resumed file is byte-identical either way, so every other check in that test passes while the resume quietly does nothing. Comment added at the assertion saying so, because loosening it to `>= 2` is the obvious-looking fix and it buries the bug. Co-Authored-By: Claude Opus 5 (1M context) --- .../fbuild-packages-fetch/src/downloader.rs | 24 +++++++++++++++++-- .../src/downloader_tests.rs | 9 +++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/crates/fbuild-packages-fetch/src/downloader.rs b/crates/fbuild-packages-fetch/src/downloader.rs index 64ecf373..c35bc589 100644 --- a/crates/fbuild-packages-fetch/src/downloader.rs +++ b/crates/fbuild-packages-fetch/src/downloader.rs @@ -619,15 +619,23 @@ async fn fetch_into_part( let mut last_report = Instant::now(); let mut last_pct: u32 = 0; + // Set when the body stops arriving. Recorded rather than returned, so the + // flush below still runs — see the comment there. + let mut body_error: Option = None; + loop { let chunk = match tokio::time::timeout(timing.chunk_read_timeout, response.chunk()).await { Ok(Ok(Some(chunk))) => chunk, Ok(Ok(None)) => break, - Ok(Err(error)) => return Err(DownloadAttemptError::Body(error)), + Ok(Err(error)) => { + body_error = Some(DownloadAttemptError::Body(error)); + break; + } Err(_) => { - return Err(DownloadAttemptError::BodyStalled { + body_error = Some(DownloadAttemptError::BodyStalled { filename: filename.to_string(), }); + break; } }; file.write_all(&chunk) @@ -664,6 +672,14 @@ async fn fetch_into_part( // Flush before the caller stats the file to decide whether this attempt // made progress — buffered bytes would read as a stall. + // + // FastLED/fbuild#1370: this has to run on the *failure* path too, which + // is the whole point. A dropped connection used to return from inside the + // loop above, skipping this; `tokio::fs::File` makes no promise to flush + // on drop, so the bytes that did arrive could vanish. The retry loop then + // stats a short `.part`, resumes from a stale offset, and asks the server + // for a range it already has — losing exactly the progress this feature + // exists to keep, on the one path where keeping it matters. file.flush() .await .map_err(|error| DownloadAttemptError::PartFile { @@ -671,6 +687,10 @@ async fn fetch_into_part( error: error.to_string(), })?; + if let Some(error) = body_error { + return Err(error); + } + // A short body is a dropped connection that happened to end on a chunk // boundary. Treat it as a retryable failure so the resume loop continues // rather than renaming a truncated archive into place. diff --git a/crates/fbuild-packages-fetch/src/downloader_tests.rs b/crates/fbuild-packages-fetch/src/downloader_tests.rs index 3e903d92..c36111ed 100644 --- a/crates/fbuild-packages-fetch/src/downloader_tests.rs +++ b/crates/fbuild-packages-fetch/src/downloader_tests.rs @@ -457,6 +457,15 @@ async fn streaming_download_resumes_from_the_byte_offset_after_a_drop() { RESUME_BODY, "the resumed file must be byte-identical to the source" ); + // Load-bearing, and deliberately exact. A third request means the resume + // asked for a range it already had — i.e. the bytes the first attempt + // delivered were lost and the retry started over. That is the failure + // FastLED/fbuild#1370 exists to prevent, and this count is the only thing + // that detects it: the file still ends up correct either way, so every + // other assertion here passes while the feature silently does nothing. + // + // It caught exactly that on macOS runners, where the unflushed `.part` + // read short. assert_eq!( request_count.load(Ordering::SeqCst), 2,