fix(adopt): handle CTRL+C cleanly during av adopt (#692)#749
Conversation
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
🔃 FlexReview StatusCommon Owner:
Review SLO: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements context propagation and signal handling across the av command suite, enabling graceful termination on SIGINT and SIGTERM. Key changes include the introduction of RunBubbleTeaWithContext, context-aware Git operations, and a new E2E test verifying interrupt behavior. Review feedback identifies a potential data race in GetRemoteStackedPRModel and suggests refactoring the context storage in view models from provider functions to direct struct fields for better idiomatic Go compliance.
| nextPRNumber := int64(0) | ||
| for { | ||
| if err := m.ctx().Err(); err != nil { | ||
| m.failed = true |
There was a problem hiding this comment.
This line introduces a data race. The field m.failed is being modified inside a goroutine (the anonymous function returned as a tea.Cmd), while it may be read concurrently by the main thread in the View method. In the Bubble Tea architecture, models should only be modified within the Update method to ensure thread safety. Consider returning the error as a message and handling it in Update to set the failure state safely.
| } | ||
|
|
||
| type AdoptBranchesModel struct { | ||
| ctx func() context.Context |
There was a problem hiding this comment.
It is more idiomatic in Go to store the context.Context directly in the struct rather than using a provider function func() context.Context, especially since the context is passed during construction and remains stable. This simplifies the code by allowing direct access (e.g., m.ctx.Err()) and avoids unnecessary closure overhead. Please apply this change here and consistently across other view models in this PR.
| ctx func() context.Context | |
| ctx context.Context |
| onDone func() tea.Cmd, | ||
| ) tea.Model { | ||
| return &AdoptBranchesModel{ | ||
| ctx: func() context.Context { return ctx }, |
There was a problem hiding this comment.
Following the suggestion to store the context directly, the initialization should be updated to assign the context value.
| ctx: func() context.Context { return ctx }, | |
| ctx: ctx, |
The Init() goroutine was mutating m.failed, m.prs, and m.done directly, while View() reads those fields from the Bubble Tea main thread - a real data race flagged by gemini-code-assist. Replace the in-goroutine mutations with two new message types (remoteStackedPRFailedMsg, remoteStackedPRDoneMsg) emitted by the goroutine and handled in Update(). All field writes now happen on the main thread, matching the Bubble Tea model contract. Behavior is preserved: failure surfaces as colors.FailureStyle, the onDone command still fires after a successful collection, and ctrl+c cancellation still aborts via m.ctx(). The e2e_tests/adopt_test.go addition was dropped during rebase because master migrated to the testscript framework (aviator-co#695); the SIGINT e2e coverage needs to be reauthored as a testscript .txtar before re-adding. Signed-off-by: Matt Van Horn <[email protected]>
mvanhorn
force-pushed
the
fix/692-av-adopt-ctrl-c-cancellation
branch
from
ffd106a to
eaee61d
Compare
|
Rebased onto master (master picked up the testscript e2e migration in #695) and pushed eaee61d to address the HIGH-priority data race. State mutations on The Not tackling the two medium suggestions (storing |
Re-introduces TestAdopt_ExitsOnSIGINT (originally dropped during the testscript rebase) as a plain Go test in e2e_tests/sigint_test.go. The test spawns av adopt --dry-run with PATH-injected fake git that sleeps on `merge-base`, waits for the marker file confirming the sleep started, sends os.Interrupt, and asserts av returns within 5s. This locks in the SIGINT path through cmd/av/main.go's signal.NotifyContext + uiutils.RunBubbleTeaWithContext + the adopt_branches / get_remote_stacked_pr ctx checks. The test bypasses the testscript harness deliberately. testscript v1.10.0 (the version currently in go.mod) sends os.Interrupt to all backgrounded processes only at end-of-test via waitBackground, and exposes no public API for sending a signal to a specific named background process from a script. A header comment documents the escape hatch and the conditions under which this can move into testdata/script as a .txtar. Follow-up to aviator-co#749's gemini-flagged feedback (SIGINT e2e coverage). Signed-off-by: Matt Van Horn <[email protected]>
|
Follow-up on the two medium gemini items. SIGINT e2e coverage: pushed ba7620b adding context.Context direct storage on |
1 participant
Summary
av adoptnow exits cleanly on SIGINT (CTRL+C). Threadscontext.Contextthrough the adopt path, switches git subprocess invocations toexec.CommandContext, and adds a sharedsignal.NotifyContextinstall at the Cobra entry.Why this matters
From #692:
Other av commands (
av sync) honor SIGINT correctly, so the fix mirrors that pattern.Changes
cmd/av/main.go: installsignal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM)once at the Cobra rootcmd/av/adopt.go: propagate the context into the adopt actioninternal/actions/adopt_branches.go,find_adoptable_local_branches.go,get_remote_stacked_pr.go,git_fetch.go: honorctx.Done()in branch-walk loopsinternal/treedetector/detector.go,internal/utils/uiutils/tea.go: thread context throughe2e_tests/adopt_test.go: new test spawningav adopt, sending SIGINT, asserting bounded exit timePartial-state cleanup is correct: no half-adopted branches left in inconsistent state on cancellation.
Fixes #692