fix(clickhouse): keep limit breaches on our own queries at error level

Downgrading quota errors regardless of who wrote the SQL was wrong. An
internal analytics query hitting the memory ceiling or a timeout is a
runaway query of ours, and it was losing the alert it used to raise.

Both downgrades are now gated on the same condition: ClickHouse rejected
SQL that the caller wrote. Anything on SQL we generated stays at error,
which also makes query, queryFast and the streaming path plain error
logs again, since none of them can carry caller-written SQL.

Author: ericallam

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

@@ -179,16 +179,7 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
             queryId,
           };
 
-          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);
-          }
+          this.logger.error("Error querying clickhouse", errorLogFields);
 
           recordClickhouseError(span, clickhouseError);
 
@@ -494,16 +485,7 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
             queryId,
           };
 
-          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);
-          }
+          this.logger.error("Error querying clickhouse", errorLogFields);
 
           recordClickhouseError(span, clickhouseError);
 
@@ -651,11 +633,7 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
           queryId,
         };
 
-        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);
-        }
+        self.logger.error("Error streaming clickhouse", errorLogFields);
 
         if (error instanceof Error) {
           recordClickhouseError(span, error);
@@ -1053,8 +1031,8 @@ export class ClickhouseClient implements ClickhouseReader, ClickhouseWriter {
 
 /**
  * ClickHouse error types raised by a query that is valid but asks for more than
- * the caller is allowed to spend. The caller gets a 4xx and there is nothing on
- * our side to fix, so these are logged at warn rather than error.
+ * it is allowed to spend. Only downgraded for SQL the caller wrote: a runaway
+ * query we generated is our bug and still has to alert.
  */
 const CLICKHOUSE_QUOTA_ERROR_TYPES = new Set([
   "MEMORY_LIMIT_EXCEEDED",
@@ -1098,13 +1076,13 @@ function classifyClickhouseError(
   error: Error,
   userAuthoredQuery: boolean | undefined
 ): ClickhouseErrorCategory {
-  if (!(error instanceof ClickHouseError) || error.type === undefined) {
+  if (!userAuthoredQuery || !(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)) {
+  if (CLICKHOUSE_INVALID_SQL_ERROR_TYPES.has(error.type)) {
     return "invalid-sql";
   }
   return "fault";

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

@@ -1763,11 +1763,43 @@ describe("TSQL Error Log Levels", () => {
         organization_id: { op: "eq", value: "org_tenant1" },
       },
       tableSchema: [taskRunsSchema],
+      userAuthoredQuery: true,
       clickhouseSettings: { max_rows_to_read: "1" },
     });
 
     expect(error).not.toBeNull();
     expect(logged(warnSpy)).toContain("Query exceeded a ClickHouse limit");
     expect(logged(errorSpy)).not.toContain("Error querying clickhouse");
   });
+
+  clickhouseTest(
+    "keeps a limit breach on a query we generated at error level",
+    async ({ clickhouseContainer }) => {
+      const client = new ClickhouseClient({
+        name: "test",
+        url: clickhouseContainer.getConnectionUrl(),
+      });
+
+      await insertTaskRuns(client, { async_insert: 0 })([
+        createTaskRun({ run_id: "run_intlimit1" }),
+        createTaskRun({ run_id: "run_intlimit2" }),
+        createTaskRun({ run_id: "run_intlimit3" }),
+      ]);
+
+      const [error] = await executeTSQL(client, {
+        name: "test-internal-resource-limit",
+        query: "SELECT run_id FROM task_runs",
+        schema: z.object({ run_id: z.string() }),
+        enforcedWhereClause: {
+          organization_id: { op: "eq", value: "org_tenant1" },
+        },
+        tableSchema: [taskRunsSchema],
+        clickhouseSettings: { max_rows_to_read: "1" },
+      });
+
+      expect(error).not.toBeNull();
+      expect(logged(errorSpy)).toContain("Error querying clickhouse");
+      expect(logged(warnSpy)).not.toContain("Query exceeded a ClickHouse limit");
+    }
+  );
 });