Files
zero/internal/cron/store.go
PierrunoYTandClaude Sonnet 5 d9b882eb0a fix(cron): reserve job IDs atomically (#686)
* fix(cron): reserve job IDs atomically

* fix(cron): hold the per-ID lock from reservation through persistence

Address code review on PR #686: reserveID made the timestamp directory
visible via os.Mkdir before metadata.json existed, but Remove (which
takes the per-ID lock before deleting) never coordinated with that
window. A concurrent `zero cron rm <id>` could delete the bare
reservation and report success right after Add reserved it; writeJob's
MkdirAll would then silently resurrect the directory and persist the
job anyway. A second Add could also re-reserve the same id while the
first writer was still running, putting two writers on the same
metadata path.

reserveID now acquires the same per-ID lock (used by Get/Update/
Mutate/Remove/AppendRun) for each candidate id before attempting
os.Mkdir, and returns it still held; Add releases it only after
writeReservedJob commits (or tears down) the reservation. A concurrent
Remove for that id now blocks until the reservation is resolved one
way or the other, and a concurrent Add attempting the same candidate
id serializes through the lock rather than only racing the Mkdir.

Adds two regression tests: one proving Remove waits for and removes
the committed job rather than the bare reservation, and one proving a
second reserveID call for the same candidate id blocks on the shared
lock and moves on to the next candidate.

Co-Authored-By: Claude Sonnet 5 <[email protected]>

---------

Co-authored-by: Claude Sonnet 5 <[email protected]>
2026-07-22 19:28:04 +08:00

366 lines
10 KiB
Go

package cron
import (
"bufio"
"encoding/json"
"errors"
"fmt"
"os"
"path/filepath"
"strings"
"time"
"github.com/Gitlawb/zero/internal/fsutil"
)
const (
StatusActive = "active"
StatusPaused = "paused"
)
// ErrJobNotFound is returned (wrapped) by Get when a job's metadata file is
// absent — a genuinely removed job. Callers use errors.Is to distinguish this
// from a transient read failure (IO error, permission), which must NOT be
// treated as "job removed".
var ErrJobNotFound = errors.New("cron job not found")
// Job is a stored scheduled job.
type Job struct {
ID string `json:"id"`
Expr string `json:"expr"`
Prompt string `json:"prompt"`
Cwd string `json:"cwd,omitempty"`
Model string `json:"model,omitempty"`
Status string `json:"status"`
FireCount int `json:"fireCount"`
NextRunAt time.Time `json:"nextRunAt"`
CreatedAt time.Time `json:"createdAt"`
}
// RunRecord is one fire's outcome, appended to the job's runs.jsonl.
type RunRecord struct {
JobID string `json:"jobId"`
At time.Time `json:"at"`
ExitCode int `json:"exitCode"`
SessionTitle string `json:"sessionTitle,omitempty"`
Error string `json:"error,omitempty"`
}
// StoreOptions configures a Store. RootDir defaults to DefaultRoot(os env); Now
// defaults to time.Now. Both injectable for tests.
type StoreOptions struct {
RootDir string
Now func() time.Time
}
type Store struct {
root string
now func() time.Time
}
func NewStore(opts StoreOptions) *Store {
root := opts.RootDir
if root == "" {
root = DefaultRoot(envMap())
}
now := opts.Now
if now == nil {
now = time.Now
}
return &Store{root: root, now: now}
}
// DefaultRoot mirrors sessions.DefaultRoot: <XDG_DATA_HOME|~/.local/share>/zero/cron.
func DefaultRoot(env map[string]string) string {
dataHome := strings.TrimSpace(env["XDG_DATA_HOME"])
home := strings.TrimSpace(env["HOME"])
if home == "" {
// Mirror sessions.DefaultRoot: fall back to the OS user home so an unset
// HOME (Windows, restricted shells) doesn't yield a RELATIVE ".local/share"
// under the caller's cwd, which would scatter cron data per working dir.
if userHome, err := os.UserHomeDir(); err == nil {
home = userHome
}
}
base := dataHome
if base == "" {
base = filepath.Join(home, ".local", "share")
}
return filepath.Join(base, "zero", "cron")
}
func envMap() map[string]string {
return map[string]string{"XDG_DATA_HOME": os.Getenv("XDG_DATA_HOME"), "HOME": os.Getenv("HOME")}
}
// Add assigns an ID + CreatedAt and writes the job's metadata.json.
func (s *Store) Add(job Job) (Job, error) {
if job.Status == "" {
job.Status = StatusActive
}
job.CreatedAt = s.now().UTC()
id, unlock, err := s.reserveID()
if err != nil {
return Job{}, err
}
// Held from reservation through initial persistence: without this, Remove
// (which takes the same per-ID lock before deleting) could delete the
// bare reservation directory the instant after Mkdir and report success,
// then writeJob's MkdirAll would silently resurrect it and persist the
// job anyway — or a second Add could re-reserve the freed id while this
// writer is still running, putting two writers on the same metadata path.
defer unlock()
job.ID = id
if err := s.writeReservedJob(job); err != nil {
return Job{}, err
}
return job, nil
}
// reserveID atomically reserves an unused job id (by os.Mkdir-ing its
// directory) and returns the per-ID lock held for that id, still acquired.
// The caller must release it (via the returned unlock) only once the
// reservation has been committed (metadata.json written) or torn down —
// see Add.
func (s *Store) reserveID() (string, func(), error) {
if err := os.MkdirAll(s.root, 0o700); err != nil {
return "", nil, fmt.Errorf("create cron store root: %w", err)
}
base := s.now().UTC().Format("20060102-150405")
for n := 0; n < 100; n++ {
id := base
if n > 0 {
id = fmt.Sprintf("%s-%d", base, n)
}
unlock, err := s.lockJob(id)
if err != nil {
return "", nil, err
}
mkErr := os.Mkdir(s.jobDir(id), 0o700)
if mkErr == nil {
return id, unlock, nil
}
unlock()
if errors.Is(mkErr, os.ErrExist) {
continue
}
return "", nil, fmt.Errorf("reserve cron job id %q: %w", id, mkErr)
}
return "", nil, errors.New("could not allocate a unique cron job id")
}
func (s *Store) jobDir(id string) string { return filepath.Join(s.root, id) }
// validID rejects ids that could escape the store root (path separators or
// traversal). reserveID-generated timestamp ids always pass; this guards
// externally-supplied ids (get/update/remove/append).
func validID(id string) bool {
return id != "" && id != "." && id != ".." && !strings.ContainsAny(id, `/\`) && filepath.Base(id) == id
}
func (s *Store) writeJob(job Job) error {
dir := s.jobDir(job.ID)
if err := os.MkdirAll(dir, 0o700); err != nil {
return err
}
data, err := json.MarshalIndent(job, "", " ")
if err != nil {
return err
}
tmp := filepath.Join(dir, "metadata.json.tmp")
if err := os.WriteFile(tmp, data, 0o600); err != nil {
return err
}
if err := fsutil.RenameWithRetry(tmp, filepath.Join(dir, "metadata.json"), nil); err != nil {
_ = os.Remove(tmp)
return err
}
return nil
}
func (s *Store) writeReservedJob(job Job) error {
if err := s.writeJob(job); err != nil {
if cleanupErr := os.RemoveAll(s.jobDir(job.ID)); cleanupErr != nil {
return errors.Join(err, fmt.Errorf("remove failed cron job reservation: %w", cleanupErr))
}
return err
}
return nil
}
func (s *Store) Get(id string) (Job, error) {
if !validID(id) {
return Job{}, fmt.Errorf("invalid cron job id %q", id)
}
data, err := os.ReadFile(filepath.Join(s.jobDir(id), "metadata.json"))
if err != nil {
if errors.Is(err, os.ErrNotExist) {
return Job{}, fmt.Errorf("cron job %q: %w", id, ErrJobNotFound)
}
// A transient read failure (IO error, permission) is NOT a missing job;
// surface it as a distinct error so callers don't mistake it for removal.
return Job{}, fmt.Errorf("read cron job %q: %w", id, err)
}
var job Job
if err := json.Unmarshal(data, &job); err != nil {
return Job{}, fmt.Errorf("cron job %q is corrupt: %w", id, err)
}
return job, nil
}
func (s *Store) Update(job Job) error {
if !validID(job.ID) {
return fmt.Errorf("invalid cron job id %q", job.ID)
}
unlock, err := s.lockJob(job.ID)
if err != nil {
return err
}
defer unlock()
if _, err := os.Stat(s.jobDir(job.ID)); err != nil {
return fmt.Errorf("cron job %q not found", job.ID)
}
return s.writeJob(job)
}
// Mutate atomically updates job id under the per-job cross-process lock, closing
// the read-modify-write race between concurrent schedulers (and against Update/
// Remove). It re-reads the CURRENT on-disk job and passes it to mutate along with
// any read error; mutate returns the job to persist. A removed job aborts with
// ErrJobNotFound (no recreate). A transient read error (not removal) is surfaced
// via readErr so the caller can still persist a best-effort state — the fire path
// advances the schedule regardless, to avoid a re-fire.
func (s *Store) Mutate(id string, mutate func(current Job, readErr error) (Job, error)) (Job, error) {
if !validID(id) {
return Job{}, fmt.Errorf("invalid cron job id %q", id)
}
unlock, err := s.lockJob(id)
if err != nil {
return Job{}, err
}
defer unlock()
current, readErr := s.Get(id)
if errors.Is(readErr, ErrJobNotFound) {
return Job{}, ErrJobNotFound
}
next, err := mutate(current, readErr)
if err != nil {
return Job{}, err
}
if err := s.writeJob(next); err != nil {
return Job{}, err
}
return next, nil
}
func (s *Store) Remove(id string) error {
if !validID(id) {
return fmt.Errorf("invalid cron job id %q", id)
}
unlock, err := s.lockJob(id)
if err != nil {
return err
}
defer unlock()
if _, err := os.Stat(s.jobDir(id)); err != nil {
return fmt.Errorf("cron job %q not found", id)
}
return os.RemoveAll(s.jobDir(id))
}
func (s *Store) List() ([]Job, error) {
entries, err := os.ReadDir(s.root)
if errors.Is(err, os.ErrNotExist) {
return nil, nil
}
if err != nil {
return nil, err
}
var jobs []Job
var corrupt []string
for _, e := range entries {
if !e.IsDir() {
continue
}
if _, statErr := os.Stat(filepath.Join(s.jobDir(e.Name()), "metadata.json")); statErr != nil {
continue // a directory without metadata.json is not a job
}
job, gerr := s.Get(e.Name())
if gerr != nil {
corrupt = append(corrupt, e.Name())
continue
}
jobs = append(jobs, job)
}
// Surface corrupt jobs (jobs slice is still authoritative; callers treat this
// as a warning, not a fatal error, so one bad job never hides the rest).
if len(corrupt) > 0 {
return jobs, fmt.Errorf("skipped %d unreadable cron job(s): %s", len(corrupt), strings.Join(corrupt, ", "))
}
return jobs, nil
}
func (s *Store) AppendRun(id string, rec RunRecord) error {
if !validID(id) {
return fmt.Errorf("invalid cron job id %q", id)
}
unlock, err := s.lockJob(id)
if err != nil {
return err
}
defer unlock()
// Bail if the job was removed (e.g. mid-run) — otherwise the MkdirAll below
// would resurrect a deleted job's directory with an orphaned runs.jsonl and no
// metadata.json (which Runs would then still return). Under the per-job lock
// this stat-then-write is race-free against a concurrent Remove.
if _, err := os.Stat(filepath.Join(s.jobDir(id), "metadata.json")); err != nil {
if errors.Is(err, os.ErrNotExist) {
return nil
}
return err
}
dir := s.jobDir(id)
if err := os.MkdirAll(dir, 0o700); err != nil {
return err
}
f, err := os.OpenFile(filepath.Join(dir, "runs.jsonl"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600)
if err != nil {
return err
}
line, err := json.Marshal(rec)
if err != nil {
_ = f.Close()
return err
}
if _, err := f.Write(append(line, '\n')); err != nil {
_ = f.Close()
return err
}
// Surface a buffered-write failure that only materializes on Close.
return f.Close()
}
func (s *Store) Runs(id string) ([]RunRecord, error) {
if !validID(id) {
return nil, fmt.Errorf("invalid cron job id %q", id)
}
f, err := os.Open(filepath.Join(s.jobDir(id), "runs.jsonl"))
if errors.Is(err, os.ErrNotExist) {
return nil, nil
}
if err != nil {
return nil, err
}
defer f.Close()
var runs []RunRecord
scanner := bufio.NewScanner(f)
scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024)
for scanner.Scan() {
var rec RunRecord
if json.Unmarshal(scanner.Bytes(), &rec) == nil {
runs = append(runs, rec)
}
}
return runs, scanner.Err()
}