fix(data-imports): stop blaming credentials for a cancelled warehouse query - #76423
Conversation
… query A ClickHouse query cancellation (code 394, QUERY_WAS_CANCELLED) reading an S3/Delta-backed warehouse table fell through `_safe_expose_ch_error`'s classifier unmatched and got reported to the user as "Could not read the files from your storage bucket. Check that the files URL pattern, file format, and credentials are correct". That message sends users chasing a credentials problem that doesn't exist. In this pipeline, a cancelled query is virtually always our own client giving up on a slow read (a read timeout closes the connection, cancelling the still-running query server-side) - the same pattern already documented for the notebooks product's ClickHouse client. Classify it as a timeout instead, and keep it retryable. Generated-By: PostHog Code Task-Id: 69c5b2b5-4ba6-43cb-abd0-0b3fe2ff73f8
|
😎 Merged successfully - details. |
|
Hey @Gilbert09! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
There was a problem hiding this comment.
Pull request overview
This PR improves error classification for data warehouse table introspection by correctly surfacing ClickHouse “query was cancelled” failures (code 394) as a transient timeout-style message, instead of incorrectly blaming storage bucket credentials or file format.
Changes:
- Add a
QueryErrorCategory.CANCELLEDcheck inDataWarehouseTable._safe_expose_ch_errorto map cancelled queries to a retryable “took too long” message. - Add a regression test ensuring code 394 no longer falls through to the generic “check credentials” fallback.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| products/warehouse_sources/backend/models/table.py | Adds explicit handling for cancelled ClickHouse queries in _safe_expose_ch_error to avoid misleading storage-bucket guidance. |
| products/warehouse_sources/backend/tests/test_table.py | Adds a regression test asserting code 394 surfaces as a timeout message. |
Suppressed comments (1)
products/warehouse_sources/backend/models/table.py:949
- The new cancellation branch drops the original exception context. Using exception chaining (
from err) keeps the underlying ClickHouse error available in logs/error tracking while still surfacing the user-safe message.
if classify_query_error(err) == QueryErrorCategory.CANCELLED:
raise Exception(
"Reading the files from your storage bucket took too long and the query was cancelled. "
"This is usually temporary - try again, or narrow the URL pattern if the dataset is very large."
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Shortens the mid-postmortem-style comments in _safe_expose_ch_error and its test to a single durable "why", per the repo's sparse-comment convention (flagged by Copilot review). Generated-By: PostHog Code Task-Id: 69c5b2b5-4ba6-43cb-abd0-0b3fe2ff73f8
|
Trimmed both comments to a short one-liner in 31dce0c. |
There was a problem hiding this comment.
Small, contained fix within owning team's code (author is on team-warehouse-sources), scoped to a single classifier, has a regression test, and the only review comments (style nitpicks) were addressed by the author.
- Author wrote 0% of the modified lines and has 33 merged PRs in these paths (familiarity MODERATE).
- 👍 on the PR from hex-security-app[bot].
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 14L, 1F substantive, 19L/2F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1a-trivial (19L, 2F, single-area, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ f381e58 · reviewed head 31dce0c |
|
/trunk merge |
Problem
A ClickHouse query cancellation (error code 394,
QUERY_WAS_CANCELLED) while introspecting an S3/Delta-backed data warehouse table fell throughDataWarehouseTable._safe_expose_ch_error's classifier unmatched, and got reported to the user as:That message sends people chasing a credentials or file-format problem that doesn't exist. In this pipeline a cancelled query is virtually always our own client giving up on a slow read: a read timeout closes the connection, which cancels the still-running query server-side.
products/notebooks/backend/temporal/frame_materialize.pyalready documents and relies on this exact pattern for its own ClickHouse client, treating code 394 (alongside socket/network transport codes) as transient rather than a real query failure.Traced from a live error tracking event:
run_chdb_querytimed out after its 30s budget, the code fell back to the ClickHouse-cluster path viasync_execute, and that query was also cancelled. Neither failure has anything to do with the source files, but_safe_expose_ch_errordidn't have a code path for "the query itself was cancelled" and defaulted to the generic storage-bucket message.Changes
DataWarehouseTable._safe_expose_ch_errornow checksclassify_query_error(err) == QueryErrorCategory.CANCELLEDbefore falling through to the generic message, and raises an accurate, still-retryable "took too long" exception instead.wrap_clickhouse_query_errorused across the whole app), so it only changes behavior for warehouse table introspection (get_count/get_columns), not other ClickHouse query paths that rely on cancellation being classified asCANCELLEDfor their own purposes (e.g. SLO tracking inhogql_queries/query_runner.py).Exception, not added to anyNonRetryableErrors, since a cancelled/timed-out query is exactly the kind of transient condition that should keep retrying.How did you test this code?
Added one case to the existing
TestSafeExposeChErrorsuite inproducts/warehouse_sources/backend/tests/test_table.py: aServerExceptionwith code 394 now raises the timeout message instead of the generic storage-bucket one. This is the regression the live error tracking event hit — no existing test covered a cancelled/timed-out query reaching this classifier.Ran locally:
pytest products/warehouse_sources/backend/tests/test_table.py -k TestSafeExposeChError— 6 passeduv run mypy --cache-fine-grained .— cleantach check --dependencies --interfaces— cleanhogli ci:preflight --fix— cleanDocs update
Not applicable — this only changes an internal error classification, no user-facing config or documented workflow.
🤖 Agent context
Autonomy: Fully autonomous
019fc2be-dc46-7681-9632-b3d354b22879) delivered by webhook./writing-testsbefore adding the regression test.$exception_list), tracedget_count→run_chdb_querytimeout →sync_executefallback →wrap_clickhouse_query_error→_safe_expose_ch_error, and confirmed viaclassify_query_error/QueryErrorCategorythat code 394 was the only ClickHouse "infrastructure" code missing special-case handling here (code 159TIMEOUT_EXCEEDEDalready avoids this bug because it maps to an exception without a.messageattribute).wrap_clickhouse_query_errorinstead, but that function backs every ClickHouse call in the app (viasync_execute), and reclassifying code 394 there would flip SLO outcome tracking for unrelated cancelled queries (e.g. user-navigated-away insight queries) from success to failure. Kept the fix local to_safe_expose_ch_errorinstead.gh pr list --search ...on the exception type, message, and module path, plus the author's own open PRs): found two related-but-distinct open PRs, #74001 (schema-drift read errors) and #74882 (don't fail imports on a failed row-count refresh) — neither touches this classifier, andget_columns()(schema validation, which does fail the job on error) still hits it regardless of fix(data-imports): don't fail imports on a failed row count refresh #74882.Created with PostHog Code