fix(webapp,clickhouse): treat SQL the caller wrote as their mistake, not ours

The quota list was built on the wrong axis. It classified by resource
limit, but production shows the volume is in plain SQL mistakes: one
error group alone, a missing GROUP BY on the public query API, accounts
for over a million events across hundreds of users, and it stayed at
error level because its code is not a limit.

ClickHouse rejections that mean the SQL itself is wrong now log at warn,
but only when the caller wrote that SQL. The same rejection on TRQL we
generated is our bug and keeps alerting, so the decision is gated on a
userAuthoredQuery flag set by the public query API and the query editor.
Generated dashboard tiles deliberately do not set it.

Drops QUERY_WAS_CANCELLED from the limit list. It was there on the
reasoning that client disconnects would be noisy; it has not fired once
in the last fourteen days, so it was speculation rather than evidence.

Author: ericallam

apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx

@@ -183,6 +183,7 @@ export const action = async ({ request, params }: ActionFunctionArgs) => {
     const queryResult = await executeQuery({
       name: "query-page",
       query,
+      userAuthoredQuery: true,
       scope,
       organizationId: project.organizationId,
       projectId: project.id,

apps/webapp/app/routes/api.v1.query.ts

@@ -54,6 +54,7 @@ const { action, loader } = createActionApiRoute(
     const queryResult = await executeQuery({
       name: "api-query",
       query,
+      userAuthoredQuery: true,
       scope: scope as QueryScope,
       organizationId: env.organization.id,
       projectId: env.project.id,

apps/webapp/app/services/queryService.server.ts

@@ -100,6 +100,13 @@ export type ExecuteQueryOptions<TOut extends z.ZodSchema> = Omit<
   };
   /** Custom per-org concurrency limit (overrides default) */
   customOrgConcurrencyLimit?: number;
+  /**
+   * Set when the caller wrote `query` themselves, as on the public query API and
+   * the query editor. ClickHouse rejecting their SQL is then their mistake, so it
+   * is logged as a warning instead of raising an alert. Leave unset for TRQL we
+   * generate, where the same rejection is a bug worth alerting on.
+   */
+  userAuthoredQuery?: boolean;
 };
 
 /**

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

@@ -179,10 +179,15 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
             queryId,
           };
 
-          if (isClickhouseQuotaError(clickhouseError)) {
-            this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
-          } else {
-            this.logger.error("Error querying clickhouse", errorLogFields);
+          switch (classifyClickhouseError(clickhouseError, false)) {
+            case "quota":
+              this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
+              break;
+            case "invalid-sql":
+              this.logger.warn("ClickHouse rejected an invalid query", errorLogFields);
+              break;
+            default:
+              this.logger.error("Error querying clickhouse", errorLogFields);
           }
 
           recordClickhouseError(span, clickhouseError);
@@ -271,6 +276,11 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
      * record what produced the SQL, e.g. the TSQL a caller actually wrote.
      */
     logFields?: Record<string, unknown>;
+    /**
+     * Set when the SQL originates from whoever made the request rather than
+     * from us. Invalid-SQL rejections are then their mistake, not a bug.
+     */
+    userAuthoredQuery?: boolean;
   }): ClickhouseQueryWithStatsFunction<z.input<TIn>, z.output<TOut>> {
     return async (params, options) => {
       const queryId = randomUUID();
@@ -340,10 +350,15 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
             queryId,
           };
 
-          if (isClickhouseQuotaError(clickhouseError)) {
-            this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
-          } else {
-            this.logger.error("Error querying clickhouse", errorLogFields);
+          switch (classifyClickhouseError(clickhouseError, req.userAuthoredQuery)) {
+            case "quota":
+              this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
+              break;
+            case "invalid-sql":
+              this.logger.warn("ClickHouse rejected an invalid query", errorLogFields);
+              break;
+            default:
+              this.logger.error("Error querying clickhouse", errorLogFields);
           }
 
           recordClickhouseError(span, clickhouseError);
@@ -479,10 +494,15 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
             queryId,
           };
 
-          if (isClickhouseQuotaError(clickhouseError)) {
-            this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
-          } else {
-            this.logger.error("Error querying clickhouse", errorLogFields);
+          switch (classifyClickhouseError(clickhouseError, false)) {
+            case "quota":
+              this.logger.warn("Query exceeded a ClickHouse limit", errorLogFields);
+              break;
+            case "invalid-sql":
+              this.logger.warn("ClickHouse rejected an invalid query", errorLogFields);
+              break;
+            default:
+              this.logger.error("Error querying clickhouse", errorLogFields);
           }
 
           recordClickhouseError(span, clickhouseError);
@@ -631,7 +651,7 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
           queryId,
         };
 
-        if (error instanceof Error && isClickhouseQuotaError(error)) {
+        if (error instanceof Error && classifyClickhouseError(error, false) === "quota") {
           self.logger.warn("Streamed query exceeded a ClickHouse limit", errorLogFields);
         } else {
           self.logger.error("Error streaming clickhouse", errorLogFields);
@@ -1043,15 +1063,51 @@ const CLICKHOUSE_QUOTA_ERROR_TYPES = new Set([
   "TOO_MANY_ROWS",
   "TOO_MANY_BYTES",
   "TOO_MANY_ROWS_OR_BYTES",
-  "QUERY_WAS_CANCELLED",
 ]);
 
-function isClickhouseQuotaError(error: Error): boolean {
-  return (
-    error instanceof ClickHouseError &&
-    error.type !== undefined &&
-    CLICKHOUSE_QUOTA_ERROR_TYPES.has(error.type)
-  );
+/**
+ * ClickHouse error types that mean the SQL itself is wrong. Only treated as the
+ * caller's fault when the query was written by the caller — the same error on a
+ * query we generated is our bug and has to keep alerting.
+ */
+const CLICKHOUSE_INVALID_SQL_ERROR_TYPES = new Set([
+  "NOT_AN_AGGREGATE",
+  "ILLEGAL_AGGREGATION",
+  "UNKNOWN_IDENTIFIER",
+  "UNKNOWN_FUNCTION",
+  "UNKNOWN_TABLE",
+  "AMBIGUOUS_COLUMN_NAME",
+  "MULTIPLE_EXPRESSIONS_FOR_ALIAS",
+  "SYNTAX_ERROR",
+  "BAD_ARGUMENTS",
+  "TYPE_MISMATCH",
+  "NO_COMMON_TYPE",
+  "ILLEGAL_TYPE_OF_ARGUMENT",
+  "ILLEGAL_COLUMN",
+  "CANNOT_CONVERT_TYPE",
+  "CANNOT_PARSE_TEXT",
+  "CANNOT_PARSE_NUMBER",
+  "CANNOT_PARSE_DATE",
+  "CANNOT_PARSE_DATETIME",
+  "CANNOT_PARSE_INPUT_ASSERTION_FAILED",
+]);
+
+type ClickhouseErrorCategory = "quota" | "invalid-sql" | "fault";
+
+function classifyClickhouseError(
+  error: Error,
+  userAuthoredQuery: boolean | undefined
+): ClickhouseErrorCategory {
+  if (!(error instanceof ClickHouseError) || error.type === undefined) {
+    return "fault";
+  }
+  if (CLICKHOUSE_QUOTA_ERROR_TYPES.has(error.type)) {
+    return "quota";
+  }
+  if (userAuthoredQuery && CLICKHOUSE_INVALID_SQL_ERROR_TYPES.has(error.type)) {
+    return "invalid-sql";
+  }
+  return "fault";
 }
 
 function recordClickhouseError(span: Span, error: Error): void {

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

@@ -114,6 +114,11 @@ export interface ExecuteTSQLOptions<TOut extends z.ZodSchema> {
    * (counters zero-fill, gauges carry forward). Off by default.
    */
   fillGaps?: boolean;
+  /**
+   * Set when `query` was written by whoever made the request rather than by us.
+   * A rejection of their SQL is then their mistake, not a bug on our side.
+   */
+  userAuthoredQuery?: boolean;
 }
 
 /**
@@ -215,6 +220,7 @@ export async function executeTSQL<TOut extends z.ZodSchema>(
       schema: isExplain ? z.object({ explain: z.string() }) : options.schema,
       settings: options.clickhouseSettings,
       logFields: { tsql: options.query },
+      userAuthoredQuery: options.userAuthoredQuery,
     });
 
     const [error, result] = await queryFn(params);
@@ -249,6 +255,7 @@ export async function executeTSQL<TOut extends z.ZodSchema>(
             schema: z.object({ explain: z.string() }),
             settings: options.clickhouseSettings,
             logFields: { tsql: options.query },
+            userAuthoredQuery: options.userAuthoredQuery,
           });
 
           const [additionalError, additionalResult] = await additionalQueryFn(params);

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

@@ -140,6 +140,11 @@ export interface ClickhouseReader {
      * record what produced the SQL, e.g. the TSQL a caller actually wrote.
      */
     logFields?: Record<string, unknown>;
+    /**
+     * Set when the SQL originates from whoever made the request rather than
+     * from us. Invalid-SQL rejections are then their mistake, not a bug.
+     */
+    userAuthoredQuery?: boolean;
   }): ClickhouseQueryWithStatsFunction<z.input<TIn>, z.output<TOut>>;
 
   queryFast<TOut extends Record<string, any>, TParams extends Record<string, any>>(req: {

internal-packages/clickhouse/src/tsql.test.ts

@@ -1694,6 +1694,55 @@ describe("TSQL Error Log Levels", () => {
     }
   );
 
+  clickhouseTest(
+    "logs invalid caller-written SQL as a warning",
+    async ({ clickhouseContainer }) => {
+      const client = new ClickhouseClient({
+        name: "test",
+        url: clickhouseContainer.getConnectionUrl(),
+      });
+
+      const [error] = await executeTSQL(client, {
+        name: "test-user-authored-invalid",
+        query: "SELECT status, sum(is_test) AS n FROM task_runs",
+        schema: z.object({ status: z.string(), n: z.number() }),
+        enforcedWhereClause: {
+          organization_id: { op: "eq", value: "org_tenant1" },
+        },
+        tableSchema: [taskRunsSchema],
+        userAuthoredQuery: true,
+      });
+
+      expect(error).not.toBeNull();
+      expect(logged(warnSpy)).toContain("ClickHouse rejected an invalid query");
+      expect(logged(errorSpy)).not.toContain("Error querying clickhouse");
+    }
+  );
+
+  clickhouseTest(
+    "keeps invalid SQL we generated at error level",
+    async ({ clickhouseContainer }) => {
+      const client = new ClickhouseClient({
+        name: "test",
+        url: clickhouseContainer.getConnectionUrl(),
+      });
+
+      const [error] = await executeTSQL(client, {
+        name: "test-internal-invalid",
+        query: "SELECT status, sum(is_test) AS n FROM task_runs",
+        schema: z.object({ status: z.string(), n: z.number() }),
+        enforcedWhereClause: {
+          organization_id: { op: "eq", value: "org_tenant1" },
+        },
+        tableSchema: [taskRunsSchema],
+      });
+
+      expect(error).not.toBeNull();
+      expect(logged(errorSpy)).toContain("Error querying clickhouse");
+      expect(logged(warnSpy)).not.toContain("ClickHouse rejected an invalid query");
+    }
+  );
+
   clickhouseTest("logs a ClickHouse limit breach as a warning", async ({ clickhouseContainer }) => {
     const client = new ClickhouseClient({
       name: "test",