fix(repair): use os.rename for in-place archive, not shutil.move - #1945
Merged
igorls merged 1 commit intoJul 8, 2026
Merged
Conversation
shutil.move's fallback for a failed os.rename is copytree + rmtree. On Windows, when any file inside the palace is held open by another process (a live MCP server, a running mine, another harness), the rename fails and shutil.move falls back to deleting the live palace file-by-file via rmtree -- which itself then fails partway through on the first locked file, leaving the palace partially gutted next to a partial (or empty) archive copy. Reproduced live twice (Windows 11, 2026-07-05 and 2026-07-06): running `mempalace repair --mode from-sqlite --yes --archive-existing` while an MCP server / detached mine held palace/*/data_level0.bin open threw mid-rmtree in both cases. The palace itself survived only because the specific locked files could not be unlinked -- a different lock pattern (e.g. a lock on a file rmtree reaches first) would have lost data with no way back. os.rename is atomic on both platforms it matters on (POSIX rename(2), Windows MoveFileEx) -- it either fully succeeds or fails without touching anything. Catch the failure and abort cleanly with actionable guidance instead of a raw traceback.
3 tasks
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
rebuild_from_sqlite's in-place archive step (--archive-existing) usesshutil.move(dest_palace, archive_path). Whenos.renamefails (cross-filesystem, or on Windows when a file inside the tree is held open by another process),shutil.movefalls back tocopytree+rmtree. Thatrmtreedeletes the live palace file-by-file — and if it hits a locked file partway through, the palace is left partially deleted next to a partial (or empty) archive copy.Real-world repro
Hit this live, twice, on two separate nights (Windows 11, 2026-07-05 and 2026-07-06): running
mempalace repair --mode from-sqlite --yes --archive-existingwhile an MCP server / a detached mine process heldpalace/<segment-uuid>/data_level0.binopen. Both times:The palace survived only because the specific locked files happened to be ones
rmtreecouldn't unlink — a different lock pattern (a lock on whichever filermtreereaches first) would have lost data with no way back, since the archive itself is also incomplete at that point.Fix
os.renameinstead ofshutil.movefor this step. It's atomic on both platforms this matters on (POSIXrename(2), WindowsMoveFileEx) — either fully succeeds or fails touching nothing. Catch the failure and print actionable guidance (close the process holding the lock, retry) instead of a raw traceback or a silently mangled palace.Testing
test_rebuild_from_sqlite_in_place_archive_failure_leaves_palace_untouched(monkeypatchesos.renameto raise, matching the real Windows error) — asserts the palace directory andchroma.sqlite3are byte-for-byte untouched and no archive dir is created.tests/test_repair.py: 91 passed, 0 failed.Related to the general lock-contention family (#1908, #1888) but this is specifically about the archive-move fallback behavior, not the peer-writer lock itself.