Reuse connections to KB (keep-alive) through a fixed persistent adapter (1.4.0)

[fatbeard2]

Why?

Every KB call opens a fresh TCP + TLS connection to the Heroku router. The connect phase is where our KB failures now show up: Funnel's 1s Net::OpenTimeouts since the 1.0.0 rollout. Reusing connections removes that phase from most calls. Noma already calls KB over pooled keep-alive connections (OkHttp defaults), and its baseline KB failure rate was about 13 times lower than Funnel's or connected_health's (0.010% vs 0.13%). The comparison is suggestive, not proof: Noma also differs in timeouts and traffic.

• Basecamp: https://3.basecamp.com/3934852/buckets/27820160/todos/10192244455 (the keep-alive item comes from the investigation list: https://3.basecamp.com/3934852/buckets/27820160/todolists/10139583615)
• Stacked on Retry KB transport failures once, and wrap Listable connection failures in KB::Error (1.3.0) #108 (retries). Review and merge that first; this PR's diff is only the keep-alive commit.

Changes

KB::PersistentAdapter subclasses faraday-net_http_persistent 1.2's adapter, over net-http-persistent 4.0.8. It is on by default. KB.config.request.keep_alive = false restores one connection per call, so there's a rollback without a gem pin.

One pool per process. Each model class memoizes its own KB::Client, about eight of them, all pointing at the same host. They now share a single Net::HTTP::Persistent, whose pool is keyed by host and port, so every model reuses the same sockets.

The stock adapter isn't safe to drop in. Each of these fixes has a real-socket spec:

• Timeout leak across threads. The stock adapter writes each request's timeouts onto the shared Net::HTTP::Persistent. A read_timeout: 30 override, as the birthdays job uses, would then apply to whatever another thread sends next. Timeouts now go on the checked-out connection.
• TLS connections dropped constantly. The stock adapter sets a cert store object per adapter instance, meaning per model client. Any change of store object makes net-http-persistent drop every TLS connection, so alternating model clients would never reuse one. SSL is now left to net-http-persistent's defaults, which are the same as Faraday's: VERIFY_PEER with the system CA store. KB clients pass no SSL options.
• Error classes. The stock adapter raises Faraday::TimeoutError for a connect timeout and a raw Net::HTTP::Persistent::Error for a refused connection or a down host. Both now become Faraday::ConnectionFailed wrapping the Net::OpenTimeout / Errno cause, exactly as with faraday-net_http. connected_health's breaker, KB::Error wrapping, the retry policy and the kb.client.errors widget see the same classes as today.

Idle timeout. KB.config.request.idle_timeout defaults to 30s. I measured the Heroku router against KB staging, on both kb-staging.barkibu.com and the herokuapp.com host: it closes an idle client connection after about 55s. If the router closes a connection first, Net::HTTP detects the EOF before reuse and reconnects; a spec covers this with a POST, which would not be retried. A close that races the next write in the same instant is not detectable. A GET then gets retried, and a POST fails as ConnectionFailed/EOFError. CI caught that race in the spec before the spec was given a realistic gap; see the README.

Deliberate differences from faraday-net_http: connect_timeout and idle_timeout are read once per process, when the shared pool is built, so call KB::PersistentAdapter.reset! after changing them at runtime. HTTP(S)_PROXY is not honoured.

Instrumentation. Each attempt reports "new" or "reused" into the request.kb_client event as connections. "reused" means the pool handed out an already-open connection, and a failure that never reached KB is always "new". The field is e.g. ["reused", "new"] for a reused connection that failed followed by a retry on a fresh one. The Datadog span tag is kb.connections. It gives a reuse ratio in production, and it shows whether failures come from fresh connections or from stale reused ones.

Other notes:

• Fork safety. net-http-persistent's pool is a ConnectionPool. On Ruby ≥ 3.1, connection_pool ≥ 2.4 drops its connections after fork, and consumers are on 2.4.1 (Funnel), 3.0.2 (Global Admin) and 2.5.5 (connected_health). net-http-persistent 4.0.8 accepts connection_pool from 2.2.4 up to < 4, so all three resolve.
• Datadog. The Net::HTTP tracer still produces http.request spans, because net-http-persistent calls Net::HTTP#request. On a reused connection, the connect phase simply doesn't happen.
• Stale connections and POST. A POST on a connection the router closed in the instant between Net::HTTP's EOF check and the write fails as a reset. That's "maybe sent", so it is not retried for POST. The 30s idle timeout under the router's 55s keeps this rare. The connections tag will show it if it happens: an error with reused.

Released as 1.4.0. New runtime dependencies: faraday-net_http_persistent ~> 1.2, which every consumer already bundles through Faraday 1.10, and net-http-persistent ~> 4.0.

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: 225 examples, 0 failures; rubocop clean on lib and spec. CI is green, and the keep-alive spec passed 20 of 20 runs locally on ruby:3.4.

spec/persistent_adapter_spec.rb runs against a small keep-alive HTTP server on a real socket (spec/support/keep_alive_server.rb), with WebMock disabled because its Net::HTTP patch opens a new socket per real request. It checks:

• three calls use one accepted TCP connection, reported as new, reused, reused;
• two different entity clients share one connection;
• writes reuse the connection;
• a read_timeout: 30 call never writes its timeout onto the shared object, and while it is in flight, another thread's stalled call gives up at the 1s global budget;
• a POST after the server closed the connection is sent on a fresh connection without a retry;
• ssl_generation stays 0 across model clients;
• the pool connects with connect_timeout;
• a new connection after idle_timeout;
• connection refused is a retried Faraday::ConnectionFailed wrapping Errno::ECONNREFUSED, labelled new, new.

Mutation check: restoring the stock configure_request/configure_ssl makes three of these fail, including pool sharing, even over plain http.

Before merge, it's worth a staging soak of one consumer. Check that kb.connections:reused dominates on operation_name:kb.client.request, and that Puma and Sidekiq boot cleanly with the shared pool.

Upgrade Deployment Instructions

• Merge and release Retry KB transport failures once, and wrap Listable connection failures in KB::Error (1.3.0) #108 (1.3.0) first; this PR is based on it.
• No env vars or migrations. Consumers bump barkibu-kb ~> 1.4 with a conservative lock update, which adds net-http-persistent 4.0.8.
• Rollback without a gem pin: KB.config.request.keep_alive = false in the app's KB initializer.

🤖 Generated with Claude Code

[fatbeard2]

🤖 Agent review

The adapter holds up against the upstream sources. The overridden private methods are the hooks that faraday-net_http 1.0.1 and faraday-net_http_persistent 1.2.0 actually call. Error normalization produces the same classes as net_http. The requests.zero? check runs before net-http-persistent increments the counter, and the fiber-local handoff is sound. SSL defaults match the claim.

Two caveats the PR leaves out:

• connection_pool's automatic after-fork reload needs Ruby >= 3.1, because it hooks Process._fork.
• A preloading parent that called KB before forking shares TLS sockets with its children. The child's reload sends close_notify on them, and the parent recovers via EOF detection.

The main problems are in the specs. The leak spec doesn't discriminate, the stale-connection spec is masked by the retry, and the TLS fix has no spec. There are also two unstated behaviour changes: connect_timeout is frozen at first use, and env proxies are ignored. Nothing blocking.

[fatbeard2]

🤖 [should-fix] Run one call after another, the stock adapter would also pass this: the leak is a race between threads. Interleave two threads, with thread A holding a read_timeout: 30 call open while thread B's stall fails at about 1s.

[fatbeard2]

🤖 [should-fix] A retry masks this one. If the EOF check loses the race, the GET gets retried and still passes. Assert no retry, or use a POST, which is not retried after a maybe-sent failure.

[fatbeard2]

🤖 [should-fix] The TLS/cert-store fix has no spec, so restoring super in configure_ssl would pass the suite. At least pin that ssl_generation stays 0 across calls from two clients.

[fatbeard2]

🤖 [should-fix] open_timeout and idle_timeout are read once, when the process-wide object is first built, so later config changes have no effect until reset!. Nothing covers timeouts on the default adapter any more. Apply open_timeout per attempt or document it, and add a spec.

[fatbeard2]

🤖 [should-fix] When Net::HTTP silently reconnects inside request, after the router's FIN or once keep_alive_timeout elapses, the attempt is still labelled reused. A connect failure there shows as reused plus OpenTimeout. Document what reused means, or detect the reconnect.

[fatbeard2]

🤖 [nit] Overriding net_http_connection drops HTTP(S)_PROXY/NO_PROXY handling. This is a silent difference from the net_http contract, so document it.

[fatbeard2]

🤖 [nit] Every call takes the MUTEX just to read a memoized ivar. Use @http || MUTEX.synchronize { @http ||= build_http }.

[fatbeard2]

🤖 Agent review addressed in 91fa83e (plus a merge of #108's review fixes). The specs now discriminate. The shared object never carries a call's read_timeout, two interleaved threads keep their own budgets, and a server-closed connection is detected before a POST is sent. ssl_generation stays 0 across model clients, and the pool connects with connect_timeout. As a mutation check, restoring the stock configure_request/configure_ssl makes three of them fail, including pool sharing, even over plain http. Never-sent failures are always labelled new. The meaning of reused, connect/idle timeouts being fixed per process, the proxy difference and fork behaviour on Ruby >= 3.1 are now documented. The mutex is off the hot path. 225 examples, rubocop clean.