fix(batcher): Do not let a failed flush kill the flusher thread (#7138) - #7186
Open
zkasuran wants to merge 1 commit into
Open
fix(batcher): Do not let a failed flush kill the flusher thread (#7138)#7186zkasuran wants to merge 1 commit into
zkasuran wants to merge 1 commit into
Conversation
…entry#7138) An unhandled exception inside `_flush_loop` terminated the batcher's daemon flusher thread. After that the buffer kept filling with nothing draining it. Every later log, metric or span was then dropped for the rest of the process lifetime once the queue hit its cap. Wrap the flush call in each loop (`Batcher._flush_loop` and `SpanBatcher._flush_loop`) in `capture_internal_exceptions()`, the SDK's own helper for errors that should be logged rather than propagated. A single bad batch is now swallowed and logged so the loop keeps running. Adds a regression test for each loop that drives one iteration where the flush raises then asserts the loop returns instead of propagating.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7138.
What
The batcher's
_flush_loopcalls_flush()unguarded. Any exception there (theRuntimeErrorin the issue, but really anything raised while serializing or sending a batch) propagates out of the loop and ends the daemon flusher thread. After that_ensure_thread()still sees_flusher_pid == pidand returnsTrue, soadd()keeps appending to a buffer that nothing drains. Once it reachesMAX_BEFORE_DROPevery later log, metric or span is dropped silently for the rest of the process lifetime.Fix
Following the issue title, wrap the flush in each loop in
capture_internal_exceptions(), the SDK's own helper for internal errors that should be logged rather than propagated:Batcher._flush_loop(logs and metrics)SpanBatcher._flush_loop(both the pending-buckets flush and the periodic full flush)A single bad batch is now logged and swallowed. The flusher thread stays alive and the next flush delivers everything that queued up in the meantime.
Tests
One regression test per loop (
test_flush_loop_swallows_flush_exceptionintests/test_logs.pyandtests/tracing/test_span_batcher.py) drives one loop iteration where_flushraises, then asserts_flush_loopreturns instead of propagating. They fail on unpatched source (theRuntimeErrorescapes the loop) and pass with the guard. Both carry thetests_internal_exceptionsmarker since they intentionally exercise the internal-exception path.Verification
pytest tests/tracing/test_span_batcher.py tests/test_logs.py tests/test_metrics.py: 68 passed.ruff checkandruff format --check: clean on the changed files.mypy sentry_sdk: no new errors (the pre-existing ones are all in an unrelated integration).AI assistance (Claude, Anthropic) was used in developing this change. The design, review and verification were done by the author. Verified locally before submitting: the tests, ruff and mypy above.