Skip to content

fix(entity_registry): atomic write to prevent partial corruption on crash - #1215

Merged
igorls merged 2 commits into
MemPalace:developfrom
arnoldwender:fix/entity-registry-atomic-write
May 6, 2026
Merged

igorls merged 2 commits into
MemPalace:developfrom
arnoldwender:fix/entity-registry-atomic-write

Conversation

@arnoldwender

Copy link
Copy Markdown
Contributor

What and Why

EntityRegistry.save() calls Path.write_text() directly, which truncates the target file and then writes. A crash between truncate and full-flush (power loss, OOM, filesystem full, kill -9) leaves an empty or half-written entity_registry.json. The whole people/projects map is lost; the system silently falls back to an empty registry on next load() (json.JSONDecodeError is swallowed in load()).

For a tool whose value is built up from months of mining transcripts and disambiguating entities, this is a real data-loss risk on any crash.

Root Cause

mempalace/entity_registry.py:323self._path.write_text(...) is not atomic. The kernel sees: open(..., 'w') (truncates), then write, then close. Any failure between truncate and successful close corrupts the file.

Fix

Standard atomic-write pattern:

  1. Serialize to a sibling .tmp file in the same directory (so os.replace stays on one filesystem)
  2. fsync the temp file
  3. chmod 0o600 the temp file before rename
  4. os.replace(tmp, target) — atomic on POSIX and Windows

Any crash before os.replace returns leaves the previous registry intact. The new registry only becomes visible to readers when the rename has fully landed on disk.

Test plan

  • test_save_is_atomic_does_not_leave_tmp.tmp sidecar removed on successful save
  • test_save_preserves_previous_on_serialization_failure — previous content unchanged when os.replace raises mid-save (simulates filesystem-full or permission flip after temp write)
  • All 31 test_entity_registry.py tests pass
  • Full suite: 1318 passed locally (2 pre-existing test_backends.py::test_pin_hnsw_threads* failures unrelated — chromadb configuration_json["hnsw"] schema drift, also fails on clean develop)

@jphein

jphein commented Apr 27, 2026

Copy link
Copy Markdown
Collaborator

+1 to the atomic-write pattern; the truncate-then-write window in Path.write_text is the kind of thing that bites on power loss exactly when you can least afford to lose the file.

One small hardening that's worth considering for the rename-durability claim: os.replace is atomic with respect to readers seeing either the old or the new file, but on some Linux filesystems (notably ext4 with default mount options) the rename's durability across power loss requires an additional fsync on the parent directory. Without it, the kernel can ack the rename, then a crash reverts to a state where the temp file is present and the target is at the old version.

os.replace(tmp_path, self._path)
dir_fd = os.open(str(self._path.parent), os.O_RDONLY)
try:
    os.fsync(dir_fd)
finally:
    os.close(dir_fd)

For entity_registry.json specifically this is academic — the file is regenerated by mempalace init on demand — but the PR motivation frames the registry as months-of-accumulated-state, and at that bar the directory fsync is the standard finishing move (SQLite's atomic-commit doc spells it out). Cheap belt-and-suspenders if you want to take it; happy to land as-is otherwise since the core truncate-window fix is the load-bearing part.

arnoldwender added a commit to arnoldwender/mempalace that referenced this pull request Apr 28, 2026
Without this, on ext4 (and similar) filesystems the rename ack does not
guarantee durability across power loss — a crash can revert to a state
where the temp file is present and the target is at the old version.

Suggested by @jphein on MemPalace#1215.
@arnoldwender

Copy link
Copy Markdown
Contributor Author

@jphein Good catch — the parent-dir fsync is the natural completion of "atomic write means durable write." Even though entity_registry.json is regenerable, leaving the gap would be inconsistent with the PR framing.

Folded it in as 490f585. OSError is swallowed for Windows and special filesystems that reject directory fds (they have different durability semantics on rename anyway).

Test suite: 1317 passed. (Two unrelated pre-existing failures in test_backends.py HNSW retrofit reproduce on develop too — not from this change.)

Ready for re-review.

@jphein

jphein commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Thanks @arnoldwender490f585 looks right. Swallowing OSError for Windows and special filesystems is the right call (os.fsync(fd) on a directory has no portable Windows equivalent, and the rename durability semantics differ enough there that the per-platform skip is the cleanest path). The PR is now strict-stronger than my original review asked for — +1 to merge from my side once CI is green.

@arnoldwender

Copy link
Copy Markdown
Contributor Author

Friendly ping — CI is green across all platforms and @jphein gave +1 on 2026-04-30. Ready to merge whenever a maintainer has a moment.

…rash

EntityRegistry.save() called Path.write_text() directly, which truncates
the target file and then writes — so a crash mid-write (power loss, OOM,
filesystem-full mid-flush) leaves an empty or half-written
entity_registry.json. The whole people/projects map is lost; the system
falls back to an empty registry on next load.

Switch to the standard atomic-write pattern: serialize to a sibling
.tmp file in the same directory (so os.replace stays on one filesystem),
fsync, chmod 0o600, then os.replace over the target. The replace is
atomic on POSIX and Windows, so any crash leaves the previous registry
intact instead of a truncated file.

Tests cover: no leftover .tmp on success, and previous content preserved
when os.replace itself raises mid-save.
Without this, on ext4 (and similar) filesystems the rename ack does not
guarantee durability across power loss — a crash can revert to a state
where the temp file is present and the target is at the old version.

Suggested by @jphein on MemPalace#1215.
@arnoldwender
arnoldwender force-pushed the fix/entity-registry-atomic-write branch from 490f585 to 2e441d1 Compare May 4, 2026 09:08
@igorls
igorls merged commit e18981a into MemPalace:develop May 6, 2026
6 checks passed
igorls added a commit that referenced this pull request May 6, 2026
…1282 #1167 #1160

Bundled CHANGELOG entries for the seven Tier-1 PRs merged today, including
the behavior-change call-out for #1167 (KG date validators now reject
non-ISO inputs that previously produced silent empty results).
xcarbo added a commit to xcarbo/mempalace that referenced this pull request May 7, 2026
Catches up on a heavy upstream day — 22 fixes merged in 24h plus prior
backlog. Highlights pulled in:

- MemPalace#1305 hooks: ~/.mempalace/ deletion is now a stable kill-switch (hooks
  no longer rebuild the dir hierarchy on Stop/PreCompact/SessionStart)
- MemPalace#1214 KG: reject inverted intervals (valid_to < valid_from) at write time —
  prevents silently invisible triples
- MemPalace#1067/MemPalace#1105 chroma: ChromaBackend.close_palace() now actually releases
  the SQLite file lock (PersistentClient.close() on evict + invalidation)
- MemPalace#1215 entity_registry: atomic save (tmp+fsync+rename) — no more
  corruption on crash mid-write
- MemPalace#1073/MemPalace#1107 mempalace compress: paginated drawer fetch — no longer
  trips SQLITE_MAX_VARIABLE_NUMBER on palaces >32k drawers
- MemPalace#1282 stdio: Windows console UTF-8 reconfig for cli/mcp_server/hooks_cli
- MemPalace#1164/MemPalace#1167 mcp KG: sanitize_iso_date() blocks malformed date strings
  silently producing empty result sets
- MemPalace#1136/MemPalace#1160 mcp: per-path KG cache for multi-tenant hosts that rotate
  MEMPALACE_PALACE_PATH between tool calls
- MemPalace#1286 mcp: retry _get_collection() once on transient failure
- MemPalace#1138 lint cleanup, MemPalace#1019 search-crash fix
- 4 new tools/ scripts (backup_claude_jsonls, find_orphan_claude_jsonls,
  render_jsonl, save.md)

Conflict resolution (CHANGELOG.md only — code files all auto-merged):

- 3.3.5 section: untouched (already merged in our prior commit; upstream
  added several new bug-fix entries which auto-merged cleanly)
- 3.3.4 Bug Fixes: kept upstream's new MemPalace#1305 entry; preserved our richer
  detail on topic-tunnels (MemPalace#1194/MemPalace#1195/MemPalace#1197), HNSW-bloat (MemPalace#1191),
  max_seq_id (MemPalace#1135), and auto-ingest (MemPalace#1230/MemPalace#1231) — upstream's shorter
  topic-tunnels entry was a strict subset of ours.

xdev patches preserved (still on this branch, untouched by merge):
- 6ef44cb fix(hooks): route CC transcripts via convo_miner with cwd-based wings
- 3fad61d fix(config): allow leading dash in wing names
- 3fc821a fix(config): tighten leading-char to allow dash but not underscore

Tests: 1557 passed, 1 skipped (full unit suite excluding benchmarks).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@igorls igorls mentioned this pull request May 10, 2026
3 tasks
arnoldwender added a commit to arnoldwender/mempalace that referenced this pull request May 10, 2026
MemPalace#1373)

MemPalace#1215 made `EntityRegistry.save()` atomic via temp-file + fsync + os.replace.
Crash-mid-write durability is correct: the previous registry stays intact on
any failure. But if `f.write()` / `f.flush()` / `os.fsync()` / `os.replace()`
raise (disk full, perms flip, broken FUSE mount, IO error), the `.tmp` sidecar
was left on disk. Subsequent saves overwrite the same path so it does not
grow unboundedly, but it litters the palace directory and obscures
diagnostics — a user inspecting `entity_registry.json.tmp` after a crash
cannot distinguish in-flight writes from stale debris.

Wrap the write+chmod+replace block in try/except. On any exception, attempt
`tmp_path.unlink(missing_ok=True)` before re-raising. The dir-fsync is
deliberately outside the try — that is durability for a successful rename,
not a write step that needs cleanup.

Tests:
- Augment `test_save_preserves_previous_on_serialization_failure` to also
  assert the .tmp sidecar is gone after a forced os.replace failure.
- New `test_save_cleans_tmp_on_write_failure` forces os.fsync to raise,
  covering the gap between write and rename that the existing test does
  not exercise.

Closes MemPalace#1373.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants