Skip to content

Let a deploy rerun reclaim its own failed attempt's lease - #440

Merged
tonyalaribe merged 1 commit into
masterfrom
fix/deploy-rerun-reclaims-own-lease
Oct 26, 2026
Merged

tonyalaribe merged 1 commit into
masterfrom
fix/deploy-rerun-reclaims-own-lease

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

On 2026-10-26 a HANDOFF refusal on 64ae142 left an unresolved production lease. Running gh run rerun then waited 45 minutes and failed. A rerun keeps its run id, so run_is_finished asks GitHub about the rerun itself, gets "running", and treats the holder as alive. Only a fresh workflow_dispatch (new run id) could reclaim the lease.

  • Leases now record GITHUB_RUN_ATTEMPT.
  • abandoned() treats an earlier attempt of the same run as finished. GitHub only starts a new attempt after the previous one completes.
  • All other reclaim proofs are unchanged: the production-boot witness and the receipt witness.
  • An equal attempt, a missing attempt and another run's id all still mean "alive".

Test: test_a_rerun_reclaims_its_own_failed_attempts_lease. It fails with the check disabled (red arm checked).

Helper tests (scripts/deploy/test_*.py, production-image, ci-selftest) pass locally. make ci-status shows all five Rust checks already attested (scripts/deploy is not an input), and the push does not deploy (scripts/deploy/** is in paths-ignore).

🤖 Generated with Claude Code

@claude

claude Bot commented Oct 26, 2026

Copy link
Copy Markdown

Review of #440: the logic looks correct, and I have only minor suggestions.

Checked

  • superseded_attempt fails closed. A missing GITHUB_RUN_ID or GITHUB_RUN_ATTEMPT, a None attempt in the record, or a non-numeric value all raise KeyError, TypeError or ValueError, which are caught, so the lease reads as alive.
  • Lease updates use dict(record, **changes), so run_attempt survives the mutating transition.
  • Only the run_is_finished proof is short-circuited. The production-boot and receipt witnesses still gate reclaim afterwards, so a rerun can't reclaim a lease whose rollout actually replaced production.
  • The test covers a later attempt, an equal attempt and another run's id. The PR says the red arm was checked.

Suggestions (non-blocking)

  1. The chained comparison a == b is not None parses as a == b and b is not None. It is correct, but easy to misread and the one-liner is long. Splitting it would help:
    run_id = os.environ.get('GITHUB_RUN_ID')
    if not run_id or record.get('run_id') != run_id: return False
  2. A legacy lease with no run_attempt defaults to 1, so any rerun (attempt ≥ 2) of that run reclaims it. That is the right outcome here, and the behaviour is sound. The PR text says a missing attempt means alive, but for the record the code treats it as attempt 1. The test doesn't cover it, so consider a case for it or an updated description.
  3. Style: the record dict in hold() is now a very long line.

LGTM otherwise. 🤖 Generated with Claude Code

@monoscope-bot

monoscope-bot Bot commented Oct 26, 2026

Copy link
Copy Markdown

Monoscope production impact · Worth checking

Project 87576849-4941-49d3-a15d-680fef88a1a8 · reviewed b9077f04ea812adcaa48fd91ee53138ebda34245 · advisory

Evidence window (UTC): 2026-10-19 11:50:30.577750102 UTC → 2026-10-26 11:50:30.577750102 UTC

scripts/deploy/lease.py:170 (Head)

Production details are available in Monoscope.

telemetry evidence

Coverage: Open Monoscope for evidence and coverage details.

Review history and settings

@tonyalaribe
tonyalaribe merged commit b9077f0 into master Oct 26, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant