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"))