mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-11 23:40:28 -10:00
* audit fixes: cron lock, accounting race, O(n^2) hot paths, /style wiring
Fixes five verified findings from a code audit. Two related findings were already
addressed upstream and three need larger/separate work (noted below).
H-1 cron store: cross-process lock for the read-modify-write.
Two schedulers seeing a job due raced the metadata read-modify-write; fireJob
even documented it ("full atomicity needs file locking"). Adds a per-job O_EXCL
lock (mirroring internal/oauth's pattern, stale-reclaim included) and a Mutate
primitive doing the re-read+modify+write atomically. Update/Remove lock too. The
lock is held only for the millisecond RMW, never across a job's exec.
M-5 specialist accounting: close the check-then-append race.
specialistEventExists (unlocked ReadEvents) + a later AppendEvent let two
finishers (TaskOutput poll vs onExit, even cross-process) both pass the check and
write duplicate stop/usage events. Adds sessions.Store.AppendEventUnlessExists
(check + append atomic under the session lock) and routes the stop and usage
paths through it.
M-2 hooks audit: O(1) sequence via tail read.
append re-read and scanned the whole audit log every call to find the max
sequence (O(n^2) over a session). Reads only the file tail now; multi-process
safe (the log is a shared global file), unlike an in-memory counter which would
collide sequences across processes.
M-4 transcript rehydration: O(n^2) -> O(n).
The three resume/rewind/compact rehydration loops called appendTranscriptRow per
event, each re-scanning every existing row for the keyed-row dedup. A bulk
appendTranscriptRowsDedup builds the key set once; semantics are identical.
M-6 /style: actually shape responses.
The TUI /style command stored a style but never threaded it anywhere. Adds
agent.Options.ResponseStyle, a system-prompt directive for concise/explanatory/
review, and wires m.responseStyle through the run. "balanced" is a no-op (prompt
byte-identical to before).
Already addressed upstream (no change):
- M-1 forked-session double-count: Store.Fork already skips EventUsage events,
with a comment naming the exact double-count it prevents.
- M-3 lineage O(N^2): Tree already does a single store.List() scan and indexes
children in memory; no path calls List per node.
Deferred (need larger/separate changes):
- M-7 escalate_model in the TUI needs a ModelSwitcher that builds a provider
mid-run; registering the tool alone yields a tool that errors on call.
- L-1 MCP SSE reconnect is a stream-lifecycle change (low-likelihood after the
8 MiB per-event cap).
- L-2 dead-code removal belongs in a dedicated cleanup sweep.
Tests: per-fix unit tests including concurrency (cron Mutate, sessions
AppendEventUnlessExists) under -race; full go test ./... green.
* fix(windows): treat ERROR_ACCESS_DENIED as lock contention (cron + oauth)
The Windows smoke run failed TestMutateIsAtomicUnderConcurrency with
"...lock: Access is denied" (FireCount 19/20). On Windows a concurrent holder's
os.Remove leaves the lock file in a delete-pending state, so an O_EXCL create
races it with ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist — the
retry loop only treated ErrExist as contention and hard-failed.
Treat os.ErrPermission as contention (retry) in the new cron lock and in the
oauth lock it mirrors (oauth's own concurrent test exposed the same pre-existing
bug under the added load). Unix is unaffected — it returns ErrExist.
* fix: address review — cross-process atomic audit append + AppendRun guard
Addresses the review feedback on this PR.
P1 (blocking): AuditStore.append is now cross-process atomic.
The O(1) tail read observes another process's prior appends, but the read-then-
append was not atomic across processes — two could both read sequence N and write
N+1. Wraps lastSequence()+append in a per-file O_EXCL lock (hooks/lock.go, same
stale-reclaim + Windows ErrPermission handling as cron/oauth); store.mu stays as
the in-process fast path. Adds TestAuditStoreSequenceConcurrent (4 stores x 12
goroutines on one file -> unique dense sequences), which the prior sequential
test could not catch. Walked back the comment that oversold the tail read.
P2: AppendRun no longer resurrects a removed job.
fireJob's AppendRun ran after Mutate released the per-job lock; a Remove in that
window let AppendRun's MkdirAll recreate the dir with an orphaned runs.jsonl.
AppendRun now takes the per-job lock and bails if metadata.json is gone. Adds
TestAppendRunDoesNotResurrectRemovedJob.
P2: lastSequence ReadAt tolerates a short final read.
ReadAt may return io.EOF with n < len(buf) if the file shrank between Stat and
the read; treat EOF as non-fatal and use buf[:n] (matches lastEventSequence).
P3: oauth stale-reclaim no-op removed.
The double os.ReadFile "stale == data" guard compared the file to itself read
twice in a row (always true); staleness is decided by the mtime check, so the
guard is dropped (behavior-preserving).
Tests under -race; cron/hooks/oauth green. A shared filelock helper would DRY the
three near-identical locks — left as a follow-up to avoid churning the
already-green cron/oauth packages here.
325 lines
8.9 KiB
Go
325 lines
8.9 KiB
Go
package cron
|
|
|
|
import (
|
|
"bufio"
|
|
"encoding/json"
|
|
"errors"
|
|
"fmt"
|
|
"os"
|
|
"path/filepath"
|
|
"strings"
|
|
"time"
|
|
)
|
|
|
|
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, err := s.allocID()
|
|
if err != nil {
|
|
return Job{}, err
|
|
}
|
|
job.ID = id
|
|
if err := s.writeJob(job); err != nil {
|
|
return Job{}, err
|
|
}
|
|
return job, nil
|
|
}
|
|
|
|
func (s *Store) allocID() (string, error) {
|
|
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)
|
|
}
|
|
if _, err := os.Stat(filepath.Join(s.root, id)); errors.Is(err, os.ErrNotExist) {
|
|
return id, nil
|
|
}
|
|
}
|
|
return "", 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). allocID-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
|
|
}
|
|
return os.Rename(tmp, filepath.Join(dir, "metadata.json"))
|
|
}
|
|
|
|
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()
|
|
}
|