-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(max): keep the real cause when an async query fails #89626
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.
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
b294ef8
79b2902
550d644
8941853
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -442,7 +442,12 @@ def process_query_dict_with_tags() -> dict | BaseModel: | |
| if query_status.get("error"): | ||
| if error_message := query_status.get("error_message"): | ||
| raise APIException(error_message) | ||
| raise Exception("Query failed") | ||
| # The status hides the message text because it's not user-safe, but error_code | ||
| # carries the machine-readable cause (a ClickHouse memory limit, an internal | ||
| # error, a worker crash). These are transient, so retry instead of dead-ending | ||
| # on an opaque "Query failed" that the report pipeline never retries. | ||
| error_code = query_status.get("error_code") | ||
| raise MaxToolRetryableError(f"Query failed: {error_code}" if error_code else "Query failed") | ||
|
Comment on lines
+445
to
+450
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Infrastructure failures enter the SQL rewrite loopWhy we think it's a valid issue
Issue descriptionEvery hidden async failure becomes MaxToolRetryableError. This class enters the report pipeline's query-rewrite loop. Internal failures cannot be fixed by changing SQL. Each step can add two LLM calls and two ClickHouse runs during an outage. These blind retries can increase system load while dependencies are unhealthy. Suggested fixClassify error_code before choosing the exception. Use MaxToolRetryableError only when a SQL rewrite can fix the failure. Use MaxToolTransientError for temporary failures and retry once with backoff. Do not send unknown internal failures through the rewrite loop. Prompt to fix with AI (copy-paste)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a valid finding, and I'm escalating it for a human design decision rather than making an unattended change. What I confirmed: a hidden-message async failure is now raised as the "retryable" error class, which the report pipeline treats as "the SQL is wrong, ask the LLM to rewrite it." But these are transient infrastructure failures (a ClickHouse outage, a worker crash), and no rewrite can fix them. Because the pipeline can't extract a cause from this error, it feeds the fixer only the literal class name and does a blind rewrite. During a ClickHouse degradation, each failed report step can add two 30-second fix-LLM calls plus two more query reruns, and Max chat is told to "retry with adjusted inputs" for something no input change addresses. That contradicts the pipeline's own stated policy that infra failures should not be retried. Why this isn't a mechanical swap: the error taxonomy has a class that fits exactly — the transient class, meaning "retry once without changes." But that class is not in the pipeline's retryable set, so simply switching to it would remove the retry this PR was specifically written to add (before the PR, this failure did not retry at all). Reconciling "these should retry" with "don't blindly rewrite them" requires a capability the pipeline doesn't have today: retrying the same query without invoking the fix LLM. What a human needs to decide: (1) the intended retry policy for hidden-message infra failures — retry the same SQL without a rewrite, or don't retry in the report pipeline at all; (2) if retrying, add the transient error class to the pipeline's retryable set together with a branch in the fix loop that skips the fix-LLM and reruns as-is; (3) how this interacts with Max chat, which consumes the same executor and shows the LLM a different retry hint depending on the class. This is best decided together with the related thread about carrying the structured cause through to classification, since the right class depends on that cause being available. |
||
|
|
||
| # Use the completed query results | ||
| response_dict = query_status["results"] | ||
|
|
@@ -453,6 +458,7 @@ def process_query_dict_with_tags() -> dict | BaseModel: | |
| HogQLNotImplementedError, | ||
| ExposedCHQueryError, | ||
| UserAccessControlError, | ||
| MaxToolRetryableError, | ||
| ) as err: | ||
| elapsed = time.time() - start_time | ||
| # Handle known query execution errors with user-friendly messages | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Structured cause is lost before report classification
Why we think it's a valid issue
ee/hogai/context/insight/query_executor.py:449-450into the AI report pipeline (products/exports/backend/temporal/subscriptions/ai_subscription/report_pipeline.py), the disclosure helpers inproducts/exports/backend/temporal/subscriptions/types.py, and the delivery UI inproducts/subscriptions/frontend/scenes/components/SubscriptionAiReportDelivery.tsx.error_codelives only inside the exception text.report_pipeline.py:589setstype_name = type(last_exc).__name__, which isMaxToolRetryableError.report_pipeline.py:590callsundisclosed_query_error_type(last_exc), which walks__cause__/__context__for a ClickHouse class; the executor raises from a status dict, so that walk finds nothing.safe_error_message()(types.py:47) matches onlyExposedHogQLError/ResolutionError, sohuman_readable_errorstaysNone. The persistedQueryStepDiagnostic.error_typetherefore recordsMaxToolRetryableError.report_pipeline.py:600-602rendersQuery failed to run (MaxToolRetryableError), andGenerateAIReportResult.failure_error()(types.py:288-298) builds the fully-degraded delivery message from those type names.report_pipeline.py:575feeds the fix LLMsafe_error_message(exc) or type(exc).__name__, so each retry asks for a rewrite based on the stringMaxToolRetryableError. Each failed step now spends up to 2 fix-LLM calls plus 2 ClickHouse reruns with no cause to work from.clickhouse_error_type(posthog/errors.py:79-83) emitsCHQueryError<Label>names, butUNDISCLOSED_QUERY_ERROR_TYPES(types.py:13) holdsClickHouseQueryMemoryLimitExceeded. A plain plumb-through of the code would pass the memory-limit case straight through the suppression that exists to hide it.AI_REPORT_DIAGNOSTICS_KEYexists for (types.py:102-104).should_fix. Two claims in the finding overstate the harm. The cause does reach logs and error tracking:report_pipeline.py:591-599passesexc_info=last_excand callscapture_exception(last_exc), and both carry the message text. The per-step UI also does not print the class name —SubscriptionAiReportDelivery.tsx:52,61collapses a step with nohuman_readable_errortoFailedplus a generic internal-error note. The retry half of the change does work, becauseMaxToolRetryableErroris in_RETRYABLE_QUERY_ERRORS(report_pipeline.py:107-111). Nothing regresses and no data leaks today, so this is an incomplete fix rather than a merge blocker.Issue description
The status provides a machine-readable cause. This code places the cause only in
MaxToolRetryableErrortext. The subscription pipeline classifies failures withtype(exc).__name__. Itssafe_error_message()also ignoresMaxToolRetryableError. The report therefore records and showsMaxToolRetryableError, not the new cause. This leaves the AI subscription problem unresolved.Suggested fix
Add an
error_codeattribute to a query-specific retryable exception. Preserve it when the catch block rethrows the exception. Make the subscription pipeline read this field for diagnostics and apply its existing disclosure policy. Add a pipeline test for the final diagnostic and report copy.Prompt to fix with AI (copy-paste)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid, but incomplete, gap and I'm escalating it for a human design decision rather than fixing it unattended.
What I confirmed: the query executor puts the real cause only in the exception's message text (
Query failed: <code>) and re-raises without a structured field. The AI subscription pipeline classifies failures by exception class name and by walking the exception's cause chain — but the executor raises from a status dictionary, so there is no cause chain to walk. The result is that the persisted per-step diagnostic recordsMaxToolRetryableErrorinstead of the real ClickHouse cause, even though that diagnostics surface exists specifically to make a degraded report debuggable after the fact. So the structured cause genuinely does not reach classification. (The cause still reaches logs and error tracking, which carry the message text, and nothing regresses today.)Why I'm not fixing it here: the correct fix has to carry the code as a structured field from the executor (in
ee/hogai) into the subscription pipeline (inproducts/exports), which crosses a product-isolation boundary and lands in a module deliberately kept import-light for the Temporal sandbox. More importantly, the pipeline's error type also feeds recipient-facing report copy, gated by a suppression list — and the two naming schemes diverge: the executor's code uses theCHQueryError…form, while the suppression list uses theClickHouseQuery…form. A straight plumb-through would therefore risk showing a subscription recipient an error type that the suppression exists to hide. That is a disclosure decision, and there are a few defensible designs for both the field and the naming reconciliation.What a human needs to decide: (1) how to carry the code as a structured field through the executor's re-raise; (2) how to reconcile the
CHQueryError…vsClickHouseQuery…naming so the suppression policy still holds before this reaches recipient-facing copy; (3) confirm the pipeline change respects the exports product boundary and Temporal-sandbox import constraints. Then a pipeline test should assert the final diagnostic carries the real cause and that a suppressed type stays suppressed in recipient copy.