Skip to content

fix(engine): stop init cleanup from deleting a completed graph - #502

Merged
aaltshuler merged 4 commits into
ModernRelay:mainfrom
azimafroozeh:init-cleanup-completed-store
Aug 16, 2026
Merged

fix(engine): stop init cleanup from deleting a completed graph#502
aaltshuler merged 4 commits into
ModernRelay:mainfrom
azimafroozeh:init-cleanup-completed-store

Conversation

@azimafroozeh

@azimafroozeh azimafroozeh commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

What & why

Closes #495. Omnigraph::init runs its best-effort schema cleanup on every error, including errors returned after GraphCoordinator::init has committed the stamped __manifest, which is the point where the graph is durably complete. Past that point the cleanup deletes the three schema files while the Lance data stays, leaving a graph that neither opens (raw storage read error) nor re-inits (Dataset already exists); the only recovery is deleting the root by hand. A process crash at the same point leaves an openable graph, so the damage is entirely cleanup-induced: backward recovery applied past the commit point.

  • Split init at the commit point: the commit phase now ends at the __manifest commit, so an error from it is provably pre-commit and keeps today's cleanup; the post-commit checks run as a separate step whose errors return with the graph untouched and a message stating the graph is intact and opens normally.
  • Open-path floor for already-damaged graphs: when __manifest and data are present but _schema.pg is missing, open returns a typed not-found naming the state and the remedy (restore the schema files, or rebuild via export) instead of a raw storage read error.
  • Both error surfaces change intentionally and are pinned by tests: the init tail error gains the intact-graph note, and the open path trades an untyped storage error for the typed diagnosis. Callers matching on the old texts must update.

Future work

A forward-recovery path that completes a post-commit failure instead of reporting it was considered and deferred; it needs the recovery-sidecar protocol extended to init and is worth its own RFC.

Local verification

  • cargo test -p omnigraph-engine --features failpoints --test failpoints: green; the flipped pinning test init_failpoint_after_coordinator_init_leaves_completed_store_intact failed pre-fix with the predicted signature (schema files deleted, raw storage read error on reopen), and the new open_missing_schema_pg_reports_schema_files_missing covers the floor.
  • cargo test --workspace --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints: green, plus scripts/check-agents-md.sh after the docs edit.
  • cargo clippy --workspace --all-targets --locked -- -D warnings -W clippy::dbg_macro on both CI feature graphs and cargo fmt --all --check: clean.

Greptile Summary

The PR separates initialization at the durable manifest commit boundary and adds exact-attempt recovery for acknowledgement-unknown creates.

  • Preserves schema artifacts whenever physical initialization may have committed.
  • Wraps confirmed post-commit failures in typed initialization errors.
  • Adds a durable root-scoped initialization claim, exact-genesis probing, damaged-schema diagnostics, and failpoint coverage.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/omnigraph/src/db/omnigraph.rs Splits initialization into pre-physical, commit, ambiguous-outcome, and post-commit phases while ensuring only pre-physical failures authorize schema cleanup.
crates/omnigraph/src/db/manifest/graph.rs Adds attempt-local genesis receipts, explicit manifest-create outcome classification, exact-genesis authentication, and a separate post-commit state load.
crates/omnigraph/src/db/graph_coordinator.rs Exposes distinct coordinator operations for manifest commit, exact-genesis recovery, and post-commit assembly.
crates/omnigraph/src/error.rs Adds typed claimed, committed, and indeterminate initialization outcomes for callers and operators.
crates/omnigraph/tests/failpoints.rs Exercises confirmed post-commit failures, lost acknowledgements, failed probes, cleanup failures, retained claims, and successful reopening.
crates/omnigraph/src/db/manifest/tests.rs Verifies that exact-genesis probing accepts only the receipt minted by the creating initialization attempt.
crates/omnigraph/src/db/omnigraph.rs The earlier cleanup-boundary and unannotated-error findings are both resolved at current HEAD.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Acquire durable init claim] --> B[Write schema contract]
  B --> C[Create physical tables and manifest]
  C -->|Confirmed manifest commit| D[Run post-commit checks]
  C -->|Outcome unknown| E[Probe exact genesis receipt]
  E -->|Exact receipt found| F[Finish coordinator validation]
  E -->|Probe fails or mismatches| G[Return InitializationIndeterminate and retain claim]
  D -->|Success| H[Release claim and return graph]
  D -->|Failure| I[Release claim and return InitializationCommitted]
  F -->|Success| H
  F -->|Failure| I
  B -->|Failure before physical init| J[Clean owned schema artifacts and release claim]
Loading

Reviews (4): Last reviewed commit: "fix(engine): make graph initialization o..." | Re-trigger Greptile

Context used (3)

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Comment thread crates/omnigraph/src/db/omnigraph.rs Outdated
Comment thread crates/omnigraph/src/db/omnigraph.rs Outdated
@aaltshuler
aaltshuler force-pushed the init-cleanup-completed-store branch from 89bd7b3 to 79a0d2e Compare August 16, 2026 22:11
@aaltshuler

Copy link
Copy Markdown
Collaborator

Rebased this PR onto current main and pushed the initialization-safety fixes to the contributor branch.

The updated implementation now:

  • acquires one durable root-scoped init claim for strict and --force callers, so concurrent initializers cannot overwrite or clean up one another;
  • treats every error after physical dataset initialization begins as acknowledgement-unknown and preserves the schema contract;
  • authenticates an exact attempt-local genesis receipt after a lost manifest acknowledgement;
  • distinguishes committed, indeterminate, and already-claimed initialization outcomes with typed errors;
  • refuses strict or forced init when an existing __manifest is present;
  • retains the claim when cleanup or physical outcome is indeterminate;
  • documents the transient __init_claim.json object and the narrowed --force behavior.

The new tests cover force/force and force/strict races, table- and manifest-Create lost acknowledgements, exact-genesis mismatch, failed outcome probes, cleanup-delete ambiguity, stale claims, and fresh reopen after recovered acknowledgement.

Validation on the final tree:

  • canonical workspace failpoint-superset: passed (with the repository-required macOS debug stack setting);
  • default and failpoint-superset all-target clippy: passed;
  • rustfmt, AGENTS link check, focused init/failpoint/lifecycle/forbidden-API suites, CLI parity, server/OpenAPI suites: passed;
  • fresh GitHub required CI: passed;
  • fresh Greptile review: 18 files reviewed, 0 comments added.

@aaltshuler
aaltshuler merged commit 426c7b9 into ModernRelay:main Aug 16, 2026
10 checks passed
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.

bug: after a failed init, cleanup deletes a completed store's schema files but not its data, leaving it unable to open or re-init

2 participants