Raise default read timeout to 600s; map timeouts to BentoTimeoutError (#31)#35
Conversation
The Historical guide showed `foreach_record(client, DBN.TradeMsg; ...)`, a method that does not exist — the call raises a MethodError. The record type is inferred from `schema` (overridable via the `record_type` kwarg). Corrected the example and the surrounding prose. Fixes #32 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Long-range get_range queries spend well over 100s in server-side assembly before the first byte streams, so the old 100s default killed every multi-year pull. Read timeouts are deterministic for a given query shape, so they are no longer retried; HTTP.jl TimeoutError is mapped to a new BentoTimeoutError whose message names the remedy. Fixes #31 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90a784d024
ℹ️ 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".
| # Read timeout → BentoTimeoutError whether or not `f` started: a | ||
| # mid-body stall deserves the same actionable message as a | ||
| # pre-first-byte one. | ||
| e isa HTTP.Exceptions.TimeoutError && throw(BentoTimeoutError(c.timeout)) |
There was a problem hiding this comment.
When the callback passed to foreach_record performs an operation that raises HTTP.Exceptions.TimeoutError—for example, its own HTTP request—this unconditional mapping converts the callback's exception into BentoTimeoutError(c.timeout), incorrectly blaming the Databento stream and reporting its timeout value. Since consumed[] is already true while f(io) runs, consumer exceptions should be distinguished and rethrown while only timeouts originating from the stream are mapped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ce6748b. open_stream now records the exact exception object that escapes the consumer callback and only maps stream-originated TimeoutErrors to BentoTimeoutError; a consumer-raised TimeoutError propagates untouched. A mid-body stall of our own stream still maps correctly because HTTP.jl raises the TimeoutError from the HTTP.open frame (a different object than whatever escaped the consumer). Regression test added.
The guide only imports DatabentoAPI and Dates, so the record_type override examples referenced an undefined DBN name (review feedback). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
If the foreach_record callback itself throws HTTP.Exceptions.TimeoutError (e.g. it makes its own HTTP request), open_stream unconditionally mapped it to BentoTimeoutError, blaming the Databento stream and reporting the wrong timeout value (review feedback). Track the exact exception object that escapes the consumer and map only stream-originated timeouts; a mid-body stall of our own stream still maps correctly because HTTP.jl raises the TimeoutError from the HTTP.open frame, a different object than whatever escaped the consumer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # CHANGELOG.md
1 participant
Summary
Stacked on #34 (merge that first; this PR auto-retargets to
mainwhen its base branch is deleted).The default 100s read timeout killed every long-range
get_range— the gateway spends well over 100s on server-side assembly (continuous-symbol resolution + day-partitioned scans) before streaming the first byte, and the rawTimeoutError: Connection closed after 100 secondsgave no hint the client was the one giving up.DEFAULT_TIMEOUT100s → 600s (the value that worked in the field viaHistorical(timeout = 600)). It is an inactivity timeout, so a genuinely hung connection now takes up to 10 min to surface — called out in the CHANGELOG.BentoTimeoutError <: BentoError(exported):HTTP.Exceptions.TimeoutErrorfrom both the eagerrequestpath and the streamingopen_streampath is translated to it, with a message naming the remedy ("constructHistorical(timeout = ...)with a larger value or reduce the requested range").ConnectErrorand transient statuses (429/5xx) retry as before — guarded by a test.Historical,get_range),docs/src/api/errors.md, and a new troubleshooting section document the failure mode.Tests
Full offline suite passes (1826 tests, Julia 1.12.6). New coverage: timeout →
BentoTimeoutErrorwith no retry burned (eager + streaming paths), connect errors still retryable, showerror hint text, new default value.Fixes #31
🤖 Generated with Claude Code