Fix REPL single-message hang and cap CI test duration - #1643
Conversation
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 addresses critical stability and reliability concerns across the application and its continuous integration pipeline. It fixes a specific bug in the REPL's single-message handling that could lead to indefinite hangs, ensuring proper resource release. Additionally, it updates an end-to-end test for Telegram hot-activation to reflect current data structures and introduces robust timeout mechanisms in the CI workflow to prevent long-running or stuck tests from blocking the pipeline indefinitely. Highlights
Ignored Files
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
|
There was a problem hiding this comment.
Code Review
This pull request refines the ReplChannel's behavior by ensuring that a sender handle is only kept alive for interactive modes, preventing potential resource leaks in single-message scenarios. It also enhances the robustness of the single_message_channel_sends_message_and_quits test by adding timeouts to stream assertions. Additionally, the Telegram hot activation E2E test is updated to standardize the setup payload structure to include an empty fields dictionary. There is no feedback to provide as no review comments were given.
There was a problem hiding this comment.
Pull request overview
This PR addresses a regression where REPL single-message mode could keep its channel stream open (hanging a targeted test), updates Telegram hot-activation E2E expectations to the current setup payload shape, and adds CI timeouts to prevent stuck Rust test runs from consuming GitHub Actions’ full default runtime.
Changes:
- Fix REPL single-message mode stream closure behavior and add per-await timeouts to the regression test.
- Update Telegram hot-activation E2E test to expect
fields: {}in setup POST payloads. - Add job-level and step-level timeouts to Rust test jobs in the GitHub Actions workflow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/channels/repl.rs |
Adjusts sender retention in single-message mode and hardens the regression test with timeouts. |
tests/e2e/scenarios/test_telegram_hot_activation.py |
Updates expected captured setup payloads to include fields. |
.github/workflows/test.yml |
Caps job runtimes and wraps cargo test invocations with timeout to avoid hangs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zmanian
left a comment
There was a problem hiding this comment.
LGTM. The REPL hang fix is correct -- the extra tx.clone() stored in msg_tx kept the channel open after single-message mode's thread dropped its sender. Gating on self.single_message.is_none() is the right fix since single-message mode never needs the approval injection path.
CI timeout caps are well-layered (job-level + step-level with --signal=INT). Step timeouts correctly nest inside job timeouts with 5-10 min headroom.
One minor note: timeout is a GNU coreutils utility not available on macOS runners. Fine for current ubuntu-only CI, but worth noting if macOS runners are ever added.
Ship it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Spawn a fire in a background task. | ||
| fn spawn_fire(&self, routine: Routine, trigger_type: &str, trigger_detail: Option<String>) { | ||
| fn spawn_fire( | ||
| &self, | ||
| routine: Routine, | ||
| trigger_type: &str, | ||
| trigger_detail: Option<String>, | ||
| ) -> JoinHandle<()> { |
There was a problem hiding this comment.
spawn_fire now returns a tokio::task::JoinHandle<()>, which is #[must_use]. There are still call sites in this file that ignore the return value (e.g. in emit_system_event and check_cron_triggers), which will trigger an "unused JoinHandle" warning and fail CI under cargo clippy -D warnings. Please explicitly detach the task (e.g. drop(...) / let _ = ...) or otherwise handle the handle at those call sites.
|
Already got approvals were addressing existing bugs from before. filed some issues for the rest + more testing |
* Fix REPL single-message hang and cap CI test duration * Fix Clippy nested-if lint in REPL startup * Fix single-message approval flow * Handle empty single-message REPL exits * Wait for one-shot event routines before exit
* Fix REPL single-message hang and cap CI test duration * Fix Clippy nested-if lint in REPL startup * Fix single-message approval flow * Handle empty single-message REPL exits * Wait for one-shot event routines before exit
Summary
Testing
Notes