From 92ca71c1d6d455152a528be399527feb9989f7af Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:20:34 -0300 Subject: [PATCH] fix(db): key-scoped completed-batch sweep never binds more than INSTANCE_SWEEP_CHUNK ids per statement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The B3 restructure collected ALL of a key's completed batch ids into one IN (…) list; past SQLite's 32766 bound-parameter ceiling the sweep would throw and the tenant's only bulk-cleanup path would stay dead. Key mode now runs the same 200-id unit inside ONE outer transaction (nested calls are savepoints), so it stays all-or-nothing and never exceeds the ceiling. Also: whitespace-only apiKeyId rejected at the top; JSDoc qualifies the nulled-file guarantee as per-chunk in instance mode. Refs #12969 --- src/lib/db/batches.ts | 66 ++++++++++++------- ...es-delete-completed-ownership-wvxc.test.ts | 52 +++++++++++++++ 2 files changed, 96 insertions(+), 22 deletions(-) diff --git a/src/lib/db/batches.ts b/src/lib/db/batches.ts index b981c511d0..0f03cb47ce 100644 --- a/src/lib/db/batches.ts +++ b/src/lib/db/batches.ts @@ -448,17 +448,24 @@ export const INSTANCE_SWEEP_CHUNK = 200; * * The file soft-deletes, the checkpoint DELETE and the batches DELETE for a set * of batch ids run in one transaction, so a mid-sweep failure rolls that set back - * — no batch row is left pointing at a file whose content was already nulled. - * Key mode runs that unit once over every completed batch the key owns. - * Instance mode (`allTenants`) runs it per chunk of `INSTANCE_SWEEP_CHUNK` ids - * (SEC-D): a large sweep never holds one write lock over the whole table, each - * chunk stays atomic, and a failure inside chunk N leaves chunks < N committed, - * chunk N fully rolled back, and rethrows. The returned totals sum the chunks. + * — within a chunk, no batch row is left pointing at a file whose content was + * already nulled. Both modes walk the key's/instance's completed batches in + * chunks of `INSTANCE_SWEEP_CHUNK` ids and run that unit once per chunk. + * Key mode wraps the whole chunk loop in ONE outer transaction (a nested + * transaction call is a savepoint on every adapter), so the key sweep stays + * all-or-nothing: a failure in any chunk rolls every earlier chunk back too. + * Instance mode (`allTenants`) commits per chunk (SEC-D): a large sweep never + * holds one write lock over the whole table, each chunk stays atomic, and a + * failure inside chunk N leaves chunks < N committed, chunk N fully rolled + * back, and rethrows. Inherent to per-chunk commits: a file shared by batches + * in two different chunks can be nulled by chunk 1 before chunk 2 fails; the + * surviving batch row is swept by the next run. The returned totals sum the + * chunks. * - * The ids of a unit are bound as `IN (?, …)` placeholders. A chunk is far below - * SQLite's default SQLITE_MAX_VARIABLE_NUMBER (32766 since 3.32); the key-mode - * list is bounded by that key's completed batches — should a single key ever - * own more than ~32k completed batches, chunk key mode the same way. + * The ids of a unit are bound as `IN (?, …)` placeholders. No statement ever + * binds more than `INSTANCE_SWEEP_CHUNK` ids in either mode, so a tenant with + * tens of thousands of completed batches never hits SQLite's default + * SQLITE_MAX_VARIABLE_NUMBER (32766 since 3.32). */ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { deletedBatches: number; @@ -467,7 +474,7 @@ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { const scopeObj = scope && typeof scope === "object" ? scope : {}; const allTenants = "allTenants" in scopeObj && scopeObj.allTenants === true; const apiKeyId = "apiKeyId" in scopeObj ? scopeObj.apiKeyId : undefined; - if (!allTenants && !apiKeyId) { + if (!allTenants && (typeof apiKeyId !== "string" || apiKeyId.trim() === "")) { throw new Error("deleteCompletedBatches: apiKeyId required unless allTenants"); } if (allTenants && apiKeyId) { @@ -477,9 +484,10 @@ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { const db = getDbInstance(); // One consistent unit: file soft-deletes → checkpoints → batch rows for a - // given set of batch ids. Key mode runs it once over every completed batch - // the key owns; instance mode runs it per chunk so a large sweep never holds - // one write-lock for the whole table (SEC-D) while each chunk stays atomic. + // given set of batch ids. Both modes run it per chunk of INSTANCE_SWEEP_CHUNK + // ids; key mode nests the chunks in one outer transaction, instance mode + // commits each chunk so a large sweep never holds one write-lock for the + // whole table (SEC-D) while each chunk stays atomic. const sweepIds = db.transaction((ids: string[]) => { if (ids.length === 0) return { deletedBatches: 0, deletedFiles: 0 }; const marks = ids.map(() => "?").join(","); @@ -522,14 +530,28 @@ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { }); if (!allTenants) { - const ids = ( - db - .prepare( - "SELECT id FROM batches WHERE status = 'completed' AND api_key_id = ? ORDER BY rowid" - ) - .all(apiKeyId) as Array<{ id: string }> - ).map((r) => r.id); - return sweepIds(ids); + // One outer transaction so the key sweep stays all-or-nothing; inside it, + // the same 200-id unit as instance mode (nested transaction calls become + // savepoints on every adapter), so no statement ever binds more than + // INSTANCE_SWEEP_CHUNK ids — a tenant with tens of thousands of completed + // batches must not hit SQLite's 32766 bound-parameter ceiling. + const keyChunk = db.prepare( + "SELECT id FROM batches WHERE status = 'completed' AND api_key_id = ? ORDER BY rowid LIMIT ?" + ); + const sweepKey = db.transaction(() => { + const totals = { deletedBatches: 0, deletedFiles: 0 }; + for (;;) { + const ids = (keyChunk.all(apiKeyId, INSTANCE_SWEEP_CHUNK) as Array<{ id: string }>).map( + (r) => r.id + ); + if (ids.length === 0) break; + const part = sweepIds(ids); + totals.deletedBatches += part.deletedBatches; + totals.deletedFiles += part.deletedFiles; + } + return totals; + }); + return sweepKey(); } const totals = { deletedBatches: 0, deletedFiles: 0 }; diff --git a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts index 3849638c08..9c12fd59eb 100644 --- a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts +++ b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts @@ -334,6 +334,7 @@ describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)", const second = Array.from({ length: 10 }, (_, i) => seedCompletedBatch(null, `c2-${i}`)); const poison = second[5].batch.id; const db = getDbInstance(); + // DDL cannot take bound parameters in SQLite; the value is createBatch's generated id. db.exec( `CREATE TRIGGER wvxc_chunk_poison BEFORE DELETE ON batches WHEN OLD.id = '${poison}' BEGIN SELECT RAISE(ABORT, 'poison'); END` ); @@ -352,4 +353,55 @@ describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)", ); } }); + + it("SEC-D: a key with more than INSTANCE_SWEEP_CHUNK completed batches is swept in one call, atomically", () => { + const total = INSTANCE_SWEEP_CHUNK + 1; + const own = Array.from({ length: total }, (_, i) => seedCompletedBatch("key-big", `big-${i}`)); + const other = seedCompletedBatch("key-small", "big-other"); + const db = getDbInstance(); + let runs = 0; + const origTx = db.transaction.bind(db); + const txSpy = mock.method(db, "transaction", (fn: (...a: unknown[]) => unknown) => { + const tx = origTx(fn); + return (...args: unknown[]) => { + runs++; + return tx(...args); + }; + }); + let result: ReturnType; + try { + result = deleteCompletedBatches({ apiKeyId: "key-big" }); + } finally { + txSpy.mock.restore(); + } + assert.strictEqual(result.deletedBatches, total); + assert.strictEqual(result.deletedFiles, total); + assert.ok(runs >= 3, `outer transaction + 2 chunk units expected, got ${runs}`); + for (const s of own) assert.strictEqual(getBatch(s.batch.id), null); + assert.ok(getBatch(other.batch.id), "another key's batch survives"); + }); + + it("SEC-D: a failure in the key sweep's second chunk rolls the WHOLE key sweep back (single atomic transaction)", () => { + const own = Array.from({ length: INSTANCE_SWEEP_CHUNK + 5 }, (_, i) => + seedCompletedBatch("key-atomic", `atomic-${i}`) + ); + const poison = own[INSTANCE_SWEEP_CHUNK + 2].batch.id; + const db = getDbInstance(); + // DDL cannot take bound parameters in SQLite; the value is createBatch's generated id. + db.exec( + `CREATE TRIGGER wvxc_key_poison BEFORE DELETE ON batches WHEN OLD.id = '${poison}' BEGIN SELECT RAISE(ABORT, 'key poison'); END` + ); + try { + assert.throws(() => deleteCompletedBatches({ apiKeyId: "key-atomic" }), /key poison/); + } finally { + db.exec("DROP TRIGGER IF EXISTS wvxc_key_poison"); + } + for (const s of own) { + assert.ok(getBatch(s.batch.id), "key sweep is all-or-nothing: chunk 1 rolled back too"); + assert.strictEqual( + getFileContent(s.file.id)?.toString(), + s.file.filename.replace(".jsonl", "") + ); + } + }); });