fix(fetch): flush partial progress when a download connection drops

[zackees]

#1370's resume path could lose the bytes it had already received — on the one
path where keeping them is the entire point.

The bug

fetch_into_part writes chunks to a tokio::fs::File and flushes after the
read loop:

Ok(Err(error)) => return Err(DownloadAttemptError::Body(error)),   // <- drop
...
// Flush before the caller stats the file to decide whether this attempt
// made progress — buffered bytes would read as a stall.
file.flush().await?;

The comment is right about why the flush matters. It just wasn't reachable
from the case that matters: a dropped connection returns from inside the
loop, skipping it. tokio::fs::File makes no promise to flush on drop, so the
delivered bytes could go nowhere.

The retry loop then does:

resume_from = part_len(&part_path).await;

…reads a short .part, computes a stale offset, and asks the server for a
range it already holds. The download still completes — it just costs a full
extra round trip and re-transfers bytes the feature exists to avoid
re-transferring.

The body error is now recorded and returned after the flush rather than
instead of it.

How it surfaced, and the wrong fix I nearly shipped

streaming_download_resumes_from_the_byte_offset_after_a_drop started failing
on macOS runners:

assertion `left == right` failed: one dropped attempt plus one ranged resume
  left: 3
 right: 2

My first read was "the connection count is a reqwest pooling detail, loosen it
to >= 2". I had that committed. It is wrong, and worth writing down why:

the exact count is the only assertion that can detect this bug. The resumed
file is byte-identical whether the resume works or restarts from zero, so the
byte-identity check, the .part-cleanup check and the progress-monotonicity
check all pass while the feature silently does nothing. A third request is
the symptom.

I've added a comment at the assertion saying exactly that, since >= 2 is the
obvious-looking fix and it buries the defect.

Why macOS

Buffered-write timing. Nothing about the bug is macOS-specific — Linux and
Windows runners happened to get the bytes to the file descriptor before the
error surfaced. main's macOS check is currently green, so this is intermittent
rather than deterministic; it reproduced on two unrelated branches
(#1384, #1386) within an hour.

Verification

soldr cargo test -p fbuild-packages-fetch --lib — 138 passed, 0 failed, with
the strict == 2 assertion in place. clippy -D warnings clean.

[coderabbitai]

Warning

Review limit reached

Next included review available in 15 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4ad2c114-4e40-4526-8a8b-8bbfbf9fcefc

📥 Commits

Reviewing files that changed from the base of the PR and between 1a36249 and 8d00e88.

📒 Files selected for processing (2)

• crates/fbuild-packages-fetch/src/downloader.rs
• crates/fbuild-packages-fetch/src/downloader_tests.rs

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[zackees]

Swept the workspace for the same shape — a buffered writer whose flush sits after a loop that can return early — to check whether this was one instance or a pattern:

• fbuild-core/src/fs.rs (atomic write): correct. Captures write_res / sync_res, removes the temp file, and returns the error, so no path skips the sync.
• fbuild-cli/src/cli/usb_recovery.rs: std::fs (unbuffered) with an explicit sync_all chained onto the write.
• fbuild-deploy/src/esp32/image.rs: std::fs, same reasoning.
• No BufWriter anywhere in production code.

downloader.rs was the only instance. Recording the negative result so the next person doesn't have to redo the sweep.

[zackees]

Check (macos-latest) passed here with the strict == 2 assertion intact — the same check that failed on #1384 and #1386 against unfixed code.

Stating the strength of that honestly: the failure was intermittent (main's macOS check was green throughout), so one green run is consistent with the fix rather than proof of it. The load-bearing argument is the mechanism — unflushed buffer → short .part → stale resume_from → a range request for bytes already held — and that the flush is now reachable from the dropped-connection path it was written for.

[zackees]

Updating the evidence, since it is now considerably stronger than when I opened this.

Without this fix — four macOS failures, all identical (left: 3, right: 2):

• fix(test): block accepted sockets so Windows test servers stop flaking #1386 (socket fix) — first sighting
• feat(lnk): write build-time blob pointers as .fetch #1384 (.fetch) — twice, on separate runs
• chore(daemon): make daemon diagnostics name real paths, ratchet six sites #1387 (ratchet batch 3) — just now, run 32663598882

None of those four PRs touch fbuild-packages-fetch.

With this fix: Check (macos-latest) passed here.

I opened this describing the failure as intermittent, on the strength of main's macOS check being green. Four-for-four on branches against one green on main does not read as intermittent any more — it reads as reliably reproducible on macOS runners, with main's single green being the outlier worth explaining rather than the baseline.

That also sharpens why loosening the assertion to >= 2 would have been the wrong call: it would have permanently silenced a signal that was firing correctly, every time, on the exact platform where the buffered write was being lost.

[zackees]

Correction to my previous comment: I said the failure "does not read as intermittent any more." That overstated it. Checking properly, main is 8 consecutive green macOS runs (5ea78eb9 through 9f82da30), and check-macos.yml has no path filtering — those runs executed the same test.

The accurate picture is an asymmetry, not determinism:

• PR branches: 4 failures, 4 attempts (fix(test): block accepted sockets so Windows test servers stop flaking #1386, feat(lnk): write build-time blob pointers as .fetch #1384 ×2, chore(daemon): make daemon diagnostics name real paths, ratchet six sites #1387)
• main: 8 passes, 8 attempts

Same code both sides. The plausible difference is machine load — a PR run schedules ~94 checks including the full board-build matrix, main's runs are lighter — and this is a race between a buffered write reaching the file and the connection error surfacing. Heavier load widens the window.

That is still a real bug with a real fix; it just makes the failure load-dependent rather than platform-deterministic, and it explains main's clean record without needing main to be an outlier. It also means one green macOS run here remains supporting evidence rather than proof — the mechanism is still the load-bearing argument.

[zackees]

Post-merge confirmation, which is stronger evidence than anything available while this was open.

#1387 (ratchet batch 3) failed Check (macos-latest) with left: 3, right: 2 on every run before this landed. Rebased onto main with this fix in place, its macOS check now passes — on a branch that touches fbuild-daemon only and has nothing to do with the downloader.

That is the control I was missing earlier: my own green run here could have been the timing window closing by luck, but an unrelated PR flipping from reliably-red to green across exactly this merge is what the mechanism predicts.

Recap of the record, for anyone who finds this later:

	macOS Check
before the fix, on PR branches	4 failures / 4 runs (#1386, #1384 ×2, #1387)
before the fix, on main	8 passes / 8 runs — lighter runner load
after the fix	#1388 pass, #1387 pass