fix(coprocessor): honor HTTP/2 keep-alive over Unix domain sockets#9785
Open
theJC wants to merge 4 commits into
Open
fix(coprocessor): honor HTTP/2 keep-alive over Unix domain sockets#9785theJC wants to merge 4 commits into
theJC wants to merge 4 commits into
Conversation
The coprocessor/subgraph UDS client was built with only pool_idle_timeout and http2_only, so experimental_http2_keep_alive_interval/_timeout were silently ignored for unix:// transports even though the TCP client honored them. Effect over UDS: a dead peer is still detected promptly via EOF, but a hung-but-open peer (long GC pause, deadlock, event-loop stall) was never detected, because idle pooled UDS h2 connections were not pinged. Stale connections were only discovered when a request rode them, so each request paid the full downstream timeout. Mirror the TCP client's keep-alive block (timer + interval + timeout + keep_alive_while_idle) on the Unix socket client so the same experimental_http2_keep_alive_* config applies to both transports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a regression test that spins up a Unix-socket server which completes the HTTP/2 handshake and then freezes (never polls the connection again, so the request and the client's keep-alive PINGs are never read or ACKed while the socket stays open, modeling a stalled coprocessor with no EOF). Two rstest cases: - with keep-alive: the client tears the connection down after ~interval + timeout and the request fails inside a bounded window. - without keep-alive: the same hung server leaves the request pending until the window elapses. The second case guards the regression property: because a unix:// request carried no keep-alive at all before the fix, the pre-fix client would leave the first case hanging too. It also documents that keep-alive — not some other client timeout — is what recovers from a hung UDS peer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
HTTP/2 keep-alive pings are an h2-only feature. Over a Unix socket the client only speaks HTTP/2 when it is forced on (`http2only`): unlike TCP there is no TLS/ALPN over UDS for the client to negotiate h2, so `enable` (the default) and `disable` fall back to HTTP/1.1 and the keep-alive pings never fire. Previously such a config was accepted silently, so an operator who set experimental_http2_keep_alive_interval for a unix:// coprocessor expecting hung-peer detection got none. Validate at config load and fail with an error that points at `experimental_http2: http2only`, matching the existing "reject bad UDS config" behavior in validate_coprocessor_url. The check covers the top-level url and every per-stage override, and only fires for unix:// URLs — TCP keep-alive with the default HTTP/2 config stays valid because ALPN can still negotiate h2 there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@carodewig -- could I ask you to take a look since you introduced the keep alive settings in #9056 ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
HTTP/2 keep-alive (
experimental_http2_keep_alive_interval/_timeout) was applied only to the TCP client inHttpClientService. The Unix-socket client was built with justpool_idle_timeout+http2_only, so keep-alive was silently ignored for coprocessors (and subgraphs/connectors) reached over aunix://URL.Over a Unix socket a dead peer is detected promptly via EOF, but a hung-but-open peer — one whose socket stays open with no EOF, e.g. during a stop-the-world pause or an event-loop stall — went undetected. Idle/in-flight pooled h2 connections were never pinged, so the router would wait on the stalled peer (up to the downstream timeout) instead of tearing the connection down.
Fix
apollo-router/src/services/http/service.rs: apply the same keep-alive block (timer + interval + timeout +keep_alive_while_idle) to the Unix-socket client as the TCP client, soexperimental_http2_keep_alive_*is honored on both transports.Guardrail
HTTP/2 keep-alive is an h2-only feature, and a Unix socket has no TLS/ALPN for the client to negotiate h2 — so
hyper-util's auto mode falls back to HTTP/1.1 unless HTTP/2 is forced on. That means keep-alive over a socket is only effective withexperimental_http2: http2only.apollo-router/src/plugins/coprocessor/mod.rs: reject at config load when keep-alive is set for aunix://coprocessor URL (top-level or any per-stage override) whileexperimental_http2is nothttp2only, with an actionable error. Previously that combination was accepted and did nothing. TCP keep-alive with the default HTTP/2 config stays valid (ALPN can still negotiate h2 there).Tests
services::http::tests::h2c_keep_alive_unix— a Unix-socket server that completes the h2 handshake then freezes (never acks pings, socket held open), modeling a stalled peer. Asserts keep-alive tears the connection down; the no-keep-alive case confirms the teardown is caused by keep-alive.plugins::coprocessor::test::tests— config-load coverage: reject for default/enable/disable+unix://(incl. a per-stage override), accept forhttp2only+unix://, and a TCP false-positive guard.All new tests pass;
cargo fmtandcargo clippyare clean.🤖 Generated with Claude Code
Scope & follow-up
The keep-alive fix in
service.rsapplies to every client that talks over a Unix socket — coprocessor, subgraph, and connector — since they shareHttpClientService.The new config-load validation that rejects keep-alive +
unix://+ non-http2onlyis currently coprocessor-only, because that plugin is where the URL and client config are validated together. Subgraph and connector configurations can express the same inert combination and are not yet guarded. Extending equivalent validation to subgraph/connectorunix://URLs is a sensible follow-up (separate config-validation sites); happy to do it here or in a follow-up PR depending on reviewer preference.