diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 9551534..c1e8412 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -12168,6 +12168,13 @@ def gitea_heartbeat_reviewer_pr_lease( verify_preflight_purity(remote, task="review_pr") h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) + # Slide the sliding TTL forward (#747): the heartbeat is the liveness proof, + # so renewal is stated explicitly rather than inherited from the acquisition + # default. + beat_at = datetime.now(timezone.utc) + renewed_expiry = beat_at + timedelta( + minutes=reviewer_pr_lease.LEASE_RENEWAL_MINUTES + ) body = reviewer_pr_lease.format_lease_body( repo=f"{o}/{r}", pr_number=pr_number, @@ -12180,6 +12187,8 @@ def gitea_heartbeat_reviewer_pr_lease( candidate_head=candidate_head or session.get("candidate_head"), target_branch=session.get("target_branch") or "master", target_branch_sha=target_branch_sha or session.get("target_branch_sha"), + last_activity=beat_at, + ttl_minutes=reviewer_pr_lease.LEASE_RENEWAL_MINUTES, ) comment_url = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments" with _audited( @@ -12220,6 +12229,13 @@ def gitea_heartbeat_reviewer_pr_lease( "phase": phase, "comment_id": posted.get("id"), "session_lease": updated, + # Report the renewed window so an operator can tell "held and live" + # from "held and dying" (#747). + "ttl_minutes": reviewer_pr_lease.LEASE_RENEWAL_MINUTES, + "expires_at": renewed_expiry.replace(microsecond=0) + .isoformat() + .replace("+00:00", "Z"), + "seconds_remaining": reviewer_pr_lease.LEASE_RENEWAL_MINUTES * 60, "reasons": [], } diff --git a/pr_work_lease.py b/pr_work_lease.py index f5a843d..2fb6876 100644 --- a/pr_work_lease.py +++ b/pr_work_lease.py @@ -36,7 +36,8 @@ _TERMINAL_CONFLICT_FIX_PHASES = frozenset({"released", "blocked", "done"}) _ACTIVE_CONFLICT_FIX_PHASES = frozenset({"claimed", "pushing", "pushed"}) DEFAULT_CONFLICT_FIX_TTL_MINUTES = 120 -DEFAULT_REVIEWER_LEASE_TTL_MINUTES = 120 +# The reviewer/merger PR-lease TTL lives in reviewer_pr_lease.LEASE_TTL_MINUTES +# (#747). A second copy here had no readers and could only drift out of sync. def _parse_timestamp(value: str | None) -> datetime | None: diff --git a/reviewer_pr_lease.py b/reviewer_pr_lease.py index 7541a43..a6e1192 100644 --- a/reviewer_pr_lease.py +++ b/reviewer_pr_lease.py @@ -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" diff --git a/tests/test_issue_747_sliding_lease_ttl.py b/tests/test_issue_747_sliding_lease_ttl.py new file mode 100644 index 0000000..13edc92 --- /dev/null +++ b/tests/test_issue_747_sliding_lease_ttl.py @@ -0,0 +1,210 @@ +"""Tests for the 10-minute sliding TTL on reviewer and merger PR leases (#747). + +The lease ledger previously minted a fixed 120-minute expiry and derived +staleness from separate 30/60-minute activity bands. A dead session therefore +held a PR for up to two hours. These tests pin the sliding-window contract: +acquisition mints a 10-minute expiry, every heartbeat slides it forward, and an +expired lease is immediately reclaimable with no intermediate waiting tier. +""" + +import sys +import unittest +from datetime import datetime, timedelta, timezone + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import reviewer_pr_lease as leases + + +def _body( + *, + session_id: str = "session-a", + pr_number: int = 747, + phase: str = "claimed", + last_activity: datetime | None = None, + expires_at: datetime | None = None, + ttl_minutes: int | None = None, +) -> str: + kwargs = {} + if ttl_minutes is not None: + kwargs["ttl_minutes"] = ttl_minutes + return leases.format_lease_body( + repo="Scaled-Tech-Consulting/Gitea-Tools", + pr_number=pr_number, + issue_number=747, + reviewer_identity="rev1", + profile="prgs-reviewer", + session_id=session_id, + worktree="branches/review-pr747", + phase=phase, + candidate_head="a" * 40, + target_branch="master", + target_branch_sha="b" * 40, + last_activity=last_activity, + expires_at=expires_at, + **kwargs, + ) + + +def _comment(**kwargs) -> dict: + return {"id": 1, "body": _body(**kwargs), "user": {"login": "rev1"}} + + +def _minutes_ago(minutes: int) -> datetime: + return datetime.now(timezone.utc) - timedelta(minutes=minutes) + + +class TestSlidingTTLConstant(unittest.TestCase): + """AC6: one named constant per lease kind, no duplicated literals.""" + + def test_ttl_is_ten_minutes(self): + self.assertEqual(leases.LEASE_TTL_MINUTES, 10) + + def test_renewal_window_is_separately_named(self): + self.assertEqual(leases.LEASE_RENEWAL_MINUTES, 10) + + +class TestAcquisitionTTL(unittest.TestCase): + """AC1 / AC2: reviewer and merger acquisition both mint now + 10 minutes.""" + + def test_acquire_mints_ten_minute_expiry(self): + now = datetime(2026, 7, 18, 12, 0, 0, tzinfo=timezone.utc) + lease = leases.parse_lease_comment(_body(last_activity=now)) + expires = leases._parse_timestamp(lease["expires_at"]) + self.assertEqual(expires, now + timedelta(minutes=10)) + + def test_merger_acquisition_shares_the_same_window(self): + # Merger acquisition funnels through the same lease-body formatter, so + # the reviewer TTL is the merger TTL by construction. + now = datetime(2026, 7, 18, 12, 0, 0, tzinfo=timezone.utc) + lease = leases.parse_lease_comment(_body(phase="merging", last_activity=now)) + expires = leases._parse_timestamp(lease["expires_at"]) + self.assertEqual(expires, now + timedelta(minutes=10)) + + +class TestHeartbeatSlides(unittest.TestCase): + """AC3: a heartbeat slides expires_at to now + 10 minutes.""" + + def test_heartbeat_slides_expiry_forward(self): + acquired = datetime(2026, 7, 18, 12, 0, 0, tzinfo=timezone.utc) + beat = acquired + timedelta(minutes=7) + first = leases.parse_lease_comment(_body(last_activity=acquired)) + renewed = leases.parse_lease_comment(_body(last_activity=beat)) + + first_expiry = leases._parse_timestamp(first["expires_at"]) + renewed_expiry = leases._parse_timestamp(renewed["expires_at"]) + + self.assertEqual(renewed_expiry, beat + timedelta(minutes=10)) + self.assertGreater(renewed_expiry, first_expiry) + + def test_renewal_window_is_independently_tunable(self): + # The renewal amount must not be hardwired to the acquisition TTL; + # format_lease_body accepts an explicit window. + now = datetime(2026, 7, 18, 12, 0, 0, tzinfo=timezone.utc) + lease = leases.parse_lease_comment(_body(last_activity=now, ttl_minutes=3)) + expires = leases._parse_timestamp(lease["expires_at"]) + self.assertEqual(expires, now + timedelta(minutes=3)) + + +class TestFreshnessBands(unittest.TestCase): + """AC5: expiry is the only gate; no intermediate reclaim tier.""" + + def test_fresh_lease_is_active(self): + lease = leases.parse_lease_comment(_body(last_activity=_minutes_ago(1))) + self.assertEqual(leases.classify_lease_freshness(lease), "active") + + def test_idle_past_half_ttl_warns_before_expiry(self): + lease = leases.parse_lease_comment(_body(last_activity=_minutes_ago(6))) + self.assertEqual(leases.classify_lease_freshness(lease), "stale_warning") + + def test_lease_expires_after_ten_idle_minutes(self): + lease = leases.parse_lease_comment(_body(last_activity=_minutes_ago(11))) + self.assertEqual(leases.classify_lease_freshness(lease), "expired") + + def test_no_separate_reclaimable_tier_remains(self): + # The old 60-minute reclaim band sat between "stale" and "expired" and + # blocked acquisition. Under a sliding TTL an idle lease is already + # expired, so the tier must not reappear at any idle duration. + for minutes in (11, 30, 65, 121, 600): + lease = leases.parse_lease_comment(_body(last_activity=_minutes_ago(minutes))) + self.assertEqual( + leases.classify_lease_freshness(lease), + "expired", + f"idle {minutes}m should be expired, not a waiting tier", + ) + + +class TestExpiredLeaseIsImmediatelyReclaimable(unittest.TestCase): + """AC5: another session takes over an expired lease with no extra wait.""" + + def setUp(self): + leases.clear_session_lease() + + def _acquire_against(self, comments: list[dict]) -> dict: + return leases.assess_acquire_lease( + comments, + pr_number=747, + reviewer_identity="rev2", + profile="prgs-reviewer", + session_id="session-b", + repo="Scaled-Tech-Consulting/Gitea-Tools", + issue_number=747, + worktree="branches/review-pr747-b", + candidate_head="c" * 40, + target_branch="master", + target_branch_sha="d" * 40, + ) + + def test_expired_foreign_lease_does_not_block_acquisition(self): + comments = [_comment(session_id="dead-session", last_activity=_minutes_ago(11))] + result = self._acquire_against(comments) + self.assertTrue(result["acquire_allowed"], result["reasons"]) + + def test_live_foreign_lease_still_blocks_acquisition(self): + comments = [_comment(session_id="live-session", last_activity=_minutes_ago(2))] + result = self._acquire_against(comments) + self.assertFalse(result["acquire_allowed"]) + self.assertTrue(any("already has active" in r for r in result["reasons"])) + + +class TestRemainingTimeReporting(unittest.TestCase): + """AC7: diagnostics can distinguish 'held and live' from 'held and dying'.""" + + def test_seconds_remaining_on_live_lease(self): + now = datetime(2026, 7, 18, 12, 0, 0, tzinfo=timezone.utc) + lease = leases.parse_lease_comment(_body(last_activity=now)) + remaining = leases.lease_seconds_remaining(lease, now=now + timedelta(minutes=4)) + self.assertEqual(remaining, 360) + + def test_seconds_remaining_is_zero_when_expired(self): + now = datetime(2026, 7, 18, 12, 0, 0, tzinfo=timezone.utc) + lease = leases.parse_lease_comment(_body(last_activity=now)) + remaining = leases.lease_seconds_remaining(lease, now=now + timedelta(minutes=30)) + self.assertEqual(remaining, 0) + + def test_seconds_remaining_is_none_without_parsable_expiry(self): + self.assertIsNone(leases.lease_seconds_remaining({"expires_at": "not-a-time"})) + + +class TestLegacyLeaseRows(unittest.TestCase): + """AC9: leases minted under the old 120-minute TTL still evaluate.""" + + def test_legacy_two_hour_expiry_is_honoured_until_it_passes(self): + now = datetime.now(timezone.utc) + legacy = leases.parse_lease_comment( + _body(last_activity=now - timedelta(minutes=90), expires_at=now + timedelta(minutes=30)) + ) + # Still inside its originally minted window: not expired, but idle long + # enough to warn. + self.assertEqual(leases.classify_lease_freshness(legacy), "stale_warning") + + def test_legacy_row_past_its_own_expiry_is_expired(self): + now = datetime.now(timezone.utc) + legacy = leases.parse_lease_comment( + _body(last_activity=now - timedelta(minutes=180), expires_at=now - timedelta(minutes=60)) + ) + self.assertEqual(leases.classify_lease_freshness(legacy), "expired") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_reviewer_pr_lease.py b/tests/test_reviewer_pr_lease.py index 61efab1..ba55f82 100644 --- a/tests/test_reviewer_pr_lease.py +++ b/tests/test_reviewer_pr_lease.py @@ -79,22 +79,26 @@ class TestReviewerLeaseAcquire(unittest.TestCase): class TestReviewerLeaseFreshness(unittest.TestCase): - def test_stale_warning_after_30_minutes(self): + def test_stale_warning_at_half_the_sliding_window(self): + # #747 warns at half the 10-minute window, while the owner can still + # heartbeat and keep the lease. lease = leases.parse_lease_comment( - _lease_comment(382, "session-a", minutes_ago=35)["body"] + _lease_comment(382, "session-a", minutes_ago=6)["body"] ) self.assertEqual( leases.classify_lease_freshness(lease), "stale_warning", ) - def test_reclaimable_after_60_minutes(self): + def test_expired_once_the_sliding_window_lapses(self): + # Pre-#747 a 65-minute-idle lease was "reclaimable" and had to wait out + # a second timer. It is now simply expired and immediately takeable. lease = leases.parse_lease_comment( _lease_comment(382, "session-a", minutes_ago=65)["body"] ) self.assertEqual( leases.classify_lease_freshness(lease), - "reclaimable", + "expired", ) @@ -281,7 +285,10 @@ class TestReviewerLeaseHandoffDiagnose(unittest.TestCase): self.assertEqual(result["active_lease"]["comment_id"], 8647) self.assertFalse(result["mutation_allowed"]) - def test_foreign_reclaimable_release_expired(self): + def test_foreign_expired_release_expired(self): + # Pre-#747 this classified as "foreign_reclaimable" after the 60-minute + # activity band. Under the sliding TTL the lease is simply expired, and + # the sanctioned next action is unchanged. reclaim = _lease_comment( 592, "foreign-old", phase="claimed", minutes_ago=65 ) @@ -293,7 +300,7 @@ class TestReviewerLeaseHandoffDiagnose(unittest.TestCase): current_reviewer_identity="sysadmin", proposed_worktree="branches/review-pr-592", ) - self.assertEqual(result["classification"], "foreign_reclaimable") + self.assertEqual(result["classification"], "foreign_expired") self.assertEqual( result["next_action"], leases.NEXT_ACTION_RELEASE_EXPIRED_LEASE )