fix: prevent client lifecycle deadlocks - #832
Merged
Merged
Conversation
Serialize join and shutdown ownership without holding locks across user callbacks, defer reentrant callback lifecycle work, discard unflushed join queues safely, and rebuild fork-unsafe sync queues.
Ensure repeated flush requests from lifecycle callbacks share one non-daemon helper instead of accumulating blocked threads.
Run one strongest follow-up flush for callback requests that arrive while the coalesced helper is active.
Avoid creating one waiting lifecycle thread per callback while preserving deferred cleanup retries.
Keep one bounded deferred lifecycle waiter so callback shutdown requests survive an active owner's cleanup failure.
Combine the merged drain signaling behavior from #797 with serialized lifecycle shutdown before merging main.
# Conflicts: # posthog/client.py # posthog/test/test_client.py
Contributor
posthog-python Compliance ReportDate: 2026-08-06 14:10:16 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Propagate lifecycle callback context through the runner default executor and avoid unbounded drains when no consumer can process queued work.
Wrap all runner executor submissions with callback context and make shutdown discard only queues that have no live consumer.
Propagate lifecycle context through thread executors while leaving process-pool submissions picklable.
Track active provider calls without mutating event loops or wrapping executors, and make unbounded drains recover if all consumers stop.
Propagate callback context through known thread executors without affecting unrelated callers or serialization executors, and balance dequeued work if batching is interrupted.
Use a private context-aware event loop with a process-safe registry wrapper so thread-backed executor proxies remain reentrant without breaking serialization executors.
Carry callback context inside a serialization-safe callable so executor cancellation, process pools, and fork do not depend on shared registry state.
Use a context-aware platform event loop and make startup errors and concurrent close observable without orphaning runner threads.
Prevent close from publishing or stopping a loop during an active startup/run and close partially initialized loops on startup failure.
Submit concurrent work without holding completion locks, reject loop-thread synchronous reentry, and define close-during-startup cancellation.
Use the configured event-loop policy when extensible, fall back only for read-only implementations, and stabilize startup-close coverage.
marandaneto
marked this pull request as ready for review
August 5, 2026 19:18
Contributor
|
Reviews (1): Last reviewed commit: "fix: preserve async runner loop policy" | Re-trigger Greptile |
Document direct deferred lifecycle calls and the safe application-thread handoff pattern for callbacks that require blocking shutdown completion.
Member
Author
|
@dustinbyrne some eyes here would be good since its a complex code path, but i managed to reproduce the bug before and it seems fixed now + extensible tests run locally |
Contributor
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 5 consider. Published 6 findings (view the review). |
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ReviewHog Report
Bugfix
Issues: 3 issues
Files (4)
posthog/client.pyposthog/consumer.pyposthog/__init__.py.sampo/changesets/lifecycle-deadlocks.md
What were the main changes
- Adds a lifecycle lock/condition to serialize join()/shutdown() ownership and prevent deadlocks from reentrant or concurrent calls
- Defers and coalesces lifecycle (join/shutdown) and flush calls made from consumer error callbacks, poller threads, or cache-provider async runner threads via _is_lifecycle_callback_thread/_defer_lifecycle_from_callback/_defer_flush_from_callback
- Adds discard_undrainable_queued_work() to _Lane to safely drop queued events when no consumer is running instead of blocking forever
- Reworks lane flush()/join() to use unfinished_tasks polling with drain-signal integration and graceful handling of dead consumers
- Rebuilds fork-unsafe lifecycle locks/state (_lifecycle_lock, deferred-flush state, shutdown event) before restarting child threads after fork
- Consumer.next_batch now tracks pending_items and calls task_done on exceptions to avoid leaking unfinished_tasks counts on interruption
- Updates public docstrings (init.py, client.py) clarifying that on_error callbacks must be non-blocking and lifecycle calls from callbacks are deferred
- Adds changeset describing the lifecycle deadlock fix
Infrastructure
Issues: 3 issues
Files (1)
posthog/_async_utils.py
What were the main changes
- Adds _ContextExecutorCall/_PlainExecutorCall to preserve contextvars across run_in_executor calls while remaining picklable for serializing executors
- Adds _ContextEventLoop fallback for event loop policies that don't allow monkey-patching run_in_executor
- Hardens _BackgroundEventLoopRunner startup: tracks _startup_error, _close_requested, and only restarts the thread if not alive
- Hardens close(): tracks _closing_threads set, avoids deadlock when closing during startup or from the runner's own thread, adds owns_thread() to detect lifecycle-callback ownership
- Ensures run() rejects synchronous calls from the runner thread itself and retries loop acquisition if closed concurrently
dustinbyrne
reviewed
Aug 5, 2026
Member
Author
|
@dustinbyrne tried to simplify as much as i could, also ran a few cycles of reviews with different models |
dustinbyrne
approved these changes
Aug 6, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
ran a bunch of targeted tests locally to validate, all look good:
- Full suite: 2045 passed, 16 skipped
- Focused lifecycle suite on Python 3.11, 3.12, and 3.13: 305 passed + 14 subtests each
- 150 concurrent lifecycle rounds with producer threads, two lanes, upload failures, and callback re-entry
- 200 shutdown/delivery races: every accepted event delivered exactly once
- 250 async run()/close() races plus 100 startup/close races
- 100 forks while all relevant client/queue locks were held
- Real HTTP 503/on-error shutdown with 40 accepted batches
- Real in-flight synchronous request versus concurrent shutdown
- Five-client atexit test completed in about 1.16s
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
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.
💡 Motivation and Context
Client lifecycle methods could deadlock when invoked reentrantly from consumer error callbacks, poller/cache-provider cleanup, executor hops, or concurrent threads. Forked clients could also inherit queue and synchronization locks held by threads that no longer exist.
This merges the drain signaling from #797 and serializes lifecycle ownership without holding state locks across user callbacks. It defers and coalesces callback lifecycle work, preserves queued delivery for normal
join()/shutdown(), safely accounts for undrainable or interrupted queue work, hardens async cache-provider runner startup/close/executor behavior, and rebuilds fork-unsafe state before restarting child threads. Theon_errorandshutdown()documentation now explains direct deferred calls and the safe application-thread handoff pattern. Review follow-up also makesjoin()/shutdown()no-throw public APIs and removes redundant lifecycle status/error fields. Deferred callback requests are coalesced with a lock-protected dirty flag rather than a generation counter. Automated review follow-up adds per-attempt async-runner startup state, closes rejected/cancelled coroutines, daemonizes deferred helpers, and clarifies best-effort delivery semantics. Interpreter exit now shares one process-wide one-second best-effort flush budget across clients, then stops daemon consumers without joining in-flight transports. Read-only event loops supplied by custom policies are preserved rather than replaced with a platform-default loop.💚 How did you test it?
uv run --extra test pytest -q --timeout=30— 2008 passed, 15 skippeduv run --extra test pytest -q --timeout=30 posthog/test/test_async_utils.py posthog/test/test_client.py posthog/test/test_consumer.py posthog/test/test_client_fork.py posthog/test/test_ai_capture_lane.pyuv run --extra dev ruff format --check .uv run --extra dev ruff check .uv run --extra dev mypy --no-site-packages --config-file mypy.ini . | uv run --extra dev mypy-baseline filteruv run --extra dev python .github/scripts/check_public_api.pyuv run python -W error -c "import posthog"📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with Pi, independent reviewer agents, and autoreview. The final review found no concrete lifecycle, queue, fork, or async-runner defects. Autoreview's remaining read-only-loop concern was rejected after verification: uvloop permits per-instance wrapping, configured extensible policy loops are preserved, and the fallback is intentionally limited to loop implementations that prohibit the wrapping required for safe custom-executor reentry.