fix(ruby): classify a read timeout as a network error, not a status error

Merged
JE
2mo ago
+134-16

From the SDK leak audit's Ruby pass, the one classification defect it turned up; the same shape is in fizzy-sdk and has its own PR there.

The failure. Faraday::TimeoutError < Faraday::ServerError, so single_request and single_request_raw, which rescue ServerError, ClientError first and Faraday::Error second, caught a stalled read on the status clause and handed it to handle_error, which has no status to map. The caller got a non-retryable ApiError reading Request failed (HTTP ) with http_status 0 and no cause, and the governed GET retry loop declined to retry it — the opposite of SPEC §6's network row (retryable, "connection failure, timeout, DNS") and §7's network-error retry. Faraday::ConnectionFailed and SSLError inherit from Faraday::Error directly and already reached the network path; only the read timeout was misrouted. No test caught it because WebMock's to_timeout raises Net::OpenTimeout, which the Net::HTTP adapter maps to ConnectionFailed, so the read-side class was never exercised. On main:

Faraday::TimeoutError → Basecamp::ApiError code=api_error status=0 retryable=false msg="Request failed (HTTP )" cause=nil
Net::ReadTimeout      → Basecamp::ApiError code=api_error status=0 retryable=false msg="Request failed (HTTP )" cause=nil

The fix. Both primitives name Faraday::TimeoutError ahead of the status clause and route it, with every other transport failure, through one transport_failure helper: a NetworkError — Request timed out for a timeout, Connection failed for the rest, matching the Go ErrNetwork / TypeScript network / Rust network_timeout classification — retryable, status-less, with the Faraday error as its cause, and reported to on_request_end the same way. A download hop 1 keeps its SPEC §9 shape: fixed message, no cause, raised outside the rescue so MRI's implicit cause is severed too. A 408 response stays on the status path, since Faraday raises it as a ClientError.

The tests. http_test.rb: a Net::ReadTimeout through the shipped adapter is a network, Request timed out, retryable, status-less NetworkError whose cause is the Faraday::TimeoutError, and reaches on_request_end with a nil status and the same error; a timeout is retried on a governed GET; ConnectionFailed keeps its shape; the raw POST path classifies the same and is not retried; a 408 response is still an ApiError with status 408. download_test.rb: a hop-1 read timeout is the severed Network error with no cause and no query in any rendering. The http_test.rb cases fail on main with Basecamp::ApiError where Basecamp::NetworkError is expected.

SPEC §9, separate docs-only commit. After #869, #871 and #872, "ordinary API requests are untouched: the projection is gated on the download flow" no longer describes the rule. The sentence now states what those PRs implement — the text is built from the URL the SDK issued, never the transport's rendering; origin and path on the API origin without userinfo, origin alone anywhere else; never query, userinfo or fragment — with each SDK's shape beside it: Rust's without_url, Swift keeping origin and path on every host, Kotlin's projected Ktor exceptions, and Go's ordinary path, which still keeps the gate.

Not done here: Faraday::NilStatusError (also < ServerError, raised only when an adapter yields a nil status) still lands on the status path. The shipped adapter cannot produce it.

Gates run locally: make rb-check (1519 runs, 0 failures; rubocop clean), make conformance-ruby (209 passed, 0 failed), make doc-constants-check.

Watching the review loop on this one through to convergence.

Summary by cubic

Fixes Ruby's read-timeout classification so a stalled read is now a retryable NetworkError instead of a non-retryable ApiError with no status and no cause.

  • Faraday::TimeoutError now matches before the status clause and becomes "Request timed out," retried by the governed GET loop; other transport failures keep "Connection failed" with the Faraday error as cause.
  • Both single_request and single_request_raw share one transport_failure helper; download hop 1 keeps its projected fixed error and no cause, and a 408 response stays a status error.
  • Adapter-level tests exercise the read-side timeout classes that WebMock's to_timeout (which raises Net::OpenTimeout, mapped to ConnectionFailed) never reached.
  • SPEC.md §9 now states that the transport-error projection applies to every request, not just downloads, matching #869, #871, and #872.