fix: release pre-acquired stream IDs - #964
Closed
arnabnandy7 wants to merge 1 commit into
Closed
Conversation
Signed-off-by: Arnab Nandy <arnab_nandy7@yahoo.com>
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
@coderabbitai review |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
nikagra
added a commit
to nikagra/java-driver
that referenced
this pull request
Aug 3, 2026
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and Policies
when the report is built, i.e. once per Cluster as it initializes.
Adds public getters to DCAwareRoundRobinPolicy and
RackAwareRoundRobinPolicy needed to report node-location-preference
and dc-failover, and makes PagingOptimizingLoadBalancingPolicy
implement ChainableLoadBalancingPolicy so the reporter can unwrap the
LB policy Cluster.Manager wraps at runtime.
Caps the report at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString
writes each STARTUP value with a 16-bit length prefix and no bounds
check, so a value over 65535 bytes truncates the prefix modulo 65536
while still appending the whole body -- a corrupt frame and a failed
handshake, and not something the fail-safe try/catch can contain since
nothing throws. Parts of the report are user-supplied and unbounded
(DC/rack names, consistency levels, custom policy class names). Over
the limit means WARN and no DRIVER_CONFIG.
Hardens the other two ways reporting could break a connection rather
than merely fail to report:
- The fail-safe catch also covers InternalError, since customPolicy()
calls getClass().getSimpleName() on arbitrary user policy objects
(documented JDK edge case for certain synthetic classes). Not a bare
Error, so OutOfMemoryError/StackOverflowError still surface.
- The load balancing policy chain walk is bounded at 16 policies and
shared by both callers. It follows getChildPolicy() on arbitrary user
policies, so a cyclic chain used to spin forever on the Cluster
initialization path -- the one failure mode the try/catch cannot
contain, because it hangs rather than throws.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit, plus a
negative test proving additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nikagra
added a commit
to nikagra/java-driver
that referenced
this pull request
Aug 3, 2026
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and Policies
when the report is built, i.e. once per Cluster as it initializes.
Adds public getters to DCAwareRoundRobinPolicy and
RackAwareRoundRobinPolicy needed to report node-location-preference
and dc-failover, and makes PagingOptimizingLoadBalancingPolicy
implement ChainableLoadBalancingPolicy so the reporter can unwrap the
LB policy Cluster.Manager wraps at runtime.
Caps the report at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString
writes each STARTUP value with a 16-bit length prefix and no bounds
check, so a value over 65535 bytes truncates the prefix modulo 65536
while still appending the whole body -- a corrupt frame and a failed
handshake, and not something the fail-safe try/catch can contain since
nothing throws. Parts of the report are user-supplied and unbounded
(DC/rack names, consistency levels, custom policy class names). Over
the limit means WARN and no DRIVER_CONFIG.
Hardens the other two ways reporting could break a connection rather
than merely fail to report:
- The fail-safe catch also covers InternalError, since customPolicy()
calls getClass().getSimpleName() on arbitrary user policy objects
(documented JDK edge case for certain synthetic classes). Not a bare
Error, so OutOfMemoryError/StackOverflowError still surface.
- The load balancing policy chain walk is bounded at 16 policies and
shared by both callers. It follows getChildPolicy() on arbitrary user
policies, so a cyclic chain used to spin forever on the Cluster
initialization path -- the one failure mode the try/catch cannot
contain, because it hangs rather than throws.
A custom load balancing policy is now named after the policy the user
configured rather than PagingOptimizingLoadBalancingPolicy. Cluster
.Manager wraps every session's policy in that internal class, and it is
the outermost element of the chain, so every custom policy was reported
as {"type":"custom","name":"PagingOptimizingLoadBalancingPolicy"}.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit, plus a
negative test proving additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nikagra
added a commit
to nikagra/java-driver
that referenced
this pull request
Aug 3, 2026
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and Policies
when the report is built, i.e. once per Cluster as it initializes.
connection-pool.desired-connections-count reports the core connections
per host rather than the maximum: HostConnectionPool opens core
connections when it initializes and only grows towards the maximum
under load, which is what the schema's "connections to open per host"
means. Both protocol-v3 defaults are 1, so this only shows on a tuned
pool -- where reporting the maximum was wrong in both directions,
overstating a configured core/max pair and falling back to the v3
default when only the core count was set.
Where a configured value falls outside what the schema can express, an
optional key or group is omitted rather than emitted as a number the
schema rejects: a disabled read timeout (connection.read and
control-plane.system-queries.timeout.client-side-ms), a negative
SO_LINGER, a non-positive socket buffer size. Omission is not always
available -- connection.connect.timeout-ms and
query-defaults.request.timeout-ms are required and positive-only, and
query-defaults.consistency is an enum without the serial levels, all
three reachable through unvalidated 3.x setters -- so those are
reported as-is and the limitation is documented on the class: the
reporter neither fabricates an in-range value, which would misreport a
setting an operator may have chosen on purpose, nor drops the whole
report over one field. Recorded as a cross-driver schema gap, to be
fixed the way control-plane.schema-agreement.timeout-ms already admits
0.
Adds public getters to DCAwareRoundRobinPolicy and
RackAwareRoundRobinPolicy needed to report node-location-preference
and dc-failover, and makes PagingOptimizingLoadBalancingPolicy
implement ChainableLoadBalancingPolicy so the reporter can unwrap the
LB policy Cluster.Manager wraps at runtime.
Caps the report at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString
writes each STARTUP value with a 16-bit length prefix and no bounds
check, so a value over 65535 bytes truncates the prefix modulo 65536
while still appending the whole body -- a corrupt frame and a failed
handshake, and not something the fail-safe try/catch can contain since
nothing throws. Parts of the report are user-supplied and unbounded
(DC/rack names, consistency levels, custom policy class names). Over
the limit means WARN and no DRIVER_CONFIG.
Hardens the other two ways reporting could break a connection rather
than merely fail to report:
- The fail-safe catch also covers InternalError, since customPolicy()
calls getClass().getSimpleName() on arbitrary user policy objects
(documented JDK edge case for certain synthetic classes). Not a bare
Error, so OutOfMemoryError/StackOverflowError still surface.
- The load balancing policy chain walk is bounded at 16 policies and
shared by both callers. It follows getChildPolicy() on arbitrary user
policies, so a cyclic chain used to spin forever on the Cluster
initialization path -- the one failure mode the try/catch cannot
contain, because it hangs rather than throws.
A custom load balancing policy is now named after the policy the user
configured rather than PagingOptimizingLoadBalancingPolicy. Cluster
.Manager wraps every session's policy in that internal class, and it is
the outermost element of the chain, so every custom policy was reported
as {"type":"custom","name":"PagingOptimizingLoadBalancingPolicy"}.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit, plus a
negative test proving additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nikagra
added a commit
to nikagra/java-driver
that referenced
this pull request
Aug 4, 2026
Fills in the full DRIVER_CONFIG JSON report in the approved v2
cross-driver schema shape, replacing the stage-1 {"version":1}
placeholder. All groups are populated from Configuration and Policies
when the report is built, i.e. once per Cluster as it initializes.
token-aware is the only built-in load balancing shape the schema
defines, so every other built-in policy -- a bare
DCAwareRoundRobinPolicy, RoundRobinPolicy, WhiteListPolicy -- is
reported as custom with its class name, which identifies it but carries
none of the normalized flags; its datacenter and rack still show up in
node-location-preference. A token-aware chain reports load-distribution
from its replica ordering (RANDOM, the 3.x default, is "shuffle";
TOPOLOGICAL is "replica-set"; NEUTRAL keeps the child's plan order, so
"round-robin") and adaptive-ordering from LatencyAwarePolicy, the only
runtime observation a 3.x policy can reorder candidates on.
Where a configured value falls outside what the schema can express, an
optional key or group is omitted rather than emitted as a number the
schema rejects: a disabled connect timeout, a disabled read timeout
(connection.read and control-plane.system-queries.timeout
.client-side-ms), a negative SO_LINGER, a non-positive socket buffer
size, an unbounded page size. Omission is not always available, so
these required keys are left in the one state that is accurate:
- connection.requests.orphaned.max has no 3.x equivalent to report at
all. A request the driver stopped waiting for keeps its stream
identifier until the response arrives, with no configurable bound and
no connection replacement, so the key is omitted -- which its
required-ness then rejects. This is the one violation every report
carries.
- connection.requests.in-flight.max is capped at 32767 by the schema,
one below the 32768 stream identifiers protocol v3 provides and
PoolingOptions accepts.
- query-defaults.request.timeout-ms is positive-only, yet the read
timeout feeding it documents a non-positive value as "no timeout",
and query-defaults.consistency is an enum without the serial levels
that QueryOptions.setConsistencyLevel accepts unvalidated. Both
setters validate nothing.
Such a value is reported as-is and the limitation is documented on the
class: the reporter neither fabricates an in-range value -- which would
misreport a setting an operator may have chosen on purpose, or a policy
3.x does not implement -- nor drops the whole report over one field.
Recorded as a cross-driver schema gap, to be fixed the way
control-plane.schema-agreement.timeout-ms already admits 0.
Adds public getters to DCAwareRoundRobinPolicy,
RackAwareRoundRobinPolicy and TokenAwarePolicy needed to report
node-location-preference, dc-failover and load-distribution, and makes
PagingOptimizingLoadBalancingPolicy implement
ChainableLoadBalancingPolicy so the reporter can unwrap the LB policy
Cluster.Manager wraps at runtime.
Caps the report at 32KiB of UTF-8 (MAX_DRIVER_CONFIG_LENGTH), matching
the 4.x sibling PR scylladb#968, gocql scylladb#964 and csharp-driver scylladb#262. Beyond
cross-driver parity this is a correctness fix: CBUtil.writeString
writes each STARTUP value with a 16-bit length prefix and no bounds
check, so a value over 65535 bytes truncates the prefix modulo 65536
while still appending the whole body -- a corrupt frame and a failed
handshake, and not something the fail-safe try/catch can contain since
nothing throws. Parts of the report are user-supplied and unbounded
(DC/rack names, consistency levels, custom policy class names). Over
the limit means WARN and no DRIVER_CONFIG.
Hardens the other two ways reporting could break a connection rather
than merely fail to report:
- The fail-safe catch also covers InternalError, since customPolicy()
calls getClass().getSimpleName() on arbitrary user policy objects
(documented JDK edge case for certain synthetic classes). Not a bare
Error, so OutOfMemoryError/StackOverflowError still surface.
- The load balancing policy chain walk is bounded at 16 policies and
shared by both callers. It follows getChildPolicy() on arbitrary user
policies, so a cyclic chain used to spin forever on the Cluster
initialization path -- the one failure mode the try/catch cannot
contain, because it hangs rather than throws.
A custom load balancing policy is now named after the policy the user
configured rather than PagingOptimizingLoadBalancingPolicy. Cluster
.Manager wraps every session's policy in that internal class, and it is
the outermost element of the chain, so every custom policy was reported
as {"type":"custom","name":"PagingOptimizingLoadBalancingPolicy"}. An
anonymous policy class falls back to its binary name, since it has no
simple name and the schema requires a non-empty one.
Adds a JSON-Schema conformance test suite (mirroring the 4.x sibling
PR scylladb#968): the normative schema is shipped as a test resource and
validated via com.networknt:json-schema-validator (pinned to 1.5.x,
the last line still targeting Java 8), covering every discriminated-
union branch and optional group the 3.x reporter can emit. Since one
required key has no value to report, the assertion is that a report
violates the schema in exactly the documented ways and no other, with
a test naming each gap and a negative test proving
additionalProperties=false is enforced.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Description
What changed
DriverChannel.write()rejects a request before submitting it to the Netty pipeline.write()is called.Root cause
Channel selection calls
preAcquireId()before returning a channel. Request handlers then perform additional work before the resultingRequestMessagereachesInFlightHandler.If that work throws, or
DriverChannel.write()rejects the request before pipeline submission,InFlightHandlernever sees the request and therefore cannot release the reservation. Repeated failures can cause available stream-ID accounting to drift and eventually make healthy channels appear saturated.Impact
Every successful stream-ID pre-acquisition now ends in one of two outcomes:
InFlightHandler; orFixes #947.
Validation
git diff --checkpassed