From b9077f04ea812adcaa48fd91ee53138ebda34245 Mon Sep 17 00:00:00 2001 From: Anthony Alaribe Date: Mon, 26 Oct 2026 12:48:22 +0100 Subject: [PATCH] Let a deploy rerun reclaim the lease its own failed attempt left behind --- scripts/deploy/lease.py | 17 +++++++++++++++-- scripts/deploy/test_lease.py | 17 +++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/scripts/deploy/lease.py b/scripts/deploy/lease.py index bf0b55459..231b09dc7 100644 --- a/scripts/deploy/lease.py +++ b/scripts/deploy/lease.py @@ -90,6 +90,19 @@ def run_is_finished(run_id): return done.returncode == 0 and done.stdout.strip() == 'completed' +def superseded_attempt(record): + """True when this process is a later attempt of the run that wrote `record`. + + A rerun keeps its run id, so `run_is_finished` reports the holder alive and the + rerun waits out its own failed attempt's lease. GitHub starts a new attempt only + after the previous one completed, so an older attempt of THIS run is dead. + """ + try: + return record.get('run_id') == os.environ.get('GITHUB_RUN_ID') is not None and int(record.get('run_attempt', 1)) < int(os.environ['GITHUB_RUN_ATTEMPT']) + except (KeyError, TypeError, ValueError): + return False + + @dataclass class RolloutGuard: owner: str @@ -154,7 +167,7 @@ def abandoned(self, commit): if record.get('mutating') is not False: # Reclaimable only once the holder is PROVABLY gone. Age alone never # qualifies a mutating lease, and a holder we cannot ask about never does. - if not run_is_finished(record.get('run_id')): + if not (superseded_attempt(record) or run_is_finished(record.get('run_id'))): return False # A finished run cannot still be mid-rollout. What remains is whether # it ever REPLACED anything, and there are two independent witnesses — @@ -201,7 +214,7 @@ def receipt_image_differs(self, image): @contextlib.contextmanager def hold(self, image, wait_seconds=2700): record = {'image': image, 'nonce': uuid.uuid4().hex, 'mutating': False, - 'run_id': os.environ.get('GITHUB_RUN_ID'), 'pid': os.getpid(), 'started': time.time()} + 'run_id': os.environ.get('GITHUB_RUN_ID'), 'run_attempt': os.environ.get('GITHUB_RUN_ATTEMPT'), 'pid': os.getpid(), 'started': time.time()} owner = self.commit(record) deadline = time.monotonic() + wait_seconds last_notice = 0 diff --git a/scripts/deploy/test_lease.py b/scripts/deploy/test_lease.py index 77e5f51bc..1def9aa2f 100644 --- a/scripts/deploy/test_lease.py +++ b/scripts/deploy/test_lease.py @@ -1,4 +1,5 @@ import contextlib +import os import pathlib import subprocess import tempfile @@ -138,6 +139,22 @@ def test_a_mutating_lease_is_reclaimed_when_production_predates_it(self): with mock.patch.object(lease_module, 'run_is_finished', return_value=False): self.assertFalse(b.abandoned(held), 'a live holder keeps its lease') + def test_a_rerun_reclaims_its_own_failed_attempts_lease(self): + """2026-10-26: a failed HANDOFF left an unresolved lease, and `gh run rerun` + waited 45 minutes on it because the rerun shares the run id, so GitHub + reported the holder still running. A later attempt proves the earlier one + finished; an equal or unknown attempt proves nothing.""" + started = 1_000_000 + with repo() as (a, b), self.stale_after(0), self.serving_boot(boot_micros=(started - 3600) * 1_000_000): + a.git('push', 'origin', a.commit({'image': 'image-a', 'mutating': True, 'started': started, 'run_id': '123', 'run_attempt': '1'}) + ':' + LEASE_REF) + held = a.remote(LEASE_REF) + with mock.patch.object(lease_module, 'run_is_finished', return_value=False): + for attempt, reclaimed in [('2', True), ('1', False)]: + with mock.patch.dict(os.environ, {'GITHUB_RUN_ID': '123', 'GITHUB_RUN_ATTEMPT': attempt}): + self.assertEqual(b.abandoned(held), reclaimed, f'attempt {attempt}') + with mock.patch.dict(os.environ, {'GITHUB_RUN_ID': '456', 'GITHUB_RUN_ATTEMPT': '2'}): + self.assertFalse(b.abandoned(held), 'another run still running keeps its lease') + def test_a_lease_that_did_replace_production_is_not_reclaimed_early(self): """The direction that must never be wrong.