[
https://issues.apache.org/jira/browse/CASSANDRA-21508?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Francisco Guerrero reassigned CASSANDRA-21508:
----------------------------------------------
Assignee: Francisco Guerrero
> Coordinator load-shedding returns OverloadedException without setting
> streamId, misrouting query responses
> ----------------------------------------------------------------------------------------------------------
>
> Key: CASSANDRA-21508
> URL: https://issues.apache.org/jira/browse/CASSANDRA-21508
> Project: Apache Cassandra
> Issue Type: Bug
> Components: Messaging/Client
> Reporter: C. Scott Andreas
> Assignee: Francisco Guerrero
> Priority: Normal
> Fix For: 4.1.x, 5.0, 6.0, 7.0
>
> Attachments: StreamIdMisrouteTest.java
>
>
> When the coordinator sheds a request that has waited in the
> Native-Transport-Requests (NTR) queue longer than
> {{{}native_transport_timeout{}}}, it returns an {{OverloadedException}} frame
> without setting the response stream id. The frame goes out on stream id 0
> instead of the timed-out request's stream id. Under load this misroutes an
> error to an unrelated in-flight request on the same connection, and can
> escalate into silent client-side data corruption (a value from one query
> decoded against another query's column definitions) when the native protocol
> v5 skip-metadata optimization is in effect.
> *Severity*
> Silent read corruption. A single mis-stamped error frame can cause an
> unrelated query's rows to be decoded against the wrong column definitions,
> producing either a decode exception or a plausible-but-wrong value returned
> in response to a query.
> *Root Cause*
> Dispatcher.processRequest, Dispatcher.java:366-369:
> {color:#505f79}{{if (queueTime >
> DatabaseDescriptor.getNativeTransportTimeout(TimeUnit.NANOSECONDS)) {}}{color}
> {color:#505f79}{{
> ClientMetrics.instance.markTimedOutBeforeProcessing();}}{color}
> {color:#505f79}{{ return ErrorMessage.fromException(new
> OverloadedException("Query timed out before it could start"));}}{color}
> {color:#505f79}{{} *// <-- no setStreamId(request.getStreamId())*}}{color}
> This is the only response-producing exit in {{Dispatcher}} that does not set
> the stream id. The other exits set it explicitly
> ({{{}Dispatcher.java:113{}}}, {{{}:423{}}}, {{{}:448{}}}).
> * {{ErrorMessage.fromException(e, null)}} initializes {{streamId = 0}}
> ({{{}ErrorMessage.java:415{}}}).
> * {{OverloadedException}} extends {{RequestExecutionException}} (not
> {{{}WrappedException{}}}), so no stream id is recovered from the throwable.
> * {{Message.streamId}} defaults to 0.
> * The outer {{processRequest}} ({{{}Dispatcher.java:433-437{}}}) returns
> this response via the normal (non-exceptional) path, so the catch block that
> would have called {{setStreamId}} ({{{}:448{}}}) never runs.
> * The frame is flushed and {{Message.encode}} uses {{{}getStreamId() ==
> 0{}}}.
> Result: the OVERLOADED error reaches the client on stream id 0, not on the
> stream id of the request that actually timed out.
> {{OverloadedException}} encodes as a normal, non-fatal error, so the client
> demultiplexes it by stream id (the channel is not torn down).
>
> *Mechanism for triggering client-side corruption:*
> Two independent requests are involved:
> * {*}Request A{*}: the one that sat in the NTR queue past the deadline. The
> server sheds it and emits the OVERLOADED error, mis-stamped with stream id 0.
> * {*}Request B{*}: a different, healthy request that the client currently
> has on stream id 0. The server is still processing it normally.
> On a busy connection stream id 0 is almost always in use, because the
> DataStax java-driver allocates the lowest free id first
> ({{{}StreamIdGenerator.acquire(){}}} uses {{{}ids.nextClearBit(0){}}}). The
> sequence:
> # The client receives OVERLOADED on stream id 0 and applies it to request B.
> B is completed (failed or retried) and the client performs a normal, reusable
> release of stream id 0. This differs from a client-side timeout, which would
> orphan the id and keep it reserved; here the client is led to believe B
> genuinely finished.
> # The client reuses stream id 0 for a new request C.
> # Request B's real rows response arrives on stream id 0 and is delivered to
> C.
> # Under skip-metadata the rows frame carries no column metadata, so the
> driver decodes B's row bytes positionally against C's cached column
> definitions.
> When B's and C's column orders differ, each value lands one or more columns
> away from where it belongs. Type-incompatible positions throw in the codec
> (for example {{Invalid boolean value, expecting 1 byte but got 8}} when an
> 8-byte value lands in a boolean slot, or a {{MalformedInputException}} when
> non-UTF-8 bytes land in a text slot). Type-compatible positions decode
> cleanly and return the wrong value, which the application can then persist.
> The stream id 0 misroute happens at the transport layer, below result
> metadata. A schema or metadata change is not required to trigger it.
>
> *Steps to Reproduce*
> Extreme load produces this naturally (NTR queue backlog on a saturated
> coordinator, correlating with the OVERLOADED errors and a client-side timeout
> storm). It can be reproduced deterministically with the in-JVM dtest below:
> * Start a 1-node cluster with {{native_transport_max_threads=1}} and
> {{{}native_transport_timeout=500ms{}}}.
> * Slow down SELECT execution (a ByteBuddy interceptor on
> {{{}SelectStatement.execute{}}}, as in {{{}OverloadTest.SlowSelect{}}}) so a
> request queued behind a running one crosses the deadline.
> * On one {{SimpleClient}} connection, saturate the single NTR worker and
> pile several requests behind it. On a second connection, send a request on a
> non-zero stream id (42) that queues past the deadline and is shed.
> * Observe that the response is an {{OverloadedException}} and that its
> stream id is 0, not 42.
>
> *Simplest Fix*
> Set the stream id on the load-shed path, matching every other
> response-producing exit in {{{}Dispatcher{}}}:
> {{if (queueTime >
> DatabaseDescriptor.getNativeTransportTimeout(TimeUnit.NANOSECONDS)) }}{{{ }}
> {{ ClientMetrics.instance.markTimedOutBeforeProcessing();}}
> {{ Message.Response response = ErrorMessage.fromException(new
> OverloadedException("Query timed out before it could start"));}}
> {{ }}{{response.setStreamId(request.getStreamId()); return response;}}
> {{}}}{{ }}{{}}
>
> This response path is overly brittle, though. As it's designed today, each
> return site is responsible for setting the streamId on the response, which is
> easy to miss.
>
> We should consider:
> * {*}Cassandra{*}{*}:{*} Setting the streamId centrally in the Dispatcher
> instance's processRequest method to ensure all paths are guarded (suggestion
> from Benedict).
> * *Cassandra:* Initialize the streamId to an invalid sentinel value and
> assert that it is properly set before returning to the client (suggestion
> from Benedict).
> * *Java* *Driver:* Altering the driver to stop using streamId=0 to avoid
> this behavior from older/unpatched server versions.
> * *Java Driver:* Validate value count versus column definitions count on the
> skip-metadata decode path so a rows/definitions mismatch fails loudly instead
> of silently mis-decoding.
> * *Protocol:* Sending a digest of schema and/or metadata with responses for
> clients to validate, ensuring that the version of the schema rows are decoded
> against matches between the client and server.
> * *Other drivers:* Review behavior and apply changes as appropriate.
>
> *Repro Test*
> The dtest below asserts the shed error comes back on stream id 0. Applying
> the fix above changes the observed stream id from 0 to the request's own id
> (42), so the test fails when the bug is fixed and passes while it is present,
> which confirms it exercises the real path.
> Verified locally:
> * Unmodified source: {{tests=2 errors=0 failures=0}} (both pass).
> * With the fix applied: {{loadShedErrorIsStampedWithStreamIdZero}} fails
> with {{{}expected:<0> but was:<42>{}}}; the decode test still passes.
> Place at
> {{test/distributed/org/apache/cassandra/distributed/test/StreamIdMisrouteTest.java}}
> [dtest attached].
>
> *Determining Exposure - Server Side*
> The errant response path ticks ClientMetrics.timedOutBeforeProcessing. This
> metric is only incremented by this response path and is a strong signal that
> responses have been routed incorrectly.
> *Determining Exposure - Client Side*
> Cassandra users can assess exposure by reviewing application logs and
> searching for instances of {{{}OverloadedException{}}}s thrown with the
> message: "Query timed out before it could start."
> This message is only returned when the faulty response path is triggered and
> indicates that a response was incorrectly routed to streamId=0.
> Users may also observe codec decode failures from Cassandra clients, such as
> MalformedInputExceptions thrown in the Java driver's StringCodec.decode
> method, or exceptions such as "IllegalArgumentException: Invalid boolean
> value, expecting 1 byte but got 8" in BooleanCodec.decodePrimitive. Codecs
> for other types may present similar exceptions in decoding.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
