fix: warn on duplicate async PostHog clients#686
Open
marandaneto wants to merge 3 commits into
Open
Conversation
marandaneto
commented
Jun 19, 2026
marandaneto
commented
Jun 19, 2026
marandaneto
commented
Jun 19, 2026
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
posthog/test/test_client.py:83-132
**Class-level state left dirty after tests**
Both new tests clear `_client_registry` and `_duplicate_client_warnings` at the top, but never restore them. After `test_warns_once_on_duplicate_async_client_same_key_and_host` finishes, `_duplicate_client_warnings` permanently retains `(FAKE_TEST_API_KEY, "https://us.i.posthog.com")`. Every subsequent test's `setUp` creates a `Client(FAKE_TEST_API_KEY)` with the same default host, so any future test that asserts a warning fires for that (key, host) pair will silently fail to warn. Using `self.addCleanup` (or a `tearDown`) to reset both class attributes would make the test order-independent.
### Issue 2 of 3
posthog/test/test_client.py:107-132
**Multiple no-warning scenarios bundled in a single non-parameterized test**
`test_duplicate_client_warning_allows_intentional_multi_client_cases` tests three distinct suppression cases (different host, `sync_mode=True`, `warn_on_duplicate_clients=False`) inside one test. A parameterized test would let each scenario fail independently and name each case explicitly, which is the project's stated preference.
### Issue 3 of 3
posthog/client.py:480
**`is not self` guard is redundant at this point**
`self` is added to `clients` on the very next line, so it cannot already be in the `WeakSet` when `has_existing_client` is evaluated. The condition `any(client is not self for client in clients)` is equivalent to `len(clients) > 0` here. The extra guard adds noise without protecting against any realistic scenario, and could mislead future readers into thinking `self` might already be present.
Reviews (1): Last reviewed commit: "fix: warn on duplicate async clients" | Re-trigger Greptile |
Contributor
posthog-python Compliance ReportDate: 2026-06-19 13:55:09 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/client.py:1334-1389
**Class-level registry lock not reinitialized after fork**
`_reinit_after_fork` explicitly reinitializes `_flag_definition_cache_provider_async_runner_lock` (line 1382) because threads don't survive fork and inherited locks may be in a corrupted state. The new `Client._client_registry_lock` is a class-level `threading.Lock()` with exactly the same exposure: if another thread holds it at fork time (e.g., during a concurrent `Client.__init__` or `join`), the child process will deadlock the next time it tries to acquire it — which happens when a new client is created or an existing one is shut down. Adding `Client._client_registry_lock = threading.Lock()` at the end of `_reinit_after_fork` would fix this, consistent with how the existing instance-level lock is handled.
Reviews (2): Last reviewed commit: "address remaining pr review feedback" | Re-trigger Greptile |
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
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
Fixes #152.
The SDK supports multiple
Posthog/Clientinstances for valid multi-project or multi-host use cases, so enforcing a strict singleton would be too disruptive. The risky case from the issue is accidentally creating multiple async clients for the same project and host, which can make background queues and shutdown flushing harder to reason about.💚 How did you test it?
uv run --extra test pytest -q posthog/test/test_client.pyuv run --extra test pytest -q posthog/test/test_client.py::TestClient::test_warns_once_on_duplicate_async_client_same_key_and_host posthog/test/test_client.py::TestClient::test_duplicate_client_warning_allows_intentional_multi_client_cases_0_different_host posthog/test/test_client.py::TestClient::test_duplicate_client_warning_allows_intentional_multi_client_cases_1_sync_mode posthog/test/test_client.py::TestClient::test_duplicate_client_warning_allows_intentional_multi_client_cases_2_send_disabled posthog/test/test_module.py posthog/test/test_client_fork.pyuv run --extra test ruff format --check posthog/client.py posthog/__init__.py posthog/test/test_client.pyuv run --extra test ruff check posthog/client.py posthog/__init__.py posthog/test/test_client.pyuv run --extra dev mypy posthog/client.py posthog/__init__.pyuv run --extra dev python .github/scripts/check_public_api.py📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with pi in a dedicated worktree. I chose the recommended non-strict approach: keep intentional multi-client support, add a one-time warning for duplicate async clients with the same project API key and host, and keep lifecycle guidance in the Python SDK docstrings rather than README.
After review, I removed the public configurability for the warning, added registry cleanup when clients are joined/shut down so stale registry entries and warning state do not persist after clients are no longer operable, rolled back the README change, parameterized the no-warning tests, and simplified the duplicate registry check.