Skip to content

Commit b3fc169

Browse files
groksrcclaude
groksrc
and
claude
committed
Add file size to cache invalidation check
Addresses Codex review feedback: on filesystems with coarse mtime granularity (1s resolution), two writes within the same second would share the same mtime. Adding file size to the check catches config changes even when mtime doesn't change, since config edits almost always change file size. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Drew Cain <groksrc@gmail.com>
1 parent 72f6ebe commit b3fc169

11 files changed

Lines changed: 63 additions & 11 deletions

‎src/basic_memory/config.py‎

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -629,10 +629,12 @@ def data_dir_path(self) -> Path:
629629

630630
# Module-level cache for configuration
631631
_CONFIG_CACHE: Optional[BasicMemoryConfig] = None
632-
# Track config file mtime so cross-process changes (e.g. `bm project set-cloud`
632+
# Track config file mtime+size so cross-process changes (e.g. `bm project set-cloud`
633633
# in a separate terminal) invalidate the cache in long-lived processes like the
634-
# MCP stdio server.
634+
# MCP stdio server. Using both mtime and size guards against coarse-granularity
635+
# filesystems where two writes within the same second share the same mtime.
635636
_CONFIG_MTIME: Optional[float] = None
637+
_CONFIG_SIZE: Optional[int] = None
636638

637639

638640
class ConfigManager:
@@ -671,25 +673,33 @@ def load_config(self) -> BasicMemoryConfig:
671673
separate terminal) are picked up by long-lived processes like
672674
the MCP stdio server.
673675
"""
674-
global _CONFIG_CACHE, _CONFIG_MTIME
676+
global _CONFIG_CACHE, _CONFIG_MTIME, _CONFIG_SIZE
675677

676678
# Trigger: cached config exists but the on-disk file may have been
677679
# modified by another process (CLI command in a different terminal).
678680
# Why: the MCP server is long-lived; without this check it would
679681
# serve stale project routing forever.
680-
# Outcome: cheap os.stat() per access; re-read only when mtime differs.
682+
# Outcome: cheap os.stat() per access; re-read only when mtime or size differs.
681683
if _CONFIG_CACHE is not None:
682684
try:
683-
current_mtime = self.config_file.stat().st_mtime
685+
st = self.config_file.stat()
686+
current_mtime = st.st_mtime
687+
current_size = st.st_size
684688
except OSError:
685689
current_mtime = None
690+
current_size = None
686691

687-
if current_mtime is not None and current_mtime == _CONFIG_MTIME:
692+
if (
693+
current_mtime is not None
694+
and current_mtime == _CONFIG_MTIME
695+
and current_size == _CONFIG_SIZE
696+
):
688697
return _CONFIG_CACHE
689698

690-
# mtime changed or file gone — invalidate and fall through to re-read
699+
# mtime/size changed or file gone — invalidate and fall through to re-read
691700
_CONFIG_CACHE = None
692701
_CONFIG_MTIME = None
702+
_CONFIG_SIZE = None
693703

694704
if self.config_file.exists():
695705
try:
@@ -744,11 +754,14 @@ def load_config(self) -> BasicMemoryConfig:
744754

745755
_CONFIG_CACHE = BasicMemoryConfig(**merged_data)
746756

747-
# Record mtime so subsequent calls detect cross-process changes
757+
# Record mtime+size so subsequent calls detect cross-process changes
748758
try:
749-
_CONFIG_MTIME = self.config_file.stat().st_mtime
759+
st = self.config_file.stat()
760+
_CONFIG_MTIME = st.st_mtime
761+
_CONFIG_SIZE = st.st_size
750762
except OSError:
751763
_CONFIG_MTIME = None
764+
_CONFIG_SIZE = None
752765

753766
# Re-save to normalize legacy config into current format
754767
if needs_resave:
@@ -780,11 +793,12 @@ def load_config(self) -> BasicMemoryConfig:
780793

781794
def save_config(self, config: BasicMemoryConfig) -> None:
782795
"""Save configuration to file and invalidate cache."""
783-
global _CONFIG_CACHE, _CONFIG_MTIME
796+
global _CONFIG_CACHE, _CONFIG_MTIME, _CONFIG_SIZE
784797
save_basic_memory_config(self.config_file, config)
785798
# Invalidate cache so next load_config() reads fresh data
786799
_CONFIG_CACHE = None
787800
_CONFIG_MTIME = None
801+
_CONFIG_SIZE = None
788802

789803
@property
790804
def projects(self) -> Dict[str, str]:

‎test-int/conftest.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ def config_manager(app_config: BasicMemoryConfig, config_home) -> ConfigManager:
259259

260260
config_module._CONFIG_CACHE = None
261261
config_module._CONFIG_MTIME = None
262+
config_module._CONFIG_SIZE = None
262263

263264
config_manager = ConfigManager()
264265
# Update its paths to use the test directory

‎tests/cli/conftest.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ def isolated_home(tmp_path, monkeypatch) -> Path:
2626

2727
config_module._CONFIG_CACHE = None
2828
config_module._CONFIG_MTIME = None
29+
config_module._CONFIG_SIZE = None
2930

3031
monkeypatch.setenv("HOME", str(tmp_path))
3132
if os.name == "nt":

‎tests/cli/test_json_output.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,7 @@ def _write(config_data: dict):
351351

352352
config_module._CONFIG_CACHE = None
353353
config_module._CONFIG_MTIME = None
354+
config_module._CONFIG_SIZE = None
354355

355356
config_dir = tmp_path / ".basic-memory"
356357
config_dir.mkdir(parents=True, exist_ok=True)

‎tests/cli/test_project_add_with_local_path.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ def mock_config(tmp_path, monkeypatch):
2828

2929
config_module._CONFIG_CACHE = None
3030
config_module._CONFIG_MTIME = None
31+
config_module._CONFIG_SIZE = None
3132

3233
config_dir = tmp_path / ".basic-memory"
3334
config_dir.mkdir(parents=True, exist_ok=True)

‎tests/cli/test_project_list_and_ls.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ def _write(config_data: dict) -> Path:
3030

3131
config_module._CONFIG_CACHE = None
3232
config_module._CONFIG_MTIME = None
33+
config_module._CONFIG_SIZE = None
3334

3435
config_dir = tmp_path / ".basic-memory"
3536
config_dir.mkdir(parents=True, exist_ok=True)

‎tests/cli/test_project_set_cloud_local.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ def mock_config(tmp_path, monkeypatch):
2323

2424
config_module._CONFIG_CACHE = None
2525
config_module._CONFIG_MTIME = None
26+
config_module._CONFIG_SIZE = None
2627

2728
config_dir = tmp_path / ".basic-memory"
2829
config_dir.mkdir(parents=True, exist_ok=True)
@@ -70,6 +71,7 @@ def test_set_cloud_no_credentials(self, runner, tmp_path, monkeypatch):
7071

7172
config_module._CONFIG_CACHE = None
7273
config_module._CONFIG_MTIME = None
74+
config_module._CONFIG_SIZE = None
7375

7476
config_dir = tmp_path / ".basic-memory"
7577
config_dir.mkdir(parents=True, exist_ok=True)
@@ -94,6 +96,7 @@ def test_set_cloud_with_oauth_session(self, runner, tmp_path, monkeypatch):
9496

9597
config_module._CONFIG_CACHE = None
9698
config_module._CONFIG_MTIME = None
99+
config_module._CONFIG_SIZE = None
97100

98101
config_dir = tmp_path / ".basic-memory"
99102
config_dir.mkdir(parents=True, exist_ok=True)
@@ -165,12 +168,14 @@ def test_set_local_clears_workspace_id(self, runner, mock_config):
165168
# Manually set workspace_id on the project
166169
config_module._CONFIG_CACHE = None
167170
config_module._CONFIG_MTIME = None
171+
config_module._CONFIG_SIZE = None
168172
config_data = json.loads(mock_config.read_text())
169173
config_data["projects"]["research"]["mode"] = "cloud"
170174
config_data["projects"]["research"]["workspace_id"] = "11111111-1111-1111-1111-111111111111"
171175
mock_config.write_text(json.dumps(config_data, indent=2))
172176
config_module._CONFIG_CACHE = None
173177
config_module._CONFIG_MTIME = None
178+
config_module._CONFIG_SIZE = None
174179

175180
# Set back to local
176181
result = runner.invoke(app, ["project", "set-local", "research"])
@@ -179,6 +184,7 @@ def test_set_local_clears_workspace_id(self, runner, mock_config):
179184
# Verify workspace_id was cleared
180185
config_module._CONFIG_CACHE = None
181186
config_module._CONFIG_MTIME = None
187+
config_module._CONFIG_SIZE = None
182188
updated_data = json.loads(mock_config.read_text())
183189
assert updated_data["projects"]["research"]["workspace_id"] is None
184190
assert updated_data["projects"]["research"]["mode"] == "local"
@@ -194,6 +200,7 @@ def test_set_cloud_with_workspace_stores_workspace_id(self, runner, mock_config,
194200

195201
config_module._CONFIG_CACHE = None
196202
config_module._CONFIG_MTIME = None
203+
config_module._CONFIG_SIZE = None
197204

198205
async def fake_get_available_workspaces():
199206
return [
@@ -218,6 +225,7 @@ async def fake_get_available_workspaces():
218225
# Verify workspace_id was persisted
219226
config_module._CONFIG_CACHE = None
220227
config_module._CONFIG_MTIME = None
228+
config_module._CONFIG_SIZE = None
221229
updated_data = json.loads(mock_config.read_text())
222230
assert (
223231
updated_data["projects"]["research"]["workspace_id"]
@@ -231,6 +239,7 @@ def test_set_cloud_with_workspace_not_found(self, runner, mock_config, monkeypat
231239

232240
config_module._CONFIG_CACHE = None
233241
config_module._CONFIG_MTIME = None
242+
config_module._CONFIG_SIZE = None
234243

235244
async def fake_get_available_workspaces():
236245
return [
@@ -259,19 +268,22 @@ def test_set_cloud_uses_default_workspace_when_no_flag(self, runner, mock_config
259268

260269
config_module._CONFIG_CACHE = None
261270
config_module._CONFIG_MTIME = None
271+
config_module._CONFIG_SIZE = None
262272

263273
# Set default_workspace in config
264274
config_data = json.loads(mock_config.read_text())
265275
config_data["default_workspace"] = "global-default-tenant-id"
266276
mock_config.write_text(json.dumps(config_data, indent=2))
267277
config_module._CONFIG_CACHE = None
268278
config_module._CONFIG_MTIME = None
279+
config_module._CONFIG_SIZE = None
269280

270281
result = runner.invoke(app, ["project", "set-cloud", "research"])
271282
assert result.exit_code == 0
272283

273284
# Verify workspace_id was set from default
274285
config_module._CONFIG_CACHE = None
275286
config_module._CONFIG_MTIME = None
287+
config_module._CONFIG_SIZE = None
276288
updated_data = json.loads(mock_config.read_text())
277289
assert updated_data["projects"]["research"]["workspace_id"] == "global-default-tenant-id"

‎tests/cli/test_workspace_commands.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ def _setup_config(self, monkeypatch):
7777
monkeypatch.setenv("BASIC_MEMORY_CONFIG_DIR", str(config_dir))
7878
basic_memory.config._CONFIG_CACHE = None
7979
basic_memory.config._CONFIG_MTIME = None
80+
basic_memory.config._CONFIG_SIZE = None
8081

8182
config_manager = ConfigManager()
8283
test_config = BasicMemoryConfig(
@@ -108,6 +109,7 @@ async def fake_get_available_workspaces(context=None):
108109
# Verify config was updated
109110
basic_memory.config._CONFIG_CACHE = None
110111
basic_memory.config._CONFIG_MTIME = None
112+
basic_memory.config._CONFIG_SIZE = None
111113
config = ConfigManager().config
112114
assert config.default_workspace == "11111111-1111-1111-1111-111111111111"
113115

‎tests/conftest.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ def config_manager(app_config: BasicMemoryConfig, config_home: Path, monkeypatch
139139

140140
config_module._CONFIG_CACHE = None
141141
config_module._CONFIG_MTIME = None
142+
config_module._CONFIG_SIZE = None
142143

143144
# Create a new ConfigManager that uses the test home directory
144145
config_manager = ConfigManager()

‎tests/services/test_project_service.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -779,6 +779,7 @@ async def test_add_project_with_project_root_sanitizes_paths(
779779

780780
config_module._CONFIG_CACHE = None
781781
config_module._CONFIG_MTIME = None
782+
config_module._CONFIG_SIZE = None
782783

783784
test_cases = [
784785
# (project_name, user_path, expected_sanitized_name)
@@ -847,6 +848,7 @@ async def test_add_project_with_project_root_rejects_escape_attempts(
847848

848849
config_module._CONFIG_CACHE = None
849850
config_module._CONFIG_MTIME = None
851+
config_module._CONFIG_SIZE = None
850852

851853
# All of these should succeed by being sanitized to paths under project_root
852854
# The sanitization removes dangerous patterns, so they don't escape
@@ -934,6 +936,7 @@ async def test_add_project_with_project_root_normalizes_case(
934936

935937
config_module._CONFIG_CACHE = None
936938
config_module._CONFIG_MTIME = None
939+
config_module._CONFIG_SIZE = None
937940

938941
test_cases = [
939942
# (input_path, expected_normalized_path)
@@ -989,6 +992,7 @@ async def test_add_project_with_project_root_detects_case_collisions(
989992

990993
config_module._CONFIG_CACHE = None
991994
config_module._CONFIG_MTIME = None
995+
config_module._CONFIG_SIZE = None
992996

993997
# First, create a project with lowercase path
994998
first_project = "documents-project"
@@ -1164,6 +1168,7 @@ async def test_add_project_nested_validation_with_project_root(
11641168

11651169
config_module._CONFIG_CACHE = None
11661170
config_module._CONFIG_MTIME = None
1171+
config_module._CONFIG_SIZE = None
11671172

11681173
parent_project_name = f"cloud-parent-{os.urandom(4).hex()}"
11691174
child_project_name = f"cloud-child-{os.urandom(4).hex()}"

0 commit comments

Comments
 (0)