From 18e7ad448623f30d76806a06db35c02af53fd590 Mon Sep 17 00:00:00 2001 From: Florent Collin Date: Fri, 6 Feb 2026 14:14:53 +0000 Subject: [PATCH 1/4] CFSQL-1473: fix D1 exec throwing TypeError on invalid SQL queries We were trying to call `addAggregatedD1MetaToSpan` before checking if any of the results are errors in a `.exec` call. When an error is returned the meta property is not set. This commit is now checking the errors first and then call the `addAggregatedD1MetaToSpan` if there is no error. --- src/cloudflare/internal/d1-api.ts | 9 ++++---- .../test/d1/d1-api-instrumentation-test.js | 23 +++++++++++++++++++ .../internal/test/d1/d1-api-test-common.js | 17 ++++++++++++++ .../internal/test/d1/d1-api-test.js | 11 ++++++++- src/cloudflare/internal/test/d1/d1-mock.js | 12 ++++++++-- 5 files changed, 64 insertions(+), 8 deletions(-) diff --git a/src/cloudflare/internal/d1-api.ts b/src/cloudflare/internal/d1-api.ts index dffbb3eb7cf..67e6a1a5e53 100644 --- a/src/cloudflare/internal/d1-api.ts +++ b/src/cloudflare/internal/d1-api.ts @@ -390,11 +390,6 @@ class D1DatabaseSessionAlwaysPrimary extends D1DatabaseSession { const _exec = await this._send('/execute', lines, [], 'NONE', span); const exec = Array.isArray(_exec) ? _exec : [_exec]; - addAggregatedD1MetaToSpan( - span, - exec.map((e) => e.meta) - ); - const error = exec .map((r) => { return r.error ? 1 : 0; @@ -413,6 +408,10 @@ class D1DatabaseSessionAlwaysPrimary extends D1DatabaseSession { } ); } else { + addAggregatedD1MetaToSpan( + span, + exec.map((e) => e.meta) + ); return { count: exec.length, duration: exec.reduce((p, c) => { diff --git a/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js b/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js index 2f3881437d8..534105310c6 100644 --- a/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js +++ b/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js @@ -31,6 +31,29 @@ export const test = { }; const expectedSpans = [ + // testExecWithInvalidSQL: exec() with invalid SQL should throw a proper + // error, not a TypeError on undefined meta (regression test for #5218). + { + name: 'fetch', + 'network.protocol.name': 'http', + 'network.protocol.version': 'HTTP/1.1', + 'http.request.method': 'POST', + 'url.full': 'http://d1/execute?resultsFormat=NONE', + 'http.request.header.content-type': 'application/json', + 'http.request.body.size': 23n, + 'http.response.status_code': 200n, + 'http.response.body.size': 0n, + closed: true, + }, + { + name: 'd1_exec', + 'db.system.name': 'cloudflare-d1', + 'db.operation.name': 'exec', + 'db.query.text': 'INVALID SQL', + 'cloudflare.binding.type': 'D1', + 'error.type': 'Error in line 1', + closed: true, + }, { name: 'fetch', 'network.protocol.name': 'http', diff --git a/src/cloudflare/internal/test/d1/d1-api-test-common.js b/src/cloudflare/internal/test/d1/d1-api-test-common.js index 5de4ddae9c7..325be91c0f0 100644 --- a/src/cloudflare/internal/test/d1/d1-api-test-common.js +++ b/src/cloudflare/internal/test/d1/d1-api-test-common.js @@ -537,3 +537,20 @@ export async function testD1ApiQueriesHappyPath(DB) { ] ); } + +// Regression test for https://github.com/cloudflare/workerd/pull/5218. +// exec() with invalid SQL should throw a SQL error, not a TypeError from +// accessing properties on undefined meta during span aggregation. +export async function testD1ExecWithInvalidSQL(DB) { + await assert.rejects( + () => DB.exec('INVALID SQL'), + (e) => { + assert.notEqual(e.constructor, TypeError); + assert.ok( + e.message.includes('D1_EXEC_ERROR'), + `Expected D1 error, got: ${e.message}` + ); + return true; + } + ); +} diff --git a/src/cloudflare/internal/test/d1/d1-api-test.js b/src/cloudflare/internal/test/d1/d1-api-test.js index 5428562ff24..e991234646f 100644 --- a/src/cloudflare/internal/test/d1/d1-api-test.js +++ b/src/cloudflare/internal/test/d1/d1-api-test.js @@ -2,10 +2,19 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -import { testD1ApiQueriesHappyPath } from './d1-api-test-common'; +import { + testD1ApiQueriesHappyPath, + testD1ExecWithInvalidSQL, +} from './d1-api-test-common'; export const testWithoutSessions = { async test(_ctr, env) { await testD1ApiQueriesHappyPath(env.d1); }, }; + +export const testExecWithInvalidSQL = { + async test(_ctr, env) { + await testD1ExecWithInvalidSQL(env.d1); + }, +}; diff --git a/src/cloudflare/internal/test/d1/d1-mock.js b/src/cloudflare/internal/test/d1/d1-mock.js index ea51700eecf..3eb8cb8faf6 100644 --- a/src/cloudflare/internal/test/d1/d1-mock.js +++ b/src/cloudflare/internal/test/d1/d1-mock.js @@ -21,10 +21,18 @@ export class D1MockDO { : resultsFormatParam === 'NONE' ? 'NONE' : 'ARRAY_OF_OBJECTS'; + const safeRunQuery = (query) => { + try { + return this.runQuery(query, resultsFormat); + } catch (e) { + // Reproduce the production behavior by catching any error and returning a V4Failure + return { success: false, error: String(e.message) }; + } + }; return Response.json( Array.isArray(body) - ? body.map((query) => this.runQuery(query, resultsFormat)) - : this.runQuery(body, resultsFormat) + ? body.map((query) => safeRunQuery(query)) + : safeRunQuery(body) ); } else { return Response.json({ error: 'Not found' }, { status: 404 }); From 694d5b54ccc54b0404288d73475426e9027020b7 Mon Sep 17 00:00:00 2001 From: Florent Collin Date: Fri, 6 Feb 2026 15:57:16 +0000 Subject: [PATCH 2/4] CFSQL-1473: Add defensive checks in D1 meta aggregation functions --- src/cloudflare/internal/d1-api.ts | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/cloudflare/internal/d1-api.ts b/src/cloudflare/internal/d1-api.ts index 67e6a1a5e53..2c4c07ed3a4 100644 --- a/src/cloudflare/internal/d1-api.ts +++ b/src/cloudflare/internal/d1-api.ts @@ -731,6 +731,10 @@ async function toJson(response: Response): Promise { } function addAggregatedD1MetaToSpan(span: Span, metas: D1Meta[]): void { + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (!metas?.length) { + return; + } const aggregatedMeta = aggregateD1Meta(metas); addD1MetaToSpan(span, aggregatedMeta); } @@ -775,12 +779,22 @@ function aggregateD1Meta(metas: D1Meta[]): D1Meta { }; for (const meta of metas) { - aggregatedMeta.duration += meta.duration; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (!meta) { + continue; + } + + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + aggregatedMeta.duration += meta.duration ?? 0; // for size_after, we only want the last value - aggregatedMeta.size_after = meta.size_after; - aggregatedMeta.rows_read += meta.rows_read; - aggregatedMeta.rows_written += meta.rows_written; - aggregatedMeta.last_row_id = meta.last_row_id; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + aggregatedMeta.size_after = meta.size_after ?? 0; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + aggregatedMeta.rows_read += meta.rows_read ?? 0; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + aggregatedMeta.rows_written += meta.rows_written ?? 0; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + aggregatedMeta.last_row_id = meta.last_row_id ?? 0; if (meta.served_by_region) { aggregatedMeta.served_by_region = meta.served_by_region; } @@ -798,7 +812,8 @@ function aggregateD1Meta(metas: D1Meta[]): D1Meta { aggregatedMeta.total_attempts = (aggregatedMeta.total_attempts ?? 0) + meta.total_attempts; } - aggregatedMeta.changes += meta.changes; + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + aggregatedMeta.changes += meta.changes ?? 0; if (meta.changed_db) { aggregatedMeta.changed_db = true; } From a1577caa37178aa3a7fef8b8ced2991034391b10 Mon Sep 17 00:00:00 2001 From: Florent Collin Date: Mon, 9 Feb 2026 13:12:59 +0000 Subject: [PATCH 3/4] CFSQL-1473: Remove meta from D1UpstreamFailure --- src/cloudflare/internal/d1-api.ts | 80 +++++++++++++++---------------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/src/cloudflare/internal/d1-api.ts b/src/cloudflare/internal/d1-api.ts index 2c4c07ed3a4..1f58f864af6 100644 --- a/src/cloudflare/internal/d1-api.ts +++ b/src/cloudflare/internal/d1-api.ts @@ -60,7 +60,7 @@ type D1UpstreamFailure = { results?: never; error: string; success: false; - meta: D1Meta & Record; + meta?: never; }; type D1RowsColumns = D1Response & { @@ -386,39 +386,42 @@ class D1DatabaseSessionAlwaysPrimary extends D1DatabaseSession { span.setAttribute('db.query.text', query); span.setAttribute('cloudflare.binding.type', 'D1'); + // TODO: splitting by lines is overly simplification because a single line + // can contain multiple statements (ex: `select 1; select 2;`). + // Also, a statement can span multiple lines... + // Either, we should do a more reasonable job to split the query into multiple statements + // like we do in the D1 codebase or we report a simpler error without the line number. const lines = query.trim().split('\n'); const _exec = await this._send('/execute', lines, [], 'NONE', span); const exec = Array.isArray(_exec) ? _exec : [_exec]; - const error = exec - .map((r) => { - return r.error ? 1 : 0; - }) - .indexOf(1); - if (error !== -1) { - span.setAttribute('error.type', `Error in line ${error + 1}`); - throw new Error( - `D1_EXEC_ERROR: Error in line ${error + 1}: ${lines[error]}: ${ - exec[error]?.error - }`, - { - cause: new Error( - `Error in line ${error + 1}: ${lines[error]}: ${exec[error]?.error}` - ), - } - ); - } else { - addAggregatedD1MetaToSpan( - span, - exec.map((e) => e.meta) - ); - return { - count: exec.length, - duration: exec.reduce((p, c) => { - return p + c.meta['duration']; - }, 0), - }; + let duration = 0; + const metas: D1Meta[] = []; + for (let i = 0; i < exec.length; i++) { + const res = exec[i]; + if (!res?.success) { + span.setAttribute('error.type', `Error in line ${i + 1}`); + throw new Error( + `D1_EXEC_ERROR: Error in line ${i + 1}: ${lines[i]} ${res?.error ? `: ${res.error}` : ''}`, + { + cause: new Error( + `Error in line ${i + 1}: ${lines[i]} ${res?.error ? `: ${res.error}` : ''}` + ), + } + ); + } + + duration += res.meta.duration; + metas.push(res.meta); } + + if (metas.length) { + addAggregatedD1MetaToSpan(span, metas); + } + return { + count: exec.length, + duration, + }; }); } @@ -711,12 +714,11 @@ function mapD1Result(result: D1UpstreamResponse): D1UpstreamResponse { return result.error ? { success: false, - meta: result.meta, error: result.error, } : { success: true, - meta: result.meta, + meta: (result as D1UpstreamSuccess).meta, ...('results' in result ? { results: result.results } : {}), }; } @@ -730,9 +732,10 @@ async function toJson(response: Response): Promise { } } -function addAggregatedD1MetaToSpan(span: Span, metas: D1Meta[]): void { - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - if (!metas?.length) { +type PartialD1Meta = Partial | undefined; + +function addAggregatedD1MetaToSpan(span: Span, metas: PartialD1Meta[]): void { + if (!metas.length) { return; } const aggregatedMeta = aggregateD1Meta(metas); @@ -767,7 +770,7 @@ function addD1MetaToSpan(span: Span, meta: D1Meta): void { // When a query is executing multiple statements, and we receive a D1Meta // for each statement, we need to aggregate the meta data before we annotate // the telemetry, with different rules for each field. -function aggregateD1Meta(metas: D1Meta[]): D1Meta { +function aggregateD1Meta(metas: PartialD1Meta[]): D1Meta { const aggregatedMeta: D1Meta = { duration: 0, size_after: 0, @@ -779,21 +782,15 @@ function aggregateD1Meta(metas: D1Meta[]): D1Meta { }; for (const meta of metas) { - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition if (!meta) { continue; } - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition aggregatedMeta.duration += meta.duration ?? 0; // for size_after, we only want the last value - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition aggregatedMeta.size_after = meta.size_after ?? 0; - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition aggregatedMeta.rows_read += meta.rows_read ?? 0; - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition aggregatedMeta.rows_written += meta.rows_written ?? 0; - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition aggregatedMeta.last_row_id = meta.last_row_id ?? 0; if (meta.served_by_region) { aggregatedMeta.served_by_region = meta.served_by_region; @@ -812,7 +809,6 @@ function aggregateD1Meta(metas: D1Meta[]): D1Meta { aggregatedMeta.total_attempts = (aggregatedMeta.total_attempts ?? 0) + meta.total_attempts; } - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition aggregatedMeta.changes += meta.changes ?? 0; if (meta.changed_db) { aggregatedMeta.changed_db = true; From 9f88c747ee5006820bba48f5af41d92da508ed6e Mon Sep 17 00:00:00 2001 From: Florent Collin Date: Tue, 10 Feb 2026 10:16:40 +0000 Subject: [PATCH 4/4] CFSQL-1473: Add simple D1 exec test --- .../test/d1/d1-api-instrumentation-test.js | 29 +++++++++++++++++-- .../internal/test/d1/d1-api-test-common.js | 11 +++++-- .../internal/test/d1/d1-api-test.js | 9 ++---- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js b/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js index 534105310c6..88b43e1cd81 100644 --- a/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js +++ b/src/cloudflare/internal/test/d1/d1-api-instrumentation-test.js @@ -31,8 +31,33 @@ export const test = { }; const expectedSpans = [ - // testExecWithInvalidSQL: exec() with invalid SQL should throw a proper - // error, not a TypeError on undefined meta (regression test for #5218). + // testExec: exec() happy path and error handling (regression test for #5218). + { + name: 'fetch', + 'network.protocol.name': 'http', + 'network.protocol.version': 'HTTP/1.1', + 'http.request.method': 'POST', + 'url.full': 'http://d1/execute?resultsFormat=NONE', + 'http.request.header.content-type': 'application/json', + 'http.request.body.size': 20n, + 'http.response.status_code': 200n, + 'http.response.body.size': 0n, + closed: true, + }, + { + name: 'd1_exec', + 'db.system.name': 'cloudflare-d1', + 'db.operation.name': 'exec', + 'db.query.text': 'select 1', + 'cloudflare.binding.type': 'D1', + 'cloudflare.d1.response.size_after': 4096, + 'cloudflare.d1.response.rows_read': 0, + 'cloudflare.d1.response.rows_written': 0, + 'cloudflare.d1.response.last_row_id': 0, + 'cloudflare.d1.response.changed_db': false, + 'cloudflare.d1.response.changes': 0, + closed: true, + }, { name: 'fetch', 'network.protocol.name': 'http', diff --git a/src/cloudflare/internal/test/d1/d1-api-test-common.js b/src/cloudflare/internal/test/d1/d1-api-test-common.js index 325be91c0f0..49c0b3cd2be 100644 --- a/src/cloudflare/internal/test/d1/d1-api-test-common.js +++ b/src/cloudflare/internal/test/d1/d1-api-test-common.js @@ -539,9 +539,14 @@ export async function testD1ApiQueriesHappyPath(DB) { } // Regression test for https://github.com/cloudflare/workerd/pull/5218. -// exec() with invalid SQL should throw a SQL error, not a TypeError from -// accessing properties on undefined meta during span aggregation. -export async function testD1ExecWithInvalidSQL(DB) { +// exec() with invalid SQL should throw a proper D1 error, not a TypeError +// from accessing properties on undefined meta during span aggregation. +export async function testD1Exec(DB) { + await itShould('run a simple exec', () => DB.exec('select 1'), { + count: 1, + duration: anything, + }); + await assert.rejects( () => DB.exec('INVALID SQL'), (e) => { diff --git a/src/cloudflare/internal/test/d1/d1-api-test.js b/src/cloudflare/internal/test/d1/d1-api-test.js index e991234646f..b721f6db97f 100644 --- a/src/cloudflare/internal/test/d1/d1-api-test.js +++ b/src/cloudflare/internal/test/d1/d1-api-test.js @@ -2,10 +2,7 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -import { - testD1ApiQueriesHappyPath, - testD1ExecWithInvalidSQL, -} from './d1-api-test-common'; +import { testD1ApiQueriesHappyPath, testD1Exec } from './d1-api-test-common'; export const testWithoutSessions = { async test(_ctr, env) { @@ -13,8 +10,8 @@ export const testWithoutSessions = { }, }; -export const testExecWithInvalidSQL = { +export const testExec = { async test(_ctr, env) { - await testD1ExecWithInvalidSQL(env.d1); + await testD1Exec(env.d1); }, };