Skip to content

fix(project-files): persist index completeness across restarts - #1900

Open
ewen-poch wants to merge 8 commits into
mainfrom
fix/files-index-completeness-restart
Open

fix(project-files): persist index completeness across restarts#1900
ewen-poch wants to merge 8 commits into
mainfrom
fix/files-index-completeness-restart

Conversation

@ewen-poch

@ewen-poch ewen-poch commented Aug 29, 2026

Copy link
Copy Markdown
Member

Problem

Project Files index completeness was held primarily in ProjectFilesMutationOwner memory. A failed Session file sync did persist ManagedFileSessionSync.filesRevision = -1, but queries ignored that durable retry marker. Catalog-wide reconciliation failure was not persisted at all.

After an application restart, a ready Session Summary projection can avoid rescanning Session JSON. The recreated repository therefore had empty in-memory failure state and could report isIndexComplete: true for an incomplete index, allowing zero or partial results to be treated as authoritative by Files Overview, downloads, and global search.

Proposed change

  • Derive project completeness from active ManagedFileSessionSync.filesRevision < 0 rows.
  • Persist catalog-wide reconciliation completeness in a constrained singleton ManagedFileIndexState row.
  • Mark reconciliation incomplete before reconciliation work and complete only after it succeeds.
  • Read completeness in the query owner from SQLite instead of a synchronous mutation-owner callback.
  • Preserve the original operation error when the completeness marker cannot immediately be written, and retry the conservative global marker before the next observable Project Files read.
  • Track fallback marker writes by Session so a successful retry clears only that Session without hiding unrelated pending failures.
  • Normalize SQLite raw booleans across true, 1n, and numeric 1, and flush pending state before Artifact group projection reads.
  • Make the singleton migration idempotently seed a missing row when adopting an already-generated table, while preserving an existing persisted false value.
  • Serialize pending-marker flushes with synchronization of the same Session so a late -1 write cannot overwrite a successful retry.
  • Preserve Session deletion tombstones when flushing a deferred incomplete marker.
  • Flush deferred Session markers only for the projects involved in the current read, so one
    project's locked marker cannot block an unrelated project's Files view.
  • Pin migration 0020_project_files_index_state in packaged migration-ledger certification and replay fixtures.
  • Add public Repository regression coverage that recreates the Repository over the same database after both a Session indexing failure and a reconciliation failure.

Scope and non-goals

  • Data persistence changes: adds migration 0020_project_files_index_state and the ManagedFileIndexState singleton table.
  • Historical compatibility: existing databases are seeded with catalog reconciliation complete. Existing negative Session revisions remain authoritative and immediately make their projects incomplete. A catalog-wide failure from a pre-fix process cannot be reconstructed historically; the next reconciliation establishes the durable state.
  • No new enum values.
  • No Project Files IPC contract, renderer interaction, or user-visible behavior changes.
  • No Session JSON or Session Summary data-model changes.
  • No attempt to force a Session JSON rescan when its Summary projection is already ready.
  • Includes an independent one-character French punctuation correction inherited from current
    main; without it, the full Module shard fails its existing locale invariant.

Acceptance criteria and validation

Before the production fix, the two new restart tests failed repeatedly at the public ManagedFileIndexRepository.getOverview() boundary with expected false, received true. Both pass after the fix. Eight AI-review follow-up regressions also failed before their fixes: successful retry remained incomplete, numeric SQLite true parsed as false, Artifact group reads failed to persist pending state before restart, an adopted table could omit the singleton row, a concurrent pending flush could overwrite a successful retry, a pending flush could revive a deleted Session group, packaged ledger certification stopped at migration 0019, and a Project A read attempted to flush Project B's locked marker. All now pass.

Final checks after the last material edit:

  • vitest Project Files module: 66 passed, 1 skipped.
  • Project Files index-state migration suite: 3 passed.
  • Packaged database migration-ledger smoke: 12 passed.
  • Module-impact validation: 9 passed.
  • Generated database schema consistency check: passed.
  • vitest Session Persistence coordinator: 149 passed.
  • vitest Session deletion integration: 16 passed.
  • vitest artifact finalization recovery integration: 14 passed.
  • Targeted ESLint for the final touched implementation and migration files: passed.
  • i18n resources guard: 743 passed; locale-owner suite: 7 passed.
  • git diff --check: passed before commit; committed worktree is clean.

Additional affected checks run during implementation:

  • Other affected migration suites: 23 passed.
  • Migration service suite excluding one stale generated-client case: 60 passed, 1 skipped.
  • Application database integration excluding two stale generated-client cases: 20 passed, 2 skipped.
  • Full repository ESLint: passed.

Uncovered/local environment risks:

  • This worktree intentionally reused the main checkout dependencies because its lockfile differs. That Prisma Client was generated from the old schema, so three legacy-classification tests see ManagedFileIndexState as an unknown table. Their remaining assertions pass when those cases are excluded; CI's fresh generated client is authoritative.
  • Node TypeScript checking reaches one unrelated dependency mismatch: the reused @agentclientprotocol/claude-agent-acp package lacks the branch-expected waitForMcpServers export. No changed file reports a type error.
  • Full npm test was not run locally as requested; PR CI owns the full matrix.

Review focus

  • Compatibility-first true seed for existing databases versus conservatively marking every upgraded catalog incomplete.
  • State transitions around reconciliation failure/crash and successful retry.
  • Completeness SQL for active negative Session revisions, including searched overviews and cross-project artifact search.
  • Keeping the query owner read-only while removing completeness authority from process memory.

@github-actions github-actions Bot added the bug Something isn't working label Aug 29, 2026
@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P2] Clear pending incompleteness after a successful retry

src/main/project-files/mutation-owner.ts:236

Impact: If both the failed sync and its fallback marker write fail, hasPendingIncompleteState is set. A later successful syncSession does not clear it; the next read persists the global marker as false, so Project Files remains reported incomplete until a full reconciliation/repair runs.

Recommendation: Track pending failures by session or otherwise clear the pending state when the affected session is successfully synchronized, while preserving unrelated pending reconciliation failures.

Summary: A transient sync failure can leave the durable completeness state stuck incomplete after a successful retry.

@ewen-poch

Copy link
Copy Markdown
Member Author

Addressed in 13896b1. Pending fallback writes are now tracked per Session, successful retries clear only their own entry, and reconciliation pending state remains independent. Added a public Repository regression test that failed before the fix with expected true / received false and now passes.

@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Treats SQLite numeric true as incomplete

src/main/project-files/index-state.ts:31

Impact: SQLite CASE expressions may return numeric 1, but this check accepts only true or 1n. Completed indexes can therefore be reported as incomplete; query-support.ts repeats the same defect.

Recommendation: Normalize the raw result or accept numeric 1 in both completeness readers and their row types.

[P2] Artifact-group reads skip pending-state flush

src/main/project-files/query-owner.ts:456

Impact: listArtifactGroups can return projection data without persisting an in-memory incomplete marker. If the process exits before another Project Files query, restart may treat stale data as complete.

Recommendation: Call beforeRead(client) before executing listArtifactGroups, and preserve this flush invariant for all projection reads.

Summary: Static inspection found two concrete correctness issues. Branch and pull-request title prechecks are valid.

@ewen-poch

Copy link
Copy Markdown
Member Author

Addressed both findings in 798495d. Completeness parsing now shares a true/1n/1 normalizer, and listArtifactGroups flushes pending state before reading the projection. Added focused regressions for numeric SQLite true and restart persistence through the Artifact-group read path; both failed before the fix and now pass. Full Project Files suite: 59 passed, 1 skipped.

@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Migration can skip seeding the singleton marker

src/main/database/migrations/0020-project-files-index-state.ts:15

Impact: The migration verifier checks only the table and constraint. On a supported pre-ledger database that already has the generated table but no row, migration execution skips both statements, so every completeness read returns false until reconciliation runs.

Recommendation: Verify the singleton row exists, or make the migration idempotently insert it when the table already satisfies the structural contract.

[P2] Pending retry flush can overwrite a successful sync

src/main/project-files/mutation-owner.ts:523

Impact: A Project Files read can flush a pending -1 marker concurrently with a retrying sync. If the flush upsert commits after the successful sync, it restores filesRevision = -1 and leaves the index incorrectly incomplete, potentially across restart.

Recommendation: Serialize pending-marker flushes with session mutations or condition the durable update on the session still being in the failed/pending state.

Summary: Static inspection found two correctness defects in durable Project Files completeness handling.

@ewen-poch

Copy link
Copy Markdown
Member Author

Addressed both findings in 46fdeb8. Migration 0020 now verifies the singleton row and idempotently seeds it when an existing generated table is adopted, without requiring or overwriting the persisted completeness value. Pending Session marker flushes now share a per-Session serialization lane with sync retries, preventing a late -1 upsert from overwriting a successful revision. Added focused regressions for both cases; each failed before the fix and now passes. Final Project Files module: 64 passed, 1 skipped; migration suite: 3 passed.

@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Update packaged migration-ledger smoke expectations for migration 0020

scripts/database-migration-ledger-smoke.mjs:10

Impact: Package and installer smoke workflows still expect the ledger to end at migration 0019, so they will reject applications that correctly record 0020_project_files_index_state.

Recommendation: Append migration 0020 and its checksum to the expected ledger, and update the companion smoke-test fixtures that remove or pin migration 0019.

Summary: The new migration is not reflected in packaged migration-ledger smoke expectations.

@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Pending failure flush can revive deleted sessions

src/main/project-files/mutation-owner.ts:580

Impact: If a sync fails while the database is unavailable, a later session/project deletion tombstones its sync row, and the next read flushes the pending marker. This update clears deletedAt and deleteOperationId while retaining artifactCount, so deleted artifact groups can reappear.

Recommendation: Remove pending markers when deletion succeeds, or make pending-marker persistence refuse to reactivate deleted rows; add coverage for failure, deletion, and subsequent reads.

Summary: Found one deletion/retry race that can re-expose deleted artifact groups.

@ewen-poch

Copy link
Copy Markdown
Member Author

Addressed the latest-head deletion finding and the prior packaged-ledger finding in 4898b97. Deferred Session marker updates no longer clear deletedAt/deleteOperationId, so a tombstoned Session cannot be reactivated by a later Project Files read. Packaged migration certification now pins 0020 and replay fixtures remove/reapply it with dependent suffixes. Both regressions failed before the fixes (revived group count 1; ledger checksum still 0019) and now pass. Project Files module: 65 passed, 1 skipped; packaged ledger smoke: 12 passed.

@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Flushes unrelated project markers during reads

src/main/project-files/mutation-owner.ts:532

Impact: Every Project Files read flushes all pending session markers, including markers for unrelated projects. A readable Project A can therefore be blocked by a failed or locked write for Project B, bypassing the existing project-scoped deletion/recovery boundary and making unrelated file views fail.

Recommendation: Scope pending-marker flushing to the project IDs being read (while retaining the global reconciliation marker behavior), or make marker persistence best-effort and return the indexed data with completeness false.

Summary: Static review found one cross-project read-path regression.

@ewen-poch

Copy link
Copy Markdown
Member Author

Addressed the latest cross-project flush finding in 50fc8ca. The read hook now receives the projects participating in the query and flushes only pending Session markers owned by those projects; the global reconciliation marker remains global. A public Repository regression creates a pending marker for Project B, makes B's marker write fail, and verifies Project A's overview still resolves. It failed before the fix with 'unrelated project is locked' and now passes. After merging current main, I also isolated and corrected its existing French pre-colon spacing failure in 8a8a4cf; the exact locale test and the 743-case i18n guard pass. Final Project Files module: 66 passed, 1 skipped.

@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects in the pull request changes.

@github-actions github-actions Bot added the ready-to-merge All completed AI reviewers found this pull request mergeable. label Aug 29, 2026
Read per-project completeness from durable retry revisions and persist catalog reconciliation state so repository recreation cannot turn an incomplete index into an authoritative empty result.

Seed the new singleton state as complete for compatibility with existing databases.
Track fallback marker writes by Session so a successful retry clears only its own pending failure without hiding unrelated incomplete Sessions or reconciliation failures.
Accept numeric SQLite booleans consistently and flush pending Session markers before Artifact group projection reads.
@ewen-poch
ewen-poch force-pushed the fix/files-index-completeness-restart branch from 9f88df7 to 9b5e990 Compare August 30, 2026 06:52
@github-actions github-actions Bot removed the ready-to-merge All completed AI reviewers found this pull request mergeable. label Aug 30, 2026
@github-actions

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Do not mark the index complete while active pending sync markers remain

src/main/project-files/mutation-owner.ts:410

Impact: A full reconciliation can set the durable marker to complete while an active session remains only in pendingIncompleteSessions. If the process restarts before a read flushes that marker, the in-memory state is lost and the projection may be reported complete despite missing data.

Recommendation: Persist active pending session markers before setting the catalog marker complete, or keep the durable marker incomplete until those markers have been flushed successfully.

[P2] Await the asynchronous incomplete-state marker

src/main/session-persistence/deletion-owner.ts:538

Impact: This call now returns a Promise but is still fire-and-forget. The scoped reconciliation can finish and trigger global reconciliation before the durable marker write completes, causing ordering races and potentially stale completeness state.

Recommendation: Await markReconciliationIncomplete() in this catch path so the marker operation is ordered before returning.

Summary: Static inspection found two correctness issues in durable Project Files completeness handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant