feat(webapp,clickhouse): return an actionable error instead of a 500 when a runs list query is too expensive - #4773
Conversation
…when a runs list query is too expensive When a runs list query exceeds ClickHouse resource limits, surface a clear error asking the user to narrow their time range, and stop retrying it. The public runs.list API now returns 422 with a helpful message and an x-should-retry: false header, so the failure is non-fatal for tasks. The dashboard runs list shows the same guidance instead of a generic error page.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change detects ClickHouse resource-limit errors and preserves their types through 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the problem and the implemented fix, but it omits the required issue reference, checklist, Testing section, Changelog section, and Screenshots section from the repository template. ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Observability mapAs of 19/100 over 449 measured of 467 entry points (base 19, no change) What this PR changed
FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Route the runs list on the task, scheduled, agent, webhook, and error-group pages through the same capped ClickHouse pool as the main runs list, and let a resource-limit failure surface the in-panel error state instead of being swallowed to an empty list or bubbling to a full-page error.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…wait An empty JSX fragment as errorElement trips the no-useless-fragment lint rule. Replace it with a small component that renders null, which handles a runs-list rejection locally on the pagination await so it does not bubble to the route error boundary.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/components/runs/v3/RunsListErrorState.tsx (1)
12-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
RunsListErrorStatefill the available runs-list region before centering.The root
<div>at Line 12 has no height or flex-growth rule, soitems-centeronly centers inside its content-sized box. Give fixed-height callers a definite height, such ash-full, and let auto-sized flex callers grow before centering.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fd89371-8ece-4961-9f07-529aa0acd51f
📒 Files selected for processing (5)
apps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.$webhookParam/route.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.$webhookParam/route.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (32)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: 🛡️ E2E Auth Tests (full)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (7)
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsxapps/webapp/app/components/runs/v3/RunsListErrorState.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx
🔇 Additional comments (4)
apps/webapp/app/components/runs/v3/RunsListErrorState.tsx (1)
21-28: LGTM!apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.agents.$agentParam/route.tsx (1)
25-29: LGTM!Also applies to: 100-103, 162-162, 174-179, 354-354, 408-408
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.scheduled.$taskParam/route.tsx (1)
63-67: LGTM!Also applies to: 151-154, 219-219, 232-237, 388-395
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.tasks.standard.$taskParam/route.tsx (1)
33-37: LGTM!Also applies to: 111-114, 161-161, 175-180, 284-291
… tuning Every ClickHouse read query now records duration (wall-clock and server-side), rows and bytes read, peak memory usage, an in-flight gauge, and an error counter tagged by ClickHouse error type, all keyed by client and operation. These give the runs-list pool live signal for sizing max_open_connections and the per-query caps instead of one-off query_log snapshots.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…eturning A thrown exception in the query path (e.g. a malformed response or a stream error) bypassed the metrics recording and emitted no error or duration sample. Record an exception-typed error metric before rethrowing so those failures are visible too.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal-packages/clickhouse/src/client/client.ts (1)
134-140: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winUse bounded metric attributes.
this.nameandreq.namehave unrestrictedstringtypes. Theclientandoperationattributes can create a new Prometheus time series for each value. Remove these attributes, or map them to a closed bounded set, before callingrecord()andadd(). Keep bounded attributes such asstatusanderror_type.Proposed fix
- const attributes = { client: this.name, operation }; + const attributes = {}; ... - this.queryInFlight.add(1, { client: this.name }); + this.queryInFlight.add(1); ... - .finally(() => this.queryInFlight.add(-1, { client: this.name })); + .finally(() => this.queryInFlight.add(-1));As per coding guidelines, metric attributes must use only enums, booleans, bounded error codes, or bounded shard IDs.
Also applies to: 194-194, 325-325, 378-378, 550-550, 571-571, 681-681
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb2fe487-bbb9-4be8-8944-356929b32901
📒 Files selected for processing (1)
internal-packages/clickhouse/src/client/client.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (36)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: runops-guard / runops-guard
- GitHub Check: report
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: 🛡️ E2E Auth Tests (full)
- GitHub Check: 🛡️ E2E Auth Tests (full)
🧰 Additional context used
📓 Path-based instructions (5)
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/src/client/client.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/src/client/client.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/clickhouse/src/client/client.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/clickhouse/src/client/client.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/clickhouse/src/client/client.ts
🔇 Additional comments (1)
internal-packages/clickhouse/src/client/client.ts (1)
74-102: Add required development crumbs.Add
//@crumbs`` markers around metric initialization and exception metric recording. Use an existing crumb namespace, or ask before selecting one.As per coding guidelines, “Add crumbs as you write code.”
Also applies to: 321-324, 546-549, 677-680
Source: Coding guidelines
2 participants
Summary
When a runs list query is too expensive to complete, it now fails with a clear, actionable error instead of a generic 500.
Previously, a runs list query that exceeded ClickHouse resource limits threw an opaque error. On the public
runs.listAPI that surfaced as a retryable 500, so a customer task calling it would keep retrying a query that could never succeed. On the dashboard it rendered as a generic error page with no hint about what to do.Fix
The ClickHouse client now tags resource-limit failures (memory, time, rows, bytes) with their error type, and the runs repository maps those to a dedicated
RunsListQueryError(HTTP 422).runs.listAPI returns 422 with a message telling the user to narrow theircreated_atrange, plus anx-should-retry: falseheader so the SDK does not retry it.