Conversation
…on/pruning
- Add context.Context to Scheduler public API (e.g., NewScheduler, Schedule,
Remove, Stop, JobCount, JobIDs, JobStatus/JobStatuses, JobHistory,
QueryJobStatuses/QueryJobHistory, reconcileRecoveredState, etc.)
- Update storage interfaces to require context across all operations
- SQLite storage:
- Introduce finished_at_unix_nano column and handle bootstrap migration
- Add history retention options (WithSQLiteHistoryRetention,
WithSQLiteHistoryMaxAge, WithSQLiteHistoryMaxRowsPerJob)
- Implement PruneHistory / PruneHistoryWithRetention and related helpers
- Provide NewSQLiteJobsStorageWithOptions for options-based setup
- Update examples and tests to use context (t.Context()) and new APIs
- Docs: update README and PRD with SQLite retention and pruning guidance
BREAKING CHANGE: Scheduler and storage method signatures now take
context.Context. All call sites must be updated to pass a context.
SQLite history schema adds finished_at_unix_nano; migration is handled
automatically during bootstrap.
- Introduce pollTimeout (5s) and create ctx via context.WithTimeout; defer cancel - Pass ctx to NewScheduler and NewSQLiteJobsStorageWithOptions - Update calls: Stop() → Stop(ctx), Schedule(job) → Schedule(ctx, job) - Update lookups: JobStatus(id) → JobStatus(ctx, id), JobHistory(id) → JobHistory(ctx, id) - SQLite example: PruneHistory() → PruneHistory(ctx) Aligns examples with the new context-aware API and ensures graceful shutdowns and bounded polling.
There was a problem hiding this comment.
Pull request overview
This PR extends the scheduler and storage layers to be context-aware and adds SQLite job-history retention/pruning capabilities, updating tests and examples accordingly.
Changes:
- Add
context.Contextparameters across scheduler andJobsStorageAPIs. - Implement SQLite history retention (max-age, max-rows-per-job) plus manual pruning APIs and a schema migration column for retention metadata.
- Update scheduler tests, storage contract tests, and examples to use the new context-aware APIs.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/scheduler_test.go |
Updates scheduler tests to pass contexts and refactors status-query assertions into helpers. |
pkg/scheduler/storage_sqlite.go |
Adds history retention options, prune APIs, and a finished_at_unix_nano column used for age-based pruning. |
pkg/scheduler/storage_contract_test.go |
Updates contract tests for ctx-aware storage and adds retention/pruning behavior tests for SQLite. |
pkg/scheduler/storage.go |
Updates JobsStorage interface and in-memory implementation to accept context.Context. |
pkg/scheduler/scheduler_internal_test.go |
Updates internal scheduler tests to use ctx-aware constructors and methods. |
pkg/scheduler/scheduler.go |
Makes scheduler APIs ctx-aware and threads contexts through storage/status/history operations and job execution. |
__examples/scheduler_sqlite/scheduler_sqlite.go |
Updates SQLite example to use ctx-aware APIs and demonstrates pruning. |
__examples/scheduler/scheduler.go |
Updates basic scheduler example to use ctx-aware APIs. |
README.md |
Attempts to document SQLite retention/pruning features (currently has signature mismatches). |
PRD.md |
Updates product notes to include SQLite history retention/pruning capabilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dbPath := filepath.Join(os.TempDir(), "go-again-scheduler-example.db") | ||
| storage, err := scheduler.NewSQLiteJobsStorage(dbPath) | ||
| storage, err := scheduler.NewSQLiteJobsStorageWithOptions( | ||
| dbPath, | ||
| scheduler.WithSQLiteHistoryMaxAge(24*time.Hour), | ||
| scheduler.WithSQLiteHistoryMaxRowsPerJob(100), | ||
| ) |
There was a problem hiding this comment.
The README example no longer matches the updated SQLite storage constructor signature: NewSQLiteJobsStorageWithOptions now requires a context.Context as the first argument. As written, this snippet won’t compile and should be updated to pass a ctx (and likely update the rest of the snippet to use the new ctx-taking scheduler APIs as well).
| pruned, err := storage.PruneHistory() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| fmt.Println("pruned rows:", pruned) |
There was a problem hiding this comment.
SQLiteJobsStorage.PruneHistory now requires a context.Context parameter. This example calls it with no args, so it won’t compile; update it to pass the same ctx used to construct the storage/scheduler (or another appropriate context).
| - `NewSQLiteJobsStorage(path)` provides a built-in durable storage implementation for `WithJobsStorage(...)`; call `Close()` when finished. | ||
| - `NewSQLiteJobsStorageWithOptions(path, ...)` configures SQLite retention controls: `WithSQLiteHistoryMaxAge(duration)`, `WithSQLiteHistoryMaxRowsPerJob(n)`, and `WithSQLiteHistoryRetention(...)`. | ||
| - `SQLiteJobsStorage.PruneHistory()` and `PruneHistoryWithRetention(...)` provide manual pruning for periodic cleanup jobs. |
There was a problem hiding this comment.
These bullets document the SQLite APIs without the new context.Context parameters (e.g., NewSQLiteJobsStorageWithOptions and PruneHistory/PruneHistoryWithRetention). Please update the documented signatures to include the ctx arguments so readers don’t copy/paste non-compiling examples.
| - `NewSQLiteJobsStorage(path)` provides a built-in durable storage implementation for `WithJobsStorage(...)`; call `Close()` when finished. | |
| - `NewSQLiteJobsStorageWithOptions(path, ...)` configures SQLite retention controls: `WithSQLiteHistoryMaxAge(duration)`, `WithSQLiteHistoryMaxRowsPerJob(n)`, and `WithSQLiteHistoryRetention(...)`. | |
| - `SQLiteJobsStorage.PruneHistory()` and `PruneHistoryWithRetention(...)` provide manual pruning for periodic cleanup jobs. | |
| - `NewSQLiteJobsStorage(ctx context.Context, path)` provides a built-in durable storage implementation for `WithJobsStorage(...)`; call `Close()` when finished. | |
| - `NewSQLiteJobsStorageWithOptions(ctx context.Context, path, ...)` configures SQLite retention controls: `WithSQLiteHistoryMaxAge(duration)`, `WithSQLiteHistoryMaxRowsPerJob(n)`, and `WithSQLiteHistoryRetention(...)`. | |
| - `SQLiteJobsStorage.PruneHistory(ctx context.Context)` and `PruneHistoryWithRetention(ctx context.Context, ...)` provide manual pruning for periodic cleanup jobs. |
| func (s *Scheduler) runJob(ctx context.Context, entry *jobEntry) { | ||
| defer s.wg.Done() | ||
| defer s.cleanupJob(entry) | ||
| defer s.cleanupJob(ctx, entry) | ||
|
|
||
| job := entry.job | ||
|
|
||
| terminalState := JobStateCompleted | ||
|
|
||
| defer func() { | ||
| s.finalizeJobState(job.ID, terminalState) | ||
| s.finalizeJobState(ctx, job.ID, terminalState) | ||
| }() |
There was a problem hiding this comment.
runJob defers cleanupJob and finalizeJobState using the job’s ctx. If that ctx is canceled (e.g., if callers pass a request-scoped context into Schedule), the SQLite storage operations inside these paths can be aborted, leaving active-job rows and/or terminal status updates unpersisted. Consider using a non-cancelable context for best-effort persistence (e.g., context.WithoutCancel(...) or the scheduler’s lifecycle context) for cleanup/finalization writes, while still respecting cancellation for the execution loop itself.
| func pruneHistoryOlderThanForJobTx(ctx context.Context, tx *sql.Tx, id string, cutoffUnixNano int64) (int64, error) { | ||
| res, err := tx.ExecContext( | ||
| ctx, | ||
| `DELETE FROM scheduler_job_history WHERE id = ? AND finished_at_unix_nano > 0 AND finished_at_unix_nano < ?`, | ||
| id, | ||
| cutoffUnixNano, | ||
| ) | ||
| if err != nil { | ||
| return 0, ewrap.Wrapf(ErrStorageOperation, "prune execution history by age for job failed: %v", err) | ||
| } | ||
|
|
||
| return rowsAffected(res) | ||
| } | ||
|
|
||
| func pruneHistoryOlderThanTx(ctx context.Context, tx *sql.Tx, cutoffUnixNano int64) (int64, error) { | ||
| res, err := tx.ExecContext( | ||
| ctx, | ||
| `DELETE FROM scheduler_job_history WHERE finished_at_unix_nano > 0 AND finished_at_unix_nano < ?`, | ||
| cutoffUnixNano, | ||
| ) |
There was a problem hiding this comment.
Age-based pruning ignores rows where finished_at_unix_nano is 0 (> 0 filter). After migrating an existing DB, all pre-existing history rows will have the new column defaulted to 0, so MaxAge retention and PruneHistory* won’t remove any of that existing history by age. Consider backfilling finished_at_unix_nano from payload_json during bootstrap/migration, or adjusting the pruning semantics for 0 values so retention behaves as expected on upgraded databases.
No description provided.