fix(review): handle SIGTERM for graceful shutdown - #1111
Open
HOWILLMAKEIT wants to merge 1 commit into
Open
Conversation
review_cmd.go registered signal.NotifyContext with only os.Interrupt. SIGTERM — sent by docker stop, GitLab CI job cancellation/timeout, and the project's own VS Code extension on review cancel (PR alibaba#489) — killed the process with no cleanup: the --output report was never created (lazyFileWriter opens on first write at emit time), session_end and run_manifest were never persisted, and --resume was rejected even when completed per-file checkpoints existed on disk, forcing a full paid re-review. Add syscall.SIGTERM to the NotifyContext call so SIGTERM follows the same graceful path as Ctrl+C: context cancellation runs the existing defer chain (closeOut, MCP client close, manifest freeze, session_end persist). Measured graceful-shutdown latency on the SIGINT path is ~25ms — two orders of magnitude inside both docker's 10s grace period and the VS Code extension's 3s SIGKILL escalation window. Long-running container/CI CLI peers (docker compose, hugo, terraform, controller-runtime) all register SIGINT+SIGTERM; SIGINT-only tools (gh, kubectl port-forward) are short-lived commands where the distinction rarely matters.
Contributor
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). |
wu21-web
approved these changes
Aug 30, 2026
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
2 participants
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.
Description
ocr reviewregisterssignal.NotifyContextwith onlyos.Interrupt, so SIGTERM kills the process with zero cleanup. Real-world SIGTERM producers:Impact when SIGTERM hits mid-review:
--outputreport is never created (the lazy writer opens the file only at emit time — not truncated, simply absent)session_endandrun_manifestare never persisted--resumeis rejected even when completed per-file checkpoints are on disk — the user pays for a full re-reviewWith this change, SIGTERM follows the same graceful path as Ctrl+C: context cancellation runs the existing defer chain (closeOut, MCP client close, manifest freeze, session_end persist). Measured graceful-shutdown latency on the SIGINT path is ~25ms — two orders of magnitude inside docker's 10s grace and the extension's 3s escalation window.
Ecosystem precedent: long-running container/CI CLIs (docker compose, hugo, terraform, controller-runtime) all register SIGINT+SIGTERM; SIGINT-only tools (gh CLI, kubectl port-forward) are short-lived commands where the distinction rarely matters.
Scope: only
review_cmd.go. The scan path has a separate open PR (#996).Type of Change
How Has This Been Tested?
make checkpasses (gofmt, go vet, license, english-check)go test ./cmd/opencodereview/ -run TestReviewpassessyscall.SIGTERMis a cross-platform constant--outputwritten, session_end persisted,--resumeaccepted (identical to SIGINT behavior); unpatched control → exit 143, output absent, resume rejectedChecklist
go fmt,go vet)Related Issues
Follow-up to #902 (which introduced the SIGINT-only NotifyContext and scoped out non-graceful termination) and #1054 (which fixed the launcher's exit code on signal death but not the graceful-shutdown half).