Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Reviewed by Cursor Bugbot for commit d8eb6bb. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
d8eb6bb to
f0d7bcf
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2108 +/- ##
==========================================
+ Coverage 74.83% 74.90% +0.07%
==========================================
Files 103 103
Lines 27386 27461 +75
Branches 4944 4977 +33
==========================================
+ Hits 20494 20571 +77
- Misses 5543 5549 +6
+ Partials 1349 1341 -8 🚀 New features to boost your workflow:
|
Use a shared MessagePack stream for attachment manifests. The native
backend uses the manifest for attachments generally, while both
out-of-process crash handlers can also use it for crash-time hint
attachments that cannot cross backend IPC. This lets Crashpad consume
those attachments without adding JSON support.
Stream attachment objects directly to avoid format-specific conversion
and buffer the complete manifest for a single file write. Median release
benchmarks measured write/read improvements over legacy JSON of 17%/6%
for one attachment, 74%/17% for 10, and 86%/24% for 100:
1 attachment 10 attachments 100 attachments
MessagePack write 2.3 us 2.4 us 11.8 us
JSON write 2.7 us 8.9 us 82.6 us
MessagePack read 1.3 us 5.6 us 54.1 us
JSON read 1.4 us 6.8 us 71.1 us
Keep legacy JSON compatibility private to the native daemon so it can
still consume manifests written by older SDK versions.
f0d7bcf to
8166a35
Compare
| sentry_path_t *attachment_path | ||
| = sentry__attachment_make_run_path(run_path, attachment); | ||
| if (!attachment_path) { |
There was a problem hiding this comment.
Bug: The attachment manifest serialization code unnecessarily includes the internal uuid field for each attachment, increasing file size without providing any benefit to the reader.
Severity: MEDIUM
Suggested Fix
In the loop that builds the attachment manifest object, add a check to explicitly skip the uuid key. For example, if (strcmp(sentry_value_get_string(key), "uuid") == 0) { continue; }. This will prevent the UUID from being written to the manifest file, reducing its size and aligning with the optimization goals.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry_attachment.c#L720-L722
Potential issue: When serializing the attachment manifest to MessagePack, the code
iterates through all properties of an attachment object. It includes any property of
type `SENTRY_VALUE_TYPE_STRING`, which inadvertently includes the internal `uuid`. This
UUID is an implementation detail used for in-memory deduplication and is not used by the
daemon when reading the manifest. Including it adds unnecessary data (36 characters plus
encoding overhead per attachment), which partially counteracts the performance and size
improvements intended by the refactor.
1 participant
Use a shared MessagePack stream for attachment manifests. The native backend uses the manifest for attachments generally, while both out-of-process crash handlers will need to use it for crash-time hint attachments (#2099) that cannot cross backend IPC. This lets Crashpad consume those attachments with the existing MessagePack reader, without having to introduce JSON support/dependencies.
Stream attachment objects directly to avoid format-specific conversion and buffer the complete manifest for a single file write. Median release benchmarks measured write/read improvements over legacy JSON of 17%/6% for one attachment, 74%/17% for 10, and 86%/24% for 100:
Keep legacy JSON compatibility private to the native daemon so it can still consume manifests written by older SDK versions.