fix(mail): remove os.Exit from watch signal handler#311
fix(mail): remove os.Exit from watch signal handler#311OwenYWT wants to merge 3 commits intolarksuite:mainfrom
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReworks mail-watch shutdown to use a cancellable context and coordinated signal flow instead of calling Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main Goroutine
participant CLI as CLI (Start)
participant SigH as Signal Handler
participant Ch as Channels (startErrCh / shutdownBySignal)
Main->>Main: create cancellable watchCtx
Main->>CLI: go cli.Start(watchCtx) -> send err to startErrCh
Main->>SigH: start listening on sigCh
Main->>Ch: select on startErrCh or shutdownBySignal
SigH->>SigH: receive interrupt
SigH->>SigH: call handleMailWatchSignal() (unsubscribe, stop signal, cancel)
SigH->>Ch: send shutdownBySignal
CLI-->>Main: observes cancelled watchCtx and stops
alt Shutdown by signal
Ch-->>Main: shutdownBySignal received
Main->>Main: return nil (graceful exit)
else Startup error
CLI-->>Main: startErrCh receives error
Main->>Main: perform best-effort unsubscribe and return network error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
domain/mail
Greptile SummaryThis PR replaces the abrupt
Confidence Score: 4/5Safe to merge once close(shutdownComplete) is deferred to prevent a deadlock when handleMailWatchSignal panics. One P1 finding remains: close(shutdownComplete) is not deferred, creating a deadlock path if the signal handler goroutine panics after closing shutdownRequested. All other prior concerns from previous review threads have been addressed by the new channel-based coordination design. shortcuts/mail/mail_watch.go — signal goroutine around lines 491–504
|
| Filename | Overview |
|---|---|
| shortcuts/mail/mail_watch.go | Replaces os.Exit(0) with context-cancellation–based graceful shutdown using three new helpers and a signal goroutine; one P1 deadlock risk if handleMailWatchSignal panics before close(shutdownComplete). |
| shortcuts/mail/mail_watch_test.go | Adds focused unit tests for handleMailWatchSignal, newMailWatchUnsubscribeOnce, and waitForMailWatchStart covering success, error reporting, and signal-vs-error race; covers the expected paths well. |
Sequence Diagram
sequenceDiagram
participant Main
participant WSGoroutine as WebSocket goroutine
participant SigGoroutine as Signal goroutine
Main->>WSGoroutine: go cli.Start(watchCtx)
Main->>SigGoroutine: go (wait for signal)
Main->>Main: waitForMailWatchStart(startErrCh, shutdownRequested, shutdownComplete)
Note over SigGoroutine: Ctrl+C received
SigGoroutine->>SigGoroutine: close(shutdownRequested)
SigGoroutine->>SigGoroutine: handleMailWatchSignal → unsubscribe, stop, cancel()
WSGoroutine-->>WSGoroutine: context cancelled → cli.Start returns
WSGoroutine->>Main: startErrCh ← context.Canceled
SigGoroutine->>SigGoroutine: close(shutdownComplete) ⚠️ not deferred
alt startErrCh arrives before shutdownComplete
Main->>Main: inner select: shutdownRequested closed → wait for shutdownComplete
SigGoroutine-->>Main: shutdownComplete closed
Main->>Main: return nil ✓
else shutdownComplete arrives first
Main->>Main: return nil ✓
end
Note over SigGoroutine: ⚠️ panic in handleMailWatchSignal skips close(shutdownComplete) → deadlock
Reviews (3): Last reviewed commit: "fix(mail): avoid watch shutdown race" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/mail/mail_watch.go (1)
53-67: Consider using or removing the unusedsigparameter.The
sig os.Signalparameter is accepted but never used in the function body. If you intend to log which signal triggered shutdown (SIGINT vs SIGTERM), consider adding it to the log message. Otherwise, if the parameter exists solely for test verification, that's acceptable but worth a brief comment.♻️ Optional: Log the signal type
func handleMailWatchSignal(errOut io.Writer, sig os.Signal, eventCount int, unsubscribe func() error, stopSignals func(), cancel context.CancelFunc) { - fmt.Fprintf(errOut, "\nShutting down... (received %d events)\n", eventCount) + fmt.Fprintf(errOut, "\nShutting down (signal: %v)... (received %d events)\n", sig, eventCount) fmt.Fprintln(errOut, "Unsubscribing mailbox events...")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/mail/mail_watch.go` around lines 53 - 67, The function handleMailWatchSignal currently accepts sig os.Signal but never uses it; either remove the sig parameter from the signature and update any callers/tests, or include it in the shutdown log (e.g., add sig to the fmt.Fprintf/Fprintln output to show which signal triggered shutdown). If the parameter must remain only for tests, keep it but add a short comment above handleMailWatchSignal explaining it's intentionally unused for test verification. Ensure references to unsubscribe, stopSignals, cancel behavior remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 464-468: The signal handler reads eventCount concurrently with
handleEvent mutating it, causing a data race; change eventCount to an
atomic.Int64 (declare var eventCount atomic.Int64), update the increment in
handleEvent to use eventCount.Add(1), and read it in the signal-handling
goroutine via eventCount.Load() (convert to int when calling
handleMailWatchSignal) so the read/write are synchronized; adjust imports to
include sync/atomic package as needed.
---
Nitpick comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 53-67: The function handleMailWatchSignal currently accepts sig
os.Signal but never uses it; either remove the sig parameter from the signature
and update any callers/tests, or include it in the shutdown log (e.g., add sig
to the fmt.Fprintf/Fprintln output to show which signal triggered shutdown). If
the parameter must remain only for tests, keep it but add a short comment above
handleMailWatchSignal explaining it's intentionally unused for test
verification. Ensure references to unsubscribe, stopSignals, cancel behavior
remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfb2c0ff-fbee-46ed-8451-4722e100df80
📒 Files selected for processing (2)
shortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.go
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
|
Feedback: correct core architecture, a few improvements possible This is the strongest of the three PRs addressing this issue. The key architectural decision — running The extracted A few improvements to consider:
|
|
Closing in favor of #350 which has been merged and addresses the same os.Exit issue. #350 combines the most suitable approaches from all three PRs:
Thank you for the contribution — your work directly informed the final implementation. |
3 participants
Summary
Replace the
mail +watchsignal-pathos.Exit(0)with graceful shutdown so Ctrl+C cancels the WebSocket client and keeps the command testable.Changes
os.Exit(0)in the signal handler with a shutdown helper that unsubscribes, stops signal delivery, and cancels the watch contextnilon signal-driven shutdownRelated Issues
Test Plan
GOCACHE=/tmp/go-build-cache GOMODCACHE=/tmp/go-mod-cache /opt/homebrew/opt/go/bin/go test ./shortcuts/mail -run 'TestHandleMailWatchSignal_UnsubscribesAndCancels|TestHandleMailWatchSignal_ReportsUnsubscribeFailure|TestMailWatch'GOCACHE=/tmp/go-build-cache GOMODCACHE=/tmp/go-mod-cache /opt/homebrew/opt/go/bin/go test ./shortcuts/mailSummary by CodeRabbit
Bug Fixes
Refactor
Tests