From 3bf006da95f5389c1473f2f327f33e6b573576e6 Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:40:28 -0300 Subject: [PATCH 1/5] =?UTF-8?q?feat(db):=20deleteFileOwnedBy=20=E2=80=94?= =?UTF-8?q?=20owner-scoped=20file=20soft=20delete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/lib/db/files.ts | 19 ++++++++ tests/unit/files-delete-owned-by.test.ts | 56 ++++++++++++++++++++++++ 2 files changed, 75 insertions(+) create mode 100644 tests/unit/files-delete-owned-by.test.ts diff --git a/src/lib/db/files.ts b/src/lib/db/files.ts index 4d77cf59e4..64e0b64c3f 100644 --- a/src/lib/db/files.ts +++ b/src/lib/db/files.ts @@ -168,3 +168,22 @@ export function deleteFile(id: string): boolean { .run(Math.floor(Date.now() / 1000), id); return result.changes > 0; } + +/** + * Owner-scoped soft delete: same effect as `deleteFile`, but only when the + * file belongs to `apiKeyId`. Used by the key-scoped completed-batch sweep so + * a batch that references another tenant's (or an unowned) file never nulls + * that file's content (GHSA-wvxc-jp3v-5mg5, SEC-C). Returns false when the + * row is not the caller's; throws on an empty owner so a caller cannot widen + * the delete by passing a blank id. + */ +export function deleteFileOwnedBy(id: string, apiKeyId: string): boolean { + if (typeof apiKeyId !== "string" || apiKeyId.trim() === "") { + throw new Error("deleteFileOwnedBy: apiKeyId is required"); + } + const db = getDbInstance(); + const result = db + .prepare("UPDATE files SET deleted_at = ?, content = NULL WHERE id = ? AND api_key_id = ?") + .run(Math.floor(Date.now() / 1000), id, apiKeyId); + return result.changes > 0; +} diff --git a/tests/unit/files-delete-owned-by.test.ts b/tests/unit/files-delete-owned-by.test.ts new file mode 100644 index 0000000000..42fa52ca9b --- /dev/null +++ b/tests/unit/files-delete-owned-by.test.ts @@ -0,0 +1,56 @@ +// tests/unit/files-delete-owned-by.test.ts +import { describe, it, after } from "node:test"; +import assert from "node:assert"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "files-owned-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const { resetDbInstance } = await import("../../src/lib/db/core.ts"); +const { createFile, getFile, getFileContent, deleteFileOwnedBy } = + await import("../../src/lib/db/files.ts"); + +const seed = (apiKeyId: string | null, label: string) => + createFile({ + bytes: label.length, + filename: `${label}.jsonl`, + purpose: "batch", + content: Buffer.from(label), + apiKeyId, + }); + +describe("deleteFileOwnedBy — owner-scoped soft delete", () => { + after(() => { + resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + }); + + it("soft-deletes the owner's own file and nulls its content", () => { + const own = seed("key-A", "own"); + assert.strictEqual(deleteFileOwnedBy(own.id, "key-A"), true); + assert.strictEqual(getFile(own.id), null, "metadata read hides a soft-deleted file"); + assert.strictEqual(getFileContent(own.id), null); + }); + + it("returns false and leaves another key's file intact", () => { + const other = seed("key-B", "other"); + assert.strictEqual(deleteFileOwnedBy(other.id, "key-A"), false); + assert.ok(getFile(other.id)); + assert.strictEqual(getFileContent(other.id)?.toString(), "other"); + }); + + it("returns false for an unowned file (api_key_id NULL) — only the instance sweep reaches those", () => { + const unowned = seed(null, "unowned"); + assert.strictEqual(deleteFileOwnedBy(unowned.id, "key-A"), false); + assert.strictEqual(getFileContent(unowned.id)?.toString(), "unowned"); + }); + + it("throws on an empty apiKeyId instead of widening", () => { + const own = seed("key-A", "guard"); + assert.throws(() => deleteFileOwnedBy(own.id, ""), /apiKeyId/); + assert.throws(() => deleteFileOwnedBy(own.id, undefined as unknown as string), /apiKeyId/); + assert.strictEqual(getFileContent(own.id)?.toString(), "guard"); + }); +}); From 46b24d980d38aeb8b6c846e62858765ebb0adb80 Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:54:28 -0300 Subject: [PATCH 2/5] fix(db): key-scoped batch sweep soft-deletes only the key's own files (SEC-C) Refs #12969 --- src/lib/db/batches.ts | 12 ++++- ...es-delete-completed-ownership-wvxc.test.ts | 44 +++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/lib/db/batches.ts b/src/lib/db/batches.ts index 4c24e408e0..bb70071426 100644 --- a/src/lib/db/batches.ts +++ b/src/lib/db/batches.ts @@ -1,5 +1,5 @@ import { getDbInstance, rowToCamel, objToSnake } from "./core"; -import { deleteFile } from "./files"; +import { deleteFile, deleteFileOwnedBy } from "./files"; import { v4 as uuidv4 } from "uuid"; import { logger } from "../../../open-sse/utils/logger.ts"; @@ -439,6 +439,10 @@ export type DeleteCompletedBatchesScope = { apiKeyId: string } | { allTenants: t * sweep must never reach records the key does not own, so unowned batches are * only swept by `{ allTenants: true }`. * + * In key mode the file half is owner-scoped too: only files whose api_key_id is + * the caller's are soft-deleted; a referenced file another tenant owns (or an + * unowned one) is left intact and is not counted in deletedFiles. + * * The file soft-deletes, the checkpoint DELETE and the batches DELETE run in one * transaction, so a mid-sweep failure rolls everything back — no batch row is * left pointing at a file whose content was already nulled. @@ -484,7 +488,11 @@ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { let deletedFiles = 0; for (const fid of fileIds) { try { - if (deleteFile(fid)) deletedFiles++; + // Key mode: only the key's OWN files. A batch may reference a file + // another tenant (or nobody) owns; a bulk destructive sweep must not + // reach it (SEC-C). Instance mode keeps the unconditional soft delete. + const removed = allTenants ? deleteFile(fid) : deleteFileOwnedBy(fid, apiKeyId as string); + if (removed) deletedFiles++; } catch (err) { log.warn("deleteCompletedBatches: file soft-delete failed", { fid, diff --git a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts index cc562e5f19..8701342db1 100644 --- a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts +++ b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts @@ -189,6 +189,50 @@ describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)", assert.strictEqual(getFile(unowned.file.id), null, "allTenants soft-deletes its file too"); }); + it("SEC-C: a key-scoped sweep never nulls a file another key owns, even when its own batch references it", () => { + const foreignFile = createFile({ + bytes: 7, + filename: "foreign.jsonl", + purpose: "batch", + content: Buffer.from("foreign"), + apiKeyId: "key-other", + }); + const unownedFile = createFile({ + bytes: 7, + filename: "unowned.jsonl", + purpose: "batch", + content: Buffer.from("unowned"), + apiKeyId: null, + }); + const own = seedCompletedBatch("key-secc", "secc-own"); + const cross = createBatch({ + endpoint: "/v1/chat/completions", + completionWindow: "24h", + inputFileId: foreignFile.id, + outputFileId: unownedFile.id, + status: "completed", + apiKeyId: "key-secc", + }); + + const result = deleteCompletedBatches({ apiKeyId: "key-secc" }); + + assert.strictEqual(result.deletedBatches, 2, "both of the key's completed batches are swept"); + assert.strictEqual(result.deletedFiles, 1, "only the key's OWN file is soft-deleted"); + assert.strictEqual(getBatch(own.batch.id), null); + assert.strictEqual(getBatch(cross.id), null); + assert.strictEqual(getFileContent(own.file.id), null, "own file content nulled"); + assert.strictEqual( + getFileContent(foreignFile.id)?.toString(), + "foreign", + "another key's file intact" + ); + assert.strictEqual( + getFileContent(unownedFile.id)?.toString(), + "unowned", + "unowned file intact" + ); + }); + it("ATOMIC: a failure after the file soft-deletes rolls the file content back", () => { const db = getDbInstance(); const own = seedCompletedBatch("key_atomic_wvxc", "wvxc-atomic"); From 3fc22c69af797dede078f88d113d937ad0314dfc Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:07:43 -0300 Subject: [PATCH 3/5] perf(db): instance-wide completed-batch sweep commits in 200-batch chunks (SEC-D) Refs #12969 --- src/lib/db/batches.ts | 68 ++++++++++++++----- ...es-delete-completed-ownership-wvxc.test.ts | 58 +++++++++++++++- 2 files changed, 107 insertions(+), 19 deletions(-) diff --git a/src/lib/db/batches.ts b/src/lib/db/batches.ts index bb70071426..bcdf31ed1a 100644 --- a/src/lib/db/batches.ts +++ b/src/lib/db/batches.ts @@ -443,10 +443,23 @@ export type DeleteCompletedBatchesScope = { apiKeyId: string } | { allTenants: t * the caller's are soft-deleted; a referenced file another tenant owns (or an * unowned one) is left intact and is not counted in deletedFiles. * - * The file soft-deletes, the checkpoint DELETE and the batches DELETE run in one - * transaction, so a mid-sweep failure rolls everything back — no batch row is - * left pointing at a file whose content was already nulled. + * 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. + * + * 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. */ +/** Instance-wide sweeps commit in chunks of this many batches (SEC-D). */ +export const INSTANCE_SWEEP_CHUNK = 200; + export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { deletedBatches: number; deletedFiles: number; @@ -463,16 +476,18 @@ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { const db = getDbInstance(); - const ownershipClause = allTenants ? "" : " AND api_key_id = ?"; - const ownershipArgs = allTenants ? [] : [apiKeyId]; - - const sweep = db.transaction(() => { - // Collect unique file IDs from the completed batches in scope + // 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. + const sweepIds = db.transaction((ids: string[]) => { + if (ids.length === 0) return { deletedBatches: 0, deletedFiles: 0 }; + const marks = ids.map(() => "?").join(","); const rows = db .prepare( - `SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE status = 'completed'${ownershipClause}` + `SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE id IN (${marks})` ) - .all(...ownershipArgs) as Array<{ + .all(...ids) as Array<{ input_file_id: string | null; output_file_id: string | null; error_file_id: string | null; @@ -501,15 +516,32 @@ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { } } - db.prepare( - `DELETE FROM batch_item_checkpoints WHERE batch_id IN (SELECT id FROM batches WHERE status = 'completed'${ownershipClause})` - ).run(...ownershipArgs); - - const result = db - .prepare(`DELETE FROM batches WHERE status = 'completed'${ownershipClause}`) - .run(...ownershipArgs); + db.prepare(`DELETE FROM batch_item_checkpoints WHERE batch_id IN (${marks})`).run(...ids); + const result = db.prepare(`DELETE FROM batches WHERE id IN (${marks})`).run(...ids); return { deletedBatches: result.changes, deletedFiles }; }); - return sweep(); + 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); + } + + const totals = { deletedBatches: 0, deletedFiles: 0 }; + const nextChunk = db.prepare( + "SELECT id FROM batches WHERE status = 'completed' ORDER BY rowid LIMIT ?" + ); + for (;;) { + const ids = (nextChunk.all(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; } diff --git a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts index 8701342db1..3849638c08 100644 --- a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts +++ b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts @@ -37,7 +37,7 @@ process.env.DATA_DIR = TEST_DATA_DIR; process.env.APP_LOG_LEVEL = "warn"; const { createFile, getFile, getFileContent } = await import("../../src/lib/db/files.ts"); -const { createBatch, getBatch, deleteCompletedBatches } = +const { createBatch, getBatch, deleteCompletedBatches, INSTANCE_SWEEP_CHUNK } = await import("../../src/lib/db/batches.ts"); const { getDbInstance, resetDbInstance } = await import("../../src/lib/db/core.ts"); @@ -296,4 +296,60 @@ describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)", assert.ok(logged, "the failure is logged at warn level"); assert.ok(logged!.includes(own.file.id), "the log line names the file id"); }); + it("SEC-D: the instance sweep runs in chunks of INSTANCE_SWEEP_CHUNK and still deletes everything", () => { + const total = INSTANCE_SWEEP_CHUNK * 2 + 50; // 3 chunks: 200 + 200 + 50 + const ids: string[] = []; + for (let i = 0; i < total; i++) + ids.push(seedCompletedBatch(i % 2 ? "key-chunk-a" : null, `chunk-${i}`).batch.id); + // `db.transaction(fn)` is called ONCE to build the unit; what must happen per + // chunk is the INVOCATION of the unit — count those. + 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({ allTenants: true }); + } finally { + txSpy.mock.restore(); + } + + assert.strictEqual(result.deletedBatches, total); + assert.strictEqual(result.deletedFiles, total); + assert.strictEqual(runs, 3, "one transaction per chunk (200 + 200 + 50)"); + for (const id of ids) assert.strictEqual(getBatch(id), null); + }); + + it("SEC-D: a failure in chunk 2 keeps chunk 1 done and rolls chunk 2 back entirely", () => { + const first = Array.from({ length: INSTANCE_SWEEP_CHUNK }, (_, i) => + seedCompletedBatch(null, `c1-${i}`) + ); + const second = Array.from({ length: 10 }, (_, i) => seedCompletedBatch(null, `c2-${i}`)); + const poison = second[5].batch.id; + const db = getDbInstance(); + db.exec( + `CREATE TRIGGER wvxc_chunk_poison BEFORE DELETE ON batches WHEN OLD.id = '${poison}' BEGIN SELECT RAISE(ABORT, 'poison'); END` + ); + try { + assert.throws(() => deleteCompletedBatches({ allTenants: true }), /poison/); + } finally { + db.exec("DROP TRIGGER IF EXISTS wvxc_chunk_poison"); + } + for (const s of first) assert.strictEqual(getBatch(s.batch.id), null, "chunk 1 committed"); + for (const s of second) { + assert.ok(getBatch(s.batch.id), "chunk 2 rolled back as a unit"); + assert.strictEqual( + getFileContent(s.file.id)?.toString(), + s.file.filename.replace(".jsonl", ""), + "chunk 2 file content restored" + ); + } + }); }); From f23a759c20a0bcdabd8cb561e511c25a22597360 Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 11 Sep 2026 19:00:23 -0300 Subject: [PATCH 4/5] test(api): session fixture mints the login shape; changelog covers SEC-C/SEC-D Refs #12969 --- .../fixes/12969-batches-delete-completed-ownership.md | 2 +- src/lib/db/batches.ts | 6 +++--- tests/unit/batches-delete-completed-route-scope.test.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/changelog.d/fixes/12969-batches-delete-completed-ownership.md b/changelog.d/fixes/12969-batches-delete-completed-ownership.md index da4081d555..e602446524 100644 --- a/changelog.d/fixes/12969-batches-delete-completed-ownership.md +++ b/changelog.d/fixes/12969-batches-delete-completed-ownership.md @@ -1 +1 @@ -- **fix(api):** `DELETE /v1/batches/delete-completed` now sweeps only the calling API key's own completed batches (batches with no owner stay out of a key-scoped sweep on purpose), with an explicit instance-wide mode reserved for authenticated dashboard sessions, a 401 for a presented key that is unknown, revoked, deactivated, banned or expired (never falling through to the session branch), audit logging of every sweep, a sanitized 500 on failure and an atomic sweep so a mid-way error never leaves a batch pointing at a nulled file (GHSA-wvxc-jp3v-5mg5) ([#12969](https://github.com/diegosouzapw/OmniRoute/pull/12969)) +- **fix(api):** `DELETE /v1/batches/delete-completed` now sweeps only the calling API key's own completed batches (batches with no owner stay out of a key-scoped sweep on purpose), with an explicit instance-wide mode reserved for authenticated dashboard sessions, a 401 for a presented key that is unknown, revoked, deactivated, banned or expired (never falling through to the session branch), audit logging of every sweep, a sanitized 500 on failure, an owner-scoped file half (a key-scoped sweep never nulls a file another tenant owns) and an atomic sweep — chunked in 200-batch transactions in instance mode — so a mid-way error never leaves a batch pointing at a nulled file (GHSA-wvxc-jp3v-5mg5) ([#12969](https://github.com/diegosouzapw/OmniRoute/pull/12969)) diff --git a/src/lib/db/batches.ts b/src/lib/db/batches.ts index bcdf31ed1a..b981c511d0 100644 --- a/src/lib/db/batches.ts +++ b/src/lib/db/batches.ts @@ -421,6 +421,9 @@ export function deleteBatch(id: string): boolean { */ export type DeleteCompletedBatchesScope = { apiKeyId: string } | { allTenants: true }; +/** Instance-wide sweeps commit in chunks of this many batches (SEC-D). */ +export const INSTANCE_SWEEP_CHUNK = 200; + /** * Delete completed batches and the files they reference. * @@ -457,9 +460,6 @@ export type DeleteCompletedBatchesScope = { apiKeyId: string } | { allTenants: t * 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. */ -/** Instance-wide sweeps commit in chunks of this many batches (SEC-D). */ -export const INSTANCE_SWEEP_CHUNK = 200; - export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { deletedBatches: number; deletedFiles: number; diff --git a/tests/unit/batches-delete-completed-route-scope.test.ts b/tests/unit/batches-delete-completed-route-scope.test.ts index 5fdf9c7755..421db2064d 100644 --- a/tests/unit/batches-delete-completed-route-scope.test.ts +++ b/tests/unit/batches-delete-completed-route-scope.test.ts @@ -49,7 +49,7 @@ const ROUTE_URL = "http://localhost/api/v1/batches/delete-completed"; async function sessionCookie(): Promise { const secret = new TextEncoder().encode(process.env.JWT_SECRET); - const jwt = await new SignJWT({ sub: "admin" }) + const jwt = await new SignJWT({ authenticated: true, sub: "admin" }) .setProtectedHeader({ alg: "HS256" }) .setExpirationTime("1h") .sign(secret); 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 5/5] 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", "") + ); + } + }); });