fix(lsp): read a monotonic clock for idle bookkeeping, not the wall clock

agent/lsp/manager.py

@@ -177,6 +177,42 @@ def _log_cap_enforcement_result(fut: Future) -> None:
         logger.debug("detached LSP cap enforcement failed: %s", e)
 
 
+def _idle_clock() -> float:
+    """Clock for idle bookkeeping — suspend-inclusive, never the wall clock.
+
+    ``_last_used`` and the reaper's cutoff are only ever compared to each
+    other, so they need elapsed time, not a date.  The wall clock supplies
+    neither guarantee: NTP correction and sleep/wake both step it, and a
+    backwards step of more than ``idle_timeout`` pushes every cutoff into
+    the past and stalls reaping until the clock catches up (a forwards step
+    does the opposite and reaps servers that are still in use).  Both
+    failure modes are silent, and reviving unbounded accumulation is the
+    exact leak the reaper exists to close.
+
+    ``CLOCK_MONOTONIC`` fixes the stepping but introduces the mirror-image
+    bug: on Linux it *stops* while the machine is suspended.  A laptop that
+    sleeps for longer than ``idle_timeout`` therefore wakes with every
+    ``_last_used`` stamp still inside the window, and each sleep/wake cycle
+    leaks another generation of servers — worse than the wall clock, which
+    at least aged them.  ``CLOCK_BOOTTIME`` is monotonic *and* counts
+    suspended time, so it is the only source that satisfies both halves.
+
+    Platforms without ``CLOCK_BOOTTIME`` (macOS, some BSDs) fall back to
+    ``monotonic()``.  The resolution is deliberately per-call rather than
+    cached at import: it keeps the module's ``time`` reference patchable by
+    the suspend/NTP tests, and a ``getattr`` is noise next to a sweep.
+    """
+    boottime_id = getattr(time, "CLOCK_BOOTTIME", None)
+    if boottime_id is not None:
+        try:
+            return time.clock_gettime(boottime_id)
+        except (OSError, ValueError, AttributeError):
+            # Kernel or libc refused the clock — degrade rather than take
+            # the whole reaper down with an unhandled error.
+            pass
+    return time.monotonic()
+
+
 class _BackgroundLoop:
     """A daemon thread that owns one asyncio event loop.
 
@@ -729,7 +765,7 @@ async def _get_or_spawn(self, file_path: str) -> Optional[LSPClient]:
         with self._state_lock:
             client = self._clients.get(key)
             if client is not None and client.is_running:
-                self._last_used[key] = time.time()
+                self._last_used[key] = _idle_clock()
                 eventlog.log_active(srv.server_id, per_server_root)
                 return client
             spawning = self._spawning.get(key)
@@ -791,7 +827,7 @@ async def _get_or_spawn(self, file_path: str) -> Optional[LSPClient]:
                 return None
             with self._state_lock:
                 self._clients[key] = client
-                self._last_used[key] = time.time()
+                self._last_used[key] = _idle_clock()
             # Second sweep, protecting the client this call returns:
             # concurrent spawns for other roots can still have raced the
             # reservation above. Evicting the caller's own client would
@@ -1072,7 +1108,7 @@ def _touch(self, client: LSPClient) -> None:
         key = (client.server_id, client.workspace_root)
         with self._state_lock:
             if key in self._clients:
-                self._last_used[key] = time.time()
+                self._last_used[key] = _idle_clock()
 
     async def _idle_reaper_loop(self) -> None:
         interval = min(60.0, self._idle_timeout)
@@ -1089,7 +1125,7 @@ async def _idle_reaper_loop(self) -> None:
                 logger.debug("LSP idle reaper sweep error: %s", e)
 
     async def _reap_idle_once(self) -> None:
-        cutoff = time.time() - self._idle_timeout
+        cutoff = _idle_clock() - self._idle_timeout
         with self._state_lock:
             # The in-flight guard closes the residual race the
             # MIN_IDLE_TIMEOUT clamp only makes unlikely: reaping a client

contributors/emails/engineer@scaffolde.ai

@@ -0,0 +1,2 @@
+pai-scaffolde
+# SCA-4628 LSP queue restack: e2e cap fixture clock fix

tests/agent/lsp/test_client_cap_e2e.py

@@ -24,7 +24,7 @@
 import pytest
 
 from agent.lsp.client import LSPClient
-from agent.lsp.manager import LSPService
+from agent.lsp.manager import LSPService, _idle_clock
 from agent.lsp.servers import SERVERS, ServerContext, ServerDef, SpawnSpec
 
 
@@ -153,8 +153,11 @@ def test_cap_holds_the_fleet_with_every_client_active(workspaces):
             f"cap={cap} must hold the fleet at {cap}, got {len(svc._clients)}"
         )
 
-        # Prove the reaper could not have produced this result.
-        cutoff = time.time() - svc._idle_timeout
+        # Prove the reaper could not have produced this result.  Read the
+        # service's own idle clock: ``_last_used`` is written with it, and
+        # comparing those values against a wall-clock cutoff compares an
+        # uptime to an epoch, which is vacuously false for every client.
+        cutoff = _idle_clock() - svc._idle_timeout
         assert all(ts > cutoff for ts in svc._last_used.values()), (
             "every surviving client must be non-idle, otherwise this test "
             "proves nothing the idle reaper does not already cover"
@@ -198,9 +201,12 @@ def test_the_least_recently_used_workspace_is_the_victim(workspaces):
         key_a = next(k for k in svc._clients if str(fa.parent) in k[1])
         key_b = next(k for k in svc._clients if str(fb.parent) in k[1])
 
-        # Make A unambiguously the least-recently-used.
-        svc._last_used[key_a] = time.time() - 120.0
-        svc._last_used[key_b] = time.time()
+        # Make A unambiguously the least-recently-used.  Seeded from the
+        # service's own idle clock so these stay comparable to the values
+        # the spawn path writes.
+        now = _idle_clock()
+        svc._last_used[key_a] = now - 120.0
+        svc._last_used[key_b] = now
 
         svc.get_diagnostics_sync(str(workspaces("c")))
 
@@ -228,7 +234,7 @@ def test_the_idle_reaper_still_works_under_the_cap(workspaces):
         assert not svc._idle_reaper_task.done()
 
         key = next(iter(svc._clients))
-        svc._last_used[key] = time.time() - (svc._idle_timeout + 60.0)
+        svc._last_used[key] = _idle_clock() - (svc._idle_timeout + 60.0)
         svc._loop.run(svc._reap_idle_once(), timeout=5.0)
 
         assert key not in svc._clients, "idle reaping must still work under the cap"

tests/agent/lsp/test_service.py

@@ -219,6 +219,195 @@ async def _flaky_reap():
         svc.shutdown()
 
 
+class _SteppedWallClock:
+    """Stand-in for the ``time`` module with a stepped wall clock.
+
+    ``time()`` is offset (an NTP correction); ``monotonic()`` is
+    untouched, which is precisely the guarantee the idle bookkeeping is
+    supposed to rely on.
+
+    This models an NTP correction *only*.  Suspend/resume is a different
+    failure mode with the opposite shape — see :class:`_SuspendedClock`.
+    """
+
+    def __init__(self, offset: float) -> None:
+        self._offset = offset
+
+    def time(self) -> float:
+        return time.time() + self._offset
+
+    def monotonic(self) -> float:
+        return time.monotonic()
+
+    def sleep(self, seconds: float) -> None:
+        time.sleep(seconds)
+
+
+class _SuspendedClock:
+    """Stand-in for the ``time`` module across a Linux suspend/resume.
+
+    Models what the kernel actually does, which is the inverse of an NTP
+    step: ``CLOCK_MONOTONIC`` **stops** while the machine is suspended,
+    while ``CLOCK_BOOTTIME`` keeps counting.  So after a resume the wall
+    clock and BOOTTIME have both advanced by the suspend duration and
+    ``monotonic()`` has not moved at all.
+
+    ``has_boottime=False`` models a platform without ``CLOCK_BOOTTIME``
+    (macOS), where the resolver must fall back to ``monotonic()``.
+    """
+
+    # Linux's real value; only identity across the two calls matters.
+    _BOOTTIME_ID = 7
+
+    def __init__(self, suspended_for: float, *, has_boottime: bool = True) -> None:
+        self._suspended_for = suspended_for
+        self._base_monotonic = time.monotonic()
+        # Set per-instance so ``getattr(time, "CLOCK_BOOTTIME", None)``
+        # genuinely misses on the no-BOOTTIME platform.
+        if has_boottime:
+            self.CLOCK_BOOTTIME = self._BOOTTIME_ID
+
+    def time(self) -> float:
+        return time.time() + self._suspended_for
+
+    def monotonic(self) -> float:
+        # Frozen: no awake time has elapsed since the fixture was built.
+        return self._base_monotonic
+
+    def clock_gettime(self, clk_id: int) -> float:
+        if clk_id != self._BOOTTIME_ID:
+            raise ValueError(f"unexpected clock id {clk_id}")
+        return self._base_monotonic + self._suspended_for
+
+    def sleep(self, seconds: float) -> None:
+        time.sleep(seconds)
+
+
+def test_idle_clock_counts_suspended_time(monkeypatch):
+    """``_idle_clock`` must read a suspend-inclusive clock where one exists.
+
+    ``CLOCK_MONOTONIC`` stops while the machine is suspended, so a laptop
+    that sleeps longer than ``idle_timeout`` wakes with every ``_last_used``
+    stamp still inside the window.  Reading ``CLOCK_BOOTTIME`` instead
+    counts the suspended time and keeps the cutoff honest.
+
+    Positive control: this fails against a ``time.monotonic()`` resolver,
+    which returns the frozen base instead of the advanced value.
+    """
+    from agent.lsp import manager as manager_mod
+
+    clock = _SuspendedClock(3600.0)
+    monkeypatch.setattr(manager_mod, "time", clock)
+
+    assert manager_mod._idle_clock() == pytest.approx(
+        clock._base_monotonic + 3600.0
+    ), "idle bookkeeping ignored suspended time — it must read CLOCK_BOOTTIME"
+
+
+def test_idle_clock_falls_back_to_monotonic_without_boottime(monkeypatch):
+    """No ``CLOCK_BOOTTIME`` (macOS) must degrade to ``monotonic``, not crash."""
+    from agent.lsp import manager as manager_mod
+
+    clock = _SuspendedClock(3600.0, has_boottime=False)
+    monkeypatch.setattr(manager_mod, "time", clock)
+
+    assert not hasattr(clock, "CLOCK_BOOTTIME")
+    assert manager_mod._idle_clock() == pytest.approx(clock._base_monotonic)
+
+
+def test_reaper_is_immune_to_suspend(mock_pyright, monkeypatch):
+    """A suspend longer than ``idle_timeout`` must not stall the reaper.
+
+    The inverse of the NTP case below: here the wall clock jumps *forward*
+    and ``monotonic()`` freezes.  Against a ``monotonic()``-based cutoff the
+    client looks freshly used no matter how long the machine slept, so the
+    fleet accumulates across every sleep/wake cycle — exactly the leak the
+    reaper exists to close.
+
+    Positive control: this test fails against a ``monotonic()``-based
+    reaper (the client survives the sweep) and passes against a
+    BOOTTIME-based one.
+    """
+    from agent.lsp import manager as manager_mod
+
+    repo = mock_pyright
+    f = repo / "x.py"
+    f.write_text("")
+    svc = LSPService(
+        enabled=True,
+        wait_mode="document",
+        wait_timeout=3.0,
+        install_strategy="manual",
+        idle_timeout=60.0,  # sweeps manually below; the loop never fires
+    )
+    try:
+        svc.get_diagnostics_sync(str(f))
+        key = next(iter(svc._clients))
+
+        # The client was last used "just now" — well inside the timeout.
+        # Then the machine suspends for an hour, which advances BOOTTIME
+        # but leaves monotonic exactly where it was.
+        svc._last_used[key] = manager_mod._idle_clock()
+        monkeypatch.setattr(manager_mod, "time", _SuspendedClock(3600.0))
+
+        svc._loop.run(svc._reap_idle_once(), timeout=5.0)
+
+        assert key not in svc._clients, (
+            "an hour of suspend did not age the client — idle bookkeeping "
+            "must count suspended time (CLOCK_BOOTTIME), not awake time only"
+        )
+        assert svc.get_status()["clients"] == []
+    finally:
+        svc.shutdown()
+
+
+def test_reaper_is_immune_to_wall_clock_steps(mock_pyright, monkeypatch):
+    """A backwards wall-clock step must not stall the idle reaper.
+
+    Idle bookkeeping compares timestamps only to each other, so it must
+    read a monotonic clock.  Against ``time.time()`` an NTP correction or
+    a sleep/wake resume that steps the clock back further than
+    ``idle_timeout`` drags the cutoff into the past, no client ever looks
+    idle, and unbounded accumulation — the leak the reaper exists to
+    close — silently comes back.
+
+    Positive control: this test fails against a ``time.time()``-based
+    reaper (the client survives the sweep) and passes against a
+    monotonic one.
+    """
+    from agent.lsp import manager as manager_mod
+
+    repo = mock_pyright
+    f = repo / "x.py"
+    f.write_text("")
+    svc = LSPService(
+        enabled=True,
+        wait_mode="document",
+        wait_timeout=3.0,
+        install_strategy="manual",
+        idle_timeout=60.0,  # sweeps manually below; the loop never fires
+    )
+    try:
+        svc.get_diagnostics_sync(str(f))
+        key = next(iter(svc._clients))
+
+        # Idle for longer than the timeout, in whichever clock is in use.
+        svc._last_used[key] = svc._last_used[key] - 120.0
+
+        # Now step the wall clock an hour into the past, mid-flight.
+        monkeypatch.setattr(manager_mod, "time", _SteppedWallClock(-3600.0))
+
+        svc._loop.run(svc._reap_idle_once(), timeout=5.0)
+
+        assert key not in svc._clients, (
+            "a backwards wall-clock step stalled the reaper — idle "
+            "bookkeeping must read a monotonic clock"
+        )
+        assert svc.get_status()["clients"] == []
+    finally:
+        svc.shutdown()
+
+