fix(config): migrate remaining config.json writes to atomic _atomic_write_json#262
Merged
fix(config): migrate remaining config.json writes to atomic _atomic_write_json#262
Conversation
Closes wizard-leftovers follow-up #4 (atomic-write migration). PR #259 introduced _atomic_write_json (tempfile + os.replace with tmp cleanup on failure) and used it only for `mm config unset`. Two other paths still called `path.write_text(json.dumps(...))` directly, so a mid-write failure could corrupt config.json: - save_config_overrides (config.py) — every CLI set / Web UI PATCH / MCP mem_config save ultimately routes here. - _write_config_and_summary (cli/init_cmd.py) — both normal init and --fresh write through the same final statement. The --fresh path is the worse case: shutil.copy2 backup could succeed, then write_text could fail halfway, leaving a half-written config.json next to a valid .bak, requiring manual recovery. Swapping both call sites to _atomic_write_json gives the same guarantee --fresh already asked for: either config.json is wholly updated or it is byte-identical to what it was before the call. The backup semantics are unchanged (still copy2 before the atomic write), so the failure-mode outcome improves to "backup == current original, user can delete backup if desired" from the previous "backup is the only valid state, original is garbage." Helper signature (path: Path, data: dict) matches both call sites exactly — no helper changes needed. The happy-path behaviour is bit-identical (same json.dump args via the helper), so the existing 8 --fresh tests and every TestSaveConfigOverrides test pass unchanged. Redundant path.parent.mkdir call in save_config_overrides removed — the helper handles it. Tests (2 new, failure-mode only; happy path is already covered upstream): - TestSaveConfigOverrides::test_save_atomic_on_replace_failure — mock os.replace to raise; assert existing config.json byte-identical and no orphan .config.*.tmp in the parent directory. - TestFreshFlag::test_fresh_atomic_write_failure_keeps_backup_and_original — same mock applied through the --fresh path; assert both the pre-fresh original and the .bak-<ts> survive intact.
The mkdir removal in the previous commit delegated directory creation to _atomic_write_json, but every existing TestSaveConfigOverrides case writes into the isolated fixture's tmp_path — which pytest creates up front — so the removal was not structurally verified. Dropping the helper's mkdir too would still pass the full suite. Add test_save_creates_parent_directory_if_missing that points _override_path at a nested path whose ancestors don't exist, calls save_config_overrides, and asserts both the directory and file are created. Locks in "the helper owns config-dir creation."
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Closes wizard-leftovers follow-up #4 (atomic-write migration).
PR #259 introduced
_atomic_write_json(tempfile +os.replacewithtmp cleanup on failure) and used it only for
mm config unset. Twoother paths still called
path.write_text(json.dumps(...))directly,so a mid-write failure could corrupt
config.json:save_config_overrides(config.py) — everymm config set, WebUI PATCH, MCP
mem_configsave, andmemory-dirs/add|removeroutes here.
_write_config_and_summary(cli/init_cmd.py) — both normalinitandinit --freshshare the same final write statement.The
--freshpath is the worse case:shutil.copy2backup couldsucceed, then
write_textfail halfway, leaving a half-writtenconfig.jsonnext to a valid.bak, requiring manual recovery.Swapping both call sites to
_atomic_write_jsongives the guarantee--freshalready asked for: eitherconfig.jsonis wholly updatedor it is byte-identical to what it was before the call. Backup
semantics unchanged (still
copy2before the atomic write), so thefailure-mode outcome improves to "backup == current original, user
can delete backup if desired" from "backup is the only valid state,
original is garbage."
Helper signature fits both call sites
_atomic_write_json(path: Path, data: dict)usesindent=2+ theshared
_json_defaultfallback internally — bit-identical to whatboth former call sites passed to
json.dumps. No helper changesneeded. The redundant
path.parent.mkdirinsave_config_overrideswas removed since the helper does it.
Test layering
Three layers together cover the atomic-write contract:
_atomic_write_jsonitself preserves originals onos.replacefailure and cleans up its
.config.*.tmpon success. Scope: thehelper in isolation.
call sites propagate that guarantee correctly: a mid-write failure
through
save_config_overridesor--freshleaves realconfig.json/.bakstate intact, with no orphan tmp.owns config-dir creation; removing the
mkdirfrom either sidewithout a replacement would be caught.
Happy path is already covered bit-identically by the 8 existing
TestFreshFlagtests and every test inTestSaveConfigOverrides;those pass unchanged.
Tests (3 new)
TestSaveConfigOverrides::test_save_creates_parent_directory_if_missing—point
_override_pathat a nested path whose ancestors don'texist; assert
save_config_overridescreates both directory andfile. Guards the
path.parent.mkdirremoval.TestSaveConfigOverrides::test_save_atomic_on_replace_failure—monkeypatch
os.replaceto raise, write a config with anon-comparand field; assert existing
config.jsonstaysbyte-identical and no
.config.*.tmporphan intmp_path.TestFreshFlag::test_fresh_atomic_write_failure_keeps_backup_and_original—same mock through the
--freshpath; assert both the pre-freshconfig.jsonand the.bak-<ts>survive intact and no tmp orphanremains in
~/.memtomem/.Scope
Small, mechanical tail of the wizard-leftovers sequence — 2
call-site swaps, 3 tests, no behaviour change on the happy path.
+99 / -4.Test plan
uv run pytest -m "not ollama" -q— 1715 passeduv run ruff check packages/memtomem/src packages/memtomem/testsuv run ruff format --check packages/memtomem/src packages/memtomem/testsuv run mypy packages/memtomem/src— no issues