[
https://issues.apache.org/jira/browse/CASSANDRA-21508?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
C. Scott Andreas updated CASSANDRA-21508:
-----------------------------------------
Bug Category: Parent values: Correctness(12982)Level 1 values:
Unrecoverable Corruption / Loss(13161)
Complexity: Normal
Discovered By: Code Inspection
Severity: Critical
Status: Open (was: Triage Needed)
> 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]
