From b3bdd5914f3968e4105610e2a28a047487a33a65 Mon Sep 17 00:00:00 2001 From: phernandez Date: Fri, 12 Jun 2026 00:25:09 -0500 Subject: [PATCH] fix(cli): write config.json atomically and isolate auto-update in mcp routing tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the test_mcp_sse_forces_local KeyError('FORCE_LOCAL') flake (#940, Python 3.14 leg): the stdio variants of the mcp routing tests invoke `bm mcp`, which starts a real background auto-update daemon thread before mcp_server.run (the tests only mock run). That thread hits PyPI and then rewrites config.json (auto_update_last_checked_at) with an in-place Path.write_text, which truncates the file before writing. If that write lands while a later test's CLI invocation is reading config.json (the app callback's CliContainer.create()), load_config() sees empty/partial JSON and raises SystemExit. CliRunner.invoke swallows it, the mocked mcp_server.run never executes, and the test dies with KeyError on env_at_run['FORCE_LOCAL'] — exactly the observed CI failure shape. Nothing is 3.14-specific; that leg only shifted the timing. The same torn write is user-visible in production: the MCP stdio server re-reads config.json on mtime change and load_config() exits the process on invalid JSON if it races a CLI save. Fix: save_basic_memory_config writes a per-process/per-thread sibling temp file and publishes it with os.replace, so readers always observe either the old or the new complete document. The regression test injects an interrupted write and asserts the published config stays untouched; it fails against the old in-place write. Test hardening: the stdio routing tests stub run_auto_update so no PyPI call or config write leaks across tests, and all four transport tests now assert result.exit_code == 0 so a future pre-run failure surfaces its real error instead of a KeyError. Refs #940 Co-Authored-By: Claude Fable 5 Signed-off-by: phernandez --- src/basic_memory/config.py | 18 +++++++- test-int/cli/test_routing_integration.py | 35 ++++++++++++-- tests/test_config.py | 59 ++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 6 deletions(-) diff --git a/src/basic_memory/config.py b/src/basic_memory/config.py index 6578bff5..92ff504a 100644 --- a/src/basic_memory/config.py +++ b/src/basic_memory/config.py @@ -4,6 +4,7 @@ import importlib.util import json import os import shutil +import threading from dataclasses import dataclass from datetime import datetime from enum import Enum @@ -1099,8 +1100,21 @@ def save_basic_memory_config(file_path: Path, config: BasicMemoryConfig) -> None _secure_config_dir(file_path.parent) # Use model_dump with mode='json' to serialize datetime objects properly config_dict = config.model_dump(mode="json") - file_path.write_text(json.dumps(config_dict, indent=2)) - _secure_config_file(file_path) + # Trigger: long-lived readers (MCP stdio server config reload, background + # auto-update threads) re-read config.json whenever its mtime changes, + # concurrently with CLI commands saving it. + # Why: writing the destination in place truncates it first, so a concurrent + # reader can observe empty/partial JSON and load_config() exits the process. + # Outcome: write a sibling temp file (unique per process/thread so parallel + # savers cannot interleave) and publish atomically via os.replace — readers + # always see either the old or the new complete document. (#940) + tmp_path = file_path.parent / f"{file_path.name}.{os.getpid()}.{threading.get_ident()}.tmp" + try: + tmp_path.write_text(json.dumps(config_dict, indent=2)) + _secure_config_file(tmp_path) + os.replace(tmp_path, file_path) + finally: + tmp_path.unlink(missing_ok=True) except Exception as e: # pragma: no cover logger.error(f"Failed to save config: {e}") diff --git a/test-int/cli/test_routing_integration.py b/test-int/cli/test_routing_integration.py index c204dec8..e63ceeec 100644 --- a/test-int/cli/test_routing_integration.py +++ b/test-int/cli/test_routing_integration.py @@ -89,6 +89,27 @@ class TestRoutingFlagsValidation: assert "Cannot specify both --local and --cloud" in result.output +def _stub_auto_update(monkeypatch, mcp_mod) -> None: + """Keep `bm mcp` stdio tests from running the real background auto-update. + + The command starts a daemon thread before mcp_server.run; unstubbed it hits + PyPI and rewrites config.json from a background thread, leaking into later + tests in the same process (#940's KeyError flake on test_mcp_sse_forces_local). + """ + from basic_memory.cli.auto_update import AutoUpdateResult, AutoUpdateStatus, InstallSource + + def skipped_auto_update(**kwargs) -> AutoUpdateResult: + return AutoUpdateResult( + status=AutoUpdateStatus.SKIPPED, + source=InstallSource.UNKNOWN, + checked=False, + update_available=False, + updated=False, + ) + + monkeypatch.setattr(mcp_mod, "run_auto_update", skipped_auto_update) + + class TestMcpCommandRouting: """Tests that MCP routing varies by transport.""" @@ -109,8 +130,10 @@ class TestMcpCommandRouting: monkeypatch.setattr(mcp_mod.mcp_server, "run", mock_run) monkeypatch.setattr(mcp_mod, "init_mcp_logging", lambda: None) + _stub_auto_update(monkeypatch, mcp_mod) - runner.invoke(cli_app, ["mcp"]) # default transport is stdio + result = runner.invoke(cli_app, ["mcp"]) # default transport is stdio + assert result.exit_code == 0, result.output # Command should not have set these vars assert env_at_run["FORCE_LOCAL"] is None @@ -132,8 +155,10 @@ class TestMcpCommandRouting: monkeypatch.setattr(mcp_mod.mcp_server, "run", mock_run) monkeypatch.setattr(mcp_mod, "init_mcp_logging", lambda: None) + _stub_auto_update(monkeypatch, mcp_mod) - runner.invoke(cli_app, ["mcp"]) + result = runner.invoke(cli_app, ["mcp"]) + assert result.exit_code == 0, result.output # Externally-set vars should be preserved assert env_at_run["FORCE_CLOUD"] == "true" @@ -153,7 +178,8 @@ class TestMcpCommandRouting: monkeypatch.setattr(mcp_mod.mcp_server, "run", mock_run) monkeypatch.setattr(mcp_mod, "init_mcp_logging", lambda: None) - runner.invoke(cli_app, ["mcp", "--transport", "streamable-http"]) + result = runner.invoke(cli_app, ["mcp", "--transport", "streamable-http"]) + assert result.exit_code == 0, result.output assert env_at_run["FORCE_LOCAL"] == "true" assert env_at_run["EXPLICIT"] == "true" @@ -172,7 +198,8 @@ class TestMcpCommandRouting: monkeypatch.setattr(mcp_mod.mcp_server, "run", mock_run) monkeypatch.setattr(mcp_mod, "init_mcp_logging", lambda: None) - runner.invoke(cli_app, ["mcp", "--transport", "sse"]) + result = runner.invoke(cli_app, ["mcp", "--transport", "sse"]) + assert result.exit_code == 0, result.output assert env_at_run["FORCE_LOCAL"] == "true" assert env_at_run["EXPLICIT"] == "true" diff --git a/tests/test_config.py b/tests/test_config.py index 4f3bab0b..f88ad839 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1600,3 +1600,62 @@ class TestAutoUpdateConfig: assert loaded.auto_update is False assert loaded.update_check_interval == 7200 assert loaded.auto_update_last_checked_at == checked_at + + +class TestAtomicConfigSave: + """Regression tests for #940: saving config must never tear the published file. + + Long-lived readers (the MCP stdio server's mtime-based config reload, the CLI + background auto-update thread) re-read config.json while other code saves it. + An in-place write truncates the file first, so a concurrent reader can observe + empty/partial JSON — and load_config() raises SystemExit on invalid JSON. + """ + + def test_interrupted_save_preserves_published_config(self, config_home, monkeypatch): + """A write that dies mid-stream must leave the existing config untouched.""" + import json + + from basic_memory.config import save_basic_memory_config + + with tempfile.TemporaryDirectory() as temp_dir: + temp_path = Path(temp_dir) + config_file = temp_path / "config.json" + config = BasicMemoryConfig( + projects={"main": {"path": str(temp_path / "main")}}, + default_project="main", + ) + save_basic_memory_config(config_file, config) + published = config_file.read_text(encoding="utf-8") + json.loads(published) # sanity: complete, valid document + + def torn_write_text(self, content, *args, **kwargs): + # Fault injection: the write dies halfway through. For an in-place + # write this is exactly the truncated state a concurrent reader + # observes mid-save; an atomic save must confine it to a temp file. + with open(self, "w", encoding="utf-8") as fh: + fh.write(content[: len(content) // 2]) + raise OSError("simulated interrupted write") + + monkeypatch.setattr(Path, "write_text", torn_write_text) + # save_basic_memory_config logs write failures instead of raising + save_basic_memory_config(config_file, config) + monkeypatch.undo() + + assert config_file.read_text(encoding="utf-8") == published + + def test_save_leaves_no_temp_files(self, config_home): + """The atomic-write temp file must not survive a successful save.""" + with tempfile.TemporaryDirectory() as temp_dir: + temp_path = Path(temp_dir) + config_file = temp_path / "config.json" + config = BasicMemoryConfig( + projects={"main": {"path": str(temp_path / "main")}}, + default_project="main", + ) + + from basic_memory.config import save_basic_memory_config + + save_basic_memory_config(config_file, config) + + assert config_file.exists() + assert not list(temp_path.glob("*.tmp"))