fix(db): owner-scoped file half and chunked instance sweep for DELETE /v1/batches/delete-completed (SEC-C/SEC-D, omni-code-sec on #12969)

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))

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";
 
@@ -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.
  *
@@ -439,9 +442,30 @@ 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 }`.
  *
- * 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.
+ * 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 for a set
+ * of batch ids run in one transaction, so a mid-sweep failure rolls that set back
+ * — 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. 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;
@@ -450,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) {
@@ -459,16 +483,19 @@ 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. 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(",");
     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;
@@ -484,7 +511,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,
@@ -493,15 +524,46 @@ 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) {
+    // 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 };
+  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;
 }

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;
+}

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");
 
@@ -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");
@@ -252,4 +296,112 @@ 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<typeof deleteCompletedBatches>;
+    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();
+    // 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`
+    );
+    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"
+      );
+    }
+  });
+
+  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<typeof deleteCompletedBatches>;
+    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", "")
+      );
+    }
+  });
 });

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<string> {
   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);