fix(webapp,clickhouse): stop invalid customer queries alerting, and isolate Sentry scope per request

[ericallam]

Summary

A query sent to the query API with a typo in it, like a column name that does not exist, was being reported as a server error. That put customer SQL mistakes into our error alerting, where they made up almost all of the volume on one of our noisiest alerts, and it drowned out the failures that are actually ours to fix. This makes the level match who is at fault, and fixes two related problems found alongside it.

Invalid queries are the caller's, not ours

The query API route already got this right. It checks for QueryError, logs at warn, and returns a 400, with a comment saying the system handles it gracefully and no alert is needed.

The layer underneath ignored that. executeTSQL logged every exception out of its catch block at error, including the compile failures the route was about to turn into a 400, and error-level logs are forwarded to error reporting.

The TSQL package already draws the line we need:

export class ExposedTSQLError extends BaseTSQLError {
  /** An exception that can be exposed to the user. */
}

export class InternalTSQLError extends BaseTSQLError {
  /** An internal exception in the TSQL engine. */
}

SyntaxError and QueryError extend the first. So the catch block now branches on ExposedTSQLError and logs those at warn, keeping error for InternalTSQLError and anything unanticipated, which is a genuine compiler bug.

SQL the caller wrote is their mistake, not ours

The same asymmetry showed up one level down. A query that compiles fine can still be rejected by ClickHouse at execution, and most of those rejections mean the caller's SQL is wrong rather than that we generated something bad.

This is where the volume actually is. Checking production, one error group alone, a missing GROUP BY on the public query API (NOT_AN_AGGREGATE), accounts for over a million events across hundreds of users. It is by far the largest error group in the project, and classifying only by resource limit would have left every one of those at error level.

So rejections are split three ways in ClickhouseClient, which is the only place holding the parsed ClickHouseError and its symbolic type. By the time the error reaches executeTSQL it has been wrapped and the type is gone, and the type never appears in the message text, so it cannot be recovered by string matching.

• Resource limits (memory ceiling, timeout, row/byte caps) log at warn. The query is valid, it just asked for more than it is allowed to spend.
• Invalid SQL (NOT_AN_AGGREGATE, UNKNOWN_IDENTIFIER, SYNTAX_ERROR, the type and parse families) logs at warn only when the caller wrote the SQL.
• Everything else keeps alerting.

That gate matters. The client is shared, so the identical rejection on TRQL we generated is our bug and has to stay at error. Callers opt in with userAuthoredQuery:

caller	who wrote the SQL	opts in
public query API	the customer	yes
query editor	the customer	yes
agent charts	the agent's model	yes
built-in dashboard tiles	us, in code	no
queue metric cards	us, in code	no
health report	us, in code	no

The agent is the one judgement call. Its TRQL is not typed by a person, but it is also not something a code fix makes correct, so a query it gets wrong is not worth waking anyone for. The same endpoint serves built-in tiles whose TRQL we do write, so the opt-in lives with the caller rather than the route.

Separately, when one of these queries did fail, the log recorded the generated ClickHouse SQL but not the query the caller actually wrote, which made the reports hard to act on. queryWithStats takes an optional logFields that executeTSQL uses to attach the original TSQL.

Events were attributed to the wrong request

Chasing the above turned up something broader: only a tenth of the events on that alert pointed at the query API. The rest were pinned to unrelated requests that happened to be in flight at the same time, so the alert looked like the trigger endpoint was failing.

Sentry.init runs with skipOpenTelemetrySetup: true, because we register our own OTel pipeline. That skips initOpenTelemetry, and one of the things it does is:

api.context.setGlobalContextManager(new SentryContextManager());

The async-context strategy is still installed, but withIsolationScope only marks the OTel context and delegates the actual fork to that context manager:

// "We depend on the otelContextManager to handle the context/hub"
return api.context.with(ctx.setValue(SENTRY_FORK_ISOLATION_SCOPE_CONTEXT_KEY, true), ...)

provider.register() installed a plain AsyncLocalStorageContextManager, which does not know that key. The lookup found no scopes on the context and fell back to the process-global default isolation scope, so every request wrote its request data into the same object and the last writer won.

The tracer now registers SentryContextManager, which subclasses AsyncLocalStorageContextManager, so OTel behaviour is unchanged. It is also registered on the path where tracing is disabled, which previously never called register() at all and so had no context manager of its own.

Tenant tags were always correct, because those come from our own async local storage rather than the isolation scope. That is why the attribution being wrong was not obvious.

This affects every error report the webapp sends, not just the query API.

Verification

internal-packages/clickhouse: 76 tests pass, including eight covering each level decision against a real ClickHouse container. Three pairs pin the gate open and shut at both layers: an invalid query, a compile failure, and a real limit breach driven with max_rows_to_read each log at warn with userAuthoredQuery and at error without it.

The isolation fix has a test that reproduces the leak before asserting the fix. Two overlapping requests each tag their own isolation scope; with the plain context manager the slower one reads back the other's tag, and with SentryContextManager each reads back its own.

Measured separately against a faithful reproduction of the server's wiring (own OTel pipeline, CommonJS entry) at 200 concurrent requests: per-request attribution goes from 0.5% to 100%, while span nesting, context propagation across awaits, and distinct trace IDs are identical before and after.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 5463a40

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

OpenTelemetry now installs SentryContextManager for enabled and disabled tracing, preserving isolation scopes across concurrent requests. New tests cover scope leakage and isolation behavior. ClickHouse query methods classify quota errors as warnings, accept additional logging fields, and retain errors for other failures. TSQL execution includes query metadata in logs and distinguishes invalid queries from execution failures. Change log entries document both behavior updates.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description is informative, but it does not follow the required template: no Closes #issue, checklist, Testing, Changelog, or Screenshots sections.	Add the required template sections, including Closes #issue, checklist items, a Testing section, a Changelog summary, and Screenshots if applicable.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the two main changes: query error logging and per-request Sentry isolation.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/tri-12475-customer-tsql-query-errors-page-us-real-query-failures-are

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

internal-packages/clickhouse/src/client/client.ts (2)

269-273: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Propagate logFields through every queryWithStats failure log.

req.logFields is only merged in the ClickHouse request-error branch. Parameter-validation and result-schema failures still omit it, so callers lose the originating query metadata despite the new field’s documentation and PR contract.

Suggested fix

 this.logger.error("Error parsing query params", {
+  ...req.logFields,
   name: req.name,
   error: validParams.error,
   query: req.query,
   params,
   queryId,
 });

 this.logger.error("Error parsing clickhouse query result", {
+  ...req.logFields,
   name: req.name,
   error: parsed.error,
   query: req.query,
   params,
   queryId,
 });

Also applies to: 334-347

---

474-486: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Classify errors raised while buffering queryFast results.

This catch only covers this.client.query; the subsequent resultSet.stream() loop at Lines 513-533 runs outside it. A quota error raised mid-stream therefore bypasses the warning classification, recordClickhouseError, and error logging entirely. Wrap that loop in the same shared error-handling path used by queryFastStream.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 85cc8419-6c7b-499e-9977-49a2d4dcccee

📥 Commits

Reviewing files that changed from the base of the PR and between 660e7f1 and 57b6caf.

📒 Files selected for processing (2)

• internal-packages/clickhouse/src/client/client.ts
• internal-packages/clickhouse/src/client/tsql.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• internal-packages/clickhouse/src/client/tsql.ts

📜 Review details

⏰ Context from checks skipped due to timeout. (3)

• GitHub Check: code-quality / code-quality
• GitHub Check: Analyze (javascript-typescript)
• GitHub Check: Analyze (actions)

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead

**/*.{ts,tsx}: Prefer static imports over dynamic import(); use dynamic imports only for unresolvable circular dependencies, genuine performance code splitting, or conditional runtime loading.
Import Trigger.dev tasks from @trigger.dev/sdk; never use @trigger.dev/sdk/v3 or deprecated client.defineJob.
Add agentcrumbs while writing code using approved namespaces; mark lines with // @Crumbs or blocks with `// `#region` `@crumbs, and strip them before merging.

Files:

• internal-packages/clickhouse/src/client/client.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Use function declarations instead of default exports

Files:

• internal-packages/clickhouse/src/client/client.ts

**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)

**/*.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
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries

Files:

• internal-packages/clickhouse/src/client/client.ts

internal-packages/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

For internal packages, use typecheck for verification and never use build as the correctness check.

Files:

• internal-packages/clickhouse/src/client/client.ts

🧠 Learnings (10)

📚 Learning: 2026-03-22T13:26:12.060Z

Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-03-22T19:24:14.403Z

Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-05-18T08:21:27.694Z

Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-05-18T08:21:27.694Z

Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-06-13T19:53:13.759Z

Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3937
File: packages/trigger-sdk/skills/realtime-and-frontend/SKILL.md:258-260
Timestamp: 2026-06-13T19:53:13.759Z
Learning: When reviewing code that uses `trigger.dev/react-hooks`’s `useRealtimeRun`, preserve the call signature where the first argument is the full realtime handle object (not `handle.id`). This is intentional to maintain type-safety and is consistent with the official docs; do not suggest changing the first argument from the handle object to `handle.id`.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-06-17T17:13:49.929Z

Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3948
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsx:48-62
Timestamp: 2026-06-17T17:13:49.929Z
Learning: In triggerdotdev/trigger.dev, within `dashboardLoader`/`dashboardAction` (or similar context resolver code) whenever you resolve an organization ID from an organization slug for RBAC/enterprise authorization scope, always read from the primary Prisma client (`prisma`), not `$replica`. Using `$replica` can hit replica-lag and cause the RBAC lookup/authorization to run without the correct org scope (bypassing intended role enforcement). Implement the slug→org lookup with `prisma.organization.findFirst(...)` (or equivalent primary-client query) and add an inline comment documenting why the primary client is required (replica lag could lead to unscoped RBAC checks).

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-06-23T13:04:21.413Z

Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4023
File: apps/webapp/app/services/upsertBranch.server.ts:14-18
Timestamp: 2026-06-23T13:04:21.413Z
Learning: In TypeScript, it’s valid to `import { type X }` and then use `typeof X` in a type-only position, e.g. `type Alias = z.infer<typeof X>`. The `type` modifier suppresses the runtime import, but the type checker still has the full exported type so `z.infer<typeof X>` can resolve correctly. In code reviews, don’t flag this as a TypeScript compile error as long as `typeof X` is used in a type context (e.g., with `z.infer`, `type` aliases, generics), not as a runtime value.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-06-04T18:16:35.386Z

Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-06-09T17:58:04.699Z

Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3879
File: apps/webapp/app/models/vercelIntegration.server.ts:619-630
Timestamp: 2026-06-09T17:58:04.699Z
Learning: In this codebase, outbound raw `fetch` calls should typically rely on Node/undici’s default request timeout (about ~300s) rather than adding a per-call `AbortController` + `setTimeout` wrapper inside individual functions (e.g. in files like `apps/webapp/app/models/vercelIntegration.server.ts`). During code review, do not flag the absence of a per-call timeout on a single `fetch` as an issue; if per-call timeouts are needed, they should be implemented via a codebase-wide convention (e.g., a shared fetch wrapper or documented pattern) rather than ad-hoc per-function changes.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

📚 Learning: 2026-07-03T09:41:46.517Z

Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 4131
File: internal-packages/metrics-pipeline/src/types.ts:0-0
Timestamp: 2026-07-03T09:41:46.517Z
Learning: When generating ClickHouse `UInt64`-backed ordering keys from epoch-derived values (e.g., `ms` and `seq`), avoid JS `number` arithmetic that can exceed the safe-integer range (2^53). Compute the key using `BigInt` (e.g., `BigInt(ms) * 100000n + BigInt(seq)`) and return it as a `string` (via `.toString()`) to preserve exact ordering. Ensure the corresponding Zod schema for the raw input (e.g., `QueueMetricsRawV1Input.order_key`) accepts/preserves this exact value (typically via `z.union([z.string(), z.number()]).optional()`), so callers can assign the computed value directly into the ClickHouse `UInt64` column without precision loss or misordering.

Applied to files:

• internal-packages/clickhouse/src/client/client.ts

🔇 Additional comments (3)

internal-packages/clickhouse/src/client/client.ts (3)

174-186: LGTM!

---

626-638: LGTM!

---

1034-1056: 🎯 Functional Correctness

No change needed. The quota error set covers memory, timeout/slow query, row, byte, combined row/byte, and cancelled query limits, matching the ClickHouse errors this package’s error type maps use.

[pkg-pr-new]

Open in StackBlitz

@trigger.dev/build

npm i https://pkg.pr.new/@trigger.dev/build@5463a40

trigger.dev

npm i https://pkg.pr.new/trigger.dev@5463a40

@trigger.dev/core

npm i https://pkg.pr.new/@trigger.dev/core@5463a40

@trigger.dev/python

npm i https://pkg.pr.new/@trigger.dev/python@5463a40

@trigger.dev/react-hooks

npm i https://pkg.pr.new/@trigger.dev/react-hooks@5463a40

@trigger.dev/redis-worker

npm i https://pkg.pr.new/@trigger.dev/redis-worker@5463a40

@trigger.dev/rsc

npm i https://pkg.pr.new/@trigger.dev/rsc@5463a40

@trigger.dev/schema-to-json

npm i https://pkg.pr.new/@trigger.dev/schema-to-json@5463a40

@trigger.dev/sdk

npm i https://pkg.pr.new/@trigger.dev/sdk@5463a40

commit: 5463a40

[devin-ai-integration]

Devin Review found 1 new potential issue.