[
https://issues.apache.org/jira/browse/CASSANDRA-21508?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
C. Scott Andreas updated CASSANDRA-21508:
-----------------------------------------
Attachment: StreamIdMisrouteTest.java
> 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
> Priority: Normal
> 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{}}}:
> if (queueTime >
> DatabaseDescriptor.getNativeTransportTimeout(TimeUnit.NANOSECONDS))
> { ClientMetrics.instance.markTimedOutBeforeProcessing(); return
> ErrorMessage.fromException(new OverloadedException("Query timed out before it
> could start"));
> } // <-- no setStreamId(request.getStreamId())
>
> 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 (a different prepared
> statement, with its own cached column definitions).
> # 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
> ({{{}ResultSet.java{}}}: the column count is written unconditionally, but the
> name/type loop is skipped under the no-metadata flag), 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.
> Diagram:
>
> {{{{ Client (java-driver) Server (coordinator,
> overloaded)
> --------------------
> --------------------------------
> B = healthy query --- stream 0 ---> B queued, still
> processing ...
> A = other query --- stream 1 ---> A sits in NTR queue too
> long
> -> load-sheds A: sends an
> overload
> error but does not set
> stream id
> <-- error --------- => stamped stream id
> 0 [defect]
> (stream 0)
> inFlight[0] == B => error delivered to B,
> B completed, stream 0 released (normal reuse,
> not an orphan)
> C = new query --- reuse 0 ---> B's real read finally
> finishes ...
> <-- rows ----------- B's rows sent on
> stream 0 (correct
> (stream 0) from the server's
> view)
> inFlight[0] == C => B's rows decoded against
> C's column definitions (skip-metadata: no
> column info in the frame)
> => a value from B lands in the wrong column of C}}}}
>
> *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.
> Run:
> {{ant test-jvm-dtest-some
> -Dtest.name=org.apache.cassandra.distributed.test.StreamIdMisrouteTest}}
>
> *Expected*
> The load-shed OVERLOADED error carries the stream id of the request that
> timed out, so the client attributes it to the correct request and never frees
> a stream id that still has an in-flight response.
>
> *Actual*
> The OVERLOADED error carries stream id 0. On a busy connection it is
> delivered to an unrelated in-flight request, whose stream id is then freed
> and reused, so a later legitimate rows response is decoded against the wrong
> query's column definitions.
>
> *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;
> }
>
> With this change the shed error is delivered to the request that actually
> timed out, and no unrelated stream id is freed early.
>
> 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.
>
> *Reproduction 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].
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
