mirror of
https://github.com/basicmachines-co/basic-memory
synced 2026-06-21 13:47:35 +00:00
fix(cli): write config.json atomically and isolate auto-update in mcp routing tests
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 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
This commit is contained in:
committed by
Paul Hernandez
parent
85c701b8c2
commit
b3bdd5914f
@@ -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}")
|
||||
|
||||
|
||||
Reference in New Issue
Block a user