Skip to content

fix(db): cap per-request batch sweep and guard shared files (#13680, #13681) - #13805

Merged
diegosouzapw merged 2 commits into
release/v3.8.51from
fix/13680-batches-sweep-cap-shared-file
Sep 16, 2026
Merged

diegosouzapw merged 2 commits into
release/v3.8.51from
fix/13680-batches-sweep-cap-shared-file

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Closes #13680
Closes #13681

Root cause (short)

Both residual after PR #13684 (merged same day), which fixed different findings (chunked write-lock duration + forward-progress guard) from the same omni-code-sec battery but did not cap chunks or guard shared files.

Fix

  • src/lib/db/batches.ts: added MAX_CHUNKS_PER_REQUEST = 25 (5000 batches/request). sweepLoop now peeks the next chunk once the cap is hit — without deleting or counting anything — and returns hasMore: true if more remain; the caller (the DELETE route) simply calls again. Resumption is natural (no cursor): swept rows are gone, rowid only increases, so the next call's ORDER BY rowid LIMIT ? picks up exactly where the last one left off.
  • Added isFileReferencedByOtherBatch(fileId, excludeBatchIds) — a small SELECT 1 … WHERE (input_file_id = ? OR output_file_id = ? OR error_file_id = ?) AND id NOT IN (…) LIMIT 1 guard (drops the NOT IN clause entirely when the exclude list is empty, since an empty NOT IN () is invalid SQL). Applied before every file soft-delete in deleteCompletedBatches's sweepIds, deleteBatch, and cleanupExpiredBatches's terminal-batch expiry loop.
  • deleteCompletedBatches's return type and the DELETE /v1/batches/delete-completed route response both gained hasMore: boolean (additive field; existing deletedBatches/deletedFiles callers are unaffected).
  • Updated the deleteCompletedBatches JSDoc to document the cap/hasMore contract and to narrow the pre-existing "shared file can be nulled across chunks" caveat to the one race the guard genuinely cannot see (a new batch created between chunk commits, reusing the just-nulled file id) — the guard now covers every case of a concurrently existing sibling batch, regardless of its status or chunk.

Regression tests

  • tests/unit/issue-13680-batches-delete-completed-unbounded-work.test.ts — RED on unfixed code: MAX_CHUNKS_PER_REQUEST did not exist (assertion evaluated to NaN/undefined); GREEN after the fix (2/2 passing), including a resumption test that calls the sweep repeatedly until hasMore is false and verifies every seeded batch is eventually swept.

    RED excerpt:

    AssertionError [ERR_ASSERTION]: expected a single request to sweep at most NaN batches
    (MAX_CHUNKS_PER_REQUEST=undefined × INSTANCE_SWEEP_CHUNK=200), but one call deleted 0 of NaN ...
    

    GREEN excerpt:

    ✔ stops after MAX_CHUNKS_PER_REQUEST chunks and reports hasMore instead of sweeping the whole table in one synchronous call
    ✔ resumes across repeated calls until hasMore is false, sweeping the entire backlog
    ℹ pass 2 / fail 0
    
  • tests/unit/issue-13681-shared-file-across-batches.test.ts — RED on unfixed code (2/2 failing): a file shared by a completed batch and a surviving in_progress sibling was nulled by both deleteCompletedBatches and deleteBatch; GREEN after the fix (3/3 passing), including a case proving the file IS still deleted once the last referencing batch is gone (no regression toward "never delete").

    RED excerpt:

    ✖ deleteCompletedBatches must NOT null a file still referenced by a surviving in_progress batch
      AssertionError: the shared file must survive — batch B still references it (actual: null)
    ✖ deleteBatch (single) must NOT null a file still referenced by a sibling batch
      AssertionError: the shared file's content must survive — batch B still references it
    

    GREEN excerpt:

    ✔ deleteCompletedBatches must NOT null a file still referenced by a surviving in_progress batch
    ✔ deleteBatch (single) must NOT null a file still referenced by a sibling batch
    ✔ deleteCompletedBatches DOES delete the file once the LAST referencing batch is gone
    ℹ pass 3 / fail 0
    

Gates run

  • npx eslint --suppressions-location config/quality/eslint-suppressions.json <changed files> → 0 findings
  • npm run typecheck:core → exit 0
  • node scripts/check/check-file-size.mjs → no ✗ on touched files (the one pre-existing ✗, open-sse/utils/stream.ts, is unrelated/untouched)
  • node scripts/check/check-complexity.mjs and node scripts/check/check-cognitive-complexity.mjs → 0 findings on touched files
  • node scripts/check/check-test-discovery.mjs → OK, both new test files discovered
  • Full focused suite green: batch-deletion.test.ts (8/8), batches-delete-completed-ownership-wvxc.test.ts (15/15), batch-delete-completed-ownership-wvxc.test.ts (5/5), batches-delete-completed-route-scope.test.ts (11/11), batch-deletion-route-logic.test.ts (11/11), plus the two new regression files (2/2, 3/3)

Existing tests aligned

  • tests/unit/batches-delete-completed-route-scope.test.ts: extended the shared response-body type with hasMore?: boolean and added one assertion (body.hasMore === false) to the existing "an inference key sweeps only its own batches" case — additive, no existing assertion weakened or removed.

No other existing assertion needed changing: batch-deletion.test.ts's "shared file IDs across multiple completed batches" test seeds two completed batches (both inside the same sweep chunk), which the new guard does not affect — a file is only preserved when a batch outside the current chunk still references it.

…13681)

deleteCompletedBatches ran an unbounded synchronous for(;;) loop over
INSTANCE_SWEEP_CHUNK-sized chunks, so one request could hold the event
loop for as long as it took to sweep every completed batch on the
instance, with no way for the caller to detect or bound the work.
Separately, the sweep (and deleteBatch) nulled a batch's input/output/
error file unconditionally, even when another batch — in progress, or
completed but outside the swept chunk — still referenced the same file.

Adds MAX_CHUNKS_PER_REQUEST (25 * INSTANCE_SWEEP_CHUNK = 5000 batches)
to sweepLoop with a hasMore continuation flag threaded through the
DELETE /v1/batches/delete-completed response (resumption is natural via
rowid ordering, no cursor needed); and isFileReferencedByOtherBatch(),
applied before every file soft-delete in deleteCompletedBatches,
deleteBatch, and cleanupExpiredBatches.

Regression tests: tests/unit/issue-13680-batches-delete-completed-unbounded-work.test.ts,
tests/unit/issue-13681-shared-file-across-batches.test.ts
@diegosouzapw
diegosouzapw force-pushed the fix/13680-batches-sweep-cap-shared-file branch from f221057 to 0de1660 Compare September 15, 2026 22:56
@diegosouzapw
diegosouzapw merged commit b33c00b into release/v3.8.51 Sep 16, 2026
19 of 21 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…zapw#13680, diegosouzapw#13681) (diegosouzapw#13805)

Merged in the 2026-09-16 sweep of the maintainer's own open PRs, at the owner's explicit instruction. No push was made to the PR branch: the merge took the head as the owning session left it (verified OPEN, non-draft and MERGEABLE against the release tip immediately before merging).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant