fix(indexer): resume interrupted indexing runs from incremental checkpoints

[Nicolas-nwb]

Context

When an indexing run was interrupted (crash, kill, shutdown), the next run re-embedded the whole project: artifacts (vectors, BM25, database) were only persisted at the end. On large repositories, an interruption after 90% of the work cost a full re-run.

Changes

Incremental checkpoints (src/indexer/index.ts): during indexing, each chunk threshold (checkpointIntervalChunks, floored at totalChunks / 10) now persists the database, vectors, BM25, failed batches, and file-hash cache. An interrupted run resumes where it stopped: already-checkpointed files are skipped, only the remaining ones are re-embedded.

Interrupted lease recovery:

• Global: an interrupted clear is completed on recovery, scoped to the current project when foreign data is present (instead of wiping other projects); an interrupted force-index replays only the indexing phase.
• Project: recovery preserves checkpointed artifacts for incremental resume; the local reset is kept only for clear operations and force-index runs with a clearing-phase marker.

Write consistency:

• BM25 is persisted before the vector store at checkpoints: a crash between the two writes can no longer orphan chunks from keyword search (the resume re-embeds and repopulates BM25).
• Pending retries are persisted at checkpoints (no more lost retries on crash), and resolved retries are excluded from the final failed-batches.json.
• Pending retry chunks are no longer duplicated at every checkpoint, and finalization deduplicates failed-batch records by chunk ID, keeping the highest attempt count.
• Out-of-scope failed batches survive checkpoints.

Tests: 17 tests in tests/indexer-checkpoint-resume.test.ts covering incremental resume, multi-batch checkpoints, dead-lease recovery (global and project), interrupted clear/force-index, retries, and forced re-embed migration.

Validation

• npm run typecheck, npm run lint, git diff --check: OK
• npm run test:run: 1457 passed (2 pre-existing failures tied to the fr_FR locale, unrelated to this PR)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6148767bce

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Helweg]

Blocking issues found in interrupted global-index recovery:

1. A recovered global clear / clearing-phase force-index is replayed through the recovering Indexer, but IndexLockOwner has no originating project identity. If project B reclaims project A's dead lease, clearGlobalIndexUnlocked() derives B's roots and can remove B's data while not repairing A's partially cleared state. Persist the originating scope/project identity with the recovery marker and replay against that owner.

2. Recovery unconditionally calls healthCheckUnlocked() after checkpoints. A failed checkpointed chunk has an SQLite chunk row but no branch_chunks association until embedding succeeds, so orphan GC can delete its row before retry. The subsequent retry can then create a branch reference with missing chunk metadata. Preserve pending failures across recovery GC or restore their chunk rows before retry.

Please add regressions for both recovery scenarios and rerun the full hosted gate.

[Nicolas-nwb]

Thanks a lot for the review, Helweg! The two blocking points were spot on, and they led us to a third issue we hadn't seen. Here is what we did:

Fixes

1. Recovered clears are now bound to the originating project

IndexLockOwner now persists the originating projectRoot and scopedRoots (written into the lease and therefore into the recovery marker, with backward-compatible parsing). recoverFromInterruptedIndexingUnlocked replays each interrupted clear / clearing-phase force-index against the scope of the project that started it, falling back to the current project only for owners written before this change.

While implementing this, we found a subtle trap: the scoped clear purges the file-hash cache, but the recovery ran before the persisted cache was loaded, so the purge was silently lost. Recovery now loads the cache before replaying clears.

2. Failed checkpoint rows survive recovery GC

A failed chunk checkpointed before its embedding attempt has a committed SQLite row but no branch_chunks association, so the recovery health check's gcOrphanChunks() could delete it. Both retry paths (in-run retries and retryFailedBatches) now restore missing chunk rows from the pending chunk metadata before re-embedding, so a branch reference can never point at a chunk without metadata.

3. Bonus find: cross-project clear recovery could damage the reclaiming project's branch catalog

While reviewing the first fix, we noticed getProjectScopedBranchCatalogCleanupKeys() unconditionally added the current project's branch keys, even when cleaning up another project's scope. If project B reclaims project A's dead clear lease through retryFailedBatches() (which never re-scans the knowledge base), B could lose its shared knowledge-base entries from its branch catalog until a full re-index. Current-project branch keys are now only added when the cleared project is the current one.

Validation

• 3 new regression tests in tests/indexer-checkpoint-resume.test.ts (one per scenario above), each verified to fail without its fix.
• Full gate: npm run build, npm run typecheck, npm run lint, npm run test:run — 1462 tests, 93 files, all passing.
• The changes also went through two independent review passes (no findings).

Thanks again for the careful review, it made the recovery path noticeably safer.

[Helweg]

Thanks for the detailed follow-up. I can only review pushed changes, and PR #285 still points to 6148767, the revision where the blocking issues were found. Please push the described fixes, then request another review.

[Nicolas-nwb]

Sorry for the premature follow-up: I described the fixes before pushing the commit for review. The corrected commit (6010f2a) is now on the PR, with the full validation gate passing (build, typecheck, lint, and 1,462 tests). @Helweg, could you please take another look when convenient? Thank you again for the careful review.

[Helweg]

Thanks for pushing the follow-up. The targeted checkpoint tests, typecheck, and lint pass locally, and the new project-scoped recovery coverage addresses the primary issue for newly written leases. I found three remaining recovery hazards, so this still needs another update before it is safe to merge:

1. High: legacy leases can clear the reclaiming project. parseOwner intentionally accepts older owners with no persisted scope, but recovery falls back to the reclaimer's this.projectRoot and this.getScopedRoots(). If project B reclaims a dead pre-change project-A clear lease, it can replay A's clear against B. The new tests only exercise owners that already have the new scope fields. Please make legacy recovery fail safe, for example retain the recovery marker and require the originating scope to be known, rather than replaying a destructive clear against the current project.

2. Medium: recovery applies the reclaimer's compatibility/configuration decision to the dead owner's project. The replay has persisted paths but calls clearGlobalIndexUnlocked using the current Indexer and its configured provider/knowledge-base setup. A project B process with a different embedding configuration can therefore make a destructive compatibility decision while replaying project A's interrupted clear. Please either persist the needed recovery decision/configuration with the lease or avoid automatically replaying cross-project destructive clears when those settings differ.

3. Medium: force-index-phase is global and not owner-bound. Recovery identifies every dead force-index owner as being in its clearing phase whenever the single shared marker exists. A stale marker from a different lease can cause an unrelated recovered force-index to replay a clear. Please bind phase state to the lease token, or persist the phase on the owner/recovery marker, and add a regression that covers a stale marker from another owner.

Please also rebase onto current main once these are addressed, so the updated recovery path receives full current-head CI coverage.

[Nicolas-nwb]

Thanks for the careful follow-up. I addressed all three remaining recovery hazards and rebased the PR onto the current main (d23f2a9). The updated commit is 7a86c97.

What changed:

1. Legacy global clear leases now fail closed before any mutation when their originating recovery state is unavailable. Their recovery marker is retained for manual inspection instead of falling back to the reclaiming project.
2. The destructive clear phase is now persisted atomically on the lease owner together with the effective provider, model, dimensions, embedding-strategy version, and the originating compatibility decision. Cross-project replay requires the same effective embedding configuration and consistently uses the persisted project/knowledge-base roots; mismatches retain the marker without clearing data.
3. force-index clearing state is owner-bound rather than inferred from the shared force-index-phase file. A stale legacy marker cannot clear a protocol-v1 owner, while an ambiguous legacy owner plus legacy marker fails closed.

During an independent review, we found one additional legacy edge case: simply ignoring the old phase marker could resume a pre-change force-index from partially cleared artifacts. That case now also fails closed, with a dedicated regression. A second independent review reported no findings.

Validation:

• 5 new regressions, each reproduced against the previous implementation.
• 43 lock/checkpoint tests pass.
• Build, typecheck, and lint pass.
• The full local run reached 1,481/1,486 tests; the only five failures were watcher tests hitting EMFILE: too many open files. All three affected watcher files pass in isolation (37/37), so hosted CI remains the authoritative full-gate result.

@Helweg, could you please take another look when convenient? Thank you again for the detailed review.

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a86c972f2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Nicolas-nwb]

Small follow-up after the latest automated review: commit 19f1054 fixes the stale failed-batch state that could survive an empty checkpoint and be replayed after a crash.

The fix now publishes filtered failed-batch state before new file hashes at both intermediate checkpoints and final index exits, including an atomic empty state when obsolete retries are discarded. Regression coverage exercises crashes with and without an intermediate checkpoint.

Validation: the 27 checkpoint-recovery tests and 79 related recovery/lock/multiprocess tests pass; build, typecheck, and lint pass. The four watcher failures in the full parallel run were all EMFILE resource-limit failures and pass 38/38 when run individually. An independent review of the final diff reported no findings.

@Helweg, the PR head is now 19f1054 and remains rebased on current main. Could you please re-review when convenient? Thanks again.

[Helweg]

Re-reviewed the updated recovery protocol. The prior blockers are addressed with targeted regressions, and local plus hosted validation is green.