Skip to content

refactor(autoland): handle Ctrl+C once at the run boundary - #73

Merged
micahjsmith merged 1 commit into
micahjsmith/stack/6from
micahjsmith/stack/7
Oct 12, 2026
Merged

micahjsmith merged 1 commit into
micahjsmith/stack/6from
micahjsmith/stack/7

Conversation

@micahjsmith

@micahjsmith micahjsmith commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

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_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

  • New tests/test_autoland_interrupt.py, run against injected fake waits and UI:
    • 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.
    • Ctrl+C at a c confirm prompt gives the same result, and the confirmation stays unconfirmed.
    • --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.
    • 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.
  • 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.
  • 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

Co-Authored-By: Claude Opus 5.5 (1M context) noreply@anthropic.com

## 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
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