Feature/resumable http by JustinLoye · Pull Request #76 · bgpkit/oneio

JustinLoye · 2026-07-15T05:52:14Z

Closes #74

Summary

Remote HTTP(S) transfers now recover transparently from mid-stream connection
drops. Previously, if a server closed the connection before the full body was
received, the read (or download) failed and the caller had to restart from
scratch. This PR adds a ResumableHttpReader that tracks the byte offset and,
on an unexpected connection drop, reconnects with a Range: bytes={offset}-
request and continues from where it left off. Upper layers (decompressors,
line readers, file writers) see a single contiguous stream and are unaffected.

Changes

Add ResumableHttpReader (src/resumable_http.rs): wraps a
reqwest::blocking::Response, implements std::io::Read, and transparently
reconnects via Range requests when the underlying stream errors mid-read.
- Retries are bounded (MAX_RETRIES = 5) with exponential backoff, so a
  server that repeatedly fails to make progress causes the read to error out
  rather than loop forever.
- Resumed responses are validated before use to avoid splicing mismatched
  data into the stream:
  - The Content-Range start offset must equal the current read offset.
  - Last-Modified (when the original response provided it) must match on
    the resumed response.
  - 416 Range Not Satisfiable at the end of a stream is treated as EOF;
    a 200 OK or any non-206 reply to a Range request is treated as
    unsupported and surfaces the original error.
Wire the wrapper into the streaming read path (get_reader_raw in
src/client.rs) so get_reader, read_to_string, read_lines, etc. all
benefit.
Wire the wrapper into the download path (download in src/client.rs) so
download and download_with_retry resume mid-transfer instead of only
retrying from byte 0.

Testing

Added unit tests in src/resumable_http.rs that script exact HTTP responses
over a raw TcpListener and simulate connection drops:

no_drop — normal read with no resume needed.
drop_resume — server drops mid-body; reader resumes and returns the full
content.
download_resumes_after_drop — end-to-end through download(); the file
written to disk contains the complete content after a mid-body drop.
range_not_supported_is_err — non-206 reply to a Range request errors.
range_oob_is_eof — over-declared Content-Length forces an out-of-range
resume that returns 416, treated as EOF.
new_last_modified_is_err — resumed response with a different Last-Modified
is rejected.
missing_last_modified_on_resume_is_err — resumed response omitting
Last-Modified is rejected.
max_retries_exhausted_is_err — repeated no-progress 206 responses stop after
exactly MAX_RETRIES attempts.
no_data_206 — empty-body 206 responses do not loop and error out.

Commands run locally:

cargo fmt --check
cargo test --features https
cargo clippy --features https --lib -- -D warnings

Checklist

Tests pass with all feature combinations (verify --all-features in CI)
CHANGELOG.md updated
Documentation updated (doc comments on the new module)

Copilot

Pull request overview

Adds resilient HTTP(S) streaming by introducing a resumable reader that can recover from mid-stream connection drops using HTTP Range requests, and wires it into the main read and download paths in OneIO.

Changes:

Introduce ResumableHttpReader (src/resumable_http.rs) that reconnects and resumes reads via Range: bytes={offset}-, with validation of Content-Range and Last-Modified.
Integrate resumable behavior into OneIo::get_reader_raw() and OneIo::download() for HTTP(S) paths.
Document the user-facing behavior in CHANGELOG.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/resumable_http.rs	New resumable HTTP reader implementation plus unit tests simulating dropped connections and Range behavior
src/client.rs	Wrap HTTP(S) raw readers and download streams with `ResumableHttpReader`
src/lib.rs	Feature-gated module registration for `resumable_http`
CHANGELOG.md	Notes new resumable HTTP(S) read/download behavior under Unreleased

+        for attempt in 1..=MAX_RETRIES {
+            let backoff_ms = BASE_RETRY_DELAY_MS.saturating_mul(1u64 << attempt.min(4));
+            std::thread::sleep(std::time::Duration::from_millis(backoff_ms));
+
+            let resp = match self
+                .client
+                .get(&self.url)
+                .header("Range", format!("bytes={}-", self.offset))
+                .send()
+            {
+                Ok(resp) => resp,
+                // Couldn't reach the server — back off and try again.
+                Err(_) => continue,
+            };


JustinLoye · 2026-07-15T09:29:08Z

+                std::io::copy(&mut reader, &mut writer)?;
                Ok(())


@digizeph if the AI raised a valid concern you might want to do the same for the ftp feature

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

+                let response = self.get_http_reader_raw(remote_path)?;
+                let mut reader = crate::resumable_http::ResumableHttpReader::new(
+                    self.http_client().clone(),
+                    remote_path.to_string(),
+                    response,
+                );
+                std::io::copy(&mut reader, &mut writer)?;


+
+            let (mut stream2, _) = listener.accept().unwrap();
+            let req = read_request(&mut stream2);
+            assert!(req.contains("range: bytes=5-"));


+            // server reports 416 which the reader must treat as a clean EOF.
+            let (mut stream2, _) = listener.accept().unwrap();
+            let req = read_request(&mut stream2);
+            assert!(req.contains("range: bytes=10-"));


+
+            let (mut stream2, _) = listener.accept().unwrap();
+            let req = read_request(&mut stream2);
+            assert!(req.contains("range: bytes=5-"));


+
+            let (mut stream2, _) = listener.accept().unwrap();
+            let req = read_request(&mut stream2);
+            assert!(req.contains("range: bytes=5-"));


+
+            let (mut stream2, _) = listener.accept().unwrap();
+            let req = read_request(&mut stream2);
+            assert!(req.contains("range: bytes=5-"));


…name

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

JustinLoye · 2026-07-15T09:39:43Z

+fn parse_content_range_start(value: &str) -> Option<u64> {
+    // "bytes 5-9/10" -> "5-9/10" -> "5"
+    let range = value.strip_prefix("bytes ")?;
+    let start = range.split_once('-')?.0;
+    start.trim().parse().ok()
+}


this is actually true, will do
https://www.rfc-editor.org/rfc/rfc9110.html#section-14.1-4

JustinLoye · 2026-07-15T09:49:11Z

+            return match resp.status() {
+                // Offset is at or past end of file.
+                reqwest::StatusCode::RANGE_NOT_SATISFIABLE => Ok(Resume::Eof),
+                // Server honored the Range request.
+                reqwest::StatusCode::PARTIAL_CONTENT => {
+                    self.accept_resumed_response(resp)?;
+                    Ok(Resume::Resumed)
+                }
+                // Anything else means the Range request was ignored.
+                _ => Ok(Resume::Unsupported),
+            };


A connection dropped before any bytes are successfully delivered is a niche case.
A server replying to a range request with a 200 is also a niche case.
In my opinion it's nitpicking, but the proposed fixed is short... so is it worth keeping it @digizeph?

JustinLoye · 2026-07-15T09:53:33Z

+}
+
+#[cfg(test)]
+mod test {


digizeph · 2026-07-22T22:40:53Z

Thanks for putting this together, and for working through the Copilot feedback so carefully. The implementation itself is solid, and the test suite scripting exact server behavior over raw sockets are all exactly what I wanted to see.

Before we can merge, though, there's one design point I want to change, plus two correctness issues in the resume logic.

This needs to be opt-in, not the default behavior.

Right now the PR wraps every HTTP(S) read in get_reader_raw and download with the resumable reader, which changes runtime behavior for every existing user on a routine version bump. oneio sits underneath data-ingestion pipelines (bgpkit-parser among them), and a connection drop today is a loud, retryable error at the application level, and after this change it becomes a transparent reconnect with spliced data. Most of the time that's what people want, but the decision should be theirs.

What I'd prefer: keep get_reader_raw and download exactly as they are, and expose the resumable reader as an additional API — e.g. a OneIoBuilder::http_resume(true) flag that opts the whole client in, and/or a dedicated get_resumable_reader constructor for the streaming path. Default stays byte-for-byte identical to current behavior. Your use case (concurrent streams with idle-connection drops) opts in explicitly.

Treating any 416 as EOF can silently truncate reads.

In resume(), 416 Range Not Satisfiable maps to Resume::Eof at any offset, and the range_oob_is_eof test locks that in. The problem: today, a server that over-declares Content-Length and then drops produces a hard incomplete-body error. With this change it produces a successful, silently truncated read — and the same happens if a misbehaving server or proxy returns 416 mid-stream for any other reason. For gzip input the CRC probably catches it downstream, but read_to_string/read_lines on plain text would accept partial data with no signal at all. In a data pipeline, silent truncation is much worse than a visible error.

I'd only treat 416 as EOF when the current offset is at or past the Content-Length of the original response (when it declared one). If offset is below that, or we don't know the total, a 416 should surface as an error.

Splice validation needs more than Last-Modified.

Right now the resumed response is validated against the original Last-Modified only, and only when the original response had one. Two gaps:

If the original response carried no Last-Modified, there is no validation at all — a resource that changed between the drop and the resume gets silently spliced into a single stream.
Last-Modified has one-second granularity, so a same-second replacement passes validation. If the server sends an ETag, comparing it would catch this (it's a strong validator), and many static-file servers do send one.
Could you add an ETag comparison when both responses provide one, and decide what to do when neither validator is present? My inclination is to still resume in that case (it's the common case for the servers this feature targets) but with a doc comment on the reader acknowledging the residual risk, so opting in is an informed choice.

None of this changes the core design, it's about how it's wired in and how conservative the validation is. Happy to discuss the API shape for the opt-in if you have a preference. Thanks again for the thorough work on this.

JustinLoye added 3 commits June 10, 2026 12:50

resumable http to handle dropped connections

296098e

cover and test more edge cases

cc1d1b1

make download() also use the resumable http reader

03ce583

Copilot AI review requested due to automatic review settings July 15, 2026 05:52

Copilot started reviewing on behalf of JustinLoye July 15, 2026 05:52 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

validate bytes is in content range header

3e3fdb6

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 15, 2026 06:01

Copilot started reviewing on behalf of JustinLoye July 15, 2026 06:02 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

JustinLoye added 3 commits July 15, 2026 17:17

remove backoff for first resume attempt and use reqwest range header …

4bbfbd2

…name

flush the writer before returning Ok in download()

1f9f908

more robust range request header check in tests

c8e9557

Copilot AI review requested due to automatic review settings July 15, 2026 09:24

Copilot started reviewing on behalf of JustinLoye July 15, 2026 09:24 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/resumable http#76

Feature/resumable http#76
JustinLoye wants to merge 7 commits into
bgpkit:mainfrom
JustinLoye:feature/resumable-http

JustinLoye commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

JustinLoye Jul 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

JustinLoye Jul 15, 2026

Uh oh!

JustinLoye Jul 15, 2026

Uh oh!

JustinLoye Jul 15, 2026

Uh oh!

digizeph commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

JustinLoye commented Jul 15, 2026

Summary

Changes

Testing

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

JustinLoye Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

JustinLoye Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

JustinLoye Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

JustinLoye Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

digizeph commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants