feat: add jitter wrappers to prevent thundering herd#258
Conversation
Add Jitter() and JitterWithLogger() JobWrappers that add a random delay before job execution to prevent thundering herd problems when many jobs are scheduled at the same time. Features: - Jitter(maxJitter): Adds random delay in [0, maxJitter) range - JitterWithLogger(logger, maxJitter): Same with delay logging - Zero or negative maxJitter disables jitter (no-op) - Uses math/rand/v2 from Go stdlib (no external dependency) This addresses issue #227, implementing jitter using the existing JobWrapper pattern rather than the upstream approach of adding multiple new methods to the Cron type.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
This PR adds jitter support to prevent thundering herd problems when multiple jobs execute simultaneously. The implementation uses a composable JobWrapper pattern that integrates seamlessly with existing wrappers.
Key changes:
- Adds
Jitter(maxJitter)andJitterWithLogger(logger, maxJitter)wrapper functions that add random delays before job execution - Uses
math/rand/v2for generating uniformly distributed delays in [0, maxJitter) - Zero or negative maxJitter values disable jitter for zero overhead
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| chain.go | Implements Jitter and JitterWithLogger wrapper functions with uniform random delay distribution |
| chain_test.go | Adds comprehensive tests covering delay behavior, edge cases, composition with other wrappers, and logging |
| doc.go | Updates documentation to include jitter in the list of available wrappers |
| example_test.go | Adds three example functions demonstrating global jitter, per-job jitter, and jitter with logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| panicJob := FuncJob(func() { | ||
| recovered = false | ||
| panic("test panic") | ||
| }) | ||
|
|
||
| wrappedPanicJob := NewChain( | ||
| Recover(DiscardLogger), | ||
| Jitter(10*time.Millisecond), | ||
| ).Then(panicJob) | ||
|
|
||
| // Should not panic due to Recover wrapper | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| t.Errorf("panic was not recovered: %v", r) | ||
| } else { | ||
| recovered = true | ||
| } | ||
| }() | ||
| wrappedPanicJob.Run() | ||
| }() |
There was a problem hiding this comment.
The test logic for verifying panic recovery is flawed. The 'recovered' variable is set to false inside the panicJob function, but this assignment never executes because the panic occurs immediately. The defer function in the test then sets 'recovered = true' in the else branch when no panic reaches it (since the Recover wrapper catches it). This doesn't actually verify that the Recover wrapper caught the panic - it only verifies that the test function itself didn't receive a panic.
To properly test this, you should verify that the job panicked and was recovered by checking logs or using a different mechanism to track recovery.
Summary
Adds jitter support to prevent thundering herd problems when many jobs are scheduled at the same time.
Closes #227
Jitter(maxJitter time.Duration)JobWrapper - adds random delay in [0, maxJitter)JitterWithLogger(logger, maxJitter)- same with delay logging for observabilitymath/rand/v2from Go stdlib (no external dependency)Implementation Approach
This implements jitter using the existing JobWrapper pattern rather than the upstream approach (robfig#525) that adds 4 new methods to the Cron type. Benefits:
WithChainor per-job viaNewChain().Then()Usage Examples
Test plan