From dc8f72acdfe62b18ecb5522f13852a584d7e99d3 Mon Sep 17 00:00:00 2001 From: Robert Sigmundsson <230784065+RobertSigmundsson@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:03:48 +0200 Subject: [PATCH] fix(consolidation): the lock honours SURREAL_MEMORY_DIR like the rest of the data dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _lock_path derived its directory from Path.home() alone, so the consolidation lock was the one piece of state a redirected data dir could not move. Everything else in the package already honours the variable — cli/config.get_default_data_dir and cli/update_check._get_cache_path both read it. Measured on SurrealDB-independent ground, by asking _lock_path where it would put the file: SURREAL_MEMORY_DIR=/tmp/throwaway -> ~/.surrealmemory/consolidation-.lock A run against a throwaway brain therefore created and deleted files in the operator's real ~/.surrealmemory. With the change the same call returns /tmp/throwaway/consolidation-.lock. The trade-off is worth naming rather than discovering: two processes pointed at different SURREAL_MEMORY_DIR values now take different locks and no longer exclude one another. That is the intended reading — they are operating on separate data directories — but the lock is now per data dir rather than per machine. The test class gets a per-test directory and a test that asserts the redirection rather than assuming it, since the failure mode is silent: the lock simply reappears in the real home. Its docstring is brought up to date at the same time, and the history is worth stating rather than flattening. When #74 added xdist_group, the note it left was accurate: these tests did share one real lock file and did race on it. #121 then added a session-scoped $HOME redirect to conftest for unrelated reasons, which gives every worker its own home and therefore its own lock file. The race has not been reachable since, and nothing updated the note. So the isolation was already there — implicitly, resting on a fixture that exists for another purpose, which is the problem this fixture addresses. xdist_group is kept as a guard against shared state being reintroduced. --- docs/reference/config.md | 2 +- .../utils/consolidation_lock.py | 19 +++++++++- tests/unit/test_multi_agent.py | 37 +++++++++++++++---- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/docs/reference/config.md b/docs/reference/config.md index e00fdd2d..f371496e 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -385,7 +385,7 @@ places, the environment wins. | `SURREAL_MEMORY_API_KEY` | `src/surreal_memory/unified_config.py` | | `SURREAL_MEMORY_BRAIN` | `src/surreal_memory/cli/_helpers.py`, `src/surreal_memory/cli/commands/brain.py`, `src/surreal_memory/unified_config.py` | | `SURREAL_MEMORY_DASHBOARD_CACHE_TTL` | `src/surreal_memory/server/dashboard_cache.py` | -| `SURREAL_MEMORY_DIR` | `src/surreal_memory/cli/config.py`, `src/surreal_memory/cli/update_check.py`, `src/surreal_memory/engine/reasoning_injection.py`, +3 more | +| `SURREAL_MEMORY_DIR` | `src/surreal_memory/cli/config.py`, `src/surreal_memory/cli/update_check.py`, `src/surreal_memory/engine/reasoning_injection.py`, +4 more | | `SURREAL_MEMORY_DISABLE_SUPERSEDED_FILTER` | `src/surreal_memory/mcp/recall_handler.py` | | `SURREAL_MEMORY_EMBEDDING_API_KEY` | `src/surreal_memory/engine/embedding/bge_m3_embedding.py` | | `SURREAL_MEMORY_EMBEDDING_DIMENSION` | `src/surreal_memory/engine/embedding/bge_m3_embedding.py`, `src/surreal_memory/unified_config.py` | diff --git a/src/surreal_memory/utils/consolidation_lock.py b/src/surreal_memory/utils/consolidation_lock.py index b8302158..afb2d0aa 100755 --- a/src/surreal_memory/utils/consolidation_lock.py +++ b/src/surreal_memory/utils/consolidation_lock.py @@ -21,8 +21,23 @@ def _lock_path(brain_id: str = "") -> Path: - """Get the lock file path, optionally per-brain.""" - base = Path.home() / ".surrealmemory" + """Get the lock file path, optionally per-brain. + + Honours ``SURREAL_MEMORY_DIR`` like the rest of the data directory does + (``cli/config.get_default_data_dir``, ``cli/update_check._get_cache_path``). + Hard-coding ``Path.home()`` here made the lock the one piece of state that + a redirected data dir could not move: anything run against a throwaway + brain still created and deleted files in the operator's real + ``~/.surrealmemory``. + + The trade-off is worth naming: two processes pointed at different + ``SURREAL_MEMORY_DIR`` values now take different locks and no longer + exclude one another. That is the intended reading -- they are working on + separate data directories -- but it does mean the lock is per data dir + rather than per machine. + """ + env_dir = os.environ.get("SURREAL_MEMORY_DIR") + base = Path(env_dir).resolve() if env_dir else Path.home() / ".surrealmemory" base.mkdir(parents=True, exist_ok=True) safe_name = brain_id.replace("/", "_").replace("\\", "_") if brain_id else "" filename = f"consolidation-{safe_name}.lock" if safe_name else "consolidation.lock" diff --git a/tests/unit/test_multi_agent.py b/tests/unit/test_multi_agent.py index a9d75592..d4fe8b55 100755 --- a/tests/unit/test_multi_agent.py +++ b/tests/unit/test_multi_agent.py @@ -11,6 +11,7 @@ import json import os import time +from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -120,22 +121,42 @@ def test_agent_tag_coexists_with_user_tags(self) -> None: class TestConsolidationLock: """Test file-based consolidation lock. - _lock_path() is a real, fixed file under ~/.surrealmemory/ shared across - the whole process tree — not test-isolated. Under pytest-xdist's default - per-test scheduling, two of these tests can land on different worker - processes and race on that one file (reproduced live: a different subset - fails each full-suite run, 100% pass in isolation). xdist_group pins the - whole class to one worker, same fix as the aiosqlite leak-guard pair. + _lock_path() ignored SURREAL_MEMORY_DIR and derived its directory from + Path.home() alone. When #74 added xdist_group here, that made these tests + share one real lock file across the process tree and race on it, exactly as + the note it left said. #121 then gave the suite a session-scoped $HOME + redirect for unrelated reasons, which hands every xdist worker its own home + and therefore its own lock file -- so the race has not been reachable since, + and the note quietly stopped describing the present. + + What the redirect does not do is make the intent explicit: a test that + relies on $HOME being someone else's problem is one refactor away from + writing into a real home again. The fixture below points SURREAL_MEMORY_DIR + at a per-test directory and the first test asserts the redirection, because + the failure mode is silent. xdist_group is kept as a guard against shared + state being reintroduced. """ @pytest.fixture(autouse=True) - def _cleanup_lock(self) -> None: - """Remove lock file before/after each test.""" + def _cleanup_lock(self, tmp_path, monkeypatch) -> None: # type: ignore[no-untyped-def] + """Point the lock at a per-test directory, and leave nothing behind.""" + monkeypatch.setenv("SURREAL_MEMORY_DIR", str(tmp_path)) lock = _lock_path() lock.unlink(missing_ok=True) yield # type: ignore[misc] lock.unlink(missing_ok=True) + def test_lock_lives_in_the_configured_data_dir(self, tmp_path) -> None: # type: ignore[no-untyped-def] + """SURREAL_MEMORY_DIR must move the lock, like it moves everything else. + + The autouse fixture already redirects it; this asserts the redirection + rather than assuming it, because the failure mode is silent — the lock + simply appears in the real home directory instead. + """ + assert _lock_path().parent == tmp_path.resolve() + assert _lock_path("some-brain").parent == tmp_path.resolve() + assert Path.home() / ".surrealmemory" != _lock_path().parent + def test_acquire_fresh_lock(self) -> None: """Should acquire lock when no lock exists.""" assert acquire_consolidation_lock() is True