feat: handle SIGTERM in +watch and +subscribe for clean shutdown#550
feat: handle SIGTERM in +watch and +subscribe for clean shutdown#550
Conversation
Add shared shutdown_signal() helper that merges SIGINT and SIGTERM into a single future. Replace tokio::signal::ctrl_c() in both watch and subscribe pull loops so they exit cleanly under Kubernetes, Docker, and systemd. On non-Unix platforms, only SIGINT (Ctrl+C) is handled.
🦋 Changeset detectedLatest commit: 74e7bc5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
googleworkspace-bot
added
area: core
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of long-running Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
googleworkspace-bot
added
the
cla: yes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
=======================================
Coverage 69.04% 69.04%
=======================================
Files 42 42
Lines 19261 19284 +23
=======================================
+ Hits 13299 13315 +16
- Misses 5962 5969 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 introduces handling for SIGTERM to allow for graceful shutdown in containerized environments. The approach of creating a shared shutdown_signal helper is good. However, the current implementation of shutdown_signal has a critical race condition that could lead to unclean shutdowns, which defeats the purpose of the change. I've provided a detailed comment and a suggested fix to make the signal handling robust.
github-actions
bot
added
the
gemini: reviewed
Use OnceLock + tokio::sync::Notify so the signal handler stays active for the process lifetime. Eliminates the race window between loop iterations where a SIGTERM would bypass the handler.
github-actions
bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a shutdown_signal helper to handle both SIGINT and SIGTERM for graceful shutdown, which is a great improvement for containerized environments. The implementation is mostly solid, using a OnceLock and Notify pattern effectively. However, I've found a critical issue in the error handling within the new shutdown_signal function that could lead to a deadlock if registering the SIGTERM handler fails. My review includes a suggestion to fix this.
github-actions
bot
added
the
gemini: reviewed
Replace expect() with match: if signal(SIGTERM) fails, log a warning and fall back to SIGINT-only. Prevents silent task death that would hang all shutdown_signal() callers indefinitely.
github-actions
bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a helpful shutdown_signal helper to handle both SIGINT and SIGTERM for graceful shutdown, which is a great improvement for containerized environments. However, the implementation of shutdown_signal has a critical flaw that could cause the application to terminate unexpectedly on startup if it fails to register a signal handler. I've provided a detailed comment and a code suggestion to address this issue.
github-actions
bot
added
the
gemini: reviewed
Use Ok(_) pattern matching in select! branches and expect() for standalone ctrl_c().await calls. Previously .ok() silently swallowed errors, causing notify_waiters() to fire immediately.
github-actions
bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a clean way to handle both SIGINT and SIGTERM for graceful shutdown, which is a great improvement for containerized environments. The implementation uses a shutdown_signal helper that correctly encapsulates the logic. I found one high-severity issue in the new helper where a failure to register the SIGINT handler would be silently ignored, making the application unresponsive to Ctrl+C. I've provided a suggestion to fix this by explicitly handling the result and panicking on error, consistent with other parts of the code.
github-actions
bot
added
the
gemini: reviewed
Bind the full Result from ctrl_c() and expect() on it instead of pattern matching Ok(_), which silently dropped the branch on Err.
github-actions
bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a helpful shutdown_signal utility to handle both SIGINT and SIGTERM for graceful shutdowns, which is a great improvement for containerized environments. The implementation, however, contains a critical issue where a failure to register the SIGINT handler can cause the background signal-watching task to panic. This leads to a deadlock in any part of the application awaiting a shutdown signal, making the process hang. I've provided a detailed comment and a suggested fix in src/helpers/mod.rs to address this by handling errors gracefully instead of panicking.
github-actions
bot
added
the
gemini: reviewed
3 participants
Summary
Long-running pull loops (
gws gmail +watch,gws events +subscribe) currently only handle Ctrl+C (SIGINT). Sending SIGTERM — the default signal used by Kubernetes, Cloud Run, Docker, and systemd — leaves the process running until the next poll timeout.Changes
src/helpers/mod.rsshutdown_signal()async helper — merges SIGINT + SIGTERM into one futuresrc/helpers/gmail/watch.rsctrl_c()→src/helpers/events/subscribe.rsDesign
shutdown_signal()encapsulates the cfg-gated signal registration so callers just swap one callctrl_c()stays inside the helper — no behavior change on any platformctrl_c())Checklist
cargo fmt --allcargo clippy -- -D warningsclean