Drop malformed tuple payloads instead of killing the receiving worker - #9076
Drop malformed tuple payloads instead of killing the receiving worker#9076L1nq0 wants to merge 2 commits into
Conversation
A tuple payload that cannot be decoded escaped recv() into the Netty fatal handler, terminating the worker. The supervisor restarted the worker, and the same poison message terminated it again. recv() now catches per-message deserialization failures whose cause chain contains one of the exceptions raised by undecodable payloads (IOException, KryoException, IllegalArgumentException, NegativeArraySizeException, ClassCastException, ArrayIndexOutOfBoundsException, BufferUnderflowException, NullPointerException, ClassNotFoundException). The offending message is dropped, the failure is logged with the destination task and payload size, the count is exposed as a deserializationFailures metric next to the message size metrics, and the rest of the batch is delivered. Any other Exception still propagates unchanged, and Errors are not caught. apache#9074
rzo1
left a comment
rzo1
left a comment
There was a problem hiding this comment.
Thanks — this is a well-scoped change and the reasoning in the description is sound. I built the branch and ran the new tests locally on JDK 25: 9/9 pass. Note that CI has not run on this PR yet (fork workflows need approval here); I'll get that triggered.
Two things I want to endorse explicitly, because they are the parts someone will inevitably want to change:
Not widening StormServerHandler.ALLOWED_EXCEPTIONS is correct. That whitelist is about transport-level channel errors, and handling a decode failure where the decode happens is what keeps the rest of the batch alive. MessageDecoder-level failures already land in MessageDecoder.exceptionCaught → ctx.close(), so recv() really is the only path that kills the worker. Please keep it as is.
Continuing the loop after a mid-stream failure is safe, which was my main correctness worry — the thread-local KryoTupleDeserializer is reused for the next message in the batch. I checked: Kryo 5.6.2 resets in a finally (Kryo.java:629), so the class resolver isn't left dirty. Your testJavaFallbackMissingClassDroppedAndBatchContinues case exercises exactly the name-based path where that would have bitten. Good test choice.
Now the review proper.
1. The remaining DoS is log volume
"One frame kills the worker" becomes "one ERROR with a full stack trace per frame". An attacker who can reach the worker port now fills the log volume instead of killing the process — better, but still an unauthenticated remote knob on a shared disk.
Please rate-limit: log the first N and then periodically with a running total. There's precedent in WorkerState.dropMessage (WorkerState.java:622), which carries a running dropCount in the message.
The same applies to the non-hostile case, which I think matters more day to day: a class genuinely missing from the worker classpath now produces ClassNotFoundException on every tuple. The topology silently makes no progress while emitting one stack trace per tuple, where before it failed loudly and obviously. A WARN on N consecutive failures would keep that visible in an operator's face.
2. NullPointerException in the tolerated set is papering over a missing check
The failure mode you map it to — a bogus task id inside a tuple — comes from KryoTupleDeserializer.deserializeTuple (KryoTupleDeserializer.java:79-80): context.getComponentId(taskId) returns null for an unknown id, and then ids.getStreamName(null, streamId) NPEs. That's two lines of validation at the source throwing a typed exception, rather than blanket-tolerating NPE at the callback.
It matters because Utils.exceptionCauseIsInstanceOf walks the entire cause chain, and user-supplied serializers registered via topology.kryo.register run inside des.deserialize. A real bug in one of those that surfaces as an NPE (or IllegalArgumentException) is now silently dropped data rather than a loud failure — the opposite of what you want for a topology-logic bug.
The durable shape is a dedicated TupleDeserializationException wrapping everything the decode path can legitimately throw, caught narrowly here. If that's more than you want to take on in this PR, dropping NullPointerException from the set and adding the explicit task-id check would get most of the way.
3. The metric lands in the wrong namespace
deserializationFailures is merged into the same map as the "srcTask-destTask" byte counts, and Server.getState() publishes that whole map under "messageBytes" (Server.java:245-249). So it surfaces as __recv-iconnection.messageBytes.deserializationFailures, inside a namespace whose keys are otherwise src-dest pairs. Nothing in-tree parses those keys, but external metrics consumers do.
Put it at the top level of Server.getState() instead (ret.put("deserializationFailures", ...)) and leave getValueAndReset() returning null when size metrics are off — that keeps the existing contract untouched.
Related: emitting the key only when failures > 0 makes it appear and disappear between reporting buckets, and most backends want a stable 0. Moving it out of messageBytes lets you always emit it and solves both at once.
Also __recv-iconnection is documented at docs/Metrics.md:291; a new user-visible counter should get a line there.
Smaller things
- Import order:
com.esotericsoftware.kryo.KryoExceptionsits after thejava.*block inDeserializingConnectionCallback.javabut correctly before it in the test.CustomImportOrderis severity=warning so it won't fail the build, just inconsistent within the one PR. - The test reflectively writes the
private final ThreadLocal desfield. It works on JDK 25 — I ran it — but final-field reflection is exactly what JEP 500's integrity-by-default is aimed at, and this will become a maintenance problem. A package-private constructor taking the deserializer would outlive it. isToleratedDeserializationFailurewalks the full cause chain once per entry in the set, so nine passes per failure. Irrelevant at current volumes, and if you rate-limit the logging it stays irrelevant — mentioning it only because a single pass over the chain testinginstanceofagainst the list would be simpler to read.
Merge order
I'd like to land this before #9075. That PR's filter rejects a payload with an InvalidClassException, which on master propagates up through recv() and kills the worker — turning an RCE into a remote worker-kill. InvalidClassException extends IOException, so with this change in first, a filtered payload is dropped and counted instead. The two compose nicely in that order.
|
Thanks for the careful review, and for independently checking the two spots I was most concerned about: the Kryo reset on a mid-stream failure and the MessageDecoder path. Your read matches what I found, so it's good to have it confirmed. On the numbered points:
Smaller things, all accepted: import order fixed to match the test file, the reflective write to the ThreadLocal replaced by a package-private constructor taking the deserializer, and the tolerance check rewritten as a single pass over the cause chain testing instanceof against the list. Merge order: agreed. This PR stays independent of #9075, and I'll rebase #9075 onto it after it lands. Thanks also for triggering the workflows. |
…erver
A topology stuck receiving poison payloads would flood the worker log with
one ERROR per dropped message. recv() now logs the first 10 failures
individually, then one summary ERROR per 100 further failures carrying the
running total, in the WorkerState "Total Drop Count= {}" style. 1000
consecutive failures without a success log a single WARN pointing at a
persistent fault; any successful deserialization resets that counter.
NullPointerException stays outside the tolerated set: it usually signals a
bug rather than a malformed payload. The case a bad tuple could trigger,
an unknown source task, is rejected up front in KryoTupleDeserializer
with IllegalArgumentException naming the task; that lookup NPEd during
stream resolution before this change.
Server.getState() publishes deserializationFailures as a top-level key,
always present, including when it is 0, read through
getAndResetDeserializationFailures() on the callback. getValueAndReset()
reports only the size metrics, null when they are disabled.
isToleratedDeserializationFailure walks the exception cause chain once and
checks every tolerated type per frame, instead of once per type.
Tests inject a replacement deserializer through a package-private setter,
and a new ServerTest covers the top-level key.
apache#9074
|
The revisions are pushed. Drop logging is now rate-limited: the first 10 failures log individually, then one NullPointerException is out of the tolerated set, and KryoTupleDeserializer rejects deserializationFailures is a top-level key of __recv-iconnection now, always present The full storm-client suite is green locally (680 tests) with no new checkstyle |
2 participants
Closes #9074
Upgrade note: a malformed message body no longer kills the receiving worker, including the IOException case. Previously a channel-level IOException closed the connection and lost every message still queued on it; now the one undecodable message is dropped and the connection, along with the rest of the batch, keeps going. Payloads that used to tear down a connection mid-stream will instead surface as deserializationFailures counts and per-message ERROR logs.
What this changes
DeserializingConnectionCallback.recv() now wraps each message's deserialization in a try/catch. When the failure's cause chain matches a known decode-time exception type, the message is dropped with an ERROR log (exception class and message, destination task id, payload length, and no payload bytes) and the loop continues with the next message in the batch. Anything outside that set is rethrown and keeps today's behavior: StormServerHandler.exceptionCaught, then Utils.handleUncaughtException, then worker exit. Errors are never caught.
The tolerated set is the set of exception types a garbage or hostile byte stream can already produce during tuple decode, checked with the existing cause-chain helper Utils.exceptionCauseIsInstanceOf:
IOException, KryoException, IllegalArgumentException, NegativeArraySizeException, ClassCastException, ArrayIndexOutOfBoundsException, BufferUnderflowException, NullPointerException, ClassNotFoundException
Mapping to wire-level failure modes: truncated or negative length fields (ArrayIndexOutOfBoundsException, NegativeArraySizeException, BufferUnderflowException), unregistered classes under topology.kryo.register with registration required (IllegalArgumentException), classes present only on the sending side (ClassNotFoundException), bogus task ids inside a tuple (NullPointerException from task-info lookup), wrong runtime types (ClassCastException), and kryo's own decode failures (KryoException) plus underlying stream problems (IOException).
I deliberately did not broaden StormServerHandler.ALLOWED_EXCEPTIONS. That whitelist is about transport-level channel errors and closing the connection; a payload that fails to decode is an application-level event, and handling it where the decode happens is what keeps the rest of the batch alive. The whitelist stays as the backstop for anything that still escapes.
Failures are counted in a deserializationFailures counter exposed through the existing getValueAndReset() metrics contract: it stays null until a failure occurs when tuple-size metrics are disabled, matching how the adjacent serialization metrics behave.
Tests
9 cases in DeserializingConnectionCallbackTest, driving recv() through the real deserializer where possible: one tolerant drop per interesting exception type (verify log, counter, and batch continuation), an IOException mid-batch that skips exactly the bad message while a good message later in the same batch still lands, rethrow of a non-tolerated exception, counter null when nothing failed, and counter reset after getValueAndReset().
Also verified on a live 3.0.1-SNAPSHOT cluster: replaying a 27-byte frame carrying an unregistered class against a worker port kills the worker on master (terminating server, then process exit and supervisor restart) and on this branch produces the log line "Failed to deserialize a message of 27 bytes destined for task 2, dropping it" with the worker surviving and the topology staying active.