Skip to content

fix(snapshot): preserve data when restoring pre-subject_id snapshots - #248

Open
loveRhythm1990 wants to merge 7 commits into
matrixorigin:mainfrom
loveRhythm1990:fix/legacy-snapshot-schema-compat
Open

fix(snapshot): preserve data when restoring pre-subject_id snapshots#248
loveRhythm1990 wants to merge 7 commits into
matrixorigin:mainfrom
loveRhythm1990:fix/legacy-snapshot-schema-compat

Conversation

@loveRhythm1990

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • feat (new feature)
  • fix (bug fix)
  • docs (documentation)
  • style (formatting, no code change)
  • refactor (code change that neither fixes a bug nor adds a feature)
  • perf (performance improvement)
  • test (adding or updating tests)
  • chore (maintenance, tooling)
  • build / ci (build or CI changes)

Which issue(s) this PR fixes

Fixes #246

What this PR does / why we need it

Restoring a snapshot created before subject_id was introduced previously ran an
autocommitted DELETE followed by INSERT ... SELECT *. The INSERT could fail on
the historical column count after live rows had already been deleted. Historical
snapshot branches also inherited an outdated schema.

This is a separate snapshot-safety fix, based on current main after #244. It does
not reintroduce that PR's scoped-key changes or modify MatrixOne source/deployment.

Changes

  • Map historical rows to current columns by name; honor current defaults/NULL and
    reject missing required values before touching live data.
  • Materialize a private constrained staging table outside the write transaction;
    replace live rows with DELETE + INSERT in one transaction. Preserve the original
    statement error even if rollback fails. Do not retry the non-idempotent stage INSERT.
  • Schedule best-effort stage cleanup, including cancellation/no-runtime diagnostics.
  • Share startup/clone subject_id migration. Require all current memory-column
    names before registering a historical clone, while treating index creation as
    best-effort. Reject and clean up unusable clones.
  • Generate ASCII physical branch names while preserving Unicode display names;
    retain quoted access/migration/cleanup for existing Unicode physical names.
  • Add an offline-only, read-only-by-default cleanup example with exact-name and
    explicit all-writers-stopped requirements. Protect every registered branch and
    fail closed if the registry cannot be read; no online prefix-based reaper.
  • Cover 512 real embeddings with FULLTEXT/IVF indexes, failure atomicity, historical
    schema gaps, Unicode names, permissions, and cleanup guardrails. Explicitly run
    ignored DB failure-injection tests and example tests in both CI test workflows.

Validation

  • Previous isolated runs passed on MatrixOne 3.0.11 (6a0394c) and 3.0.15 (43e871c):
    9 legacy snapshot tests, 1 transaction failure test, 1 DB cleanup test, 49 MCP
    branch tests, 21 MCP snapshot tests, and 20 storage compatibility tests per build.
  • Post-rebase validation against 3.0.15 and database-free checks are recorded in
    the follow-up validation comment. These are regression baselines, not production
    upgrade approval. Remote CI results must be checked separately.
  • cargo clippy -p memoria-git -p memoria-storage -p memoria-mcp -p memoria-api --lib -- -D warnings
    and git diff --check pass.

Production upgrade status — not yet approved

The intended production target is MatrixOne
v4.2.1-d2393868a-2026-08-28
, not 3.0.15. A separate 3.0.11-to-target rehearsal
using unchanged Memoria apiserver-20260511-27107f5-df643526 hit an engine catalog
migration failure: no such table mo_catalog.mo_feature_registry. The internal
upgrade remained incomplete and SHOW SNAPSHOTS failed with
column kind does not exist. The target accepted SQL connections, so connectivity
and API liveness alone are not sufficient upgrade readiness checks.

This PR does not fix that MatrixOne upgrade failure, and its new Memoria DB
suites have not been validated on the target build. Resolve/rehearse the supported
MatrixOne upgrade path first, validate old Memoria, then validate/deploy new Memoria.

Review focus / boundaries

  • Atomicity is per restored table, not the whole memory/graph operation. Serialize
    snapshot operations and quiesce competing writes.
  • Staging retains FULLTEXT/IVF indexes: additional storage/index writes and a
    whole-table transaction need capacity planning; 512 rows is not a scale limit.
  • Abrupt shutdown can leave staging/orphan tables. Offline cleanup is an operator
    tool, not proof that an unregistered table is safe to delete while writers run.
  • Column-presence checks do not normalize types/order or guarantee native Data
    Branch diff schema compatibility on every MatrixOne version.

See the compatibility and verification notes
for the regression commands, operational constraints, and exact upgrade evidence.

…nches

Materialize historical rows with explicit column mappings and current defaults before transactional replacement. Reconcile subject_id on snapshot branches before registration. Add legacy-schema, failure-atomicity and MCP regressions verified on MatrixOne 3.0.11 and 3.0.15.

Fixes matrixorigin#246
@loveRhythm1990

Copy link
Copy Markdown
Collaborator Author

Post-rebase local validation (24fabd1)

Rebased onto main at 58012cf (merged #244), then reran these checks on an
isolated MatrixOne 3.0.15 instance (43e871c), not the user's running database:

Suite Result
memoria-git --lib --include-ignored 4 passed (includes DB rollback failure injection)
memoria-git --test legacy_snapshot 9 passed
memoria-git --example restore_cleanup --include-ignored 3 passed (includes DB deletion guardrails)
memoria-mcp --test branch_e2e 49 passed
memoria-mcp --test snapshot_e2e 21 passed
memoria-storage --test branch_ops 10 passed
memoria-storage --test subject_id_mo_compat 10 passed
Focused shared-schema and ASCII-branch-name unit tests 2 passed

Total: 101 DB test results + 7 database-free tests, zero failures in these
commands. Also passed Clippy with -D warnings for the git/storage/MCP/API
libraries and git diff --check.

Remote CI is still running; this comment does not claim CI completion or a
production upgrade approval. The separate 3.0.11 -> 4.2.1-d2393868a rehearsal
remains blocked by MatrixOne's incomplete catalog migration, as documented in
the PR body. MatrixOne source and the existing local service stack are unchanged.

@loveRhythm1990

Copy link
Copy Markdown
Collaborator Author

Fixed the DB-test failure in appended commits e9cdebb and b8a2761.

Root cause

test_existing_unicode_physical_branch_migrates_and_remains_usable created an ordinary table with CREATE TABLE ... LIKE, then expected DATA BRANCH DELETE to delete it. The MatrixOne image used by CI rejects it because it is not an active native branch. The offline-cleanup example had the same fixture problem, hidden behind the earlier failed step.

Changes

  • Build genuine native branches, then use a quoted rename to represent legacy Unicode physical table names.
  • Execute native DDL statements individually: MatrixOne 3.0.11 cannot parse these correctly inside a multi-statement request.
  • Assert physical branch removal and preservation of the cleanup source table/data.
  • Preserve the existing fail-closed deletion checks. No production deletion behavior changed, no generic DROP TABLE fallback added, and no tests skipped.

Validation

The previously failing Unicode-branch test and all three cleanup-example tests pass on each of:

  • MatrixOne 3.0.11
  • MatrixOne 3.0.15
  • The requested stable 4.2.1 image (d2393868a)
  • The exact CI image, reporting 4.2.2 (4f62825), pinned locally to sha256:5baf93ca9f88a9b83e016644c1420e356841934797d257ec364e80f212462913; tested with CI's 1024-dimensional embeddings.

On that exact CI image, the broader regression run also passed: 49 branch E2E tests, 21 snapshot E2E tests, 9 legacy-snapshot tests, 4 library tests (including the ignored transaction-failure regression), and 3 cleanup-example tests. The broader run used the first fixture-fix revision; after splitting the DDL requests for 3.0.11, both affected test targets were rerun successfully on all four versions above. Formatting and the focused Clippy check pass.

The new remote run for b8a2761 has passed PR title, Check & Clippy, and Unit Tests; DB Tests are still running at the time of this comment. This fixture fix does not resolve or change the separately reported MatrixOne 3.0.11-to-4.2.1 catalog-upgrade issue.

@gouhongshen

Copy link
Copy Markdown
Contributor

Findings on b8a2761:

  1. [P1] Default merge drops subject_id and author_id. memory_merge maps the default accept strategy to the replace SQL path, but the INSERT INTO mem_memories column list and its SELECT omit both fields (memoria/crates/memoria-mcp/src/git_tools.rs:1153-1229). A historical branch is explicitly migrated to include subject_id, and the new regression writes a non-null subject_id, but no test merges that row. I reproduced this on MatrixOne 4.1.2: a branch row with subject_id='subject-1' and author_id='author-1' is inserted into main with both values NULL. Add the fields to both lists and cover from-snapshot write → merge → read.

  2. [P1] Presence-only schema validation can register a branch whose native diff is corrupt. ensure_branch_subject_id only compares column names and appends a missing subject_id (memoria/crates/memoria-storage/src/store.rs:2017-2047), then memory_branch registers the clone (memoria/crates/memoria-mcp/src/git_tools.rs:1078-1096). MatrixOne DATA BRANCH DIFF is ordinal/schema-sensitive. In a disposable MatrixOne 4.1.2 database, the live table had subject_id between user_id and memory_type, while the historical clone got it appended; DATA BRANCH DIFF did not reject the clone and returned the live row with memory_type=NULL, content=semantic, and subject_id=old. That makes memory_diff/merge unsafe for a registered historical branch. Compare the full column order/types or reject the clone before registration, and add a legacy-clone diff/merge regression.

  3. [P2] Unicode snapshot names are accepted but sent as unquoted SQL identifiers. The new shared validator accepts Unicode (memoria/crates/memoria-core/src/lib.rs:10-14), and snap_internal preserves Unicode names (memoria/crates/memoria-mcp/src/git_tools.rs:162-173), but create_snapshot emits CREATE SNAPSHOT {safe} without quoting (memoria/crates/memoria-git/src/service.rs:669-675). On the local MatrixOne 4.1.2 instance, CREATE SNAPSHOT mem_snap_1_review_实验 ... fails with SQL parser error 1064 near 实验. Therefore memory_snapshot with a Chinese name still fails; quote the physical snapshot identifier or make it ASCII while retaining the display name. The added Unicode coverage tests branch names only, not snapshot creation.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on b8a2761:

  1. [P1] Default merge drops subject_id and author_id. memory_merge maps the default accept strategy to the replace SQL path, but the INSERT INTO mem_memories column list and its SELECT omit both fields (memoria/crates/memoria-mcp/src/git_tools.rs:1153-1229). A historical branch is explicitly migrated to include subject_id, and the new regression writes a non-null subject_id, but no test merges that row. I reproduced this on MatrixOne 4.1.2: a branch row with subject_id='subject-1' and author_id='author-1' is inserted into main with both values NULL. Add the fields to both lists and cover from-snapshot write → merge → read.

  2. [P1] Presence-only schema validation can register a branch whose native diff is corrupt. ensure_branch_subject_id only compares column names and appends a missing subject_id (memoria/crates/memoria-storage/src/store.rs:2017-2047), then memory_branch registers the clone (memoria/crates/memoria-mcp/src/git_tools.rs:1078-1096). MatrixOne DATA BRANCH DIFF is ordinal/schema-sensitive. In a disposable MatrixOne 4.1.2 database, the live table had subject_id between user_id and memory_type, while the historical clone got it appended; DATA BRANCH DIFF did not reject the clone and returned the live row with memory_type=NULL, content=semantic, and subject_id=old. That makes memory_diff/merge unsafe for a registered historical branch. Compare the full column order/types or reject the clone before registration, and add a legacy-clone diff/merge regression.

  3. [P2] Unicode snapshot names are accepted but sent as unquoted SQL identifiers. The new shared validator accepts Unicode (memoria/crates/memoria-core/src/lib.rs:10-14), and snap_internal preserves Unicode names (memoria/crates/memoria-mcp/src/git_tools.rs:162-173), but create_snapshot emits CREATE SNAPSHOT {safe} without quoting (memoria/crates/memoria-git/src/service.rs:669-675). On the local MatrixOne 4.1.2 instance, CREATE SNAPSHOT mem_snap_1_review_实验 ... fails with SQL parser error 1064 near 实验. Therefore memory_snapshot with a Chinese name still fails; quote the physical snapshot identifier or make it ASCII while retaining the display name. The added Unicode coverage tests branch names only, not snapshot creation.

@aptend aptend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional blocker found on b8a2761:

[P1] Preserve subject_id in every memory_apply copy path. ensure_branch_subject_id makes scoped writes valid on historical branches, but every selective_apply branch-to-main INSERT ... SELECT copies author_id while omitting subject_id (memoria/crates/memoria-git/src/service.rs:1273-1281, 1308-1316, 1370-1394, 1470-1478, 1561-1569). Consequently, applying an add, restore, update, remove, or accepted conflict sets the target row’s subject_id to its default NULL, so the memory disappears from subject-scoped reads and can surface in unscoped reads. Add subject_id to both sides of all these column lists and cover historical branch → scoped write → memory_apply → scoped main read.

The three blockers in the earlier changes-request review also remain unresolved on this head.

Address PR matrixorigin#248 reviews: preserve subject and author metadata through merge and all apply paths, isolate similarity conflicts by subject, quote Unicode snapshot identifiers, and validate native branch schemas across migration and operations.

Add ordinary/legacy branch scope regressions and schema/Unicode guards. Validated 93 focused and regression tests on MatrixOne 4.2.1 plus strict Clippy checks.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on 3fdf3eef:

  1. [P1] Do not ALTER a native branch before using native branch operations. migrate_branch_subject_id adds subject_id to the result of DATA BRANCH CREATE TABLE ... FROM SNAPSHOT with ALTER TABLE (memoria/crates/memoria-storage/src/store.rs:2026-2037), and may add the subject index with another ALTER TABLE (memoria/crates/memoria-storage/src/store.rs:2065-2075). On MatrixOne 4.1.2 this rewrites the object into a physical copy and drops its native branch lineage: mo_catalog.mo_tables.rel_createsql contains a ..._copy_<uuid> table, and DATA BRANCH DELETE then returns 20101, is not an active branch table. The same happens for an existing Unicode branch migrated at startup. A historical clone is registered after this mutation (memoria/crates/memoria-mcp/src/git_tools.rs:1078-1096), so subsequent native diff/merge/pick/delete operations are unusable; when schema validation rejects the clone, the cleanup call uses the same native delete and leaves the unregistered clone behind. I reproduced this with the new tests: branch_scope_e2e failed 3/6, legacy_snapshot failed 1/11, and branch_e2e failed 2/49 on MatrixOne 4.1.2. Preserve native lineage through migration, or keep the migrated object out of native operations and provide a deliberate cleanup/operation path that matches its actual table type. Add a regression on the supported 4.x build.

@aptend aptend left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocker on 3fdf3eef:

[P1] Do not destroy native branch lineage while migrating historical branches. The historical-branch path creates a native branch, then migrate_branch_subject_id runs ALTER TABLE to add/reposition subject_id and may run another ALTER TABLE for the index (memoria/crates/memoria-storage/src/store.rs:2026-2075). On MatrixOne 4.1.2, ALTER rewrites that native branch into a physical-copy table, after which DATA BRANCH DIFF, merge, pick, and delete no longer recognize it as an active branch. This also means cleanup after validation failure can leave an unregistered clone behind because it calls native branch delete.

Preserve native lineage, or stop routing the migrated object through native branch operations and give it a matching cleanup/operation path. Add a supported-4.x regression covering historical clone → migration → diff/merge/delete.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings on 0015e31:

  1. [P1] Migrate author_id on Unicode legacy branch tables. The PR now accepts Unicode physical names in migrate_branch_subject_id and documents that existing Unicode physical names remain valid, but the author_id compatibility loop still skips every non-ASCII table name (memoria/crates/memoria-storage/src/store.rs:1844-1851). For a pre-author_id native branch such as br_1234_实验, migration adds author_id to mem_memories and silently leaves the branch without it. The next schema check reports the branch missing author_id, and branch read/write SQL that projects author_id also fails, so the registered branch becomes unusable. Remove the ASCII-only guard and use the shared identifier validator/quoted self.t(bt_raw), then add a regression for a Unicode physical branch missing author_id that runs migration and a branch read/write or native operation.

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]: Legacy snapshot restore and branch creation are unsafe after subject_id migration

3 participants