Reuse HTTP connections and add timeouts in Gitlab::HttpIO

Hordur Freyr Yngvasonrequested to merge
fix/http-io-persistent-connections into master

What does this MR do and why?

Gitlab::HttpIO reads remote files (archived CI traces and remote artifacts opened through GitlabUploader#open) from object storage in 128KB range GETs. Each chunk currently opens a brand-new Net::HTTP connection with the library defaults of 60s open/read/write timeouts. Reading a 64MB archived trace therefore opens 512 connections, each paying DNS + TCP + TLS setup, and a single unresponsive object storage endpoint can stall a high-urgency Sidekiq worker for up to 60s per chunk — invisibly, because labkit only instruments requests on already-started sessions (fixed in gitlab-labkit v2.8.1 via gitlab-org/ruby/gems/labkit-ruby!327 (merged); GitLab currently bundles v2.7.0).

Behind the new http_io_persistent_connections feature flag (gitlab_com_derisk, disabled by default), this MR changes Gitlab::HttpIO to:

  • Reuse one keep-alive Net::HTTP session per opened file. Net::HTTP transparently reconnects and retries the idempotent GET once if the server drops the keep-alive connection between chunks. max_retries is now set explicitly rather than inherited from the Net::HTTP default of 1, because the reconnect behavior described here and pinned by the specs depends on that value.
  • Set keep_alive_timeout to 30s. The Net::HTTP default is 2s, short enough that ordinary work between chunk reads — artifact parsing, trace processing, Sidekiq GVL contention — forces a reconnect and erodes the reuse this flag exists to measure. Object storage endpoints close idle keep-alive sockets well before 30s on their own; when that happens Net::HTTP reconnects and retries the idempotent GET.
  • Set explicit timeouts using Gitlab::HTTP::DEFAULT_TIMEOUT_OPTIONS (currently open 10s / read 20s / write 30s). The read timeout applies per socket read of a 128KB chunk, so it is generous. The worst case for an unresponsive endpoint drops from a silent 60s hang per chunk to a Net::OpenTimeout after 10s.
  • Set ignore_eof: false on the session. Net::HTTP's default (true) silently returns a truncated body when the connection dies mid-response despite a Content-Length header, and Gitlab::HttpIO would then cache the partial chunk against the full claimed Content-Range and silently truncate reads. This pre-existing bug (the per-chunk path has it too) now raises: Net::HTTP retries once on a fresh connection, and a persistent failure surfaces as Gitlab::HttpIO::FailedToGetChunkError — the class already used for an unexpected response code, so both failure modes of a chunk fetch share one class. The flag-disabled path is left unchanged and should get the same fix at flag cleanup.
  • Implement #close (previously a no-op) to finish the session. Every current caller closes the stream: through GitlabUploader#open's block form, or through its own ensure block (Gitlab::Ci::Trace#read_stream via Trace::Stream's delegated close, Gitlab::Lfs::Client#upload!, and LfsDownloadService#link_existing_lfs_object!). The session lifetime is therefore one open/read cycle.

With the flag disabled, behavior is unchanged: a fresh connection per chunk with Net::HTTP default timeouts.

Verification

Validated against a local keep-alive HTTP server serving 206 range responses for a 1 MiB file (8 chunks), counting TCP accepts vs HTTP requests:

Scenario Before After (flag enabled)
Well-behaved keep-alive server 8 requests over 8 connections 8 requests over 1 connection
Server drops socket after every 3rd request n/a 8 requests over 3 connections, read succeeds (Net::HTTP auto-reconnect)
2.5s idle gap between two chunk reads n/a 1 connection — with the Net::HTTP default of 2s this reconnected, giving 2
Server sends correct headers but dies mid-body silently truncated read (pre-existing ignore_eof default) retried transparently on a fresh connection, byte-correct result; a persistent failure raises FailedToGetChunkError
Timeouts on session open/read/write 60s (defaults) open 10s / read 20s / write 30s, keep-alive idle 30s

CI pins equivalent scenarios with specs that run against a real keep-alive server on localhost (spec/lib/gitlab/http_io_spec.rb, "connection lifecycle against a real keep-alive server"), asserting on TCP accept counts: single-connection reuse, reconnect when the server drops the socket, mid-body truncation recovery, and error surfacing followed by a successful retry on the same HttpIO instance. Those specs drop the connection after every response rather than every third, so their connection counts differ from the table above. Stubbed requests cannot cover any of this, because WebMock intercepts above the socket layer; the specs rely on WebMock's allow_localhost pass-through instead.

On GitLab.com, enabling the flag should eliminate per-chunk connection setup (DNS + TCP + TLS) for archived-trace reads and remove object-storage connect hangs as a contributor to the duration_s-only latency signature for Ci::BuildFinishedWorker. Note that external_http_count is unchanged: it counts chunk GETs, which are the same with or without connection reuse. The signature may not disappear entirely: other mechanisms also hide time from the duration breakdown (for example subprocess CPU during artifact decompression, #605510).

Manual smoke test

Reproduce the table above with $6018367, which reads a 1 MiB file through the real Gitlab::HttpIO in both flag states against a throwaway connection-counting local HTTP server (it toggles the feature flag in your development database and restores the original state):

bundle exec rails runner /path/to/http_io_connection_validation.rb

To smoke test against real object storage instead (GDK with MinIO, object_store.enabled: true):

# rails console
artifact = Ci::JobArtifact.where(file_store: ObjectStorage::Store::REMOTE).where('size > ?', 256.kilobytes).last

Feature.enable(:http_io_persistent_connections)
io = artifact.file.open                    # the flag is read here - re-open after toggling
io.read.bytesize                           # => artifact file size
io.instance_variable_get(:@http_session)   # => #<Net::HTTP ...> open_timeout 10 / read_timeout 20 / keep_alive_timeout 30
io.close                                   # finishes the session

Feature.disable(:http_io_persistent_connections)
io = artifact.file.open
io.read.bytesize
io.instance_variable_get(:@http_session)   # => nil - per-chunk block-form connections, as before
io.close

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist.

References

Edited by Hordur Freyr Yngvason

Merge request reports