Skip to content

fix: Cancel _target task in AsyncWorker.kill() and improve sync close()

c46fb6f
Select commit
Loading
Failed to load commit list.
Sign in for the full log view
Open

feat: Add experimental async transport (port of PR #4572) #5646

fix: Cancel _target task in AsyncWorker.kill() and improve sync close()
c46fb6f
Select commit
Loading
Failed to load commit list.
GitHub Actions / warden: code-review completed Mar 12, 2026 in 8m 28s

6 issues

code-review: Found 6 issues (1 high, 4 medium, 1 low)

High

__aexit__ silently fails to close sync-transport clients - `sentry_sdk/client.py:1144-1145`

When async with Client(...) is used with a sync transport (the default without transport_async experiment), __aexit__ calls close_async(), which returns early without any cleanup at line 1073. This leaves session flusher, batchers, monitor, and transport resources unclosed. Users may reasonably use async with syntax in async code even without the async transport experiment, expecting proper cleanup.

Medium

AsyncHttpTransport ignores keep_alive configuration option - `sentry_sdk/transport.py:888-897`

The AsyncHttpTransport._get_pool_options() method always applies keep-alive socket options regardless of the keep_alive configuration setting. This differs from HttpTransport._get_pool_options() which only applies them when self.options["keep_alive"] is True. Users who explicitly set keep_alive=False will find their setting ignored when using the async transport.

AsyncWorker queue not reset on kill() causes restart failure - `sentry_sdk/worker.py:220-231`

The kill() method puts a _TERMINATOR in the queue (line 220) but does not reset self._queue to None. When start() is called later, it checks if self._queue is None (line 237) and reuses the existing queue containing the terminator. The new _target() task will immediately read the terminator and exit, preventing the worker from processing any new items after restart.

Test will not verify close_async is called due to isinstance check failing on Mock object - `tests/integrations/asyncio/test_asyncio.py:662-663`

The test creates mock_transport = Mock(spec=AsyncHttpTransport) but in patch_loop_close._flush(), there's an isinstance(client.transport, AsyncHttpTransport) check (line 71 in asyncio.py). Mock(spec=...) creates a mock that mimics the interface but does not pass isinstance() checks - it's still a Mock instance. This causes _flush() to return early at line 72 without calling client.close_async(), making the test assertions pass vacuously if the mock is simply never called, or fail to catch regressions.

httpcore[asyncio] dependency will fail for Python 3.6 and 3.7 common tests - `tox.ini:340`

The common: httpcore[asyncio] dependency is added without a Python version constraint, but httpcore 1.x requires Python 3.8+. The common test environment includes py3.6 and py3.7 (line 21: {py3.6,py3.7,...}-common), so these tests will fail during dependency installation. This should use a version-qualified selector like {py3.8,...}-common: httpcore[asyncio] similar to how other version-specific dependencies are handled (e.g., line 338: py3.8-common: hypothesis).

Low

Test does not clean up global scope client, potentially causing test isolation issues - `tests/test_transport.py:1074`

The test_async_transport_concurrent_requests test sets the global scope client via sentry_sdk.get_global_scope().set_client(client) but does not register a finalizer to reset it. This can cause state leakage between tests, leading to flaky or order-dependent test failures. Compare with test_async_transport_rate_limiting_with_concurrency which properly uses request.addfinalizer(lambda: sentry_sdk.get_global_scope().set_client(None)).


Duration: 8m 17s · Tokens: 5.6M in / 59.8k out · Cost: $7.50 (+extraction: $0.01, +merge: $0.00, +fix_gate: $0.01)

Annotations

Check failure on line 1145 in sentry_sdk/client.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

__aexit__ silently fails to close sync-transport clients

When `async with Client(...)` is used with a sync transport (the default without `transport_async` experiment), `__aexit__` calls `close_async()`, which returns early without any cleanup at line 1073. This leaves session flusher, batchers, monitor, and transport resources unclosed. Users may reasonably use `async with` syntax in async code even without the async transport experiment, expecting proper cleanup.

Check warning on line 897 in sentry_sdk/transport.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

AsyncHttpTransport ignores keep_alive configuration option

The `AsyncHttpTransport._get_pool_options()` method always applies keep-alive socket options regardless of the `keep_alive` configuration setting. This differs from `HttpTransport._get_pool_options()` which only applies them when `self.options["keep_alive"]` is True. Users who explicitly set `keep_alive=False` will find their setting ignored when using the async transport.

Check warning on line 231 in sentry_sdk/worker.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

AsyncWorker queue not reset on kill() causes restart failure

The `kill()` method puts a `_TERMINATOR` in the queue (line 220) but does not reset `self._queue` to `None`. When `start()` is called later, it checks `if self._queue is None` (line 237) and reuses the existing queue containing the terminator. The new `_target()` task will immediately read the terminator and exit, preventing the worker from processing any new items after restart.

Check warning on line 663 in tests/integrations/asyncio/test_asyncio.py

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

Test will not verify close_async is called due to isinstance check failing on Mock object

The test creates `mock_transport = Mock(spec=AsyncHttpTransport)` but in `patch_loop_close._flush()`, there's an `isinstance(client.transport, AsyncHttpTransport)` check (line 71 in asyncio.py). `Mock(spec=...)` creates a mock that mimics the interface but does not pass `isinstance()` checks - it's still a `Mock` instance. This causes `_flush()` to return early at line 72 without calling `client.close_async()`, making the test assertions pass vacuously if the mock is simply never called, or fail to catch regressions.

Check warning on line 340 in tox.ini

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

httpcore[asyncio] dependency will fail for Python 3.6 and 3.7 common tests

The `common: httpcore[asyncio]` dependency is added without a Python version constraint, but httpcore 1.x requires Python 3.8+. The common test environment includes py3.6 and py3.7 (line 21: `{py3.6,py3.7,...}-common`), so these tests will fail during dependency installation. This should use a version-qualified selector like `{py3.8,...}-common: httpcore[asyncio]` similar to how other version-specific dependencies are handled (e.g., line 338: `py3.8-common: hypothesis`).