Add transport failure and connect latency counters to node stats

[jbacchus126]

Description

_nodes/stats currently reports transport volume only — rx_count, rx_size_in_bytes, tx_count, tx_size_in_bytes, server_open, and total_outbound_connections. There is no always-available signal for transport failure or connection-open latency, which makes a class of network faults hard to diagnose on a running cluster: the volume counters keep incrementing normally while requests silently time out or connections take seconds to establish.

The connection-open path is the largest gap. A slow or hung connect blocks the cluster applier thread, and no existing metric observes it at all — total_outbound_connections is incremented only after a handshake succeeds, so a node struggling to open connections looks identical to a healthy idle node.

This PR adds six counters to TransportStats:

Field	Meaning
connect_failures	Outbound connection opens that failed or timed out
connect_time_millis	Cumulative time spent on connection opens that succeeded. Average open latency is connect_time_millis / total_outbound_connections
connect_time_millis_max	Worst-case single connection open since node start. Useful for judging whether lowering transport.connect_timeout is safe
channel_close_by_type	Individual channel sockets closed, keyed by channel type (RECOVERY, BULK, REG, STATE, PING, STREAM)
outgoing_timeouts	Outbound requests that received no response within their timeout
requests_failed_on_disconnect	In-flight requests cancelled because the connection closed before a response arrived

channel_close_by_type is the counter that distinguishes partial impairment from a whole-node failure. Because a connection profile assigns separate sockets per channel type, REG/BULK degrading while PING/STATE stay healthy is a materially different fault from a node going away, and today the two are indistinguishable from node stats.

These live in _nodes/stats rather than the telemetry metrics registry so they are available on any cluster with no plugin installed and no configuration, which is what makes them usable during an active incident. Publishing the same values through MetricsRegistry is a reasonable follow-up, along the lines of NodeRuntimeMetrics (#20844), which exposes JvmStats data as pull-based gauges while _nodes/stats remains the source of truth.

Where the counters are incremented

• TcpTransport.ChannelsConnectedListener — connect latency is measured from listener construction to successful handshake; closeAndFail increments connect_failures. The countDown.fastForward() guard on the failure and timeout paths means a single attempt cannot be counted twice.
• TcpTransport.NodeChannels — a per-channel close listener attributes socket closes to the channel types sharing that socket. The reverse index is derived from ConnectionProfile.ConnectionTypeHandle's own offset/length range, so attribution matches the channel that getChannel actually selects.
• TransportService.onConnectionClosed — counts pruned in-flight handlers.
• TransportService.TimeoutHandler — counts expired requests.

None of the counters sit on the per-request send path, so there is no added contention at request volume.

DEBUG logging

Also adds DEBUG logging for connect latency, send failures, and per-channel socket closes. NodeConnectionsService now reports elapsed time on both successful and failed connects, so failed to connect to {} after [Nms] distinguishes an immediate refusal from a full connect-timeout stall. Nothing is added at INFO, so production log volume is unchanged.

Sample output

GET _nodes/stats/transport:

"transport": {
  "server_open": 26,
  "total_outbound_connections": 143,
  "rx_count": 1043321,
  "rx_size_in_bytes": 884736102,
  "tx_count": 1043298,
  "tx_size_in_bytes": 901324887,
  "channel_close_by_type": {
    "RECOVERY": 0,
    "BULK": 12,
    "REG": 12,
    "STATE": 0,
    "PING": 0,
    "STREAM": 0
  },
  "outgoing_timeouts": 37,
  "requests_failed_on_disconnect": 214,
  "connect_failures": 6,
  "connect_time_millis": 8412,
  "connect_time_millis_max": 4903
}

Backwards compatibility

New fields are written and read only when the negotiated stream version is 3.8.0 or later, so mixed-version clusters and cross-cluster connections to older clusters are unaffected — older nodes exchange only the pre-existing counters and
report the new ones as zero.

This is purely additive to the _nodes/stats response; no existing field changes name, type, or meaning. An empty channel_close_by_type is omitted rather than rendered as an empty object, so stats built without it are byte-identical to the previous response shape.

TransportStats also gains a Builder(TransportStats) copy constructor. TransportService.stats() previously restated all twelve fields by hand in order to override two, which meant any field added later would be silently dropped there; it now seeds from the transport's own stats and overrides only the two request-level counters it owns.

Testing

• TransportStatsTests — xcontent rendering, round-trip, serialization to a pre-3.8.0 node, and deserialization from a pre-3.8.0 node (asserting the stream is fully consumed so no stray bytes are read), plus builder copy semantics and the empty-map omission guarantee.
• NodeStatsTests — the new counters are randomized and asserted across NodeStats round-trip.
• SimpleNetty4TransportTests and SimpleMockNioTransportTests exercise getStats() against a real transport.

All of the above were run at -Dtests.iters=20.

Related Issues

Resolves #[issue number]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit c11f2a6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue In NodeChannels.sendRequest, the original code retrieved channel from channel(options.type()) and then passed it to handshakerHandler.sendRequest. The new code wraps the send in a try/catch but no longer uses the local channel variable — handshakerHandler.sendRequest is called with channel still (verify), but the variable declaration TcpChannel channel = channel(options.type()); is retained. If channel is unused in the actual call (the diff shows the send passes channel), this is fine; however, only IOException is caught. If handshakerHandler.sendRequest throws a TransportException (declared on the method signature) or a RuntimeException, it will bypass the debug logging. Consider whether the narrow catch is intended, or whether TransportException/other failures should also be logged. TcpChannel channel = channel(options.type()); try { handshakerHandler.sendRequest(node, channel, requestId, action, request, options, getVersion(), compress, false); } catch (IOException e) { logger.debug("send [{}] failed with IOException on [{}] channel to [{}]", action, options.type(), node, e); throw e; } Race Condition connectTimeMillisMax is updated via updateAndGet(prev -> Math.max(prev, connectMillis)), but getStats() reads connectTimeMillis and connectTimeMillisMax independently without any synchronization. This is expected for counters, but note that connectTimeMillis.addAndGet(connectMillis) and the max update are non-atomic relative to each other, so a stats snapshot taken concurrently with a completing connect can show an updated max but not-yet-updated total (or vice versa). Low severity since these are advisory metrics, but worth noting for consumers computing connectTimeMillis / totalOutboundConnections. final long connectMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos); connectTimeMillis.addAndGet(connectMillis); connectTimeMillisMax.updateAndGet(prev -> Math.max(prev, connectMillis)); Potential Bug connectFailures is incremented in initiateConnection catch blocks and in ChannelsConnectedListener.closeAndFail. If initiateChannel fails partway through the loop (some channels opened, one fails), connectFailures is incremented once and the listener is invoked with onFailure, which is correct. However, if all channels open successfully but then a subsequent per-channel connect fails, onFailure is called on the listener, which eventually calls closeAndFail, incrementing connectFailures. A ConnectTransportException on channel open followed by later failures in the same flow would not double-count because early failures return before creating the ChannelsConnectedListener. Verify that the timeout path (onTimeout -> closeAndFail) and normal onFailure paths cannot both fire and double-count a single failed connect attempt — CountDown.fastForward should prevent that but confirm. private void closeAndFail(Exception e) { connectFailures.incrementAndGet(); try { Version Gating The serialization is gated on Version.V_3_8_0. Ensure this constant exists and that the PR target branch actually releases as 3.8.0; if the release ends up being a different version, mixed-cluster deserialization will corrupt the stream (older nodes reading newer payloads or vice versa). Also, the truncated PR description mentions BWC handling but no main branch version bump is shown in the diff — worth confirming V_3_8_0 is defined. if (in.getVersion().onOrAfter(Version.V_3_8_0)) { channelCloseByType = in.readMap(StreamInput::readString, StreamInput::readVLong); outgoingTimeouts = in.readVLong(); requestsFailedOnDisconnect = in.readVLong(); connectFailures = in.readVLong(); connectTimeMillis = in.readVLong(); connectTimeMillisMax = in.readVLong(); } else { channelCloseByType = Collections.emptyMap(); outgoingTimeouts = 0L; requestsFailedOnDisconnect = 0L; connectFailures = 0L; connectTimeMillis = 0L; connectTimeMillisMax = 0L; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to c11f2a6

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Preserve unmodifiable/sorted map on deserialization readMap returns a mutable HashMap, but the field is assigned directly and later returned via getChannelCloseByType(). The constructor from a Builder wraps its input in an unmodifiable TreeMap, but this stream constructor does not — so a TransportStats deserialized off the wire exposes a mutable, unsorted map, breaking the invariant the class documents (stable rendered order, unmodifiable view). Wrap the deserialized map the same way as in the builder-based constructor. server/src/main/java/org/opensearch/transport/TransportStats.java [135-136] if (in.getVersion().onOrAfter(Version.V_3_8_0)) { - channelCloseByType = in.readMap(StreamInput::readString, StreamInput::readVLong); + channelCloseByType = Collections.unmodifiableMap(new TreeMap<>(in.readMap(StreamInput::readString, StreamInput::readVLong))); Suggestion importance[1-10]: 6 __ Why: Valid observation: the stream constructor assigns a mutable HashMap directly, breaking the unmodifiable/sorted invariant documented for the class and violated by tests like testChannelCloseByTypeIsUnmodifiable after wire deserialization.	Low
General	Guard against missing enum entries in map channelCloseByType is initialized only for the enum values known at TcpTransport construction time. If a TransportRequestOptions.Type value is added later (e.g. STREAM) but not present in the map, channelCloseByType.get(t) returns null and .get() throws NPE. Guard against a missing entry to keep stats collection robust. server/src/main/java/org/opensearch/transport/TcpTransport.java [1040-1043] Map<String, Long> closeSnapshot = new HashMap<>(); for (TransportRequestOptions.Type t : TransportRequestOptions.Type.values()) { - closeSnapshot.put(t.name().toLowerCase(Locale.ROOT), channelCloseByType.get(t).get()); + AtomicLong counter = channelCloseByType.get(t); + closeSnapshot.put(t.name().toLowerCase(Locale.ROOT), counter == null ? 0L : counter.get()); } Suggestion importance[1-10]: 3 __ Why: The map is populated in the constructor by iterating over all TransportRequestOptions.Type.values(), so entries cannot be missing at runtime unless new enum values are added—a defensive but low-impact change.	Low
	Avoid mis-attributing deliberate connection closes recordChannelClose checks isClosing.get() to filter deliberate/downstream closes, but here it runs before nodeChannels.close() is called, so on the first-closing socket isClosing is still false and it counts — which is intended. However, if the connection is closed externally (e.g. via nodeChannels.close() from another path), the close listeners for each channel fire and each will observe isClosing == true only after the first invocation flips it; the very first channel-close callback that runs may see isClosing == false and mis-attribute a deliberate close as a failure. Consider setting isClosing before propagating channel closes triggered by close(), or gate recordChannelClose on whether the close originated externally. server/src/main/java/org/opensearch/transport/TcpTransport.java [1182-1188] ch.addCloseListener(ActionListener.wrap(() -> { - // Attribute the close before tearing the connection down, while we can still tell - // whether this socket is the cause. Attribution per channel type helps spot failures - // isolated to one type (e.g. REG/BULK/RECOVERY bad while PING/STATE stay healthy). - nodeChannels.recordChannelClose(channelIndex, relativeMillisTime); + if (nodeChannels.isClosing.get() == false) { + nodeChannels.recordChannelClose(channelIndex, relativeMillisTime); + } nodeChannels.close(); })); Suggestion importance[1-10]: 3 __ Why: The concern about race conditions during external close is somewhat speculative, and the proposed fix is essentially what recordChannelClose already does internally with its isClosing check.	Low
	Keep paired counters consistently updated The comment on connectTimeMillis states "Average open latency is connectTimeMillis / totalOutboundConnections", but outboundConnectionCount is incremented a few lines above only for successful opens after the handshake, while connectTimeMillis is added here for the same path — so pairing is fine only if both are consistently updated. Consider incrementing outboundConnectionCount and connectTimeMillis atomically together (or documenting that a brief window can produce a slight ratio skew) so consumers relying on the documented ratio don't observe inconsistent snapshots. server/src/main/java/org/opensearch/transport/TcpTransport.java [1165-1167] +final long connectionId = outboundConnectionCount.incrementAndGet(); final long connectMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos); connectTimeMillis.addAndGet(connectMillis); connectTimeMillisMax.updateAndGet(prev -> Math.max(prev, connectMillis)); Suggestion importance[1-10]: 2 __ Why: The suggestion's improved_code is essentially identical to the existing code and doesn't propose a concrete fix; it only raises a minor documentation/consistency concern.	Low

---

Previous suggestions

Suggestions up to commit ef1f318

Category	Suggestion	Impact
General	Make deserialized map immutable and sorted channelCloseByType read from the stream is a mutable HashMap, but the builder's copy constructor and the private constructor from the builder wrap incoming maps in an unmodifiable TreeMap only via the builder path. Here, the field is assigned directly from in.readMap, bypassing the unmodifiable wrapping done in the builder-based constructor. Wrap it here as well so getChannelCloseByType() is consistently immutable regardless of how the instance was constructed. server/src/main/java/org/opensearch/transport/TransportStats.java [135-142] if (in.getVersion().onOrAfter(Version.V_3_8_0)) { - channelCloseByType = in.readMap(StreamInput::readString, StreamInput::readVLong); + channelCloseByType = Collections.unmodifiableMap(new TreeMap<>(in.readMap(StreamInput::readString, StreamInput::readVLong))); outgoingTimeouts = in.readVLong(); requestsFailedOnDisconnect = in.readVLong(); connectFailures = in.readVLong(); connectTimeMillis = in.readVLong(); connectTimeMillisMax = in.readVLong(); } else { Suggestion importance[1-10]: 6 __ Why: Valid consistency concern: instances constructed via the builder have an unmodifiable sorted map, but deserialized instances would have a mutable HashMap, which contradicts the testChannelCloseByTypeIsUnmodifiable invariant for deserialized objects.	Low
	Handle null transport stats safely transport.getStats() may return null in some transport implementations or lifecycle states (e.g., before start or during shutdown). Passing null into new TransportStats.Builder(base) will NPE. Guard against a null base to preserve the previous behaviour where stats() propagated whatever the transport returned. server/src/main/java/org/opensearch/transport/TransportService.java [488-493] public TransportStats stats() { // The transport owns every counter except the two request-level ones tracked here. - return new TransportStats.Builder(transport.getStats()).outgoingTimeouts(outgoingTimeouts.get()) + TransportStats base = transport.getStats(); + if (base == null) { + return null; + } + return new TransportStats.Builder(base).outgoingTimeouts(outgoingTimeouts.get()) .requestsFailedOnDisconnect(requestsFailedOnDisconnect.get()) .build(); } Suggestion importance[1-10]: 4 __ Why: Reasonable defensive check; transport.getStats() typically doesn't return null in current implementations, but a null guard preserves previous behavior and avoids potential NPE.	Low
	Guard against missing enum entries in snapshot channelCloseByType is initialized only for the enum values existing at construction time, but iterating over TransportRequestOptions.Type.values() here reads via channelCloseByType.get(t). If any enum value could be null (or if the map was later modified), this NPEs. Since the map is constructed once with every enum entry this is safe today, but guarding with a null-check or using getOrDefault would make the snapshot resilient to future additions of enum values not initialized in the constructor. server/src/main/java/org/opensearch/transport/TcpTransport.java [1040-1043] Map<String, Long> closeSnapshot = new HashMap<>(); for (TransportRequestOptions.Type t : TransportRequestOptions.Type.values()) { - closeSnapshot.put(t.name().toLowerCase(Locale.ROOT), channelCloseByType.get(t).get()); + AtomicLong counter = channelCloseByType.get(t); + closeSnapshot.put(t.name().toLowerCase(Locale.ROOT), counter == null ? 0L : counter.get()); } Suggestion importance[1-10]: 2 __ Why: The map is initialized in the constructor with every enum value, so channelCloseByType.get(t) cannot be null today. The suggestion is defensive but of marginal value.	Low
Possible issue	Avoid double-counting connect failures A failure here also causes ChannelsConnectedListener.closeAndFail to be invoked later via the listener path in some flows, and closeAndFail itself increments connectFailures. Verify that a single failed connection cannot be double-counted; if it can, increment in only one place (e.g., only in closeAndFail, or only here) to keep connectFailures accurate. server/src/main/java/org/opensearch/transport/TcpTransport.java [480-490] +} catch (ConnectTransportException e) { + connectFailures.incrementAndGet(); + CloseableChannel.closeChannels(channels, false); + listener.onFailure(e); + return channels; +} catch (Exception e) { + connectFailures.incrementAndGet(); + CloseableChannel.closeChannels(channels, false); + listener.onFailure(new ConnectTransportException(node, "general node connection failure", e)); + return channels; +} - Suggestion importance[1-10]: 6 __ Why: Legitimate concern about potential double-counting between initiateConnection catch blocks and closeAndFail. Worth verifying, though the two paths appear mutually exclusive (early failure returns before creating the listener).	Low

Suggestions up to commit a4cf5bd

Category	Suggestion	Impact
Possible issue	Preserve immutability and ordering after deserialization readMap returns a mutable HashMap, but the constructor path from StreamInput bypasses the private Builder constructor that wraps the map in an unmodifiable TreeMap. This makes getChannelCloseByType() mutable and unsorted after deserialization, violating the invariants asserted by testChannelCloseByTypeIsUnmodifiable and the documented stable field order. server/src/main/java/org/opensearch/transport/TransportStats.java [133-134] if (in.getVersion().onOrAfter(Version.V_3_8_0)) { - channelCloseByType = in.readMap(StreamInput::readString, StreamInput::readVLong); + channelCloseByType = Collections.unmodifiableMap(new TreeMap<>(in.readMap(StreamInput::readString, StreamInput::readVLong))); outgoingTimeouts = in.readVLong(); Suggestion importance[1-10]: 7 __ Why: Valid observation: the StreamInput constructor bypasses the Builder's TreeMap/unmodifiableMap wrapping, so deserialized instances would fail the testChannelCloseByTypeIsUnmodifiable invariant and lose stable ordering.	Medium
	Avoid NPE when iterating channel types channelCloseByType is initialized only for the built-in TransportRequestOptions.Type values, but iterating over values() may include newly added types (e.g. STREAM) that were not pre-populated, causing channelCloseByType.get(t).get() to throw NullPointerException. Guard against missing entries or ensure the map is fully populated in the constructor. server/src/main/java/org/opensearch/transport/TcpTransport.java [1035-1038] Map<String, Long> closeSnapshot = new HashMap<>(); for (TransportRequestOptions.Type t : TransportRequestOptions.Type.values()) { - closeSnapshot.put(t.name().toLowerCase(Locale.ROOT), channelCloseByType.get(t).get()); + AtomicLong counter = channelCloseByType.get(t); + closeSnapshot.put(t.name().toLowerCase(Locale.ROOT), counter == null ? 0L : counter.get()); } Suggestion importance[1-10]: 4 __ Why: The channelCloseByType EnumMap is populated in the constructor for all TransportRequestOptions.Type.values(), so a NPE from get(t) is not currently possible. The defensive check is minor and only helpful for future type additions.	Low
General	Ensure handshake failures increment connect failures connectTimeMillis is only incremented on the successful-handshake path, but totalOutboundConnections is incremented on the same path (outboundConnectionCount.incrementAndGet()), which is fine — however if the handshake throws (caught below), startNanos elapsed time is silently dropped and no connectFailures counter is incremented for handshake failures either. Consider incrementing connectFailures in the handshake failure branch so handshake timeouts show up in the impairment signal. server/src/main/java/org/opensearch/transport/TcpTransport.java [1160-1162] final long connectMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos); connectTimeMillis.addAndGet(connectMillis); connectTimeMillisMax.updateAndGet(prev -> Math.max(prev, connectMillis)); +// Note: handshake failures below should also increment connectFailures via closeAndFail. Suggestion importance[1-10]: 3 __ Why: The closeAndFail method already increments connectFailures, and handshake failures route through it, so the concern raised may already be addressed. The suggestion also does not modify code, just adds a comment.	Low

Suggestions up to commit 9ca395a

Category	Suggestion	Impact
General	Avoid double-counting multi-type channel closes A single TcpChannel may map to multiple TransportRequestOptions.Type values (a handle can serve several types), causing the same close event to increment counters for every type sharing that channel and inflate close counts. Consider incrementing only once per channel close (e.g., by attributing it to a representative type or by tracking the count per unique channel), or explicitly document that a close on a multi-type channel counts against each type. server/src/main/java/org/opensearch/transport/TcpTransport.java [1147-1160] nodeChannels.channels.forEach(ch -> { // Mark the channel init time ch.getChannelStats().markAccessed(relativeMillisTime); ch.addCloseListener(ActionListener.wrap(nodeChannels::close)); - // Log which channel type's individual socket closed — useful for detecting - // per-channel-type failures (e.g. REG/BULK/RECOVERY bad while PING/STATE healthy). final Set<TransportRequestOptions.Type> types = nodeChannels.channelTypes(ch); if (types.isEmpty() == false) { ch.addCloseListener(ActionListener.wrap(() -> { logger.debug( "individual [{}] channel socket closed to [{}] (connection age [{}ms])", types, node, threadPool.relativeTimeInMillis() - relativeMillisTime ); - for (TransportRequestOptions.Type type : types) { - channelCloseByType.get(type).incrementAndGet(); - } + // Count each socket close once, attributed to the first (primary) type. + channelCloseByType.get(types.iterator().next()).incrementAndGet(); })); } }); Suggestion importance[1-10]: 7 __ Why: Valid concern: a single channel mapped to multiple types will increment counters for each type on close, potentially inflating aggregate counts. This is a legitimate semantic issue worth clarifying or fixing.	Medium
	Capture connect start time per attempt connectStartNanos is stored on the connectActivity runnable, which is reused across multiple connect attempts for the same ConnectionTarget. If a stale onFailure callback from a previous attempt fires after a subsequent doRun() has overwritten connectStartNanos, the elapsed time logged will be wrong (potentially negative or nonsensical). Consider capturing the start time in a local variable inside doRun() and closing over it in the anonymous ActionListener to make the elapsed measurement per-attempt. server/src/main/java/org/opensearch/cluster/NodeConnectionsService.java [349-352] -final AbstractRunnable abstractRunnable = this; -// Tracks wall-clock start of the current connect attempt (nanoseconds). -// Written in doRun() before connectToNode; read in onResponse/onFailure callbacks. -// volatile ensures visibility across the thread-pool hand-off. -volatile long connectStartNanos; +@Override +protected void doRun() { + assert Thread.holdsLock(mutex) == false : "mutex unexpectedly held"; + if (transportService.nodeConnected(discoveryNode)) { + logger.trace("still connected to {}", discoveryNode); + onConnected(); + } else { + logger.debug("connecting to {}", discoveryNode); + final long attemptStartNanos = System.nanoTime(); + connectStartNanos = attemptStartNanos; + transportService.connectToNode(discoveryNode, new ActionListener<Void>() { + @Override + public void onResponse(Void aVoid) { + final long elapsedMs = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - attemptStartNanos); + logger.debug("connected to {} in [{}ms]", discoveryNode, elapsedMs); + onConnected(); + } + // ... + }); + } +} Suggestion importance[1-10]: 7 __ Why: Valid race concern: since the runnable instance is reused, a stale callback could observe an overwritten connectStartNanos, producing incorrect elapsed timings. Capturing per-attempt makes the measurement robust.	Medium
	Make deserialized map unmodifiable too in.readMap returns a mutable map, but the constructor from a builder wraps channelCloseByType in Collections.unmodifiableMap, so the stream-input path and the builder path expose different mutability semantics (a test asserts unmodifiability). Wrap the deserialized map in Collections.unmodifiableMap for consistency, otherwise callers that receive a stats object built via serialization can mutate it while ones built via the builder cannot. server/src/main/java/org/opensearch/transport/TransportStats.java [128-135] if (in.getVersion().onOrAfter(Version.V_3_8_0)) { - channelCloseByType = in.readMap(StreamInput::readString, StreamInput::readVLong); + channelCloseByType = Collections.unmodifiableMap(in.readMap(StreamInput::readString, StreamInput::readVLong)); outgoingTimeouts = in.readVLong(); requestsFailedOnDisconnect = in.readVLong(); connectFailures = in.readVLong(); connectTimeMillis = in.readVLong(); connectTimeMillisMax = in.readVLong(); } else { Suggestion importance[1-10]: 5 __ Why: Correct observation about inconsistent mutability between builder and stream-input paths; wrapping in unmodifiableMap improves consistency and matches the test expectation.	Low
	Guard against double-counting connect failures connectTimeMillis is only incremented on the success path here, but connectFailures is incremented in closeAndFail. If a caller later computes average open latency as connectTimeMillis / totalOutboundConnections, that's fine, but the javadoc on connectTimeMillis should reflect it excludes failed attempts (which it does). Additionally, ensure connectFailures is not double-counted when both onFailure and onTimeout paths flow through closeAndFail for the same attempt — verify each attempt calls closeAndFail at most once. server/src/main/java/org/opensearch/transport/TcpTransport.java [1129-1131] final long connectMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos); connectTimeMillis.addAndGet(connectMillis); connectTimeMillisMax.updateAndGet(prev -> Math.max(prev, connectMillis)); +// Note: connectFailures is bumped in closeAndFail; ensure it runs only once per attempt. Suggestion importance[1-10]: 3 __ Why: The suggestion is mostly a verification request and only adds a comment; it does not concretely fix a bug, and the countdown mechanism likely already prevents double counting.	Low

Suggestions up to commit cfcb4e6

Category	Suggestion	Impact
Possible issue	Avoid double-counting shared channel closes A single physical channel can serve multiple TransportRequestOptions.Type values (e.g., a handle covering REG+STATE+RECOVERY). Incrementing channelCloseByType once per type on a single close inflates the counter by the number of types sharing that socket rather than the number of sockets closed. Increment per closed channel only once (e.g., pick a representative type) or use a distinct semantic to avoid double-counting. server/src/main/java/org/opensearch/transport/TcpTransport.java [1156-1158] nodeChannels.channels.forEach(ch -> { // Mark the channel init time ch.getChannelStats().markAccessed(relativeMillisTime); ch.addCloseListener(ActionListener.wrap(nodeChannels::close)); - // Log which channel type's individual socket closed — useful for detecting - // per-channel-type failures (e.g. REG/BULK/RECOVERY bad while PING/STATE healthy). final Set<TransportRequestOptions.Type> types = nodeChannels.channelTypes(ch); if (types.isEmpty() == false) { ch.addCloseListener(ActionListener.wrap(() -> { logger.debug( "individual [{}] channel socket closed to [{}] (connection age [{}ms])", types, node, threadPool.relativeTimeInMillis() - relativeMillisTime ); - for (TransportRequestOptions.Type type : types) { - channelCloseByType.get(type).incrementAndGet(); - } + // Increment once per closed socket using its primary type to avoid inflating counts + // when multiple types share the same channel. + TransportRequestOptions.Type primary = types.iterator().next(); + channelCloseByType.get(primary).incrementAndGet(); })); } }); Suggestion importance[1-10]: 8 __ Why: Valid concern: a ConnectionTypeHandle can serve multiple types, so incrementing channelCloseByType for every type in the set on a single socket close inflates the counter. This affects the accuracy of the newly-introduced observability metric.	Medium
	Confirm BWC version gate is correct Verify Version.V_3_8_0 actually exists in the Version class at the time of merge; if the target release is later renamed/bumped (e.g., 3.9.0 or 4.0.0), this constant becomes wrong and BWC serialization silently breaks between mixed-version nodes. Confirm the constant and gate on the actually released version to prevent stream corruption when a 3.8 node negotiates with a 3.9 node. server/src/main/java/org/opensearch/transport/TransportStats.java [128-134] +if (in.getVersion().onOrAfter(Version.V_3_8_0)) { + channelCloseByType = in.readMap(StreamInput::readString, StreamInput::readVLong); + outgoingTimeouts = in.readVLong(); + requestsFailedOnDisconnect = in.readVLong(); + connectFailures = in.readVLong(); + connectTimeMillis = in.readVLong(); + connectTimeMillisMax = in.readVLong(); +} else { - Suggestion importance[1-10]: 4 __ Why: Only asks the author to verify the version constant. Verification suggestions are capped, and the concern about later renames is speculative.	Low
General	Clarify connect-time counter semantics on failure connectTimeMillis is accumulated only for successful connects, but the XContent field name and comment suggest a total. On failure paths closeAndFail increments connectFailures without recording elapsed time, so any "average per attempt" computation using connectTimeMillis / (totalOutboundConnections + connectFailures) is misleading. Either document precisely that this covers only successful opens (as the comment states) and align the field naming, or also record elapsed time on failure to make the counters comparable. server/src/main/java/org/opensearch/transport/TcpTransport.java [1129-1131] final long connectionId = outboundConnectionCount.incrementAndGet(); +// connectTimeMillis covers only successful opens; average success latency is +// connectTimeMillis / totalOutboundConnections. final long connectMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startNanos); connectTimeMillis.addAndGet(connectMillis); connectTimeMillisMax.updateAndGet(prev -> Math.max(prev, connectMillis)); Suggestion importance[1-10]: 3 __ Why: The comment in TransportStats already documents that connectTimeMillis covers only successful opens. This is a minor documentation/clarification suggestion with limited impact.	Low
	Defensively copy map in builder Assigning base.channelCloseByType directly aliases the (unmodifiable) map from the source stats. If a subsequent channelCloseByType(...) call is skipped and the builder is later used to derive further stats, mutations elsewhere would be blocked but the reference is still shared. Copy the map defensively to keep the builder’s state independent of the source. server/src/main/java/org/opensearch/transport/TransportStats.java [262] public Builder(TransportStats base) { this.serverOpen = base.serverOpen; this.totalOutboundConnections = base.totalOutboundConnections; this.rxCount = base.rxCount; this.rxSize = base.rxSize; this.txCount = base.txCount; this.txSize = base.txSize; - this.channelCloseByType = base.channelCloseByType; + this.channelCloseByType = new HashMap<>(base.channelCloseByType); Suggestion importance[1-10]: 3 __ Why: Since channelCloseByType in TransportStats is already wrapped as unmodifiable, aliasing is safe. Defensive copying is a minor code-hygiene improvement.	Low

[github-actions]

Persistent review updated to latest commit 9ca395a

[github-actions]

❌ Gradle check result for 9ca395a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit a4cf5bd

[github-actions]

Persistent review updated to latest commit ef1f318

[github-actions]

❌ Gradle check result for ef1f318: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit c11f2a6

[github-actions]

✅ Gradle check result for c11f2a6: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 89.18919% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.52%. Comparing base (599785a) to head (c11f2a6).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...in/java/org/opensearch/transport/TcpTransport.java	89.28%	5 Missing and 1 partial ⚠️
.../java/org/opensearch/transport/TransportStats.java	92.30%	6 Missing ⚠️
...ava/org/opensearch/transport/TransportService.java	57.14%	3 Missing ⚠️
...org/opensearch/cluster/NodeConnectionsService.java	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22646      +/-   ##
============================================
- Coverage     71.54%   71.52%   -0.03%     
- Complexity    77023    77035      +12     
============================================
  Files          6153     6156       +3     
  Lines        358354   358550     +196     
  Branches      52237    52255      +18     
============================================
+ Hits         256399   256438      +39     
- Misses        81586    81707     +121     
- Partials      20369    20405      +36     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.