Skip to content

fix: warn on duplicate async PostHog clients#686

Open
marandaneto wants to merge 3 commits into
mainfrom
fix/client-singleton-warnings-docs-152
Open

fix: warn on duplicate async PostHog clients#686
marandaneto wants to merge 3 commits into
mainfrom
fix/client-singleton-warnings-docs-152

Conversation

@marandaneto

@marandaneto marandaneto commented Jun 19, 2026

Copy link
Copy Markdown
Member

💡 Motivation and Context

Fixes #152.

The SDK supports multiple Posthog/Client instances 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.py
  • uv 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.py
  • uv run --extra test ruff format --check posthog/client.py posthog/__init__.py posthog/test/test_client.py
  • uv run --extra test ruff check posthog/client.py posthog/__init__.py posthog/test/test_client.py
  • uv run --extra dev mypy posthog/client.py posthog/__init__.py
  • uv run --extra dev python .github/scripts/check_public_api.py

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran sampo add to 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.

@marandaneto marandaneto self-assigned this Jun 19, 2026
Comment thread posthog/__init__.py Outdated
Comment thread posthog/client.py
Comment thread README.md Outdated
@greptile-apps

greptile-apps Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
Fix 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

Comment thread posthog/test/test_client.py Outdated
Comment thread posthog/test/test_client.py Outdated
Comment thread posthog/client.py Outdated
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

posthog-python Compliance Report

Date: 2026-06-19 13:55:09 UTC
Duration: 540103ms

✅ All Tests Passed!

45/45 tests passed


Capture Tests

29/29 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields 517ms
Format Validation.Event Has Uuid 10007ms
Format Validation.Event Has Lib Properties 10007ms
Format Validation.Distinct Id Is String 10007ms
Format Validation.Token Is Present 10007ms
Format Validation.Custom Properties Preserved 10007ms
Format Validation.Event Has Timestamp 10007ms
Retry Behavior.Retries On 503 18017ms
Retry Behavior.Does Not Retry On 400 12008ms
Retry Behavior.Does Not Retry On 401 10006ms
Retry Behavior.Respects Retry After Header 16010ms
Retry Behavior.Implements Backoff 32032ms
Retry Behavior.Retries On 500 16002ms
Retry Behavior.Retries On 502 16010ms
Retry Behavior.Retries On 504 16006ms
Retry Behavior.Max Retries Respected 32033ms
Deduplication.Generates Unique Uuids 9993ms
Deduplication.Preserves Uuid On Retry 16016ms
Deduplication.Preserves Uuid And Timestamp On Retry 23019ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 16006ms
Deduplication.No Duplicate Events In Batch 10003ms
Deduplication.Different Events Have Different Uuids 10006ms
Compression.Sends Gzip When Enabled 10008ms
Batch Format.Uses Proper Batch Structure 10006ms
Batch Format.Flush With No Events Sends Nothing 5004ms
Batch Format.Multiple Events Batched Together 10006ms
Error Handling.Does Not Retry On 403 12009ms
Error Handling.Does Not Retry On 413 10006ms
Error Handling.Retries On 408 14013ms

Feature_Flags Tests

16/16 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 9503ms
Request Payload.Flags Request Uses V2 Query Param 10007ms
Request Payload.Flags Request Hits Flags Path Not Decide 10007ms
Request Payload.Flags Request Omits Authorization Header 10007ms
Request Payload.Token In Flags Body Matches Init 10007ms
Request Payload.Groups Round Trip 10007ms
Request Payload.Groups Default To Empty Object 10007ms
Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It 10007ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False 10007ms
Request Payload.Disable Geoip Omitted Defaults To False 10007ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key 10007ms
Request Lifecycle.No Flags Request On Init Alone 5003ms
Request Lifecycle.No Flags Request On Normal Capture 10508ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests 9510ms
Request Lifecycle.Mock Response Value Is Returned To Caller 10003ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event 10509ms

@marandaneto marandaneto marked this pull request as ready for review June 19, 2026 13:48
@marandaneto marandaneto requested a review from a team as a code owner June 19, 2026 13:48
@greptile-apps

greptile-apps Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
Fix 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce (or provide user-facing warnings) instantiating the Python client as a singleton

1 participant