# Plan: Consolidated `jobs` Package for Background Cron Tasks **Goal:** Extract 9 side-effect cleanup functions from `GetAvailableHours` HTTP handler into a dedicated `jobs` package with cron-scheduled, parallel execution. Remove the lazy-cleanup antipattern. **Current state:** - 9 cleanup functions in `backend/handlers/scheduling/time-blockers.go` — all called synchronously in `default-hours.go:364-407` on every `GET /available-hours` request - Only `CleanupOldReservations` ALSO runs on a background goroutine (`main.go:432-450`, 5min ticker) - 4 other ad-hoc background goroutines exist (JWT cleanup, GDPR cache, login state, rate limiter) - `robfig/cron/v3` v3.0.1 is already a `go.mod` dependency (currently used only for parsing time-blocker cron expressions) --- ## Phase 1: Create `backend/internal/jobs/` Package **New package:** `crussell/internal/jobs` ### File: `scheduler.go` Core scheduler abstraction built on `robfig/cron/v3`: ```go package jobs import ( "context" "log" "time" "github.com/robfig/cron/v3" ) // Job defines a single periodic task. type Job struct { Name string // Human-readable name (for logging) Schedule string // Standard cron expression ("*/5 * * * *") Timeout time.Duration // Per-execution timeout Concurrency int // Max concurrent runs (0 = unlimited, 1 = serial) Handler func(context.Context) error // The actual work } // Scheduler manages all registered cron jobs with parallel execution. type Scheduler struct { cron *cron.Cron entries []cron.EntryID registry []Job baseCtx context.Context cancel context.CancelFunc semaphores map[string]chan struct{} // Per-job concurrency limit } ``` **Key design decisions:** 1. Each job runs in its own goroutine (cron v3 default) — parallel by nature 2. Per-job concurrency control via channel semaphore — prevents overlapping runs of the same job 3. Panic recovery wrapper — matches existing pattern in `jwt.go`, `gdpr_export.go`, `auth/local.go` 4. Timeout via `context.WithTimeout` — matches existing `main.go` pattern 5. Graceful shutdown via `baseCtx` — the cron scheduler is stopped, then active jobs drain ```go func New() *Scheduler { ctx, cancel := context.WithCancel(context.Background()) return &Scheduler{ cron: cron.New(cron.WithLocation(londonLocation)), baseCtx: ctx, cancel: cancel, semaphores: make(map[string]chan struct{}), } } func (s *Scheduler) Register(job Job) { // Validate cron expression at registration time parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) if _, err := parser.Parse(job.Schedule); err != nil { log.Fatalf("jobs: invalid cron schedule %q for job %q: %v", job.Schedule, job.Name, err) } s.registry = append(s.registry, job) } func (s *Scheduler) Start() { for _, job := range s.registry { j := job // capture entryID, err := s.cron.AddFunc(j.Schedule, s.wrapJob(j)) if err != nil { log.Fatalf("jobs: failed to register %q: %v", j.Name, err) } s.entries = append(s.entries, entryID) } s.cron.Start() } func (s *Scheduler) Shutdown() <-chan struct{} { ctx := s.cron.Stop() // Stop scheduler (returns ctx that completes when all jobs finish) s.cancel() // Cancel base context so in-flight jobs know to stop return ctx.Done() // Caller can wait for this } // wrapJob adds panic recovery, timeout, concurrency control, and logging. func (s *Scheduler) wrapJob(job Job) func() { // Set up semaphore if concurrency limited var sem chan struct{} if job.Concurrency > 0 { sem = make(chan struct{}, job.Concurrency) sem <- struct{}{} // Initial slot filled } return func() { // Concurrency guard if sem != nil { select { case <-sem: // Acquired — proceed default: log.Printf("jobs: %q skipped (previous run still in progress)", job.Name) return } defer func() { sem <- struct{}{} }() } // Panic recovery defer func() { if r := recover(); r != nil { log.Printf("jobs: panic recovered in %q: %v", job.Name, r) } }() // Timeout ctx, cancel := context.WithTimeout(s.baseCtx, job.Timeout) defer cancel() start := time.Now() if err := job.Handler(ctx); err != nil { log.Printf("jobs: %q failed: %v (duration: %v)", job.Name, err, time.Since(start)) } else { log.Printf("jobs: %q completed (duration: %v)", job.Name, time.Since(start)) } } } ``` ### File: `cleanup.go` Registration of all 9 scheduling cleanup functions + JWT cleanup. ```go package jobs import ( "context" "crussell/auth" "crussell/handlers/scheduling" "time" ) // RegisterAll registers every background job. func RegisterAll(s *Scheduler) { // === HIGH FREQUENCY (every 5 min) === s.Register(Job{ Name: "cleanup-reservations", Schedule: "*/5 * * * *", Timeout: 30 * time.Second, Concurrency: 1, Handler: scheduling.CleanupOldReservations, }) s.Register(Job{ Name: "cleanup-expired-deposits", Schedule: "*/5 * * * *", Timeout: 30 * time.Second, Concurrency: 1, Handler: scheduling.CleanupExpiredDeposits, }) // === MID FREQUENCY (hourly) === s.Register(Job{ Name: "cleanup-expired-loyalty-redemptions", Schedule: "0 * * * *", Timeout: 30 * time.Second, Concurrency: 1, Handler: scheduling.CleanupExpiredLoyaltyRedemptions, }) s.Register(Job{ Name: "cleanup-old-idempotency-keys", Schedule: "0 * * * *", Timeout: 30 * time.Second, Concurrency: 1, Handler: scheduling.CleanupOldIdempotencyKeys, }) s.Register(Job{ Name: "cleanup-revoked-jtis", Schedule: "0 * * * *", Timeout: 30 * time.Second, Concurrency: 1, Handler: func(ctx context.Context) error { auth.CleanupRevokedJTIs(ctx) return nil }, }) // === LOW FREQUENCY (daily, off-peak) === s.Register(Job{ Name: "anonymize-stale-guest-accounts", Schedule: "0 3 * * *", // 3am Timeout: 5 * time.Minute, Concurrency: 1, Handler: scheduling.AnonymizeStaleGuestAccounts, }) s.Register(Job{ Name: "cleanup-expired-financial-records", Schedule: "0 4 * * *", // 4am Timeout: 10 * time.Minute, Concurrency: 1, Handler: scheduling.CleanupExpiredFinancialRecords, }) s.Register(Job{ Name: "cleanup-expired-gift-cards", Schedule: "0 5 * * *", // 5am Timeout: 5 * time.Minute, Concurrency: 1, Handler: scheduling.CleanupExpiredGiftCards, }) s.Register(Job{ Name: "cleanup-idle-accounts", Schedule: "30 3 * * *", // 3:30am (offset from financial) Timeout: 5 * time.Minute, Concurrency: 1, Handler: scheduling.CleanupIdleAccounts, }) s.Register(Job{ Name: "cleanup-old-name-history", Schedule: "30 4 * * *", // 4:30am Timeout: 30 * time.Second, Concurrency: 1, Handler: scheduling.CleanupOldNameHistory, }) } ``` **Staggering rationale:** Daily jobs at 3am/3:30am/4am/4:30am/5am spread the load so heavy operations (financial aggregation, idle account scans) don't contend with each other or with the hourly jobs. High-frequency jobs (reservations, deposits) are decoupled on their own 5-min schedules. --- ## Phase 2: Wire into `main.go` ### Before: ```go // main.go:142 auth.StartJTICleanup() // main.go:432-450 (background goroutine for reservations) cleanupCtx, cleanupStop := context.WithCancel(context.Background()) go func() { ticker := time.NewTicker(reservationCleanupInterval) ... }() // main.go:457-468 (shutdown) cleanupStop() ``` ### After: ```go // At package level, after constants but before init() var sched *jobs.Scheduler // In main(), after initDB()/initSquare()/etc but before r := chi.NewRouter(): sched = jobs.New() jobs.RegisterAll(sched) sched.Start() // In main(), shutdown block (replace the old cleanupStop() call): quit := make(chan os.Signal, 1) signal.Notify(quit, syscall.SIGTERM, syscall.SIGINT) go func() { <-quit log.Println("Shutting down server...") // Graceful HTTP shutdown ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() if err := srv.Shutdown(ctx); err != nil { log.Printf("Server forced to shutdown: %v", err) } // Graceful job scheduler shutdown <-sched.Shutdown() log.Println("Background jobs stopped") }() ``` **Remove:** - `reservationCleanupInterval` and `reservationCleanupTimeout` constants (`main.go:71-72`) - `cleanupCtx`/`cleanupStop` variables (`main.go:433`) - The goroutine block (`main.go:434-450`) - Import `scheduling` from `main.go` (no longer needed there) - `auth.StartJTICleanup()` call (`main.go:142`) --- ## Phase 3: Remove Side-Effects from `GetAvailableHours` **File: `backend/handlers/scheduling/default-hours.go`** Delete lines 364-407 (the entire cleanup block): ```diff - // Clean up old reservations (older than 1 hour) - if err := CleanupOldReservations(r.Context()); err != nil { - log.Printf("Failed to cleanup old reservations: %v", err) - } - - // Anonymize stale guest accounts (6+ months after last booking) - if err := AnonymizeStaleGuestAccounts(r.Context()); err != nil { - log.Printf("Failed to anonymize stale guest accounts: %v", err) - } - - // Clean up expired loyalty redemptions (pending past expires_at) - if err := CleanupExpiredLoyaltyRedemptions(r.Context()); err != nil { - log.Printf("Failed to cleanup expired loyalty redemptions: %v", err) - } - - // Clean up expired financial records (aggregate + delete granular data) - if err := CleanupExpiredFinancialRecords(r.Context()); err != nil { - log.Printf("Failed to cleanup expired financial records: %v", err) - } - - // Clean up bookings past deposit deadline (no deposit paid) - if err := CleanupExpiredDeposits(r.Context()); err != nil { - log.Printf("Failed to cleanup expired deposits: %v", err) - } - - // Clean up expired gift cards (unused for 24+ months) - if err := CleanupExpiredGiftCards(r.Context()); err != nil { - log.Printf("Failed to cleanup expired gift cards: %v", err) - } - - // Clean up idle accounts (2yr no money, 5yr with money) - if err := CleanupIdleAccounts(r.Context()); err != nil { - log.Printf("Failed to cleanup idle accounts: %v", err) - } - - // Clean up old idempotency keys (24h+ and non-pending) - if err := CleanupOldIdempotencyKeys(r.Context()); err != nil { - log.Printf("Failed to cleanup old idempotency keys: %v", err) - } - - // Clean up old name history (6+ months) - if err := CleanupOldNameHistory(r.Context()); err != nil { - log.Printf("Failed to cleanup old name history: %v", err) - } ``` Also remove `"log"` import from `default-hours.go` if it becomes unused after removal. --- ## Phase 4: Remove `auth.StartJTICleanup()` (Optional but Recommended) **File: `backend/auth/jwt.go`** - Keep `CleanupRevokedJTIs(ctx)` — it's used by tests and now by the jobs package - Delete `StartJTICleanup()` entirely (no longer needed, replaced by the jobs package) - Remove the ticker goroutine --- ## Phase 5: Update Documentation **File: `obsidian/Crussell/Technical Manual.md`** - Update lines ~545 and ~999: change "lazy cleanup triggered on availability fetch" to "all cleanup runs on cron schedules via the `jobs` package" - Remove the lazy-cleanup justification - Add a new section documenting the `jobs` package and its job registry **File: `obsidian/Crussell/Future Work - Gap Backlog.md`** - Mark item #2 as fully complete (strikethrough) - Remove the note about remaining functions still running on `GET /api/availability` **File: `README.md`** (optional) - Add a line about the background job scheduler --- ## Effort Estimate | Step | Files Changed | Effort | |------|--------------|--------| | Phase 1a: `scheduler.go` | 1 new file | 2-3h | | Phase 1b: `cleanup.go` | 1 new file | 1h | | Phase 2: Wire `main.go` | 1 file | 30min | | Phase 3: Remove side-effects from handler | 1 file | 15min | | Phase 4: Remove `StartJTICleanup` | 1 file | 15min | | Phase 5: Update docs | 2-3 files | 30min | | Testing & go vet | — | 1h | | **Total** | **4-5 files (+2 new)** | **~6h** | --- ## Risks & Mitigations | Risk | Mitigation | |------|------------| | **Concurrent DB load** from multiple jobs at the same cron tick | Stagger daily jobs across 3am-5am range. High-frequency jobs (5min) are lightweight. Per-job `Concurrency: 1` prevents overlapping runs. | | **Job takes longer than interval** (e.g., financial aggregation >5min while scheduled every 5min) | `Concurrency: 1` + skip-logic: if previous run still in-flight, the new invocation is skipped and logged. | | **Cron expression parsing differs** from existing time-blocker parser | Both use `cron.NewParser(cron.Minute \| cron.Hour \| cron.Dom \| cron.Month \| cron.Dow)` — identical. | | **Tests expect cleanup side-effects** from `GetAvailableHours` | Search for tests that rely on the side-effect calls. Some may need an explicit cleanup call before assertions. | | **`CleanupExpiredDeposits` timing gap** — deposit-lapsed slots not freed until next cron tick | 5-min frequency is acceptable. The previous behavior freed on the *next* availability fetch, which could be minutes or hours apart depending on user activity. 5-min max latency is actually *better* than the lazy approach during quiet periods. | | **Panic in one job takes down the scheduler** | `wrapJob` has defer-recover. One job's panic cannot affect others (separate goroutines). | --- ## Edge Cases & Exclusions **Not in scope (Phase 1):** - `gdpr_export.go:init()` — GDPR cache cleanup (5min ticker). Tightly coupled to package-level state. Leave as-is. - `auth/local.go:init()` — Login state cleanup. Tightly coupled. Leave as-is. - `mw/ratelimit.go` goroutines — Rate limiter cleanup. Leave as-is. - These can be migrated to the scheduler in a follow-up if desired, but require more refactoring. **Not a concern:** - `backend/handlers/scheduling/time-blockers.go` already uses `robfig/cron/v3` — no new dependency, no version conflict. - All 9 cleanup functions have signature `func(context.Context) error` — perfectly uniform for the `Job.Handler` type. - The `scheduling` package is already imported in `main.go` — no new import needed for that. **Key invariant:** All cleanup functions are idempotent (documented in code). Running them on cron instead of on-demand has zero correctness impact — they produce the same result regardless of how often they run.