-
Notifications
You must be signed in to change notification settings - Fork 371
Ensure that we don't send massive error message over gRPC #1108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ use arroyo_operator::operator::Registry; | |
| use arroyo_planner::physical::new_registry; | ||
| use std::fmt::Write; | ||
| use std::sync::Arc; | ||
| use tracing::warn; | ||
|
|
||
| fn format_arrow_schema_fields(schema: &Schema) -> Vec<(String, String)> { | ||
| schema | ||
|
|
@@ -135,3 +136,55 @@ pub async fn to_d2(logical: &LogicalProgram) -> anyhow::Result<String> { | |
|
|
||
| Ok(d2) | ||
| } | ||
|
|
||
| pub(crate) const MAX_TASK_ERROR_FIELD_BYTES: usize = 64 * 1024; | ||
|
|
||
| pub(crate) fn maybe_truncate(mut value: String, max_size_bytes: usize) -> String { | ||
| let original_bytes = value.len(); | ||
| if original_bytes <= max_size_bytes { | ||
| return value; | ||
| } | ||
|
|
||
| let suffix = format!(" [truncated; original_bytes={original_bytes}]"); | ||
| let mut end = max_size_bytes - suffix.len(); | ||
| while !value.is_char_boundary(end) { | ||
| end -= 1; | ||
| } | ||
|
|
||
| value.truncate(end); | ||
| value.push_str(&suffix); | ||
|
|
||
| warn!( | ||
| "Truncated oversized String from {} bytes to {} bytes: {}", | ||
| original_bytes, | ||
| value.len(), | ||
| value | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I read this right, these logs will be ~64kB of unknown data. Is that something we want to include in the log?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to log them as they can be useful to understand the root cause. In theory (🤞), we shouldn't really have this truncation happen often, if at all. |
||
| ); | ||
| value | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::maybe_truncate; | ||
|
|
||
| #[test] | ||
| fn maybe_truncate_preserves_value_at_limit() { | ||
| let value = "a".repeat(64); | ||
|
|
||
| assert_eq!(maybe_truncate(value.clone(), value.len()), value); | ||
| } | ||
|
|
||
| #[test] | ||
| fn maybe_truncate_respects_limit() { | ||
| let value = "a".repeat(100); | ||
| let max_size_bytes = 64; | ||
|
|
||
| let truncated = maybe_truncate(value, max_size_bytes); | ||
|
|
||
| assert_eq!(truncated.len(), max_size_bytes); | ||
| assert_eq!( | ||
| truncated, | ||
| format!("{} [truncated; original_bytes=100]", "a".repeat(32)) | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Oversized non-fatal error messages can still exceed the gRPC limit and fail the job
The non-fatal error message and details are sent to the controller untruncated (
error: message/detailsatcrates/arroyo-worker/src/lib.rs:605-608), unlike the fatal path which was capped, so a large error can still blow past the 4MB gRPC message limit.Impact: A single oversized non-fatal error (e.g. from bad input data) makes the RPC fail, which cancels the worker and takes the whole job down — the exact scenario this change was meant to prevent.
Why the non-fatal path is also at risk
The fatal path at
crates/arroyo-worker/src/lib.rs:579-586now wraps botherroranddetailsinmaybe_truncate(..., MAX_TASK_ERROR_FIELD_BYTES). TheNonfatalErrorReqbuilt atcrates/arroyo-worker/src/lib.rs:597-610assignserror: messageanddetailsdirectly with no size guard. Thedetailsvalue originates from deserialization/bad-data handling (crates/arroyo-operator/src/context.rs:389), which can embed raw offending record contents and thus be arbitrarily large. When such a message exceeds the 4MB limit,send_control_rpc!returns an error andcancel_token.cancel()is invoked (crates/arroyo-worker/src/lib.rs:637-643), terminating the job.(Refers to lines 605-608)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed