Skip to content

Historical HTTP retries, backpressured foreach_record, start_dt/end_dt rename#25

Merged
tbeason merged 2 commits into
mainfrom
feat/http-retries-and-time-range-rename
Jun 3, 2026
Merged

Historical HTTP retries, backpressured foreach_record, start_dt/end_dt rename#25
tbeason merged 2 commits into
mainfrom
feat/http-retries-and-time-range-rename

Conversation

@tbeason

@tbeason tbeason commented Jun 3, 2026

Copy link
Copy Markdown
Owner

Summary

Three changes, from the review/polish pass:

1. Automatic HTTP retries (Historical API)

request() now retries transient failures — HTTP 429 and 5xx, plus connection/timeout errors — with full-jitter exponential backoff, honoring a Retry-After header (capped). Configurable via a new max_retries keyword on Historical(...) (default 3); the retry budget, backoff, and sleep hook are injectable for testing. Once the budget is exhausted the final status still maps to BentoClientError/BentoServerError as before. This was the most visible gap vs. the official Databento clients.

2. foreach_record backpressure + deterministic teardown

The streaming path previously drained the connection into an unbounded Base.BufferStream on a background task — which could buffer the entire compressed payload in memory, and left the producer task running (blocking until server EOF/timeout) when the consumer aborted or finished early. It now reads the response body synchronously on the calling task, so records are pulled from the socket only as fast as the consumer processes them (real backpressure), and the connection is torn down on every exit path — including an early return or an exception out of the callback. Adds an injectable stream_opener seam so the streaming path is unit-testable (it had no HTTP-level test before).

3. Breaking: start / end_start_dt / end_dt

The old pair was asymmetric — the trailing underscore only existed because end is a Julia reserved word. Renamed across get_range, foreach_record, submit_job, get_record_count, get_billable_size, get_cost, and the Live subscribe! / live_session subscription / stream_to_file / stream_multi_to_files start keyword. Databento wire parameters (start/end) are unchanged.

Docs, README, benchmarks, and CHANGELOG updated. Adds retry + streaming test coverage.

Verification note (please read)

Local test verification was not completed cleanly: my machine runs Julia 1.13.0-beta3, and the async-TCP test_live_streaming.jl mock-gateway tests fail/hang there. Those tests run through the Live capture path, which none of these changes touch (the retry/open_stream work is Historical-only; the rename is keyword names). The package targets julia = "1.12" and CI runs 1.12, so CI on this PR is the source of truth. Flagging it rather than claiming a green run I didn't get.

🤖 Generated with Claude Code

…t rename

HTTP layer:
- Retry transient failures (HTTP 429/5xx + connection/timeout errors) with
  full-jitter exponential backoff, honoring Retry-After (capped). Configurable
  via the new max_retries keyword on Historical(...) (default 3); retry budget,
  backoff, and sleep hook are injectable for testing. Once the budget is spent,
  the final status still maps to BentoClientError/BentoServerError as before.
- Rewrite foreach_record/open_stream to read the response body synchronously on
  the calling task: real backpressure (no unbounded Base.BufferStream buffering
  the whole payload) and deterministic teardown on every exit path, including an
  early return or an exception out of the callback (no leaked producer task).
  Adds an injectable stream_opener seam so the streaming path is unit-testable.

API rename (breaking):
- Rename the time-range keyword arguments start/end_ -> start_dt/end_dt across
  get_range, foreach_record, submit_job, get_record_count, get_billable_size,
  get_cost, and the Live subscribe! / live_session subscription /
  stream_to_file / stream_multi_to_files start keyword. The trailing underscore
  only existed because `end` is a Julia reserved word; the pair is now symmetric.
  Databento wire parameters (start/end) are unchanged.

Adds retry + streaming test coverage; updates docs, README, benchmarks, CHANGELOG.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

start_initial = start, snapshot = snapshot,

P1 Badge Pass start_dt to the unified session

Every stream_multi_to_files call starts this @async block, but start is no longer a keyword or local variable after the rename to start_dt. This raises UndefVarError(:start) before any subscription is opened; the surrounding catch only logs the crash, so callers can get file paths back even though no live capture ran. Use the renamed start_dt value here, as stream_to_file does above.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/http.jl
- streaming.jl: `stream_multi_to_files` still passed `start_initial = start`
  (the 12-space-indented copy my earlier replace_all missed — it only matched
  the 16-space copy in `stream_to_file`). On the capture path this threw
  `UndefVarError: start not defined`, crashing the session mid-capture and
  causing the live-streaming round-trip test to fail/hang. Now `start_dt`.
- http.jl (open_stream): only retry transient transport errors in the
  pre-body phase. Once the consumer callback has started reading the body it
  may already have observed records, so a mid-body socket error must surface
  rather than replay the partial stream. Addresses Codex review (P2).

Verified: full offline suite passes locally (all 19 files, including
test_live_streaming, which previously hung/failed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tbeason

tbeason commented Jun 3, 2026

Copy link
Copy Markdown
Owner Author

Pushed 21aeb03 fixing the CI failure:

  • Root cause of the hang/failure: stream_multi_to_files still passed start_initial = start (a 12-space-indented copy my earlier bulk rename missed — it only caught the 16-space copy in stream_to_file). That threw UndefVarError: start not defined on the capture path, crashing the session mid-capture, which is what failed the live-streaming round-trip test. Now start_dt.
  • Codex P2 (open_stream): the retry now only fires in the pre-body phase; once the consumer callback has started reading the body, a transport error surfaces instead of replaying the partial stream.

Verified the full offline suite passes locally (all 19 test files, including test_live_streaming).

@tbeason
tbeason merged commit 71b8f8b into main Jun 3, 2026
8 checks passed
This was referenced Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant