Skip to content

fix(errors): return 400 for ClickHouse type-coercion parse errors #92068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
posthog wants to merge 4 commits into
base: master
Choose a base branch
from
+85 −39
Open

fix(errors): return 400 for ClickHouse type-coercion parse errors #92068

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68fd381
fix(errors): return 400 for ClickHouse type-coercion parse errors
posthog[bot] Aug 31, 2026
70ba776
fix(errors): correct CANNOT_PARSE_TEXT message direction
posthog[bot] Aug 31, 2026
959a848
fix(feature-flags): update blast radius for user_safe parse codes
posthog[bot] Sep 1, 2026
0943714
fix(errors): classify parse-error family as USER_ERROR explicitly
posthog[bot] Sep 1, 2026
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
29 changes: 22 additions & 7 deletions posthog/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
)


def wrap_clickhouse_query_error(err: Exception) -> Exception:

Check warning on line 107 in posthog/errors.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04)

lint:complexity 

`wrap_clickhouse_query_error` has cyclomatic complexity 29 (warn >10)

Check warning on line 107 in posthog/errors.py

View workflow job for this annotation

GitHub Actions / Python code quality (depot-ubuntu-24.04) 

`wrap_clickhouse_query_error` has cyclomatic complexity 29 (warn >10)
"Beautifies clickhouse client errors, using custom error classes for every code"
if not isinstance(err, ServerException):
return err
Expand Down Expand Up @@ -367,9 +367,13 @@
2: ErrorCodeMeta("UNSUPPORTED_PARAMETER"),
3: ErrorCodeMeta("UNEXPECTED_END_OF_FILE"),
4: ErrorCodeMeta("EXPECTED_END_OF_FILE"),
# Stays internal: the CH message embeds the failing data value, which would leak stored
# data to anonymous viewers of public shared insights. Only user_safe once sanitized.
6: ErrorCodeMeta("CANNOT_PARSE_TEXT", category=QueryErrorCategory.USER_ERROR),
# Fixed message: the raw CH text embeds the failing data value, which would leak stored data
# to anonymous viewers of public shared insights. A fixed string keeps that value out of the
# response while still returning a 400 for the bad query (see also codes 26, 72, 130).
6: ErrorCodeMeta(
"CANNOT_PARSE_TEXT",
user_safe="Cannot parse a value in the query as text. Check the types in your comparisons and IN clauses.",
),
Comment thread
posthog[bot] marked this conversation as resolved.

@posthog posthog Bot Aug 31, 2026

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.

Do not treat broad parser codes as query-only failures

consider bug

Why we think it's a valid issue
  • Checked: the diff against origin/master (e18f91a), the lookup path look_up_clickhouse_error_code_meta and the generic wrap branch, both SLO/capture sites in posthog/hogql_queries/query_runner.py, the API error branches in posthog/api/query.py, and the full logs alert chain (alert_error_classifier.py → alert_state_machine.py → temporal/activities.py).
  • Found: the category change is real and undisclosed. Before the change, codes 26 and 130 were bare ErrorCodeMeta("CANNOT_PARSE_QUOTED_STRING") / ErrorCodeMeta("CANNOT_READ_ARRAY_FROM_TEXT") with no category, so get_category() returned ERROR (posthog/errors.py:60-73). They now return USER_ERROR. Codes 6 and 72 already carried category=QueryErrorCategory.USER_ERROR, so only 26 and 130 move.
  • Found: the consequence chain for that move is confirmed at two sites. _classify_error_for_slo maps USER_ERROR to SloOutcome.SUCCESS (posthog/hogql_queries/query_runner.py:331-333), and capture_exception is gated on that outcome — query_runner.py:2344-2355 captures only when the outcome is FAILURE. The API path agrees: posthog/api/query.py:358-370 returns a 400 for ExposedCHQueryError with no capture, and captures only on the InternalCHQueryError branch. So codes 26 and 130 stop counting as platform failures and stop reaching error tracking.
  • Found: the match is by numeric code only (look_up_clickhouse_error_code_meta, posthog/errors.py:211-215), and the generic branch applies the fixed string to every exception with that code (posthog/errors.py:205-207). The same file already shows that non-query, storage-side parse failures reach this wrapper: it carries dedicated branches for a Parquet file that is not a Parquet file and for corrupted Parquet thrift metadata. A malformed customer CSV or TSV read by a data warehouse query is a realistic source of code 26 or 130, and it now returns a 400 that tells the reader to check the types in their comparisons.
  • Found: two of the stated consequences do not hold. The logs alert path calls capture_exception unconditionally, before any use of the classification (products/logs/backend/temporal/activities.py:1428-1430), so no capture is lost there. And auto-disable needs repeated deterministic failures of the same alert; the classifier's own contract reserves the transient set for cluster problems and counts a misconfigured alert toward BROKEN (products/logs/backend/alert_error_classifier.py:19-22), which is what a repeating coercion error is. Logs alerts read a native ClickHouse table, not customer files, so the storage-parser case does not apply there.
  • Found: the claimed metadata, settings and JSON input causes are not reachable. wrap_clickhouse_query_error sees only exceptions raised by query execution.
  • Impact: the surviving defect is a silent observability change on two codes. Failures that used to raise a 500, count as an SLO failure and land in error tracking now return a 400, count as an SLO success and are never captured. If any part of that traffic is a PostHog-side or source-data problem, the team loses the signal with no metric to notice the loss.
  • Priority: lowered to consider. The sharpest reliability claims are wrong on inspection, the remaining effect is bounded to two codes whose dominant cause in a read-only workload is the user type coercion this PR targets, and the proposed remedy — matching ClickHouse message signatures — is more fragile than the code-granularity design the whole table uses, which this file reserves for two narrowly identified S3 cases.
Issue description

user_safe applies to every ClickHouse exception with each code. These codes do not identify only type-coercion errors. ClickHouse uses them for metadata, JSON input, settings, Parquet data, and array serialization. The new mapping converts those failures to ExposedCHQueryError. It also changes codes 26 and 130 from ERROR to USER_ERROR. Query runners then count these failures as SLO successes and omit capture_exception. The logs alert classifier also treats them as non-transient invalid queries. A storage parser failure can trigger per-alert fallback and can auto-disable working alerts.

Suggested fix

Keep unmatched messages internal. Add a dedicated exposed exception for recognized coercion signatures in wrap_clickhouse_query_error. Classify that exception as USER_ERROR before the generic ServerException branch. Add regression cases for coercion messages and non-query parser messages.

Prompt to fix with AI (copy-paste) 
## Context
@posthog/errors.py#L370-376
@posthog/errors.py#L391-395
@posthog/errors.py#L443-447
@posthog/errors.py#L498-502

<issue_description>
`user_safe` applies to every ClickHouse exception with each code. These codes do not identify only type-coercion errors. ClickHouse uses them for metadata, JSON input, settings, Parquet data, and array serialization. The new mapping converts those failures to `ExposedCHQueryError`. It also changes codes 26 and 130 from `ERROR` to `USER_ERROR`. Query runners then count these failures as SLO successes and omit `capture_exception`. The logs alert classifier also treats them as non-transient invalid queries. A storage parser failure can trigger per-alert fallback and can auto-disable working alerts.
</issue_description>

<issue_validation>
- **Checked:** the diff against `origin/master` (e18f91af), the lookup path `look_up_clickhouse_error_code_meta` and the generic wrap branch, both SLO/capture sites in `posthog/hogql_queries/query_runner.py`, the API error branches in `posthog/api/query.py`, and the full logs alert chain (`alert_error_classifier.py` → `alert_state_machine.py` → `temporal/activities.py`).
- **Found:** the category change is real and undisclosed. Before the change, codes 26 and 130 were bare `ErrorCodeMeta("CANNOT_PARSE_QUOTED_STRING")` / `ErrorCodeMeta("CANNOT_READ_ARRAY_FROM_TEXT")` with no category, so `get_category()` returned `ERROR` (posthog/errors.py:60-73). They now return `USER_ERROR`. Codes 6 and 72 already carried `category=QueryErrorCategory.USER_ERROR`, so only 26 and 130 move.
- **Found:** the consequence chain for that move is confirmed at two sites. `_classify_error_for_slo` maps `USER_ERROR` to `SloOutcome.SUCCESS` (posthog/hogql_queries/query_runner.py:331-333), and `capture_exception` is gated on that outcome — `query_runner.py:2344-2355` captures only when the outcome is `FAILURE`. The API path agrees: `posthog/api/query.py:358-370` returns a 400 for `ExposedCHQueryError` with no capture, and captures only on the `InternalCHQueryError` branch. So codes 26 and 130 stop counting as platform failures and stop reaching error tracking.
- **Found:** the match is by numeric code only (`look_up_clickhouse_error_code_meta`, posthog/errors.py:211-215), and the generic branch applies the fixed string to every exception with that code (posthog/errors.py:205-207). The same file already shows that non-query, storage-side parse failures reach this wrapper: it carries dedicated branches for a Parquet file that is not a Parquet file and for corrupted Parquet thrift metadata. A malformed customer CSV or TSV read by a data warehouse query is a realistic source of code 26 or 130, and it now returns a 400 that tells the reader to check the types in their comparisons.
- **Found:** two of the stated consequences do not hold. The logs alert path calls `capture_exception` unconditionally, before any use of the classification (`products/logs/backend/temporal/activities.py:1428-1430`), so no capture is lost there. And auto-disable needs repeated deterministic failures of the same alert; the classifier's own contract reserves the transient set for cluster problems and counts a misconfigured alert toward `BROKEN` (`products/logs/backend/alert_error_classifier.py:19-22`), which is what a repeating coercion error is. Logs alerts read a native ClickHouse table, not customer files, so the storage-parser case does not apply there.
- **Found:** the claimed metadata, settings and JSON input causes are not reachable. `wrap_clickhouse_query_error` sees only exceptions raised by query execution.
- **Impact:** the surviving defect is a silent observability change on two codes. Failures that used to raise a 500, count as an SLO failure and land in error tracking now return a 400, count as an SLO success and are never captured. If any part of that traffic is a PostHog-side or source-data problem, the team loses the signal with no metric to notice the loss.
- **Priority:** lowered to `consider`. The sharpest reliability claims are wrong on inspection, the remaining effect is bounded to two codes whose dominant cause in a read-only workload is the user type coercion this PR targets, and the proposed remedy — matching ClickHouse message signatures — is more fragile than the code-granularity design the whole table uses, which this file reserves for two narrowly identified S3 cases.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Keep unmatched messages internal. Add a dedicated exposed exception for recognized coercion signatures in `wrap_clickhouse_query_error`. Classify that exception as `USER_ERROR` before the generic `ServerException` branch. Add regression cases for coercion messages and non-query parser messages.
</potential_solution>

@posthog posthog Bot Sep 1, 2026

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.

Escalating this for a human decision rather than changing it unattended. The finding is real: adding fixed user_safe strings to codes 26 (CANNOT_PARSE_QUOTED_STRING) and 130 (CANNOT_READ_ARRAY_FROM_TEXT) also flips their observability category from ERROR to USER_ERROR, because the category defaults to USER_ERROR when user_safe is set and no explicit category is given. Codes 6 and 72 were unaffected — they already carried category=USER_ERROR on master. The effect for 26 and 130 on the query-runner path: they now count as SLO successes and are no longer sent to error tracking, where before they were SLO failures and were captured. That is not mentioned in the PR description, so it reads as an unintended side effect of the reclassification. The narrower reliability claims in the thread don't hold (the logs-alert path captures unconditionally, and metadata/settings/JSON causes aren't reachable here), so the only live question is these two codes. A maintainer needs to decide which of these is wanted: (a) accept it — a malformed query or source file is a user/source problem, so not capturing it is correct; (b) set category=QueryErrorCategory.ERROR explicitly on 26 and 130 to keep the 400 response but preserve SLO-failure + capture, at the cost of ordinary user type-coercion errors adding noise to the SLO and error tracking; or (c) split storage-side parse failures from query-side ones by ClickHouse message signature, like the existing S3/Parquet special cases — more code and, as the thread notes, more fragile. I did not change anything, since picking among these is a judgment call about observability signal versus noise, and the outcome is only observable in the live SLO/capture pipeline, not provable from tests here.

Comment thread
posthog[bot] marked this conversation as resolved.
Outdated
7: ErrorCodeMeta("INCORRECT_NUMBER_OF_COLUMNS"),
8: ErrorCodeMeta("THERE_IS_NO_COLUMN"),
9: ErrorCodeMeta("SIZES_OF_COLUMNS_DOESNT_MATCH"),
Expand All @@ -384,7 +388,11 @@
23: ErrorCodeMeta("CANNOT_READ_FROM_ISTREAM"),
24: ErrorCodeMeta("CANNOT_WRITE_TO_OSTREAM"),
25: ErrorCodeMeta("CANNOT_PARSE_ESCAPE_SEQUENCE"),
26: ErrorCodeMeta("CANNOT_PARSE_QUOTED_STRING"),
# Fixed message: same type-coercion family as code 6, and the raw CH text embeds the value.
26: ErrorCodeMeta(
"CANNOT_PARSE_QUOTED_STRING",
user_safe="Cannot parse a value in the query as a quoted string. Check the types in your comparisons and IN clauses.",
),
27: ErrorCodeMeta("CANNOT_PARSE_INPUT_ASSERTION_FAILED"),
28: ErrorCodeMeta("CANNOT_PRINT_FLOAT_OR_DOUBLE_NUMBER"),
32: ErrorCodeMeta("ATTEMPT_TO_READ_AFTER_EOF"),
Expand Down Expand Up @@ -432,8 +440,11 @@
user_safe="Cannot convert one type to another in the query. Check the types in your comparisons and IN clauses.",
),
71: ErrorCodeMeta("CANNOT_WRITE_AFTER_END_OF_BUFFER"),
# 72 stays internal: the CH message embeds the failing data value (see code 6 note).
72: ErrorCodeMeta("CANNOT_PARSE_NUMBER", category=QueryErrorCategory.USER_ERROR),
# Fixed message: same type-coercion family as code 6, and the raw CH text embeds the value.
72: ErrorCodeMeta(
"CANNOT_PARSE_NUMBER",
user_safe="Cannot parse a value in the query as a number. Check the types in your comparisons and IN clauses.",
),
73: ErrorCodeMeta("UNKNOWN_FORMAT"),
74: ErrorCodeMeta("CANNOT_READ_FROM_FILE_DESCRIPTOR"),
75: ErrorCodeMeta("CANNOT_WRITE_TO_FILE_DESCRIPTOR"),
Expand Down Expand Up @@ -484,7 +495,11 @@
127: ErrorCodeMeta("ILLEGAL_INDEX"),
128: ErrorCodeMeta("TOO_LARGE_ARRAY_SIZE"),
129: ErrorCodeMeta("FUNCTION_IS_SPECIAL"),
130: ErrorCodeMeta("CANNOT_READ_ARRAY_FROM_TEXT"),
# Fixed message: same type-coercion family as code 6, and the raw CH text embeds the value.
130: ErrorCodeMeta(
"CANNOT_READ_ARRAY_FROM_TEXT",
user_safe="Cannot parse a value in the query as an array. Check the types in your comparisons and IN clauses.",
),
131: ErrorCodeMeta("TOO_LARGE_STRING_SIZE"),
133: ErrorCodeMeta("AGGREGATE_FUNCTION_DOESNT_ALLOW_PARAMETERS"),
134: ErrorCodeMeta("PARAMETERS_TO_AGGREGATE_FUNCTIONS_MUST_BE_LITERALS"),
Expand Down
26 changes: 23 additions & 3 deletions posthog/test/test_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,28 @@ def test_user_error_codes_wrap_as_exposed_error(self, code: int, name: str) -> N
"CANNOT_CONVERT_TYPE",
"Cannot convert one type to another in the query. Check the types in your comparisons and IN clauses.",
),
# Type-coercion parse family: exposed as a 400, but the fixed string keeps the failing
# data value the raw CH text embeds out of the response.
(
6,
"CANNOT_PARSE_TEXT",
"Cannot parse a value in the query as text. Check the types in your comparisons and IN clauses.",
),
(
26,
"CANNOT_PARSE_QUOTED_STRING",
"Cannot parse a value in the query as a quoted string. Check the types in your comparisons and IN clauses.",
),
(
72,
"CANNOT_PARSE_NUMBER",
"Cannot parse a value in the query as a number. Check the types in your comparisons and IN clauses.",
),
(
130,
"CANNOT_READ_ARRAY_FROM_TEXT",
"Cannot parse a value in the query as an array. Check the types in your comparisons and IN clauses.",
),
(407, "DECIMAL_OVERFLOW", "Decimal overflow while executing query."),
]
)
Expand All @@ -67,10 +89,8 @@ def test_fixed_message_codes_hide_raw_clickhouse_text(self, code: int, name: str
# SYNTAX_ERROR (62) stays internal: HogQL validates syntax first, so a raw CH syntax error
# signals a PostHog SQL-generation bug that belongs in error tracking.
(62, "SYNTAX_ERROR"),
# These parse/convert codes embed the failing data value in the CH message, so they stay
# These parse codes embed the failing data value in the CH message, so they stay
# internal to avoid leaking source values on public shared insights.
(6, "CANNOT_PARSE_TEXT"),
(72, "CANNOT_PARSE_NUMBER"),
(675, "CANNOT_PARSE_IPV4"),
(676, "CANNOT_PARSE_IPV6"),
(691, "UNKNOWN_ELEMENT_OF_ENUM"),
Expand Down
Loading