fix(langgraph-dev): rotate langgraph_dev.log when it exceeds 50MB (#270)

* 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>
This commit is contained in:
houren Antony
2026-06-09 22:55:44 +08:00
committed by GitHub
parent cd2baa9588
commit 2dc1e227eb
7 changed files with 484 additions and 106 deletions
+2 -2
View File
@@ -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(
+2 -2
View File
@@ -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]"
),
+122 -34
View File
@@ -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 ``<log_path>.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 <other-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