Skip to content

fix(config): migrate remaining config.json writes to atomic _atomic_write_json#262

Merged
memtomem merged 2 commits intomainfrom
fix/atomic-write-migration
Apr 18, 2026
Merged

fix(config): migrate remaining config.json writes to atomic _atomic_write_json#262
memtomem merged 2 commits intomainfrom
fix/atomic-write-migration

Conversation

@memtomem
Copy link
Copy Markdown
Owner

@memtomem memtomem commented Apr 18, 2026

Summary

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 mm config set, Web
    UI PATCH, MCP mem_config save, and memory-dirs/add|remove
    routes here.
  • _write_config_and_summary (cli/init_cmd.py) — both normal
    init and init --fresh share the same final write statement.
    The --fresh path is the worse case: shutil.copy2 backup could
    succeed, then write_text 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 guarantee
--fresh already asked for: either config.json is wholly updated
or it is byte-identical to what it was before the call. Backup
semantics unchanged (still copy2 before the atomic write), so the
failure-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) uses indent=2 + the
shared _json_default fallback internally — bit-identical to what
both former call sites passed to json.dumps. No helper changes
needed.
The redundant path.parent.mkdir in save_config_overrides
was removed since the helper does it.

Test layering

Three layers together cover the atomic-write contract:

  1. Helper self-tests (shipped in PR feat(cli): add mm config unset for targeted override removal #259) — establish that
    _atomic_write_json itself preserves originals on os.replace
    failure and cleans up its .config.*.tmp on success. Scope: the
    helper in isolation.
  2. This PR's integration failure tests — confirm the two migrated
    call sites propagate that guarantee correctly: a mid-write failure
    through save_config_overrides or --fresh leaves real
    config.json / .bak state intact, with no orphan tmp.
  3. Structural parent-dir guard — locks in that the helper now
    owns config-dir creation; removing the mkdir from either side
    without a replacement would be caught.

Happy path is already covered bit-identically by the 8 existing
TestFreshFlag tests and every test in TestSaveConfigOverrides;
those pass unchanged.

Tests (3 new)

  • TestSaveConfigOverrides::test_save_creates_parent_directory_if_missing
    point _override_path at a nested path whose ancestors don't
    exist; assert save_config_overrides creates both directory and
    file. Guards the path.parent.mkdir removal.
  • TestSaveConfigOverrides::test_save_atomic_on_replace_failure
    monkeypatch os.replace to raise, write a config with a
    non-comparand field; assert existing config.json stays
    byte-identical and no .config.*.tmp orphan in tmp_path.
  • TestFreshFlag::test_fresh_atomic_write_failure_keeps_backup_and_original
    same mock through the --fresh path; assert both the pre-fresh
    config.json and the .bak-<ts> survive intact and no tmp orphan
    remains 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 passed
  • uv run ruff check packages/memtomem/src packages/memtomem/tests
  • uv run ruff format --check packages/memtomem/src packages/memtomem/tests
  • uv run mypy packages/memtomem/src — no issues

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."
@memtomem memtomem merged commit 962efbd into main Apr 18, 2026
7 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 18, 2026
@memtomem memtomem deleted the fix/atomic-write-migration branch April 20, 2026 14:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants