Retry KB transport failures once, and wrap Listable connection failures in KB::Error (1.3.0)

[fatbeard2]

Why?

About 0.1% of KB calls fail in the transport, in morning bursts, and a caller sees every one of them as an error. Most of these failures never reached KB, so a second attempt on a fresh connection is safe and very likely succeeds. Since the 1.0.0 rollout, Funnel's KB connect failures sit at exactly the 1s connect budget (Net::OpenTimeout), which is the safest case to retry.

• Basecamp: https://3.basecamp.com/3934852/buckets/27820160/todos/10192244455
• Investigation list: https://3.basecamp.com/3934852/buckets/27820160/todolists/10139583615
• Stacked follow-up (keep-alive connections) is based on this branch.

Changes

Retries (KB::RetryPolicy, faraday-retry in the connection stack). One retry by default. The decision uses the underlying Ruby error, not the Faraday class, because the net_http and persistent adapters wrap Net::OpenTimeout differently:

Failure	Retried for
Never sent: Net::OpenTimeout (TCP connect + TLS), ECONNREFUSED, EHOSTUNREACH, ENETUNREACH, EADDRNOTAVAIL, SocketError	every verb, POST included
Maybe sent: read/write timeout, ECONNRESET, EOFError, TLS error	GET and HEAD only
HTTP 4xx/5xx	never

spec/retry_policy_spec.rb pins the full error × verb matrix.

Open decisions from the todo, and what I chose:

• Write timeouts are treated as "maybe sent", so they are retried for GET only.
• Idempotent-by-contract writes are not retried on "maybe sent" failures. That includes PUT and DELETE. upsert and PetParent#merge! are PUTs whose second run changes KB state or fails.
• Retry count and backoff: 1 retry, 0.1s interval, randomized to 0.1–0.2s so a burst of callers doesn't retry in lockstep. Both are configurable: KB.config.request.retries (0 disables) and retry_interval.
• EOF/reset after the request was sent stays GET/HEAD only, by the same rule.
• Calls with their own read_timeout: (birthdays, 30s) are not retried on "maybe sent" failures, so their worst case isn't doubled. Never-sent failures still retry. This came from the agent review.

Latency envelope. Worst case is two attempts' worth of phase budgets plus the interval. A GET that read-times-out twice takes about 2 × (1 + 3 + 5)s with the defaults. A connect failure costs at most 2 × 1s. connected_health's circuit breaker sees one failure only after both attempts fail.

Instrumentation. Retries are reported on the call's own request.kb_client event as retries and retry_errors, e.g. ["Net::OpenTimeout"], through the Faraday request context. The Datadog subscriber tags them as kb.retries / kb.retry_errors. Both appear only on retried calls, so payloads of non-retried calls are unchanged. This matters for measurement: kb.client.request wraps all attempts, so kb.client.errors will count only calls that failed after the retry. A connect-phase attempt produces no http.request span, so the tag is the only trace of retried connect failures. A span with kb.retries and no error is a failure the retry absorbed.

Listable fix (separate commit). KB::Listable.all re-raised Faraday::ConnectionFailed raw, on purpose since 2021 (fbea43d), while every other model call wraps into KB::Error. So rescue KB::Error handlers, like connected_health's rescue_from, missed connection failures from PetParent.all(email:), Pet.all, Breed.all, Product.all and others. Since 1.0.0 that includes TLS-handshake timeouts. I checked Funnel, Global Admin and connected_health masters, and none of them rescues the raw class.

Released as 1.3.0. faraday-retry is now an explicit dependency. It was already in every consumer's bundle through Faraday 1.10 (1.0.3 / 1.0.4).

Checklist

• Tests added/updated
• README file updated
• Changelog updated
• Version file lib/kb/version.rb updated

How to test

Automated tests cover it. From the repo root:

docker compose run --rm kb bash -c "bundle install && bundle exec rspec && bundle exec rubocop lib spec"

Result: 213 examples, 0 failures; rubocop clean on lib and spec. CI is green.

• spec/retry_policy_spec.rb: the error × verb matrix.
• spec/client_retries_spec.rb: end to end through WebMock. It covers a GET connect timeout that retries and succeeds with the retry recorded on the event, a POST retried on connection refused, a POST not retried on a read timeout, no retry on a 503, giving up after one retry, and retries = 0.
• spec/instrumentation/datadog_spec.rb: kb.retries / kb.retry_errors on the span. The existing closed-port spec now also shows a real ECONNREFUSED on a real socket being retried.
• spec/models/concerns/listable_spec.rb: ConnectionFailed from .all becomes KB::Error.

In production, once a consumer bumps: operation_name:kb.client.request @kb.retries:* in Trace Explorer shows absorbed failures.

Upgrade Deployment Instructions

• No env vars or migrations. Consumers bump barkibu-kb ~> 1.3 with a conservative lock update.
• Any code rescuing Faraday::ConnectionFailed around .all must rescue KB::Error instead. None of our three consumers does.

🤖 Generated with Claude Code

[fatbeard2]

🤖 Agent review

The change is solid and does what it says. I checked it against faraday-retry 1.0.3, faraday-net_http 1.0.1 and Faraday 1.10.3. The net_http adapter wraps OpenTimeout, ECONNREFUSED and SocketError as ConnectionFailed and ReadTimeout/WriteTimeout as TimeoutError, and root_cause reads the original error through wrapped_exception. raise_error only raises plain ServerError/ClientError, which the retry matcher does not catch, so HTTP responses are never retried.

The request-context mechanism is thread-safe. build_request dups the options per request, and faraday-retry reuses the env across attempts, so the context reaches retry_block. The backoff numbers match the docs, and max: 0 stays 0.

The Listable change is correct. It does break code that rescues the raw Faraday class, which strictly calls for a major version, but the consumers were checked and the PR says so.

Points not tied to one line:
(a) retries and retry_interval are read once, when each client builds its memoized connection, like the timeouts. The README should say so.
(b) Errno::ETIMEDOUT is treated as maybe-sent. That is conservative, just not called out.

[fatbeard2]

🤖 [should-fix] The retry also applies to per-call read_timeout: overrides. The README's own request('birthdays', ..., read_timeout: 30) example can now take about 2 × 30s plus the connect/write budgets before it fails. Either skip the maybe-sent retry when read_timeout is overridden, or at least document the doubled worst case next to that example.

[fatbeard2]

🤖 [nit] faraday-retry is declared as a dependency but never required. The gem relies on Faraday 1.10's faraday.rb loading it. On Faraday 1.0–1.9, :retry resolves to the built-in Faraday::Request::Retry instead. Add require 'faraday/retry' or pin faraday >= 1.10.

[fatbeard2]

🤖 [nit] The comment says "only GET is retried", but MAYBE_SENT_VERBS also includes HEAD, as do the README and spec. Say "GET/HEAD".

[fatbeard2]

🤖 [nit] Assigning context replaces any context the connection-level request options already carry. Nothing sets one today, but merging costs nothing and keeps this safe later.

[fatbeard2]

🤖 [nit] get_metric || get_tag accepts either storage, so the spec doesn't pin it. Assert get_metric only, so a regression to a string tag, which breaks numeric facets, would fail the spec.

[fatbeard2]

🤖 Agent review addressed in f73bd4b: a call with its own read_timeout: is no longer retried on maybe-sent failures (never-sent still retry), faraday/retry is required explicitly, the request context is merged, the comments and README cover GET/HEAD, DELETE and when settings are read, and the Datadog spec pins kb.retries as a metric. Errno::ETIMEDOUT stays maybe-sent on purpose. 213 examples, rubocop clean.