fix(mail): replace os.Exit with graceful shutdown in mail watch#350
fix(mail): replace os.Exit with graceful shutdown in mail watch#350chanthuang merged 4 commits intomainfrom
Conversation
The signal handler in mail +watch called os.Exit(0), which bypassed all deferred cleanup functions, made the code path untestable, and did not follow Go's idiomatic context cancellation pattern. Key changes: - Remove os.Exit(0) and use context.WithCancel to propagate shutdown - Run cli.Start in a separate goroutine so the main goroutine can return immediately on signal receipt (the Lark WebSocket SDK does not return promptly after context cancellation) - Extract handleMailWatchSignal as a testable standalone function - Use sync.Once + defer for idempotent cleanup on all exit paths - Fix eventCount data race with atomic.Int64 - Add signal.Reset to support forced termination via a second Ctrl+C Closes #268
|
|
github-actions
bot
added
domain/mail
|
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 (2)
📝 WalkthroughWalkthroughReplace immediate process exit on signal with context-driven graceful shutdown: add Changes
Sequence Diagram(s)sequenceDiagram
participant OS as "OS (SIGINT/SIGTERM)"
participant Sig as "Signal Handler"
participant Main as "Main Goroutine"
participant CLI as "cli.Start (goroutine)"
participant Mail as "Mailbox / unsubscribe"
OS->>Sig: send signal
Sig->>Sig: handleMailWatchSignal(signal)
Sig->>Mail: unsubscribeWithLog() -- once
Sig->>Main: notify shutdownBySignal (chan)
Sig->>Main: cancel(watchCtx)
Main->>CLI: wait (select) for CLI error or shutdown
CLI-->>Main: returns error (if any)
Main-->>OS: exit after graceful shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c1cb75e0bf87ab28a26658ca10fb8bbce5a3c7c2🧩 Skill updatenpx skills add larksuite/cli#fix/mail-watch-graceful-shutdown -y -g |
Greptile SummaryThis PR replaces Confidence Score: 5/5Safe to merge — graceful shutdown is correctly implemented and previous panic-path concerns are fully resolved. All previously raised P0/P1 concerns are addressed: the panic-recovery path now calls triggerShutdown() (closes shutdownBySignal directly) rather than only cancelWatch(), so the main select unblocks immediately even when the WebSocket SDK hangs. The eventCount data race is fixed with atomic.Int64, sync.Once guards prevent duplicate unsubscribe logs, and the startErrCh buffered channel ensures no goroutine leak. No new P0/P1 issues found in this review pass. No files require special attention.
|
| Filename | Overview |
|---|---|
| shortcuts/mail/mail_watch.go | Replaces os.Exit with context-based graceful shutdown using sync.Once, triggerShutdown, goroutine-based cli.Start, and atomic.Int64 for eventCount; panic recovery now correctly closes shutdownBySignal. |
| shortcuts/mail/mail_watch_test.go | Adds unit tests for handleMailWatchSignal covering callback ordering, unsubscribe-failure reporting, and panic-recovery-unblocks-shutdown; existing tests unchanged. |
Sequence Diagram
sequenceDiagram
participant Main as Execute goroutine
participant SigG as Signal goroutine
participant StartG as cli.Start goroutine
Main->>StartG: go cli.Start(watchCtx)
Main->>SigG: go signal handler goroutine
Main->>Main: select on shutdownBySignal or startErrCh
SigG->>SigG: receive SIGINT via sigCh
SigG->>SigG: handleMailWatchSignal()
Note over SigG: stopSignals() + signal.Reset()
Note over SigG: unsubscribeWithLog() via sync.Once
Note over SigG: cancelWatch() cancels watchCtx
SigG->>Main: triggerShutdown() closes shutdownBySignal
Main-->>Main: select fires on shutdownBySignal, return nil
Note over StartG: cli.Start still running (SDK does not exit promptly)
StartG-->>StartG: eventually returns, writes to buffered startErrCh
alt Panic inside handleMailWatchSignal
SigG->>SigG: recover() calls triggerShutdown()
SigG->>Main: shutdownBySignal closed via sync.Once
Main-->>Main: select fires on shutdownBySignal, return nil
end
Reviews (4): Last reviewed commit: "fix(mail): use triggerShutdown to unbloc..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Tip: Greploop — 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. |
If handleMailWatchSignal panics, the recover block now calls cancelWatch() to unblock the main select. Without this, a panic would leave shutdownBySignal unclosed and watchCtx uncancelled, causing the process to hang.
…er panic The previous panic recovery only called cancelWatch(), but since the WebSocket SDK does not return promptly after context cancellation, the main select could still hang waiting on startErrCh. Introduce triggerShutdown() that closes shutdownBySignal (via sync.Once) and cancels the watch context, used by both the normal signal path and the panic recovery path. This ensures the main select unblocks immediately regardless of how the signal goroutine exits. Add regression test that forces a panic and asserts shutdownBySignal is closed promptly.
infeng
left a comment
infeng
left a comment
There was a problem hiding this comment.
LGTM. No issues found.
Review baseline: base main, head fix/mail-watch-graceful-shutdown.
3 participants
Summary
os.Exit(0)from the signal handler inmail +watchand replace with context-based graceful shutdowncli.Startin a separate goroutine to avoid blocking on signal receipt (the Lark WebSocket SDK does not return promptly after context cancellation)handleMailWatchSignalas a testable standalone functionsync.Once+deferfor idempotent cleanup on all exit pathseventCountdata race withatomic.Int64signal.Resetto support forced termination via a second Ctrl+CContext
Three existing PRs (#269, #273, #311) independently addressed the
os.Exitissue but each had gaps. Through local verification we found thatcli.Start(watchCtx)does not return after context cancellation, so it must run in a goroutine — #269 and #273 both call it synchronously and hang on Ctrl+C. This PR combines the best approaches: goroutine-basedcli.Startfrom #311,sync.Once+deferunified cleanup andsignal.Resetfrom #269, and testable extracted function with tests from #311.Closes #268
Test plan
go test -race ./shortcuts/mail/...— all pass, no race warningsmake unit-test— full suite pass, no regressionsSummary by CodeRabbit
Bug Fixes
Tests