fix(snapshots): reject mismatched resume Content-Range starts

[Kewe63]

Summary

Fixes #319

This fixes resumable snapshot downloads so a HTTP 206 Partial Content response is only appended to an existing .part file when the response Content-Range start offset matches the local partial file size.

Previously, the downloader sent:

Range: bytes=<existing_size>-

when a .part file existed, but it only used Content-Range to read the total size. It did not validate that the returned byte range actually started at <existing_size>.

That allowed a mismatched response such as:

Content-Range: bytes 0-3/11

to be appended to a 7-byte .part file requested with:

Range: bytes=7-

which could corrupt the resumed snapshot.

---

Changes

• Use reqwest's CONTENT_RANGE header constant instead of a string literal.
• Add Content-Range start parsing for 206 responses.
• Reject resumed 206 responses when the Content-Range start does not match the local .part file size.
• Add a regression test covering a mismatched Content-Range start.
• Verify the rejected response does not append mismatched bytes to the existing .part file.

---

Tests

Regression test failed before the fix:

cargo +1.94.0 test -p arc-snapshots resumable_download_rejects_mismatched_content_range_start -- --nocapture

Failure before fix:

mismatched Content-Range should not append to the existing .part file

After the fix:

cargo +1.94.0 test -p arc-snapshots resumable_download_rejects_mismatched_content_range_start -- --nocapture

Result:

1 passed; 0 failed

Related resumable download tests:

cargo +1.94.0 test -p arc-snapshots resumable_download -- --nocapture

Result:

11 passed; 0 failed

Full arc-snapshots package tests:

cargo +1.94.0 test -p arc-snapshots

Result:

108 passed; 0 failed

Formatting and lint:

cargo +1.94.0 fmt -p arc-snapshots --check
cargo +1.94.0 clippy -p arc-snapshots --all-targets -- -D warnings

Result:

passed

---

Notes

There is an existing open PR #139 touching crates/snapshots/src/download.rs, but it handles a different resume edge case: treating a fully downloaded .part file as complete when the server returns 416 with a matching total size. This PR addresses #319 specifically: rejecting mismatched 206 Content-Range start offsets before appending to .part files.

---

Checklist

• Tests pass — 108/108 full package, regression test confirmed failing before fix
• cargo fmt / cargo clippy clean
• Follows Conventional Commits
• Changes scoped to this fix only

---

Risk & Impact

Low. The validation only rejects the specific mismatched-offset case — a correctly-aligned 206 response (Content-Range start matching the local .part size) is unaffected. Verified the regression test fails against the old behavior and passes with the fix, confirming it exercises the actual corruption path.

Type: 🐛 Bug fix
Fixes: #319

[osr21]

Reviewed at c93a4f3. I don't have a Rust toolchain in this environment, so this is a source review rather than a re-run of your test commands — I've flagged below where that limit matters. The fix is correct and does exactly what #319 asked for. One behavioural consequence is worth resolving before merge, and it isn't visible from the test as written.

What's right

• parse_content_range_start correctly extracts the start from bytes <start>-<end>/<total>.
• The guard is gated on is_partial && existing_size > 0 — the identical predicate that open_part_file(part_path, is_partial && existing_size > 0) uses to decide append-vs-truncate. Validating on exactly the condition that enables appending is the right invariant, and it means the 200-ignores-Range path and the existing_size == 0 path are provably untouched. No regression surface.
• Running the check before parse_total_size means a mismatched response is rejected before any file handle is opened.
• The CONTENT_RANGE constant swap is a real if small improvement.

Blocking: rejecting without clearing the .part turns self-healing corruption into a permanent wedge

On mismatch the function returns Err and leaves the 7-byte .part and its marker on disk. Following that through the call chain:

1. resumable_download's retry loop re-reads existing_size from that same file at the top of every iteration, so all ten attempts (MAX_DOWNLOAD_RETRIES = 10, RETRY_BACKOFF_SECS = 5) send the identical Range: bytes=7- to the same misbehaving cache. That's ~45s of backoff spent re-asking a question already answered.
2. When the loop gives up, nothing cleans up. force_download_and_extract_both only calls remove_dir_all(pair.tmp_dir) on extraction failure. download_archive's doc comment makes this explicit and deliberate: a failed download must leave the .part behind so a later run resumes "instead of transferring tens of gigabytes again."
3. On the next invocation, prepare_partial_download finds a matching url_identity marker and returns early — keeping the .part. Resume from 7 bytes, same rejection.

So the operator is stuck permanently until someone manually deletes the .part, and the error message doesn't hint that this is the remedy.

Compare the pre-fix behaviour I traced on #319: the corrupt append produced a bad archive, extraction failed, remove_dir_all(tmp_dir) fired, and the next run started clean. Silent corruption — but self-healing.

This PR removes a correctness bug and installs an availability bug in its place. That's a net improvement (a wedged download beats a corrupted datadir), but it's avoidable. #319 offered two options and this takes option 1; option 2 costs one line and has neither failure mode:

if range_start != existing_size {
    let _ = std::fs::remove_file(part_path);
    return Err(eyre::eyre!(
        "Server returned Content-Range starting at {range_start}, expected {existing_size}; discarding partial download"
    ));
}

Because attempt_download re-reads existing_size from disk on entry, attempt 2 then sees 0, sends no Range header, receives a plain 200, and open_part_file(.., false) truncates. The download recovers inside the same run, with no operator intervention. Keeping the marker is correct here — the URL hasn't changed, and the part is simply rebuilt from zero.

The test can't see any of this

It calls attempt_download directly, unlike all eleven sibling resumable_download_* tests which go through run_resumable_download. Three consequences:

• .expect(1) only holds because the retry loop is bypassed. Through the real entry point the mock would receive ten requests — itself worth asserting.
• It cannot observe the .part lifecycle across attempts, which is precisely what this change affects.
• Snapshot resume should reject mismatched Content-Range starts before appending to .part files #319's own suggestion was to "assert that run_resumable_download(...) returns an error," so this is narrower than the issue proposed.

The single-attempt test is a good unit test of the parse and should stay. Worth adding one alongside it that drives run_resumable_download and asserts the end state of the .part — that test is what would distinguish option 1 from option 2 above.

Correcting the note about #139

The description says #139 touches the same file but handles a different edge case. Semantically true, but I tested the merges rather than assuming, and the mechanics matter for whoever lands these:

merge	result
#325 alone onto current main (97f8da0)	clean
#139 alone onto current main	conflict
#139 on top of main + #325	conflict

#139 already conflicts with main on its own — its merge base is a85368c0, well behind — so it needs a rebase regardless and nothing here is #325's fault. Worth stating plainly so this PR doesn't get blamed for it.

That said, the two do collide textually. The import change auto-resolves because both PRs make a byte-identical edit, but both then rewrite the body of parse_total_size in different ways, and that conflicts:

<<<<<<< HEAD          (#325: inlined, using CONTENT_RANGE)
=======
        parse_content_range_total(response.headers())   (#139: extracted helper)
>>>>>>> pr139

If both land you end up with parse_content_range_total(&HeaderMap) and parse_content_range_start(&Response) side by side — two near-duplicate parsers of the same header with different signatures. A single parse_content_range() -> Option<ContentRange { start, end, total }> would serve both and remove the conflict entirely. Worth a word with #139's author about who absorbs it.

Also worth noting the two are genuinely complementary: #139's 416 handling covers the case where the .part is already complete, which is a neighbouring branch of the same resume logic.

Carried over from #319 (not blocking, scope discipline here is good)

Listing only because they live in the lines this PR touches, so they're cheap to fold in if a maintainer wants them:

• Content-Range: bytes 7-10/* is legal (RFC 9110 §14.4, complete-length may be *). The new start check passes, then parse_total_size does "*".parse::<u64>() → None → "Server did not provide Content-Length or Content-Range header", which is false — it did.
• strip_prefix("bytes ") is exact on case and spacing; range units are case-insensitive tokens per RFC 9110.
• An inverted range like bytes 7-3/11 still validates, since only the start is compared.
• The final .part length is still never compared against total before rename.

Good, tightly-scoped fix — the remove_file line is the one thing I'd want before it merges.