Skip to content

fix(storage): use current tenant for account snapshots#168

Closed
gouhongshen wants to merge 14 commits intomatrixorigin:mainfrom
gouhongshen:feat/per-user-db-isolation
Closed

fix(storage): use current tenant for account snapshots#168
gouhongshen wants to merge 14 commits intomatrixorigin:mainfrom
gouhongshen:feat/per-user-db-isolation

Conversation

@gouhongshen
Copy link
Copy Markdown
Contributor

Summary

  • remove the hardcoded FOR ACCOUNT sys from execute-path pre-migration account snapshots
  • align legacy single-db safety snapshots to use the same current-tenant account snapshot form
  • update the git-for-data RFC examples to match the tenant-safe MatrixOne syntax

Validation

  • cargo check -p memoria-storage -p memoria-cli
  • real memoria migrate legacy-to-multi-db --execute on a non-sys tenant test DB
  • inspected migrate-run.json: no warnings, no row mismatches
  • SQL spot-check: sampled mem_user_registry mapping and mem_memories counts match legacy/user DB

gouhongshen and others added 14 commits April 3, 2026 14:54
Add the per-user database isolation RFC covering shared-vs-user DB split, restore safety, snapshot/branch handling, migration, and metadata reconciliation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 4, 2026 07:57
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates snapshot/migration behavior to be tenant-safe and adds multi-database (“shared DB + per-user DB”) routing support across the storage layer, service layer, CLI, MCP tools, and API endpoints.

Changes:

  • Remove hardcoded FOR ACCOUNT sys snapshot usage and align snapshot tooling to current-tenant / DB-scoped semantics.
  • Introduce DbRouter + legacy single-DB → multi-DB migration utilities, and wire routing through service/API/MCP paths.
  • Expand/adjust integration tests and RFC documentation examples to reflect the new snapshot + routing behavior.

Reviewed changes

Copilot reviewed 35 out of 36 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
memoria/docs/git-for-data-rfc.md Updates snapshot SQL examples (but still needs alignment with DB-scoped implementation).
memoria/crates/memoria-test-utils/src/lib.rs Adds MySQL readiness helper for integration tests.
memoria/crates/memoria-test-utils/Cargo.toml Adds sqlx dependency for readiness helper.
memoria/crates/memoria-storage/tests/router_multi_db.rs Adds integration test validating user isolation across routed DBs.
memoria/crates/memoria-storage/src/router.rs Introduces DbRouter for user→DB routing with caches and provisioning.
memoria/crates/memoria-storage/src/migration.rs Adds legacy single-DB → multi-DB migration planner/executor with snapshots.
memoria/crates/memoria-storage/src/lib.rs Exports new migration/router APIs.
memoria/crates/memoria-storage/Cargo.toml Adds sha2 dependency for deterministic user DB naming.
memoria/crates/memoria-service/tests/plugin_repository.rs Extends service config used in tests for new fields.
memoria/crates/memoria-service/src/service.rs Routes per-user operations via DbRouter; adjusts access counter + retrieval param lookups.
memoria/crates/memoria-service/src/scheduler.rs Updates test config initialization for new config fields.
memoria/crates/memoria-service/src/pipeline.rs Makes persist phase resolve the correct per-user SQL store.
memoria/crates/memoria-service/src/governance.rs Executes governance operations across routed per-user stores when enabled.
memoria/crates/memoria-service/src/config.rs Adds shared_db_url + multi_db config and helpers.
memoria/crates/memoria-mcp/tests/snapshot_e2e.rs Updates snapshot tests for scoped naming + adds concurrency/multi-db rollback tests.
memoria/crates/memoria-mcp/tests/edit_log_e2e.rs Updates safety snapshot naming expectations to include DB scope.
memoria/crates/memoria-mcp/tests/branch_e2e.rs Adds merge/replace regression test and test embedder wiring.
memoria/crates/memoria-mcp/src/tools.rs Routes tool operations through per-user SQL stores.
memoria/crates/memoria-mcp/src/git_tools.rs Scopes snapshot names by DB, adds duplicate-create coalescing, and improves safety snapshot visibility.
memoria/crates/memoria-git/src/service.rs Creates DB-scoped snapshots and filters snapshot listing by database.
memoria/crates/memoria-cli/src/main.rs Adds memoria migrate legacy-to-multi-db command; wires multi-db server startup + URL redaction.
memoria/crates/memoria-api/tests/pool_isolation.rs Fixes db name parsing to ignore URL suffixes.
memoria/crates/memoria-api/tests/api_e2e.rs Strengthens snapshot detail/diff assertions and adds structured branch list test.
memoria/crates/memoria-api/tests/api_db_verify.rs Uses DB readiness helper for more reliable CI startup.
memoria/crates/memoria-api/src/state.rs Routes background flushers through MemoryService for correct routing in multi-db mode.
memoria/crates/memoria-api/src/routes/snapshots.rs Enhances snapshot endpoints with structured JSON and multi-db-aware snapshot resolution.
memoria/crates/memoria-api/src/routes/sessions.rs Makes session summary reads branch-aware by using the active table.
memoria/crates/memoria-api/src/routes/metrics.rs Makes metrics collection multi-db aware by aggregating per-user DBs.
memoria/crates/memoria-api/src/routes/memory.rs Uses per-user SQL store for profile/history/retrieval param routes.
memoria/crates/memoria-api/src/routes/governance.rs Uses per-user SQL store and makes cooldown keys user-scoped.
memoria/crates/memoria-api/src/routes/auth.rs Uses shared/auth pool selection (no longer runs per-route DDL).
memoria/crates/memoria-api/src/routes/admin.rs Updates admin stats and per-user operations for multi-db routing.
memoria/crates/memoria-api/src/auth.rs Routes tool usage + call log batch flushes via MemoryService (per-user in multi-db).
memoria/Cargo.lock Locks new dependencies (sha2, sqlx in test utils).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 5 to 10
### Snapshots (account-level)
```sql
CREATE SNAPSHOT <name> FOR ACCOUNT sys
CREATE SNAPSHOT <name> FOR ACCOUNT
SHOW SNAPSHOTS
RESTORE ACCOUNT sys FROM SNAPSHOT <name>
RESTORE ACCOUNT FROM SNAPSHOT <name>
DROP SNAPSHOT <name>
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

The RFC still documents snapshots as account-level (CREATE SNAPSHOT <name> FOR ACCOUNT / RESTORE ACCOUNT ...), but the implementation now creates DB-scoped snapshots (CREATE SNAPSHOT <name> FOR DATABASE <db>) and SHOW SNAPSHOTS results are filtered to the current database. This doc section should be updated to avoid suggesting account-level snapshot/restore semantics that won’t match the current Rust implementation.

Copilot uses AI. Check for mistakes.
.ok_or((StatusCode::SERVICE_UNAVAILABLE, "SQL store required".into()))?;
let sql = user_snapshot_store(&state, &user_id).await?;
let pool = sql.pool();
let table = sql.active_table(&user_id).await.map_err(api_err)?;
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

Snapshot API reads are now based on the user’s active branch table (let table = sql.active_table(...)). This can make /v1/snapshots/:name and /v1/snapshots/:name/diff return branch-table snapshot contents when the user is checked out to a branch, which is inconsistent with rollback behavior (rollback always restores mem_memories). Consider hardcoding these snapshot detail/diff queries to mem_memories (or otherwise explicitly define the intended semantics) so snapshot reads don’t depend on the current checkout state.

Suggested change
let table = sql.active_table(&user_id).await.map_err(api_err)?;
let table = "mem_memories";

Copilot uses AI. Check for mistakes.
Comment on lines 69 to +76
pub async fn create_snapshot(&self, name: &str) -> Result<Snapshot, MemoriaError> {
let safe = validate_identifier(name)?;
exec_ddl(
&self.pool,
&format!("CREATE SNAPSHOT {safe} FOR ACCOUNT sys"),
&format!(
"CREATE SNAPSHOT {safe} FOR DATABASE {}",
quote_identifier(&self.db_name)
),
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

create_snapshot() relies on validate_identifier() which currently accepts any Unicode alphanumeric characters (char::is_alphanumeric). Elsewhere (e.g., memoria-mcp git tools) identifier validation is ASCII-only, and MatrixOne identifiers are typically ASCII-centric. Tightening validation to is_ascii_alphanumeric() (plus underscore) would make behavior consistent and avoid runtime DDL failures for identifiers that pass validation but are rejected by the database.

Copilot uses AI. Check for mistakes.
@gouhongshen
Copy link
Copy Markdown
Contributor Author

Superseded by #169 to avoid conflicts from the previously merged feature-branch history.

@gouhongshen gouhongshen closed this Apr 4, 2026
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.

2 participants