fix(uptime): remove and prevent unwrap/panic #462
Conversation
expect() are still allowed, since there are lots of places where it seems perfectly valid (poisoned locks, in particular, but also lots of things in the startup path.)
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| let check_task_result = if conf.record_task_metrics { | ||
| if conf.checker_parallel { | ||
| tokio::spawn(metrics_monitor.instrument(check_fut)) | ||
| .await | ||
| .expect("The check task should not fail"); | ||
| tokio::spawn(metrics_monitor.instrument(check_fut)).await | ||
| } else { | ||
| metrics_monitor.instrument(check_fut).await; | ||
| Ok(()) | ||
| } | ||
| } else { | ||
| tokio::spawn(check_fut) | ||
| .await | ||
| .expect("The check task should not fail"); | ||
| tokio::spawn(check_fut).await | ||
| }; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| .timeout | ||
| .to_std() | ||
| .expect("Timeout duration should be representable as a duration"); | ||
| let timeout = check_config.timeout.to_std().unwrap_or_default(); |
There was a problem hiding this comment.
Bug: A negative check_config.timeout value causes to_std() to fail, and unwrap_or_default() converts it to Duration::ZERO, making all HTTP requests timeout immediately.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The change from expect() to unwrap_or_default() introduces a silent failure. Negative check_config.timeout values, which are not validated upstream, can exist in production. When a negative TimeDelta is converted using to_std(), it returns an error. The new unwrap_or_default() call handles this by returning Duration::ZERO. This causes the reqwest client to timeout all HTTP check requests immediately, breaking the timeout functionality without any error or alert. The previous implementation would have panicked, which served as a safeguard against invalid data.
💡 Suggested Fix
Instead of using unwrap_or_default(), handle the Err case from to_std() explicitly. Propagate the error, log it and use a reasonable default timeout, or add validation upstream to prevent negative TimeDelta values from reaching this code path. The previous expect() call should be replaced with proper error handling, not a silent failure.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/checker/reqwest_checker.rs#L71
Potential issue: The change from `expect()` to `unwrap_or_default()` introduces a silent
failure. Negative `check_config.timeout` values, which are not validated upstream, can
exist in production. When a negative `TimeDelta` is converted using `to_std()`, it
returns an error. The new `unwrap_or_default()` call handles this by returning
`Duration::ZERO`. This causes the `reqwest` client to timeout all HTTP check requests
immediately, breaking the timeout functionality without any error or alert. The previous
implementation would have panicked, which served as a safeguard against invalid data.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8184931
expect() are still allowed, since there are lots of places where it seems perfectly valid (poisoned locks, in particular, but also lots of things in the startup path.)