Skip to content

fix(file-search): seek the backfill cursor instead of rescanning each page - #7956

Merged
waleedlatif1 merged 3 commits into
stagingfrom
fix/file-search-backfill-keyset-seek
Sep 18, 2026
Merged

waleedlatif1 merged 3 commits into
stagingfrom
fix/file-search-backfill-keyset-seek

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

What is happening

workspace-file-search-dispatch fails in prod on every run, in seedBackfillPage, at ~11.2s — the 10s FILE_SEARCH_DISPATCH_STATEMENT_TIMEOUT_MS plus connection overhead. The whole dispatch transaction rolls back, so no page ever commits and the chunk-search backfill (workspace-file-search-chunks-v2, ~94k live workspace files) makes zero progress, retrying hourly forever.

Root cause

The backfill pages through live workspace files ordered by (workspace_id, id). Two separate defects compound:

1. No index supplies that order under that predicate. Prod's plan for the exact failing query:

Limit  (cost=82.18..1422.84 rows=1000 width=81)
  -> Incremental Sort  (cost=82.18..125811.59 rows=93782 width=81)
       Sort Key: workspace_id, id
       Presorted Key: workspace_id
       -> Index Scan using workspace_files_workspace_folder_name_active_unique
            Filter: ((workspace_id > '…') OR ((workspace_id = '…') AND (id > '…')))

Every page sorts the whole remaining set, with a heap fetch per candidate to evaluate context. That alone exceeds the timeout.

2. The keyset is a filter, not an index condition. a > x OR (a = x AND b > y) is not something the planner converts into an index condition — verified by forcing the plan, it stays a Filter even with a perfectly matching index. So each page restarts at the low end of the index and rescans every page before it. Buffer cost measured against cursor offset, with the index present in both cases:

cursor offset OR-form row-wise
0 12 12
20,000 176 12
40,000 340 13
60,000 504 12
80,000 667 11

Linear in the offset versus flat — the O(offset) vs O(page) signature.

The fix

  • workspace_files_workspace_active_keyset_idx on (workspace_id, id), partial on exactly the walk's predicate.
  • Compare the cursor row-wise, (workspace_id, id) > (:ws, :id), which the planner does turn into Index Cond: ROW(workspace_id, id) > ROW(...) — a true seek.

Both halves are required; neither is sufficient. Full 95-page walk on a prod-shaped local fixture (5.5M files, 94k live, warm cache, measured in-session so process startup is excluded):

configuration total
current staging — no index, OR-form 786 ms
index only 267 ms
index + row-wise cursor (this PR) 42 ms

Prod's absolute latency is far worse than the fixture's because it is dominated by cold random heap I/O over a 5.5M-row table; the fixture reproduces the plan pathology and the cost-growth signature, not prod's wall clock. The direct evidence for prod is prod's own EXPLAIN above.

Migration safety

0361 follows the established house pattern for indexing an existing hot table (0351, 0355): leading COMMIT;, lock_timeout = 0, DROP INDEX CONCURRENTLY IF EXISTS then CREATE INDEX CONCURRENTLY IF NOT EXISTS, restore lock_timeout. CONCURRENTLY never blocks writes to workspace_files, and the drop-then-create makes replay recover an interrupted build rather than leaving an INVALID index behind.

Verified: full migration chain applies clean on a fresh database and the index lands indisvalid/indisready; replaying 0361 twice more leaves exactly one valid index; bun run check:migrations reports backward-compatible; drizzle-kit generate is a no-op against the committed snapshot.

Preventing recurrence

  • New integration test walks a multi-page backfill and asserts every live file is seeded exactly once in exactly ceil(files / page size) pages. Confirmed it fails (expected 2 to be 3) when the cursor comparison is mutated.
  • TSDoc on both the index and seedBackfillPage records that the column order, the partial predicate and the row-wise spelling are coupled, and what breaks if any one of them drifts.
  • The index cannot be silently dropped: CI already fails if drizzle-kit generate is not a no-op.

Two pre-existing drifts in the integration fixture are corrected as a side effect, both of which masked this path: workspace_file_search_revision was declared with a composite primary key and was missing five columns the real insert emits, and the backfill cursor was not reset between tests.

Testing

  • dispatcher.integration.ts 7/7 against real PostgreSQL
  • lib/workspace-files/search/ 133/133
  • type-check clean in apps/sim and packages/db; biome clean

… page

The hourly backfill walks every live workspace file by `(workspace_id, id)`,
but no index supplied that order under its predicate, so each page sorted the
whole remaining set and the dispatcher's 10s statement timeout aborted the
transaction before any page committed.

Adds the matching partial index and compares the cursor row-wise. The previous
`workspace_id > :ws OR (workspace_id = :ws AND id > :id)` spelling is only ever
an index filter, never an index condition, so even with the index each page
restarted at the low end and rescanned every page before it.

On a prod-shaped fixture (5.5M files, 94k live) a full 95-page walk goes from
786ms to 42ms; the index alone accounts for 786ms -> 267ms and the row-wise
cursor for the rest.
@vercel

vercel Bot commented Sep 18, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated
docs Ready Ready Preview Sep 18, 2026 3:15am 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 cursor query, index definition, migration metadata, and integration coverage are aligned.

Summary

This PR changes the workspace-file-search backfill from a disjunctive cursor filter to a row-wise keyset seek and adds the matching partial PostgreSQL index.

  • Aligns the cursor predicate, ordering, and index columns on (workspace_id, id).
  • Adds a concurrent, replay-safe migration and corresponding Drizzle schema metadata.
  • Updates the PostgreSQL integration fixture and adds multi-page exactly-once coverage.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Load persisted backfill cursor] --> B{Both cursor values present?}
    B -- No --> C[Start at beginning of partial index]
    B -- Yes --> D[Seek to tuple greater than cursor]
    C --> E[Read one page ordered by workspace_id and id]
    D --> E
    E --> F[Insert missing revision rows]
    F --> G[Persist last tuple as next cursor]
    G --> H{Page shorter than limit?}
    H -- No --> A
    H -- Yes --> I[Mark backfill complete]
Loading

Reviews (1) · Last reviewed commit: "fix(file-search): seek the backfill curs..."

@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 6 files

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Fix all with cubic | Re-trigger cubic

Comment thread packages/db/schema.ts
Comment thread apps/sim/lib/workspace-files/search/dispatcher.integration.ts Outdated
Bound the walk loop by the expected page count so a cursor regression fails on
the first extra page instead of looping; keep the fixture size off a page
multiple explicitly; drop a distinct-count assertion the primary key already
guarantees. Separate the two coupled causes in the TSDoc so neither reads as a
consequence of the other.
Addresses both cubic findings.

Mark the index `.concurrently()` so a schema reconciliation outside the
hand-written migration also builds it without blocking writes, matching how
workflow_execution_logs_workspace_activity_idx is declared. Drizzle now emits
CREATE INDEX CONCURRENTLY itself; the migration keeps its hand-written wrapper
for lock_timeout and replay recovery.

The walk test only proved logical pagination, which the OR spelling also
satisfies. Record the statements the dispatcher issues and EXPLAIN the exact
backfill SELECT, requiring the cursor to appear as a row-wise index condition.
Reverting to the OR spelling now fails on the plan itself, not just on the SQL
text.

That assertion also exposed a third divergence in the fixture: workspace_id was
declared NOT NULL where production has it nullable, which let PostgreSQL drop
the walk's IS NOT NULL clause and then refuse to match the partial index at all.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Both findings were valid and are fixed in 396202b.

P1 — .concurrently() on the schema index. Correct, and it matches the house convention I had missed: workflow_execution_logs_workspace_activity_idx (migration 0355, the same hand-written COMMIT + lock_timeout + CONCURRENTLY wrapper) is declared .concurrently() in schema.ts for exactly this reason. Added it and regenerated the snapshot, which now records concurrently: true; Drizzle emits CREATE INDEX CONCURRENTLY on its own, so a push or a future regeneration is non-blocking. The migration keeps its hand-written wrapper, which .concurrently() alone does not provide — the lock_timeout = 0 and the DROP INDEX CONCURRENTLY IF EXISTS that recovers an INVALID index left by an interrupted build.

Verified both provisioning paths against a fresh database with the CI extension set: drizzle-kit push applies and leaves the index indisvalid, the full migration chain does the same, and drizzle-kit generate is a no-op.

P2 — the test only proved logical pagination. Correct; that was the honest limit of it. The fixture is too small for the planner to choose the index on cost, so instead of scaling the workload the test now records the statements the dispatcher issues, finds the actual backfill SELECT, and EXPLAINs it with enable_seqscan/enable_sort pinned off. What it asserts is the shape only a row-wise cursor can reach — Index Cond: ... ROW(...) on the keyset index.

Confirmed it discriminates: reverting the cursor to workspace_id > :ws OR (workspace_id = :ws AND id > :id) fails on the plan assertion by itself, with the cursor demoted out of the index condition:

Index Cond: (workspace_id IS NOT NULL)

That assertion also surfaced a third divergence in this fixture: workspace_id was declared NOT NULL where production has it nullable. PostgreSQL then discards the walk's workspace_id IS NOT NULL clause as trivially true and can no longer prove the partial index covers the query, so it silently stops using it — the stub was quietly unable to reproduce the very plan under test. Fixed, with a comment recording why it must stay nullable.

@waleedlatif1
waleedlatif1 merged commit 27fde4c into staging Sep 18, 2026
32 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/file-search-backfill-keyset-seek branch September 18, 2026 03:20

This branch was successfully deployed

1 active deployment
Preview 396202be 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