Repository navigation
refactor(autoland): handle Ctrl+C once at the run boundary - #73
Merged
Merged
Conversation
## Summary
Phase 5 of the autoland step-abstraction stack: cancellation. This one intentionally changes behavior, listed at the end.
**What was wrong.** `commands._install_signal_handler` installed a SIGINT handler that did the whole interruption from inside the signal handler: it set `ctx.aborted` and the abort reason, saved the checkpoint, printed, rendered the status, disposed of the worktree, and called `sys.exit(130)`. A Python signal handler runs between two arbitrary bytecodes of the main thread, so all of that could start in the middle of a checkpoint write, a console print, or a mutation of the run state. Meanwhile ~10 wait loops in `waits.py` and `native_stack.py` polled `ctx.aborted` / `ctx.abort_requested()`, but nothing set it during a run (the handler exited instead of returning), so those checks never fired, and neither did the "aborted" results they returned.
**The new interrupt path.** There is no custom handler. Ctrl+C raises `KeyboardInterrupt` (Python's default) from wherever the run is: a wait's sleep, a `gh` call, a confirm prompt's `input()`. The fresh, resume, and replan flows all execute the plan through one new function, `commands._land`, which catches it around `engine.execute_plan` and does what the handler did, in normal control flow: records `abort_reason = "User interrupted (Ctrl+C)"`, saves the checkpoint, prints "Interrupted! State saved. Resume with --resume." and the status, disposes of the worktree by the same keep/cleanup rules (kept unless `--always-cleanup`), and exits 130. The `finally` blocks that were already in the flows release the lock and the worktree as `SystemExit` unwinds. The checkpoint stays at the step that was interrupted, so `--resume` runs that step again. `checkpointer.save` writes a temporary file and renames it into place, so a second Ctrl+C during that save leaves the checkpoint written as the step started, which resumes the same way.
`_land` still calls `signal.signal(SIGINT, signal.default_int_handler)` once, as landing starts. That restores Python's default and is not a custom handler. A process started with SIGINT ignored (a background job of a non-interactive shell, e.g. an agent or script running `stack-pr autoland &`) inherits `SIG_IGN`, and Python keeps it. The old handler overrode that as a side effect. Without the restore, a takeover's SIGINT would be ignored and the takeover would time out. The new subprocess test fails without it.
Takeover (`_stop_running_autoland`, from `--replan` or a new run of the same branch) is unchanged: the other run gets SIGINT, raises `KeyboardInterrupt`, saves, exits 130, and releases the lock.
**Dead checks removed.** `LandingContext.aborted` and `abort_requested()` are gone, along with every `if ctx.aborted` check in `wait_for_workflow`, `wait_for_approval`, `wait_for_checks`, `wait_for_mergeable`, `enqueue_and_wait` (both loops), and `_await_native_stack_merge`, and the results only they produced (`MergeableResult(error="aborted")`, the stack merge's `"aborted"` failure and its "Stack merge was aborted" reason). `wait_for_approval`, `wait_for_checks`, `wait_for_mergeable`, `enqueue_and_wait`, `_rewait_after_retry`, and `_await_native_stack_merge` only took `ctx` for that check, so they and the `Waits` protocol no longer take it (`wait_for_workflow` keeps it, since it uses `ctx.last_landed_sha` as its default target). The status display now keys "ABORTED: …" off `abort_reason`, the field `aborted` was derived from on load. `abort_reason` is still persisted and shown by `--status`. The state file format is unchanged (`STATE_VERSION` 1, same keys). `aborted` was never written to it, only derived on load.
**No `CancelToken`.** It would have no consumer. Nothing in the process asks for cancellation except SIGINT, and SIGINT is fully handled by `KeyboardInterrupt` unwinding to `_land`. That also covers the upcoming `x <command>` step, which runs a script as a subprocess. Ctrl+C in the terminal goes to the whole foreground process group, so the child gets SIGINT directly. In the parent, `subprocess.run` lets `KeyboardInterrupt` interrupt its wait, gives the child a moment to exit, then kills it and re-raises. A takeover's SIGINT reaches only the autoland process, not the child, but the same `subprocess.run` cleanup kills the child before the exception reaches `_land`. So the `x` step needs no cancellation machinery, only `subprocess.run` (or anything else that kills its child on `BaseException`). A token is worth adding when something has to cancel a run without a signal.
**Landed SHA lookup in `WorkflowStep`: kept.** Phase 3 had the workflow step get the landed SHA through `sc.landed_sha()`, which records it on the run before the wait starts, so an interrupted wait's checkpoint includes it. That's still needed. The checkpoint is now saved at the boundary, but still mid-step, from the same run state. If the step only reported the SHA in its `StepResult`, an interrupted wait would never return one, and the resumed wait would follow whatever the target branch head is by then.
**User-visible behavior changes**
- When Ctrl+C or a takeover stops a landing run, the messages, status, checkpoint, worktree handling, and exit code 130 are the same as before. The difference is that this now runs in normal control flow instead of inside a signal handler.
- A Ctrl+C after the plan has finished (while the final status prints or the worktree is cleaned up) is no longer treated as an interruption of the run. It used to save a checkpoint marked "User interrupted" for a finished run (re-creating one a successful run had just deleted). Now it is a plain `KeyboardInterrupt`, and the lock and worktree are still released by the existing `finally` blocks. Ctrl+C before landing starts (discovery, the replan/overwrite prompts) behaves as before.
- A PR closed while autoland waits for its approval now stops the run with "PR #N was closed while waiting for approval" instead of "PR #N approval wait was aborted". This is the only way that wait fails, in both the one-at-a-time and the stack-merge path.
## Test plan
- [x] New `tests/test_autoland_interrupt.py`, run against injected fake waits and UI:
- [x] Ctrl+C (`KeyboardInterrupt`) inside an approval wait exits 130, the checkpoint has `abort_reason` "User interrupted (Ctrl+C)" at the interrupted step with earlier progress kept, "Interrupted! State saved. Resume with --resume." is printed, the lock is free, and the `--branch` worktree is preserved, or removed with `--always-cleanup`.
- [x] Ctrl+C at a `c` confirm prompt gives the same result, and the confirmation stays unconfirmed.
- [x] `--resume` after an interrupt continues from the interrupted step: the PR landed before it is not landed again, and the run finishes and deletes its checkpoint.
- [x] A real subprocess run blocked in a wait, started with SIGINT ignored like a background job, is stopped by `_stop_running_autoland`: it exits 130, prints the interrupt message, checkpoints "User interrupted (Ctrl+C)", and gives up the lock. The test fails without the `default_int_handler` restore.
- [x] Wait-loop tests that used to stop a poll by setting `ctx.aborted` now raise from the sleep instead, and still check that an unapproved PR, an unknown ancestry, or a failed or unfinished workflow run is still being waited on.
- [x] Golden scenarios pass without regenerating any golden file.
- [ ] Manual: press Ctrl+C during a real autoland wait (approval, checks, or merge queue) and at a `c` confirm prompt. Check the message, the exit code (`echo $?` shows 130), and `--status` ("ABORTED: User interrupted (Ctrl+C)"), then `--resume` and check it continues from the interrupted step.
- [ ] Manual: start an autoland, then run `--replan` for the same branch from another terminal and confirm the takeover stops the first run and continues.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
stack-info: PR: #73, branch: micahjsmith/stack/7
micahjsmith
force-pushed
the
micahjsmith/stack/7
branch
from
October 12, 2026 23:05
6734471 to
abf54fa
Compare
This was referenced Oct 12, 2026
micahjsmith
added this pull request to stack #77
October 12, 2026 23:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked PRs:
Summary
Phase 5 of the autoland step-abstraction stack: cancellation. This one intentionally changes behavior, listed at the end.
What was wrong.
commands._install_signal_handlerinstalled a SIGINT handler that did the whole interruption from inside the signal handler: it setctx.abortedand the abort reason, saved the checkpoint, printed, rendered the status, disposed of the worktree, and calledsys.exit(130). A Python signal handler runs between two arbitrary bytecodes of the main thread, so all of that could start in the middle of a checkpoint write, a console print, or a mutation of the run state. Meanwhile ~10 wait loops inwaits.pyandnative_stack.pypolledctx.aborted/ctx.abort_requested(), but nothing set it during a run (the handler exited instead of returning), so those checks never fired, and neither did the "aborted" results they returned.The new interrupt path. There is no custom handler. Ctrl+C raises
KeyboardInterrupt(Python's default) from wherever the run is: a wait's sleep, aghcall, a confirm prompt'sinput(). The fresh, resume, and replan flows all execute the plan through one new function,commands._land, which catches it aroundengine.execute_planand does what the handler did, in normal control flow: recordsabort_reason = "User interrupted (Ctrl+C)", saves the checkpoint, prints "Interrupted! State saved. Resume with --resume." and the status, disposes of the worktree by the same keep/cleanup rules (kept unless--always-cleanup), and exits 130. Thefinallyblocks that were already in the flows release the lock and the worktree asSystemExitunwinds. The checkpoint stays at the step that was interrupted, so--resumeruns that step again.checkpointer.savewrites a temporary file and renames it into place, so a second Ctrl+C during that save leaves the checkpoint written as the step started, which resumes the same way._landstill callssignal.signal(SIGINT, signal.default_int_handler)once, as landing starts. That restores Python's default and is not a custom handler. A process started with SIGINT ignored (a background job of a non-interactive shell, e.g. an agent or script runningstack-pr autoland &) inheritsSIG_IGN, and Python keeps it. The old handler overrode that as a side effect. Without the restore, a takeover's SIGINT would be ignored and the takeover would time out. The new subprocess test fails without it.Takeover (
_stop_running_autoland, from--replanor a new run of the same branch) is unchanged: the other run gets SIGINT, raisesKeyboardInterrupt, saves, exits 130, and releases the lock.Dead checks removed.
LandingContext.abortedandabort_requested()are gone, along with everyif ctx.abortedcheck inwait_for_workflow,wait_for_approval,wait_for_checks,wait_for_mergeable,enqueue_and_wait(both loops), and_await_native_stack_merge, and the results only they produced (MergeableResult(error="aborted"), the stack merge's"aborted"failure and its "Stack merge was aborted" reason).wait_for_approval,wait_for_checks,wait_for_mergeable,enqueue_and_wait,_rewait_after_retry, and_await_native_stack_mergeonly tookctxfor that check, so they and theWaitsprotocol no longer take it (wait_for_workflowkeeps it, since it usesctx.last_landed_shaas its default target). The status display now keys "ABORTED: …" offabort_reason, the fieldabortedwas derived from on load.abort_reasonis still persisted and shown by--status. The state file format is unchanged (STATE_VERSION1, same keys).abortedwas never written to it, only derived on load.No
CancelToken. It would have no consumer. Nothing in the process asks for cancellation except SIGINT, and SIGINT is fully handled byKeyboardInterruptunwinding to_land. That also covers the upcomingx <command>step, which runs a script as a subprocess. Ctrl+C in the terminal goes to the whole foreground process group, so the child gets SIGINT directly. In the parent,subprocess.runletsKeyboardInterruptinterrupt its wait, gives the child a moment to exit, then kills it and re-raises. A takeover's SIGINT reaches only the autoland process, not the child, but the samesubprocess.runcleanup kills the child before the exception reaches_land. So thexstep needs no cancellation machinery, onlysubprocess.run(or anything else that kills its child onBaseException). A token is worth adding when something has to cancel a run without a signal.Landed SHA lookup in
WorkflowStep: kept. Phase 3 had the workflow step get the landed SHA throughsc.landed_sha(), which records it on the run before the wait starts, so an interrupted wait's checkpoint includes it. That's still needed. The checkpoint is now saved at the boundary, but still mid-step, from the same run state. If the step only reported the SHA in itsStepResult, an interrupted wait would never return one, and the resumed wait would follow whatever the target branch head is by then.User-visible behavior changes
KeyboardInterrupt, and the lock and worktree are still released by the existingfinallyblocks. Ctrl+C before landing starts (discovery, the replan/overwrite prompts) behaves as before.Test plan
tests/test_autoland_interrupt.py, run against injected fake waits and UI:KeyboardInterrupt) inside an approval wait exits 130, the checkpoint hasabort_reason"User interrupted (Ctrl+C)" at the interrupted step with earlier progress kept, "Interrupted! State saved. Resume with --resume." is printed, the lock is free, and the--branchworktree is preserved, or removed with--always-cleanup.cconfirm prompt gives the same result, and the confirmation stays unconfirmed.--resumeafter an interrupt continues from the interrupted step: the PR landed before it is not landed again, and the run finishes and deletes its checkpoint._stop_running_autoland: it exits 130, prints the interrupt message, checkpoints "User interrupted (Ctrl+C)", and gives up the lock. The test fails without thedefault_int_handlerrestore.ctx.abortednow raise from the sleep instead, and still check that an unapproved PR, an unknown ancestry, or a failed or unfinished workflow run is still being waited on.cconfirm prompt. Check the message, the exit code (echo $?shows 130), and--status("ABORTED: User interrupted (Ctrl+C)"), then--resumeand check it continues from the interrupted step.--replanfor the same branch from another terminal and confirm the takeover stops the first run and continues.🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 5.5 (1M context) noreply@anthropic.com