Skip to content

fix: reset client report counters during initialization#1632

Merged
tustanivsky merged 4 commits intomasterfrom
fix/discard-count-reset
Apr 10, 2026
Merged

fix: reset client report counters during initialization#1632
tustanivsky merged 4 commits intomasterfrom
fix/discard-count-reset

Conversation

@tustanivsky
Copy link
Copy Markdown
Collaborator

@tustanivsky tustanivsky commented Apr 10, 2026

This PR fixes a bug where the global g_discard_counts array in the client report system is not reset between
sentry_close() / sentry_init() cycles. As a result, discard counts from a previous SDK session leak into the next
one within the same process.

This causes the client_report_concurrent unit test to fail on platforms that run all tests in a single process (e.g.
Xbox, where process forking is unavailable). A prior test leaves one residual discard count, causing the concurrent
test to observe 80,001 instead of the expected 80,000 (failing Xbox CI run example).

The fix introduces sentry__client_report_reset() and calls it at the start of sentry_init(), ensuring that each SDK
initialization begins with a clean state. This is also semantically correct for production: applications that
re-initialize the SDK within a single process should not carry over stale discard counts into a new session.

Related items:

@jpnurmi
Copy link
Copy Markdown
Collaborator

jpnurmi commented Apr 10, 2026

This is also semantically correct for production: applications that re-initialize the SDK within a single process should not carry over stale discard counts into a new session.

I went a bit back and forth with the init-time reset. I ended up leaving it out because as far as I know, client reports are not associated with sessions, so carrying over the counts felt like the right behavior. I don't have a strong opinion on this, but perhaps fixing the concurrent test could be an alternative option?

@tustanivsky tustanivsky marked this pull request as ready for review April 10, 2026 10:27
@jpnurmi
Copy link
Copy Markdown
Collaborator

jpnurmi commented Apr 10, 2026

Wouldn't it be the correct behavior to reset the counters at the beginning of test_client_report_concurrent since the test assumes/requires a clean state, instead of throwing away counts in production? The test could either call sentry__client_report_reset or sentry__client_report_save() with a temp throwaway report to drain the counters.

@tustanivsky
Copy link
Copy Markdown
Collaborator Author

Even though client reports aren’t technically tied to sessions, I'd say that carrying them over into a new SDK lifecycle feels somewhat misleading compared to simply dropping them (which as I understand from https://develop.sentry.dev/sdk/telemetry/client-reports/#core-operation is acceptable).

We could fix this in the test itself but wouldn't that mean individual tests depend on that implicit global state shared across SDK sessions? Adding new tests or simply reordering existing ones could break things again.

@jpnurmi
Copy link
Copy Markdown
Collaborator

jpnurmi commented Apr 10, 2026

I think either way makes sense. :) @supervacuus do you have any preference?

@supervacuus
Copy link
Copy Markdown
Collaborator

I think either way makes sense. :) @supervacuus do you have any preference?

Nope, not really. As always, when specs are unclear, I would look at how a sample of other SDKs is doing it.

Copy link
Copy Markdown
Collaborator

@jpnurmi jpnurmi left a comment

Choose a reason for hiding this comment

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

Thanks! Looks like other SDKs implicitly reset the counts because they are typically stored in some Client/Transport/Options/Recorder instance, which is recreated on SDK re-init.

Copy link
Copy Markdown

@cursor cursor bot left a comment

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.

Reviewed by Cursor Bugbot for commit 63cdeec. Configure here.

@tustanivsky tustanivsky merged commit 91e7a66 into master Apr 10, 2026
58 checks passed
@tustanivsky tustanivsky deleted the fix/discard-count-reset branch April 10, 2026 14:41
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