Add HTTP/2 PING for broken connection detection.#49095
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a custom HTTP/2 PING keepalive handler installed on H2 parent channels to prevent middlebox idle-connection reaping and to detect broken connections. The handler sends PINGs at a configurable interval while idle and closes the channel after a configurable number of consecutive PING ACK timeouts. New system-property knobs are added to Configs, and a new CI stage (Cosmos_Live_Test_Http2NetworkFault) runs iptables-based fault-injection tests.
Changes:
- New
Http2PingHandler(idle tracking, PING send, ACK timeout, consecutive-failure close) and wiring viaReactorNettyClient.doOnConnected. - Four new
Configsproperties:HTTP2_PING_HEALTH_ENABLED,HTTP2_PING_INTERVAL_IN_SECONDS,HTTP2_PING_TIMEOUT_IN_SECONDS,HTTP2_PING_FAILURE_THRESHOLD. - New
Http2PingKeepaliveTest(manual-http-network-fault group), Maven profile, TestNG suite, matrix JSON, andtests.ymlstage withiproute2/iptablesinstall.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/Http2PingHandler.java | New handler implementing idle-based PING, ACK timeout tracking, consecutive-failure close. |
| sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/http/ReactorNettyClient.java | Adds doOnConnected that installs the PING handler on H2 parent channels; minor refactor to accessor variable. |
| sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/Configs.java | Adds 4 system properties + getters for PING enable/interval/timeout/failure-threshold. |
| sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/faultinjection/Http2PingKeepaliveTest.java | Two tests: idle PING keepalive and iptables-induced PING timeout closing the connection. |
| sdk/cosmos/azure-cosmos-tests/src/test/java/com/azure/cosmos/rx/TestSuiteBase.java | Adds manual-http-network-fault group to before/after suite annotations. |
| sdk/cosmos/azure-cosmos-tests/src/test/resources/manual-http-network-fault-testng.xml | TestNG suite for the new group. |
| sdk/cosmos/azure-cosmos-tests/pom.xml | New manual-http-network-fault Maven profile bound to the new TestNG suite. |
| sdk/cosmos/tests.yml | New Cosmos_Live_Test_Http2NetworkFault pipeline stage with iptables/tc pre-steps. |
| sdk/cosmos/live-http2-network-fault-platform-matrix.json | Matrix config for the new pipeline stage. |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@sdkReviewAgent |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
✅ Review complete (45:33) Posted 5 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Benchmark Results: HTTP/2 Ping Keepalive (Corrected)Branch: ReadThroughput (Document/Read)
WriteThroughput (Document/Create)
QuerySingle (Document/Query)
Delta Analysis (Feature vs Main)
Summary✅ All deltas within ±1.5% — no measurable performance impact from HTTP/2 ping keepalive.
|
|
/azp run java - cosmos - spark |
|
/azp run java - cosmos - kafka |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 similar comments
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
kushagraThapar
left a comment
There was a problem hiding this comment.
Re-approving on 5b70c7c. Pulled the delta from my prior approval at c7dc195 (11 commits) and the PR is in noticeably better shape — flagging what landed for posterity:
Closures from my prior review:
- ✅ M1 / M2 —
5aaf3e8eaddskillSwitchOff_clearsOutstandingPingStateandchannelInactive_cancelsPingTaskunit tests. Both target exactly the invariants I called out (stale-state clear on kill-switch flip, scheduled task cancellation on channel close). - ✅ H4 —
1835b558moves the HTTP/2 PING + thin-client flags out of the stage-levelAdditionalArgsinsdk/cosmos/tests.ymland into per-profile<systemPropertyVariables>inazure-cosmos-tests/pom.xml. The Compute matrix leg now routes to the regional gateway on :443 as intended — Maven-Dprecedence no longer forces every leg into thin-client. - ✅ H2 (partial) —
c6120154rewrites the Configs.java header comment so the "single missed PING round ≈ 3s, close after threshold ≈ 15s" narrative is internally consistent with theDEFAULT_HTTP2_PING_FAILURE_THRESHOLD = 5doc 10 lines below. The misleading "detected within 3s" line is gone.
New fixes during this round (caught by @xinlian12, not in my prior list):
- 🟢 Write-failure now counts toward close threshold (
b74b11697,Http2PingHandler.java:178-195). Previously a write failure only clearedpingOutstandingSinceNanoswithout incrementingconsecutiveFailures— so a channel stuck in a state where writes always fail butisActive()stays true (H2 codec rejecting frames, stalled flow-control, queuedClosedChannelExceptionnot propagated) would loop forever. Good catch; matching unit testwriteFailure_incrementsConsecutiveFailuresAndClosesAtThresholdcovers it. - 🟢 TOCTOU guard on
customHeaderCleaner(c6120154,ReactorNettyClient.java:194-205). The newget("customHeaderCleaner") == nullcheck could race with concurrentdoOnConnectedcallbacks; now wrapped intry { ... } catch (IllegalArgumentException ignored)matching the siblingexHandlerpattern. Safe and minimal. - 🟢 ACK-timeout cascade test (
c6120154,Http2PingHandlerTest:ackTimeout_incrementsConsecutiveFailuresAndClosesAtThreshold). The primary operational failure path (PING sent → no ACK withinpingTimeoutNanos→ after N timeouts close the channel) now has explicit coverage. Time is sidestepped viapingOutstandingSinceNanosreflection — clean approach.
Still-open suggestions (non-blocking, leaving these as you like):
- H1 — PR body §3 still claims "active-stream suppression via
numActiveStreams() > 0".grepstill finds zero refs inHttp2PingHandler. Either strike the claim or add the suppression — your call. - H3 —
ctx.close()in the failure path is still a region-failover signal once consumed downstream. Wrapping inHttp2PingTimeoutChannelClosedExceptionwould letWebExceptionUtilitydistinguish "we tore down the connection on purpose" from "remote slammed the door"; nice-to-have for diagnostics. - H5 — The
customHeaderCleaneridempotencyget(...) == nullguard (the pre-existing duplicate-add fix you added in this PR) still lacks an explicit one-line comment in the PR body / changelog. Now even more worth calling out sincec6120154added the matching TOCTOU catch.
Verified-negatives (still hold at HEAD):
@Sharableis intentional and safe for this stateless installer pattern.handlerRemoved/channelInactivecleanup is correct and now unit-tested.- Late-ACK guards still in place.
- No public API surface change (env vars + a
UserAgentFeatureFlagsbit, both internal).
CI: The two red legs (Spark Databricks integration and StrongSession_Strong_Tcp_Fast) are TCP / Spark legs — unrelated to HTTP/2 PING which is HTTP-only. Worth retrying or letting the on-duty triage them, but not a blocker.
Approving — ship it whenever 🚀
…GELOG
Addresses kushagraThapar PR review 4432203224:
- H2: Configs.java threshold comment no longer claims false alignment
with Rust SDK's http2_consecutive_failure_threshold (which is a
per-HTTP-request shard-health knob, not per-PING-ACK). Now explicitly
notes that peer HTTP/2 stacks (Hyper / .NET SocketsHttpHandler /
Go net/http) typically close on the first PING-ACK timeout, and that
Java's threshold of 5 is intentionally more tolerant.
- L8 + H5: CHANGELOG entry for PR 49095 now calls out:
* default-ON state
* all four COSMOS.HTTP2_PING_* tunable knob names
* the defensive concurrent-install hardening on the H2 parent
pipeline (customHeaderCleaner / Http2ParentChannelExceptionHandler)
No code semantics change; comments and changelog only.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lure Addresses kushagraThapar H3 review on PR Azure#49095 - when Http2PingHandler closes a connection after consecutive PING ACK failures, surface the close as a retryable transport failure so ClientRetryPolicy retries in the SAME region without marking the regional gateway endpoint unavailable. Production: - Http2PingHandler fires typed Http2PingTimeoutChannelClosedException through the pipeline before closing, so reactor-netty's response Mono surfaces the typed cause on any in-flight HTTP read. - New Http2PingCloseRewrapHandler installed alongside the PING handler rewraps NIO ClosedChannelException as Http2PingTimeoutChannelClosedException when fired after a PING-driven close (handles the post-close race where the exception travels the quiescent path). - ClientRetryPolicy gains a shouldRetryOnGatewayTimeout H3 branch that detects Http2PingTimeoutChannelClosedException (and substatus 10006) and routes to in-region retry without endpoint mark-down. - RxGatewayStoreModel stamps substatus 10006 (HTTP2_PING_TIMEOUT_CHANNEL_CLOSED) on the resulting CosmosException so diagnostics and retry policy identify the cause without unwrapping. - WebExceptionUtility.isWebExceptionRetriable returns true for the typed exception. - HttpConstants exposes the new substatus. Tests: - Http2PingKeepaliveTest restructured for Option B (in-flight read with iptables DROP on thin-client port). Verified in Docker end-to-end: Tests run: 1, Failures: 0; SAME_REGION=true, DIFFERENT_CONNECTION=true. - New ClientRetryPolicyHttp2PingCloseTest covers the retry-policy branch in isolation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@sdkReviewAgent |
…path The H3 close path fires on EITHER consecutive PING-ACK timeouts OR consecutive PING-send failures (the latter was added later for stalled H2 codec/flow-control). Several comments, the typed exception message, the Javadoc of Http2PingCloseRewrapHandler, the install-site comment in ReactorNettyClient, and the CHANGELOG entry still described only the ACK-timeout half. Update all six sites to mention both paths. Also corrects the stale 'per-request captor in ReactorNettyClient.onErrorMap' reference in Http2PingHandler -- the actual consumer of PING_TIMEOUT_CLOSED is Http2PingCloseRewrapHandler.channelInactive on each H2 child stream. Pure documentation cleanup: no behavioral change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…helpers The HTTP/2 PING keepalive change added a new Http2PingHealth bit (1 << 6) to UserAgentFeatureFlags. When both H2 and PING are enabled (the default), the runtime emits |F50 instead of the previous |F10, breaking userAgent assertions in CosmosDiagnosticsTest and UserAgentSuffixTest. Both helpers (generateHttp2OptedInUserAgentIfRequired, validateUserAgentSuffix) now compute the hex suffix dynamically from the runtime conditions in RxDocumentClientImpl.addUserAgentSuffix and Http2PingHandler.isPingHealthEffectivelyEnabled: when H2 is enabled, the Http2 bit is set; when the PING kill-switch is on AND interval > 0, the Http2PingHealth bit is OR'd in. Integer.toHexString(featureValue).toUpperCase(Locale.ROOT) matches the runtime UserAgentContainer.setFeatureEnabledFlagsAsSuffix output (|F10 for H2 only, |F50 for H2+PING). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Review complete (50:38) Posted 4 inline comment(s). Steps: ✓ context, correctness, cross-sdk, design, history, past-prs, synthesis, test-coverage |
…er robustness Addresses two of xinlian12's latest review comments on PR Azure#49095: 1. ClientRetryPolicy: clarify that the gateway-timeout retry path may cycle through preferred locations on multi-region accounts (via routeToLocation(failoverRetryCount, true) inside shouldRetryOnGatewayTimeout), not always stay on the same endpoint. The previous wording was accurate only for single-preferred-region accounts. The key invariant (no markEndpointUnavailableFor* call) remains correctly described. 2. Configs: defend the three HTTP/2 PING getters (getHttp2PingIntervalInSeconds, getHttp2PingTimeoutInSeconds, getHttp2PingFailureThreshold) against NumberFormatException from malformed user input. These getters execute inside the doOnConnected lambda in ReactorNettyClient, so a bad value like COSMOS_HTTP2_PING_INTERVAL_IN_SECONDS=1s would fail every new H2 connection. Mirrors the defensive pattern already used by getConnectionAcquireTimeout / getThinClientConnectionTimeoutInMs: try/catch with WARN log and fallback to default. Extracted to a private parseIntConfigOrDefault helper to avoid 3x duplication. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ity, not just parent-pipeline frames Follow-up to Azure#49095. Root cause: Http2PingHandler is installed on the parent (TCP) channel and only updated lastReadActivityNanos from parent-pipeline channelRead -- which sees PING ACK / SETTINGS / GOAWAY but NOT HEADERS / DATA frames. With reactor-netty's Http2MultiplexHandler in the parent pipeline, all application H2 traffic is demuxed onto per-stream child channels, so a connection serving thousands of QPS still looked idle to the PING handler, causing unnecessary PINGs and measurable P95 overhead under load. Fix: - New package-private AttributeKey<AtomicLong> LAST_CHILD_STREAM_READ_ACTIVITY_NANOS seeded on the parent channel in handlerAdded. - Http2PingCloseRewrapHandler (already installed in every child stream's pipeline) overrides channelReadComplete to stamp the parent attribute via accumulateAndGet(now, Math::max) -- once per read cycle, monotonic. - Http2PingHandler.maybeSendPing now uses effectiveLastReadActivityNanos = max(field, child attr) for both the idle-threshold check AND the outstanding-PING early-return: if child-stream reads arrived after an outstanding PING, clear pingOutstandingSinceNanos and reset consecutiveFailures (defense against middleboxes that drop PING-ACK but pass application H2 frames). - Names use lastReadActivity / read activity -- both signals are inbound reads, never writes. Tests: two new unit tests in Http2PingHandlerTest covering the suppression path and the outstanding-PING reset path; both bypass real time via reflection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
What
Adds an HTTP/2 PING keepalive handler that sends periodic PING frames on idle H2 parent channels to:
Timing parameters: PING interval=1s, ACK timeout=2s, consecutive-failure threshold=5 (worst-case eviction ≈ 12–15s). All three are tunable via
COSMOS.HTTP2_PING_*system properties / env vars. The choice ofthreshold=5is intentional — it trades a few extra seconds of detection latency for resilience against transient blips. Note: Rust'sconsecutive_failure_thresholdis a per-HTTP-request shard-health counter (not a per-PING-ACK count) and is not directly comparable.Why not reactor-netty's native
pingAckTimeout/pingAckDropThreshold?Reactor-netty 1.2.12+ exposes
pingAckTimeout(Duration)andpingAckDropThreshold(int). These have a fundamentally different purpose and timing model:Different definition of "idle"
From the reactor-netty docs:
This is a "verify before evict" mechanism -- it sends a PING only when the full
maxIdleTimehas elapsed (e.g. 60s), as a last-ditch check before pool eviction:Our handler: continuous proactive keepalive
Our
Http2PingHandlersends PINGs continuously at a fixed interval (default 1s) while idle, regardless ofmaxIdleTime:Summary
pingAckTimeoutHttp2PingHandlermaxIdleTime(e.g. 60s)maxIdleTimeDesign decisions
Parent channel installation -- PING is a connection-level frame (RFC 9113 sec 6.7). Handler is installed on the TCP parent channel, not H2 stream channels. Detected via
Http2MultiplexHandlerpresence in pipeline.Idempotent installation -- in reactor-netty 1.2.x
doOnConnectedfires on the parent'sState.CONFIGUREDevent only (not per-stream); child H2 stream channels emitSTREAM_CONFIGUREDseparately. The handler still resolves to the parent channel and uses a pipeline-name-basedinstallIfAbsentguard (consistent withHttp2ParentChannelExceptionHandler) as defense-in-depth in case the contract changes. Concurrent races are caught viaIllegalArgumentException(benign).Idle-based gating (no active-stream check) -- PING is sent only when
(now - lastActivityNanos) >= pingInterval.lastActivityNanosis refreshed on every inbound frame the parent pipeline sees inchannelRead-- which is PING ACKs, SETTINGS, and GOAWAY only (HTTP request/response frames are dispatched byHttp2MultiplexHandlerto child stream channels and do not surface here). It is also refreshed when a PING is sent, so consecutive PINGs naturally pace atpingIntervalapart. This is why nonumActiveStreams()inspection is needed: PING cadence is anchored to PING activity itself, not to request traffic.At-most-one outstanding PING --
pingOutstandingSinceNanosgates to 1 in-flight. On timeout, resets to 0 to allow next retry.Consecutive failure threshold -- default 5. Tolerates transient blips; on each timeout or write failure: increment counter, unblock next PING, close only at threshold. Each round is ≈
pingTimeout + tick-detection lag≈ 2.5s, so worst case is ≈ 12–15s with defaults. Tune lower viaCOSMOS.HTTP2_PING_FAILURE_THRESHOLDfor tighter detection. Counting write failures (not just ACK timeouts) closes a state-machine gap where a channel stuck with persistently failing writes butchannel.isActive()==true(e.g. H2 codec rejecting frames, stalled flow-control) would otherwise loop forever without ever closing.No sharding needed -- Rust SDK shards because hyper opens 1 connection per client. reactor-netty natively pools multiple H2 connections via
Http2AllocationStrategy.Runtime toggle --
Configs.isHttp2PingHealthEnabled()is re-checked on every periodic tick. When flipped tofalsethe tick becomes a no-op and clears any in-flight PING state, but the scheduled timer is kept alive so flipping it back totrueresumes PINGing on the same connection without needing a reconnect.H2-only registration --
doOnConnectedPING handler registration is insideif (isH2Enabled)branch to avoid per-connect overhead for HTTP/1.1 clients.Configuration
Each setting is resolved with the standard
System.getProperty -> System.getenv -> defaultchain (matching the existingHTTP2_ENABLEDfamily), so both forms below work.COSMOS.HTTP2_PING_HEALTH_ENABLEDCOSMOS_HTTP2_PING_HEALTH_ENABLEDtrueCOSMOS.HTTP2_PING_INTERVAL_IN_SECONDSCOSMOS_HTTP2_PING_INTERVAL_IN_SECONDS1COSMOS.HTTP2_PING_TIMEOUT_IN_SECONDSCOSMOS_HTTP2_PING_TIMEOUT_IN_SECONDS2COSMOS.HTTP2_PING_FAILURE_THRESHOLDCOSMOS_HTTP2_PING_FAILURE_THRESHOLD5Changes
Production (azure-cosmos):
Http2PingHandler--ChannelDuplexHandler: idle tracking, PING send, ACK timeout, consecutive failure threshold (counting both ACK timeouts and write failures), connection closeConfigs-- 4 PING configuration getters using theSystem.getProperty -> System.getenv -> defaultresolution chain (parity withHTTP2_ENABLED-family settings)ReactorNettyClient-- installs handler viadoOnConnectedon H2 parent channels (insideif (isH2Enabled)branch)Test (azure-cosmos-tests):
Http2PingKeepaliveTest.connectionClosedOnPingTimeout-- network fault test (manual-http-network-faultgroup). Runs in both themanual-http-network-fault(thin-client, port 10250) andmanual-http-network-fault-compute(Compute Gateway, port 443) Maven profiles;sudodetection lets it run as root in Docker and viasudoin CI.Http2PingHandlerTest-- unit tests for kill-switch state-clear (clearspingOutstandingSinceNanosandconsecutiveFailureswhen flipped off),channelInactivecleanup (cancels the scheduled task), and the write-failure path (incrementsconsecutiveFailuresand closes at threshold whenwriteAndFlushfails).CI:
Cosmos_live_test_http2_network_faultstage insdk/cosmos/tests.yml(Ubuntu-only,MaxParallel=1).PreStepsinstallsiptablesfor network fault injection.eng/pipelines/templates/stages/cosmos/live-http2-network-fault-platform-matrix.json-- two matrix entries (thin-client and Compute Gateway).-Dflags (HTTP2_ENABLED,HTTP2_PING_HEALTH_ENABLED,HTTP2_PING_INTERVAL_IN_SECONDS,HTTP2_PING_TIMEOUT_IN_SECONDS) are wired per-profile via<systemPropertyVariables>inazure-cosmos-tests/pom.xmlrather than via stage-levelAdditionalArgs. This prevents a CLI-Dfrom overriding each profile'sCOSMOS.THINCLIENT_ENABLEDvalue (SUREFIRE-121), which is what keeps the thin-client leg (THINCLIENT=true) and the Compute leg (THINCLIENT=false) actually distinct.Testing:
connectionClosedOnPingTimeoutWhat it proves: PINGs are actively flowing on idle connections, ACK timeouts are detected, and broken connections are closed.
Without PING probing,
iptables -j DROPwould go completely undetected --channel.isActive()stays true, no GOAWAY arrives, no TCP RST is sent. The test would time out. The fact that the handler detects and closes the connection is definitive proof that PINGs are being sent, ACKs are being tracked, and the consecutive failure threshold works.How it works:
Http2ConnectionConfigAPI)iptables -j DROPrule to silently discard all outbound TCP traffic to the Cosmos DB gateway portconsecutiveFailures=1, handler retriesconsecutiveFailures=2 >= threshold-- handler closes connectionLocal Docker test evidence
Http2PingKeepaliveTest#connectionClosedOnPingTimeoutwas run end-to-end in Docker (--cap-add=NET_ADMIN) againstthin-client-mr-bs-cifor both transports the test now covers. Per-test config:interval=1s,timeout=2s,failureThreshold=2, single-conn pool.Thin-client / Gateway V2 variant (
-Pmanual-http-network-fault, port 10250)Key log lines for the affected channel
378fbef3:Compute Gateway variant (
-Pmanual-http-network-fault-compute, port 443)Key log lines for the affected channel
b1d3461e(IP-scoped DROP to the resolved regional GW IP):Signal summary
Http2PingHandler installed on channel [id: 0x..., L:..., R:...], interval=1s, timeout=2s, checkEvery=500ms(DEBUG)PING #N sent on channel [id: 0x..., L:..., R:...](DEBUG)PING #Nover the same windowPING ACK not received for 2 consecutive attempts on channel [id: 0x..., ...] -- closing connection(INFO)initial != recovery(DIFFERENT_CONNECTION=true)Channel identifier format (full Netty
Channel.toString()includingL:local andR:remote address) matchesHttp2ParentChannelExceptionHandlerso PING events can be correlated with parent-channel exceptions and pool-eviction logs.