fix(flags): interpret bare numeric timeout as seconds - #1675
Conversation
- Add isPureNumeric helper for bare integer/float detection - Pre-parse bare values in envDuration before viper/cast processes them - Add table-driven tests for isPureNumeric and envDuration - Add fuzz test for isPureNumeric invariant verification - Document bare numeric interpretation in timeout flag docs
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1675 +/- ##
==========================================
+ Coverage 74.60% 74.68% +0.08%
==========================================
Files 59 59
Lines 9839 9878 +39
==========================================
+ Hits 7340 7377 +37
- Misses 2237 2238 +1
- Partials 262 263 +1
🚀 New features to boost your workflow:
|
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 2 medium |
🟢 Metrics 30 complexity · 0 duplication
Metric Results Complexity 30 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR treats bare numeric environment values for duration flags as seconds by adding a preprocessing step in envDuration that detects pure numeric strings, parses them as floats (seconds), clamps overflow, and falls back to viper.GetDuration for non-numeric/unit-suffixed inputs. Tests, a fuzz test, and docs were added. ChangesBare Numeric Duration Timeout Parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/flags/flags.go`:
- Around line 616-619: The float-to-duration conversion using time.Duration(val
* float64(time.Second)) must be guarded against int64 overflow; compute nanos :=
val * float64(time.Second) and check against float64(math.MaxInt64) and
float64(math.MinInt64) (import math), then if nanos exceeds those bounds clamp
to math.MaxInt64 or math.MinInt64 (or return an error per surrounding
convention) before converting to time.Duration; update the strconv.ParseFloat
branch (the val variable and conversion expression) to perform this bounds check
and safe conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a2a13b4-1766-41fd-b684-43b4bcd83ac8
📒 Files selected for processing (4)
docs/configuration/arguments/index.mdinternal/flags/flags.gointernal/flags/flags_fuzz_test.gointernal/flags/flags_test.go
- Add overflow checks before converting float64 nanoseconds to time.Duration - Clamp values exceeding int64 range to math.MaxInt64 or math.MinInt64 - Update test to expect MaxInt64 clamping instead of zero fallback for very large values - Add test case for explicit overflow clamping behavior
This PR fixes container restart failures when
WATCHTOWER_TIMEOUTis set as a bare number (e.g.,60).Problem
Setting
WATCHTOWER_TIMEOUT=60was interpreted as 60 nanoseconds instead of 60 seconds by viper/cast, causing immediate context deadline exceeded errors during container creation. Users reported containers failing to restart after updates (see #1664).Solution
Pre-parse bare numeric values in
envDurationbefore viper/cast processes them (which would otherwise interpret bare numbers as nanoseconds).Changes
isPureNumerichelper to detect bare integer/float values without duration unitsenvDurationfor bare numeric values, interpreting them as secondsTestIsPureNumerictable-driven testsTestEnvDuration_LegacyBareNumberAsSecondstable-driven testsFuzzIsPureNumericfuzz test for invariant verificationSummary by CodeRabbit
Documentation
60,1.5) are treated as seconds; explicit unit suffixes required for other time units.Behavior
Tests