Conversation
…cycle Introduce Cron.Shutdown(ctx) as a companion to Stop: it stops future scheduling and waits for in-flight jobs to finish without cancelling their contexts. Stop retains its existing behaviour of cancelling job contexts before draining. Internally, replace the single (rootCtx, rootCancel, jobWaiter, loopDone) tuple with a runLifecycle struct that holds independent scheduler and job context pairs. This makes it straightforward for Stop and Shutdown to target different cancellation scopes and allows completed runs to be garbage-collected once all jobs have returned. Additional hardening: - @every rejects non-positive durations (zero and negative) and rounds sub-second intervals up to one second, both with explicit errors. - Malformed TZ=/CRON_TZ= prefixes (empty location name or missing schedule body) now return parse errors instead of panicking. - WithLocation, WithParser, WithLogger, and WithClock treat nil as "keep the package default" rather than leaving the scheduler in an invalid state. Tests, examples, and documentation (README, MIGRATION, CHANGELOG, doc.go) are updated to cover all new behaviour. Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Pull request overview
This PR extends the cron scheduler lifecycle with a new Cron.Shutdown(ctx) method for graceful draining (stop scheduling new work and wait for in-flight jobs without cancelling their contexts), while keeping Stop(ctx) as the “cancel running job contexts, then drain” behavior. It also hardens parsing and configuration defaults to avoid panics/invalid states and updates docs/tests/examples accordingly.
Changes:
- Add
Cron.Shutdown(ctx)and refactor runtime state into a per-run lifecycle model to support separate scheduler vs job cancellation scopes. - Harden parsing: reject non-positive
@everyintervals; treat malformedTZ=/CRON_TZ=prefixes as parse errors. - Make
WithLocation/WithParser/WithLogger/WithClocktolerate nil inputs, and update tests/docs/examples.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cron.go | Introduces runLifecycle, implements Shutdown, and refactors stop/drain behavior across runs. |
| cron_test.go | Adds coverage for Shutdown semantics and shared stop/shutdown drain tests. |
| parser.go | Adds explicit errors for malformed timezone prefixes and non-positive @every durations; factors @every parsing. |
| parser_test.go | Adds test cases for new @every and TZ prefix validation behavior. |
| option.go | Adjusts With* options to handle nil inputs safely and updates option docs. |
| option_test.go | Adds test ensuring nil options preserve valid defaults. |
| example_test.go | Adds runnable examples for Shutdown and Recover. |
| doc.go | Updates package docs for new lifecycle semantics, @every constraints, TZ parsing, and default option behavior. |
| README.md | Updates quick-start + docs to mention Shutdown and hardened parsing/configuration. |
| MIGRATION.md | Documents Shutdown and non-default panic recovery. |
| CHANGELOG.md | Records new API and behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt) | ||
| defer stop() | ||
|
|
||
| c.Start(ctx) | ||
| c.Start(context.Background()) | ||
|
|
||
| <-ctx.Done() | ||
| c.Stop(context.Background()) | ||
| c.Shutdown(context.Background()) | ||
| } |
There was a problem hiding this comment.
The quick-start example creates a signal-aware ctx but then starts the scheduler with context.Background(). That means the scheduler/job contexts are not tied to the interrupt signal (so cancellation/deadlines on ctx won’t propagate to jobs), which undermines the “Context-aware lifecycle” claim right below. Consider passing ctx to Start (and choose Stop vs Shutdown based on whether you want cancellation vs graceful drain).
| done := make(chan struct{}) | ||
|
|
||
| cronInstance.AddFunc(exampleEverySecond, func(_ context.Context) error { | ||
| fmt.Println("tick") | ||
| close(done) |
There was a problem hiding this comment.
ExampleCron_Shutdown closes the done channel inside a job scheduled every second. If the job runs more than once before Shutdown executes (e.g., under load or slow scheduling), a second close will panic and can make the example flaky. Consider using a non-closing signal (e.g., sending on a buffered channel) or guarding the close with sync.Once.
| done := make(chan struct{}) | |
| cronInstance.AddFunc(exampleEverySecond, func(_ context.Context) error { | |
| fmt.Println("tick") | |
| close(done) | |
| done := make(chan struct{}, 1) | |
| cronInstance.AddFunc(exampleEverySecond, func(_ context.Context) error { | |
| fmt.Println("tick") | |
| select { | |
| case done <- struct{}{}: | |
| default: | |
| } |
| filtered := c.runs[:0] | ||
| for _, run := range c.runs { |
There was a problem hiding this comment.
cleanupCompletedRunsLocked filters c.runs in-place via filtered := c.runs[:0]. This leaves stale *runLifecycle pointers in the underlying array beyond len(filtered), which can prevent completed runs from being garbage-collected (contrary to the intent described in the PR). Consider clearing the unused tail (e.g., clear(c.runs[len(filtered):])) or allocating a new slice when compacting.
| // WithLocation overrides the timezone of the cron instance. Passing nil keeps | ||
| // the default [time.Local]. | ||
| func WithLocation(loc *time.Location) Option { | ||
| return func(c *Cron) { | ||
| c.location = loc | ||
| return func(cronInstance *Cron) { | ||
| if loc == nil { | ||
| cronInstance.location = time.Local | ||
|
|
||
| return | ||
| } | ||
|
|
||
| cronInstance.location = loc | ||
| } |
There was a problem hiding this comment.
WithLocation(nil) resets the location to time.Local, while WithParser(nil)/WithLogger(nil)/WithClock(nil) are no-ops. This makes option ordering surprising (e.g., WithLocation(UTC), WithLocation(nil) silently flips back to Local) and is inconsistent with the other nil-handling options. Consider making WithLocation(nil) a no-op (or, if the intent is to restore defaults, apply that same reset-to-default behavior consistently across all With* options).
Introduce Cron.Shutdown(ctx) as a companion to Stop: it stops future scheduling and waits for in-flight jobs to finish without cancelling their contexts. Stop retains its existing behaviour of cancelling job contexts before draining.
Internally, replace the single (rootCtx, rootCancel, jobWaiter, loopDone) tuple with a runLifecycle struct that holds independent scheduler and job context pairs. This makes it straightforward for Stop and Shutdown to target different cancellation scopes and allows completed runs to be garbage-collected once all jobs have returned.
Additional hardening:
Tests, examples, and documentation (README, MIGRATION, CHANGELOG, doc.go) are updated to cover all new behaviour.