popertotsandSisyphus
510828c924
CI / Go vulnerabilities (push) Successful in 1m10s
CI / Build & Vet (push) Successful in 1m39s
CI / Frontend build (gate) (push) Successful in 1m42s
CI / Frontend QC (audit) (push) Successful in 56s
CI / Frontend QC (typecheck) (push) Successful in 1m36s
CI / Frontend QC (lint) (push) Successful in 1m51s
CI / Tests (prod) (push) Has been cancelled
CI / Tests (dev) (push) Has been cancelled
CI / Race (prod) (push) Has been cancelled
CI / Race (dev) (push) Has been cancelled
106 files: interface{}→any, strings.Split→SplitSeq, CutPrefix/Cut, strings.Builder, slices.Contains, remove redundant // +build directives, gofmt import ordering and indentation.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
432 lines
15 KiB
Markdown
432 lines
15 KiB
Markdown
# 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.
|