Skip to content

ref(scope): Let global scope flush itself - #2167

Merged
jpnurmi merged 2 commits into
masterfrom
jpnurmi/ref/scope-flush-func
Oct 12, 2026
Merged

jpnurmi merged 2 commits into
masterfrom
jpnurmi/ref/scope-flush-func

Conversation

@jpnurmi

@jpnurmi jpnurmi commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

A significant step toward letting users acquire direct access to the global scope.

One of the blocker problems for direct global scope access that came up in #2160 was that it would bypass flushing, which was previously controlled by the internal WITH_SCOPE wrapper macro. This PR solves the problem by assigning a flush_func to let the global scope flush itself after mutations.

@jpnurmi
jpnurmi added this pull request to stack #2168 October 5, 2026 14:30
@jpnurmi
jpnurmi force-pushed the jpnurmi/ref/scope-flush-func branch from 3fdc2e5 to c643461 Compare October 5, 2026 15:37

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread src/sentry_scope.c
@codecov

codecov Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.78261% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.55%. Comparing base (1ae39d3) to head (c77cb6e).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2167      +/-   ##
==========================================
+ Coverage   75.48%   75.55%   +0.07%     
==========================================
  Files         103      103              
  Lines       28089    28171      +82     
  Branches     5150     5159       +9     
==========================================
+ Hits        21202    21286      +84     
+ Misses       5545     5530      -15     
- Partials     1342     1355      +13     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ 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 c441a85. Configure here.

Comment thread src/sentry_scope.c
Comment thread src/sentry_core.c Outdated
Base automatically changed from jpnurmi/ref/breadcrumb-scope-observers to master October 6, 2026 04:48
@jpnurmi
jpnurmi force-pushed the jpnurmi/ref/scope-flush-func branch from 37d1de1 to 1a7ddcf Compare October 6, 2026 04:48
@jpnurmi
jpnurmi removed this pull request from stack #2168 October 6, 2026 13:22
@jpnurmi jpnurmi changed the title ref(scope): Let the global scope handle its own flushing ref(scope): Let global scope flush itself Oct 6, 2026
@jpnurmi
jpnurmi force-pushed the jpnurmi/ref/scope-flush-func branch from 1a7ddcf to 767513a Compare October 6, 2026 15:04

@limbonaut limbonaut left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was flagged during a scan:

Running flush_func under observers_lock can deadlock with sentry_close(). With Crashpad’s external reporter enabled on macOS or Windows, the flush calls sentry__envelope_new(), which acquires the options lock. Concurrent shutdown holds that lock and waits for observers_lock, so both threads block indefinitely. I reproduced this lock ordering with an isolated backend hook. Using sentry__envelope_new_with_dsn(options->dsn) in flush_external_crash_report() avoids the nested options-lock acquisition and lets the reproduction complete.

Suggested fix:

--- a/src/backends/sentry_backend_crashpad.cpp
+++ b/src/backends/sentry_backend_crashpad.cpp
@@ -320,7 +320,7 @@ flush_external_crash_report(crashpad_state_t *state,
     const base::FilePath &external_report_path, const sentry_options_t *options,
     const sentry_uuid_t *crash_event_id)
 {
-    sentry_envelope_t *envelope = sentry__envelope_new();
+    sentry_envelope_t *envelope = sentry__envelope_new_with_dsn(options->dsn);
     if (!envelope) {
         return;
     }

@limbonaut limbonaut left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement. Scope setters know better which changes require persistence, so the caller does not have to pick the correct macro (a potential failure point). And it helps batching. Take a look at the flagged issue above before merging.

@jpnurmi
jpnurmi added this pull request to stack #2183 October 9, 2026 08:28
A step toward letting users acquire access to the global scope.

One of the blocker problems for direct global scope access was that it
would bypass flushing, which was previously controlled by the internal
WITH_SCOPE wrapper macro. This PR solves the problem by assigning a
flush_func to the global scope, and letting it flush itself whenever
appropriate.
@jpnurmi
jpnurmi force-pushed the jpnurmi/ref/scope-flush-func branch from 712e704 to 822310f Compare October 9, 2026 09:07
@jpnurmi
jpnurmi merged commit 6e2447d into master Oct 12, 2026
71 checks passed
@jpnurmi
jpnurmi deleted the jpnurmi/ref/scope-flush-func branch October 12, 2026 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants