fix(storage): make save and export crash-safe (fixes #282)

[sshekhar563]

Description

Fixes #282 — Soul save and export are not crash-safe; a crash can destroy the only copy.

The Bug

save_soul_full() and save_soul_flat() used shutil.rmtree() / unlink() to delete the existing soul directory before moving new data into place. A crash between the delete and the move destroys the only readable copy. Soul.export() used Path.write_bytes() directly, which truncates the file on open — a mid-write crash leaves a corrupt, partial archive.

The Fix

Function	Before	After
save_soul_full()	rmtree → move	temp dir → fsync → _replace_with_backup
save_soul_flat()	per-file delete → move	temp dir → fsync → whole-directory swap
Soul.export()	Path.write_bytes()	write_bytes_atomic() (temp + fsync + swap)

New crash-safe helpers in storage/file.py:

• _replace_with_backup(source, target) — moves existing target to .bak, swaps source into place, restores .bak if swap fails
• write_bytes_atomic(path, data) — writes to temp file, fsync, atomic replace via _replace_with_backup
• _fsync_directory(path) — platform-aware dir fsync (no-op on Windows where os.open() on dirs raises PermissionError)
• _fsync_tree(path) — fsyncs all files and directories under a generated soul directory

Tests Added

[prakashUXtech]

Strong work on the mechanism. You create the temp in the target's parent so os.replace always runs on the same filesystem (that's the atomic-write bug most people miss, and you got it right), you fsync the files and the directory before the swap, and moving the old directory to .bak first is the correct way to make the directory case work on Windows. For an in-process failure like a full disk, the original is correctly restored, and I verified that path.

Two things need resolving before this is crash-safe in the way the title promises.

1. The power-loss case still loses the soul. _replace_with_backup swaps in two steps: move the target to .bak, then move the new copy into place. Between those two steps the canonical path doesn't exist. If the process dies there, the good data is sitting at <path>.bak, but no loader knows to look there, so the soul reads as "not found." Worse, the next save sees the target missing, hits the if backup.exists(): remove branch and deletes the .bak, and writes no new backup because the target wasn't there to back up, so the one good copy is gone. Could you add load-time recovery: when the canonical path is missing and a sibling .bak exists, promote the .bak? And avoid deleting a .bak when the target is absent. A test that sets up target-absent plus .bak-present and asserts the soul still loads would pin this down.

2. save_soul_flat now replaces the whole directory, which silently changes a documented contract. The old flat save merged per-item specifically to preserve other files in the folder (it's in the wiki as intentional). Since save_local points at a user-supplied path, the wholesale replace can delete unmanaged files a user keeps alongside their soul (notes, a .git/, an earlier keys/ or trust_chain/ the current save doesn't regenerate). Either keep the preserve-existing-files behavior, or make the wholesale replace deliberate with a test and a wiki update.

Two smaller, non-blocking notes. Every successful export() now leaves a .soul.bak next to the archive, which doubles disk for large souls and can leave a previous soul's full memory sitting next to a file you meant to share, so consider scrubbing it after a successful export. And for the file case specifically, os.replace already overwrites atomically in one step, so moving to .bak first is actually less atomic than a plain replace would be; hardlinking or copying to .bak before the replace keeps a backup with zero window (the move-first is only needed for the directory case).

Also add the Updated: … (#282) header lines and update the atomic-writes wiki page, since the mechanism it describes has changed.

This takes the issue from "a crash destroys the only copy" to "a crash needs manual recovery," which is a real improvement. Closing the load-time recovery gap gets it the rest of the way.

[prakashUXtech]

This is a real step up from the last round. The blocker is genuinely fixed now, end to end. Soul.awaken and load_soul_full both call _recover_backup() before the not-found check, so a soul left in a .bak by a power cut actually comes back, and there's a real test that sets up target-absent plus .bak-present and asserts the soul loads and the canonical path is restored. That was the exact test missing last time. The "delete the only copy" footgun is gated away too. My other concern is resolved as well: save_soul_flat now merges per file and scopes cleanup to memory, trust_chain, and keys, so a user's unmanaged files (a NOTES.md, a .git/) survive. Nicely done.

A few things remain, mostly polish.

1. The single-file write path is less atomic than it needs to be. write_bytes_atomic (and therefore export) routes through the move-old-to-.bak-first swap, which creates the exact absent-path window that _recover_backup then has to patch. For one file, os.replace(tmp, target) after fsync is atomic on its own: the path always resolves to old or new, never missing, no recovery needed. Since crash-safety is the whole point, the file path should be the plain temp + fsync + replace, and if you still want a previous-version backup, make it by copy before the replace rather than by moving the original aside. The directory path genuinely needs the backup dance (you can't swap a directory's contents in one syscall), so keep that as is.

2. One new edge case: the keys/ cleanup can delete keys/private.key. If someone runs save_local(include_keys=False) after a prior save that included keys, the current memory_data["keys"] has no private.key, so cleanup prunes it. The old per-item merge left it alone. Could you exclude keys/private.key from pruning, or confirm the scrub is intended?

3. Two of the new tests don't exercise what their names claim. test_recover_backup_race_safe defines a simulate_race helper but never uses it, so the FileNotFoundError branch (the actual race-safety code) is never hit. And test_replace_with_backup_does_not_delete_orphan_bak only checks the target has new data, which passes even with the old buggy unconditional delete, so it doesn't guard the regression. The code behind both is correct; the tests just need to trigger the failure path (monkeypatch os.replace to raise, then assert).

Non-blocking: every successful export() leaves a .soul.bak next to the archive, so for a file you mean to share, the previous soul's full memory sits beside it. The issue did suggest keeping a backup, so it's a judgment call, but for export specifically I'd lean toward defaulting keep_backup=False or scrubbing after. And load_soul / FileStorage.load don't route through recovery, so a short note that awaken and load_soul_full are the crash-safe entry points would help.

The P1 win is the real story here, and it's solid. This is the last mile.

[prakashUXtech]

Two of the three are genuinely fixed. The single-file write path is now a plain os.replace after fsync with a copy-based backup, so the canonical file is never missing, and the move-first dance is correctly confined to the directory case. Export passes keep_backup=False so it no longer litters a .soul.bak. And the two previously-vacuous tests now actually monkeypatch os.replace to raise and assert the right thing. Good iteration.

The one that still blocks is the private-key guard, and it's a real data-loss path that currently looks fixed but isn't. The prune-protected set names keys/private_key and keys/private.pem, but the actual on-disk file is keys/private.key with a dot (it's PRIVATE_KEY_FILENAME in crypto/keystore.py:36, and your own file header says keys/private.key). Since Path("keys/private.key") isn't in that set, save_local(include_keys=False) after a prior key-bearing save still unlinks the private signing key, which is irreversible: the soul becomes verification-only. And test_save_soul_flat_preserves_private_key creates keys/private_key (the wrong name), so it passes green while the real file stays unprotected, the same false-green shape as the two tests you just fixed.

The fix is small: import PRIVATE_KEY_FILENAME from crypto.keystore and protect Path("keys") / "private.key" (the private_key and private.pem entries are dead code, nothing writes them), and retarget the test to the real filename so it fails until the guard is right.

Non-blocking notes: FileStorage.load still doesn't route through recovery, but FileStorage.save writes in place and never makes a .bak, so that path is simply outside this PR's crash-safety scope, worth one line saying so. And there's a redundant local import shutil at file.py:182 that's already imported at the top.