feat: add JSONL export/import for cross-device palace sync - #2209
Conversation
Adds `mempalace export` (deterministic, git-friendly JSONL tree by wing/room; --format markdown exposes the existing browsable exporter) and `mempalace import` (merge by drawer id: adds new, skips existing, idempotent, re-embeds locally). Closes the multi-machine sync gap: export on one machine, sync the tree via a private git repo, import on another. Fixes MemPalace#452
- import --dry-run is now a parse-only preview: it never opens (or creates) the palace, since opening a local backend is itself a write - malformed-input tolerance widened: RecursionError on pathological JSON and non-UTF-8 files are counted and skipped, never fatal - concurrent-insert race on add degrades to per-item skip instead of aborting the batch - export groups by sanitized wing/room names, so names that sanitize to the same path merge into one file instead of overwriting - honest memory note in the JSONL export docstring
Round-2 review findings: the per-item fallback now checks existence AFTER a failed add (closing the second race and never masking a real backend error); unreadable inputs (IsADirectoryError, permissions, I/O) are counted like malformed lines instead of aborting; the exporter module docstring no longer claims bounded memory for the JSONL leg.
A backend error during a mid-file flush was caught by the file-read OSError handler, miscounted as malformed input, and left the batch pending. Files are now parsed under the guard and flushed outside it, so backend failures surface and filesystem failures stay per-file.
`import_palace` opened every `*.jsonl` glob hit with a plain `open()`. That is safe for the cases the surrounding comment named — a directory, bad permissions, bad UTF-8 — because each raises, and the `except OSError` books it as malformed. A FIFO does neither: opening one for reading parks in the kernel until a writer appears, so a named pipe carrying a `.jsonl` name wedges the whole import and no handler can see it. Adopt the guard the ingest side already uses (`miner._read_text_no_follow`, MemPalace#2221/MemPalace#2244): `O_NONBLOCK` makes the `S_ISREG` check reachable, `O_NOFOLLOW` keeps a symlinked entry from reading through to a target outside the import tree — matching the `_reject_symlink` posture the exporter applies on the write side — and the EAGAIN arm preserves the blocking read for a regular file whose write lease we broke. A non-regular entry is booked exactly like an unreadable one, so a partially hostile export still imports what survives. The test bounds the regression with SIGALRM, since a regression does not raise — it hangs. The alarm handler deliberately raises a non-OSError: the first version used TimeoutError, which IS an OSError subclass, so the importer's own handler swallowed it and the test passed against unfixed code.
Findings from two adversarial review passes over this round, each verified
before being acted on.
1. The no-follow guard was half a guard. O_NOFOLLOW checks only the final
path component, and `glob.glob(..., recursive=True)` traverses symlinked
DIRECTORIES — driven: a probe tree returned `linked_dir/a.jsonl` from
outside the root. So a symlinked leaf was refused while a regular file
reached through a symlinked directory was imported as if it belonged to
the export. `miner._read_text_no_follow` pairs its no-follow open with a
containment test for exactly this reason; only half of that convention
was adopted. `_path_within_root` adds the other half. The check is
resolve-then-open, so a symlink swapped in between the check and the open
still wins — that race needs per-component `openat` walking and is stated
as a limitation rather than half-closed.
2. Non-scalar metadata was dropped silently, on a premise that does not hold.
The filter's comment claimed the backend cannot store non-scalars, which is
true of Chroma and false of the palace generally: `sqlite_exact` serializes
metadata through an unrestricted `json.dumps`, so a palace on that backend
can hold a list or a nested dict. The exporter writes it out raw and the
importer discarded it without a word. The values are still dropped — this
import path stores scalars only — but they are counted in a new
`metadata_dropped` stat and reported. A `metadata` that is not a dict at
all is discarded WHOLE and counts as one drop; replacing it with `{}`
before counting reported zero for a total loss, which was the same silent
loss one level up. An absent `metadata` key is not a drop and reports zero.
Each fix carries a reversion control that fires: disabling containment fails
the outside-tree test, zeroing either drop count fails its metadata test, and
reverting the FIFO guard hangs the import until the test's alarm fires.
0b05bed to
470f1ca
Compare
|
Rebased onto Reconciling against the new base turned up that the importer predated the non-regular-file work in #2221 / #2207 / #2244 and had exactly the defect those fixed, so there are two hardening commits on top of the rebase.
Import reads are contained to the import tree. Non-scalar metadata is now reported rather than dropped silently. The filter's premise was that the backend cannot store non-scalars. That holds for Chroma and not for the palace generally — Each fix has a test that fails when the fix is reverted — four controls, all four fire. The FIFO test bounds the hang with Full suite on the rebased branch: 4303 passed, 6 failed, 31 skipped. All 6 failures reproduce on a clean Two things I did not change, both design calls rather than defects, and both yours to make:
Happy to take any of these in this PR or a follow-up, whichever you prefer. One process note: after the push the PR is |
Re-exporting rewrote the rooms that still exist and left the rest in place. The PR body already noted that as a git-diff wrinkle, but it is worse than cosmetic: import walks every `*.jsonl` under the tree without consulting `export-manifest.json`, so a room whose last drawer was deleted survives in a stale file and the next device re-imports it — the drawer comes back. Re-export now prunes room files it did not write, and removes wing directories the prune emptied. Two safety properties, both deliberate: * Pruning is gated on an `export-manifest.json` that existed BEFORE this run, captured before the manifest is rewritten. Exporting into an unrelated directory therefore deletes nothing on the first run — the gate is proof the tree is one of ours, not merely that it now contains our manifest. * Only regular files are unlinked; symlinks are skipped rather than followed, matching the `_reject_symlink` posture already applied on the write side. The zero-drawer path deliberately does NOT prune, and says so. An empty count is also what a palace that failed to open looks like, and silently deleting a good export on that reading is far worse than leaving a stale one behind — so it warns and leaves the tree alone. Also opens the collection with `read_only=True`, since export is a pure read. `create` is left at its default: flipping it changes what exporting a not-yet-existing palace does, which is a separate call from this one.
The module docstring claimed import "follows the same single-writer expectations as `mine`", but nothing enforced it: `import_palace` opened the collection and added drawers without taking `mine_palace_lock`, which the adapter write path holds at cli.py for exactly this. The existing-id check and the add are not one atomic operation, so a concurrent miner could file an id between them — the docstring was aspirational rather than true. The lease now spans the whole walk, not just the final flush, because batches are added AS files are read. The walk moved into `_import_into` so that scope is explicit and readable rather than an eighty-line indent. A dry run still takes no lease and opens nothing: acquiring one would create the palace directory the preview promises not to touch. Tests cover the pruning, the first-export gate, the zero-drawer warning, and that the lease is taken for a real import and not for a dry run. Each has a reversion control that fires.
|
Went ahead and did both of the two design calls I flagged at the end of the last comment, one scoped commit each — so either can be dropped or split into a follow-up without touching the other, if you'd rather keep this PR narrow. Re-export now prunes stale room files (
The zero-drawer path deliberately does not prune, and says so. An empty count is also what a palace that failed to open looks like, and silently deleting a good export on that reading is worse than leaving a stale one behind — so it warns and leaves the tree alone.
Same commit also opens the collection Four new tests (pruning, the first-export gate, the zero-drawer warning, and that a real import takes the lease while a dry run does not), each with a reversion control that fires against the unfixed code. Full suite on the branch: 4307 passed, 6 failed, 31 skipped — the same 6 pre-existing failures I flagged last time, unchanged and unrelated. PR body updated: the caveat about re-export leaving stale files is no longer true, so it has been replaced rather than left standing. |
1 participant
feat: add JSONL export/import for cross-device palace sync
What does this PR do?
Implements the recommended solution from #452: a portable, git-friendly export/import pair so a palace can be synced between machines through a private git repo.
mempalace export(default--format jsonl) streams every drawer into a JSONL tree organized by wing/room (<out>/<wing>/<room>.jsonl, plus anexport-manifest.jsonwith a format version and counts). Output is deterministic — drawers sorted by id, JSON keys sorted, no timestamps — so re-exporting an unchanged palace produces a byte-identical tree and an empty git diff.--format markdownexposes the existingexporter.export_palace(previously library-only) on the CLI.mempalace import <dir>merges an export into a palace: adds drawers whose ids are absent, skips existing ones, and is idempotent on re-import.--dry-runis a parse-only true preview — it never opens (or creates) the palace at all, since opening a local backend is itself a write (the same principle as the repair --dry-run performs the real rebuild in the default (legacy) mode #2144/repair --mode from-sqlite --dry-run ARCHIVES AND REBUILDS the palace (dry-run not honored) #2133 dry-run fixes). Malformed input (bad JSON, pathological nesting, non-UTF-8 files) is counted and reported, never fatal; a concurrent insert of the same id degrades to a skip rather than an abort. Imports into a missing palace create it, so first import on a fresh machine works.Scope is deliberately the narrow v1 from the issue discussion (idempotent hash-id dedup). Out of scope, per that thread: metadata-conflict detection for moved drawers, near-duplicate detection for divergent edits (#452 comments), binary backup (#448), and KG export (previously #435/#499). One known v1 caveat, stated rather than hidden: import accepts any
*.jsonltree without requiringexport-manifest.json(tolerant by design; the manifest is informational in format v1). That tolerance is also why re-export prunes room files it did not write — otherwise a room whose last drawer was deleted survives as a stale file and the next device re-imports it, resurrecting the drawer. The prune is gated on anexport-manifest.jsonthat existed before the run, so exporting into an unrelated directory deletes nothing, and it is skipped on a zero-drawer palace (which warns instead, since an empty count is also what a palace that failed to open looks like). The--formatenum currently coversjsonl(round-trippable) andmarkdown(one-way, browsable); new formats extend the same seam.How to test
New tests cover: export structure + byte-level determinism across two runs, full round-trip content fidelity, idempotent re-import, malformed-line tolerance, dry-run creating nothing, and non-directory input rejection. Later review rounds add coverage for the non-regular-file/FIFO guards, symlinked-directory containment, non-scalar metadata reporting, stale-export pruning with its first-run and zero-drawer gates, and the import writer lease. Each of those fixes carries a test that fails when the fix is reverted.
Checklist
python -m pytest tests/ -v)ruff check .)Fixes #452