Fix: guard addJitter against zero/sub-nanosecond durations to prevent panic (fixes #8149)#8178
Conversation
shivamtiwari3
force-pushed
the
fix/addjitter-panic-on-zero-duration
branch
2 times, most recently
from
49d6ecb to
a97fc99
Compare
There was a problem hiding this comment.
Pull request overview
Prevents a runtime panic in adaptive sampling’s jitter calculation by guarding rand.Int63n against zero (or sub-nanosecond truncation to zero) bounds, and adds unit tests to cover the edge cases reported in #8149.
Changes:
- Add a
half := jitterAmount / 2guard inaddJitterto avoidrand.Int63n(0)panics. - Add
TestAddJitterto validate zero/1ns inputs and ensure typical durations stay within expected bounds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/sampling/samplingstrategy/adaptive/post_aggregator.go | Adds a small-duration guard to prevent rand.Int63n panic in addJitter. |
| internal/sampling/samplingstrategy/adaptive/post_aggregator_test.go | Adds tests covering the panic scenarios and validating jitter bounds for normal durations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, time.Duration(0), addJitter(0)) | ||
|
|
||
| // 1ns duration: jitterAmount/2 == 0, must not panic and must return 1ns | ||
| assert.Equal(t, time.Duration(1), addJitter(1)) |
| half := jitterAmount / 2 | ||
| if half <= 0 { | ||
| return jitterAmount | ||
| } | ||
| return half + time.Duration(rand.Int63n(int64(half))) |
…arity Replace raw time.Duration(0) and time.Duration(1) literals with the explicit 0*time.Nanosecond and time.Nanosecond constants to make the intended unit immediately clear to readers. Per Copilot review suggestion on PR jaegertracing#8178.
|
Addressed both Copilot review suggestions:
|
… panic (fixes jaegertracing#8149) Root cause: addJitter called rand.Int63n(int64(jitterAmount/2)); when jitterAmount is 0 or 1ns the integer division yields 0, and rand.Int63n(0) panics at runtime. Fix: compute half = jitterAmount/2 once and return jitterAmount unchanged when half <= 0, so no call to rand.Int63n is made for degenerate inputs. Signed-off-by: shivamtiwari3 <33183708+shivamtiwari3@users.noreply.github.com>
…arity Replace raw time.Duration(0) and time.Duration(1) literals with the explicit 0*time.Nanosecond and time.Nanosecond constants to make the intended unit immediately clear to readers. Per Copilot review suggestion on PR jaegertracing#8178. Signed-off-by: shivamtiwari3 <33183708+shivamtiwari3@users.noreply.github.com>
shivamtiwari3
force-pushed
the
fix/addjitter-panic-on-zero-duration
branch
from
5cc3195 to
31446be
Compare
|
Added |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8178 +/- ##
=======================================
Coverage 95.67% 95.67%
=======================================
Files 317 317
Lines 16747 16750 +3
=======================================
+ Hits 16023 16026 +3
Misses 571 571
Partials 153 153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6275e86
Summary
Fixes #8149.
addJitterininternal/sampling/samplingstrategy/adaptive/post_aggregator.gopassedint64(jitterAmount/2)directly torand.Int63n. WhenjitterAmountis0or1ns, integer division yields0, andrand.Int63n(0)panics at runtime withpanic: invalid argument to Int63n.Root Cause
In
post_aggregator.go:145:rand.Int63n(n)requiresn > 0; it panics otherwise. No guard prevented calling it withn == 0. A misconfiguredFollowerLeaseRefreshInterval(e.g. set to0or1nsin a YAML config or test environment) or an unexpected zero value reachingaddJitterfromprovider.go:92would crash the process.Solution
Compute
half = jitterAmount / 2once. Ifhalf <= 0(coversjitterAmount == 0andjitterAmount == 1ns), returnjitterAmountunchanged — no jitter is added, but the caller is not panicked. Otherwise apply the existing formula usinghalf.Testing
TestAddJitterinpost_aggregator_test.go:addJitter(0)returns0without panickingaddJitter(1ns)returns1nswithout panicking (sub-nanosecond integer division edge case)2ms,1s,20s,60s): 100 calls each confirm the result is always in[d/2, d)Run with:
Checklist