fix(batcher): replace static global with dynamic allocation and refcounting by jpnurmi · Pull Request #1556 · getsentry/sentry-native

jpnurmi · 2026-03-06T07:32:38Z

This PR adds a new stress test that spawns multiple producer threads continuously logging while the main thread repeatedly re-inits the SDK, and fixes the resulting issue flagged by TSan:

$ RUN_ANALYZER=tsan pytest -v --capture=no "tests/test_unit.py::test_unit[logs_reinit_stress]"
...
WARNING: ThreadSanitizer: data race (pid=1194861)
  Write of size 8 at 0x5555556341b8 by main thread (mutexes: write M0):
    #0 sentry__batcher_startup sentry-native/src/sentry_batcher.c:302 (sentry_test_unit+0x37c38) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #1 sentry__logs_startup sentry-native/src/sentry_logs.c:463 (sentry_test_unit+0x48297) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #2 sentry_init sentry-native/src/sentry_core.c:302 (sentry_test_unit+0x3903f) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #3 test_sentry_logs_reinit_stress sentry-native/tests/unit/test_logs.c:461 (sentry_test_unit+0x7da63) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #4 test_do_run__ sentry-native/include/../vendor/acutest.h:943 (sentry_test_unit+0x70ad1) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #5 test_run__ sentry-native/include/../vendor/acutest.h:1114 (sentry_test_unit+0x1d154) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #6 main sentry-native/include/../vendor/acutest.h:1623 (sentry_test_unit+0x1d154)

  Previous read of size 8 at 0x5555556341b8 by thread T2:
    #0 pthread_cond_signal ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1319 (libtsan.so.2+0x8b77e) (BuildId: edd4cc50d12638c09456940b2f9b3fb7d4f8eec4)
    #1 sentry__batcher_enqueue sentry-native/src/sentry_batcher.c:203 (sentry_test_unit+0x37ad1) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #2 sentry__logs_log sentry-native/src/sentry_logs.c:385 (sentry_test_unit+0x47892) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #3 sentry__logs_log sentry-native/src/sentry_logs.c:362 (sentry_test_unit+0x47e9c) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)
    #4 sentry_log_info sentry-native/src/sentry_logs.c:422 (sentry_test_unit+0x47e9c)
    #5 log_producer_thread sentry-native/tests/unit/test_logs.c:442 (sentry_test_unit+0x7badd) (BuildId: 19335422fa6a19b9e786eea02ccd28b1770e2ea3)

  Location is global 'g_batcher' of size 248 at 0x555555634120 (sentry_test_unit+0xe01b8)

jpnurmi · 2026-03-06T09:17:32Z

@sentry review

jpnurmi · 2026-03-06T09:17:35Z

@cursor review

src/sentry_logs.c

jpnurmi · 2026-03-06T09:34:57Z

@sentry review

jpnurmi · 2026-03-06T09:35:04Z

@cursor review

src/sentry_batcher.c

jpnurmi · 2026-03-06T10:17:28Z

@sentry review

jpnurmi · 2026-03-06T10:17:36Z

@cursor review

src/sentry_batcher.c

jpnurmi · 2026-03-06T10:46:28Z

@sentry review

jpnurmi · 2026-03-06T10:46:35Z

@cursor review

src/sentry_logs.c

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Spawns 8 producer threads continuously logging while the main thread does 10 cycles of sentry_init/sentry_close. Reproduces a condvar corruption hang in the batcher under TSan. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…unting The static g_batcher persisted across init/close cycles, causing condvar corruption when sentry__batcher_startup re-initialized the condvar while old threads still used it. Dynamic allocation with refcounting solves both the condvar corruption and the lifetime problem. Introduces sentry_batcher_ref_t with acquire/release/swap API that encapsulates a spinlock to make concurrent access from producer threads and shutdown safe. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

When the previous startup failed to spawn its thread, shutdown_begin returns false and shutdown_wait is never called. The old batcher stays in g_batcher and leaks when startup swaps it out without releasing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

A producer that acquired a ref before shutdown could enqueue items after the final flush. These items were leaked when the batcher was freed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add sentry__cond_destroy for all platforms (pthread_cond_destroy on POSIX, CloseHandle on pre-Vista Windows, no-op on Vista+) and call it in sentry__batcher_release. Without this, each SDK re-initialization leaks kernel handles on pre-Vista Windows and violates POSIX which requires pthread_cond_destroy before freeing memory containing a pthread_cond_t. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

This reverts commit 467bfb5ecb2c4528e1048dab1c467203fdc29cdf.

The two-phase shutdown (begin/wait) was designed to signal both the logs and metrics threads in parallel before joining either. With #1558 replacing the condvar with a level-triggered waitable flag, thread wake latency drops to sub-millisecond (futex/WaitOnAddress), making the parallel signaling unnecessary. Merging into a single function also fixes a race where a concurrent sentry_init() could replace g_batcher between the signal and join steps, causing the new batcher to be shut down instead of the old one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpnurmi · 2026-03-06T15:55:09Z

@cursor review

src/sentry_logs.c

The crash-safe flush functions called sentry__batcher_acquire, which spins on a spinlock. If the crash occurs on the thread holding that lock, the signal handler deadlocks. Replace with a new lock-free sentry__batcher_peek that reads ref->ptr via atomic load without taking the spinlock or bumping the refcount (safe because the process is dying). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

src/sentry_batcher.c

On Windows x64, `long` is 32-bit but pointers are 64-bit. Use InterlockedCompareExchangePointer on Windows and __atomic_load elsewhere to avoid truncating the pointer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

src/sentry_metrics.c

force_flush_begin and force_flush_wait each acquired the batcher independently. A concurrent sentry_init could swap the batcher between the two calls, causing wait to flush the new empty batcher while the original data is lost. Fix by returning an opaque token (the acquired batcher ref) from begin and passing it to wait, ensuring both operate on the same instance. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpnurmi · 2026-03-06T17:12:51Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

supervacuus

This looks very clean! Only minor comments.

src/sentry_logs.c

src/sentry_batcher.c

Co-authored-by: Mischan Toosarani-Hausberger <mischan@abovevacant.com>

cursor

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

Bugbot Autofix prepared a fix for the issue found in the latest run.

✅ Fixed: Metrics startup swaps NULL batcher on allocation failure
- Updated metrics startup to return early on batcher allocation failure and to shut down any swapped-out batcher before releasing it.

Or push these changes by commenting:

@cursor push e977efb2bc

Preview (e977efb2bc)

diff --git a/src/sentry_metrics.c b/src/sentry_metrics.c
--- a/src/sentry_metrics.c
+++ b/src/sentry_metrics.c
@@ -133,10 +133,17 @@
 {
     sentry_batcher_t *batcher
         = sentry__batcher_new(sentry__envelope_add_metrics);
-    if (batcher) {
-        sentry__batcher_startup(batcher, options);
+    if (!batcher) {
+        SENTRY_WARN("failed to allocate metrics batcher");
+        return;
     }
-    sentry__batcher_release(sentry__batcher_swap(&g_batcher, batcher));
+
+    sentry__batcher_startup(batcher, options);
+    sentry_batcher_t *old = sentry__batcher_swap(&g_batcher, batcher);
+    if (old) {
+        sentry__batcher_shutdown(old, 0);
+    }
+    sentry__batcher_release(old);
 }
 
 void

_{This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.}

src/sentry_metrics.c

…er-global

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpnurmi changed the title ~~test(logs): add stress test for batcher reinit hang~~ test(logs): add stress test for batcher reinit Mar 6, 2026

jpnurmi changed the title ~~test(logs): add stress test for batcher reinit~~ fix(batcher): replace static global with dynamic allocation and refcounting Mar 6, 2026

sentry bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_logs.c Show resolved Hide resolved

cursor bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_logs.c Outdated Show resolved Hide resolved

src/sentry_logs.c Outdated Show resolved Hide resolved

sentry bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_batcher.c Show resolved Hide resolved

cursor bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_batcher.c Show resolved Hide resolved

sentry bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_batcher.c Show resolved Hide resolved

cursor bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_batcher.c Show resolved Hide resolved

sentry bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_logs.c Outdated Show resolved Hide resolved

cursor bot reviewed Mar 6, 2026

View reviewed changes

jpnurmi and others added 12 commits March 6, 2026 16:45

test(logs): add stress test for batcher reinit hang

a7f8505

Spawns 8 producer threads continuously logging while the main thread does 10 cycles of sentry_init/sentry_close. Reproduces a condvar corruption hang in the batcher under TSan. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix(batcher): NULL-guard batcher in shutdown_wait

0e419f1

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Update CHANGELOG.md

75ffc91

fix(batcher): drain buffer items in release to prevent leaks

b4c7bef

A producer that acquired a ref before shutdown could enqueue items after the final flush. These items were leaked when the batcher was freed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

refactor(batcher): use SENTRY_MAKE macro for allocation

d125155

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Revert "fix(batcher): destroy condvar before freeing batcher memory"

0c8200d

This reverts commit 467bfb5ecb2c4528e1048dab1c467203fdc29cdf.

test(metrics): add reinit stress test

c6da129

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

test: speed up reinit stress tests

ba0b566

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpnurmi force-pushed the jpnurmi/fix/batcher-global branch from a3738d5 to ba0b566 Compare March 6, 2026 15:45

jpnurmi marked this pull request as ready for review March 6, 2026 15:45

cursor bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_logs.c Show resolved Hide resolved

cursor bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_batcher.c Show resolved Hide resolved

jpnurmi and others added 2 commits March 6, 2026 17:21

docs(batcher): add header comments for ref management functions

7e2d690

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sentry bot reviewed Mar 6, 2026

View reviewed changes

src/sentry_metrics.c Outdated Show resolved Hide resolved

cursor bot reviewed Mar 6, 2026

View reviewed changes

jpnurmi requested review from JoshuaMoelans, mujacica and supervacuus March 6, 2026 17:42

mujacica approved these changes Mar 7, 2026

View reviewed changes

supervacuus approved these changes Mar 9, 2026

View reviewed changes

src/sentry_logs.c Show resolved Hide resolved

src/sentry_batcher.c Show resolved Hide resolved

jpnurmi and others added 2 commits March 9, 2026 12:47

Update src/sentry_logs.c

55daae0

Co-authored-by: Mischan Toosarani-Hausberger <mischan@abovevacant.com>

Update sentry__metrics_startup

22f81fd

cursor bot reviewed Mar 9, 2026

View reviewed changes

src/sentry_metrics.c Outdated Show resolved Hide resolved

sentry bot reviewed Mar 9, 2026

View reviewed changes

src/sentry_metrics.c Show resolved Hide resolved

jpnurmi and others added 2 commits March 9, 2026 12:51

Merge remote-tracking branch 'upstream/master' into jpnurmi/fix/batch…

03cac7e

…er-global

docs(batcher): clarify ownership semantics of options refs

d945453

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpnurmi merged commit 5f1b18c into master Mar 9, 2026
50 checks passed

jpnurmi deleted the jpnurmi/fix/batcher-global branch March 9, 2026 12:41

Uh oh!

Conversation

jpnurmi commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

Uh oh!

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

Uh oh!

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpnurmi commented Mar 6, 2026

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

supervacuus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cursor bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jpnurmi commented Mar 6, 2026 •

edited

Loading

cursor bot left a comment •

edited

Loading