feat(mcp): use a 10-minute sliding TTL for reviewer and merger PR leases (Closes #747)

Reviewer and merger PR leases minted a fixed 120-minute expiry (#407 AC6/AC7)
and derived staleness from two further activity bands: stale at 30 minutes,
reclaimable at 60. A session that died — daemon crash, transport flap, client
restart — therefore kept a PR blocked for an hour before anyone could reclaim
it, and two hours before manual cleanup was sanctioned. #718 records the
resulting deadlock: a merge stalled behind a reviewer lease that had stopped
being live long before it stopped being authoritative.

The flaw was using a long fixed expiry as a proxy for "the owner is probably
still alive" instead of making the owner continuously prove liveness.

Sliding window
--------------
`LEASE_TTL_MINUTES = 10` now governs acquisition, and every write of the lease
marker re-derives `expires_at` from the moment of the write, so each heartbeat
slides the window forward. An actively heartbeating session is never evicted
and has no maximum lifetime; a dead one releases its hold within one TTL.

`LEASE_RENEWAL_MINUTES` is named separately from the acquisition TTL. Renewal
previously had no seam at all: the heartbeat slid the expiry only as a side
effect of re-defaulting the acquisition constant, so the two durations could
not be reasoned about or tuned independently. `format_lease_body` now takes an
explicit `ttl_minutes`, and the heartbeat passes the renewal window rather than
relying on that default.

Merger leases acquire through the same lease-body formatter, so they inherit
the identical window by construction rather than by a parallel constant.

Removal of the reclaim tier
---------------------------
`classify_lease_freshness` no longer returns `reclaimable`. Beyond being an
extra waiting tier, that band is unreachable under a sliding TTL: a heartbeat
stamps `last_activity` and `expires_at` together, so a lease idle for a full
TTL is necessarily already expired. Expiry is now the only takeover gate.

No reclaim path is lost. `find_active_reviewer_lease` already ignores expired
markers, so an expired foreign lease never gated acquisition; and the
`foreign_expired` classification carries the same
`NEXT_ACTION_RELEASE_EXPIRED_LEASE` the retired `foreign_reclaimable` did. The
two updated tests in `test_reviewer_pr_lease.py` assert exactly that: the
classification label changes, the sanctioned next action does not.

`STALE_WARNING_MINUTES` drops to 5 — half the window — so the warning still
fires while the owner can heartbeat and recover.

Diagnostics
-----------
Adds `lease_seconds_remaining`, and the heartbeat tool now returns
`ttl_minutes`, `expires_at`, and `seconds_remaining`, so an operator can
distinguish "held and live" from "held and dying" instead of only seeing that
a lease exists. All additions are additive; no existing key changed.

Also removes `pr_work_lease.DEFAULT_REVIEWER_LEASE_TTL_MINUTES`, a duplicate of
the reviewer TTL with no readers anywhere in the tree, which could only drift.

Legacy markers minted under the old 120-minute TTL still parse and are judged
against their own recorded `expires_at`, so no lease is retroactively expired
by this change.

Full suite: 3425 passed, 6 skipped, 2 failed. Both failures
(`test_issue_702_review_findings_f1_f6`, `test_reconciler_supersession_close`)
reproduce identically on clean master b05075fd25 and are pre-existing.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01QxXHZ7rqXtLgTusngaWgKZ
This commit is contained in:
2026-07-18 13:55:46 -04:00
co-authored by Claude Opus 4.8
parent b05075fd25
commit 277ec5269d
5 changed files with 287 additions and 14 deletions
+46 -7
View File
@@ -29,9 +29,19 @@ _ACTIVE_PHASES = frozenset({
"adopted",
})
DEFAULT_LEASE_TTL_MINUTES = 120
STALE_WARNING_MINUTES = 30
RECLAIMABLE_MINUTES = 60
# Reviewer and merger PR leases use a short *sliding* window (#747): a lease
# expires 10 minutes after its last heartbeat, and every heartbeat slides the
# expiry forward. An actively heartbeating session is never evicted, while a
# dead session releases its hold in at most one TTL instead of the two hours
# the previous fixed 120-minute expiry allowed.
LEASE_TTL_MINUTES = 10
# Renewal is named separately from acquisition so the slide amount is tunable
# without silently re-defining how long a fresh lease lives.
LEASE_RENEWAL_MINUTES = 10
# Retained for callers that imported the pre-#747 name.
DEFAULT_LEASE_TTL_MINUTES = LEASE_TTL_MINUTES
# Warn at half the window, while the owner can still heartbeat and recover.
STALE_WARNING_MINUTES = 5
_SESSION_LEASE: dict[str, Any] | None = None
@@ -80,10 +90,19 @@ def format_lease_body(
target_branch_sha: str | None,
last_activity: datetime | None = None,
expires_at: datetime | None = None,
ttl_minutes: int = LEASE_TTL_MINUTES,
blocker: str = "none",
) -> str:
"""Serialize a lease marker.
Every write of this marker — acquisition, heartbeat, adoption — re-derives
``expires_at`` from the moment of the write, which is what makes the TTL
slide (#747). Callers renewing an existing lease pass
``ttl_minutes=LEASE_RENEWAL_MINUTES``; an explicit ``expires_at`` still
wins so a lease can be minted with a deliberate window.
"""
now = last_activity or datetime.now(timezone.utc)
expires = expires_at or (now + timedelta(minutes=DEFAULT_LEASE_TTL_MINUTES))
expires = expires_at or (now + timedelta(minutes=ttl_minutes))
last_text = now.astimezone(timezone.utc).replace(microsecond=0).isoformat().replace(
"+00:00", "Z"
)
@@ -169,8 +188,30 @@ def _minutes_since_activity(lease: dict, *, now: datetime) -> float | None:
return (now - last).total_seconds() / 60.0
def lease_seconds_remaining(lease: dict, *, now: datetime | None = None) -> int | None:
"""Seconds until *lease* expires, clamped at 0; ``None`` if unparsable.
Lets diagnostics distinguish "held and live" from "held and dying" (#747)
rather than only reporting that a lease exists.
"""
expires_at = _parse_timestamp(lease.get("expires_at"))
if not expires_at:
return None
now = now or datetime.now(timezone.utc)
return max(0, int((expires_at - now).total_seconds()))
def classify_lease_freshness(lease: dict, *, now: datetime | None = None) -> str:
"""Return active, stale_warning, reclaimable, expired, or terminal."""
"""Return active, stale_warning, expired, or terminal.
Expiry is the only takeover gate (#747). The pre-#747 ``reclaimable`` band
sat between "stale" and "expired" and made a dead lease wait out a second
timer before anyone could reclaim it. Under a sliding TTL that band is also
unreachable: a heartbeat stamps ``last_activity`` and ``expires_at``
together, so a lease idle for a full TTL is already expired. Foreign
expired leases are handled by the ``foreign_expired`` classification, which
carries the same sanctioned release next-action the old tier did.
"""
now = now or datetime.now(timezone.utc)
phase = (lease.get("phase") or "").strip().lower()
if phase in _TERMINAL_PHASES:
@@ -180,8 +221,6 @@ def classify_lease_freshness(lease: dict, *, now: datetime | None = None) -> str
minutes = _minutes_since_activity(lease, now=now)
if minutes is None:
return "active"
if minutes >= RECLAIMABLE_MINUTES:
return "reclaimable"
if minutes >= STALE_WARNING_MINUTES:
return "stale_warning"
return "active"