P6 review: runner shutdown, cancel-while-busy, concurrent submits, batch-off 503, transactional store, CLI 202

Owner review of p6 (Ornith tasks 01-09), with an independent reviewer pass whose findings I
confirmed against the code. Every fix has a regression test that fails on the old code.

internal/batch
- Close let the workers keep draining every live job under the cancelled context, failing
  every target; they now stop at once, store nothing on a failure during shutdown, and leave
  the job live for MarkInFlightFailed("restart").
- A job cancelled while its in-flight target came back busy stayed in the rotation forever
  (depth gauge stuck, job held in memory); it now leaves. The task 03 design had this gap: mine.
- Submit's idempotency lookup, caps and create ran unlocked: 20 concurrent submits by one
  caller made 8 jobs. Serialised with submitMu.
- A failed target insert left a queued job no worker would run (holding the caller's cap);
  it is marked failed.
- A second concurrent cancel returned true and counted twice.
- Config: zero OR negative values take the defaults (negative Workers panicked the
  WaitGroup; a negative TTL or byte budget would sweep or evict everything).
- Retry timing used the pre-fetch time (a 5 s busy answer made RetryWait moot).
- The rotation cursor is kept within [0, len(live)] (it was absolute and drifted on removal).
- The unused in-memory callers map is gone (Submit racing Close wrote to a nil map).
internal/dcache
- PutBatchTargets, SweepBatch and eviction are transactional; the sweep never removes a
  queued or running job.
internal/api
- Every /v1/batch endpoint answers 503 when batch is off; POST panicked on the nil runner.
- GET /{id}/{idx} bounds idx by the rows actually returned.
- batchFetch copies a *fetch.Error instead of mutating one the PDF pool may share.
internal/config
- ParseSize no longer overflows to a negative size.
cmd/sift
- sift batch treated 202 Accepted (submit) as an error and exited 1 with no message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/batch/batch.go

@@ -118,14 +118,13 @@ type Runner struct {
 	fetch     FetchFunc
 	log       *slog.Logger
 	cfg       Config
-	callers   map[string]*auth.Caller // job id → submitting caller, held in memory only
-	callMu    sync.Mutex
 	closeOnce sync.Once
+	submitMu  sync.Mutex // serialises Submit: idempotency lookup, caps and create are one step
 
 	mu       sync.Mutex // guards everything below
 	cond     *sync.Cond // on mu
 	live     []*liveJob // the rotation, in submit order
-	cursor   int        // absolute rotation position; wraps at access, so it survives jobs being added
+	cursor   int        // where the next scan starts, within [0, len(live)]; read as cursor % len(live)
 	closed   bool
 	ctx      context.Context // the FetchFunc context; Close cancels it
 	stop     context.CancelFunc
@@ -134,42 +133,43 @@ type Runner struct {
 }
 
 func New(db *dcache.DB, fetch FetchFunc, cfg Config, log *slog.Logger) *Runner {
-	if cfg.TTL == 0 {
+	// Zero or negative means the default: a negative TTL or byte budget would make the sweep or
+	// the eviction delete everything, and negative workers would panic the WaitGroup.
+	if cfg.TTL <= 0 {
 		cfg.TTL = 24 * time.Hour
 	}
-	if cfg.TargetsPerJob == 0 {
+	if cfg.TargetsPerJob <= 0 {
 		cfg.TargetsPerJob = 32
 	}
-	if cfg.JobsPerCaller == 0 {
+	if cfg.JobsPerCaller <= 0 {
 		cfg.JobsPerCaller = 1
 	}
-	if cfg.JobsTotal == 0 {
+	if cfg.JobsTotal <= 0 {
 		cfg.JobsTotal = 8
 	}
-	if cfg.Workers == 0 {
+	if cfg.Workers <= 0 {
 		cfg.Workers = 1
 	}
 	if cfg.Workers > 2 {
 		cfg.Workers = 2
 	}
-	if cfg.MaxBytes == 0 {
+	if cfg.MaxBytes <= 0 {
 		cfg.MaxBytes = 64 << 20
 	}
-	if cfg.RetryWait == 0 {
+	if cfg.RetryWait <= 0 {
 		cfg.RetryWait = 2 * time.Second
 	}
-	if cfg.RetryFor == 0 {
+	if cfg.RetryFor <= 0 {
 		cfg.RetryFor = 10 * time.Minute
 	}
 	ctx, stop := context.WithCancel(context.Background())
 	r := &Runner{
-		db:      db,
-		fetch:   fetch,
-		log:     log,
-		cfg:     cfg,
-		callers: make(map[string]*auth.Caller),
-		ctx:     ctx,
-		stop:    stop,
+		db:    db,
+		fetch: fetch,
+		log:   log,
+		cfg:   cfg,
+		ctx:   ctx,
+		stop:  stop,
 	}
 	r.cond = sync.NewCond(&r.mu)
 	r.wg.Add(cfg.Workers)
@@ -193,6 +193,11 @@ func (r *Runner) Submit(ctx context.Context, c *auth.Caller, callerClass string,
 	sum := sha256.Sum256([]byte(c.Name + "\x00" + string(raw)))
 	idemKey := hex.EncodeToString(sum[:])
 
+	// The lookup, the caps and the create must be one step, or concurrent submits all pass the
+	// checks before any of them is stored.
+	r.submitMu.Lock()
+	defer r.submitMu.Unlock()
+
 	now := time.Now().Unix()
 	if existing, ok := r.db.BatchJobByIDem(c.Name, idemKey, now, r.cfg.TTL); ok {
 		r.callJob("reused")
@@ -241,13 +246,11 @@ func (r *Runner) Submit(ctx context.Context, c *auth.Caller, callerClass string,
 	}
 	if err := r.db.PutBatchTargets(id, btargets); err != nil {
 		r.log.Warn("batch: store targets", "job", id, "err", err)
+		// Never leave a queued job that no worker will run: it would hold the caller's cap.
+		_ = r.db.SetBatchJobStatus(id, "failed", "store", now)
 		return "", false, fmt.Errorf("batch: store targets: %w", err)
 	}
 
-	r.callMu.Lock()
-	r.callers[id] = c
-	r.callMu.Unlock()
-
 	r.mu.Lock()
 	r.live = append(r.live, &liveJob{
 		id:      id,
@@ -278,6 +281,10 @@ func (r *Runner) Cancel(id string) bool {
 		}
 	}
 	if j != nil {
+		if j.cancelled { // a concurrent DELETE got here first
+			r.mu.Unlock()
+			return false
+		}
 		j.cancelled = true
 		if err := r.db.SetBatchJobStatus(id, "cancelled", "", time.Now().Unix()); err != nil {
 			r.log.Warn("batch: cancel job", "job", id, "err", err)
@@ -285,7 +292,6 @@ func (r *Runner) Cancel(id string) bool {
 		r.callJob("cancelled")
 		if !j.running {
 			r.removeLocked(j)
-			r.forgetCaller(id)
 			r.callDepth(len(r.live))
 		}
 		r.cond.Broadcast()
@@ -302,9 +308,6 @@ func (r *Runner) Cancel(id string) bool {
 		r.log.Warn("batch: cancel job", "job", id, "err", err)
 		return false
 	}
-	r.callMu.Lock()
-	delete(r.callers, id)
-	r.callMu.Unlock()
 	r.callJob("cancelled")
 	return true
 }
@@ -356,13 +359,10 @@ func (r *Runner) Touch(id string) {
 }
 
 // Close stops the workers, cancels their context, and waits for an in-flight fetch to return.
-// Safe to call more than once. The in-memory callers are dropped.
+// Safe to call more than once. Jobs still live stay queued or running in the store, for
+// MarkInFlightFailed("restart") on the next start.
 func (r *Runner) Close() {
 	r.closeOnce.Do(func() {
-		r.callMu.Lock()
-		r.callers = nil
-		r.callMu.Unlock()
-
 		r.mu.Lock()
 		r.closed = true
 		r.stop()

internal/batch/review_test.go

@@ -0,0 +1,177 @@
+package batch
+
+// P6 owner review: regression tests for the runner (findings from the independent review).
+
+import (
+	"context"
+	"fmt"
+	"sync"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"git.wntrmute.dev/kyle/sift/internal/auth"
+	"git.wntrmute.dev/kyle/sift/internal/dcache"
+)
+
+// A job cancelled while its in-flight target comes back busy leaves the rotation.
+func TestCancelWhileBusyLeavesRotation(t *testing.T) {
+	started := make(chan struct{}, 1)
+	release := make(chan struct{})
+	var depth atomic.Int64
+	fetch := func(context.Context, *auth.Caller, Target) (Result, error) {
+		select {
+		case started <- struct{}{}:
+		default:
+		}
+		<-release
+		return Result{}, fmt.Errorf("busy: %w", ErrRetryLater)
+	}
+	r := newRunner(t, fetch, Config{RetryWait: time.Millisecond, Hooks: Hooks{Depth: func(n int) { depth.Store(int64(n)) }}})
+	id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	<-started
+	if !r.Cancel(id) {
+		t.Fatal("cancel returned false")
+	}
+	close(release)
+	deadline := time.Now().Add(2 * time.Second)
+	for time.Now().Before(deadline) {
+		r.mu.Lock()
+		n := len(r.live)
+		r.mu.Unlock()
+		if n == 0 && depth.Load() == 0 {
+			return
+		}
+		time.Sleep(5 * time.Millisecond)
+	}
+	t.Fatalf("cancelled job still in the rotation (depth %d)", depth.Load())
+}
+
+// Close stops the workers: no further target is fetched, and the job is left live in the
+// store for MarkInFlightFailed("restart") to reap, not failed target by target.
+func TestCloseStopsDraining(t *testing.T) {
+	started := make(chan struct{}, 1)
+	var calls atomic.Int64
+	fetch := func(ctx context.Context, _ *auth.Caller, _ Target) (Result, error) {
+		calls.Add(1)
+		select {
+		case started <- struct{}{}:
+		default:
+		}
+		<-ctx.Done()
+		return Result{}, ctx.Err()
+	}
+	r := newRunner(t, fetch, Config{})
+	id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b", "c", "d"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	<-started
+	r.Close()
+	if n := calls.Load(); n != 1 {
+		t.Fatalf("%d fetches after Close, want only the one in flight", n)
+	}
+	j, ts, _ := r.Status(id)
+	if j.Status != "running" && j.Status != "queued" {
+		t.Fatalf("job %q after Close, want it left live for the restart reaper", j.Status)
+	}
+	for _, x := range ts {
+		if x.Status != "pending" {
+			t.Fatalf("target %d is %q after Close, want pending", x.Idx, x.Status)
+		}
+	}
+}
+
+// Concurrent submits respect the caps and idempotency: one job per caller, and identical
+// lists all get the same job.
+func TestConcurrentSubmits(t *testing.T) {
+	release := make(chan struct{})
+	defer close(release)
+	fetch := func(context.Context, *auth.Caller, Target) (Result, error) {
+		<-release
+		return Result{Content: "ok"}, nil
+	}
+	r := newRunner(t, fetch, Config{})
+	var wg sync.WaitGroup
+	var mu sync.Mutex
+	ids := map[string]bool{}
+	for i := 0; i < 20; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls(fmt.Sprintf("u%d", i)))
+			if err == nil {
+				mu.Lock()
+				ids[id] = true
+				mu.Unlock()
+			}
+		}(i)
+	}
+	wg.Wait()
+	if len(ids) != 1 {
+		t.Fatalf("20 concurrent submits by one caller (cap 1) created %d jobs", len(ids))
+	}
+	same := map[string]bool{}
+	for i := 0; i < 20; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			id, _, err := r.Submit(context.Background(), bob, dcache.ClassFleet, urls("same"))
+			if err == nil {
+				mu.Lock()
+				same[id] = true
+				mu.Unlock()
+			}
+		}()
+	}
+	wg.Wait()
+	if len(same) != 1 {
+		t.Fatalf("20 concurrent identical submits created %d jobs, want 1", len(same))
+	}
+}
+
+// A second cancel of a job with a target in flight returns false and counts once.
+func TestDoubleCancel(t *testing.T) {
+	started := make(chan struct{}, 1)
+	release := make(chan struct{})
+	var cancels atomic.Int64
+	fetch := func(context.Context, *auth.Caller, Target) (Result, error) {
+		select {
+		case started <- struct{}{}:
+		default:
+		}
+		<-release
+		return Result{Content: "ok"}, nil
+	}
+	r := newRunner(t, fetch, Config{Hooks: Hooks{Job: func(o string) {
+		if o == "cancelled" {
+			cancels.Add(1)
+		}
+	}}})
+	defer close(release)
+	id, _, _ := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b"))
+	<-started
+	if !r.Cancel(id) {
+		t.Fatal("first cancel returned false")
+	}
+	if r.Cancel(id) {
+		t.Fatal("second cancel returned true")
+	}
+	if n := cancels.Load(); n != 1 {
+		t.Fatalf("cancelled counted %d times", n)
+	}
+}
+
+// Negative or zero durations and sizes fall back to the defaults.
+func TestConfigDefaults(t *testing.T) {
+	r := newRunner(t, func(context.Context, *auth.Caller, Target) (Result, error) { return Result{}, nil },
+		Config{TTL: -time.Hour, MaxBytes: -1, RetryWait: -1, RetryFor: -1, TargetsPerJob: -1, JobsPerCaller: -1, JobsTotal: -1, Workers: -1})
+	c := r.cfg
+	if c.TTL != 24*time.Hour || c.MaxBytes != 64<<20 || c.RetryWait != 2*time.Second || c.RetryFor != 10*time.Minute ||
+		c.TargetsPerJob != 32 || c.JobsPerCaller != 1 || c.JobsTotal != 8 || c.Workers != 1 {
+		t.Fatalf("config %+v", c)
+	}
+}

internal/batch/worker.go

@@ -36,13 +36,13 @@ func (r *Runner) worker() {
 
 	for {
 		r.mu.Lock()
+		if r.closed { // stop taking targets at once; live jobs are reaped on the next start
+			r.mu.Unlock()
+			return
+		}
 		now := time.Now()
 		j := r.pick(now)
 		if j == nil {
-			if r.closed {
-				r.mu.Unlock()
-				return
-			}
 			r.scheduleRetryLocked(now)
 			r.cond.Wait() // re-locks on wake; release before the next top-of-loop Lock
 			r.mu.Unlock()
@@ -68,10 +68,28 @@ func (r *Runner) worker() {
 		res, err := r.runTarget(r.ctx, j, idx, target)
 
 		r.mu.Lock()
+		now = time.Now() // after the fetch: a busy answer can take seconds to come back
 		j.running = false
 		r.inflight--
 		r.callInflight(r.inflight)
 
+		// Shutting down: a failure now is most likely the cancelled context. Store nothing; the
+		// job stays live in the store for the restart reaper.
+		if r.closed && err != nil {
+			r.cond.Broadcast()
+			r.mu.Unlock()
+			continue
+		}
+
+		// A cancelled job whose in-flight target was only busy is finished: nothing more runs.
+		if errors.Is(err, ErrRetryLater) && j.cancelled {
+			r.removeLocked(j)
+			r.callDepth(len(r.live))
+			r.cond.Broadcast()
+			r.mu.Unlock()
+			continue
+		}
+
 		// A target that was only busy stays pending and is retried after RetryWait, unless it
 		// has been busy for longer than RetryFor, in which case it is stored failed below.
 		if errors.Is(err, ErrRetryLater) {
@@ -102,7 +120,6 @@ func (r *Runner) worker() {
 				r.setTerminal(j)
 			}
 			r.removeLocked(j)
-			r.forgetCaller(j.id)
 			r.callDepth(len(r.live))
 		}
 		r.cond.Broadcast()
@@ -124,7 +141,9 @@ func (r *Runner) pick(now time.Time) *liveJob {
 			continue
 		}
 		if !now.Before(j.retryAt) { // now >= retryAt; a zero retryAt is always ready
-			r.cursor += i + 1 // absolute; wraps at access, so the rotation survives jobs being added
+			// The job after this one; n (one past the end) means "whatever is appended next,
+			// else wrap to 0" — reads take cursor % n.
+			r.cursor = (r.cursor+i)%n + 1
 			return j
 		}
 	}
@@ -168,6 +187,9 @@ func (r *Runner) removeLocked(j *liveJob) {
 	if r.cursor > idx {
 		r.cursor--
 	}
+	if r.cursor > len(r.live) {
+		r.cursor = len(r.live)
+	}
 }
 
 // setTerminal writes the job's finished status once: all done → done, all failed → failed, a mix
@@ -239,13 +261,6 @@ func (r *Runner) storeTarget(j *liveJob, idx int, res Result, err error) int {
 	return 0
 }
 
-// forgetCaller drops the in-memory caller for a finished job.
-func (r *Runner) forgetCaller(id string) {
-	r.callMu.Lock()
-	delete(r.callers, id)
-	r.callMu.Unlock()
-}
-
 func (r *Runner) callDepth(live int) {
 	if r.cfg.Hooks.Depth != nil {
 		r.cfg.Hooks.Depth(live)