Skip to content

fix: replace os.Exit with graceful shutdown in mail watch #269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
luojiyin1987 wants to merge 5 commits into
base: main
Choose a base branch
from
+40 −14
Open

fix: replace os.Exit with graceful shutdown in mail watch #269

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e45bca
fix: replace os.Exit with graceful shutdown in mail watch
luojiyin1987 Apr 5, 2026
9c93401
fix: order signal shutdown markers in mail watch
luojiyin1987 Apr 5, 2026
c5e7b5a
fix: improve signal handler to avoid blocking shutdown
luojiyin1987 Apr 5, 2026
e4656a4
refactor: use defer + sync.Once for cleanup on all exit paths
luojiyin1987 Apr 5, 2026
8ea53f9
fix: harden mail watch signal shutdown path
luojiyin1987 Apr 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 40 additions & 14 deletions shortcuts/mail/mail_watch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"sort"
"strings"
"sync"
"sync/atomic"
"syscall"

larkcore "github.com/larksuite/oapi-sdk-go/v3/core"
Expand Down Expand Up @@ -259,19 +260,30 @@ var MailWatch = common.Shortcut{
})
return unsubErr
}
var unsubscribeLogOnce sync.Once
unsubscribeWithLog := func() {
unsubscribeLogOnce.Do(func() {
info("Unsubscribing mailbox events...")
if err := unsubscribe(); err != nil {
fmt.Fprintf(errOut, "Warning: unsubscribe failed: %v\n", err)
} else {
info("Mailbox unsubscribed.")
}
})
}
defer unsubscribeWithLog()
Comment on lines +263 to +274
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Propagate unsubscribe failures on the graceful-exit paths.

Lines 262-273 downgrade unsubscribe failures to a warning, and Lines 463-470 still return nil for signal/cancel shutdown. That means this command exits 0 even when the subscription it created earlier was not cleaned up successfully.

Also applies to: 463-470

🤖 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 262 - 273, The unsubscribe failure
is currently only logged as a warning by unsubscribeWithLog and the
shutdown/signal handling code still returns nil, so process exits 0 even if
unsubscribe failed; change unsubscribeWithLog to capture the unsubscribe error
into a shared variable (e.g., declare var unsubscribeErr error outside, call
unsubscribeErr = err inside the Do block when unsubscribe() fails) and have the
deferred call set/return that error via a named return or by wrapping the
shutdown path: on graceful-exit paths (the signal/cancel shutdown code that
currently returns nil), check unsubscribeErr (or call unsubscribe synchronously
and capture its error) and return that error instead of nil so unsubscribe
failures propagate; reference unsubscribeWithLog, unsubscribeLogOnce and the
unsubscribe() call when making these changes.


// Resolve "me" to the actual email address so we can filter events.
mailboxFilter := mailbox
if mailbox == "me" {
resolved, profileErr := fetchMailboxPrimaryEmail(runtime, "me")
if profileErr != nil {
unsubscribe() //nolint:errcheck // best-effort cleanup; primary error is profileErr
return enhanceProfileError(profileErr)
}
mailboxFilter = resolved
}

eventCount := 0
var eventCount int64

handleEvent := func(data map[string]interface{}) {
// Extract event body
Expand Down Expand Up @@ -337,7 +349,7 @@ var MailWatch = common.Shortcut{
}
}

eventCount++
atomic.AddInt64(&eventCount, 1)

// Prompt injection detection: warn when email body contains known injection patterns.
// Body fields may be base64url-encoded; decode before scanning.
Expand Down Expand Up @@ -426,27 +438,41 @@ var MailWatch = common.Shortcut{

sigCh := make(chan os.Signal, 1)
signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)
defer signal.Stop(sigCh)

watchCtx, cancelWatch := context.WithCancel(ctx)
defer cancelWatch()

shutdownBySignal := make(chan struct{})
go func() {
defer func() {
if r := recover(); r != nil {
fmt.Fprintf(errOut, "panic in signal handler: %v\n", r)
}
}()
<-sigCh
info(fmt.Sprintf("\nShutting down... (received %d events)", eventCount))
info("Unsubscribing mailbox events...")
if unsubErr := unsubscribe(); unsubErr != nil {
fmt.Fprintf(errOut, "Warning: unsubscribe failed: %v\n", unsubErr)
} else {
info("Mailbox unsubscribed.")
select {
case <-sigCh:
// Restore default signal behavior so a second Ctrl+C can force terminate.
signal.Stop(sigCh)
signal.Reset(os.Interrupt, syscall.SIGTERM)
info(fmt.Sprintf("\nShutting down... (received %d events)", atomic.LoadInt64(&eventCount)))
close(shutdownBySignal)
cancelWatch()
case <-watchCtx.Done():
return
}
signal.Stop(sigCh)
os.Exit(0)
}()

info("Connected. Waiting for mail events... (Ctrl+C to stop)")
if err := cli.Start(ctx); err != nil {
unsubscribe() //nolint:errcheck // best-effort cleanup
if err := cli.Start(watchCtx); err != nil {
select {
case <-shutdownBySignal:
return nil
default:
}
if errors.Is(err, context.Canceled) {
return nil
}
return output.ErrNetwork("WebSocket connection failed: %v", err)
}
return nil
Expand Down
Loading