fix(project-files): persist index completeness across restarts - #1900
fix(project-files): persist index completeness across restarts#1900ewen-poch wants to merge 8 commits into
Conversation
Codex ReviewVerdict: needs changes [P2] Clear pending incompleteness after a successful retrysrc/main/project-files/mutation-owner.ts:236 Impact: If both the failed sync and its fallback marker write fail, 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. |
|
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. |
Codex ReviewVerdict: needs changes [P1] Treats SQLite numeric true as incompletesrc/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 flushsrc/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. |
|
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. |
Codex ReviewVerdict: needs changes [P1] Migration can skip seeding the singleton markersrc/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 syncsrc/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. |
|
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. |
Codex ReviewVerdict: needs changes [P1] Update packaged migration-ledger smoke expectations for migration 0020scripts/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. |
Codex ReviewVerdict: needs changes [P1] Pending failure flush can revive deleted sessionssrc/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. |
|
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. |
Codex ReviewVerdict: needs changes [P1] Flushes unrelated project markers during readssrc/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. |
|
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. |
Codex ReviewVerdict: mergeable No actionable findings. Summary: Static inspection found no concrete merge-blocking defects in the pull request changes. |
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.
9f88df7 to
9b5e990
Compare
Codex ReviewVerdict: needs changes [P1] Do not mark the index complete while active pending sync markers remainsrc/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 markersrc/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. |
Problem
Project Files index completeness was held primarily in
ProjectFilesMutationOwnermemory. A failed Session file sync did persistManagedFileSessionSync.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: truefor an incomplete index, allowing zero or partial results to be treated as authoritative by Files Overview, downloads, and global search.Proposed change
ManagedFileSessionSync.filesRevision < 0rows.ManagedFileIndexStaterow.true,1n, and numeric1, and flush pending state before Artifact group projection reads.falsevalue.-1write cannot overwrite a successful retry.project's locked marker cannot block an unrelated project's Files view.
0020_project_files_index_statein packaged migration-ledger certification and replay fixtures.Scope and non-goals
0020_project_files_index_stateand theManagedFileIndexStatesingleton table.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 withexpected 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:
vitestProject Files module: 66 passed, 1 skipped.vitestSession Persistence coordinator: 149 passed.vitestSession deletion integration: 16 passed.vitestartifact finalization recovery integration: 14 passed.git diff --check: passed before commit; committed worktree is clean.Additional affected checks run during implementation:
Uncovered/local environment risks:
ManagedFileIndexStateas an unknown table. Their remaining assertions pass when those cases are excluded; CI's fresh generated client is authoritative.@agentclientprotocol/claude-agent-acppackage lacks the branch-expectedwaitForMcpServersexport. No changed file reports a type error.npm testwas not run locally as requested; PR CI owns the full matrix.Review focus
trueseed for existing databases versus conservatively marking every upgraded catalog incomplete.