[superlog] Retry transient ClickHouse single-query drops#511
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
izadoesdev
left a comment
izadoesdev
left a comment
There was a problem hiding this comment.
Reviewed the clean replacement for #505. This carries only the intended packages/ai/src/query/batch-executor.ts retry behavior plus focused tests, with no stale branch noise.
Local verification passed:
cd packages/ai && bun test src/query/batch-executor.test.tscd packages/ai && bun run check-typesbun run lintbun run check-typesbun run test
I will merge this to staging after GitHub CI is green.
Greptile SummaryThis PR adds a single-retry mechanism to
Confidence Score: 4/5Safe to merge — the retry is strictly bounded to one extra attempt, AbortError is correctly excluded, and the new test suite validates all four key scenarios. The implementation is correct and well-tested. The only gaps are a missing telemetry event when a retry fires (making production retry frequency invisible) and a logically unreachable fallback return after the loop — neither affects correctness or user-facing behavior. Both changed files are straightforward. The unreachable return and absent retry telemetry are both in Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[runSingle called] --> B{config exists?}
B -- No --> C[return unknown type error]
B -- Yes --> D[attempt = 0]
D --> E[build SimpleQueryBuilder\nexecute query]
E -- success --> F[mergeWideEvent success\nreturn data]
E -- throws --> G{attempt == 0 AND\nisTransientClickHouseError?}
G -- Yes --> H[attempt = 1\nretry]
H --> E
G -- No --> I[mergeWideEvent error\nreturn error result]
E2[attempt 1 throws] --> I
H --> E2
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[runSingle called] --> B{config exists?}
B -- No --> C[return unknown type error]
B -- Yes --> D[attempt = 0]
D --> E[build SimpleQueryBuilder\nexecute query]
E -- success --> F[mergeWideEvent success\nreturn data]
E -- throws --> G{attempt == 0 AND\nisTransientClickHouseError?}
G -- Yes --> H[attempt = 1\nretry]
H --> E
G -- No --> I[mergeWideEvent error\nreturn error result]
E2[attempt 1 throws] --> I
H --> E2
Reviews (1): Last reviewed commit: "fix(ai): retry transient clickhouse quer..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (attempt === 0 && isTransientClickHouseError(e)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When a transient error triggers the retry path (continue), there is no mergeWideEvent call to record the event. In production you will have no signal for how often retries occur, making it impossible to distinguish "working as intended" from "constantly hammering ClickHouse on every request." A counter like mergeWideEvent({ query_retry: 1, query_retry_code: … }) before the continue would surface this in your wide traces.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return { type: req.type, data: [], error: "Query failed" }; | ||
| } | ||
|
|
||
| function groupBySchema( |
There was a problem hiding this comment.
Given the loop's exit conditions, this return is logically dead code. Attempt 0 always either succeeds (early return), is transient (continues to attempt 1), or is non-transient (early return). Attempt 1's catch block always returns because attempt === 0 is false for both success and error paths. TypeScript requires a trailing return here for type-narrowing, but a comment would signal the unreachability to future readers.
| return { type: req.type, data: [], error: "Query failed" }; | |
| } | |
| function groupBySchema( | |
| // This point is logically unreachable: attempt 0 always returns or continues, | |
| // and attempt 1 always returns from within the catch block. | |
| return { type: req.type, data: [], error: "Query failed" }; | |
| } | |
| function groupBySchema( |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 4/5
- In
packages/ai/src/query/batch-executor.ts, the retry branch cancontinueafter transient errors without amergeWideEvent, which creates an observability gap where healthy retries and ongoing connection instability look the same in production — add telemetry on the retry path before merging. - In
packages/ai/src/query/batch-executor.ts, retries may still run after cancellation if the error looks transient but is notAbortError, so canceled work can continue and waste resources or surprise callers — checkopts.abortSignalbefore scheduling/entering retry and short-circuit when aborted.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/ai/src/query/batch-executor.ts">
<violation number="1" location="packages/ai/src/query/batch-executor.ts:342">
P2: Honor opts.abortSignal before retrying. Otherwise a canceled request can issue the retry path whenever the thrown error is transient-shaped but not named AbortError.</violation>
<violation number="2" location="packages/ai/src/query/batch-executor.ts:343">
P2: The retry path emits no telemetry. When a transient error triggers `continue`, there is no `mergeWideEvent` call to record it. In production this makes it impossible to distinguish healthy retries from persistent connection issues. Consider adding something like `mergeWideEvent({ query_retry: 1, query_retry_code: ... })` before `continue`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant BE as Batch Executor
participant QB as SimpleQueryBuilder
participant CH as ClickHouse (chQuery)
participant Metrics as Wide Event Metrics
Note over Client,Metrics: Single Query Execution with Retry
Client->>BE: executeBatch([singleQueryRequest])
BE->>BE: runSingle(config, req)
loop attempt=0..1
BE->>QB: new SimpleQueryBuilder(config, req)
QB->>CH: builder.execute(abortSignal)
alt Success
CH-->>QB: query result data
QB-->>BE: data array
BE->>Metrics: mergeWideEvent(query_type, from, to, rows)
BE-->>Client: {type, data}
else Error raised
CH-->>QB: throws Error
QB-->>BE: error object
BE->>BE: isTransientClickHouseError(error)
alt transient AND attempt===0
BE->>BE: continue (retry)
else not transient OR attempt===1
BE->>Metrics: mergeWideEvent(query_error)
BE-->>Client: {type, data:[], error}
end
end
end
Note over BE,BE: Transient Error Detection (nested cause traversal)
BE->>BE: hasAbortError(error)
alt name==="AbortError" OR code==="ABORT_ERR"
BE->>BE: return false (no retry)
else
BE->>BE: check code in TRANSIENT_CODES (ECONNRESET, EPIPE, ETIMEDOUT, etc.)
alt code matches
BE->>BE: return true (transient)
else
BE->>BE: check message against TRANSIENT_MESSAGES
alt message matches
BE->>BE: return true (transient)
else
BE->>BE: recurse into cause (max depth 5)
end
end
end
Note over BE,BE: Single retry only on first attempt (attempt 0)
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| return { type: req.type, data: [], error }; | ||
| return { type: req.type, data }; | ||
| } catch (e) { | ||
| if (attempt === 0 && isTransientClickHouseError(e)) { |
There was a problem hiding this comment.
P2: Honor opts.abortSignal before retrying. Otherwise a canceled request can issue the retry path whenever the thrown error is transient-shaped but not named AbortError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ai/src/query/batch-executor.ts, line 342:
<comment>Honor opts.abortSignal before retrying. Otherwise a canceled request can issue the retry path whenever the thrown error is transient-shaped but not named AbortError.</comment>
<file context>
@@ -247,27 +321,35 @@ async function runSingle(
- return { type: req.type, data: [], error };
+ return { type: req.type, data };
+ } catch (e) {
+ if (attempt === 0 && isTransientClickHouseError(e)) {
+ continue;
+ }
</file context>
| if (attempt === 0 && isTransientClickHouseError(e)) { | |
| if (attempt === 0 && !opts?.abortSignal?.aborted && isTransientClickHouseError(e)) { |
| return { type: req.type, data }; | ||
| } catch (e) { | ||
| if (attempt === 0 && isTransientClickHouseError(e)) { | ||
| continue; |
There was a problem hiding this comment.
P2: The retry path emits no telemetry. When a transient error triggers continue, there is no mergeWideEvent call to record it. In production this makes it impossible to distinguish healthy retries from persistent connection issues. Consider adding something like mergeWideEvent({ query_retry: 1, query_retry_code: ... }) before continue.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ai/src/query/batch-executor.ts, line 343:
<comment>The retry path emits no telemetry. When a transient error triggers `continue`, there is no `mergeWideEvent` call to record it. In production this makes it impossible to distinguish healthy retries from persistent connection issues. Consider adding something like `mergeWideEvent({ query_retry: 1, query_retry_code: ... })` before `continue`.</comment>
<file context>
@@ -247,27 +321,35 @@ async function runSingle(
+ return { type: req.type, data };
+ } catch (e) {
+ if (attempt === 0 && isTransientClickHouseError(e)) {
+ continue;
+ }
+
</file context>
1 participant
Summary
code,message, and nestedcausefields while explicitly skippingAbortErrorSupersedes the useful part of #505 from a clean
stagingbranch. The original #505 is conflicted and polluted with stale already-landed history, so this PR carries only the intended AI query change.Verification
cd packages/ai && bun test src/query/batch-executor.test.tscd packages/ai && bun run check-typesbun run lintbun run check-typesbun run testSummary by cubic
Retries single-query analytics once on transient ClickHouse connection drops to reduce flaky failures. AbortError and non-transient errors are not retried; if the retry also fails, the final error is returned.
code,message, and nestedcause(e.g., ECONNRESET, ECONNREFUSED, EPIPE, ETIMEDOUT, undici socket errors); AbortError is excluded.@databuddy/db/clickhouseto cover retry success, final failure after retry, non-transient no-retry, and AbortError no-retry.Written for commit 4dee431. Summary will update on new commits.