From 2dc1e227eb4281549c77cde5fa77b3b18dfacbe7 Mon Sep 17 00:00:00 2001 From: houren Antony <2212222@mail.nankai.edu.cn> Date: Tue, 9 Jun 2026 22:55:44 +0800 Subject: [PATCH] fix(langgraph-dev): rotate langgraph_dev.log when it exceeds 50MB (#270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(langgraph-dev): rotate langgraph_dev.log when it exceeds 50MB ``_LOG_FILE`` (``~/.config/evoscientist/langgraph_dev.log``) was opened in ``start_langgraph_dev`` with plain ``"ab"`` and never rotated, so it grew unbounded over weeks/months of heavy use — especially when chatty MCP servers spawned by langgraph dev filled it, or when failure paths produced stack traces. Implement the recommended option 1 from #209: filesize-based rollover. When the active log exceeds 50MB on the next ``start_langgraph_dev`` invocation, rename it to ``langgraph_dev.log.1`` (overwriting any existing backup) via ``os.replace`` and start fresh. Single-backup policy keeps the disk footprint bounded at roughly 2x threshold. Rotation is best-effort: ``_rotate_log_if_needed`` logs and swallows OSError so a permission error or racing rename can't block langgraph dev from starting. The next ``start`` invocation will try again — worst case the log grows for one more session. Options 2 (timestamped per-session + 7-day sweep) and 3 (``RotatingFileHandler`` + pipe) are explicitly NOT done — option 1 is simplest, no async machinery, matches the issue's recommendation. Closes #209 * test(langgraph-dev): redirect _PID_DIR in rotate integration test Address CodeRabbit review comment on #270: the ``TestStartLanggraphDevRotatesLog::test_rotate_called_before_open`` test patched only ``_LOG_FILE`` to a tmp path, but ``start_langgraph_dev`` also calls ``_PID_DIR.mkdir(...)`` as part of its prelude, which would create a real directory under ``~/.config/evoscientist/`` on a dev machine. Redirect ``_PID_DIR`` to ``tmp_path / "pids"`` too so the test stays fully isolated. Add a final assertion that ``pid_dir.is_dir()`` holds, proving the function reached past the mkdir call. * refactor(langgraph-dev): bundle runtime paths into LanggraphRuntimePaths @din0s review follow-up on #270: the previous test isolation patched only ``_LOG_FILE`` (and after a second round, ``_PID_DIR``), but ``start_langgraph_dev`` still touches 5 distinct on-disk paths. Patching any subset of those still leaves the others pointing at the user's real ``~/.config/evoscientist/`` — exactly the case that produced the "Port 6174 cannot be bound after waiting 60s" symptom on the reviewer's machine. Replace the five free-floating module-level constants (``_PID_DIR`` / ``_PID_FILE`` / ``_LOG_FILE`` / ``_WORKSPACE_SIDECAR`` / ``_FILE_LOCK_PATH``) with a single ``LanggraphRuntimePaths`` frozen dataclass exposed as a module-level ``RUNTIME`` instance. Production code accesses ``RUNTIME.pid_file`` etc.; tests can now substitute the *whole* bundle in one assignment: monkeypatch.setattr( manager, "RUNTIME", manager.LanggraphRuntimePaths.for_directory(tmp_path / "runtime"), ) The classmethod ``for_directory(pid_dir)`` builds an isolated bundle rooted at a single dir, so the test author doesn't spell out every path field. Tests that only care about one field (e.g. pid_file during the stale-process kill path) use ``dataclasses.replace(manager.RUNTIME, pid_file=X)`` — frozen dataclass-friendly, no need to enumerate the other four fields. The dataclass's docstring records the migration rationale (the old five-name layout invited inconsistent patches). External callers of the old constants updated: - ``EvoScientist/deploy/server.py`` and ``webui.py`` now import ``RUNTIME`` and use ``RUNTIME.log_file`` for the on-screen log path hint. The other imports they had (``_DEFAULT_PORT``, ``_is_port_occupied``, ``_read_workspace_sidecar``) are still module-level functions/values, untouched. Test updates: - ``tests/test_langgraph_manager.py``: ``patch.object(manager, "_XXX", X)`` patterns now go through ``dataclasses.replace(manager.RUNTIME, xxx=X)``; the ``TestStartLanggraphDevRotatesLog::test_rotate_called_before_open`` test (from the previous #270 review iteration) uses ``for_directory`` for one-shot isolation. - ``tests/test_langgraph_dev_workspace_sidecar.py``: each test now goes through a tiny ``_isolated_runtime(monkeypatch, tmp_path)`` helper that calls ``for_directory``. - ``tests/test_langgraph_dev_deploy_mode.py``: same ``for_directory`` swap. No production behavior change. All ``langgraph_dev``-side tests (``test_langgraph_manager.py`` 26/26, ``test_langgraph_dev_workspace_sidecar.py`` 14/14, ``test_langgraph_dev_deploy_mode.py`` 14/14, ``test_cli_deploy.py`` 18/18 — which indirectly exercises deploy/server.py and deploy/webui.py imports) pass. Full-project test count unchanged from baseline; the remaining 22 Windows-only pre-existing failures (test_background ``os.killpg``, test_file_mentions tilde, mcp_client ``shutil.which``, test_sessions 8.3 short path) are documented as out-of-scope for #207. * style: apply ruff format to langgraph_dev test + module files CI lint check on #270 failed: Run ruff format --check . Would reformat: EvoScientist/langgraph_dev/manager.py Would reformat: tests/test_langgraph_manager.py Plus two test files touched by the prior consolidation commit that ``ruff format`` hadn't seen yet: tests/test_langgraph_dev_deploy_mode.py tests/test_langgraph_dev_workspace_sidecar.py Just formatting. No logic change. All 75 refactor-related tests pass. * fix(test): use for_directory for full path isolation + patch _can_bind_port to skip real socket ops Two fixes for TestStartLanggraphDevRotatesLog: 1. Replace dataclasses.replace(manager.RUNTIME, ...) with LanggraphRuntimePaths.for_directory(pid_dir) so pid_file, workspace_sidecar, and lock_file are also temp-rooted (prevents leak to ~/.config/evoscientist/). 2. Monkeypatch _can_bind_port to always return True so the bind-poll loop in _wait_for_port_bindable passes immediately without touching real sockets (fixes 60s timeout on machines where port 6174 is already in use). * fix: cross-platform compatibility for Windows CI runners - background.py: replace POSIX-only os.killpg/os.getpgid with cross-platform _kill_process_tree() helper. On Windows falls back to Popen.terminate()/Popen.kill() (TerminateProcess); on POSIX keeps existing os.killpg logic. - test_backends.py: replace mkdir -p shell execution in test_literal_workspace_path_replaced with preprocessing-boundary assertion (patch LocalShellBackend.execute, capture command, assert workspace path was rewritten to ./). Avoids POSIX-only mkdir -p on Windows runners. - test_file_mentions.py: monkeypatch USERPROFILE on Windows so ntpath.expanduser() resolves ~ to tmp_path even when HOME is unset on CI runners. * refactor(test): add runtime_paths fixture to isolate manager.RUNTIME Adds a reusable fixture that monkeypatches manager.RUNTIME to a temp-rooted LanggraphRuntimePaths.for_directory(). Tests that need specific fields can still dataclasses.replace(runtime_paths, ...) but the baseline is always temp-isolated, preventing leaks to ~/.config/evoscientist/. Updated test_langgraph_dev_deploy_mode.py, test_langgraph_dev_workspace_sidecar.py, and test_langgraph_manager.py to use the fixture, consolidating sequential lock_file + pid_dir patches into single dataclasses.replace calls. * Revert "fix: cross-platform compatibility for Windows CI runners" This reverts commit eb025d24af32e195a982cd40f6d70dba885c4019. * style: ruff format conftest.py * fix: address review issues in log-rotation + runtime paths - Use for_directory(tmp_path/pids) as base in ensure_langgraph_dev tests so pid_file/log_file are co-located with pid_dir, not split across paths - Remove unused runtime_paths param from test_no_existing_file_is_noop - Replace manager.RUNTIME with runtime_paths in two sidecar tests - Use for_directory(DEFAULT_PID_DIR) instead of explicit construction - Fix stale _LOG_FILE reference in TestRotateLogIfNeeded docstring * style: ruff format test files --------- Co-authored-by: Xi Zhang <106144707+X-iZhang@users.noreply.github.com> --- EvoScientist/deploy/server.py | 4 +- EvoScientist/deploy/webui.py | 4 +- EvoScientist/langgraph_dev/manager.py | 156 ++++++++--- tests/conftest.py | 20 ++ tests/test_langgraph_dev_deploy_mode.py | 55 ++-- tests/test_langgraph_dev_workspace_sidecar.py | 101 +++++-- tests/test_langgraph_manager.py | 250 ++++++++++++++++-- 7 files changed, 484 insertions(+), 106 deletions(-) diff --git a/EvoScientist/deploy/server.py b/EvoScientist/deploy/server.py index e015cda..1c9a862 100644 --- a/EvoScientist/deploy/server.py +++ b/EvoScientist/deploy/server.py @@ -61,7 +61,7 @@ def deploy( from ..config import apply_config_to_env, get_effective_config from ..langgraph_dev.manager import ( _DEFAULT_PORT, - _LOG_FILE, + RUNTIME, _is_port_occupied, is_langgraph_dev_running, start_langgraph_dev, @@ -187,7 +187,7 @@ def deploy( console.print("[green]✓[/green] langgraph dev ready") # 9. Ready banner - log_hint = _shorten(str(_LOG_FILE)) + log_hint = _shorten(str(RUNTIME.log_file)) console.print( Panel( Text.from_markup( diff --git a/EvoScientist/deploy/webui.py b/EvoScientist/deploy/webui.py index a3d4c83..cd44fd1 100644 --- a/EvoScientist/deploy/webui.py +++ b/EvoScientist/deploy/webui.py @@ -59,7 +59,7 @@ def run_webui(config: Any, workspace_dir: str | None = None) -> None: from ..config import apply_config_to_env from ..langgraph_dev.manager import ( _DEFAULT_PORT, - _LOG_FILE, + RUNTIME, _is_port_occupied, _read_workspace_sidecar, is_langgraph_dev_running, @@ -210,7 +210,7 @@ def run_webui(config: Any, workspace_dir: str | None = None) -> None: f"[dim](langgraph dev — Assistant: EvoScientist)[/dim]\n" f"[bold]WebUI:[/bold] http://localhost:{webui_port} " f"[dim](opens in your browser)[/dim]\n" - f"[bold]Logs:[/bold] {_shorten(str(_LOG_FILE))}\n\n" + f"[bold]Logs:[/bold] {_shorten(str(RUNTIME.log_file))}\n\n" f"[dim]Fetching {_WEBUI_PACKAGE} via npx (first run may take a " f"moment)… Press Ctrl+C to stop.[/dim]" ), diff --git a/EvoScientist/langgraph_dev/manager.py b/EvoScientist/langgraph_dev/manager.py index 78a70ad..1de2731 100644 --- a/EvoScientist/langgraph_dev/manager.py +++ b/EvoScientist/langgraph_dev/manager.py @@ -19,6 +19,7 @@ import shutil import subprocess import threading import time +from dataclasses import dataclass from pathlib import Path import httpx @@ -35,6 +36,57 @@ from EvoScientist.config import ( logger = logging.getLogger(__name__) +@dataclass(frozen=True) +class LanggraphRuntimePaths: + """All on-disk paths the langgraph_dev manager writes to. + + Grouped into a single object (rather than a handful of free-floating + module-level constants) so tests can substitute *one* object to + redirect every file the manager touches, instead of patching each + path constant separately. The previous design — five separate + ``_PID_DIR`` / ``_PID_FILE`` / ``_LOG_FILE`` / ``_WORKSPACE_SIDECAR`` + / ``_FILE_LOCK_PATH`` names — invited inconsistent patches: a test + might redirect ``_LOG_FILE`` but leave ``_PID_DIR`` pointing at + ``~/.config/evoscientist``, so the function under test still wrote + to the user's real home directory. With a single object there's + one knob to turn; if you replace it, *every* path moves with it. + + Production code constructs ``RUNTIME`` below with the conventional + ``~/.config/evoscientist/`` layout. Tests call + :meth:`for_directory` to spin up an isolated instance. + """ + + pid_dir: Path + pid_file: Path + log_file: Path + workspace_sidecar: Path + lock_file: Path + + @classmethod + def for_directory(cls, pid_dir: Path) -> LanggraphRuntimePaths: + """Build a runtime-paths bundle rooted at ``pid_dir``. + + Used by tests (and any future embedded-deployment override) to + spin up an isolated set of paths without spelling out every + individual path by hand. + """ + return cls( + pid_dir=pid_dir, + pid_file=pid_dir / "langgraph_dev.pid", + log_file=pid_dir / "langgraph_dev.log", + workspace_sidecar=pid_dir / "langgraph_dev.workspace.json", + lock_file=pid_dir / "langgraph_dev.lock", + ) + + +# Module-level runtime paths. Defaulted to the conventional +# ``~/.config/evoscientist/`` layout; tests override ``RUNTIME`` with +# :meth:`LanggraphRuntimePaths.for_directory` to point at a temp dir +# without touching the user's real home directory. +DEFAULT_PID_DIR = Path.home() / ".config" / "evoscientist" +RUNTIME: LanggraphRuntimePaths = LanggraphRuntimePaths.for_directory(DEFAULT_PID_DIR) + + def needs_langgraph_dev(config: EvoScientistConfig) -> bool: """Return whether this config needs the background langgraph dev server.""" if config.enable_async_subagents: @@ -64,9 +116,42 @@ def _base_url(port: int = _DEFAULT_PORT) -> str: return f"http://localhost:{port}" -_PID_DIR = Path.home() / ".config" / "evoscientist" -_PID_FILE = _PID_DIR / "langgraph_dev.pid" -_LOG_FILE = _PID_DIR / "langgraph_dev.log" +# Default rollover threshold for ``RUNTIME.log_file`` — once the log +# exceeds this size, the next ``start_langgraph_dev`` invocation rotates +# it to ``langgraph_dev.log.1`` (overwriting any existing backup) and +# starts fresh. Single-backup policy keeps the disk footprint bounded +# at roughly 2x the threshold even under heavy use (chatty MCP servers, +# repeated failure paths with stack traces). See #209. +_LOG_ROTATION_BYTES = 50 * 1024 * 1024 # 50 MB + + +def _rotate_log_if_needed(log_path: Path) -> None: + """Rotate ``log_path`` to ``.1`` when it exceeds the + module's ``_LOG_ROTATION_BYTES`` threshold. + + Single-backup policy: at most one rotated copy is kept on disk. The + active log is fresh (zero bytes) after rotation, so the next + ``open(log_path, "ab")`` writes at offset 0. + + Best-effort: failures are logged and swallowed. A failed rotation + must NOT block ``start_langgraph_dev`` — the worst case is the log + keeps growing for one more session and the next ``start`` try + rotates it. + """ + try: + if not log_path.exists(): + return + if log_path.stat().st_size <= _LOG_ROTATION_BYTES: + return + backup = log_path.with_name(log_path.name + ".1") + os.replace(log_path, backup) + except OSError as exc: + logger.warning( + "Failed to rotate log %s: %s. Continuing with the existing log.", + log_path, + exc, + ) + # Workspace fingerprint sidecar — JSON recording the workspace + pid of the # running langgraph dev. Cross-process callers (e.g. TUI starting up while @@ -74,7 +159,6 @@ _LOG_FILE = _PID_DIR / "langgraph_dev.log" # silently operating on a different workspace's files. Missing/corrupt sidecar # degrades gracefully to a log warning for backward compatibility with # langgraph devs started before this protocol existed. -_WORKSPACE_SIDECAR = _PID_DIR / "langgraph_dev.workspace.json" class WorkspaceMismatchError(RuntimeError): @@ -103,13 +187,13 @@ def _write_workspace_sidecar(workspace_dir: Path, pid: int) -> None: ``ensure_langgraph_dev``. """ try: - _PID_DIR.mkdir(parents=True, exist_ok=True) - tmp = _WORKSPACE_SIDECAR.with_suffix(".json.tmp") + RUNTIME.pid_dir.mkdir(parents=True, exist_ok=True) + tmp = RUNTIME.workspace_sidecar.with_suffix(".json.tmp") tmp.write_text(json.dumps({"workspace": str(workspace_dir), "pid": pid})) - os.replace(tmp, _WORKSPACE_SIDECAR) + os.replace(tmp, RUNTIME.workspace_sidecar) except OSError as exc: logger.warning( - "Failed to write workspace sidecar %s: %s", _WORKSPACE_SIDECAR, exc + "Failed to write workspace sidecar %s: %s", RUNTIME.workspace_sidecar, exc ) @@ -126,10 +210,10 @@ def _read_workspace_sidecar() -> dict | None: ``Path(...)`` and surface as an unhandled exception instead of the documented log-warning fallback. """ - if not _WORKSPACE_SIDECAR.exists(): + if not RUNTIME.workspace_sidecar.exists(): return None try: - data = json.loads(_WORKSPACE_SIDECAR.read_text()) + data = json.loads(RUNTIME.workspace_sidecar.read_text()) except (OSError, ValueError): return None if not isinstance(data, dict): @@ -141,10 +225,10 @@ def _read_workspace_sidecar() -> dict | None: def _unlink_workspace_sidecar() -> None: - """Best-effort sidecar removal — called alongside every ``_PID_FILE.unlink()`` + """Best-effort sidecar removal — called alongside every ``RUNTIME.pid_file.unlink()`` so the workspace fingerprint never outlives the PID file it pairs with.""" try: - _WORKSPACE_SIDECAR.unlink() + RUNTIME.workspace_sidecar.unlink() except OSError: pass @@ -156,7 +240,7 @@ def _unlink_workspace_sidecar() -> None: # Shell B blocks until Shell A's health-check finishes, then sees the # healthy server and reuses it. ``threading.RLock`` is process-local and # can't coordinate across CLI invocations. -_FILE_LOCK_PATH = _PID_DIR / "langgraph_dev.lock" +# Lock path lives on ``RUNTIME.lock_file``; timeout stays module-level. _FILE_LOCK_TIMEOUT = 120.0 # 60s cold-start health-check + buffer # Module-level handle to the langgraph dev subprocess we started, if any. @@ -332,7 +416,7 @@ def _list_pids_on_port(port: int) -> list[int]: def _kill_owned_stale_process(port: int) -> bool: """Kill ONLY a previously-owned langgraph dev process bound to ``port``. - "Owned" means the PID written to ``_PID_FILE`` by an earlier + "Owned" means the PID written to ``RUNTIME.pid_file`` by an earlier ``start_langgraph_dev`` invocation in this user account, AND the live process at that PID still has ``langgraph`` in its command line (defense against PID recycling). Returns True if a stale-but-owned process was @@ -348,10 +432,10 @@ def _kill_owned_stale_process(port: int) -> bool: to an unrelated process between sessions (e.g., after a SIGKILL'd CLI left the PID file behind). The cmdline check rules that out. """ - if not _PID_FILE.exists(): + if not RUNTIME.pid_file.exists(): return False try: - owned_pid = int(_PID_FILE.read_text().strip()) + owned_pid = int(RUNTIME.pid_file.read_text().strip()) except (OSError, ValueError): return False @@ -369,7 +453,7 @@ def _kill_owned_stale_process(port: int) -> bool: # PID file points at a dead process — clean up the file but don't # try to kill anything. try: - _PID_FILE.unlink() + RUNTIME.pid_file.unlink() except OSError: pass _unlink_workspace_sidecar() @@ -394,12 +478,12 @@ def _kill_owned_stale_process(port: int) -> bool: "PID file %s claims pid %d for langgraph dev, but that pid now " "points at a different process (cmdline=%s). Refusing to kill, " "removing stale PID file.", - _PID_FILE, + RUNTIME.pid_file, owned_pid, cmdline, ) try: - _PID_FILE.unlink() + RUNTIME.pid_file.unlink() except OSError: pass _unlink_workspace_sidecar() @@ -410,7 +494,7 @@ def _kill_owned_stale_process(port: int) -> bool: except (psutil.NoSuchProcess, psutil.AccessDenied): pass try: - _PID_FILE.unlink() + RUNTIME.pid_file.unlink() except OSError: pass _unlink_workspace_sidecar() @@ -483,7 +567,7 @@ def start_langgraph_dev( # Defensive: handle a port that's occupied but not serving /ok. # Three cases: - # (a) Our own previous langgraph dev (PID matches _PID_FILE) — kill it. + # (a) Our own previous langgraph dev (PID matches RUNTIME.pid_file) — kill it. # (b) Our own previous langgraph dev exited but the kernel still holds # the socket in TIME_WAIT — no live PID for lsof to match, and the # PID file may already be gone (stop_langgraph_dev unlinks it). The @@ -498,7 +582,7 @@ def start_langgraph_dev( if _kill_owned_stale_process(port): logger.warning( "Cleaned up stale langgraph dev (pid from %s) on port %d", - _PID_FILE, + RUNTIME.pid_file, port, ) # After SIGKILL the kernel may keep the port in TIME_WAIT for @@ -530,14 +614,18 @@ def start_langgraph_dev( f"or change ports with: `EvoSci config set langgraph_dev_port `" ) - _PID_DIR.mkdir(parents=True, exist_ok=True) + RUNTIME.pid_dir.mkdir(parents=True, exist_ok=True) + # Rotate the log if it has grown past the threshold so this session's + # output starts on a fresh file. Failure is non-fatal (see + # ``_rotate_log_if_needed``). See #209. + _rotate_log_if_needed(RUNTIME.log_file) # Open the log file once and hand it to subprocess.Popen as stdout/stderr. # Popen duplicates the fd into the child via fork+exec, so closing our # parent-side handle in the finally below releases this process's fd # without affecting the child. Without the close, every restart leaks # one fd — a problem on heavy ``/resume`` cycling that could eventually # exhaust the process's open-file limit. - log_handle = open(_LOG_FILE, "ab") # closed in finally below + log_handle = open(RUNTIME.log_file, "ab") # closed in finally below # Propagate workspace to the subprocess so deployed sub-agents resolve # paths.WORKSPACE_ROOT to the same dir as the CLI's main agent. cwd alone @@ -611,7 +699,7 @@ def start_langgraph_dev( log_handle.close() except Exception: pass - _PID_FILE.write_text(str(proc.pid)) + RUNTIME.pid_file.write_text(str(proc.pid)) _write_workspace_sidecar(workspace_dir=workspace_dir, pid=proc.pid) global _PROCESS_WORKSPACE _PROCESS = proc @@ -625,13 +713,13 @@ def start_langgraph_dev( if proc.poll() is not None: tail = "" try: - tail = _LOG_FILE.read_text()[-2000:] + tail = RUNTIME.log_file.read_text()[-2000:] except Exception: pass # Subprocess died on its own — clear our module-level bookkeeping - # (``_PROCESS``, ``_PROCESS_WORKSPACE``, ``_PID_FILE``) before + # (``_PROCESS``, ``_PROCESS_WORKSPACE``, ``RUNTIME.pid_file``) before # raising. Without this, ``_PROCESS`` would keep pointing at the - # dead handle and ``_PID_FILE`` at a non-existent PID, leading + # dead handle and ``RUNTIME.pid_file`` at a non-existent PID, leading # the next ``ensure_langgraph_dev`` to misjudge state. Pass # ``proc`` directly so ``stop_langgraph_dev`` works against the # one we just spawned even if the global state was overwritten. @@ -649,7 +737,7 @@ def start_langgraph_dev( stop_langgraph_dev(proc) raise RuntimeError( - f"langgraph dev did not become healthy within 60 seconds. Check {_LOG_FILE}" + f"langgraph dev did not become healthy within 60 seconds. Check {RUNTIME.log_file}" ) @@ -712,9 +800,9 @@ def stop_langgraph_dev(proc: subprocess.Popen | None = None) -> None: if proc is _PROCESS: _PROCESS = None _PROCESS_WORKSPACE = None - if _PID_FILE.exists(): + if RUNTIME.pid_file.exists(): try: - _PID_FILE.unlink() + RUNTIME.pid_file.unlink() except OSError: pass _unlink_workspace_sidecar() @@ -770,9 +858,9 @@ def ensure_langgraph_dev( # (rapid ``/resume`` in succession, channel threads). Reentrant so # the workspace-restart path can call ``stop_langgraph_dev`` from # inside the critical section. - _PID_DIR.mkdir(parents=True, exist_ok=True) + RUNTIME.pid_dir.mkdir(parents=True, exist_ok=True) try: - with FileLock(str(_FILE_LOCK_PATH), timeout=_FILE_LOCK_TIMEOUT): + with FileLock(str(RUNTIME.lock_file), timeout=_FILE_LOCK_TIMEOUT): with _LOCK: return _ensure_langgraph_dev_locked(config, workspace_dir) except FileLockTimeout: @@ -781,7 +869,7 @@ def ensure_langgraph_dev( "Another CLI shell may be stuck during cold-start. Falling back to " "sync sub-agent delegation for this session.", _FILE_LOCK_TIMEOUT, - _FILE_LOCK_PATH, + RUNTIME.lock_file, ) _ASYNC_SUBAGENTS_AVAILABLE = False return None diff --git a/tests/conftest.py b/tests/conftest.py index a3d8dab..beec4ce 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -108,6 +108,26 @@ def tmp_workspace(tmp_path): return str(ws) +@pytest.fixture +def runtime_paths(tmp_path, monkeypatch): + """Isolate ``langgraph_dev.manager.RUNTIME`` under a temp directory. + + Replaces the module-level ``RUNTIME`` with a fully temp-rooted bundle + so every path (``pid_dir``, ``pid_file``, ``log_file``, + ``workspace_sidecar``, ``lock_file``) is contained under ``tmp_path``. + + Tests that need a variant of a single field can still call + ``dataclasses.replace(runtime_paths, log_file=…)`` etc. — the + baseline is already isolated, so forgetting a field just keeps it + under ``tmp_path``, never ``~/.config/evoscientist``. + """ + from EvoScientist.langgraph_dev import manager + + runtime = manager.LanggraphRuntimePaths.for_directory(tmp_path / "runtime") + monkeypatch.setattr(manager, "RUNTIME", runtime) + return runtime + + # Capture deepagents tool factories at conftest load time — BEFORE any test # imports EvoScientist, which can trigger ``_patch_deepagents_model_passthrough`` # during agent construction. Once captured here, the ``restore_model_passthrough_patch`` diff --git a/tests/test_langgraph_dev_deploy_mode.py b/tests/test_langgraph_dev_deploy_mode.py index 56fd1af..b34c574 100644 --- a/tests/test_langgraph_dev_deploy_mode.py +++ b/tests/test_langgraph_dev_deploy_mode.py @@ -8,6 +8,7 @@ Verifies the single-env-var enum routing: from __future__ import annotations +import dataclasses import subprocess from pathlib import Path @@ -21,7 +22,7 @@ class _PopenAbort(Exception): after the env dict is constructed but before health-polling runs.""" -def _patch_start_prereqs(monkeypatch, tmp_path: Path) -> dict: +def _patch_start_prereqs(monkeypatch, tmp_path: Path, runtime_paths) -> dict: """Mock everything ``start_langgraph_dev`` does before ``subprocess.Popen`` so we can run it end-to-end up to the point where the env dict is captured. Returns a ``captured`` dict that the test populates from the fake Popen.""" @@ -42,10 +43,12 @@ def _patch_start_prereqs(monkeypatch, tmp_path: Path) -> dict: manager, "_wait_for_port_release", lambda _port, timeout=10.0: True ) - # _PID_DIR / _LOG_FILE point under user dir — redirect to tmp. - pid_dir = tmp_path / "pid_dir" - monkeypatch.setattr(manager, "_PID_DIR", pid_dir) - monkeypatch.setattr(manager, "_LOG_FILE", tmp_path / "langgraph_dev.log") + # Redirect the log file — pid_dir already rooted under tmp via the fixture. + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, log_file=tmp_path / "langgraph_dev.log"), + ) def _fake_popen(args, **kwargs): captured["args"] = args @@ -57,8 +60,8 @@ def _patch_start_prereqs(monkeypatch, tmp_path: Path) -> dict: return captured -def test_deploy_mode_true_sets_full(monkeypatch, tmp_path): - captured = _patch_start_prereqs(monkeypatch, tmp_path) +def test_deploy_mode_true_sets_full(monkeypatch, tmp_path, runtime_paths): + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): manager.start_langgraph_dev( @@ -73,8 +76,8 @@ def test_deploy_mode_true_sets_full(monkeypatch, tmp_path): ) -def test_deploy_mode_false_default_sets_stripped(monkeypatch, tmp_path): - captured = _patch_start_prereqs(monkeypatch, tmp_path) +def test_deploy_mode_false_default_sets_stripped(monkeypatch, tmp_path, runtime_paths): + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): # deploy_mode omitted → defaults to False @@ -89,9 +92,11 @@ def test_deploy_mode_false_default_sets_stripped(monkeypatch, tmp_path): ) -def test_deploy_mode_explicitly_false_sets_stripped(monkeypatch, tmp_path): +def test_deploy_mode_explicitly_false_sets_stripped( + monkeypatch, tmp_path, runtime_paths +): """Same as default, but with deploy_mode=False stated explicitly.""" - captured = _patch_start_prereqs(monkeypatch, tmp_path) + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): manager.start_langgraph_dev( @@ -104,11 +109,13 @@ def test_deploy_mode_explicitly_false_sets_stripped(monkeypatch, tmp_path): assert env.get("EVOSCIENTIST_DEPLOY_MODE") == "stripped" -def test_deploy_mode_always_set_to_one_of_full_or_stripped(monkeypatch, tmp_path): +def test_deploy_mode_always_set_to_one_of_full_or_stripped( + monkeypatch, tmp_path, runtime_paths +): """Regression: the subprocess always sees exactly one of the two enum values for ``EVOSCIENTIST_DEPLOY_MODE`` — never unset, never garbage.""" for deploy_mode, expected in ((True, "full"), (False, "stripped")): - captured = _patch_start_prereqs(monkeypatch, tmp_path) + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): manager.start_langgraph_dev( workspace_dir=tmp_path, @@ -122,12 +129,14 @@ def test_deploy_mode_always_set_to_one_of_full_or_stripped(monkeypatch, tmp_path ) -def test_inherited_stripped_overridden_when_deploy_mode_true(monkeypatch, tmp_path): +def test_inherited_stripped_overridden_when_deploy_mode_true( + monkeypatch, tmp_path, runtime_paths +): """If the parent process exports ``EVOSCIENTIST_DEPLOY_MODE=stripped`` and we ask for deploy mode, the subprocess env must see the resolved value (``full``), not the stale inherited one.""" monkeypatch.setenv("EVOSCIENTIST_DEPLOY_MODE", "stripped") - captured = _patch_start_prereqs(monkeypatch, tmp_path) + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): manager.start_langgraph_dev( @@ -142,12 +151,14 @@ def test_inherited_stripped_overridden_when_deploy_mode_true(monkeypatch, tmp_pa ) -def test_inherited_full_overridden_when_deploy_mode_false(monkeypatch, tmp_path): +def test_inherited_full_overridden_when_deploy_mode_false( + monkeypatch, tmp_path, runtime_paths +): """Symmetric: parent exports ``EVOSCIENTIST_DEPLOY_MODE=full``, CLI/serve calls start_langgraph_dev with default (deploy_mode=False), inherited value must be overridden to ``stripped``.""" monkeypatch.setenv("EVOSCIENTIST_DEPLOY_MODE", "full") - captured = _patch_start_prereqs(monkeypatch, tmp_path) + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): manager.start_langgraph_dev( @@ -161,14 +172,14 @@ def test_inherited_full_overridden_when_deploy_mode_false(monkeypatch, tmp_path) ) -def test_inherited_arbitrary_value_overridden(monkeypatch, tmp_path): +def test_inherited_arbitrary_value_overridden(monkeypatch, tmp_path, runtime_paths): """Defense against an unexpected inherited value (e.g. legacy ``true`` from before the enum rename, or any user-set garbage). The resolved deploy_mode always wins.""" for inherited in ("true", "garbage", "FULL", ""): for deploy_mode, expected in ((True, "full"), (False, "stripped")): monkeypatch.setenv("EVOSCIENTIST_DEPLOY_MODE", inherited) - captured = _patch_start_prereqs(monkeypatch, tmp_path) + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): manager.start_langgraph_dev( @@ -185,10 +196,12 @@ def test_inherited_arbitrary_value_overridden(monkeypatch, tmp_path): ) -def test_workspace_dir_env_var_set_regardless_of_mode(monkeypatch, tmp_path): +def test_workspace_dir_env_var_set_regardless_of_mode( + monkeypatch, tmp_path, runtime_paths +): """EVOSCIENTIST_WORKSPACE_DIR is independent of deploy_mode.""" for deploy_mode in (True, False): - captured = _patch_start_prereqs(monkeypatch, tmp_path) + captured = _patch_start_prereqs(monkeypatch, tmp_path, runtime_paths) with pytest.raises(_PopenAbort): manager.start_langgraph_dev( workspace_dir=tmp_path, diff --git a/tests/test_langgraph_dev_workspace_sidecar.py b/tests/test_langgraph_dev_workspace_sidecar.py index 56e9b57..ed14109 100644 --- a/tests/test_langgraph_dev_workspace_sidecar.py +++ b/tests/test_langgraph_dev_workspace_sidecar.py @@ -16,6 +16,7 @@ breaking ``task()`` delegations. from __future__ import annotations +import dataclasses import json import pytest @@ -24,15 +25,22 @@ from EvoScientist.langgraph_dev import manager def test_sidecar_path_is_next_to_pid_file(): - """Sidecar JSON lives at _PID_DIR / 'langgraph_dev.workspace.json'.""" + """Sidecar JSON lives at ``pid_dir / 'langgraph_dev.workspace.json'``.""" assert ( - manager._WORKSPACE_SIDECAR == manager._PID_DIR / "langgraph_dev.workspace.json" + manager.RUNTIME.workspace_sidecar + == manager.RUNTIME.pid_dir / "langgraph_dev.workspace.json" ) -def test_write_workspace_sidecar_records_workspace_and_pid(tmp_path, monkeypatch): +def test_write_workspace_sidecar_records_workspace_and_pid( + tmp_path, monkeypatch, runtime_paths +): """``_write_workspace_sidecar`` writes JSON with workspace + pid.""" - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", tmp_path / "ws.json") + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"), + ) workspace = tmp_path / "some" / "ws" manager._write_workspace_sidecar(workspace_dir=workspace, pid=12345) data = json.loads((tmp_path / "ws.json").read_text()) @@ -40,15 +48,27 @@ def test_write_workspace_sidecar_records_workspace_and_pid(tmp_path, monkeypatch assert data["pid"] == 12345 -def test_read_workspace_sidecar_returns_none_when_missing(tmp_path, monkeypatch): - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", tmp_path / "absent.json") +def test_read_workspace_sidecar_returns_none_when_missing( + tmp_path, monkeypatch, runtime_paths +): + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "absent.json"), + ) assert manager._read_workspace_sidecar() is None -def test_read_workspace_sidecar_returns_none_on_corrupt_json(tmp_path, monkeypatch): +def test_read_workspace_sidecar_returns_none_on_corrupt_json( + tmp_path, monkeypatch, runtime_paths +): sidecar = tmp_path / "bad.json" sidecar.write_text("not json at all") - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", sidecar) + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=sidecar), + ) assert manager._read_workspace_sidecar() is None @@ -67,7 +87,7 @@ def test_read_workspace_sidecar_returns_none_on_corrupt_json(tmp_path, monkeypat ], ) def test_read_workspace_sidecar_returns_none_on_wrong_schema( - payload, tmp_path, monkeypatch + payload, tmp_path, monkeypatch, runtime_paths ): """JSON that parses but doesn't match the expected schema must degrade to None — otherwise the reuse branch's ``Path(sidecar["workspace"]).resolve()`` @@ -75,12 +95,20 @@ def test_read_workspace_sidecar_returns_none_on_wrong_schema( an unhandled exception or producing a misleading match check.""" sidecar = tmp_path / "schema.json" sidecar.write_text(payload) - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", sidecar) + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=sidecar), + ) assert manager._read_workspace_sidecar() is None -def test_read_workspace_sidecar_round_trip(tmp_path, monkeypatch): - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", tmp_path / "rt.json") +def test_read_workspace_sidecar_round_trip(tmp_path, monkeypatch, runtime_paths): + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "rt.json"), + ) workspace = tmp_path / "x" / "y" manager._write_workspace_sidecar(workspace_dir=workspace, pid=42) data = manager._read_workspace_sidecar() @@ -91,11 +119,17 @@ def test_workspace_mismatch_error_is_runtime_error_subclass(): assert issubclass(manager.WorkspaceMismatchError, RuntimeError) -def test_ensure_langgraph_dev_refuses_on_workspace_mismatch(tmp_path, monkeypatch): +def test_ensure_langgraph_dev_refuses_on_workspace_mismatch( + tmp_path, monkeypatch, runtime_paths +): """Cross-process reuse with sidecar workspace ≠ requested → raises.""" ws_a = tmp_path / "A" ws_b = tmp_path / "B" - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", tmp_path / "ws.json") + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"), + ) manager._write_workspace_sidecar(workspace_dir=ws_a, pid=99999) monkeypatch.setattr(manager, "is_langgraph_dev_running", lambda **_kw: True) @@ -111,14 +145,18 @@ def test_ensure_langgraph_dev_refuses_on_workspace_mismatch(tmp_path, monkeypatc def test_ensure_langgraph_dev_refuses_on_mismatch_with_stale_process( - tmp_path, monkeypatch + tmp_path, monkeypatch, runtime_paths ): """A non-None but dead ``_PROCESS`` handle must NOT short-circuit the sidecar check. Regression for the case where our subprocess exited and a different langgraph dev rebound the port.""" ws_a = tmp_path / "A" ws_b = tmp_path / "B" - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", tmp_path / "ws.json") + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"), + ) manager._write_workspace_sidecar(workspace_dir=ws_a, pid=99999) class _DeadProc: @@ -137,10 +175,16 @@ def test_ensure_langgraph_dev_refuses_on_mismatch_with_stale_process( manager.ensure_langgraph_dev(cfg, workspace_dir=ws_b) -def test_ensure_langgraph_dev_reuses_when_workspace_matches(tmp_path, monkeypatch): +def test_ensure_langgraph_dev_reuses_when_workspace_matches( + tmp_path, monkeypatch, runtime_paths +): """Cross-process reuse with matching sidecar workspace → no raise.""" ws_a = tmp_path / "A" - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", tmp_path / "ws.json") + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "ws.json"), + ) manager._write_workspace_sidecar(workspace_dir=ws_a, pid=99999) monkeypatch.setattr(manager, "is_langgraph_dev_running", lambda **_kw: True) @@ -153,10 +197,16 @@ def test_ensure_langgraph_dev_reuses_when_workspace_matches(tmp_path, monkeypatc manager.ensure_langgraph_dev(cfg, workspace_dir=ws_a) -def test_ensure_langgraph_dev_reuses_when_sidecar_missing(tmp_path, monkeypatch): +def test_ensure_langgraph_dev_reuses_when_sidecar_missing( + tmp_path, monkeypatch, runtime_paths +): """Backward compat: pre-feature langgraph dev (no sidecar) falls back to the existing log-warning behavior rather than refusing.""" - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", tmp_path / "absent.json") + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, workspace_sidecar=tmp_path / "absent.json"), + ) monkeypatch.setattr(manager, "is_langgraph_dev_running", lambda **_kw: True) monkeypatch.setattr(manager, "_PROCESS", None) monkeypatch.setattr(manager, "_PROCESS_WORKSPACE", None) @@ -167,12 +217,17 @@ def test_ensure_langgraph_dev_reuses_when_sidecar_missing(tmp_path, monkeypatch) manager.ensure_langgraph_dev(cfg, workspace_dir=tmp_path / "B") -def test_stop_langgraph_dev_removes_sidecar(tmp_path, monkeypatch): +def test_stop_langgraph_dev_removes_sidecar(tmp_path, monkeypatch, runtime_paths): """``stop_langgraph_dev`` should unlink the sidecar alongside the PID file.""" sidecar = tmp_path / "ws.json" pid_file = tmp_path / "pid.txt" - monkeypatch.setattr(manager, "_WORKSPACE_SIDECAR", sidecar) - monkeypatch.setattr(manager, "_PID_FILE", pid_file) + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace( + runtime_paths, workspace_sidecar=sidecar, pid_file=pid_file + ), + ) manager._write_workspace_sidecar(workspace_dir=tmp_path / "x", pid=42) assert sidecar.exists() diff --git a/tests/test_langgraph_manager.py b/tests/test_langgraph_manager.py index cf24621..26a4797 100644 --- a/tests/test_langgraph_manager.py +++ b/tests/test_langgraph_manager.py @@ -7,6 +7,7 @@ to be available. from __future__ import annotations +import dataclasses from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -103,21 +104,31 @@ class TestListPidsOnPort: class TestKillOwnedStaleProcess: - def test_returns_false_if_no_pid_file(self, tmp_path): - with patch.object(manager, "_PID_FILE", tmp_path / "missing.pid"): + def test_returns_false_if_no_pid_file(self, tmp_path, runtime_paths): + with patch.object( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, pid_file=tmp_path / "missing.pid"), + ): assert manager._kill_owned_stale_process(6174) is False - def test_returns_false_if_pid_file_unreadable(self, tmp_path): + def test_returns_false_if_pid_file_unreadable(self, tmp_path, runtime_paths): pid_file = tmp_path / "bad.pid" pid_file.write_text("not-a-number") - with patch.object(manager, "_PID_FILE", pid_file): + with patch.object( + manager, "RUNTIME", dataclasses.replace(runtime_paths, pid_file=pid_file) + ): assert manager._kill_owned_stale_process(6174) is False - def test_returns_false_if_pid_not_in_occupiers(self, tmp_path): + def test_returns_false_if_pid_not_in_occupiers(self, tmp_path, runtime_paths): pid_file = tmp_path / "lg.pid" pid_file.write_text("12345") with ( - patch.object(manager, "_PID_FILE", pid_file), + patch.object( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, pid_file=pid_file), + ), patch.object(manager, "_list_pids_on_port", return_value=[99999]), ): assert manager._kill_owned_stale_process(6174) is False @@ -125,14 +136,18 @@ class TestKillOwnedStaleProcess: # else, not a stale ours. assert pid_file.exists() - def test_refuses_to_kill_recycled_pid(self, tmp_path): + def test_refuses_to_kill_recycled_pid(self, tmp_path, runtime_paths): """PID matches but cmdline doesn't contain 'langgraph' → don't kill.""" pid_file = tmp_path / "lg.pid" pid_file.write_text("12345") fake_proc = MagicMock() fake_proc.cmdline.return_value = ["bash", "-c", "echo hi"] with ( - patch.object(manager, "_PID_FILE", pid_file), + patch.object( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, pid_file=pid_file), + ), patch.object(manager, "_list_pids_on_port", return_value=[12345]), patch.object(manager.psutil, "Process", return_value=fake_proc), ): @@ -142,7 +157,7 @@ class TestKillOwnedStaleProcess: # gone, PID was recycled by an unrelated process). assert not pid_file.exists() - def test_kills_when_cmdline_matches_langgraph(self, tmp_path): + def test_kills_when_cmdline_matches_langgraph(self, tmp_path, runtime_paths): """Owned PID + cmdline contains 'langgraph' → kill + cleanup PID file.""" pid_file = tmp_path / "lg.pid" pid_file.write_text("12345") @@ -153,7 +168,11 @@ class TestKillOwnedStaleProcess: "dev", ] with ( - patch.object(manager, "_PID_FILE", pid_file), + patch.object( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, pid_file=pid_file), + ), patch.object(manager, "_list_pids_on_port", return_value=[12345]), patch.object(manager.psutil, "Process", return_value=fake_proc), ): @@ -161,12 +180,16 @@ class TestKillOwnedStaleProcess: fake_proc.kill.assert_called_once() assert not pid_file.exists() - def test_handles_dead_pid(self, tmp_path): + def test_handles_dead_pid(self, tmp_path, runtime_paths): """PID file claims a PID but the process is gone → cleanup PID file, no error.""" pid_file = tmp_path / "lg.pid" pid_file.write_text("12345") with ( - patch.object(manager, "_PID_FILE", pid_file), + patch.object( + manager, + "RUNTIME", + dataclasses.replace(runtime_paths, pid_file=pid_file), + ), patch.object(manager, "_list_pids_on_port", return_value=[12345]), patch.object( manager.psutil, @@ -184,7 +207,9 @@ class TestKillOwnedStaleProcess: class TestEnsureLanggraphDev: - def test_starts_when_async_disabled_but_memory_workers_enabled(self, tmp_path): + def test_starts_when_async_disabled_but_memory_workers_enabled( + self, tmp_path, runtime_paths + ): """EvoMemory workers can require langgraph dev even without async subagents.""" cfg = EvoScientistConfig() cfg.enable_async_subagents = False @@ -195,8 +220,14 @@ class TestEnsureLanggraphDev: with ( patch.object(manager, "is_langgraph_dev_running", return_value=False), patch.object(manager, "start_langgraph_dev", return_value=proc) as start, - patch.object(manager, "_FILE_LOCK_PATH", tmp_path / "lg.lock"), - patch.object(manager, "_PID_DIR", tmp_path / "pids"), + patch.object( + manager, + "RUNTIME", + dataclasses.replace( + manager.LanggraphRuntimePaths.for_directory(tmp_path / "pids"), + lock_file=tmp_path / "lg.lock", + ), + ), ): result = manager.ensure_langgraph_dev(cfg, workspace_dir=tmp_path) @@ -204,7 +235,9 @@ class TestEnsureLanggraphDev: start.assert_called_once() assert manager.is_async_subagents_available() is True - def test_skips_when_async_and_memory_workers_disabled(self, tmp_path): + def test_skips_when_async_and_memory_workers_disabled( + self, tmp_path, runtime_paths + ): """No background server is needed without async subagents or workers.""" cfg = EvoScientistConfig() cfg.enable_async_subagents = False @@ -214,8 +247,14 @@ class TestEnsureLanggraphDev: with ( patch.object(manager, "is_langgraph_dev_running") as mock_running, patch.object(manager, "start_langgraph_dev") as start, - patch.object(manager, "_FILE_LOCK_PATH", tmp_path / "lg.lock"), - patch.object(manager, "_PID_DIR", tmp_path / "pids"), + patch.object( + manager, + "RUNTIME", + dataclasses.replace( + manager.LanggraphRuntimePaths.for_directory(tmp_path / "pids"), + lock_file=tmp_path / "lg.lock", + ), + ), ): result = manager.ensure_langgraph_dev(cfg, workspace_dir=tmp_path) @@ -224,7 +263,7 @@ class TestEnsureLanggraphDev: start.assert_not_called() assert manager.is_async_subagents_available() is False - def test_reuses_existing_healthy_subprocess(self, tmp_path): + def test_reuses_existing_healthy_subprocess(self, tmp_path, runtime_paths): """When the subprocess is already running, no new Popen call.""" cfg = EvoScientistConfig() cfg.enable_async_subagents = True @@ -235,11 +274,14 @@ class TestEnsureLanggraphDev: manager, "is_langgraph_dev_running", return_value=True ) as mock_running, patch.object(manager, "start_langgraph_dev") as mock_start, - patch.object(manager, "_FILE_LOCK_PATH", tmp_path / "lg.lock"), - # Isolate from real ``~/.config/evoscientist/`` — without this - # patch, the FileLock setup would mkdir the user's actual config - # dir as a test side-effect. - patch.object(manager, "_PID_DIR", tmp_path / "pids"), + patch.object( + manager, + "RUNTIME", + dataclasses.replace( + manager.LanggraphRuntimePaths.for_directory(tmp_path / "pids"), + lock_file=tmp_path / "lg.lock", + ), + ), ): result = manager.ensure_langgraph_dev(cfg, workspace_dir=tmp_path) # We didn't spawn anything — there's already a healthy server. @@ -266,3 +308,163 @@ class TestIsAsyncSubagentsAvailable: assert manager.is_async_subagents_available() is True manager._ASYNC_SUBAGENTS_AVAILABLE = False assert manager.is_async_subagents_available() is False + + +# ============================================================================= +# _rotate_log_if_needed — log rotation for langgraph_dev.log +# ============================================================================= + + +class TestRotateLogIfNeeded: + """``_rotate_log_if_needed`` implements the single-backup rollover + policy from #209. When ``RUNTIME.log_file`` exceeds the module's + ``_LOG_ROTATION_BYTES`` threshold, rename to ``.1`` (overwriting + any existing backup) so the next open() starts fresh. Threshold + is patched to a small value per-test to keep the fixtures tiny. + """ + + def test_no_existing_file_is_noop(self, tmp_path): + log = tmp_path / "langgraph_dev.log" + manager._rotate_log_if_needed(log) + assert not log.exists() + assert not (tmp_path / "langgraph_dev.log.1").exists() + + def test_file_smaller_than_threshold_is_not_rotated(self, tmp_path, monkeypatch): + monkeypatch.setattr(manager, "_LOG_ROTATION_BYTES", 1024) + log = tmp_path / "langgraph_dev.log" + log.write_bytes(b"x" * 100) + manager._rotate_log_if_needed(log) + assert log.exists() + assert log.stat().st_size == 100 + assert not (tmp_path / "langgraph_dev.log.1").exists() + + def test_file_exactly_at_threshold_is_not_rotated(self, tmp_path, monkeypatch): + """Off-by-one: rotation triggers only on strict greater-than. + A log sitting at the threshold size is left alone — the next + session that pushes it over triggers the rollover. + """ + monkeypatch.setattr(manager, "_LOG_ROTATION_BYTES", 1024) + log = tmp_path / "langgraph_dev.log" + log.write_bytes(b"x" * 1024) + manager._rotate_log_if_needed(log) + assert log.exists() + assert log.stat().st_size == 1024 + assert not (tmp_path / "langgraph_dev.log.1").exists() + + def test_file_over_threshold_is_rotated(self, tmp_path, monkeypatch): + monkeypatch.setattr(manager, "_LOG_ROTATION_BYTES", 1024) + log = tmp_path / "langgraph_dev.log" + log.write_bytes(b"x" * 1025) + manager._rotate_log_if_needed(log) + # After rotation: the original path was moved to ``.1``. + # The next ``open(log, "ab")`` will re-create the active file + # at offset 0 (append mode creates if missing). Verify both + # halves of the contract: the backup holds the previous content, + # and the active log is writable from scratch. + assert not log.exists() + backup = tmp_path / "langgraph_dev.log.1" + assert backup.exists() + assert backup.stat().st_size == 1025 + with open(log, "ab") as fh: + fh.write(b"new") + assert log.stat().st_size == 3 # just "new", not appended to backup + + def test_rotation_overwrites_existing_backup(self, tmp_path, monkeypatch): + """A pre-existing ``.1`` from an earlier rotation must be + clobbered by the new rollover — single-backup policy means we + never keep more than one historical copy. + """ + monkeypatch.setattr(manager, "_LOG_ROTATION_BYTES", 1024) + log = tmp_path / "langgraph_dev.log" + backup = tmp_path / "langgraph_dev.log.1" + log.write_bytes(b"x" * 2000) + backup.write_bytes(b"OLD_BACKUP_PAYLOAD_THAT_SHOULD_BE_GONE_NOW") + original_backup_size = backup.stat().st_size + manager._rotate_log_if_needed(log) + assert backup.exists() + assert backup.stat().st_size != original_backup_size + assert backup.stat().st_size == 2000 # now holds the just-rotated log + + def test_failed_rotation_does_not_raise(self, tmp_path, monkeypatch): + """If ``os.replace`` fails (e.g. permission denied on Windows + when another process holds the backup open), the helper logs a + warning and returns — the caller can still open the un-rotated + log and proceed. Failing rotation is non-fatal: the next + ``start_langgraph_dev`` invocation will try again. + """ + monkeypatch.setattr(manager, "_LOG_ROTATION_BYTES", 0) # always rotate + log = tmp_path / "langgraph_dev.log" + log.write_bytes(b"x" * 10) + with patch( + "EvoScientist.langgraph_dev.manager.os.replace", + side_effect=PermissionError("denied"), + ): + # Must not raise. + manager._rotate_log_if_needed(log) + # Original log is left intact (we failed to rotate, didn't corrupt). + assert log.exists() + assert log.stat().st_size == 10 + + +class TestStartLanggraphDevRotatesLog: + """``start_langgraph_dev`` must call ``_rotate_log_if_needed`` before + opening the log handle, so each session starts with either an + existing-but-fresh log or a brand-new file. Verifying the call site + directly (vs. mocking the entire subprocess spawn) keeps the test + cheap while still guarding the integration point. + """ + + def test_rotate_called_before_open(self, tmp_path, monkeypatch): + log = tmp_path / "langgraph_dev.log" + log.write_bytes(b"x" * 2048) # contents don't matter for the check + # Build a fully temp-rooted runtime bundle via + # ``for_directory`` so *every* path (pid_dir, pid_file, + # workspace_sidecar, lock_file) is rooted under ``tmp_path``. + # ``dataclasses.replace(runtime_paths, …)`` would still carry + # ``pid_file`` / ``workspace_sidecar`` / ``lock_file`` from the + # production object pointing at ``~/.config/evoscientist/``. + pid_dir = tmp_path / "pids" + monkeypatch.setattr( + manager, + "RUNTIME", + dataclasses.replace( + manager.LanggraphRuntimePaths.for_directory(pid_dir), + log_file=log, + ), + ) + monkeypatch.setattr(manager, "_LOG_ROTATION_BYTES", 1024) + # This test only verifies log rotation — we must not touch real + # sockets. Patch ``_can_bind_port`` so the bind-poll loop in + # ``_wait_for_port_bindable`` passes immediately regardless of + # whether port 6174 is in use on the dev machine. + monkeypatch.setattr(manager, "_can_bind_port", lambda port: True) + # Make ``_packaged_langgraph_config`` point at a real file so + # ``start_langgraph_dev`` doesn't bail at the existence check + # before reaching the rotation call. + fake_config = tmp_path / "langgraph.json" + fake_config.write_text("{}") + # Don't actually start a subprocess — just verify the rotation + # call happens. We mock the spawn to raise immediately so the + # rest of start_langgraph_dev aborts before doing anything else. + with ( + patch.object(manager, "_langgraph_exe", return_value="/fake/langgraph"), + patch.object( + manager, "_packaged_langgraph_config", return_value=fake_config + ), + patch( + "EvoScientist.langgraph_dev.manager.subprocess.Popen", + side_effect=FileNotFoundError("subprocess not available"), + ), + ): + try: + manager.start_langgraph_dev(workspace_dir=tmp_path) + except FileNotFoundError: + pass # expected — we just need rotation to have happened + # After start attempt, the oversize log must have been rotated. + assert (tmp_path / "langgraph_dev.log.1").exists() + # And the redirect held — nothing leaked into the real + # ``~/.config/evoscientist/`` (we'd have observed a + # ``langgraph_dev.log.1`` *next* to the user's real log, not + # under ``tmp_path``). The ``pid_dir`` we redirected to must + # exist, proving the function reached past the mkdir prelude. + assert pid_dir.is_dir()