fix(lsp): read a monotonic clock for idle bookkeeping, not the wall clock - #62
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1cc084db0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…(SCA-4633 class) Second instance of the same class as #66: GitHub reported #62 and #63 both CLEAN because each was measured only against main, never against each other. A merge simulation of the real queue shows they collide on agent/lsp/manager.py, so the queue could not land in any order. #62 is the cheaper side to absorb: it is a leaf with no dependents, while #63 is the base of a four-PR stack (#64, #65, #66), so resolving on #63 would force a re-merge and a fresh CI run on all four. Resolution takes both sides rather than either: _last_used keeps #62's _idle_clock() and the stack's second protected cap sweep is preserved. The conflicted hunk was not the whole risk. #63 added new _last_used write sites that git auto-merged with no conflict, and a merge that resolved only the marked hunk would have silently reinstated the wall clock on those paths and quietly undone #62. Audited the merged tree: all three _last_used writes (747, 809, 1090) and the reaper cutoff (1107) use _idle_clock(), and no time.time() remains in manager.py. The handoff deadline keeps time.monotonic() directly, which is correct for an elapsed-time budget. Verified locally: no conflict markers, manager.py compiles. Test execution is left to CI — this host is at 93% disk with the self-hosted runner disk-suspended (SCA-4625).
CI caught the exact interaction the merge created. The e2e cap fixtures seeded _last_used with time.time() and compared it against a wall-clock cutoff. Once #62 moved the service's idle bookkeeping to time.monotonic, those became an epoch (~1.78e9) measured against an uptime (~6e4), so nothing could ever look idle: test_cap_holds_the_fleet_with_every_client_active - every ts > cutoff assertion vacuously false test_the_idle_reaper_still_works_under_the_cap - the seeded key is never below the reaper's cutoff, so it is never reaped Neither PR was wrong alone, and neither could see this: the fixtures live on #63's lineage and the clock change lives on #62, so the two only meet once the queue is composed. This is the same blind spot as the merge collisions themselves, one layer down. Fixed by reading _idle_clock() rather than hardcoding time.monotonic, so the fixtures track whatever clock the service uses if it changes again. Swept every _last_used site in tests/. The remaining seeds in test_client_cap.py (100.0, 200.0, float(index)) drive LRU ordering, which only compares values to each other and is clock-agnostic. test_service.py keeps its time.time()-based FakeClock: that is the deliberate positive control proving the wall clock fails, and changing it would delete the teeth of #62's own test. The remaining time.time() calls in this file are real-elapsed wait loops for process death, not idle bookkeeping. Verified with a positive control: reverting this file reproduces exactly the two CI failures and no others; with it, 8 pass. 59 pass across test_client_cap_e2e.py, test_client_cap.py and test_service.py.
Addresses the open Codex P2 on #62. The monotonic clock this PR introduced fixes wall-clock stepping but has the mirror-image bug: 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. Each sleep/wake cycle then leaks another generation of language servers -- strictly worse than the wall clock, which at least aged them, and a revival of the exact accumulation the reaper exists to close. _idle_clock() now reads CLOCK_BOOTTIME, which is monotonic *and* counts suspended time, falling back to monotonic() on platforms that lack it (macOS, some BSDs). Resolution stays per-call so the module's `time` reference remains patchable by the clock tests. The existing test modelled suspend as a *backward* wall-clock step, which is the NTP case, not the Linux suspend case -- the two have opposite shapes. That test is kept (renamed in intent, still covering NTP) and _SuspendedClock is added to model what the kernel actually does: wall clock and BOOTTIME advance, monotonic frozen. Positive control: test_reaper_is_immune_to_suspend and test_idle_clock_counts_suspended_time both fail against the monotonic-only resolver (client survives an hour of suspend) and pass against this one. Fallback path covered for no-BOOTTIME platforms. Tests: tests/agent/lsp/ 118 passed.
Merge-order composition verified for the LSP stackAll of #62–#67 report Ancestry (
|
…or-byte Both #62 and #76 add contributors/emails/engineer@scaffolde.ai as a NEW file with different bytes (#62 carries a provenance comment, #76 did not). That is an add/add conflict: the two PRs merge clean against main individually and collide with each other, which GitHub's per-PR mergeStateStatus cannot see. Detected by scripts/pr_merge_order.py (SCA-4638). Aligning #76 to #62's exact bytes makes the add/add resolve trivially in either merge order. The mapped login is identical (pai-scaffolde) either way, so attribution is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lock 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 <noreply@anthropic.com>
6624173 to
dccbe5b
Compare
get_status() computed each client's age with time.time() while _last_used is stamped by _idle_clock() (CLOCK_BOOTTIME since #62), so every reported idle_seconds was the gap between the two epochs (~1.79e9) rather than an elapsed time. The comment directly above the line already required both to read the same clock; this makes the code match it. Surfaced by this PR's own guard test once #62 landed on main: test_status_idle_seconds_reads_the_same_clock_as_last_used. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rce (SCA-4721) (#76) * fix(lsp): guard a non-finite idle_timeout and report the bounds in force (SCA-4721) 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 <noreply@anthropic.com> * fix(lsp): read idle_seconds from the idle clock, not the wall clock get_status() computed each client's age with time.time() while _last_used is stamped by _idle_clock() (CLOCK_BOOTTIME since #62), so every reported idle_seconds was the gap between the two epochs (~1.79e9) rather than an elapsed time. The comment directly above the line already required both to read the same clock; this makes the code match it. Surfaced by this PR's own guard test once #62 landed on main: test_status_idle_seconds_reads_the_same_clock_as_last_used. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The idle reaper is on the wall clock, and the wall clock steps
LSPService's idle bookkeeping compared_last_usedagainsttime.time():Those values are only ever compared to each other. What they need is elapsed time, which the wall clock does not provide — NTP correction and sleep/wake both step it, and this fork runs on laptops and a Mac Mini that sleep.
idle_timeout: every cutoff is dragged into the past, no client ever looks idle, and the reaper silently stops reaping — reviving the unbounded language-server accumulation the reaper exists to close.Neither failure surfaces anywhere. The reaper keeps sweeping and keeps logging; it just stops finding anything.
The change
Routes the four idle-bookkeeping sites through a single
_idle_clock()returningtime.monotonic(), so the clock source is declared once, documented once, and greppable.The change is closed:
_last_usedis internal, and nothing outside the reaper reads it, serializes it, or reports it as an absolute time (grep -rn _last_used --include='*.py'→manager.pyplus one test doing purely relative arithmetic). Swapping the clock changes no observable value.+87 / −4 across 2 files. Deliberately narrow: no config surface, no docs churn, no population cap.
ruff formatis not clean on either file onmaineither and CI does not run it, so no reformatting is included.Verification
Positive control first, because a regression test that has never been seen red is indistinguishable from one that cannot go red. The new test steps the wall clock an hour into the past mid-flight while leaving
monotonic()untouched:test_reaper_is_immune_to_wall_clock_stepsorigin/main'smanager.py(unfixed)Rest:
pytest tests/agent/lsp/→ 61 passed (was 60; this adds 1)ruff check agent/lsp/manager.py tests/agent/lsp/test_service.py→ All checks passedHERMES_HOME; production~/.hermes/logs/agent.logbyte delta 0 (the pollution in fix(tests): stop the LSP suite writing into the operator's real agent.log #61 is still unmerged, so this was contained by hand)Provenance and relationship to the open SCA-4389 PRs
time.monotonic()here is salvaged from #50, where it was hardening earned in a Codex review round. #50/#51/#52 predate upstream's reaper landing onmain(d7578018c5+24a56f027c, 2026-07-29) and now conflict with it inmanager.py/eventlog.py; #58 is cap-only and does not touch the clock. So this fix is orthogonal to whatever is decided about those four, and is the one piece of that work that measurement did not retire.Upstreamable as-is — it is a fix to upstream-authored code with no fork-local dependency.