feat(webapp): isolate the runs list ClickHouse read pool - #4763
feat(webapp): isolate the runs list ClickHouse read pool#4763ericallam wants to merge 7 commits into
Conversation
Give the runs-list ClickHouse pool server-side query protection (max_execution_time, thread and memory caps, a per-user concurrency breaker, readonly) so one tenant expensive query cannot saturate the shared read service, and cap the runs list created_at lower bound to a bounded window so an unbounded filter cannot scan every partition. Billing and bulk count reads move to the read pool, off the ingestion writer. Count queries are never date-capped so billing keeps counting runs of any age.
|
|
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 runs list now uses a dedicated ClickHouse configuration with query limits, resource caps, readonly mode, and request timeouts. Runs-list filters use 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![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.
… instead of a created_at clamp Replaces the created_at window clamp with PREWHERE routing on the runs-list query. Immutable and additive-only filters (tags, task_identifier, and the rest) move into PREWHERE so ClickHouse filters, and uses the tags skip index, before FINAL reconciles versions and before materialising the wide columns. This bounds the memory a filtered runs-list query uses without dropping any rows, unlike the date clamp which hid older runs from the list and the runs.list API. status stays in WHERE (post-FINAL): it is the one lifecycle-mutable filter, so PREWHERE-ing it would keep a stale version and drop the winning one.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Drops max_memory_usage_for_user and max_concurrent_queries_for_user. Those are per-ClickHouse-user limits, and every connection is the default user, so hitting the shared budget rejects whichever query arrives next rather than the one responsible, which would fail queries for uninvolved tenants. The per-query caps (max_execution_time, max_memory_usage, max_threads) bound a bad query to itself, and the server-level max_server_memory_usage protects the node.
max_threads and max_memory_usage were opt-in env vars, so out of the box, or if deploy config lagged, the pool ran with no thread or per-query memory cap, which is the thread oversubscription that hurt throughput under load. Give both a conservative default (4 threads, 1 GiB) so the guardrails hold without depending on a deploy-time config step, following the logs and query read pools.
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/services/clickhouse/clickhouseFactory.server.ts (1)
312-314: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake the runs-list resource caps non-overridable.
readonly=2permits session setting changes. The ClickHouse wrapper merges per-operationclickhouse_settingsafter client-level settings, so callers can setmax_memory_usage=0or raisemax_threadsandmax_execution_time. Enforce these caps with immutable server-side constraints for both default and organization-specificrunsListclients.
🧹 Nitpick comments (1)
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts (1)
306-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd crumb instrumentation for the new settings path.
Add an approved
//@Crumbsmarker or `// `#region` `@crumbsblock aroundgetRunsListClickhouseSettingsduring development. Strip it before merge.As per coding guidelines, add crumbs as you write code, not only when debugging, using
//@Crumbsor `// `#region` `@crumbs.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 640a8da7-abcd-4ef2-98d9-1a8f181d42c1
📒 Files selected for processing (2)
apps/webapp/app/env.server.tsapps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
💤 Files with no reviewable changes (1)
- apps/webapp/app/env.server.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (33)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 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 (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: runops-guard / runops-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (9)
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
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/services/clickhouse/clickhouseFactory.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
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/services/clickhouse/clickhouseFactory.server.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.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:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
🔇 Additional comments (2)
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts (2)
1-1: LGTM!
352-353: 🩺 Stability & AvailabilityNo change is required for query cancellation.
@internal/clickhousealready setscancel_http_readonly_queries_on_client_close: 1for the runs-list client.> Likely an incorrect or invalid review comment.
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.
Client request timeout now sits above the server max_execution_time (default 40s vs 35s, and the factory forces it to at least exec + 5s), so the server-side cap is what stops a slow query and the client stays connected to receive the error, instead of aborting first and leaving the query running. The numeric caps reject zero and negative values, since ClickHouse treats 0 as unlimited for max_execution_time and max_memory_usage, which would silently disable them.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/webapp/app/env.server.ts (1)
2236-2243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required crumbs to the new runs-list protection code.
apps/webapp/app/env.server.ts#L2236-L2243: mark the new environment settings with//@Crumbsor `// `#region` `@crumbs.apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts#L1-L1: cover the new import with a crumb marker.apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts#L306-L331: mark the timeout helper and settings block.apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts#L361-L363: mark default runs-list client wiring.apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts#L607-L609: mark organization-specific runs-list client wiring.As per coding guidelines, add crumbs as you write code and mark lines with
//@Crumbsor `// `#region` `@crumbs.Source: Coding guidelines
apps/webapp/app/services/clickhouse/clickhouseFactory.server.ts (1)
306-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the derived timeout through the factory.
The supplied
apps/webapp/test/runsListClickhouseSettings.test.tsconstructsClickHousedirectly withrequestTimeoutMs: 30_000. It does not verifygetRunsListRequestTimeoutMs()or the"runsList"organization-client branch. Add tests for configured timeouts below and above(max_execution_time + 5) * 1000.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c598e0b-f651-4465-9fe6-7a1867b827e4
📒 Files selected for processing (2)
apps/webapp/app/env.server.tsapps/webapp/app/services/clickhouse/clickhouseFactory.server.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (36)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: typecheck / typecheck
- GitHub Check: runops-guard / runops-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: audit
- GitHub Check: audit
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (9)
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
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/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
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/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.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:
apps/webapp/app/services/clickhouse/clickhouseFactory.server.tsapps/webapp/app/env.server.ts
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The region filter uses if(region != "", region, worker_queue): a run is region="" at trigger (so the expression yields worker_queue) and gets a real region at dequeue, so the expression flips from one non-empty value to another across a run versions. Under PREWHERE that is evaluated before FINAL reconciles versions, so it could keep a stale pre-dequeue version and drop the winner, returning runs whose current region no longer matches (and listRunIds drives bulk actions). Moved it back to WHERE (post-FINAL) with a regression test.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Both reflect execution/outcome and change across a run versions, so they are unsafe in PREWHERE (evaluated before FINAL): error_fingerprint is derived from status per snapshot and is cleared when a run recovers to a non-error status, and machine_preset can escalate to a larger machine on an out-of-memory retry. In either case an earlier version matches the filter while the winning version does not, so PREWHERE could keep the stale version and drop the winner. Only trigger-time identity columns and append-only arrays stay in PREWHERE.
1 participant
Summary
Improves the performance and reliability of the runs list and the
runs.listAPI, especially for large projects and filtered views.What changed
PREWHERE. Immutable and additive-only filters (tags, task identifier, version, queue, region, machine, and the rest) are applied inPREWHEREon thetask_runs_v2 FINALscan, so ClickHouse filters, and uses the tags skip index, before it reconciles versions and materialises the wide columns. Same results, far less memory per query.statusstays inWHERE: it changes across a run's versions, so filtering it beforeFINALcould return stale rows.max_execution_timepaired with the client request timeout, a per-querymax_memory_usage, amax_threadscap, andreadonly. Each bounds a single query to itself, so a heavy query can't affect other queries, and they are safe as pool-level settings only because this pool is read-only.Defaults are conservative for self-hosters; production values are set via env.