GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix(batcher): Serialize flushed items outside the batcher lock by NicoHinderling · Pull Request #7776 · getsentry/sentry-python

Conversation

NicoHinderling commented Sep 29, 2026

Copy link

Copy Markdown

Batcher._flush held _lock while serializing the buffered items. Serializing can trigger GC, and if a collected object's finalizer logs, the flush thread needs the logging handler lock, which another thread already holds while it waits for _lock in add(). That lock-order inversion freezes every later log call in the process; it's what was hard-killing Seer's Celery tasks (#7775).

_flush now swaps the buffer out under the lock (the replacement list is allocated before taking it) and builds the envelope after releasing it. Applies to LogBatcher and MetricsBatcher.
New regression test asserts the lock isn't held while items are serialized; the repro in Deadlock between log batcher lock and logging handler lock when GC runs a finalizer that logs #7775 goes from deadlocking in ~5s to none in 5 runs of 30–40s.
Behavior change: a batch whose serialization raises is now dropped instead of being retried on every flush.
Not covered here: SpanBatcher._flush serializes under its own lock the same way, and add() calls _record_lost under the lock once the buffer is full. Happy to follow up on both.

Fixes #7775

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix(batcher): Serialize flushed items outside the batcher lock

1f3b6f0

Batcher._flush held its lock while serializing buffered items. Serializing can trigger GC, and a finalizer that logs then needs the logging handler lock, which another thread holds while waiting for the batcher lock in add(). Swap the buffer out under the lock and build the envelope after releasing it.

Fixes #7775

NicoHinderling requested a review from a team as a code owner

September 29, 2026 22:14

NicoHinderling commented Sep 29, 2026

Copy link

Copy Markdown

Author

(this was auto generated on behalf of that ticket i reported.. feel free to close this, if you would prefer to address it in a different way, I just opened it in case it's helpful)

alexander-alderman-webb reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot commented Sep 29, 2026

Copy link

Copy Markdown

Contributor

Codecov Results 📊

📊 Comparison with Base Branch

Metric	Change
Total Tests	📈 +439
Passed Tests	📈 +438
Failed Tests	📈 +1
Skipped Tests	—

➕ New Tests (2)

View new tests

test_flush_serializes_outside_the_lock
- File: tests.test_logs
- Status: ❌ Failing
test_flush_serializes_outside_the_lock
- File: tests.test_logs
- Status: ❌ Failing

➖ Removed Tests (1)

View removed tests

test_cache_spans_decorator[True]
- File: tests.integrations.django.test_cache_module

❌ Failed Tests

`test_flush_serializes_outside_the_lock`

File: tests.test_logs
Suite: py3.6-common
Error: AttributeError: module 'time' has no attribute 'time_ns'

Stack Trace

tests/test_logs.py:926: in test_flush_serializes_outside_the_lock
    sentry_sdk.logger.warning("test log")
sentry_sdk/logger.py:60: in _capture_log
    "time_unix_nano": time.time_ns(),
E   AttributeError: module 'time' has no attribute 'time_ns'

`test_flush_serializes_outside_the_lock`

File: tests.test_logs
Suite: py3.6-gevent
Error: AttributeError: module 'time' has no attribute 'time_ns'

Stack Trace

tests/test_logs.py:926: in test_flush_serializes_outside_the_lock
    sentry_sdk.logger.warning("test log")
sentry_sdk/logger.py:60: in _capture_log
    "time_unix_nano": time.time_ns(),
E   AttributeError: module 'time' has no attribute 'time_ns'

✅ Patch coverage is 100.00%. Project has 2553 uncovered lines.
❌ Project coverage is 90.25%. Comparing base (2a499ec) to head (1f3b6f0).

Coverage diff

@@            Coverage Diff             @@
##        master       #PR       +/-##
==========================================
- Coverage    90.27%    90.25%    -0.02%
==========================================
  Files          195       195         —
  Lines        26197     26198        +1
  Branches      9758      9758         —
==========================================
+ Hits         23650     23645        -5
- Misses        2547      2553        +6
- Partials      1484      1483        -1

Generated by Codecov Action

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

NicoHinderling marked this pull request as draft

September 29, 2026 23:46

alexander-alderman-webb commented Sep 30, 2026

Copy link

Copy Markdown

Collaborator

Thanks @NicoHinderling.
I don't think this is the right way to approach this because the lock should guard self._buffer and items is a shallow copy of self._buffer.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alexander-alderman-webb commented Sep 30, 2026

Copy link

Copy Markdown

Collaborator

Right this could actually work since self._buffer is cleared at the same time, so it could be safe to mutate items outside of the lock

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alexander-alderman-webb mentioned this pull request

Oct 13, 2026

fix(logs): resolve Batcher deadlock from lock-order inversion #7965

Closed

mhatipog mentioned this pull request

Oct 13, 2026

SpanBatcher serializes spans while holding its buffer lock #8023

Open

ansh-rohilla mentioned this pull request

Oct 30, 2026

fix(tracing): Serialize spans outside the SpanBatcher lock (#8023) #8170

Closed

This branch has not been deployed

No deployments

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deadlock between log batcher lock and logging handler lock when GC runs a finalizer that logs

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.