Skip to content

fix: make timeout and total_timeout mean what they say - #5

Open
simonx1 wants to merge 1 commit into
obie:mainfrom
simonx1:fix/enforce-request-deadline
Open

simonx1 wants to merge 1 commit into
obie:mainfrom
simonx1:fix/enforce-request-deadline

Conversation

@simonx1

@simonx1 simonx1 commented Sep 18, 2026

Copy link
Copy Markdown

Three defects, one subject: how long a call can actually take.

timeout didn't bound the request. Only open_timeout and read_timeout were set. Net::HTTP's write timeout kept its own default — 60 seconds in the runtimes I checked — so a request whose body stalled on the way out ran twelve times longer than timeout: 5 promised, and past the 30s total_timeout as well. The TLS handshake had no limit of its own either.

Net::HTTP.new("example.com", 443).write_timeout   # => 60

Net::WriteTimeout was not in TIMEOUT_EXCEPTIONS, so the one failure that gap produces was classified as a plain TransportError and never retried, whatever retry_timeouts said. (It's a Timeout::Error < RuntimeError, so it was caught by the generic rescue StandardError and passed straight through.)

total_timeout was documented as a budget "across attempts and delays" but was only consulted around the sleeps. An attempt already in flight ran to its own timeout regardless, so the real worst case was the budget plus a full attempt.

The fix

  • Set all four Net::HTTP timeouts: open, ssl, read, write.
  • Add Net::WriteTimeout to TIMEOUT_EXCEPTIONS.
  • Give each attempt the smaller of timeout and the remaining budget, so one slow attempt cannot outlive the whole call. An attempt that would start with nothing left raises TimeoutError instead of running.
  • budget_exceeded? compares with >=: a delay landing exactly on the deadline has spent the whole budget, and the attempt after it would have had zero seconds.
  • Validate timeout: at construction — timeout: "5" used to fail inside Net::HTTP on the first request.

Handing an attempt its budget needs a way to tell the transport, so the contract grows an optional timeout: keyword. A transport that doesn't declare one is called exactly as before, so existing custom transports keep working — there's a test for that.

Tests

test/deadline_test.rb, 14 cases: budget clamping across attempts, the exact-deadline boundary, write-timeout retry and opt-out, backwards compatibility of the three-keyword transport, and a stubbed check that all four Net::HTTP timeouts are set. Suite green on 3.2.11 / 3.3.8 / 3.4.8.


One of a series from a security and API-coverage audit. Branches are independent, each off main.

🤖 Generated with Claude Code

Three defects, one subject: how long a call can actually take.

`timeout` only set open_timeout and read_timeout. Net::HTTP's write
timeout kept its own default, 60 seconds in the runtime here, so a
request whose body stalled on the way out ran twelve times longer than
`timeout: 5` promised, and past the 30s `total_timeout` as well. The TLS
handshake had no limit of its own either. Set all four.

Net::WriteTimeout was not in TIMEOUT_EXCEPTIONS, so the one failure the
missing write timeout produces was classified as a plain transport error
and never retried, whatever `retry_timeouts` said.

`total_timeout` was documented as a budget "across attempts and delays"
but was only ever consulted around the sleeps. An attempt already in
flight ran to its own `timeout` regardless, so the real worst case was
the budget plus a full attempt. Each attempt now gets the smaller of
`timeout` and the remaining budget, and an attempt that would start with
nothing left raises TimeoutError instead of running. The comparison is
`>=` now: a delay that lands exactly on the deadline has spent the whole
budget, and the attempt after it would have had zero seconds.

Handing an attempt its budget needs a way to tell the transport. The
contract grows an optional `timeout:` keyword, and a transport that does
not declare one is called exactly as it was before, so existing custom
transports keep working.

`timeout:` itself was unvalidated, so `timeout: "5"` failed inside
Net::HTTP on the first request rather than when the client was built.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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