Add a timeout to graceful termination #846
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 02566ef in 29 seconds
More details
- Looked at
61lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. plane/src/controller/mod.rs:323
- Draft comment:
The error handling forOk(Err(err))andOk(Ok(Err(err)))is the same, which might be confusing. Consider differentiating the error messages for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a timeout for graceful termination, but the error handling for the server handle is not consistent. The error message forOk(Err(err))andOk(Ok(Err(err)))is the same, which might be confusing.
Workflow ID: wflow_s3tuZyf577uMveoD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Yes, #844 was actually made as a way to test it. It works as expected locally. |
📝 WalkthroughWalkthroughThe pull request introduces significant changes to two files: In Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
plane/src/controller/mod.rs (2)
61-64: LGTM! Consider making the timeout duration configurable.The timeout duration is well-documented and reasonable for short requests. However, consider making it configurable through
ControllerConfigto allow operators to adjust based on their specific needs.pub struct ControllerConfig { pub db_url: String, pub bind_addr: SocketAddr, pub id: ControllerName, pub controller_url: Url, pub default_cluster: Option<ClusterName>, pub cleanup_min_age_days: Option<i32>, pub cleanup_batch_size: Option<i32>, pub forward_auth: Option<Url>, + pub terminate_timeout: Option<std::time::Duration>, }
303-303: LGTM! Consider enhancing error logging.The timeout implementation is solid and handles all possible outcomes appropriately. However, the error logging could be more descriptive.
- Ok(Ok(Err(err))) => { - tracing::error!(?err, "Server error"); - } - Ok(Err(err)) => { - tracing::error!(?err, "Server error"); - } + Ok(Ok(Err(err))) => { + tracing::error!(?err, "Server failed during graceful shutdown"); + } + Ok(Err(err)) => { + tracing::error!(?err, "Server task failed during graceful shutdown"); + }Also applies to: 316-328
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
plane/src/admin.rs(0 hunks)plane/src/controller/mod.rs(3 hunks)
💤 Files with no reviewable changes (1)
- plane/src/admin.rs
🔇 Additional comments (1)
plane/src/controller/mod.rs (1)
316-328: Verify impact on long-running handlers.
The 2-second timeout might affect long-running operations. Let's verify any handlers that might need special handling.
✅ Verification successful
Based on the search results, I can now provide a final response:
Timeout duration is appropriate for server termination
The 2-second TERMINATE_TIMEOUT_DURATION is suitable because:
- Long-running operations are implemented as independent tasks/loops that can be aborted separately
- The timeout only affects new incoming requests during shutdown, not existing operations
- Key long-running operations have their own termination handling:
- Backend status streaming (
handle_backend_status_stream) - Drone sweep loop with explicit abort
- Cleanup loop as a separate task
- Heartbeat loop with sleep intervals
- Backend status streaming (
The timeout is specifically designed to handle new short-lived requests during shutdown, while existing long-running operations are handled through their respective cancellation mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential long-running handlers that might be affected by the timeout
# Look for handlers with streaming or long-running patterns
rg -A 3 "Stream|stream|subscribe|watch|poll|long|infinite|loop" plane/src/controller/
# Look for handlers using sleep or delay
ast-grep --pattern 'sleep($$$)'
ast-grep --pattern 'delay($$$)'
Length of output: 9319
Important
Add a 2-second timeout for graceful server termination in
ControllerServerand remove unused imports inadmin.rs.TERMINATE_TIMEOUT_DURATIONconstant incontroller/mod.rsto set a 2-second timeout for graceful server termination.terminate()inControllerServerto usetokio::time::timeoutfor server shutdown.StreamandStreamExtfromadmin.rs.This description was created by
for 02566ef. It will automatically update as commits are pushed.