fix(review): exit immediately on the second signal - #1185
Open
HOWILLMAKEIT wants to merge 1 commit into
Open
Conversation
HOWILLMAKEIT
force-pushed
the
fix/second-signal-exit
branch
2 times, most recently
from
September 5, 2026 17:45
7c9ee90 to
eb0bd68
Compare
Qiyuanqiii
suggested changes
Sep 5, 2026
Qiyuanqiii
left a comment
Qiyuanqiii
left a comment
Contributor
There was a problem hiding this comment.
Blocking issue in the forced-exit path; details are in the inline comment.
review_cmd.go registered SIGINT/SIGTERM via signal.NotifyContext. Its watcher goroutine receives the first signal, cancels the context and exits -- without unregistering the channel: stop() only runs in the caller's defer, after the whole graceful shutdown has finished. For that entire window (report flush, retry-report freeze, session persist, MCP client close) every further signal lands in the stdlib's unread size-one buffer and is dropped while the default kill behavior stays suppressed, so a second Ctrl+C does nothing however long the cleanup takes. Replace NotifyContext with an owned signal channel: the first signal cancels the context exactly as before, and the watcher now stays alive across the whole shutdown window; a second signal force-exits the process at once with status 1, skipping the remaining cleanup -- the user has explicitly abandoned the graceful path. A signal queued while stop() was closing done must not turn an already-successful run into exit 1, so the watcher drops latecomers once shutdown completion won the race. The buffer holds two entries: os/signal sends non-blockingly, so a size-one buffer could drop the second of two back-to-back signals before the watcher consumes the first. stop() is idempotent, unregisters the channel, releases the watcher and cancels the context, mirroring NotifyContext's stop semantics. Exit status stays plain 1 to match the current effective behavior; switching to the 128+sig convention (130 SIGINT / 143 SIGTERM) deserves its own discussion and is raised in the PR description. scan_cmd.go is untouched: it registers no signal handling today, and keeping it that way until alibaba#996 lands preserves symmetry between the two commands. Tests: in-process unit tests cover first-signal cancellation (SIGINT and SIGTERM), the second-signal forced exit through a stubbed hook, and watcher release on stop-before-signal and parent cancellation. Two subprocess integration tests re-execute the test binary so the real os.Exit path is exercised: a second SIGINT during a simulated 30s graceful shutdown ends the process well under 100ms with status 1, and a single SIGINT still completes the graceful path with the partial report persisted on disk and status 0.
HOWILLMAKEIT
force-pushed
the
fix/second-signal-exit
branch
from
September 7, 2026 06:35
eb0bd68 to
85efaa3
Compare
Contributor
Author
|
thanks to @Qiyuanqiii , a problem is fixed in 85efaa3. The diagnostic write now runs in a goroutine ( I also added a regression test ( |
Qiyuanqiii
approved these changes
Sep 7, 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 reviewregistered SIGINT/SIGTERM viasignal.NotifyContext. Its watcher goroutine exits after the first signal without unregistering, so during the entire graceful-shutdown window every further signal lands in an unread channel and is dropped — a second Ctrl+C does nothing, however long the cleanup takes.This change replaces
NotifyContextwith an owned signal channel incmd/opencodereview/interrupt.go:stop()is idempotent and mirrorsNotifyContext's stop semantics.scan_cmd.gois untouched; it registers no signal handling today.Type of Change
How Has This Been Tested?
cmd/opencodereview/interrupt_test.gocovers first-signal graceful cancellation (SIGINT and SIGTERM), second-signal forced exit, stop/parent-cancel releasing the watcher, and two subprocess-level integration tests exercising the realos.Exitpath end to end.The second-signal integration test asserts the child exits in under 100 ms with status 1; the single-signal test asserts the partial report lands on disk and the child exits 0.
make testpasses locallyManual testing: verified the second-signal exit path and the single-signal graceful path via the subprocess integration tests
Checklist
go fmt,go vet)Related Issues
Related to #1141 (item 3).