fix(storage): harden project store authority and resolution - #506
fix(storage): harden project store authority and resolution#506HashemKhalifa wants to merge 20 commits into
Conversation
Consolidation normalizes private session snapshots while holding the enclosing profile's exclusive maintenance lease. Map only its reserved staging directory to that profile authority, and keep append-only hook telemetry outside immutable confirmation fingerprints without dropping its bytes.
Reserve profile maintenance authority for the two session snapshots created by consolidation. Other files under the staging directory keep their independent database identity.
🦋 Changeset detectedLatest commit: 91944bc The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Branch-scoped stores must not inherit repository-wide dirty state from sibling worktree syncs. Reuse Database::open's read-only validation to avoid a second full recovery scan.
Dirty zero-length databases must fail before writable initialization so recovery bytes and markers remain available for offline repair.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 043a047e03
ℹ️ 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".
Treat unreadable auxiliary state as a conflict, validate the serving branch before identity repair, and recheck cached writable connections during dirty recovery.
Establish every bounded inventory health signal before populated graph or session data may authorize exact-root selection.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91944bc6ce
ℹ️ 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".
| let Some(selector_path) = selector_path else { | ||
| return true; | ||
| }; | ||
| if !GlobalDb::is_explicit_project_path_selector(selector_path) { | ||
| return true; |
There was a problem hiding this comment.
When the MCP server is attached to a linked worktree and the caller selects the shared project by project_id (or by a non-explicit basename), this returns the active TraceDecay solely because the project IDs match. Registration deliberately keeps canonical_root pinned to the primary checkout (src/tracedecay/lifecycle.rs:951-973), and the normal selected-reader path opens that root (src/mcp/tools/handlers/mod.rs:158-159); the primary checkout and active worktree can serve different branch databases. Consequently, an explicit registered-project query can silently return results from the active worktree instead of the selected canonical project, so the shortcut should also require the active root to match context.project.canonical_root when no explicit path is supplied.
Useful? React with 👍 / 👎.
| if selector_targets_active_project(active_cg, args, &context.project.project_id) { | ||
| return Ok(None); |
There was a problem hiding this comment.
When a server was started from a subdirectory and a graph tool explicitly selects the active project path, this new shortcut returns None; downstream, selected_cg.is_some() is therefore false and lines 415-419 retain the connection's scope_prefix. Before this optimization, the selected project was opened as a separate reader and its scope was cleared, just as it still is for every other registered-project selector. The same explicit project query can now silently omit files outside the server's startup subdirectory, so reusing the handle must be represented separately from having no project selector and must still clear the scope.
Useful? React with 👍 / 👎.
| let presence = ( | ||
| table_presence(db.conn(), "nodes").await, | ||
| table_presence(db.conn(), "files").await, | ||
| table_presence(db.conn(), "memory_facts").await, |
There was a problem hiding this comment.
When an identity-resolution candidate was last written by a supported pre-v11 TraceDecay version, memory_facts does not exist yet—the table is introduced by migration v11 in crates/tracedecay-runtime-core/src/db/migrations.rs:1321-1327. Because this read-only presence probe intentionally runs no migrations, the query errors even when nodes and files are healthy and populated; the fast path then rejects the selected store, and the full inventory labels it corrupt. The analogous unconditional LCM-table probes have the same problem for older session schemas. Thus an upgrade involving a preserved duplicate shard can fail during resolution before the normal writable open gets a chance to migrate it; a missing table from an older schema should contribute zero presence rather than unhealthy status.
Useful? React with 👍 / 👎.
| if is_runtime_lock(&relative) | ||
| || is_sqlite_sidecar(&relative) | ||
| || is_volatile_hook_telemetry(&relative) | ||
| { | ||
| // Hooks may append this telemetry between dry-run and apply. It is | ||
| // omitted only from the confirmation fingerprint; backup and | ||
| // artifact merge still copy and checksum every telemetry byte. |
There was a problem hiding this comment.
If consolidation is interrupted after DestinationReady or ArtifactsMerged and a hook then appends to either input's hook_analytics.jsonl, excluding this file keeps the old confirmation token valid on retry. The ledger resumes after the relevant artifact-copy phase, so the prepared destination is not refreshed from the target and an already-completed source artifact merge is not rerun; the newly appended rows are therefore absent from the consolidated store after marker cutover even though the retry succeeds. Keep the token stable if desired, but resumptions must recopy or append-merge this volatile file before cutover rather than skipping change detection without reconciling it.
Useful? React with 👍 / 👎.
1 participant
Summary
Root cause
Project resolution treated store comparison as an inventory operation even on authoritative and conflict-reporting paths. Preserved candidates therefore caused expensive graph and session-table diagnostics before ordinary graph tools could run. Branch-scoped stores could also be misclassified because health checks opened the root graph rather than the serving branch graph. An explicit selector for the already-active project opened the same store a second time, re-entering resolution and amplifying the delay.
A separate hot path remained in session correlation: ordinary requests traversed large correlation tables for diagnostic counts. During restart, scheduler shutdown could also wait indefinitely for writer administration, and watcher shutdown awaited one task before aborting the rest. The process could remain alive while the listener stopped making progress.
Crash recovery also validated the same non-empty database twice: the lifecycle performed a read-only preflight, then
Database::openperformed its own WAL-aware read-only validation before writable pragmas and migrations. Branch syncs additionally published a repository-wide legacy dirty marker, allowing one sibling worktree to trigger recovery in another branch graph. Dirty zero-length stores required an explicit guard because the ordinary open path correctly treats a zero-length database as fresh outside recovery.Behavior after this change
count_mode: presence_onlyValidation
cargo check --workspacecargo fmt --checkgit diff --checkOne-time transcript catch-up and stale-index refresh remain cold-start work; they are separate from the repeated recovery scans fixed here.
commitlintwas unavailable in this checkout (commitlint: command not found); all commit subjects follow the repository's conventional format.