Pull request overview

This PR introduces a multi-device sync system built on the LanceDB backend, including sync metadata injection, a version-vector based sync engine, and optional FastAPI-based sync server/client support. It also updates the project to make LanceDB + pluggable embedders the default storage/embedding path and refreshes CLI/docs/tests accordingly.

Changes:

• Add sync metadata primitives (node_id, seq, updated_at) and a version-vector sync engine with server/client + CLI commands (serve, sync).
• Add pluggable embedding backends (default ONNX; optional sentence-transformers / Ollama) and a DB abstraction layer (LanceDB default, ChromaDB optional).
• Update documentation, benchmarks, and tests to reflect v4.0 behavior and the new backends.

Reviewed changes

Copilot reviewed 37 out of 40 changed files in this pull request and generated 9 comments.

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

The sync design is solid — version vectors + last-writer-wins is the right tradeoff for a local-first memory tool where conflicts should be rare and data loss on rare conflicts is acceptable. The hub-and-spoke propagation fix (exporting records from any node, not just the local one) is essential for multi-client scenarios.

A few things worth discussing:

get_changes_since() scans all records: Every sync call does col.get(limit=100_000, ...) to find what's changed. This is O(N) in palace size regardless of how much has changed since last sync. For a 1,000-drawer palace this is fine. For a 100,000-drawer palace (achievable with a year of daily mining), this is 100k records over the wire on every sync interval. A seq-indexed query would be more scalable: col.get(where={"seq": {"$gt": remote_vv[our_node_id]}}) — though this only works for local node's records, not hub relay. For hub-and-spoke you'd need $or across all known node IDs, which is doable with the where-clause translator from #574.

VersionVector._save() is synchronous on every update: During apply_changes(), update_from_records() calls _save() once for each record that advances the vector. On a 500-record changeset, that's potentially 500 file writes. Consider accumulating changes and saving once at the end.

_remote_wins tiebreak by node_id string comparison: This is deterministic, which is what you want. Just worth documenting explicitly that this means the same record edited on two devices simultaneously will always prefer the lexicographically higher node ID — callers who care should ensure node IDs are stable across reinstalls (which NodeIdentity appears to handle via file-based persistence).

apply_changes() passes _raw=True to upsert(): I don't see _raw in the LanceCollection.upsert() signature in #574. This will either be ignored (if **kwargs absorbs it) or raise a TypeError. Needs coordination with #574.

Windows portability: The PR description says sync_meta.py avoids importing fcntl at module level — good. The msvcrt.locking path is the right approach for Windows.

606 tests, 44 sync-specific: That test count looks right for the scope here. The hub-and-spoke relay test and bidirectional sync test are the most important ones to have.

Conceptually this is the right direction. The $raw=True issue and the O(N) scan are the two things I'd want resolved before merge (the VV save frequency is a perf nit, not a blocker). Also flagging: this depends on #574 so merge order matters — I'd recommend the base PR (#574) land and this rebases cleanly before any merge attempt.

After the rebase and the new fix(sync): correct replication semantics and portability commit, the remaining issues from my earlier review are resolved:

Hub-and-spoke export — get_changes_since now exports from all nodes, not just local. This is the critical fix for multi-client scenarios where hub must relay client A's records to client B.

Timestamp comparison — _remote_wins parsing as timezone-aware datetimes (handling Z/+00:00/+05:00 offsets) replaces the fragile string comparison. String comparison would have silently broken any mixed-timezone sync.

Split upsert for records with/without embeddings — avoiding the whole-batch vector drop is correct. The earlier path would have silently lost provided vectors if any record in the batch was vector-less.

Cross-platform locking (fcntl→msvcrt on Windows) — right call. fcntl is Unix-only and would have broken Windows installs silently.

Pending merge of #574 first (dependency constraint), this is ready. Marking approved.

[MemPalace-AGI integration — 215 tests, 710 KG entities]

Multi-device sync via version vectors is the right protocol choice for this use case — CRDT-style convergence without a central coordinator. The hub-and-spoke model is practical for the common case (one main machine + mobile/secondary).

Notes from the design:

Depends on #574: This PR stacks on the LanceDB backend migration. The sync metadata injection (every write gets node_id, seq, updated_at) won't work with ChromaDB's metadata model without changes. The dependency should be explicit in the PR title/description, and CI should gate this on #574's tests passing.

get_changes_since scope: This was flagged in the #529 review as a potential bug — if it uses a wall-clock timestamp for change detection rather than the monotonic sequence counter, clock skew between devices will cause sync gaps. The sequence counter (seq) should be the authoritative sync cursor, not updated_at.

Version vector conflict resolution: The PR description mentions "version vector protocol" but doesn't detail the conflict resolution policy. When two devices write to the same drawer independently (network partition), which wins? Last-writer-wins by updated_at is the simplest policy but requires NTP accuracy. Per-field merging is complex. Worth documenting the chosen policy explicitly.

NodeIdentity file locking: The fcntl/msvcrt split for cross-platform file locking is the right approach. Worth confirming the Windows path (msvcrt) handles the case where the identity file is on a network mount (some network filesystems don't honor byte-range locks).

Embedding sync: If two devices embed the same text with different embedding models (e.g. one has GPU, one uses CPU default), the vectors will differ even for identical content. The sync engine needs to either standardize the embedding model or store the model name as metadata and refuse to merge drawers with different embedding models.

This is ambitious scope and the design is sound at the architecture level. The conflict resolution policy and embedding sync story are the two things that need explicit answers before production use.

[MemPalace-AGI integration — 215 tests, 710 KG entities]

Thanks for the rebase and the additional fixes in #574. Re-reviewing after the force-push.

The three performance commits from yesterday are clean. On the _raw=True API mismatch I flagged earlier — if #574's upsert() now correctly accepts that parameter (via the new db abstraction layer), this is resolved. Worth a quick cross-check that the combined test run on both PRs in sequence passes, specifically the sync integration tests that touch upsert.

The new PLAN.md and UPGRADE.md additions are helpful context for reviewers, and the v4 benchmark data in results_v4_comparison.json is a strong justification for the LanceDB default. The LanceDB numbers (R@5 0.992 vs ChromaDB 0.966) are compelling — worth calling that out in the PR description.

LGTM pending #574 merge coordination — same position as before but cleaner after the rebase.