Skip to content

fix(file-search): stop cleanup starting a batch it cannot fund - #7965

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/file-search-cleanup-budget
Sep 18, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/file-search-cleanup-budget

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

What

cleanupFileSearchBuilds() drains expired search builds in batches under a 5s wall-clock budget, passing whatever budget remains as each batch's statement_timeout. The loop admitted another batch whenever any budget remained:

for (let batch = 0; batch < FILE_SEARCH_CLEANUP_MAX_BATCHES && Date.now() < deadline; batch++) {
  await configureFileSearchTransaction(tx, { statementTimeout: Math.max(1, deadline - Date.now()) })

So a batch could start with as little as 1ms of budget, and Math.max(1, …) then handed that 1ms through as its statement timeout. Neither outcome is right: the batch either runs past the budget it was given, or aborts on its own statement timeout — and the call site can only report that as a cleanup failure rather than as work still to do.

This stops once less than one batch's nominal share of the budget remains.

Why the floor is derived, not a literal

FILE_SEARCH_CLEANUP_MIN_BATCH_MS is exactly FILE_SEARCH_CLEANUP_BUDGET_MS / FILE_SEARCH_CLEANUP_MAX_BATCHES — one batch's fair share. Writing it as 500 would keep that value if either input were retuned (say MAX_BATCHES raised to drain faster), silently re-admitting batches at more than their share and reintroducing exactly what this removes.

Scope

The guard makes this loop match the shape the outbox drain already uses (lib/core/outbox/service.ts:433, minRemainingMs). A sweep of every other BUDGET_MS/deadline loop — embeddings retry, connector deletion, outbox — found none with the unguarded shape, so nothing else is left owing the same fix.

Deliberately unchanged:

  • The .catch() at the call site. Cleanup is opportunistic work at the head of a dispatch run and must not block dispatch. It was only implicated because the loop was manufacturing spurious failures for it to log.
  • The return contract. Sustained cleanup backpressure is already measured from table state via cleanupBacklogged; a richer return type would add a weaker second source of truth.

Honest scoping of the bug

This was found by inspection while investigating an unrelated production incident, and is confirmed by test — not by production telemetry. The cleanup deferred log lines that first drew attention to this function turned out to be fully explained by a PlanetScale-side stall (pool acquisition reaching 48.9s), not by this code. This fix is worth making on its merits; it is not the cause of that incident and does not claim to be.

Testing

New integration test drives the real loop against PostgreSQL with the clock reporting a budget all but consumed, and asserts cleanup returns 0 and deletes nothing rather than starting a batch. Verified it fails when the guard is reverted (expected 1000 to be +0 — the old code ran a full 1000-row delete after its budget was spent).

  • chunks.integration.ts + dispatcher.integration.ts: 47 passed, 1 skipped
  • lib/workspace-files/search/: 134 passed
  • bun run lint:check: 26/26 tasks pass; type-check clean

The batch loop admitted another batch whenever any budget remained, then passed
that remainder through as the batch's statement timeout, clamped to 1ms. A batch
admitted with a sliver left either runs past the budget it was given or aborts on
its own statement timeout, which the caller reports as a cleanup failure rather
than as work still to do.

Stop once less than one batch's nominal share of the budget remains. The floor is
derived from the budget and the batch cap rather than fixed, so retuning either
cannot leave it admitting batches at more than their share again.
@vercel

vercel Bot commented Sep 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Sep 18, 2026 7:04am UTC

Request Review

@greptile-apps

greptile-apps Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the current implementation prevents underfunded cleanup batches without changing the public return contract or deletion semantics.

Summary

This PR prevents expired file-search cleanup from starting a batch unless at least one batch’s nominal share of the wall-clock budget remains.

  • Derives the minimum viable batch budget from the total budget and maximum batch count.
  • Checks the remaining budget both before acquiring a connection and again inside the transaction.
  • Adds PostgreSQL integration coverage for budget exhaustion before admission and during connection acquisition.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Start cleanup run] --> B{At least one batch share remains?}
    B -- No --> F[Return deleted count]
    B -- Yes --> C[Acquire connection and begin transaction]
    C --> D{At least one batch share still remains?}
    D -- No --> F
    D -- Yes --> E[Configure timeout and delete expired-build batch]
    E --> B
Loading

Reviews (2) · Last reviewed commit: "fix(file-search): measure the cleanup bu..."

Comment thread apps/sim/lib/workspace-files/search/index-state.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/workspace-files/search/index-state.ts Outdated
…held

Hoisting the remaining-budget read out of the transaction callback made it
describe the moment the batch was admitted rather than the moment its statements
begin. Time spent waiting for a pooled connection then went unaccounted, and the
batch installed a timeout larger than the budget actually left.

Keep the cheap check before opening a transaction, and re-read once the
connection is in hand so the installed timeout is the budget that remains.
@waleedlatif1

waleedlatif1 commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator Author

Both findings are the same issue and both are correct — fixed in 5a00005.

This was a regression I introduced. The original code computed the statement timeout inside the transaction callback (Math.max(1, deadline - Date.now())), so it already accounted for time spent acquiring a pooled connection. Hoisting it out to reuse the value for the new guard is what made it stale. The clamp was the defect on that line; its placement was not, and I moved the wrong thing.

It matters more than the general case suggests: under pool saturation, connection acquisition can take longer than the entire cleanup budget, so an admitted batch would install a timeout describing a budget that was already gone before its first statement ran.

The fix keeps the cheap pre-transaction check, so the common path still avoids taking a connection at all, and re-reads once the connection is held:

if (deadline - Date.now() < FILE_SEARCH_CLEANUP_MIN_BATCH_MS) break
const result = await db.transaction(async (tx) => {
  /** Re-read: acquiring the connection can itself have spent the rest of the budget. */
  const remainingBudget = deadline - Date.now()
  if (remainingBudget < FILE_SEARCH_CLEANUP_MIN_BATCH_MS) return null
  await configureFileSearchTransaction(tx, { statementTimeout: remainingBudget })

Returning null reuses the loop's existing "nothing more to do" exit, so budget exhaustion and an empty build page end the run the same way.

Added a second integration test covering the path the outer check cannot see — full budget at admission, none left once the connection is in hand. Verified it fails when only the inner guard is reverted (expected 1000 to be +0), so it pins this regression specifically rather than restating the first test.

chunks.integration.ts + dispatcher.integration.ts: 48 passed, 1 skipped. lib/workspace-files/search/: 134 passed. Typecheck and biome clean.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1
waleedlatif1 merged commit 87625de into staging Sep 18, 2026
34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/file-search-cleanup-budget branch September 18, 2026 07:13

This branch was previously deployed

1 inactive deployment
Preview 5a000059 Deployed Sep 18, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant