ref!: sentry_value-based attachments - #1974
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1974 +/- ##
==========================================
+ Coverage 74.37% 74.67% +0.30%
==========================================
Files 103 103
Lines 26817 27021 +204
Branches 4881 4911 +30
==========================================
+ Hits 19946 20179 +233
+ Misses 5531 5504 -27
+ Partials 1340 1338 -2 🚀 New features to boost your workflow:
|
4ecd0c8 to
7cfebae
Compare
7cfebae to
ed216c7
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
77576ea to
8349c66
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
8349c66 to
c7f80db
Compare
c7f80db to
b901758
Compare
jpnurmi
changed the title
4677c4e to
4c21efb
Compare
d521912 to
08e8f8c
Compare
08e8f8c to
ef00839
Compare
f08ef65 to
ed71879
Compare
ed71879 to
12cec9b
Compare
Allow callers with borrowed path strings to obtain the final path component without allocating a sentry_path_t. This prepares value-based attachment properties, which expose borrowed strings.
12cec9b to
0772fd8
Compare
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 0772fd8. Configure here.
jpnurmi
changed the title
|
On it - might take a little bit 😅 |
|
My apologies for the worst kind of PR, mixing up noisy mechanical churn with behavior changes. 😅 |
limbonaut
left a comment
limbonaut
left a comment
There was a problem hiding this comment.
Nice, some good code here! I couldn't spot any target 🎯 to assassinate 🥷
Just nits and an issue flagged by an AI scan (a good catch I think).
|
|
||
| **Breaking / Important behavior changes**: | ||
|
|
||
| - Attachment APIs now use `sentry_value_t` and `sentry_uuid_t` instead of `sentry_attachment_t *` handles. Most attachment APIs, function names and arguments, are otherwise unchanged. ([#1974](https://github.com/getsentry/sentry-native/pull/1974)) |
There was a problem hiding this comment.
Should we mention that attachment value is now consumed/frozen after adding?
| } | ||
| size_t size = sentry__attachment_get_size(attachment); | ||
| if (size > SENTRY_MAX_ATTACHMENT_SIZE) { | ||
| const char *filename = sentry__attachment_get_filename(attachment); |
There was a problem hiding this comment.
nit: repeated below - can just move out of the branch
| if (memcmp(existing_id.bytes, attachment_id->bytes, | ||
| sizeof(existing_id.bytes)) | ||
| == 0) { | ||
| sentry_value_t removed = sentry_value_incref(existing); |
There was a problem hiding this comment.
Should this also unfreeze, since the add-path freezes it? I may not grasp the whole picture here.
| return; | ||
| } | ||
| sentry_path_t *path | ||
| = make_attachment_path(options->run->run_path, attachment); |
There was a problem hiding this comment.
Clanker flagged an issue with WER and large attachments, introduced in this PR. Check it out:
P2: Preserve WER file registration for byte attachments larger than 64 KiB
With SENTRY_BACKEND=native and SENTRY_INTEGRATION_WER=ON, this change causes byte attachments larger than 64 KiB to stop being registered with Windows Error Reporting (WER).
For example, after SDK initialization:
static char diagnostic_data[128 * 1024];
sentry_attach_bytes(
diagnostic_data, sizeof(diagnostic_data), "diagnostics.bin");Previously, the native backend wrote these bytes to disk and stored the resulting path on the attachment. When the scope notified WER, it could discover that path and call WerRegisterFile.
The new implementation still writes the file, but keeps its path local to the backend. The registration sequence now becomes:
-
sentry_add_attachment()calls the backend before inserting the attachment into the scope and notifying its observers. -
native_backend_add_attachment()derives<run>/<attachment UUID>/<filename>and writes the bytes there, without storing that path on the attachment. -
wer_attachment_path()still reads only the attachment’s"path"property, which is absent for byte attachments. -
wer_add_attachment()falls back to memory registration, guarded by:if (buf && buf_len > 0 && buf_len <= WER_MAX_MEM_BLOCK_SIZE)
WER_MAX_MEM_BLOCK_SIZEis 64 KiB, so the 128 KiB attachment is silently skipped.
The size limit already existed. The regression is that native byte attachments now reach that limit instead of being registered as files.
A base-versus-PR reproduction produced:
| Attachment size | Base | PR |
|---|---|---|
| 64 KiB | File registered | Memory registered |
| 64 KiB + 1 byte | File registered | Nothing registered |
| 128 KiB | File registered | Nothing registered |
The reproduction exercised the SDK attachment flow and extracted backend/WER callback code, with Windows API calls mocked on Linux. Actual Windows crash-report collection was not tested.
Could we expose the backend’s materialized attachment path to WER through an internal lookup helper or backend callback? WER could then register the successfully written file and retain the corresponding registration state for removal.
Simply restoring a "path" assignment would not cover attachments already frozen by insertion into another scope. A backend lookup preserves attachment immutability.
Please also cover registration above 64 KiB, matching unregistration, and failed file writes.
2 participants
Caution
BREAKING CHANGE
Attachment APIs now use
sentry_value_tandsentry_uuid_tinstead ofsentry_attachment_t *.Scope data is moving to fine-grained read/write locking (#1877) to allow concurrent readers. A read-locked getter cannot safely return a borrowed
sentry_attachment_tpointer because a writer may remove and free the attachment as soon as the read lock is released, crashing a potential concurrent reader.Represent attachments and attachment collections as refcounted
sentry_value_tobjects. A getter can retain the value under the read lock and return an owned reference that remains valid after unlocking.Configure attachments before insertion and freeze them once added. Use UUIDs for removal, and retain byte values while envelopes borrow their payload to avoid copying attachment data.
See also:
Resolves: #1945