fix(max): keep the real cause when an async query fails - #89626
fix(max): keep the real cause when an async query fails#89626posthog[bot] wants to merge 4 commits into
Conversation
A non-user-safe async query failure (a ClickHouse memory limit, an internal error, a worker crash) hid its message text and set no error code, so the AI query executor raised an opaque "Query failed". That plain exception was excluded from the AI report pipeline's retryable set, so the step dead-ended with no retry and no diagnosable cause. Carry the exception class as a machine-readable error_code on the failed status, even when the message text stays hidden. Classify the resulting exception as MaxToolRetryableError so transient ClickHouse failures retry and surface the cause instead of collapsing to "Query failed". Generated-By: PostHog Desktop Task-Id: 5b2bf3ce-ead7-4283-8180-6fdff34dc0f4
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 3 should fix, 1 consider. Published 4 findings (view the review). Resolved comments: 2 fixed, 2 left for you |
🤖 CI report
|
| File | Size | Δ vs base |
|---|---|---|
render-query/src/render-query/render-query.js |
21.69 MiB | 🔺 +3.9 KiB (+0.0%) |
posthog-app/src/scenes/experiments/Experiment.js |
257.2 KiB | 🟢 -1.7 KiB (-0.7%) |
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.36 MiB · 22 files | 🔺 +596 B (+0.0%) | ███░░░░░░░ 30.3% of 4.51 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
8.78 MiB · 3,237 files | 🔺 +1.6 KiB (+0.0%) | █████████░ 90.4% of 9.71 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 306.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 258.5 KiB | ../node_modules/.pnpm/posthog-js@1.418.17/node_modules/posthog-js/dist/module.js |
| 246.0 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.7 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.25 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.25 MiB · 17 files | 🔺 +498 B (+0.0%) | ████░░░░░░ 39.3% of 5.72 MiB |
| Deferred (lazy) | 2.09 MiB · 33 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 746.7 KiB | dist/toolbar/toolbar-app-RBPKVAJT.css |
| 582.8 KiB | dist/toolbar/chunk-chunk-BYSB3M44.js |
| 484.6 KiB | dist/toolbar/chunk-chunk-QOQLAFDJ.js |
| 133.8 KiB | dist/toolbar/chunk-chunk-74HT5EN3.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-T5KY5WYR.js |
| 71.3 KiB | dist/toolbar/toolbar-app-Y3G6XWUE.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-27JL52RE.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-JZL2JUHN.js |
| 20.9 KiB | dist/toolbar/chunk-chunk-3BBZHAYC.js |
| 12.2 KiB | dist/toolbar/chunk-chunk-PIK3PADE.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — 🔺 +264.1 KiB (+0.0%)
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1442.27 MiB · 🔺 +264.1 KiB (+0.0%)
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Small, well-tested fix that makes hidden-message async query failures retryable and diagnosable via a machine-readable error code, without exposing sensitive message text; not risky territory and no unresolved concerns.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 15L, 2F substantive, 58L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (58L, 4F, two-areas, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ b294ef8 · reviewed head b294ef8 |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| # 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") |
There was a problem hiding this comment.
Structured cause is lost before report classification
Why we think it's a valid issue
- Checked: the failure path from
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. - Found: the new
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. - Found: the same class name reaches copy a person reads. The step placeholder at
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. - Found: a second effect of the new retry path.
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. - Found: a naming trap for the follow-up fix.
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. - Impact: the machine-readable cause never reaches step classification, the persisted delivery diagnostics, or the report text, so the delivery record stays undiagnosable — the surface
AI_REPORT_DIAGNOSTICS_KEYexists for (types.py:102-104). - Priority: lowered to
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 MaxToolRetryableError text. The subscription pipeline classifies failures with type(exc).__name__. Its safe_error_message() also ignores MaxToolRetryableError. The report therefore records and shows MaxToolRetryableError, not the new cause. This leaves the AI subscription problem unresolved.
Suggested fix
Add an error_code attribute 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)
## Context
@ee/hogai/context/insight/query_executor.py#L445-450
<issue_description>
The status provides a machine-readable cause. This code places the cause only in `MaxToolRetryableError` text. The subscription pipeline classifies failures with `type(exc).__name__`. Its `safe_error_message()` also ignores `MaxToolRetryableError`. The report therefore records and shows `MaxToolRetryableError`, not the new cause. This leaves the AI subscription problem unresolved.
</issue_description>
<issue_validation>
- **Checked:** the failure path from `ee/hogai/context/insight/query_executor.py:449-450` into the AI report pipeline (`products/exports/backend/temporal/subscriptions/ai_subscription/report_pipeline.py`), the disclosure helpers in `products/exports/backend/temporal/subscriptions/types.py`, and the delivery UI in `products/subscriptions/frontend/scenes/components/SubscriptionAiReportDelivery.tsx`.
- **Found:** the new `error_code` lives only inside the exception text. `report_pipeline.py:589` sets `type_name = type(last_exc).__name__`, which is `MaxToolRetryableError`. `report_pipeline.py:590` calls `undisclosed_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 only `ExposedHogQLError`/`ResolutionError`, so `human_readable_error` stays `None`. The persisted `QueryStepDiagnostic.error_type` therefore records `MaxToolRetryableError`.
- **Found:** the same class name reaches copy a person reads. The step placeholder at `report_pipeline.py:600-602` renders `Query failed to run (MaxToolRetryableError)`, and `GenerateAIReportResult.failure_error()` (`types.py:288-298`) builds the fully-degraded delivery message from those type names.
- **Found:** a second effect of the new retry path. `report_pipeline.py:575` feeds the fix LLM `safe_error_message(exc) or type(exc).__name__`, so each retry asks for a rewrite based on the string `MaxToolRetryableError`. Each failed step now spends up to 2 fix-LLM calls plus 2 ClickHouse reruns with no cause to work from.
- **Found:** a naming trap for the follow-up fix. `clickhouse_error_type` (`posthog/errors.py:79-83`) emits `CHQueryError<Label>` names, but `UNDISCLOSED_QUERY_ERROR_TYPES` (`types.py:13`) holds `ClickHouseQueryMemoryLimitExceeded`. A plain plumb-through of the code would pass the memory-limit case straight through the suppression that exists to hide it.
- **Impact:** the machine-readable cause never reaches step classification, the persisted delivery diagnostics, or the report text, so the delivery record stays undiagnosable — the surface `AI_REPORT_DIAGNOSTICS_KEY` exists for (`types.py:102-104`).
- **Priority:** lowered to `should_fix`. Two claims in the finding overstate the harm. The cause does reach logs and error tracking: `report_pipeline.py:591-599` passes `exc_info=last_exc` and calls `capture_exception(last_exc)`, and both carry the message text. The per-step UI also does not print the class name — `SubscriptionAiReportDelivery.tsx:52,61` collapses a step with no `human_readable_error` to `Failed` plus a generic internal-error note. The retry half of the change does work, because `MaxToolRetryableError` is 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_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add an `error_code` attribute 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.
</potential_solution>
There was a problem hiding this comment.
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 records MaxToolRetryableError instead 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 (in products/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 the CHQueryError… form, while the suppression list uses the ClickHouseQuery… 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… vs ClickHouseQuery… 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.
| # 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") |
There was a problem hiding this comment.
Infrastructure failures enter the SQL rewrite loop
Why we think it's a valid issue
- Checked: the error taxonomy in
ee/hogai/tool_errors.py, the report pipeline's retry set and fix loop, and what the pre-change code did with the same failure. - Found: the codebase already defines the class this case belongs to, and the diff picks the other one.
ee/hogai/tool_errors.py:10-11states the split:MaxToolTransientErroris for "Intermittent issues that can be retried without changes (e.g., rate limits, timeouts)",MaxToolRetryableErroris for "Solvable issues that can be fixed with adjusted inputs".MaxToolRetryableError.retry_strategyreturns"adjusted"(tool_errors.py:86-93), so the hint appended for the LLM is "You may retry with adjusted inputs." (tool_errors.py:44-46). The diff's own comment atee/hogai/context/insight/query_executor.py:447calls these failures transient. - Found: the cost figure is right.
_MAX_QUERY_FIX_RETRIES = 2(report_pipeline.py:96) and the loop atreport_pipeline.py:523-586runs up to three attempts, so a step can add two fix-LLM calls (30s timeout each,_FIX_LLM_TIMEOUT_SECONDS) plus two more ClickHouse runs.MaxToolRetryableErroris in_RETRYABLE_QUERY_ERRORS(report_pipeline.py:107-111), so the class choice alone decides this. - Found: the rewrite is blind.
report_pipeline.py:575passes the fixersafe_error_message(exc) or type(exc).__name__, andsafe_error_messagematches onlyExposedHogQLError/ResolutionError(products/exports/backend/temporal/subscriptions/types.py:47), so the fixer receives the literal stringMaxToolRetryableErrorand no cause. - Found: the change contradicts the pipeline's stated policy. The comment above
_RETRYABLE_QUERY_ERRORS(report_pipeline.py:104-106) says timeouts, infra failures, and generic exceptions fall through to the placeholder without retrying, "since a different SELECT won't fix a ClickHouse outage or a heartbeat timeout". - Found: before the change this path did not retry. The old
Exception("Query failed")fell into the catch-all atquery_executor.py:474-485and re-raised a plainException, which is absent from_RETRYABLE_QUERY_ERRORS. So the extra LLM calls and reruns for hidden-message failures are new with this diff. - Impact: during a ClickHouse degradation, every failed step of an AI report now triples its query attempts and spends two blind fix-LLM calls, and Max chat is told to retry with adjusted inputs for a failure no rewrite addresses.
- Impact: the fix needs a decision rather than a mechanical swap.
MaxToolTransientErroris not in_RETRYABLE_QUERY_ERRORS, so switching to it removes the retry the PR wanted, and the pipeline has no retry-without-changes path today. - Priority: kept at
should_fixrather than raised. The load added is bounded and sits on a low-volume scheduled path, and the same wrapping already happens for user-safe infra errors:query_executor.py:443-444raisesAPIException(error_message), which the handler atquery_executor.py:455-473already converts intoMaxToolRetryableError. The diff widens an existing pattern; it does not create it.
Issue description
Every 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 fix
Classify 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)
## Context
@ee/hogai/context/insight/query_executor.py#L445-450
<issue_description>
Every 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.
</issue_description>
<issue_validation>
- **Checked:** the error taxonomy in `ee/hogai/tool_errors.py`, the report pipeline's retry set and fix loop, and what the pre-change code did with the same failure.
- **Found:** the codebase already defines the class this case belongs to, and the diff picks the other one. `ee/hogai/tool_errors.py:10-11` states the split: `MaxToolTransientError` is for "Intermittent issues that can be retried without changes (e.g., rate limits, timeouts)", `MaxToolRetryableError` is for "Solvable issues that can be fixed with adjusted inputs". `MaxToolRetryableError.retry_strategy` returns `"adjusted"` (`tool_errors.py:86-93`), so the hint appended for the LLM is "You may retry with adjusted inputs." (`tool_errors.py:44-46`). The diff's own comment at `ee/hogai/context/insight/query_executor.py:447` calls these failures transient.
- **Found:** the cost figure is right. `_MAX_QUERY_FIX_RETRIES = 2` (`report_pipeline.py:96`) and the loop at `report_pipeline.py:523-586` runs up to three attempts, so a step can add two fix-LLM calls (30s timeout each, `_FIX_LLM_TIMEOUT_SECONDS`) plus two more ClickHouse runs. `MaxToolRetryableError` is in `_RETRYABLE_QUERY_ERRORS` (`report_pipeline.py:107-111`), so the class choice alone decides this.
- **Found:** the rewrite is blind. `report_pipeline.py:575` passes the fixer `safe_error_message(exc) or type(exc).__name__`, and `safe_error_message` matches only `ExposedHogQLError`/`ResolutionError` (`products/exports/backend/temporal/subscriptions/types.py:47`), so the fixer receives the literal string `MaxToolRetryableError` and no cause.
- **Found:** the change contradicts the pipeline's stated policy. The comment above `_RETRYABLE_QUERY_ERRORS` (`report_pipeline.py:104-106`) says timeouts, infra failures, and generic exceptions fall through to the placeholder without retrying, "since a different SELECT won't fix a ClickHouse outage or a heartbeat timeout".
- **Found:** before the change this path did not retry. The old `Exception("Query failed")` fell into the catch-all at `query_executor.py:474-485` and re-raised a plain `Exception`, which is absent from `_RETRYABLE_QUERY_ERRORS`. So the extra LLM calls and reruns for hidden-message failures are new with this diff.
- **Impact:** during a ClickHouse degradation, every failed step of an AI report now triples its query attempts and spends two blind fix-LLM calls, and Max chat is told to retry with adjusted inputs for a failure no rewrite addresses.
- **Impact:** the fix needs a decision rather than a mechanical swap. `MaxToolTransientError` is not in `_RETRYABLE_QUERY_ERRORS`, so switching to it removes the retry the PR wanted, and the pipeline has no retry-without-changes path today.
- **Priority:** kept at `should_fix` rather than raised. The load added is bounded and sits on a low-volume scheduled path, and the same wrapping already happens for user-safe infra errors: `query_executor.py:443-444` raises `APIException(error_message)`, which the handler at `query_executor.py:455-473` already converts into `MaxToolRetryableError`. The diff widens an existing pattern; it does not create it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Classify 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.
</potential_solution>
There was a problem hiding this comment.
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.
The async query status now carries the exception or ClickHouse error type in error_code for non-user-safe failures, not only the DRF exception code. Update the QueryStatus.error_code schema description so the documented public contract matches what the field can hold, including when the message text is hidden. Regenerated schema.json and posthog/schema.py from the TypeScript source. Generated-By: PostHog Desktop Task-Id: de4edea4-3d44-49da-93d3-0a39abdc791b
New commits were pushed — dismissing the stamphog approval from an earlier head. This PR no longer qualifies for automatic review.
A non-user-safe async query failure that happens outside execute_process_query — concurrency-limit exhaustion after retries, or worker loss — reaches the Celery on_failure callback instead of the handler the PR already fixed. That callback set no error_code, so the AI executor still saw an opaque "Query failed" for this path. Record the exception class as a machine-readable error_code in _process_query_task_failure for non-user-safe failures, mirroring execute_process_query, so the AI executor and polling clients get the cause. The user-safe APIException branch is unchanged (it already surfaces a message). Parameterize the existing callback test to cover both the APIException path and a non-API failure that must now carry the class name as its code. Generated-By: PostHog Desktop Task-Id: de4edea4-3d44-49da-93d3-0a39abdc791b
Problem
ee/hogai/context/insight/query_executor.pythen raised a plainException("Query failed")with no cause. That type is absent from_RETRYABLE_QUERY_ERRORS, so the report pipeline gave up on the step instead of retrying it.Changes
Query failed: CHQueryErrorMemoryLimitExceeded) instead of dead-ending on "Query failed".execute_process_querycarries the exception class as a machine-readableerror_codeon the failed status, even when the message text stays hidden because it is not user-safe. It reuses the existingclickhouse_error_typehelper, which is already the observability-safe representation.error_codeand raisesMaxToolRetryableErrorfor the no-message case, so transient ClickHouse failures come back as retryable. This mirrors the existing structurally-valid error branch a few lines below.MaxToolRetryableErrorjoins the executor's firstexceptclause so the catch-all below does not re-wrap it back into a plainException.How did you test this code?
test_async_query_polling_error_without_message_is_retryable: a failed status witherror_codebut noerror_messagemust raiseMaxToolRetryableErrorand surface the code. Guards the retry-and-diagnose behavior this PR adds.test_async_query_non_user_safe_error_carries_error_code_without_message: a non-user-safe error keepserror_messagehidden but setserror_codeto the class name. Guards the machine-readable cause.test_query_executor.pyandtest_execute_async.py: all pass locally.Automatic notifications
Docs update
None — no user-facing API, setting, or documented workflow changes.
🤖 Agent context
Autonomy: Fully autonomous
/writing-tests,/writing-pr-descriptions.MaxToolRetryableErrorfor the structurally-valid error case.hogli review(Greptile) could not run in this sandbox — the CLI is not installed and flox is unavailable. A manual self-review of the diff was done instead.Created with PostHog Desktop from this inbox report.