Repository navigation
Conversation
|
Local end-to-end testing exposed a parsing limit that is not part of the flamegraph contract. The MetricKit example has 719 frame entries, 19 trees, and a maximum call-path depth of 130. It is only about 128 KiB, but its nested nodes/children exceed serde_json's default recursion limit. Relay consequently dropped the attachment while retaining the event. The attachment specification limits serialized size to 10 MiB, not call-tree depth. Truncating the input to fit the parser would lose the diagnostic context this attachment format is intended to preserve. Fixed in 28b2c56. Parsing uses stack growth for deep call trees, with iterative node destruction so dropping the parsed tree is also stack-safe. The byte budget is checked before parsing. Flattened frame data retains the normal JSON nesting limit, and the existing schema and sample-count validation remain in place. Coverage includes a synthetic 1,000-frame call path and rejection of an invalid count at that depth. Replaying the untouched example through the local stacked implementation retained all 19 trees and 719 frame entries through ingestion, attachment processing, and browser rendering. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 28b2c56. Configure here.
| let mut deserializer = serde_json::Deserializer::from_slice(payload); | ||
| deserializer.disable_recursion_limit(); | ||
| let raw: Flamegraph<&serde_json::value::RawValue> = | ||
| Flamegraph::deserialize(serde_stacker::Deserializer::new(&mut deserializer))?; |
There was a problem hiding this comment.
Unbounded JSON depth can exhaust memory
Medium Severity
disable_recursion_limit plus serde_stacker lets a 10 MiB flame graph nest call trees or ignored fields hundreds of thousands of levels deep. Each level keeps a grown stack segment alive until deserialize returns, so one attachment can pin hundreds of megabytes and stall or OOM a processing worker. Invalid payloads still pay this cost before rejection; the previous 128-depth cap no longer applies.
Reviewed by Cursor Bugbot for commit 28b2c56. Configure here.


Note
This is my first contribution to relay, so it's heavily assisted by agents. Please let me know how you want me to continue working on it. Happy for any guidance for working no Relay without breaking best-practices.
Recognize and validate
event.flamegraphattachments against the draft contract in getsentry/sentry-docs#19774, preserving their type and original payload through Kafka and objectstore ingestion.Validation
Required headers and association with an explicit event or transaction in the same envelope are checked through shared attachment processing, including on non-processing Relays. Invalid attachments produce
invalid_flamegraphoutcomes without discarding the associated event. The 10 MiB cap uses the existing size-limit path, preserving shared attachment count and byte budgets, rejection metrics, and whole-envelope HTTP 413 behavior for oversized items.Processing Relays validate version 1 payloads using the V2 profiling frame and debug metadata schemas: non-empty tables and roots, native addresses, relative image references, frame indices, integer sample-count bounds, and child-count invariants. Debug-image IDs are indexed once to avoid quadratic lookup work. Validation leaves unknown fields and original symbolication inputs unchanged.
Boundary
Symbolication, its 2,500-input-frame budget, inline expansion, symbolication state, and rendering belong to separate Sentry backend and frontend PRs. Relay does not infer source thread identity or process provenance, and cannot determine whether an omitted absolute-address debug image was known to the producer. Existing attachment quotas and privacy rules still apply. This change does not add structured scrubbing or multipart filename inference.
Verification
make check, the all-feature build, default-feature Clippy with warnings denied, seven default-feature flamegraph Rust tests, eleven all-feature flamegraph Rust tests, and three transport integration cases passed locally. Added coverage checks shared size budgets, non-processing header validation, association outcomes, and large relative-address tables.Processing integration remains blocked locally by unavailable Redis services. Corrected invalid-attachment integration tests to read the preserved event from the events topic and assert that the attachments topic remains empty. Kafka and objectstore payload-preservation coverage is included but was not reverified locally during this update. The full all-feature Rust suite was not rerun.