feat(internal): add scope observer#1839
Conversation
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
8bdc687 to
f015370
Compare
This reverts commit 2d983e7 because it's annoyingly complicated and probably not needed.
267c782 to
2ad380b
Compare
|
I got rid of the annoying |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1839 +/- ##
==========================================
+ Coverage 75.18% 75.50% +0.32%
==========================================
Files 91 91
Lines 21379 21595 +216
Branches 3764 3836 +72
==========================================
+ Hits 16074 16306 +232
+ Misses 4477 4457 -20
- Partials 828 832 +4 🚀 New features to boost your workflow:
|
limbonaut
left a comment
limbonaut
left a comment
There was a problem hiding this comment.
The most complete package among all SDKs, and well-covered. Nice work!
I left some suggestions for additional tests for you to consider if they're worth adding.
| if (g_scope.is_notifying > 0) { | ||
| // defer the flush requested by a reentrant scope change | ||
| g_scope.pending_flush = flush || g_scope.pending_flush; |
| * registration; the caller must not free it after that point. | ||
| */ | ||
| typedef struct sentry_scope_observer_s { | ||
| void *data; |
There was a problem hiding this comment.
Should it be mentioned in doc comments?
| sentry_value_decref(d.attachments); | ||
| sentry_close(); | ||
| } | ||
|
|
There was a problem hiding this comment.
sentry_clear_attachments() and sentry_scope_set_fingerprints() could use some coverage.
There was a problem hiding this comment.
And also worth considering if deferred flush mechanics are worth testing against something like a fake backend.
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 5ccbbeb. Configure here.
| = sentry__attachments_add(&scope->attachments, attachment); | ||
| if (added == attachment) { | ||
| SENTRY_SCOPE_NOTIFY(scope, add_attachment, attachment); | ||
| } |
There was a problem hiding this comment.
Freed pointer compared after add
Medium Severity
sentry__attachments_add frees the input on duplicate or oversized rejection, then added == attachment still reads that pointer. That is undefined behavior on the failure path and can be miscompiled; notify gating should not depend on a freed pointer value.
Reviewed by Cursor Bugbot for commit 5ccbbeb. Configure here.
3 participants
A callback-based observer API that fires when scope properties change:
Battle-tested by the upcoming WER integration (#1837)
and Crashpad IPC (#1840, abandoned).#skip-changelog (internal)