Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion posthog/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from enum import StrEnum
from typing import Optional

from clickhouse_driver.errors import ServerException
from clickhouse_driver.errors import NetworkError, ServerException, SocketTimeoutError

from posthog.hogql.errors import ExposedHogQLError

Expand Down Expand Up @@ -1052,3 +1052,11 @@ class CHQueryErrorUnknownTable(ExposedCHQueryError):
ClickHouseAtCapacity,
ClickHouseClusterMemoryLimitExceeded,
)

# Transient ClickHouse connection failures. The driver raises these below the ServerException layer,
# so wrap_clickhouse_query_error passes them through unchanged and CH_TRANSIENT_ERRORS does not cover
# them. The driver converts a connect-time socket error or timeout - including a reset during the
# handshake - into NetworkError or SocketTimeoutError. A reset or EOF while a result streams back
# arrives as the raw ConnectionResetError or EOFError. The connection self-heals, so callers that
# retry or skip on these lose nothing.
CH_TRANSIENT_CONNECTION_ERRORS = (ConnectionResetError, EOFError, NetworkError, SocketTimeoutError)
10 changes: 9 additions & 1 deletion posthog/tasks/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from posthog.clickhouse.client.limit import ConcurrencyLimitExceeded, limit_concurrency
from posthog.clickhouse.query_tagging import Feature, Product, get_query_tags, tag_queries
from posthog.cloud_utils import is_cloud
from posthog.errors import CH_TRANSIENT_ERRORS, CHQueryErrorUnknownTable
from posthog.errors import CH_TRANSIENT_CONNECTION_ERRORS, CH_TRANSIENT_ERRORS, CHQueryErrorUnknownTable
from posthog.exceptions import ClickHouseAtCapacity
from posthog.exceptions_capture import capture_exception
from posthog.metrics import pushed_metrics_registry
Expand Down Expand Up @@ -1038,6 +1038,14 @@ def find_flags_with_enriched_analytics() -> None:
# Expected on self-hosted instances with an incomplete ClickHouse schema (e.g. missing
# migrations) - not worth capturing as an exception, just skip this run.
logger.warning("Find flags with enriched analytics skipped, table missing", error=e)
except CH_TRANSIENT_CONNECTION_ERRORS as e:
# A ClickHouse connection dropped at connect time or mid-read. This handler covers the main
# analytics query and a cold-cache materialized-column registry lookup, which both run inline.
# A warm-but-stale registry entry refreshes on a background thread (cache_for with
# background_refresh), so a transient error there stays outside this handler; the task keeps
# running on the stale value, and the SDK thread hook reports that failure separately. The
# next 12-hourly run recovers, so skip this one instead of minting an error-tracking issue.
logger.warning("Find flags with enriched analytics skipped, transient connection error", error=e)
Comment thread
posthog[bot] marked this conversation as resolved.
Comment on lines +1041 to +1048

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Permanent network failures are treated as benign

consider best_practice

Why we think it's a valid issue
  • Checked: the driver conversion in Connection.connect (clickhouse_driver/connection.py:414-447), the new handler (posthog/tasks/tasks.py:1041-1048), the task decorator and beat entry (posthog/tasks/tasks.py:1026, posthog/tasks/scheduled.py:803-807), and every other site in this repo that handles CH_TRANSIENT_ERRORS.
  • Found: The breadth claim holds. connect() converts each socket.error into NetworkError (connection.py:438-444). ConnectionRefusedError, socket.gaierror for DNS, and ssl.SSLError are all subclasses of OSError, so a wrong host, a dead cluster, and an expired certificate each arrive as NetworkError, not only a short reset.
  • Found: The task carries no retry budget. @shared_task(ignore_result=True) at posthog/tasks/tasks.py:1026 sets no autoretry_for, and the schedule runs it on crontab(minute="10", hour="*/12") (posthog/tasks/scheduled.py:803-807). A permanent condition therefore returns normally on every run, without limit.
  • Found: This handler is the only place in the repo that turns a transient ClickHouse error into a silent success. posthog/tasks/tasks.py:1465-1471, in this same file, re-raises CH_TRANSIENT_ERRORS with the comment "Swallowing one here would report a successful sync and skip the retry that recovers these runs today". posthog/temporal/alerts/activities.py:274 re-raises for the same reason. posthog/tasks/calculate_cohort.py:580 and posthog/tasks/tasks.py:1280 both use autoretry_for=CH_TRANSIENT_ERRORS. The suggested bounded retry is the established pattern here, not added machinery.
  • Impact: While the offline cluster stays unreachable or misconfigured, flags never get has_enriched_analytics set and their usage dashboards never receive the enriched insights, and no failed task marks the gap.
  • Priority: Lowered to consider, because detection does not depend on this task. The logger.warning at posthog/tasks/tasks.py:1048 still fires, and QUERY_ERROR_COUNTER in sync_execute records exception_type="NetworkError" for every failed query (posthog/clickhouse/client/execute.py:551-557). Other tasks on the same OFFLINE workload fail loudly during the same outage. The lost behavior is one background enrichment, not user data.
Issue description

NetworkError does not identify only transient failures. The driver converts connection refusal, DNS errors, and TLS errors to NetworkError. This handler returns normally for each failure. Celery marks the run successful, so error tracking never reports a persistent outage or bad ClickHouse configuration.

Suggested fix

Retry these exceptions for a bounded number of attempts. Capture the final exception and fail the task when all attempts fail. This keeps short resets quiet but reports persistent failures.

Prompt to fix with AI (copy-paste)
## Context
@posthog/tasks/tasks.py#L1041-1048

<issue_description>
`NetworkError` does not identify only transient failures. The driver converts connection refusal, DNS errors, and TLS errors to `NetworkError`. This handler returns normally for each failure. Celery marks the run successful, so error tracking never reports a persistent outage or bad ClickHouse configuration.
</issue_description>

<issue_validation>
- **Checked:** the driver conversion in `Connection.connect` (clickhouse_driver/connection.py:414-447), the new handler (posthog/tasks/tasks.py:1041-1048), the task decorator and beat entry (posthog/tasks/tasks.py:1026, posthog/tasks/scheduled.py:803-807), and every other site in this repo that handles `CH_TRANSIENT_ERRORS`.
- **Found:** The breadth claim holds. `connect()` converts each `socket.error` into `NetworkError` (connection.py:438-444). `ConnectionRefusedError`, `socket.gaierror` for DNS, and `ssl.SSLError` are all subclasses of `OSError`, so a wrong host, a dead cluster, and an expired certificate each arrive as `NetworkError`, not only a short reset.
- **Found:** The task carries no retry budget. `@shared_task(ignore_result=True)` at posthog/tasks/tasks.py:1026 sets no `autoretry_for`, and the schedule runs it on `crontab(minute="10", hour="*/12")` (posthog/tasks/scheduled.py:803-807). A permanent condition therefore returns normally on every run, without limit.
- **Found:** This handler is the only place in the repo that turns a transient ClickHouse error into a silent success. posthog/tasks/tasks.py:1465-1471, in this same file, re-raises `CH_TRANSIENT_ERRORS` with the comment "Swallowing one here would report a successful sync and skip the retry that recovers these runs today". posthog/temporal/alerts/activities.py:274 re-raises for the same reason. posthog/tasks/calculate_cohort.py:580 and posthog/tasks/tasks.py:1280 both use `autoretry_for=CH_TRANSIENT_ERRORS`. The suggested bounded retry is the established pattern here, not added machinery.
- **Impact:** While the offline cluster stays unreachable or misconfigured, flags never get `has_enriched_analytics` set and their usage dashboards never receive the enriched insights, and no failed task marks the gap.
- **Priority:** Lowered to `consider`, because detection does not depend on this task. The `logger.warning` at posthog/tasks/tasks.py:1048 still fires, and `QUERY_ERROR_COUNTER` in `sync_execute` records `exception_type="NetworkError"` for every failed query (posthog/clickhouse/client/execute.py:551-557). Other tasks on the same OFFLINE workload fail loudly during the same outage. The lost behavior is one background enrichment, not user data.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Retry these exceptions for a bounded number of attempts. Capture the final exception and fail the task when all attempts fail. This keeps short resets quiet but reports persistent failures.
</potential_solution>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed against the current code, and it is a real trade-off rather than an oversight, so I'm leaving it for a human on the feature-flags team to decide rather than changing it unattended.

The caught set here is CH_TRANSIENT_CONNECTION_ERRORS, which is now (ConnectionResetError, EOFError, NetworkError, SocketTimeoutError) in posthog/errors.py (broader than the (ConnectionResetError, EOFError) the PR description mentions). The comment right above that tuple says the driver turns any connect-time socket error into NetworkError — which is exactly your point: a refused connection, a DNS failure, and an expired cert all surface as NetworkError, so this handler currently skips them as a benign success.

The reason I'm not just applying the suggested bounded retry: NetworkError is one type that covers both a transient reset during the handshake and a permanent misconfiguration, so there's no exception-type split that separates them. Every option is a design change, not a mechanical fix:

  • Bounded retry + capture on the final attempt (your suggestion) — clean, but it adds retry machinery and re-introduces the error-tracking capture this PR set out to remove, so it partly reverses the author's deliberate skip-and-recover choice.
  • Drop NetworkError from the tuple — would lose the genuinely transient handshake-reset case the comment describes.
  • Leave as-is — defensible too: the logger.warning still fires and QUERY_ERROR_COUNTER records exception_type="NetworkError" on every failed query, so a persistent outage is still observable via logs and metrics (and other OFFLINE-workload tasks fail loudly), just not as an error-tracking issue.

So the decision a human needs to make is whether a persistent ClickHouse connection failure on this task should surface as an error-tracking issue at all, and if so via which of the above — noise reduction vs outage visibility. That's why this is flagged for review rather than fixed here.

except Exception as e:
logger.exception("Find flags with enriched analytics failed", error=e)
capture_exception(
Expand Down
16 changes: 14 additions & 2 deletions posthog/test/test_feature_flag_analytics.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@

from django.core.cache import cache

from clickhouse_driver.errors import NetworkError, SocketTimeoutError
from parameterized import parameterized

from posthog import redis
from posthog.constants import FlagRequestType
from posthog.errors import CHQueryErrorUnknownTable
Expand Down Expand Up @@ -1091,9 +1094,18 @@ def test_logs_and_captures_on_failure_without_reraising(self, mock_find_flags: M

mock_capture.assert_called_once()

@parameterized.expand(
[
("unknown_table", CHQueryErrorUnknownTable("Table default.events doesn't exist", code=60)),
("connection_reset", ConnectionResetError(104, "Connection reset by peer")),
("eof", EOFError("Unexpected EOF while reading bytes")),
("network_error", NetworkError("Connection refused (localhost:9000)")),
("socket_timeout", SocketTimeoutError("Socket timeout while connecting (localhost:9000)")),
]
)
@patch("products.feature_flags.backend.flag_analytics.find_flags_with_enriched_analytics")
def test_unknown_table_error_is_not_captured(self, mock_find_flags: MagicMock) -> None:
mock_find_flags.side_effect = CHQueryErrorUnknownTable("Table default.events doesn't exist", code=60)
def test_benign_error_is_not_captured(self, _name: str, error: Exception, mock_find_flags: MagicMock) -> None:
mock_find_flags.side_effect = error

with patch("posthog.tasks.tasks.capture_exception") as mock_capture:
find_flags_with_enriched_analytics_task()
Expand Down
Loading