From dccbe5bad6d048e0842a068ced34ab66990c7335 Mon Sep 17 00:00:00 2001 From: PAI Date: Wed, 19 Aug 2026 20:43:54 -0700 Subject: [PATCH] fix(lsp): read a monotonic clock for idle bookkeeping, not the wall clock Replayed onto current main. This branch was stacked on PRs that landed as squashes, so its original history conflicted with itself; only this PR's own delta is kept. Co-Authored-By: Claude Fable 5 --- agent/lsp/manager.py | 44 ++++- contributors/emails/engineer@scaffolde.ai | 2 + tests/agent/lsp/test_client_cap_e2e.py | 20 ++- tests/agent/lsp/test_service.py | 189 ++++++++++++++++++++++ 4 files changed, 244 insertions(+), 11 deletions(-) create mode 100644 contributors/emails/engineer@scaffolde.ai diff --git a/agent/lsp/manager.py b/agent/lsp/manager.py index 252f3d279abd..57720f631be7 100644 --- a/agent/lsp/manager.py +++ b/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 diff --git a/contributors/emails/engineer@scaffolde.ai b/contributors/emails/engineer@scaffolde.ai new file mode 100644 index 000000000000..a900a4519ff6 --- /dev/null +++ b/contributors/emails/engineer@scaffolde.ai @@ -0,0 +1,2 @@ +pai-scaffolde +# SCA-4628 LSP queue restack: e2e cap fixture clock fix diff --git a/tests/agent/lsp/test_client_cap_e2e.py b/tests/agent/lsp/test_client_cap_e2e.py index 92ab3e19d245..cb3cfba452a7 100644 --- a/tests/agent/lsp/test_client_cap_e2e.py +++ b/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" diff --git a/tests/agent/lsp/test_service.py b/tests/agent/lsp/test_service.py index 9dd7468ca9c6..fca74c09c845 100644 --- a/tests/agent/lsp/test_service.py +++ b/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() + +