C. Scott Andreas created CASSANDRA-21508:
--------------------------------------------
Summary: 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
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]
