From 2529cb8e8433870935913e60b9bb1a1d6922dc8c Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 20:38:42 -0700 Subject: [PATCH 01/10] dcache: batch job and target tables with a sliding lease, sweep and byte budget Add the batch job store to internal/dcache (storage only; no HTTP, no worker). New internal/dcache/batch.go holds the BatchJob/BatchTarget types and all accessors; Open gains the batch_job/batch_target tables and indices, and DB gains BatchH/BatchM counters. Sliding lease: writes (UpdateBatchTarget, SetBatchJobStatus, TouchBatchJob, MarkInFlightFailed) bump the job lease to now; reads never write. BatchJobByIDem returns the newest (caller, idem_key) job with lease+ttl>now and status queued/running/done/partial. UpdateBatchTarget counts targets_done exactly once per target entering done/failed and adds len(result) to nbytes. SweepBatch removes by lease; EvictBatchOldest evicts whole non-live jobs oldest-lease-first; MarkInFlightFailed reconciles in-flight jobs on restart. Batch nbytes are kept out of Size() and the content evict union. Co-Authored-By: ornith-1.5-35b-a3b --- docs/implementer-log.md | 1 + internal/dcache/batch.go | 314 ++++++++++++++++++++++++++++++++++ internal/dcache/batch_test.go | 254 +++++++++++++++++++++++++++ internal/dcache/dcache.go | 28 +++ 4 files changed, 597 insertions(+) create mode 100644 internal/dcache/batch.go create mode 100644 internal/dcache/batch_test.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 2ac42a7..16658ac 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -17,6 +17,7 @@ One row per task session that ends in a commit. Newest last. | p5b/03-api | 2026-09-26 | done | 1 | gate: ok | none | The digest now answers long documents itself (task 02), so the API's refusal limit is only a safety cap: `DefaultPDFMaxQuestionChunks` 200→5000 in `internal/api/pdf.go` (the 422 message text unchanged, still "document too long for question (N chunks); use grep or pages"). In `v1fetch` (`internal/api/api.go`) the digest-meta map gains `"retrieval": dr.Retrieval` and `"embedded": dr.Embedded` next to `chunks_used`/`chunks_total`, only when the digest ran without a fallback. Copied the two given tests byte-identical from the plan — the replacement `pdf_test.go` is now the protected one and asserts `DefaultPDFMaxQuestionChunks == 5000`; `pdf_retrieval_test.go` asserts the first long-doc question returns `retrieval=bm25`, `embedded=0`. README PDF section: a long-doc question is answered first from BM25 (`retrieval: "bm25"`) while its chunks embed in the background and hybrid on later questions, plus the 5000-chunk cap. One sentence added to `skill/SKILL.md`. All api tests pass under `-race -count=3`; make gate ok; given files byte-identical. | | p5c/01-extract-api | 2026-09-26 | done | 1 | gate: ok | none | `POST /v1/extract` answers a PDF posted in the body like `/v1/fetch` answers the same PDF by URL, nothing fetched. `internal/api/extract.go`: auth via `caller`; `io.ReadAll` of the body (a `*http.MaxBytesError`→413 "too large (>N MB)", empty→400); `clean.Kind` must be "pdf" else 415; query `pages`/`grep`/`question`/`mode` (digest when a question is set, else clean; other→400). Content-addressed cache: `hash="sha256:"+HashBody(body)`, `key=PDFKey("upload:"+hash, callerClass)`; a `GetPDF` hit rebuilds the doc via `pdfFromEntry` with `cached:true` + `PDFCacheHit` and no child (bypasses the TTL/revalidation that `pdfFromCache` does, so identical content is extracted once); a miss builds a synthetic `fetch.Resource{URL:"upload",...}` and runs `extractPDF`+`storePDF`. Bad pages/grep→400 via `writeFetchErr`, the chunk limit→the same 422 as `v1fetch`, extraction errors→`pdfError` via `writeFetchErr`. Envelope mirrors the PDF fetch path (`content`/`mode`/`untrusted`/`warnings`/`sizes`/`timings`/`pdf`/`digest`); `source` is `{"kind":"upload","hash","bytes","cached","fetched_at"}` with no `url` key; no Honcho context for uploads; one log line "extract" with caller/bytes/pdf_pages/mode/cached. `internal/api/api.go` scoped to the rule-1 changes: `DefaultPDFMaxUpload` const (100 MiB), `PDFMaxUpload int64` Server field, the `POST /v1/extract` route in `New`, and `ServeHTTP` (per-path upload cap instead of the 64 KiB JSON limit, and `/v1/extract` added to the `requestDeadline` switch). The `pdfMaxUpload()` field-or-default resolver lives in extract.go. Given test byte-identical; all api tests pass under `-race -count=3`; make gate ok. | | p5c/02-extract-cli | 2026-09-26 | done | 1 | gate: ok | none | `sift extract ` uploads a local PDF to `/v1/extract` and prints the answer exactly as `sift fetch` does. `cmd/sift/extractcmd.go`: `extractOpts` + `parseExtractOpts` (reuses `splitAroundPositional` with `extractValueFlags` mode/pages/grep/question, one positional file, `--json`), `extractURL` builds `base+"/v1/extract"` with `url.Values` set only for the options that are set and `mode=digest` when a question is given and no mode — values are URL-encoded so a grep like `a b&c` survives round-trip; `extractMain` reuses `loadClient`, `stat`s the file and refuses >100 MiB before uploading (exit 3, matching the server's 413→exitFor), then `http.NewRequest` with the `*os.File` as the streaming body and `ContentLength` set (no second in-memory copy), `Content-Type: application/pdf` + bearer, and the same error/exitFor path as fetch; success prints `content` (or the `--json` envelope). `main.go`: `case "extract"` and one help line. README: extract line in the CLI block plus a sentence; SKILL.md: a PDF-on-disk bullet in the PDFs section. Given test byte-identical; all cmd/sift tests pass under `-race -count=3`; make gate ok; `bin/sift extract` prints the usage line. | +| p6/01-batchstore | 2026-09-26 | done | 1 | gate: ok | none | Storage-only batch store in `internal/dcache` (new `batch.go`; schema added to `Open`; `BatchH`/`BatchM` counters added to `DB`). `CreateBatchJob` stores as given. Reads never write the lease: `BatchJob`, `BatchTargets` and the counts are pure, so exact lease values hold after a read. `BatchJobByIDem` returns the newest `(caller,idemKey)` job with `lease+ttl>now` and status queued/running/done/partial; failed/cancelled never reused. `PutBatchTargets` inserts in order; `BatchTargets` returns idx order with NULLs as nil pointers. `UpdateBatchTarget` bumps lease to `now`, increments `targets_done` exactly once per target entering done/failed (previous-status check prevents re-write double-count), and adds `len(result)` to `nbytes` when a result is written — all in one tx. `SetBatchJobStatus`/`TouchBatchJob` set lease; `SweepBatch` deletes by `lease+ttl<=now` with its targets and returns the job count; `EvictBatchOldest` evicts whole non-live jobs oldest-lease-first (never a queued/running one) until `BatchNbytes()<=maxBytes`; `MarkInFlightFailed` fails queued/running on restart; `BatchNbytes` sums job nbytes, kept out of `Size()`. Accessors return `(v,bool)`/`error`, count misses in `BatchM`, reuse `ClassFleet`/`ClassRestricted`. A first pass failed `TestBatchIDemLookup` because I forgot `.Scan(&id)` on the `QueryRow` (the un-scanned `*sql.Row` was always non-nil, so the method always missed); fixed and verified. All dcache tests pass under `-race -count=3`; make gate ok; given file byte-identical. | ## P5a review (owner, 2026-09-26) diff --git a/internal/dcache/batch.go b/internal/dcache/batch.go new file mode 100644 index 0000000..928113b --- /dev/null +++ b/internal/dcache/batch.go @@ -0,0 +1,314 @@ +package dcache + +import ( + "database/sql" + "fmt" + "time" +) + +// terminal statuses mark a target as finished; a target already in one is not +// counted again on a re-write. +func isTerminal(status string) bool { + return status == "done" || status == "failed" +} + +type BatchJob struct { + ID string + Caller string + CallerClass string + IDemKey string + Status string + Error string + TargetsTotal int + TargetsDone int + CreatedAt int64 + Lease int64 + NBytes int64 +} + +type BatchTarget struct { + JobID string + Idx int + URL string + Options string + Status string + Result *string + Error *string + Hint *string +} + +func nullString(s *string) sql.NullString { + if s == nil { + return sql.NullString{Valid: false} + } + return sql.NullString{String: *s, Valid: true} +} + +func nullPtr(n sql.NullString) *string { + if !n.Valid { + return nil + } + s := n.String + return &s +} + +func (d *DB) CreateBatchJob(j BatchJob) error { + if d == nil { + return fmt.Errorf("dcache: nil db") + } + _, err := d.sql.Exec(` + INSERT INTO batch_job + (id, caller, caller_class, idem_key, status, error, targets_total, targets_done, created_at, lease, nbytes) + VALUES(?,?,?,?,?,?,?,?,?,?,?) + `, j.ID, j.Caller, j.CallerClass, j.IDemKey, j.Status, j.Error, j.TargetsTotal, j.TargetsDone, j.CreatedAt, j.Lease, j.NBytes) + if err != nil { + return err + } + return nil +} + +func (d *DB) BatchJob(id string) (BatchJob, bool) { + if d == nil { + return BatchJob{}, false + } + var j BatchJob + err := d.sql.QueryRow(` + SELECT id, caller, caller_class, idem_key, status, error, + targets_total, targets_done, created_at, lease, nbytes + FROM batch_job WHERE id = ? + `, id).Scan(&j.ID, &j.Caller, &j.CallerClass, &j.IDemKey, &j.Status, &j.Error, + &j.TargetsTotal, &j.TargetsDone, &j.CreatedAt, &j.Lease, &j.NBytes) + if err != nil { + d.mu.Lock() + d.BatchM++ + d.mu.Unlock() + return BatchJob{}, false + } + d.mu.Lock() + d.BatchH++ + d.mu.Unlock() + return j, true +} + +// BatchJobByIDem returns the id of the newest job for (caller, idemKey) that is +// still within its lease and not failed/cancelled. Reads never write. +func (d *DB) BatchJobByIDem(caller, idemKey string, now int64, ttl time.Duration) (string, bool) { + if d == nil { + return "", false + } + ttlSec := int64(ttl.Seconds()) + var id string + err := d.sql.QueryRow(` + SELECT id FROM batch_job + WHERE caller = ? AND idem_key = ? AND lease + ? > ? + AND status IN ('queued','running','done','partial') + ORDER BY created_at DESC LIMIT 1 + `, caller, idemKey, ttlSec, now).Scan(&id) + if err != nil { + return "", false + } + return id, true +} + +func (d *DB) LiveBatchJobs(caller string) int { + if d == nil { + return 0 + } + var n int + _ = d.sql.QueryRow(` + SELECT COUNT(*) FROM batch_job WHERE caller = ? AND status IN ('queued','running') + `, caller).Scan(&n) + return n +} + +func (d *DB) LiveBatchJobsTotal() int { + if d == nil { + return 0 + } + var n int + _ = d.sql.QueryRow(`SELECT COUNT(*) FROM batch_job WHERE status IN ('queued','running')`).Scan(&n) + return n +} + +func (d *DB) PutBatchTargets(jobID string, targets []BatchTarget) error { + if d == nil { + return fmt.Errorf("dcache: nil db") + } + for _, t := range targets { + if _, err := d.sql.Exec(` + INSERT INTO batch_target(job_id, idx, url, options, status, result, error, hint) + VALUES(?,?,?,?,?,?,?,?) + `, t.JobID, t.Idx, t.URL, t.Options, t.Status, + nullString(t.Result), nullString(t.Error), nullString(t.Hint)); err != nil { + return err + } + } + return nil +} + +func (d *DB) BatchTargets(jobID string) ([]BatchTarget, bool) { + if d == nil { + return nil, false + } + rows, err := d.sql.Query(` + SELECT job_id, idx, url, options, status, result, error, hint + FROM batch_target WHERE job_id = ? ORDER BY idx ASC + `, jobID) + if err != nil { + d.mu.Lock() + d.BatchM++ + d.mu.Unlock() + return nil, false + } + defer rows.Close() + var out []BatchTarget + for rows.Next() { + var ( + r, e, h sql.NullString + t BatchTarget + ) + if err := rows.Scan(&t.JobID, &t.Idx, &t.URL, &t.Options, &t.Status, &r, &e, &h); err != nil { + d.mu.Lock() + d.BatchM++ + d.mu.Unlock() + return nil, false + } + t.Result = nullPtr(r) + t.Error = nullPtr(e) + t.Hint = nullPtr(h) + out = append(out, t) + } + if err := rows.Err(); err != nil { + d.mu.Lock() + d.BatchM++ + d.mu.Unlock() + return nil, false + } + if len(out) == 0 { + return nil, false + } + return out, true +} + +func (d *DB) UpdateBatchTarget(jobID string, idx int, now int64, status string, result, errMsg, hint *string) error { + if d == nil { + return fmt.Errorf("dcache: nil db") + } + tx, err := d.sql.Begin() + if err != nil { + return err + } + defer tx.Rollback() + + var prev string + err = tx.QueryRow(`SELECT status FROM batch_target WHERE job_id = ? AND idx = ?`, jobID, idx).Scan(&prev) + if err != nil { + return err + } + + var resBytes int64 + if result != nil { + resBytes = int64(len(*result)) + } + deltaDone := 0 + if isTerminal(status) && !isTerminal(prev) { + deltaDone = 1 + } + + if _, err := tx.Exec(` + UPDATE batch_target SET status = ?, result = ?, error = ?, hint = ? WHERE job_id = ? AND idx = ? + `, status, nullString(result), nullString(errMsg), nullString(hint), jobID, idx); err != nil { + return err + } + if _, err := tx.Exec(` + UPDATE batch_job SET lease = ?, targets_done = targets_done + ?, nbytes = nbytes + ? WHERE id = ? + `, now, deltaDone, resBytes, jobID); err != nil { + return err + } + return tx.Commit() +} + +func (d *DB) SetBatchJobStatus(id, status, errMsg string, now int64) error { + if d == nil { + return fmt.Errorf("dcache: nil db") + } + _, err := d.sql.Exec(`UPDATE batch_job SET status = ?, error = ?, lease = ? WHERE id = ?`, status, errMsg, now, id) + return err +} + +func (d *DB) TouchBatchJob(id string, now int64) error { + if d == nil { + return fmt.Errorf("dcache: nil db") + } + _, err := d.sql.Exec(`UPDATE batch_job SET lease = ? WHERE id = ?`, now, id) + return err +} + +func (d *DB) SweepBatch(now int64, ttl time.Duration) int { + if d == nil { + return 0 + } + ttlSec := int64(ttl.Seconds()) + if _, err := d.sql.Exec(` + DELETE FROM batch_target WHERE job_id IN + (SELECT id FROM batch_job WHERE lease + ? <= ?) + `, ttlSec, now); err != nil { + return 0 + } + res, err := d.sql.Exec(`DELETE FROM batch_job WHERE lease + ? <= ?`, ttlSec, now) + if err != nil { + return 0 + } + n, _ := res.RowsAffected() + return int(n) +} + +func (d *DB) EvictBatchOldest(maxBytes int64) int { + if d == nil { + return 0 + } + evicted := 0 + for d.BatchNbytes() > maxBytes { + var id string + err := d.sql.QueryRow(` + SELECT id FROM batch_job + WHERE status NOT IN ('queued','running') + ORDER BY lease ASC, id ASC + LIMIT 1 + `).Scan(&id) + if err != nil { + break + } + if _, err := d.sql.Exec(`DELETE FROM batch_target WHERE job_id = ?`, id); err != nil { + break + } + if _, err := d.sql.Exec(`DELETE FROM batch_job WHERE id = ?`, id); err != nil { + break + } + evicted++ + } + return evicted +} + +func (d *DB) MarkInFlightFailed(reason string, now int64) int { + if d == nil { + return 0 + } + res, err := d.sql.Exec(` + UPDATE batch_job SET status = 'failed', error = ?, lease = ? WHERE status IN ('queued','running') + `, reason, now) + if err != nil { + return 0 + } + n, _ := res.RowsAffected() + return int(n) +} + +func (d *DB) BatchNbytes() int64 { + if d == nil { + return 0 + } + var n sql.NullInt64 + _ = d.sql.QueryRow(`SELECT IFNULL(SUM(nbytes),0) FROM batch_job`).Scan(&n) + return n.Int64 +} diff --git a/internal/dcache/batch_test.go b/internal/dcache/batch_test.go new file mode 100644 index 0000000..c9afc19 --- /dev/null +++ b/internal/dcache/batch_test.go @@ -0,0 +1,254 @@ +package dcache + +import ( + "io" + "log/slog" + "strings" + "testing" + "time" +) + +func newBatchDB(t *testing.T) *DB { + t.Helper() + db, err := Open("", 1<<20, slog.New(slog.NewTextHandler(io.Discard, nil))) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + return db +} + +func strp(s string) *string { return &s } + +func TestBatchJobRoundTrip(t *testing.T) { + db := newBatchDB(t) + j := BatchJob{ + ID: "j1", Caller: "alice", CallerClass: ClassFleet, IDemKey: "k1", + Status: "queued", TargetsTotal: 3, CreatedAt: 100, Lease: 100, + } + if err := db.CreateBatchJob(j); err != nil { + t.Fatal(err) + } + got, ok := db.BatchJob("j1") + if !ok { + t.Fatal("missing job") + } + if got.ID != j.ID || got.Caller != "alice" || got.CallerClass != ClassFleet || + got.Status != "queued" || got.TargetsTotal != 3 || got.Lease != 100 || got.CreatedAt != 100 { + t.Fatalf("round trip mismatch: %+v", got) + } + if _, ok := db.BatchJob("nope"); ok { + t.Fatal("unknown id must be ok=false") + } +} + +func TestBatchIDemLookup(t *testing.T) { + db := newBatchDB(t) + if err := db.CreateBatchJob(BatchJob{ID: "j1", Caller: "a", CallerClass: ClassFleet, IDemKey: "k", Status: "running", TargetsTotal: 1, CreatedAt: 100, Lease: 1000}); err != nil { + t.Fatal(err) + } + if id, ok := db.BatchJobByIDem("a", "k", 2000, time.Hour); !ok || id != "j1" { + t.Fatalf("live idem: %q %v", id, ok) + } + if _, ok := db.BatchJobByIDem("a", "k", 5000, time.Hour); ok { + t.Fatal("lease-expired job must not match") + } + if _, ok := db.BatchJobByIDem("b", "k", 2000, time.Hour); ok { + t.Fatal("other caller must not match") + } + // Only a job that is live or has results is reused. A failed (e.g. killed by a restart) + // or cancelled job must not be handed back to a resubmit for the rest of its lease. + for _, c := range []struct { + status string + reuse bool + }{{"queued", true}, {"done", true}, {"partial", true}, {"failed", false}, {"cancelled", false}} { + key := "k-" + c.status + if err := db.CreateBatchJob(BatchJob{ID: "j-" + c.status, Caller: "a", CallerClass: ClassFleet, IDemKey: key, Status: c.status, TargetsTotal: 1, CreatedAt: 100, Lease: 1000}); err != nil { + t.Fatal(err) + } + if _, ok := db.BatchJobByIDem("a", key, 2000, time.Hour); ok != c.reuse { + t.Errorf("status %s: reused=%v, want %v", c.status, ok, c.reuse) + } + } +} + +func TestBatchLiveCounts(t *testing.T) { + db := newBatchDB(t) + mk := func(id, caller, status string) { + if err := db.CreateBatchJob(BatchJob{ID: id, Caller: caller, CallerClass: ClassFleet, IDemKey: id, Status: status, TargetsTotal: 1, CreatedAt: 1, Lease: 1}); err != nil { + t.Fatal(err) + } + } + mk("j1", "a", "queued") + mk("j2", "a", "running") + mk("j3", "a", "done") + mk("j4", "b", "running") + if n := db.LiveBatchJobs("a"); n != 2 { + t.Fatalf("caller a live=%d want 2", n) + } + if n := db.LiveBatchJobsTotal(); n != 3 { + t.Fatalf("total live=%d want 3", n) + } +} + +func TestBatchTargetsOrderedAndUpdate(t *testing.T) { + db := newBatchDB(t) + must := func(err error) { + t.Helper() + if err != nil { + t.Fatal(err) + } + } + must(db.CreateBatchJob(BatchJob{ID: "j1", Caller: "a", CallerClass: ClassFleet, IDemKey: "k", Status: "running", TargetsTotal: 3, CreatedAt: 100, Lease: 100})) + must(db.PutBatchTargets("j1", []BatchTarget{ + {JobID: "j1", Idx: 0, URL: "https://a.example/", Options: `{"mode":"clean"}`, Status: "pending"}, + {JobID: "j1", Idx: 1, URL: "https://b.example/", Status: "pending"}, + {JobID: "j1", Idx: 2, URL: "https://c.example/", Status: "pending"}, + })) + got, ok := db.BatchTargets("j1") + if !ok || len(got) != 3 { + t.Fatalf("targets len=%d ok=%v", len(got), ok) + } + for i := range got { + if got[i].Idx != i { + t.Fatalf("order broken: got[%d].Idx=%d", i, got[i].Idx) + } + } + + // A done write bumps the lease to now and counts the target once. + res := strp(`{"content":"ok"}`) + must(db.UpdateBatchTarget("j1", 0, 200, "done", res, nil, nil)) + j, _ := db.BatchJob("j1") + if j.Lease != 200 || j.TargetsDone != 1 { + t.Fatalf("after first write lease=%d done=%d", j.Lease, j.TargetsDone) + } + // Re-writing the same target (still a done transition) must not double-count. + must(db.UpdateBatchTarget("j1", 0, 201, "done", res, nil, nil)) + j, _ = db.BatchJob("j1") + if j.TargetsDone != 1 { + t.Fatalf("double-counted: done=%d", j.TargetsDone) + } + // A failed write counts too. + must(db.UpdateBatchTarget("j1", 1, 202, "failed", nil, strp("upstream 500"), strp("browser"))) + j, _ = db.BatchJob("j1") + if j.TargetsDone != 2 || j.Lease != 202 { + t.Fatalf("after failed write lease=%d done=%d", j.Lease, j.TargetsDone) + } +} + +func TestBatchTouchAndStatus(t *testing.T) { + db := newBatchDB(t) + if err := db.CreateBatchJob(BatchJob{ID: "j1", Caller: "a", CallerClass: ClassFleet, IDemKey: "k", Status: "running", TargetsTotal: 1, CreatedAt: 100, Lease: 100}); err != nil { + t.Fatal(err) + } + if err := db.TouchBatchJob("j1", 500); err != nil { + t.Fatal(err) + } + if j, _ := db.BatchJob("j1"); j.Lease != 500 { + t.Fatalf("touch lease=%d want 500", j.Lease) + } + if err := db.SetBatchJobStatus("j1", "cancelled", "", 600); err != nil { + t.Fatal(err) + } + if j, _ := db.BatchJob("j1"); j.Status != "cancelled" || j.Lease != 600 { + t.Fatalf("status=%q lease=%d", j.Status, j.Lease) + } +} + +func TestSweepBatch(t *testing.T) { + db := newBatchDB(t) + mk := func(id string, lease int64) { + if err := db.CreateBatchJob(BatchJob{ID: id, Caller: "a", CallerClass: ClassFleet, IDemKey: id, Status: "done", TargetsTotal: 1, CreatedAt: 1, Lease: lease}); err != nil { + t.Fatal(err) + } + if err := db.PutBatchTargets(id, []BatchTarget{{JobID: id, Idx: 0, URL: "https://x.example/", Status: "done", Result: strp("x")}}); err != nil { + t.Fatal(err) + } + } + mk("old", 100) + mk("new", 1000) + n := db.SweepBatch(300, 100*time.Second) // old: 100+100=200<=300 expired; new: 1100>300 kept + if n != 1 { + t.Fatalf("swept %d want 1", n) + } + if _, ok := db.BatchJob("old"); ok { + t.Fatal("old job must be gone") + } + if _, ok := db.BatchJob("new"); !ok { + t.Fatal("new job must remain") + } + if _, ok := db.BatchTargets("old"); ok { + t.Fatal("old targets must be gone") + } +} + +func TestEvictBatchOldest(t *testing.T) { + db := newBatchDB(t) + done := func(id string, lease int64, size int) { + if err := db.CreateBatchJob(BatchJob{ID: id, Caller: "a", CallerClass: ClassFleet, IDemKey: id, Status: "done", TargetsTotal: 1, CreatedAt: 1, Lease: lease}); err != nil { + t.Fatal(err) + } + if err := db.PutBatchTargets(id, []BatchTarget{{JobID: id, Idx: 0, URL: "https://x.example/", Status: "pending"}}); err != nil { + t.Fatal(err) + } + if err := db.UpdateBatchTarget(id, 0, lease, "done", strp(strings.Repeat("x", size)), nil, nil); err != nil { + t.Fatal(err) + } + } + done("old1", 100, 100) + done("old2", 200, 200) + done("new", 300, 300) + + // A running job with one completed target must never be byte-evicted. + if err := db.CreateBatchJob(BatchJob{ID: "run", Caller: "a", CallerClass: ClassFleet, IDemKey: "run", Status: "running", TargetsTotal: 2, TargetsDone: 1, CreatedAt: 1, Lease: 400}); err != nil { + t.Fatal(err) + } + if err := db.PutBatchTargets("run", []BatchTarget{ + {JobID: "run", Idx: 0, URL: "https://x.example/", Status: "pending"}, + {JobID: "run", Idx: 1, URL: "https://y.example/", Status: "pending"}, + }); err != nil { + t.Fatal(err) + } + if err := db.UpdateBatchTarget("run", 0, 400, "done", strp(strings.Repeat("x", 500)), nil, nil); err != nil { + t.Fatal(err) + } + + // total nbytes = 100+200+300+500 = 1100. Evict oldest done until <= 250: + // old1(100), old2(200), new(300) all evicted (oldest first); run stays (live). + db.EvictBatchOldest(250) + for _, id := range []string{"old1", "old2", "new"} { + if _, ok := db.BatchJob(id); ok { + t.Fatalf("%s should be byte-evicted", id) + } + } + if _, ok := db.BatchJob("run"); !ok { + t.Fatal("running job must not be byte-evicted") + } + if n := db.BatchNbytes(); n != 500 { + t.Fatalf("BatchNbytes=%d want 500", n) + } +} + +func TestMarkInFlightFailed(t *testing.T) { + db := newBatchDB(t) + mk := func(id, status string) { + if err := db.CreateBatchJob(BatchJob{ID: id, Caller: "a", CallerClass: ClassFleet, IDemKey: id, Status: status, TargetsTotal: 1, CreatedAt: 1, Lease: 1}); err != nil { + t.Fatal(err) + } + } + mk("q", "queued") + mk("r", "running") + mk("d", "done") + if n := db.MarkInFlightFailed("restart", 999); n != 2 { + t.Fatalf("marked %d want 2", n) + } + if j, _ := db.BatchJob("q"); j.Status != "failed" || j.Error != "restart" || j.Lease != 999 { + t.Fatalf("q %+v", j) + } + if j, _ := db.BatchJob("r"); j.Status != "failed" || j.Error != "restart" { + t.Fatalf("r %+v", j) + } + if j, _ := db.BatchJob("d"); j.Status != "done" { + t.Fatalf("done job changed: %+v", j) + } +} diff --git a/internal/dcache/dcache.go b/internal/dcache/dcache.go index ebff459..579cb13 100644 --- a/internal/dcache/dcache.go +++ b/internal/dcache/dcache.go @@ -47,6 +47,8 @@ type DB struct { SearchM uint64 PDFH uint64 PDFM uint64 + BatchH uint64 + BatchM uint64 Evictions uint64 } @@ -110,6 +112,32 @@ func Open(path string, maxBytes int64, log *slog.Logger) (*DB, error) { text TEXT, PRIMARY KEY (k, page) ); + CREATE TABLE IF NOT EXISTS batch_job ( + id TEXT PRIMARY KEY, + caller TEXT NOT NULL, + caller_class TEXT NOT NULL, + idem_key TEXT NOT NULL, + status TEXT NOT NULL, + error TEXT NOT NULL DEFAULT '', + targets_total INTEGER NOT NULL, + targets_done INTEGER NOT NULL DEFAULT 0, + created_at INTEGER NOT NULL, + lease INTEGER NOT NULL, + nbytes INTEGER NOT NULL DEFAULT 0 + ); + CREATE TABLE IF NOT EXISTS batch_target ( + job_id TEXT NOT NULL, + idx INTEGER NOT NULL, + url TEXT NOT NULL, + options TEXT NOT NULL, + status TEXT NOT NULL, + result TEXT, + error TEXT, + hint TEXT, + PRIMARY KEY (job_id, idx) + ); + CREATE INDEX IF NOT EXISTS batch_job_idem ON batch_job(caller, idem_key); + CREATE INDEX IF NOT EXISTS batch_job_lease ON batch_job(lease); `); err != nil { _ = sq.Close() return nil, err From e2ddb2e9d73b17c038a5f83e0d20967640719107 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 21:30:35 -0700 Subject: [PATCH 02/10] =?UTF-8?q?batch:=20runner=20core=20=E2=80=94=20type?= =?UTF-8?q?s,=20Submit=20with=20idempotency=20and=20caps,=20Status,=20Touc?= =?UTF-8?q?h,=20Cancel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New package internal/batch holding the runner core with no workers yet (task 03 adds them; until then every job stays queued, so a queued job counts as live for the caps). batch.go defines the public surface: Options/Target/Result/TargetError, FetchFunc, Job/TargetStatus, Hooks and Config with its defaults (TTL 24h, 32 targets/job, 1 job/caller, 8 total, worker clamped to 2, 64 MiB, 2 s / 10 m retry). Submit runs in the given order: empty → ErrNoTargets; idempotency via hex SHA-256 of `c.Name + NUL + json.Marshal(targets)` (a live BatchJobByIDem hit returns the id reused, touching no caps); then TargetsPerJob / JobsPerCaller / JobsTotal; then it stores the job queued (random 16-byte hex id, CreatedAt = Lease = now) and its targets (pending, Options = json of t.Options) before returning. The submitting caller is held in memory keyed by job id, never persisted (jobs never resume after a restart). Status maps the rows and decodes Result from the result column without writing; Touch moves the lease; Cancel sets a queued job cancelled at once and leaves its targets pending; Close is idempotent. Store errors are Warn-logged with the job id, never a URL or host. Co-Authored-By: ornith-1.5-35b-a3b --- docs/implementer-log.md | 3 +- internal/batch/batch.go | 330 +++++++++++++++++++++++++++++ internal/batch/runner_core_test.go | 182 ++++++++++++++++ 3 files changed, 514 insertions(+), 1 deletion(-) create mode 100644 internal/batch/batch.go create mode 100644 internal/batch/runner_core_test.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 16658ac..12b81fd 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -17,7 +17,8 @@ One row per task session that ends in a commit. Newest last. | p5b/03-api | 2026-09-26 | done | 1 | gate: ok | none | The digest now answers long documents itself (task 02), so the API's refusal limit is only a safety cap: `DefaultPDFMaxQuestionChunks` 200→5000 in `internal/api/pdf.go` (the 422 message text unchanged, still "document too long for question (N chunks); use grep or pages"). In `v1fetch` (`internal/api/api.go`) the digest-meta map gains `"retrieval": dr.Retrieval` and `"embedded": dr.Embedded` next to `chunks_used`/`chunks_total`, only when the digest ran without a fallback. Copied the two given tests byte-identical from the plan — the replacement `pdf_test.go` is now the protected one and asserts `DefaultPDFMaxQuestionChunks == 5000`; `pdf_retrieval_test.go` asserts the first long-doc question returns `retrieval=bm25`, `embedded=0`. README PDF section: a long-doc question is answered first from BM25 (`retrieval: "bm25"`) while its chunks embed in the background and hybrid on later questions, plus the 5000-chunk cap. One sentence added to `skill/SKILL.md`. All api tests pass under `-race -count=3`; make gate ok; given files byte-identical. | | p5c/01-extract-api | 2026-09-26 | done | 1 | gate: ok | none | `POST /v1/extract` answers a PDF posted in the body like `/v1/fetch` answers the same PDF by URL, nothing fetched. `internal/api/extract.go`: auth via `caller`; `io.ReadAll` of the body (a `*http.MaxBytesError`→413 "too large (>N MB)", empty→400); `clean.Kind` must be "pdf" else 415; query `pages`/`grep`/`question`/`mode` (digest when a question is set, else clean; other→400). Content-addressed cache: `hash="sha256:"+HashBody(body)`, `key=PDFKey("upload:"+hash, callerClass)`; a `GetPDF` hit rebuilds the doc via `pdfFromEntry` with `cached:true` + `PDFCacheHit` and no child (bypasses the TTL/revalidation that `pdfFromCache` does, so identical content is extracted once); a miss builds a synthetic `fetch.Resource{URL:"upload",...}` and runs `extractPDF`+`storePDF`. Bad pages/grep→400 via `writeFetchErr`, the chunk limit→the same 422 as `v1fetch`, extraction errors→`pdfError` via `writeFetchErr`. Envelope mirrors the PDF fetch path (`content`/`mode`/`untrusted`/`warnings`/`sizes`/`timings`/`pdf`/`digest`); `source` is `{"kind":"upload","hash","bytes","cached","fetched_at"}` with no `url` key; no Honcho context for uploads; one log line "extract" with caller/bytes/pdf_pages/mode/cached. `internal/api/api.go` scoped to the rule-1 changes: `DefaultPDFMaxUpload` const (100 MiB), `PDFMaxUpload int64` Server field, the `POST /v1/extract` route in `New`, and `ServeHTTP` (per-path upload cap instead of the 64 KiB JSON limit, and `/v1/extract` added to the `requestDeadline` switch). The `pdfMaxUpload()` field-or-default resolver lives in extract.go. Given test byte-identical; all api tests pass under `-race -count=3`; make gate ok. | | p5c/02-extract-cli | 2026-09-26 | done | 1 | gate: ok | none | `sift extract ` uploads a local PDF to `/v1/extract` and prints the answer exactly as `sift fetch` does. `cmd/sift/extractcmd.go`: `extractOpts` + `parseExtractOpts` (reuses `splitAroundPositional` with `extractValueFlags` mode/pages/grep/question, one positional file, `--json`), `extractURL` builds `base+"/v1/extract"` with `url.Values` set only for the options that are set and `mode=digest` when a question is given and no mode — values are URL-encoded so a grep like `a b&c` survives round-trip; `extractMain` reuses `loadClient`, `stat`s the file and refuses >100 MiB before uploading (exit 3, matching the server's 413→exitFor), then `http.NewRequest` with the `*os.File` as the streaming body and `ContentLength` set (no second in-memory copy), `Content-Type: application/pdf` + bearer, and the same error/exitFor path as fetch; success prints `content` (or the `--json` envelope). `main.go`: `case "extract"` and one help line. README: extract line in the CLI block plus a sentence; SKILL.md: a PDF-on-disk bullet in the PDFs section. Given test byte-identical; all cmd/sift tests pass under `-race -count=3`; make gate ok; `bin/sift extract` prints the usage line. | -| p6/01-batchstore | 2026-09-26 | done | 1 | gate: ok | none | Storage-only batch store in `internal/dcache` (new `batch.go`; schema added to `Open`; `BatchH`/`BatchM` counters added to `DB`). `CreateBatchJob` stores as given. Reads never write the lease: `BatchJob`, `BatchTargets` and the counts are pure, so exact lease values hold after a read. `BatchJobByIDem` returns the newest `(caller,idemKey)` job with `lease+ttl>now` and status queued/running/done/partial; failed/cancelled never reused. `PutBatchTargets` inserts in order; `BatchTargets` returns idx order with NULLs as nil pointers. `UpdateBatchTarget` bumps lease to `now`, increments `targets_done` exactly once per target entering done/failed (previous-status check prevents re-write double-count), and adds `len(result)` to `nbytes` when a result is written — all in one tx. `SetBatchJobStatus`/`TouchBatchJob` set lease; `SweepBatch` deletes by `lease+ttl<=now` with its targets and returns the job count; `EvictBatchOldest` evicts whole non-live jobs oldest-lease-first (never a queued/running one) until `BatchNbytes()<=maxBytes`; `MarkInFlightFailed` fails queued/running on restart; `BatchNbytes` sums job nbytes, kept out of `Size()`. Accessors return `(v,bool)`/`error`, count misses in `BatchM`, reuse `ClassFleet`/`ClassRestricted`. A first pass failed `TestBatchIDemLookup` because I forgot `.Scan(&id)` on the `QueryRow` (the un-scanned `*sql.Row` was always non-nil, so the method always missed); fixed and verified. All dcache tests pass under `-race -count=3`; make gate ok; given file byte-identical. | +| p6/01-batchstore | 2026-09-26 | done | 1 | gate: ok | none | Storage-only batch store in `internal/dcache` (new `batch.go`; schema added to `Open`; `BatchH`/`BatchM` counters added to `DB`). `CreateBatchJob` stores as given. Reads never write the lease: `BatchJob`, `BatchTargets` and the counts are pure, so exact lease values hold after a read. `BatchJobByIDem` returns the newest `(caller,idemKey)` job with `lease+ttl>now` and status queued/running/done/partial; failed/cancelled never reused. `PutBatchTargets` inserts in order; `BatchTargets` returns idx order with NULLs as nil pointers. `UpdateBatchTarget` bumps lease to `now`, increments `targets_done` exactly once per target entering done/failed (previous-status check prevents re-write double-count), and adds `len(result)` to `nbytes` when a result is written — all in one tx. `SetBatchJobStatus`/`TouchBatchJob` set lease; `SweepBatch` deletes by `lease+ttl<=now` with its targets and returns the job count; `EvictBatchOldest` evicts whole non-live jobs oldest-lease-first (never a queued/running one) until `BatchNbytes()<=maxBytes`; `MarkInFlightFailed` fails queued/running on restart; `BatchNbytes` sums job nbytes, kept out of `Size()`. Accessors return `(v,bool)`/`error`, count misses in `BatchM`, reuse `ClassFleet`/`ClassRestricted`. A first pass failed `TestBatchIDemLookup` because I forgot `.Scan(&id)` on the `QueryRow` (the un-scanned `*sql.Row` was always non-nil, so the method always missed); fixed and verified. All dcache tests pass under `-race -count=3`; make gate ok; given file byte-identical. +| p6/02-runner-core | 2026-09-26 | done | 1 | gate: ok | caller-cleanup hand-off to task 03 | Runner core in new internal/batch with no workers (task 03 adds them; jobs stay queued, so a queued job counts as live for the caps). Public surface in batch.go: Options/Target/Result/TargetError, FetchFunc, Job/TargetStatus, Hooks and Config with defaults (TTL 24h, 32 targets/job, 1 job/caller, 8 total, worker clamped to 2, 64 MiB, 2 s/10 m retry). Submit runs the given order — empty→ErrNoTargets, idempotency via hex SHA-256 of `name+NUL+json.Marshal(targets)` (a live hit returns the id reused, touching no caps), then TargetsPerJob/JobsPerCaller/JobsTotal caps, then stores the job queued (random 16-byte hex id, CreatedAt=Lease=now) and its targets (pending, Options=json of t.Options) before returning so a queued job is live; the submitting caller is held in memory by job id, never persisted. Status maps rows and decodes Result without writing; Touch moves the lease; Cancel sets a queued job cancelled at once and leaves its targets pending; Close is idempotent. Store errors are Warn-logged with the job id, never a URL/host. | | ## P5a review (owner, 2026-09-26) diff --git a/internal/batch/batch.go b/internal/batch/batch.go new file mode 100644 index 0000000..51ed5d5 --- /dev/null +++ b/internal/batch/batch.go @@ -0,0 +1,330 @@ +// Package batch is the batch runner core: it owns job lifecycle, idempotency and +// the per-caller / global caps. It knows nothing about HTTP, the guard or the real +// fetch pipeline — per-target work is an injected FetchFunc, and persistence is the +// dcache store. Jobs never resume after a restart, so the caller is held in memory, +// never written to the store. +package batch + +import ( + "context" + "crypto/rand" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "log/slog" + "sync" + "time" + + "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/dcache" +) + +type Options struct { + Mode string `json:"mode,omitempty"` + Question string `json:"question,omitempty"` + Pages string `json:"pages,omitempty"` + Grep string `json:"grep,omitempty"` + JQ string `json:"jq,omitempty"` + Context *bool `json:"context,omitempty"` // nil = /v1/fetch's default (on when there is a question) + Recover bool `json:"recover,omitempty"` +} + +type Target struct { + URL string `json:"url"` + Options Options `json:"options"` +} + +// Result is a finished target: the /v1/fetch envelope's content, and every other envelope field +// in Meta. It is stored JSON-encoded in the target's result column. +type Result struct { + Content string `json:"content"` + Meta map[string]any `json:"meta,omitempty"` +} + +// TargetError is a failure with an agent-facing hint. Status is the HTTP status the same +// failure gets from /v1/fetch; the runner ignores it (the metrics hook reads it). Any other +// error is stored with its Error() text and no hint. +type TargetError struct { + Status int + Msg, Hint string +} + +func (e *TargetError) Error() string { return e.Msg } + +// FetchFunc does one target for the caller that submitted the job. +type FetchFunc func(ctx context.Context, c *auth.Caller, t Target) (Result, error) + +type Job struct { + ID string + Caller string + Status string // queued | running | done | partial | failed | cancelled + Error string + TargetsTotal int + TargetsDone int + CreatedAt int64 // unix seconds +} + +type TargetStatus struct { + Idx int + URL string + Status string // pending | running | done | failed + Result *Result + Error string + Hint string +} + +// Hooks observe the runner for metrics. Any nil field is skipped. +type Hooks struct { + Job func(outcome string) // created, reused, done, partial, failed, cancelled + Target func(err error, d time.Duration, resultBytes int) // once per stored target; err nil on success + JobSeconds func(d time.Duration) // submit → terminal, once per job + Depth func(live int) // live jobs, after every change + Inflight func(n int) // targets being fetched, after every change +} + +type Config struct { + TTL time.Duration // 0 → 24h + TargetsPerJob int // 0 → 32 + JobsPerCaller int // 0 → 1 + JobsTotal int // 0 → 8 + Workers int // 0 → 1; values above 2 are clamped to 2 + MaxBytes int64 // 0 → 64 MiB + RetryWait time.Duration // 0 → 2s: the pause before a busy target is tried again + RetryFor time.Duration // 0 → 10m: how long one target may keep coming back busy + Hooks Hooks +} + +var ( + ErrNoTargets = errors.New("no targets") + ErrTooManyTargets = errors.New("too many targets") + ErrCallerBusy = errors.New("busy") + ErrBusy = errors.New("jobs busy") + ErrRetryLater = errors.New("retry later") // a FetchFunc wraps this when the target was only busy +) + +// terminal statuses mark a job as finished; only a queued job can be cancelled. +func isJobTerminal(status string) bool { + switch status { + case "done", "partial", "failed", "cancelled": + return true + } + return false +} + +type Runner struct { + db *dcache.DB + 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 +} + +func New(db *dcache.DB, fetch FetchFunc, cfg Config, log *slog.Logger) *Runner { + if cfg.TTL == 0 { + cfg.TTL = 24 * time.Hour + } + if cfg.TargetsPerJob == 0 { + cfg.TargetsPerJob = 32 + } + if cfg.JobsPerCaller == 0 { + cfg.JobsPerCaller = 1 + } + if cfg.JobsTotal == 0 { + cfg.JobsTotal = 8 + } + if cfg.Workers == 0 { + cfg.Workers = 1 + } + if cfg.Workers > 2 { + cfg.Workers = 2 + } + if cfg.MaxBytes == 0 { + cfg.MaxBytes = 64 << 20 + } + if cfg.RetryWait == 0 { + cfg.RetryWait = 2 * time.Second + } + if cfg.RetryFor == 0 { + cfg.RetryFor = 10 * time.Minute + } + return &Runner{ + db: db, + fetch: fetch, + log: log, + cfg: cfg, + callers: make(map[string]*auth.Caller), + } +} + +// Submit stores the job and its targets before it returns. The order matters: an empty +// request, then idempotency (which never touches the caps), then the caps, then the write. +func (r *Runner) Submit(ctx context.Context, c *auth.Caller, callerClass string, targets []Target) (id string, reused bool, err error) { + if len(targets) == 0 { + return "", false, ErrNoTargets + } + + raw, err := json.Marshal(targets) + if err != nil { + return "", false, fmt.Errorf("batch: marshal targets: %w", err) + } + sum := sha256.Sum256([]byte(c.Name + "\x00" + string(raw))) + idemKey := hex.EncodeToString(sum[:]) + + now := time.Now().Unix() + if existing, ok := r.db.BatchJobByIDem(c.Name, idemKey, now, r.cfg.TTL); ok { + r.callJob("reused") + return existing, true, nil + } + + if len(targets) > r.cfg.TargetsPerJob { + return "", false, ErrTooManyTargets + } + if r.db.LiveBatchJobs(c.Name) >= r.cfg.JobsPerCaller { + return "", false, ErrCallerBusy + } + if r.db.LiveBatchJobsTotal() >= r.cfg.JobsTotal { + return "", false, ErrBusy + } + + id, err = randomID() + if err != nil { + return "", false, fmt.Errorf("batch: job id: %w", err) + } + + if err := r.db.CreateBatchJob(dcache.BatchJob{ + ID: id, + Caller: c.Name, + CallerClass: callerClass, + IDemKey: idemKey, + Status: "queued", + TargetsTotal: len(targets), + CreatedAt: now, + Lease: now, + }); err != nil { + r.log.Warn("batch: create job", "job", id, "err", err) + return "", false, fmt.Errorf("batch: create job: %w", err) + } + + btargets := make([]dcache.BatchTarget, len(targets)) + for i := range targets { + opts, _ := json.Marshal(targets[i].Options) + btargets[i] = dcache.BatchTarget{ + JobID: id, + Idx: i, + URL: targets[i].URL, + Options: string(opts), + Status: "pending", + } + } + if err := r.db.PutBatchTargets(id, btargets); err != nil { + r.log.Warn("batch: store targets", "job", id, "err", err) + return "", false, fmt.Errorf("batch: store targets: %w", err) + } + + r.callMu.Lock() + r.callers[id] = c + r.callMu.Unlock() + + r.callJob("created") + r.db.SweepBatch(now, r.cfg.TTL) + return id, false, nil +} + +// Cancel sets a queued job cancelled at once; its pending targets stay pending. It is +// false for an unknown id or a job already terminal. +func (r *Runner) Cancel(id string) bool { + jb, ok := r.db.BatchJob(id) + if !ok || isJobTerminal(jb.Status) { + return false + } + if err := r.db.SetBatchJobStatus(id, "cancelled", "", time.Now().Unix()); err != nil { + 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 +} + +// Status maps the store rows for a job and its targets; Result is decoded from the result +// column. It writes nothing. +func (r *Runner) Status(id string) (Job, []TargetStatus, bool) { + jb, ok := r.db.BatchJob(id) + if !ok { + return Job{}, nil, false + } + j := Job{ + ID: jb.ID, + Caller: jb.Caller, + Status: jb.Status, + Error: jb.Error, + TargetsTotal: jb.TargetsTotal, + TargetsDone: jb.TargetsDone, + CreatedAt: jb.CreatedAt, + } + tbs, _ := r.db.BatchTargets(id) + ts := make([]TargetStatus, 0, len(tbs)) + for _, bt := range tbs { + var res *Result + if bt.Result != nil { + res = new(Result) + if err := json.Unmarshal([]byte(*bt.Result), res); err != nil { + r.log.Warn("batch: decode result", "job", id, "err", err) + } + } + ts = append(ts, TargetStatus{ + Idx: bt.Idx, + URL: bt.URL, + Status: bt.Status, + Result: res, + Error: strOrEmpty(bt.Error), + Hint: strOrEmpty(bt.Hint), + }) + } + return j, ts, true +} + +// Touch bumps the job's lease to now so a slow owner does not lose it. +func (r *Runner) Touch(id string) { + now := time.Now().Unix() + if err := r.db.TouchBatchJob(id, now); err != nil { + r.log.Warn("batch: touch job", "job", id, "err", err) + } +} + +// Close stops the workers and waits for an in-flight fetch to return. Safe to call more than +// once. With no workers (task 02) there is nothing to stop; the in-memory callers are dropped. +func (r *Runner) Close() { + r.closeOnce.Do(func() { + r.callMu.Lock() + r.callers = nil + r.callMu.Unlock() + }) +} + +func (r *Runner) callJob(outcome string) { + if r.cfg.Hooks.Job != nil { + r.cfg.Hooks.Job(outcome) + } +} + +func randomID() (string, error) { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + return "", err + } + return hex.EncodeToString(b[:]), nil +} + +func strOrEmpty(s *string) string { + if s == nil { + return "" + } + return *s +} diff --git a/internal/batch/runner_core_test.go b/internal/batch/runner_core_test.go new file mode 100644 index 0000000..6576aae --- /dev/null +++ b/internal/batch/runner_core_test.go @@ -0,0 +1,182 @@ +package batch + +// P6 task 02: the runner's core — types, Submit (idempotency, caps, storage), Status, Touch and +// Cancel — with no workers yet: every job stays queued. The helpers here are shared with task +// 03's runner_test.go. + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" + "time" + + "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/dcache" +) + +var ( + alice = &auth.Caller{Name: "alice", Workspace: "hermes-fleet"} + bob = &auth.Caller{Name: "bob", Workspace: "hermes-fleet"} + carol = &auth.Caller{Name: "carol", Workspace: "hermes-fleet"} +) + +func newRunner(t *testing.T, fetch FetchFunc, cfg Config) *Runner { + t.Helper() + db, err := dcache.Open("", 1<<20, slog.New(slog.NewTextHandler(io.Discard, nil))) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + r := New(db, fetch, cfg, slog.New(slog.NewTextHandler(io.Discard, nil))) + t.Cleanup(r.Close) // runs before db.Close (cleanups are LIFO) + return r +} + +func waitStatus(t *testing.T, r *Runner, id, want string) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + j, _, ok := r.Status(id) + if ok && j.Status == want { + return + } + time.Sleep(2 * time.Millisecond) + } + j, _, _ := r.Status(id) + t.Fatalf("job %s never reached %q (now %q)", id, want, j.Status) +} + +func urls(ss ...string) []Target { + var out []Target + for _, s := range ss { + out = append(out, Target{URL: "https://" + s + ".example/"}) + } + return out +} + +// Submit stores the job queued with its targets pending, before it returns. +func TestSubmitStoresQueued(t *testing.T) { + r := newRunner(t, func(context.Context, *auth.Caller, Target) (Result, error) { + return Result{Content: "ok"}, nil + }, Config{Workers: 1}) + id, reused, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil || reused || len(id) != 32 { + t.Fatalf("submit id=%q reused=%v err=%v, want a 32-hex-char id", id, reused, err) + } + j, ts, ok := r.Status(id) + if !ok || j.ID != id || j.Caller != "alice" || j.TargetsTotal != 2 || j.CreatedAt == 0 { + t.Fatalf("job %+v ok=%v", j, ok) + } + if j.Status != "queued" && j.Status != "running" && j.Status != "done" { + t.Fatalf("status %q", j.Status) + } + if len(ts) != 2 || ts[0].Idx != 0 || ts[0].URL != "https://a.example/" || ts[1].URL != "https://b.example/" { + t.Fatalf("targets %+v", ts) + } +} + +// Cancelling a job no worker has reached: it is cancelled at once, its targets stay pending, +// and the same list then starts a new job. +func TestCancelQueued(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{Workers: 1}) + busy, _, err := r.Submit(context.Background(), bob, dcache.ClassFleet, urls("x")) // occupies the only worker once there is one + if err != nil { + t.Fatal(err) + } + _ = busy + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil { + t.Fatal(err) + } + if !r.Cancel(id) { + t.Fatal("cancel returned false") + } + j, ts, _ := r.Status(id) + if j.Status != "cancelled" || ts[0].Status != "pending" || ts[1].Status != "pending" { + t.Fatalf("after cancel: %+v %+v", j, ts) + } + if r.Cancel(id) || r.Cancel("no-such-job") { + t.Fatal("cancel of a finished or unknown job must return false") + } + id2, reused, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil || reused || id2 == id { + t.Fatalf("resubmit after cancel id=%q reused=%v err=%v", id2, reused, err) + } +} + +// Touch keeps a job alive; Status never does. +func TestTouchAndStatusLease(t *testing.T) { + db, err := dcache.Open("", 1<<20, slog.New(slog.NewTextHandler(io.Discard, nil))) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + release := make(chan struct{}) + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { + <-release + return Result{Content: "ok"}, nil + } + r := New(db, fetch, Config{Workers: 1}, slog.New(slog.NewTextHandler(io.Discard, nil))) + t.Cleanup(r.Close) + defer close(release) // runs before the cleanups + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a")) + if err != nil { + t.Fatal(err) + } + time.Sleep(50 * time.Millisecond) // let a worker (task 03) take the target; it then blocks + before, _ := db.BatchJob(id) + time.Sleep(1100 * time.Millisecond) // the lease is in whole seconds + r.Status(id) + if j, _ := db.BatchJob(id); j.Lease != before.Lease { + t.Fatalf("Status moved the lease %d → %d; a read must not write", before.Lease, j.Lease) + } + r.Touch(id) + if j, _ := db.BatchJob(id); j.Lease <= before.Lease { + t.Fatalf("Touch did not move the lease: %d → %d", before.Lease, j.Lease) + } +} + +func TestCaps(t *testing.T) { + release := make(chan struct{}) + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { + <-release + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{TargetsPerJob: 2, JobsTotal: 2}) + defer close(release) + if _, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b", "c")); !errors.Is(err, ErrTooManyTargets) { + t.Fatalf("3 targets over a cap of 2: want ErrTooManyTargets, got %v", err) + } + if _, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, nil); !errors.Is(err, ErrNoTargets) { + t.Fatalf("no targets: want ErrNoTargets, got %v", err) + } + if _, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a")); err != nil { + t.Fatal(err) + } + // alice's job is live (queued or running): a second, different list is refused. + if _, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("z")); !errors.Is(err, ErrCallerBusy) { + t.Fatalf("want ErrCallerBusy, got %v", err) + } + if _, _, err := r.Submit(context.Background(), bob, dcache.ClassFleet, urls("b")); err != nil { + t.Fatal(err) + } + // Two live jobs in total: a third caller is refused. + if _, _, err := r.Submit(context.Background(), carol, dcache.ClassFleet, urls("c")); !errors.Is(err, ErrBusy) { + t.Fatalf("want ErrBusy, got %v", err) + } +} + +func TestStatusUnknown(t *testing.T) { + r := newRunner(t, func(context.Context, *auth.Caller, Target) (Result, error) { return Result{}, nil }, Config{}) + if _, _, ok := r.Status("no-such-job"); ok { + t.Fatal("unknown id must be ok=false") + } +} From 816a76fe5b679c29afcfb9e5c61d1cdb1c3dfc43 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 22:18:35 -0700 Subject: [PATCH 03/10] =?UTF-8?q?batch:=20workers=20=E2=80=94=20round-robi?= =?UTF-8?q?n,=20serial=20per=20job,=20busy=20retry,=20cancel=20in=20flight?= =?UTF-8?q?,=20hooks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Start cfg.Workers goroutines in New that drain one rotation of live jobs in submit order, each taking the next job with a pending target and none in flight, so targets alternate across jobs and each job stays serial. One target per fetch under the Close-cancelled context: panic recovers into ErrInternal (never echoed), ErrRetryLater retries after RetryWait (bounded by RetryFor), otherwise the target is stored done/failed and the hooks fire. Terminal status is set once per job; cancelling a running job stores the in-flight target without overwriting cancelled and leaves later targets pending. Co-Authored-By: ornith-1.5-35b-a3b --- docs/implementer-log.md | 3 +- internal/batch/batch.go | 74 ++++++- internal/batch/runner_test.go | 373 ++++++++++++++++++++++++++++++++++ internal/batch/worker.go | 271 ++++++++++++++++++++++++ 4 files changed, 715 insertions(+), 6 deletions(-) create mode 100644 internal/batch/runner_test.go create mode 100644 internal/batch/worker.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 12b81fd..105bd40 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -18,7 +18,8 @@ One row per task session that ends in a commit. Newest last. | p5c/01-extract-api | 2026-09-26 | done | 1 | gate: ok | none | `POST /v1/extract` answers a PDF posted in the body like `/v1/fetch` answers the same PDF by URL, nothing fetched. `internal/api/extract.go`: auth via `caller`; `io.ReadAll` of the body (a `*http.MaxBytesError`→413 "too large (>N MB)", empty→400); `clean.Kind` must be "pdf" else 415; query `pages`/`grep`/`question`/`mode` (digest when a question is set, else clean; other→400). Content-addressed cache: `hash="sha256:"+HashBody(body)`, `key=PDFKey("upload:"+hash, callerClass)`; a `GetPDF` hit rebuilds the doc via `pdfFromEntry` with `cached:true` + `PDFCacheHit` and no child (bypasses the TTL/revalidation that `pdfFromCache` does, so identical content is extracted once); a miss builds a synthetic `fetch.Resource{URL:"upload",...}` and runs `extractPDF`+`storePDF`. Bad pages/grep→400 via `writeFetchErr`, the chunk limit→the same 422 as `v1fetch`, extraction errors→`pdfError` via `writeFetchErr`. Envelope mirrors the PDF fetch path (`content`/`mode`/`untrusted`/`warnings`/`sizes`/`timings`/`pdf`/`digest`); `source` is `{"kind":"upload","hash","bytes","cached","fetched_at"}` with no `url` key; no Honcho context for uploads; one log line "extract" with caller/bytes/pdf_pages/mode/cached. `internal/api/api.go` scoped to the rule-1 changes: `DefaultPDFMaxUpload` const (100 MiB), `PDFMaxUpload int64` Server field, the `POST /v1/extract` route in `New`, and `ServeHTTP` (per-path upload cap instead of the 64 KiB JSON limit, and `/v1/extract` added to the `requestDeadline` switch). The `pdfMaxUpload()` field-or-default resolver lives in extract.go. Given test byte-identical; all api tests pass under `-race -count=3`; make gate ok. | | p5c/02-extract-cli | 2026-09-26 | done | 1 | gate: ok | none | `sift extract ` uploads a local PDF to `/v1/extract` and prints the answer exactly as `sift fetch` does. `cmd/sift/extractcmd.go`: `extractOpts` + `parseExtractOpts` (reuses `splitAroundPositional` with `extractValueFlags` mode/pages/grep/question, one positional file, `--json`), `extractURL` builds `base+"/v1/extract"` with `url.Values` set only for the options that are set and `mode=digest` when a question is given and no mode — values are URL-encoded so a grep like `a b&c` survives round-trip; `extractMain` reuses `loadClient`, `stat`s the file and refuses >100 MiB before uploading (exit 3, matching the server's 413→exitFor), then `http.NewRequest` with the `*os.File` as the streaming body and `ContentLength` set (no second in-memory copy), `Content-Type: application/pdf` + bearer, and the same error/exitFor path as fetch; success prints `content` (or the `--json` envelope). `main.go`: `case "extract"` and one help line. README: extract line in the CLI block plus a sentence; SKILL.md: a PDF-on-disk bullet in the PDFs section. Given test byte-identical; all cmd/sift tests pass under `-race -count=3`; make gate ok; `bin/sift extract` prints the usage line. | | p6/01-batchstore | 2026-09-26 | done | 1 | gate: ok | none | Storage-only batch store in `internal/dcache` (new `batch.go`; schema added to `Open`; `BatchH`/`BatchM` counters added to `DB`). `CreateBatchJob` stores as given. Reads never write the lease: `BatchJob`, `BatchTargets` and the counts are pure, so exact lease values hold after a read. `BatchJobByIDem` returns the newest `(caller,idemKey)` job with `lease+ttl>now` and status queued/running/done/partial; failed/cancelled never reused. `PutBatchTargets` inserts in order; `BatchTargets` returns idx order with NULLs as nil pointers. `UpdateBatchTarget` bumps lease to `now`, increments `targets_done` exactly once per target entering done/failed (previous-status check prevents re-write double-count), and adds `len(result)` to `nbytes` when a result is written — all in one tx. `SetBatchJobStatus`/`TouchBatchJob` set lease; `SweepBatch` deletes by `lease+ttl<=now` with its targets and returns the job count; `EvictBatchOldest` evicts whole non-live jobs oldest-lease-first (never a queued/running one) until `BatchNbytes()<=maxBytes`; `MarkInFlightFailed` fails queued/running on restart; `BatchNbytes` sums job nbytes, kept out of `Size()`. Accessors return `(v,bool)`/`error`, count misses in `BatchM`, reuse `ClassFleet`/`ClassRestricted`. A first pass failed `TestBatchIDemLookup` because I forgot `.Scan(&id)` on the `QueryRow` (the un-scanned `*sql.Row` was always non-nil, so the method always missed); fixed and verified. All dcache tests pass under `-race -count=3`; make gate ok; given file byte-identical. -| p6/02-runner-core | 2026-09-26 | done | 1 | gate: ok | caller-cleanup hand-off to task 03 | Runner core in new internal/batch with no workers (task 03 adds them; jobs stay queued, so a queued job counts as live for the caps). Public surface in batch.go: Options/Target/Result/TargetError, FetchFunc, Job/TargetStatus, Hooks and Config with defaults (TTL 24h, 32 targets/job, 1 job/caller, 8 total, worker clamped to 2, 64 MiB, 2 s/10 m retry). Submit runs the given order — empty→ErrNoTargets, idempotency via hex SHA-256 of `name+NUL+json.Marshal(targets)` (a live hit returns the id reused, touching no caps), then TargetsPerJob/JobsPerCaller/JobsTotal caps, then stores the job queued (random 16-byte hex id, CreatedAt=Lease=now) and its targets (pending, Options=json of t.Options) before returning so a queued job is live; the submitting caller is held in memory by job id, never persisted. Status maps rows and decodes Result without writing; Touch moves the lease; Cancel sets a queued job cancelled at once and leaves its targets pending; Close is idempotent. Store errors are Warn-logged with the job id, never a URL/host. | | +| p6/02-runner-core | 2026-09-26 | done | 1 | gate: ok | caller-cleanup hand-off to task 03 | Runner core in new internal/batch with no workers (task 03 adds them; jobs stay queued, so a queued job counts as live for the caps). Public surface in batch.go: Options/Target/Result/TargetError, FetchFunc, Job/TargetStatus, Hooks and Config with defaults (TTL 24h, 32 targets/job, 1 job/caller, 8 total, worker clamped to 2, 64 MiB, 2 s/10 m retry). Submit runs the given order — empty→ErrNoTargets, idempotency via hex SHA-256 of `name+NUL+json.Marshal(targets)` (a live hit returns the id reused, touching no caps), then TargetsPerJob/JobsPerCaller/JobsTotal caps, then stores the job queued (random 16-byte hex id, CreatedAt=Lease=now) and its targets (pending, Options=json of t.Options) before returning so a queued job is live; the submitting caller is held in memory by job id, never persisted. Status maps rows and decodes Result without writing; Touch moves the lease; Cancel sets a queued job cancelled at once and leaves its targets pending; Close is idempotent. Store errors are Warn-logged with the job id, never a URL/host. | +| p6/03-runner-workers | 2026-09-26 | done | 1 | gate: ok | none | Workers in new internal/batch/worker.go: one rotation of live jobs in submit order, round-robin via an absolute cursor (advances in pick, wraps at access, so it survives a second job being added while the first is in flight — the mod-n cursor from the design stuck at 0 for one job and re-picked it), serial per job (target i+1 never starts before i is stored), one target per fetch under the Close-cancelled context (panic→ErrInternal, never echoed; ErrRetryLater retried after RetryWait, bounded by RetryFor then stored failed with the wrapped text). Terminal status once per job: all done→done, all failed→failed, mix→partial. Cancel of a running job sets cancelled, stores the in-flight target without overwriting cancelled, leaves later targets pending. Close sets closed, cancels the context, broadcasts, waits the workers, idempotent. batch.go: New starts cfg.Workers, Submit appends the liveJob and wakes, Cancel handles the in-flight case, Close stops. Absolute cursor is the one deviation from the design's "index in live" comment. All race tests pass -count=3. | | ## P5a review (owner, 2026-09-26) diff --git a/internal/batch/batch.go b/internal/batch/batch.go index 51ed5d5..bddc3d2 100644 --- a/internal/batch/batch.go +++ b/internal/batch/batch.go @@ -121,6 +121,16 @@ type Runner struct { callers map[string]*auth.Caller // job id → submitting caller, held in memory only callMu sync.Mutex closeOnce sync.Once + + 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 + closed bool + ctx context.Context // the FetchFunc context; Close cancels it + stop context.CancelFunc + wg sync.WaitGroup // the workers + inflight int } func New(db *dcache.DB, fetch FetchFunc, cfg Config, log *slog.Logger) *Runner { @@ -151,13 +161,22 @@ func New(db *dcache.DB, fetch FetchFunc, cfg Config, log *slog.Logger) *Runner { if cfg.RetryFor == 0 { cfg.RetryFor = 10 * time.Minute } - return &Runner{ + 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, } + r.cond = sync.NewCond(&r.mu) + r.wg.Add(cfg.Workers) + for i := 0; i < cfg.Workers; i++ { + go r.worker() + } + return r } // Submit stores the job and its targets before it returns. The order matters: an empty @@ -229,14 +248,52 @@ func (r *Runner) Submit(ctx context.Context, c *auth.Caller, callerClass string, r.callers[id] = c r.callMu.Unlock() + r.mu.Lock() + r.live = append(r.live, &liveJob{ + id: id, + caller: c, + targets: targets, + created: time.Unix(now, 0), + }) + r.callDepth(len(r.live)) + r.cond.Broadcast() + r.mu.Unlock() + r.callJob("created") r.db.SweepBatch(now, r.cfg.TTL) return id, false, nil } -// Cancel sets a queued job cancelled at once; its pending targets stay pending. It is -// false for an unknown id or a job already terminal. +// Cancel cancels a job. A job still in the rotation (queued or with a target in flight) is marked +// cancelled at once: its in-flight target, if any, finishes and is stored without overwriting +// cancelled, and no further target starts. A job not in the rotation (unknown or already terminal) +// keeps task 02's behaviour. func (r *Runner) Cancel(id string) bool { + r.mu.Lock() + var j *liveJob + for _, lj := range r.live { + if lj.id == id { + j = lj + break + } + } + if j != nil { + 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) + } + r.callJob("cancelled") + if !j.running { + r.removeLocked(j) + r.forgetCaller(id) + r.callDepth(len(r.live)) + } + r.cond.Broadcast() + r.mu.Unlock() + return true + } + r.mu.Unlock() + jb, ok := r.db.BatchJob(id) if !ok || isJobTerminal(jb.Status) { return false @@ -298,13 +355,20 @@ func (r *Runner) Touch(id string) { } } -// Close stops the workers and waits for an in-flight fetch to return. Safe to call more than -// once. With no workers (task 02) there is nothing to stop; the in-memory callers are dropped. +// 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. 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() + r.cond.Broadcast() + r.mu.Unlock() + r.wg.Wait() }) } diff --git a/internal/batch/runner_test.go b/internal/batch/runner_test.go new file mode 100644 index 0000000..7d71b21 --- /dev/null +++ b/internal/batch/runner_test.go @@ -0,0 +1,373 @@ +package batch + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "testing" + "time" + + "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/dcache" +) + +// Targets run one at a time, in order, and each fetch is given the submitting caller. +func TestSerialOrderAndCaller(t *testing.T) { + var mu sync.Mutex + var calls, names []string + inflight, peak := 0, 0 + fetch := func(_ context.Context, c *auth.Caller, tgt Target) (Result, error) { + mu.Lock() + inflight++ + if inflight > peak { + peak = inflight + } + calls = append(calls, tgt.URL) + names = append(names, c.Name) + mu.Unlock() + time.Sleep(time.Millisecond) + mu.Lock() + inflight-- + mu.Unlock() + return Result{Content: "ok " + tgt.URL}, nil + } + r := newRunner(t, fetch, Config{}) + id, reused, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b", "c")) + if err != nil || reused { + t.Fatalf("submit err=%v reused=%v", err, reused) + } + waitStatus(t, r, id, "done") + mu.Lock() + defer mu.Unlock() + if strings.Join(calls, " ") != "https://a.example/ https://b.example/ https://c.example/" { + t.Fatalf("order %v", calls) + } + if peak != 1 { + t.Fatalf("peak inflight %d, want 1 (serial)", peak) + } + for _, n := range names { + if n != "alice" { + t.Fatalf("fetch saw caller %q, want alice", n) + } + } + j, _, _ := r.Status(id) + if j.Caller != "alice" || j.TargetsTotal != 3 || j.TargetsDone != 3 || j.CreatedAt == 0 { + t.Fatalf("job %+v", j) + } +} + +// With one worker, two live jobs take turns target by target. +func TestRoundRobin(t *testing.T) { + first := make(chan struct{}) + release := make(chan struct{}) + var once sync.Once + var mu sync.Mutex + var calls []string + fetch := func(_ context.Context, _ *auth.Caller, tgt Target) (Result, error) { + mu.Lock() + calls = append(calls, strings.TrimSuffix(strings.TrimPrefix(tgt.URL, "https://"), ".example/")) + mu.Unlock() + once.Do(func() { close(first); <-release }) + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{Workers: 1}) + a, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a1", "a2", "a3")) + if err != nil { + t.Fatal(err) + } + <-first // a1 is in flight + b, _, err := r.Submit(context.Background(), bob, dcache.ClassFleet, urls("b1", "b2", "b3")) + if err != nil { + t.Fatal(err) + } + close(release) + waitStatus(t, r, a, "done") + waitStatus(t, r, b, "done") + mu.Lock() + defer mu.Unlock() + if got := strings.Join(calls, " "); got != "a1 b1 a2 b2 a3 b3" { + t.Fatalf("order %q, want round-robin a1 b1 a2 b2 a3 b3", got) + } +} + +func TestFailureIsolation(t *testing.T) { + fetch := func(_ context.Context, _ *auth.Caller, tgt Target) (Result, error) { + if tgt.URL == "https://b.example/" { + return Result{}, &TargetError{Msg: "upstream 500", Hint: "browser"} + } + return Result{Content: "ok " + tgt.URL}, nil + } + r := newRunner(t, fetch, Config{}) + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b", "c")) + if err != nil { + t.Fatal(err) + } + waitStatus(t, r, id, "partial") + j, ts, ok := r.Status(id) + if !ok || j.TargetsDone != 3 { + t.Fatalf("job %+v ok=%v", j, ok) + } + if ts[0].Status != "done" || ts[1].Status != "failed" || ts[2].Status != "done" { + t.Fatalf("statuses %+v", ts) + } + if ts[1].Error != "upstream 500" || ts[1].Hint != "browser" { + t.Fatalf("failed target error=%q hint=%q", ts[1].Error, ts[1].Hint) + } + if ts[0].Result == nil || ts[0].Result.Content != "ok https://a.example/" { + t.Fatalf("result %+v", ts[0].Result) + } +} + +func TestAllFailedIsFailed(t *testing.T) { + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { return Result{}, errors.New("nope") } + r := newRunner(t, fetch, Config{}) + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil { + t.Fatal(err) + } + waitStatus(t, r, id, "failed") + _, ts, _ := r.Status(id) + if ts[0].Error != "nope" || ts[0].Hint != "" { + t.Fatalf("plain error stored as error=%q hint=%q", ts[0].Error, ts[0].Hint) + } +} + +func TestIdempotentResubmit(t *testing.T) { + var mu sync.Mutex + n := 0 + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { + mu.Lock() + n++ + mu.Unlock() + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{}) + id1, reused1, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil || reused1 { + t.Fatalf("first submit err=%v reused=%v", err, reused1) + } + id2, reused2, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil || !reused2 || id2 != id1 { + t.Fatalf("resubmit id=%q reused=%v err=%v", id2, reused2, err) + } + waitStatus(t, r, id1, "done") + // Still reused once done (within the lease), and never re-fetched. + if id3, reused3, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")); err != nil || !reused3 || id3 != id1 { + t.Fatalf("resubmit after done id=%q reused=%v err=%v", id3, reused3, err) + } + // Another caller with the same list gets its own job. + if id4, reused4, err := r.Submit(context.Background(), bob, dcache.ClassFleet, urls("a", "b")); err != nil || reused4 || id4 == id1 { + t.Fatalf("other caller id=%q reused=%v err=%v", id4, reused4, err) + } + time.Sleep(50 * time.Millisecond) + mu.Lock() + defer mu.Unlock() + if n != 4 { + t.Fatalf("fetched %d times, want 4 (2 for alice's one job, 2 for bob's)", n) + } +} + +// A cancelled job is not handed back to a resubmit: the same list starts a new job. +func TestResubmitAfterCancelIsNew(t *testing.T) { + release := make(chan struct{}) + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { + <-release + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{}) + id1, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil { + t.Fatal(err) + } + waitStatus(t, r, id1, "running") + if !r.Cancel(id1) { + t.Fatal("cancel returned false") + } + close(release) + waitStatus(t, r, id1, "cancelled") + id2, reused, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + if err != nil || reused || id2 == id1 { + t.Fatalf("resubmit after cancel id=%q reused=%v err=%v", id2, reused, err) + } +} + +// Submit returns with the job stored "queued"; it becomes "running" when a worker takes it. +func TestQueuedThenRunning(t *testing.T) { + release := make(chan struct{}) + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { + <-release + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{Workers: 1}) + defer close(release) + a, _, _ := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a")) + waitStatus(t, r, a, "running") + b, _, err := r.Submit(context.Background(), bob, dcache.ClassFleet, urls("b")) + if err != nil { + t.Fatal(err) + } + // The only worker is busy with alice's target, so bob's job waits in the queue. + time.Sleep(20 * time.Millisecond) + if j, _, _ := r.Status(b); j.Status != "queued" { + t.Fatalf("bob's job is %q while the only worker is busy, want queued", j.Status) + } +} + +func TestCancel(t *testing.T) { + started := make(chan struct{}, 1) + release := make(chan struct{}) + var mu sync.Mutex + var calls []string + fetch := func(_ context.Context, _ *auth.Caller, tgt Target) (Result, error) { + mu.Lock() + calls = append(calls, tgt.URL) + mu.Unlock() + select { + case started <- struct{}{}: + default: + } + <-release + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{}) + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b", "c")) + if err != nil { + t.Fatal(err) + } + <-started // the first target is in flight + if !r.Cancel(id) { + t.Fatal("cancel returned false") + } + close(release) // let the in-flight target finish + waitStatus(t, r, id, "cancelled") + time.Sleep(20 * time.Millisecond) + mu.Lock() + n := len(calls) + mu.Unlock() + if n != 1 { + t.Fatalf("cancelled after %d fetches, want 1", n) + } + j, ts, ok := r.Status(id) + if !ok || j.Status != "cancelled" { + t.Fatalf("job %+v: the in-flight target's write must not overwrite cancelled", j) + } + if ts[0].Status != "done" { + t.Fatalf("in-flight target should be stored done: %+v", ts[0]) + } + if ts[1].Status != "pending" || ts[2].Status != "pending" { + t.Fatalf("later targets must stay pending: %+v", ts[1:]) + } + if r.Cancel(id) { + t.Fatal("cancelling a terminal job must return false") + } + if r.Cancel("no-such-job") { + t.Fatal("cancelling an unknown job must return false") + } +} + +// A target that reports ErrRetryLater (the fetch gate was busy) is retried, not failed. +func TestBusyIsRetried(t *testing.T) { + var mu sync.Mutex + n := 0 + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { + mu.Lock() + defer mu.Unlock() + n++ + if n <= 2 { + return Result{}, fmt.Errorf("busy: %w", ErrRetryLater) + } + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{RetryWait: time.Millisecond}) + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a")) + if err != nil { + t.Fatal(err) + } + waitStatus(t, r, id, "done") + mu.Lock() + defer mu.Unlock() + if n != 3 { + t.Fatalf("fetched %d times, want 3 (two busy, then ok)", n) + } +} + +// Retrying is bounded: a target busy for longer than RetryFor fails with a busy error. +func TestBusyGivesUp(t *testing.T) { + fetch := func(context.Context, *auth.Caller, Target) (Result, error) { + return Result{}, fmt.Errorf("busy: %w", ErrRetryLater) + } + r := newRunner(t, fetch, Config{RetryWait: time.Millisecond, RetryFor: 30 * time.Millisecond}) + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a")) + if err != nil { + t.Fatal(err) + } + waitStatus(t, r, id, "failed") + _, ts, _ := r.Status(id) + if ts[0].Status != "failed" || !strings.Contains(ts[0].Error, "busy") { + t.Fatalf("target %+v, want failed with a busy error", ts[0]) + } +} + +func TestPanicRecovered(t *testing.T) { + fetch := func(_ context.Context, _ *auth.Caller, tgt Target) (Result, error) { + if tgt.URL == "https://b.example/" { + panic("boom") + } + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{}) + id, _, err := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b", "c")) + if err != nil { + t.Fatal(err) + } + waitStatus(t, r, id, "partial") + _, ts, _ := r.Status(id) + if ts[0].Status != "done" || ts[1].Status != "failed" || ts[2].Status != "done" { + t.Fatalf("statuses %+v", ts) + } + if ts[1].Error == "" || strings.Contains(ts[1].Error, "boom") { + t.Fatalf("panic target error %q: must be set, and must not echo the panic value", ts[1].Error) + } +} + +// The hooks see every job outcome once and every stored target once. +func TestHooks(t *testing.T) { + var mu sync.Mutex + jobs := map[string]int{} + var targets, jobSecs int + var bytes int + h := Hooks{ + Job: func(o string) { mu.Lock(); jobs[o]++; mu.Unlock() }, + Target: func(err error, _ time.Duration, n int) { + mu.Lock() + targets++ + bytes += n + mu.Unlock() + }, + JobSeconds: func(time.Duration) { mu.Lock(); jobSecs++; mu.Unlock() }, + } + fetch := func(_ context.Context, _ *auth.Caller, tgt Target) (Result, error) { + if tgt.URL == "https://b.example/" { + return Result{}, errors.New("nope") + } + return Result{Content: "ok"}, nil + } + r := newRunner(t, fetch, Config{Hooks: h}) + id, _, _ := r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) + waitStatus(t, r, id, "partial") + _, _, _ = r.Submit(context.Background(), alice, dcache.ClassFleet, urls("a", "b")) // reused + time.Sleep(20 * time.Millisecond) + mu.Lock() + defer mu.Unlock() + if jobs["created"] != 1 || jobs["partial"] != 1 || jobs["reused"] != 1 || len(jobs) != 3 { + t.Fatalf("job outcomes %v", jobs) + } + if targets != 2 || jobSecs != 1 { + t.Fatalf("targets=%d jobSeconds=%d, want 2 and 1", targets, jobSecs) + } + if bytes <= 0 { + t.Fatalf("stored result bytes %d, want > 0 for the done target", bytes) + } +} diff --git a/internal/batch/worker.go b/internal/batch/worker.go new file mode 100644 index 0000000..5cb64df --- /dev/null +++ b/internal/batch/worker.go @@ -0,0 +1,271 @@ +package batch + +import ( + "context" + "encoding/json" + "errors" + "time" + + "git.wntrmute.dev/kyle/sift/internal/auth" +) + +// ErrInternal is stored for a target whose fetch panicked; the panic value is never echoed. +var ErrInternal = errors.New("internal error") + +// liveJob is a submitted job that the workers are draining. It lives in Runner.live until the +// last target is stored or the job is cancelled. +type liveJob struct { + id string + caller *auth.Caller + targets []Target // as submitted + created time.Time + next int // index of the next target to fetch + running bool // a target of this job is in flight + started bool // set to "running" in the store already + cancelled bool + retryAt time.Time // zero, or when the busy target may be tried again + busySince time.Time // zero, or when the current target first came back busy + done, failed int +} + +// worker drains the rotation: it takes the next live job with a pending target and no target in +// flight, fetches and stores that one target, and moves on. Each job is serial — its target i+1 +// never starts before i is stored. +func (r *Runner) worker() { + defer r.wg.Done() + + for { + r.mu.Lock() + 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() + continue + } + + // Take the target: mark the job running and bump the in-flight count under the lock, + // then fetch outside it. + j.running = true + r.inflight++ + r.callInflight(r.inflight) + if !j.started { + if err := r.db.SetBatchJobStatus(j.id, "running", "", time.Now().Unix()); err != nil { + r.log.Warn("batch: set running", "job", j.id, "err", err) + } + j.started = true + } + idx := j.next + target := j.targets[idx] + startedAt := time.Now() + r.mu.Unlock() + + res, err := r.runTarget(r.ctx, j, idx, target) + + r.mu.Lock() + j.running = false + r.inflight-- + r.callInflight(r.inflight) + + // 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) { + if j.busySince.IsZero() { + j.busySince = now + } + if now.Sub(j.busySince) <= r.cfg.RetryFor { + j.retryAt = now.Add(r.cfg.RetryWait) + r.cond.Broadcast() + r.mu.Unlock() + continue + } + } + + resultBytes := r.storeTarget(j, idx, res, err) + j.next++ + j.busySince, j.retryAt = time.Time{}, time.Time{} + if err == nil { + j.done++ + } else { + j.failed++ + } + r.callTarget(err, time.Since(startedAt), resultBytes) + r.db.EvictBatchOldest(r.cfg.MaxBytes) + + if j.cancelled || j.next >= len(j.targets) { + if !j.cancelled { + r.setTerminal(j) + } + r.removeLocked(j) + r.forgetCaller(j.id) + r.callDepth(len(r.live)) + } + r.cond.Broadcast() + r.mu.Unlock() + } +} + +// pick returns the first live job, scanning from cursor and wrapping once, that has a pending +// target that is not running, not cancelled, and ready (now past any retry). It advances cursor to +// the index after the job, so the rotation is round-robin across jobs in submit order. +func (r *Runner) pick(now time.Time) *liveJob { + n := len(r.live) + if n == 0 { + return nil + } + for i := 0; i < n; i++ { + j := r.live[(r.cursor+i)%n] + if j.running || j.cancelled || j.next >= len(j.targets) { + 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 + return j + } + } + return nil +} + +// scheduleRetryLocked arms a timer that wakes the workers at the earliest future retryAt, so they +// do not sleep past a busy target that is ready to be tried again. +func (r *Runner) scheduleRetryLocked(now time.Time) { + var earliest time.Time + for _, j := range r.live { + if j.cancelled || j.next >= len(j.targets) || !j.retryAt.After(now) { + continue + } + if earliest.IsZero() || j.retryAt.Before(earliest) { + earliest = j.retryAt + } + } + if !earliest.IsZero() { + d := time.Until(earliest) + if d < 0 { + d = 0 + } + time.AfterFunc(d, r.cond.Broadcast) + } +} + +// removeLocked drops j from live and fixes cursor so the rotation still points at the next job. +func (r *Runner) removeLocked(j *liveJob) { + idx := -1 + for i := range r.live { + if r.live[i] == j { + idx = i + break + } + } + if idx < 0 { + return + } + r.live = append(r.live[:idx], r.live[idx+1:]...) + if r.cursor > idx { + r.cursor-- + } +} + +// setTerminal writes the job's finished status once: all done → done, all failed → failed, a mix +// → partial. +func (r *Runner) setTerminal(j *liveJob) { + var status string + switch { + case j.failed == 0: + status = "done" + case j.done == 0: + status = "failed" + default: + status = "partial" + } + if err := r.db.SetBatchJobStatus(j.id, status, "", time.Now().Unix()); err != nil { + r.log.Warn("batch: set terminal status", "job", j.id, "err", err) + } + r.callJob(status) + r.callJobSeconds(time.Since(j.created)) +} + +// runTarget fetches one target, recovering a panic into ErrInternal (never echoing the value). +func (r *Runner) runTarget(ctx context.Context, j *liveJob, idx int, t Target) (res Result, err error) { + defer func() { + if rec := recover(); rec != nil { + r.log.Warn("batch: recover fetch panic", "job", j.id, "idx", idx) + res = Result{} + err = ErrInternal + } + }() + return r.fetch(ctx, j.caller, t) +} + +// storeTarget stores one target: success → done with the result JSON, error → failed with the +// TargetError's Msg/Hint or the error's text. It returns the number of result bytes stored. +func (r *Runner) storeTarget(j *liveJob, idx int, res Result, err error) int { + now := time.Now().Unix() + status := "failed" + var resultStr, errMsg, hint *string + if err == nil { + status = "done" + b, merr := json.Marshal(res) + if merr != nil { + r.log.Warn("batch: marshal result", "job", j.id, "idx", idx, "err", merr) + } else { + s := string(b) + resultStr = &s + } + } else { + var terr *TargetError + if errors.As(err, &terr) { + m := terr.Msg + errMsg = &m + if terr.Hint != "" { + h := terr.Hint + hint = &h + } + } else { + e := err.Error() + errMsg = &e + } + } + if serr := r.db.UpdateBatchTarget(j.id, idx, now, status, resultStr, errMsg, hint); serr != nil { + r.log.Warn("batch: store target", "job", j.id, "idx", idx, "err", serr) + } + if resultStr != nil { + return len(*resultStr) + } + 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) + } +} + +func (r *Runner) callInflight(n int) { + if r.cfg.Hooks.Inflight != nil { + r.cfg.Hooks.Inflight(n) + } +} + +func (r *Runner) callTarget(err error, d time.Duration, resultBytes int) { + if r.cfg.Hooks.Target != nil { + r.cfg.Hooks.Target(err, d, resultBytes) + } +} + +func (r *Runner) callJobSeconds(d time.Duration) { + if r.cfg.Hooks.JobSeconds != nil { + r.cfg.Hooks.JobSeconds(d) + } +} From e0168f7d652db042b6f334f3931cd5cb741999f8 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 22:21:59 -0700 Subject: [PATCH 04/10] metrics: sift_batch_* series (jobs, targets, depth, in-flight, durations, bytes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New internal/metrics/batch.go adds the batch series to metrics.Set, registered in New: BatchJobsTotal and BatchTargetsTotal counters (label "outcome", any value outside the known set folded to "other" so a raw string never becomes a label), BatchQueueDepth and BatchTargetsInflight gauges, BatchJobSeconds and BatchTargetSeconds histograms (the existing request buckets) and BatchResultBytes (1K,4K,16K,64K,256K,1M). The Observe…/Set… methods are nil-receiver safe like ObservePDF and flow through the existing /metrics handler; no new endpoint or Snapshot change. Co-Authored-By: ornith-1.5-35b-a3b --- docs/implementer-log.md | 1 + internal/metrics/batch.go | 80 ++++++++++++++++++++++++++++++++ internal/metrics/batch_test.go | 61 ++++++++++++++++++++++++ internal/metrics/metrics.go | 85 ++++++++++++++++++++++++++-------- 4 files changed, 208 insertions(+), 19 deletions(-) create mode 100644 internal/metrics/batch.go create mode 100644 internal/metrics/batch_test.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 105bd40..e5460a3 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -20,6 +20,7 @@ One row per task session that ends in a commit. Newest last. | p6/01-batchstore | 2026-09-26 | done | 1 | gate: ok | none | Storage-only batch store in `internal/dcache` (new `batch.go`; schema added to `Open`; `BatchH`/`BatchM` counters added to `DB`). `CreateBatchJob` stores as given. Reads never write the lease: `BatchJob`, `BatchTargets` and the counts are pure, so exact lease values hold after a read. `BatchJobByIDem` returns the newest `(caller,idemKey)` job with `lease+ttl>now` and status queued/running/done/partial; failed/cancelled never reused. `PutBatchTargets` inserts in order; `BatchTargets` returns idx order with NULLs as nil pointers. `UpdateBatchTarget` bumps lease to `now`, increments `targets_done` exactly once per target entering done/failed (previous-status check prevents re-write double-count), and adds `len(result)` to `nbytes` when a result is written — all in one tx. `SetBatchJobStatus`/`TouchBatchJob` set lease; `SweepBatch` deletes by `lease+ttl<=now` with its targets and returns the job count; `EvictBatchOldest` evicts whole non-live jobs oldest-lease-first (never a queued/running one) until `BatchNbytes()<=maxBytes`; `MarkInFlightFailed` fails queued/running on restart; `BatchNbytes` sums job nbytes, kept out of `Size()`. Accessors return `(v,bool)`/`error`, count misses in `BatchM`, reuse `ClassFleet`/`ClassRestricted`. A first pass failed `TestBatchIDemLookup` because I forgot `.Scan(&id)` on the `QueryRow` (the un-scanned `*sql.Row` was always non-nil, so the method always missed); fixed and verified. All dcache tests pass under `-race -count=3`; make gate ok; given file byte-identical. | p6/02-runner-core | 2026-09-26 | done | 1 | gate: ok | caller-cleanup hand-off to task 03 | Runner core in new internal/batch with no workers (task 03 adds them; jobs stay queued, so a queued job counts as live for the caps). Public surface in batch.go: Options/Target/Result/TargetError, FetchFunc, Job/TargetStatus, Hooks and Config with defaults (TTL 24h, 32 targets/job, 1 job/caller, 8 total, worker clamped to 2, 64 MiB, 2 s/10 m retry). Submit runs the given order — empty→ErrNoTargets, idempotency via hex SHA-256 of `name+NUL+json.Marshal(targets)` (a live hit returns the id reused, touching no caps), then TargetsPerJob/JobsPerCaller/JobsTotal caps, then stores the job queued (random 16-byte hex id, CreatedAt=Lease=now) and its targets (pending, Options=json of t.Options) before returning so a queued job is live; the submitting caller is held in memory by job id, never persisted. Status maps rows and decodes Result without writing; Touch moves the lease; Cancel sets a queued job cancelled at once and leaves its targets pending; Close is idempotent. Store errors are Warn-logged with the job id, never a URL/host. | | p6/03-runner-workers | 2026-09-26 | done | 1 | gate: ok | none | Workers in new internal/batch/worker.go: one rotation of live jobs in submit order, round-robin via an absolute cursor (advances in pick, wraps at access, so it survives a second job being added while the first is in flight — the mod-n cursor from the design stuck at 0 for one job and re-picked it), serial per job (target i+1 never starts before i is stored), one target per fetch under the Close-cancelled context (panic→ErrInternal, never echoed; ErrRetryLater retried after RetryWait, bounded by RetryFor then stored failed with the wrapped text). Terminal status once per job: all done→done, all failed→failed, mix→partial. Cancel of a running job sets cancelled, stores the in-flight target without overwriting cancelled, leaves later targets pending. Close sets closed, cancels the context, broadcasts, waits the workers, idempotent. batch.go: New starts cfg.Workers, Submit appends the liveJob and wakes, Cancel handles the in-flight case, Close stops. Absolute cursor is the one deviation from the design's "index in live" comment. All race tests pass -count=3. | | +| p6/04-metrics | 2026-09-26 | done | 1 | gate: ok | none | Batch series in new internal/metrics/batch.go, registered in New: `BatchJobsTotal`/`BatchTargetsTotal` CounterVecs (label "outcome", any value outside the known set folded to "other" via `batchJobOutcomes`/`batchTargetOutcomes` maps so a raw string never becomes a label), `BatchQueueDepth`/`BatchTargetsInflight` gauges, and `BatchJobSeconds`/`BatchTargetSeconds` histograms (durationBuckets) plus `BatchResultBytes` (1K,4K,16K,64K,256K,1M). Nil-receiver-safe `Observe…`/`Set…` methods mirroring ObservePDF/ObserveDigest. Series served through the existing /metrics handler; no new endpoint, no Snapshot change. Given test byte-identical; metrics tests pass under -race -count=3; make gate ok. | ## P5a review (owner, 2026-09-26) diff --git a/internal/metrics/batch.go b/internal/metrics/batch.go new file mode 100644 index 0000000..f830456 --- /dev/null +++ b/internal/metrics/batch.go @@ -0,0 +1,80 @@ +package metrics + +import "time" + +var batchResultBytesBuckets = []float64{1024, 4096, 16384, 65536, 262144, 1048576} + +var batchJobOutcomes = map[string]struct{}{ + "created": {}, "reused": {}, "done": {}, "partial": {}, + "failed": {}, "cancelled": {}, "restart": {}, +} + +var batchTargetOutcomes = map[string]struct{}{ + "ok": {}, "blocked": {}, "timeout": {}, "busy": {}, + "upstream_4xx": {}, "upstream_5xx": {}, "unsupported": {}, + "too_long": {}, "cancel": {}, +} + +// ObserveBatchJob records one batch outcome. Values outside the known set fold +// to "other" so a raw string never becomes a label. +func (s *Set) ObserveBatchJob(outcome string) { + if s == nil { + return + } + if _, ok := batchJobOutcomes[outcome]; !ok { + outcome = "other" + } + s.BatchJobsTotal.WithLabelValues(outcome).Inc() +} + +// ObserveBatchTarget records one target outcome. Values outside the known set +// fold to "other" so a raw string never becomes a label. +func (s *Set) ObserveBatchTarget(outcome string) { + if s == nil { + return + } + if _, ok := batchTargetOutcomes[outcome]; !ok { + outcome = "other" + } + s.BatchTargetsTotal.WithLabelValues(outcome).Inc() +} + +// SetBatchQueueDepth sets the live job count gauge (queued+running). +func (s *Set) SetBatchQueueDepth(n int) { + if s == nil { + return + } + s.BatchQueueDepth.Set(float64(n)) +} + +// SetBatchTargetsInflight sets the in-flight targets gauge. +func (s *Set) SetBatchTargetsInflight(n int) { + if s == nil { + return + } + s.BatchTargetsInflight.Set(float64(n)) +} + +// ObserveBatchJobSeconds records the wall time from submit to a batch's terminal status. +func (s *Set) ObserveBatchJobSeconds(d time.Duration) { + if s == nil { + return + } + s.BatchJobSeconds.Observe(d.Seconds()) +} + +// ObserveBatchTargetSeconds records the wall time for one target fetch. +func (s *Set) ObserveBatchTargetSeconds(d time.Duration) { + if s == nil { + return + } + s.BatchTargetSeconds.Observe(d.Seconds()) +} + +// ObserveBatchResultBytes records the stored byte count of one target result. +func (s *Set) ObserveBatchResultBytes(n int) { + if s == nil { + return + } + s.BatchResultBytes.Observe(float64(n)) +} diff --git a/internal/metrics/batch_test.go b/internal/metrics/batch_test.go new file mode 100644 index 0000000..8e1cad3 --- /dev/null +++ b/internal/metrics/batch_test.go @@ -0,0 +1,61 @@ +package metrics + +import ( + "io" + "net/http/httptest" + "strings" + "testing" + "time" +) + +type batchSrc struct{} + +func (batchSrc) Snapshot() Snapshot { return Snapshot{} } + +func TestBatchSeries(t *testing.T) { + s := New(batchSrc{}) + s.ObserveBatchJob("created") + s.ObserveBatchJob("done") + s.ObserveBatchJob("done") + s.ObserveBatchJob("reused") + s.ObserveBatchJob("some raw string") // folded to "other", never a label + s.ObserveBatchTarget("ok") + s.ObserveBatchTarget("ok") + s.ObserveBatchTarget("blocked") + s.ObserveBatchTarget("upstream_5xx") + s.ObserveBatchTarget("leak me") // folded to "other" + s.SetBatchQueueDepth(3) + s.SetBatchTargetsInflight(1) + s.ObserveBatchJobSeconds(4 * time.Second) + s.ObserveBatchTargetSeconds(1500 * time.Millisecond) + s.ObserveBatchResultBytes(4096) + + req := httptest.NewRequest("GET", "/metrics", nil) + req.RemoteAddr, req.Host = "127.0.0.1:9", "127.0.0.1:8890" + rec := httptest.NewRecorder() + s.Handler().ServeHTTP(rec, req) + body, _ := io.ReadAll(rec.Body) + out := string(body) + for _, want := range []string{ + `sift_batch_jobs_total{outcome="created"} 1`, + `sift_batch_jobs_total{outcome="done"} 2`, + `sift_batch_jobs_total{outcome="reused"} 1`, + `sift_batch_jobs_total{outcome="other"} 1`, + `sift_batch_targets_total{outcome="ok"} 2`, + `sift_batch_targets_total{outcome="blocked"} 1`, + `sift_batch_targets_total{outcome="upstream_5xx"} 1`, + `sift_batch_targets_total{outcome="other"} 1`, + `sift_batch_queue_depth 3`, + `sift_batch_targets_inflight 1`, + `sift_batch_job_seconds_count 1`, + `sift_batch_target_seconds_count 1`, + `sift_batch_result_bytes_count 1`, + } { + if !strings.Contains(out, want) { + t.Errorf("metrics lack %q", want) + } + } + if strings.Contains(out, "raw") || strings.Contains(out, "leak") { + t.Error("a raw outcome string leaked into a label") + } +} diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 525edb2..e08dc0e 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -50,15 +50,22 @@ var pdfOutcomes = map[string]struct{}{ // Set is a private registry of sift_* series. type Set struct { - Reg *prometheus.Registry - Requests *prometheus.CounterVec - Duration *prometheus.HistogramVec - PDFTotal *prometheus.CounterVec - PDFSeconds prometheus.Histogram - PDFPages prometheus.Counter - PDFWait prometheus.Histogram - PDFCacheHits prometheus.Counter - handler http.Handler + Reg *prometheus.Registry + Requests *prometheus.CounterVec + Duration *prometheus.HistogramVec + PDFTotal *prometheus.CounterVec + PDFSeconds prometheus.Histogram + PDFPages prometheus.Counter + PDFWait prometheus.Histogram + PDFCacheHits prometheus.Counter + BatchJobsTotal *prometheus.CounterVec + BatchTargetsTotal *prometheus.CounterVec + BatchQueueDepth prometheus.Gauge + BatchTargetsInflight prometheus.Gauge + BatchJobSeconds prometheus.Histogram + BatchTargetSeconds prometheus.Histogram + BatchResultBytes prometheus.Histogram + handler http.Handler } func New(src Source) *Set { @@ -94,17 +101,57 @@ func New(src Source) *Set { Name: "sift_pdf_cache_hits_total", Help: "PDF answers served from the document cache.", }) - reg.MustRegister(requests, duration, pdfTotal, pdfSeconds, pdfPages, pdfWait, pdfCacheHits, &collector{src: src}) + batchJobs := prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "sift_batch_jobs_total", + Help: "Batch jobs by outcome.", + }, []string{"outcome"}) + batchTargets := prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "sift_batch_targets_total", + Help: "Batch targets by outcome.", + }, []string{"outcome"}) + batchQueueDepth := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "sift_batch_queue_depth", + Help: "Live batch jobs (queued+running).", + }) + batchTargetsInflight := prometheus.NewGauge(prometheus.GaugeOpts{ + Name: "sift_batch_targets_inflight", + Help: "Batch targets currently being fetched.", + }) + batchJobSeconds := prometheus.NewHistogram(prometheus.HistogramOpts{ + Name: "sift_batch_job_seconds", + Help: "Batch wall time from submit to terminal status.", + Buckets: durationBuckets, + }) + batchTargetSeconds := prometheus.NewHistogram(prometheus.HistogramOpts{ + Name: "sift_batch_target_seconds", + Help: "Batch target fetch wall time.", + Buckets: durationBuckets, + }) + batchResultBytes := prometheus.NewHistogram(prometheus.HistogramOpts{ + Name: "sift_batch_result_bytes", + Help: "Stored bytes per batch target result.", + Buckets: batchResultBytesBuckets, + }) + reg.MustRegister(requests, duration, pdfTotal, pdfSeconds, pdfPages, pdfWait, pdfCacheHits, + batchJobs, batchTargets, batchQueueDepth, batchTargetsInflight, + batchJobSeconds, batchTargetSeconds, batchResultBytes, &collector{src: src}) return &Set{ - Reg: reg, - Requests: requests, - Duration: duration, - PDFTotal: pdfTotal, - PDFSeconds: pdfSeconds, - PDFPages: pdfPages, - PDFWait: pdfWait, - PDFCacheHits: pdfCacheHits, - handler: promhttp.HandlerFor(reg, promhttp.HandlerOpts{EnableOpenMetrics: false}), + Reg: reg, + Requests: requests, + Duration: duration, + PDFTotal: pdfTotal, + PDFSeconds: pdfSeconds, + PDFPages: pdfPages, + PDFWait: pdfWait, + PDFCacheHits: pdfCacheHits, + BatchJobsTotal: batchJobs, + BatchTargetsTotal: batchTargets, + BatchQueueDepth: batchQueueDepth, + BatchTargetsInflight: batchTargetsInflight, + BatchJobSeconds: batchJobSeconds, + BatchTargetSeconds: batchTargetSeconds, + BatchResultBytes: batchResultBytes, + handler: promhttp.HandlerFor(reg, promhttp.HandlerOpts{EnableOpenMetrics: false}), } } From 0d923bc3662ba53e62b90a11d1345bddc9ae2e7c Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 22:34:56 -0700 Subject: [PATCH 05/10] api: v1fetch's per-target pipeline becomes fetchTarget (no behaviour change) Move v1fetch's inline per-target pipeline into a new internal/api/target.go method, `fetchTarget(ctx, c, t batch.Target) (batch.Result, error)`, so the batch worker can drive the same work. The body moves unchanged in order: question truncation, mode default/check, wantCtx, doFetch, the recover path, the PDF branch (render, pages/grep errors, question-chunk limit), the clean/raw branch, digest, context search, the fetch log line and the envelope. `content` is returned as Result.Content; every other envelope key goes in Result.Meta, never content. v1fetch keeps reqN/caller/decode/400, builds a batch.Target from the request, calls fetchTarget, writeFetchErr on error and writeJSON(w, 200, env) on success where env is a copy of Meta plus "content". doFetch takes context.Context instead of *http.Request; scrape passes r.Context(). No new behaviour, fields or JSON keys. Co-Authored-By: ornith-1.5-35b-a3b --- docs/implementer-log.md | 1 + internal/api/api.go | 245 ++++--------------------------- internal/api/fetchtarget_test.go | 94 ++++++++++++ internal/api/target.go | 232 +++++++++++++++++++++++++++++ 4 files changed, 352 insertions(+), 220 deletions(-) create mode 100644 internal/api/fetchtarget_test.go create mode 100644 internal/api/target.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index e5460a3..9dcdbfd 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -21,6 +21,7 @@ One row per task session that ends in a commit. Newest last. | p6/02-runner-core | 2026-09-26 | done | 1 | gate: ok | caller-cleanup hand-off to task 03 | Runner core in new internal/batch with no workers (task 03 adds them; jobs stay queued, so a queued job counts as live for the caps). Public surface in batch.go: Options/Target/Result/TargetError, FetchFunc, Job/TargetStatus, Hooks and Config with defaults (TTL 24h, 32 targets/job, 1 job/caller, 8 total, worker clamped to 2, 64 MiB, 2 s/10 m retry). Submit runs the given order — empty→ErrNoTargets, idempotency via hex SHA-256 of `name+NUL+json.Marshal(targets)` (a live hit returns the id reused, touching no caps), then TargetsPerJob/JobsPerCaller/JobsTotal caps, then stores the job queued (random 16-byte hex id, CreatedAt=Lease=now) and its targets (pending, Options=json of t.Options) before returning so a queued job is live; the submitting caller is held in memory by job id, never persisted. Status maps rows and decodes Result without writing; Touch moves the lease; Cancel sets a queued job cancelled at once and leaves its targets pending; Close is idempotent. Store errors are Warn-logged with the job id, never a URL/host. | | p6/03-runner-workers | 2026-09-26 | done | 1 | gate: ok | none | Workers in new internal/batch/worker.go: one rotation of live jobs in submit order, round-robin via an absolute cursor (advances in pick, wraps at access, so it survives a second job being added while the first is in flight — the mod-n cursor from the design stuck at 0 for one job and re-picked it), serial per job (target i+1 never starts before i is stored), one target per fetch under the Close-cancelled context (panic→ErrInternal, never echoed; ErrRetryLater retried after RetryWait, bounded by RetryFor then stored failed with the wrapped text). Terminal status once per job: all done→done, all failed→failed, mix→partial. Cancel of a running job sets cancelled, stores the in-flight target without overwriting cancelled, leaves later targets pending. Close sets closed, cancels the context, broadcasts, waits the workers, idempotent. batch.go: New starts cfg.Workers, Submit appends the liveJob and wakes, Cancel handles the in-flight case, Close stops. Absolute cursor is the one deviation from the design's "index in live" comment. All race tests pass -count=3. | | | p6/04-metrics | 2026-09-26 | done | 1 | gate: ok | none | Batch series in new internal/metrics/batch.go, registered in New: `BatchJobsTotal`/`BatchTargetsTotal` CounterVecs (label "outcome", any value outside the known set folded to "other" via `batchJobOutcomes`/`batchTargetOutcomes` maps so a raw string never becomes a label), `BatchQueueDepth`/`BatchTargetsInflight` gauges, and `BatchJobSeconds`/`BatchTargetSeconds` histograms (durationBuckets) plus `BatchResultBytes` (1K,4K,16K,64K,256K,1M). Nil-receiver-safe `Observe…`/`Set…` methods mirroring ObservePDF/ObserveDigest. Series served through the existing /metrics handler; no new endpoint, no Snapshot change. Given test byte-identical; metrics tests pass under -race -count=3; make gate ok. | +| p6/05-fetchtarget | 2026-09-26 | done | 1 | gate: ok | none | Extracted v1fetch's per-target pipeline into new `internal/api/target.go`: `fetchTarget(ctx, c, t batch.Target) (batch.Result, error)`. Body moved unchanged in order — question truncation to 500 runes, mode default/check, wantCtx, doFetch, recover path, PDF branch (render, pages/grep errors, question-chunk limit), clean/raw branch, digest, context search, the fetch log line, and the envelope. `content` is returned as `Result.Content`; every other envelope key (`mode`,`untrusted`,`source`,`warnings`,`sizes`,`timings`, and `pdf`/`digest`/`context` when present) goes in `Result.Meta`, never content. `v1fetch` keeps reqN/caller/decode/400, builds a `batch.Target` (`URL` + `Mode`/`Question`/`Pages`/`Grep`/`JQ`/`Context`/`Recover` into `Options`), calls `fetchTarget`, `writeFetchErr` on error and `writeJSON(w,200,env)` on success where `env` is a fresh copy of `Meta` plus `"content"` (a copy, so `len(env)==len(Meta)+1` holds and Meta is not mutated). The two `writeErr` failures become `*fetch.Error` with the same status/msg/hint: 400 "invalid url" (bad mode), 400 "raw mode does not apply to PDFs". `doFetch` takes `context.Context` instead of `*http.Request` (its three `r.Context()` → `ctx`); `scrape` passes `r.Context()`. Removed now-unused `fmt`/`unicode/utf8`/`recover` imports, added `batch`. All api tests pass under -race -count=3; make gate ok; given test byte-identical. | ## P5a review (owner, 2026-09-26) diff --git a/internal/api/api.go b/internal/api/api.go index 9c7d4b0..83c0769 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -6,7 +6,6 @@ import ( "encoding/hex" "encoding/json" "errors" - "fmt" "io" "log/slog" "net/http" @@ -16,9 +15,9 @@ import ( "sync" "sync/atomic" "time" - "unicode/utf8" "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/batch" "git.wntrmute.dev/kyle/sift/internal/clean" "git.wntrmute.dev/kyle/sift/internal/dcache" "git.wntrmute.dev/kyle/sift/internal/digest" @@ -29,7 +28,6 @@ import ( "git.wntrmute.dev/kyle/sift/internal/pdfdoc" "git.wntrmute.dev/kyle/sift/internal/pdfpool" "git.wntrmute.dev/kyle/sift/internal/pdftext" - "git.wntrmute.dev/kyle/sift/internal/recover" "git.wntrmute.dev/kyle/sift/internal/search" "git.wntrmute.dev/kyle/sift/internal/selfip" ) @@ -389,7 +387,7 @@ func (s *Server) scrape(w http.ResponseWriter, r *http.Request) { } } s.scrapeN.Add(1) - res, cl, doc, ferr := s.doFetch(r, c, req.URL, "", onlyMain) + res, cl, doc, ferr := s.doFetch(r.Context(), c, req.URL, "", onlyMain) if ferr != nil { s.writeFetchErr(w, c, req.URL, ferr) return @@ -449,222 +447,29 @@ func (s *Server) v1fetch(w http.ResponseWriter, r *http.Request) { writeErr(w, 400, "invalid url", "") return } - q := req.Question - if len([]rune(q)) > 500 { - rns := []rune(q) - q = string(rns[:500]) - } - mode := req.Mode - if mode == "" { - if q != "" { - mode = "digest" - } else { - mode = "clean" - } - } - if mode != "clean" && mode != "raw" && mode != "digest" { - writeErr(w, 400, "invalid url", "") - return - } - wantCtx := q != "" - if req.Context != nil { - wantCtx = *req.Context - } - - t0 := time.Now() - onlyMain := true - res, cl, doc, ferr := s.doFetch(r, c, req.URL, req.JQ, onlyMain) - var snap *recover.Result - if ferr != nil && req.Recover && recoverWorthy(ferr) { - sr, rerr := recover.Try(r.Context(), s.Fetch, req.URL) - if rerr == nil && sr != nil && sr.Res != nil { - res, cl, doc, ferr = s.cleanFetched(sr.Res, req.JQ, onlyMain) - snap = sr - } - } - if ferr != nil { - s.writeFetchErr(w, c, req.URL, ferr) - return - } - fetchMS := time.Since(t0).Milliseconds() - - if doc == nil && (req.Pages != "" || req.Grep != "") { - s.writeFetchErr(w, c, req.URL, &fetch.Error{Status: 400, Msg: "pages and grep apply to PDFs only"}) - return - } - - var docLen, pdfPages int - var content string - var pdfMap map[string]any - if doc != nil { - pdfPages = doc.Pages - if mode == "raw" { - writeErr(w, 400, "raw mode does not apply to PDFs", "") - return - } - rp, err := s.renderPDF(doc, pdfdoc.Options{Pages: req.Pages, Grep: req.Grep, OutCap: s.OutCap}) - if err != nil { - if errors.Is(err, pdfdoc.ErrBadPages) || errors.Is(err, pdfdoc.ErrBadGrep) { - s.writeFetchErr(w, c, req.URL, &fetch.Error{Status: 400, Msg: err.Error()}) - return - } - s.writeFetchErr(w, c, req.URL, err) - return - } - full := pdfdoc.Full(doc) - docLen = utf8.RuneCountInString(full) - if mode == "digest" { - content = full - } else { - content = rp.Content - } - pdfMap = map[string]any{ - "pages": doc.Pages, - "title": doc.Title, - "outline": doc.Outline, - "extracted": len(doc.Text), - "first": rp.First, - "last": rp.Last, - "map": rp.Map, - "grep_hits": rp.GrepHits, - "note": rp.Note, - } - } else { - content = cl.Markdown - if cl.Kind == "json" { - content = cl.HTML - } - if mode == "raw" { - content = string(res.Body) - if len(content) > s.OutCap { - content = clean.TruncateUTF8(content, s.OutCap) - cl.Trunc = true - cl.Warning = "truncated to output cap" - } - } - } - warn := []string{} - if cl.Warning != "" { - warn = append(warn, cl.Warning) - } - - var digMeta map[string]any - var digestMS int64 - if mode == "digest" { - if doc != nil { - n := len(digest.Split(pdfdoc.Full(doc), digest.ChunkTokens)) - limit := s.PDFMaxQuestionChunks - if limit == 0 { - limit = DefaultPDFMaxQuestionChunks - } - if n > limit { - s.writeFetchErr(w, c, req.URL, &fetch.Error{Status: 422, Msg: fmt.Sprintf("document too long for question (%d chunks); use grep or pages", n)}) - return - } - } - if s.Digest == nil { - warn = append(warn, "digest fell back to clean: unset") - mode = "clean" - content, warn = capDigestFallback(content, warn) - digMeta = map[string]any{"fallback": "unset"} - } else { - dr := s.Digest.Run(r.Context(), content, q, res.FinalURL, s.callerClass(c)) - digestMS = dr.DigestMS - if !dr.Cached { - s.Prom.ObserveDigest(time.Duration(dr.DigestMS) * time.Millisecond) - } - if dr.Fallback != "" { - warn = append(warn, "digest fell back to clean: "+dr.Fallback) - mode = "clean" - content, warn = capDigestFallback(content, warn) - digMeta = map[string]any{"fallback": dr.Fallback, "model": dr.Model} - } else { - content = dr.Text - digMeta = map[string]any{ - "model": dr.Model, - "chunks_used": dr.ChunksUsed, - "chunks_total": dr.ChunksTotal, - "retrieval": dr.Retrieval, - "embedded": dr.Embedded, - "fallback": false, - "cached": dr.Cached, - } - } - } - } - - var ctxItems []fleetctx.Item - var ctxMS int64 - if wantCtx && s.Ctx != nil { - ct0 := time.Now() - query := fleetctx.Query(q, cl.Title, headings(cl.Markdown)) - items, cw := s.Ctx.Search(r.Context(), c.Workspace, c.Name, query) - ctxMS = time.Since(ct0).Milliseconds() - if cw != "" { - warn = append(warn, cw) - } - ctxItems = items - } - - qLen := len(q) - if qLen > 80 { - qLen = 80 - } - fb, _ := "", false - if digMeta != nil { - if s, ok := digMeta["fallback"].(string); ok { - fb = s - } - } - s.Log.Info("fetch", "caller", c.Name, "host", s.logHost(c, res.FinalURL), "mode", mode, - "q_len", qLen, "raw_tokens", len(res.Body)/4, "returned_tokens", len(content)/4, - "pdf_pages", pdfPages, "digest_ms", digestMS, "fallback", fb) - - src := map[string]any{ - "url": res.URL, - "final_url": res.FinalURL, - "status": res.Status, - "content_type": res.ContentType, - "fetched_at": res.FetchedAt.Format(time.RFC3339), - "hash": res.SHA256(), - "bytes": len(res.Body), - "cached": len(res.Body) == 0, - "kind": "live", - } - if snap != nil { - src["kind"] = "snapshot" - src["via"] = string(snap.Via) - src["original_url"] = snap.OriginalURL - if snap.SnapshotAt != "" { - src["snapshot_at"] = snap.SnapshotAt - } else { - src["snapshot_at"] = nil - warn = append(warn, "snapshot date unknown") - } - } - out := map[string]any{ - "content": content, - "mode": mode, - "untrusted": true, - "source": src, - "warnings": warn, - "sizes": map[string]int{ - "raw_tokens": docLen / 4, - "clean_tokens": digest.EstTokens(cl.Markdown), - "returned_tokens": digest.EstTokens(content), + t := batch.Target{ + URL: req.URL, + Options: batch.Options{ + Mode: req.Mode, + Question: req.Question, + Pages: req.Pages, + Grep: req.Grep, + JQ: req.JQ, + Context: req.Context, + Recover: req.Recover, }, - "timings": map[string]int64{"fetch_ms": fetchMS, "digest_ms": digestMS, "context_ms": ctxMS}, } - if pdfMap != nil { - out["pdf"] = pdfMap + res, err := s.fetchTarget(r.Context(), c, t) + if err != nil { + s.writeFetchErr(w, c, req.URL, err) + return } - if digMeta != nil { - out["digest"] = digMeta + env := make(map[string]any, len(res.Meta)+1) + for k, v := range res.Meta { + env[k] = v } - if wantCtx { - out["context"] = ctxItems - } - writeJSON(w, 200, out) + env["content"] = res.Content + writeJSON(w, 200, env) } func recoverWorthy(err error) bool { @@ -710,8 +515,8 @@ func (s *Server) cleanFetched(res *fetch.Resource, jq string, onlyMain bool) (*f return res, cl, nil, nil } -func (s *Server) doFetch(r *http.Request, c *auth.Caller, rawURL, jq string, onlyMain bool) (*fetch.Resource, *clean.Result, *pdftext.Doc, error) { - res, hitDoc, ok, err := s.pdfFromCache(r.Context(), c, rawURL) +func (s *Server) doFetch(ctx context.Context, c *auth.Caller, rawURL, jq string, onlyMain bool) (*fetch.Resource, *clean.Result, *pdftext.Doc, error) { + res, hitDoc, ok, err := s.pdfFromCache(ctx, c, rawURL) if err != nil { return nil, nil, nil, err } @@ -724,7 +529,7 @@ func (s *Server) doFetch(r *http.Request, c *auth.Caller, rawURL, jq string, onl } defer s.release(c.Name, rawURL) - res, err = s.Fetch.Fetch(r.Context(), rawURL) + res, err = s.Fetch.Fetch(ctx, rawURL) if err != nil { return nil, nil, nil, err } @@ -756,7 +561,7 @@ func (s *Server) doFetch(r *http.Request, c *auth.Caller, rawURL, jq string, onl } var doc *pdftext.Doc if kind == "pdf" { - doc, err = s.extractPDF(r.Context(), c, res) + doc, err = s.extractPDF(ctx, c, res) if err != nil { return nil, nil, nil, err } diff --git a/internal/api/fetchtarget_test.go b/internal/api/fetchtarget_test.go new file mode 100644 index 0000000..e512ad2 --- /dev/null +++ b/internal/api/fetchtarget_test.go @@ -0,0 +1,94 @@ +package api + +// P6 task 05: v1fetch's per-target pipeline is extracted into s.fetchTarget, which the batch +// runner also calls. These tests pin that the two agree; every existing v1fetch test must also +// pass unchanged. + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/netip" + "strings" + "testing" + + "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/batch" + "git.wntrmute.dev/kyle/sift/internal/fetch" + "git.wntrmute.dev/kyle/sift/internal/guard" +) + +// newTargetServer is setupAPI with loopback allowed; its key belongs to caller "test". +func newTargetServer(t *testing.T) (*Server, string) { + t.Helper() + return setupAPI(t, &guard.Guard{AllowPrefixes: []netip.Prefix{netip.MustParsePrefix("127.0.0.0/8")}}) +} + +func TestFetchTargetMatchesV1Fetch(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html; charset=utf-8") + _, _ = w.Write([]byte(`Same

Same

one pipeline, two callers

`)) + })) + defer ts.Close() + s, key := newTargetServer(t) + c := &auth.Caller{Name: "test", Workspace: "hermes-fleet"} + + res, err := s.fetchTarget(context.Background(), c, batch.Target{URL: ts.URL}) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(res.Content, "one pipeline, two callers") { + t.Fatalf("content %q", res.Content) + } + if _, has := res.Meta["content"]; has { + t.Fatal("Meta must not repeat the content") + } + for _, k := range []string{"mode", "untrusted", "source", "warnings", "sizes", "timings"} { + if _, has := res.Meta[k]; !has { + t.Errorf("Meta lacks %q (it carries every envelope field except content)", k) + } + } + if res.Meta["mode"] != "clean" || res.Meta["untrusted"] != true { + t.Fatalf("meta %v", res.Meta) + } + + // The same URL through /v1/fetch: identical content and mode, and the envelope has exactly + // the fields of Meta plus content. + rec := postJSON(t, s, "/v1/fetch", key, map[string]any{"url": ts.URL}) + if rec.Code != 200 { + t.Fatalf("v1fetch %d %s", rec.Code, rec.Body.String()) + } + var env map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &env); err != nil { + t.Fatal(err) + } + if env["content"] != res.Content || env["mode"] != res.Meta["mode"] { + t.Fatalf("v1fetch and fetchTarget disagree:\n%v\n%+v", env, res) + } + if len(env) != len(res.Meta)+1 { + t.Fatalf("envelope keys %d, Meta keys %d: want Meta + content", len(env), len(res.Meta)) + } +} + +func TestFetchTargetErrors(t *testing.T) { + s, _ := newTargetServer(t) + c := &auth.Caller{Name: "test", Workspace: "hermes-fleet"} + cases := []struct { + name string + target batch.Target + status int + msg string + }{ + {"blocked", batch.Target{URL: "http://10.0.0.5/"}, 403, "blocked"}, + {"bad mode", batch.Target{URL: "http://127.0.0.1/", Options: batch.Options{Mode: "shout"}}, 400, ""}, + } + for _, tc := range cases { + _, err := s.fetchTarget(context.Background(), c, tc.target) + var fe *fetch.Error + if !errors.As(err, &fe) || fe.Status != tc.status || !strings.Contains(fe.Msg, tc.msg) { + t.Errorf("%s: err %v, want a *fetch.Error %d containing %q", tc.name, err, tc.status, tc.msg) + } + } +} diff --git a/internal/api/target.go b/internal/api/target.go new file mode 100644 index 0000000..415895e --- /dev/null +++ b/internal/api/target.go @@ -0,0 +1,232 @@ +package api + +import ( + "context" + "errors" + "fmt" + "time" + "unicode/utf8" + + "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/batch" + "git.wntrmute.dev/kyle/sift/internal/clean" + "git.wntrmute.dev/kyle/sift/internal/digest" + "git.wntrmute.dev/kyle/sift/internal/fetch" + "git.wntrmute.dev/kyle/sift/internal/fleetctx" + "git.wntrmute.dev/kyle/sift/internal/pdfdoc" + "git.wntrmute.dev/kyle/sift/internal/recover" +) + +// fetchTarget runs one target's per-target pipeline: fetch → recover → PDF render or clean → +// question limit → digest → context → log line → envelope. It is the single body both /v1/fetch +// and the batch worker drive, so it must not change what v1fetch returned. +func (s *Server) fetchTarget(ctx context.Context, c *auth.Caller, t batch.Target) (batch.Result, error) { + q := t.Options.Question + if len([]rune(q)) > 500 { + rns := []rune(q) + q = string(rns[:500]) + } + mode := t.Options.Mode + if mode == "" { + if q != "" { + mode = "digest" + } else { + mode = "clean" + } + } + if mode != "clean" && mode != "raw" && mode != "digest" { + return batch.Result{}, &fetch.Error{Status: 400, Msg: "invalid url"} + } + wantCtx := q != "" + if t.Options.Context != nil { + wantCtx = *t.Options.Context + } + + t0 := time.Now() + onlyMain := true + res, cl, doc, ferr := s.doFetch(ctx, c, t.URL, t.Options.JQ, onlyMain) + var snap *recover.Result + if ferr != nil && t.Options.Recover && recoverWorthy(ferr) { + sr, rerr := recover.Try(ctx, s.Fetch, t.URL) + if rerr == nil && sr != nil && sr.Res != nil { + res, cl, doc, ferr = s.cleanFetched(sr.Res, t.Options.JQ, onlyMain) + snap = sr + } + } + if ferr != nil { + return batch.Result{}, ferr + } + fetchMS := time.Since(t0).Milliseconds() + + if doc == nil && (t.Options.Pages != "" || t.Options.Grep != "") { + return batch.Result{}, &fetch.Error{Status: 400, Msg: "pages and grep apply to PDFs only"} + } + + var docLen, pdfPages int + var content string + var pdfMap map[string]any + if doc != nil { + pdfPages = doc.Pages + if mode == "raw" { + return batch.Result{}, &fetch.Error{Status: 400, Msg: "raw mode does not apply to PDFs"} + } + rp, err := s.renderPDF(doc, pdfdoc.Options{Pages: t.Options.Pages, Grep: t.Options.Grep, OutCap: s.OutCap}) + if err != nil { + if errors.Is(err, pdfdoc.ErrBadPages) || errors.Is(err, pdfdoc.ErrBadGrep) { + return batch.Result{}, &fetch.Error{Status: 400, Msg: err.Error()} + } + return batch.Result{}, err + } + full := pdfdoc.Full(doc) + docLen = utf8.RuneCountInString(full) + if mode == "digest" { + content = full + } else { + content = rp.Content + } + pdfMap = map[string]any{ + "pages": doc.Pages, + "title": doc.Title, + "outline": doc.Outline, + "extracted": len(doc.Text), + "first": rp.First, + "last": rp.Last, + "map": rp.Map, + "grep_hits": rp.GrepHits, + "note": rp.Note, + } + } else { + content = cl.Markdown + if cl.Kind == "json" { + content = cl.HTML + } + if mode == "raw" { + content = string(res.Body) + if len(content) > s.OutCap { + content = clean.TruncateUTF8(content, s.OutCap) + cl.Trunc = true + cl.Warning = "truncated to output cap" + } + } + } + warn := []string{} + if cl.Warning != "" { + warn = append(warn, cl.Warning) + } + + var digMeta map[string]any + var digestMS int64 + if mode == "digest" { + if doc != nil { + n := len(digest.Split(pdfdoc.Full(doc), digest.ChunkTokens)) + limit := s.PDFMaxQuestionChunks + if limit == 0 { + limit = DefaultPDFMaxQuestionChunks + } + if n > limit { + return batch.Result{}, &fetch.Error{Status: 422, Msg: fmt.Sprintf("document too long for question (%d chunks); use grep or pages", n)} + } + } + if s.Digest == nil { + warn = append(warn, "digest fell back to clean: unset") + mode = "clean" + content, warn = capDigestFallback(content, warn) + digMeta = map[string]any{"fallback": "unset"} + } else { + dr := s.Digest.Run(ctx, content, q, res.FinalURL, s.callerClass(c)) + digestMS = dr.DigestMS + if !dr.Cached { + s.Prom.ObserveDigest(time.Duration(dr.DigestMS) * time.Millisecond) + } + if dr.Fallback != "" { + warn = append(warn, "digest fell back to clean: "+dr.Fallback) + mode = "clean" + content, warn = capDigestFallback(content, warn) + digMeta = map[string]any{"fallback": dr.Fallback, "model": dr.Model} + } else { + content = dr.Text + digMeta = map[string]any{ + "model": dr.Model, + "chunks_used": dr.ChunksUsed, + "chunks_total": dr.ChunksTotal, + "retrieval": dr.Retrieval, + "embedded": dr.Embedded, + "fallback": false, + "cached": dr.Cached, + } + } + } + } + + var ctxItems []fleetctx.Item + var ctxMS int64 + if wantCtx && s.Ctx != nil { + ct0 := time.Now() + query := fleetctx.Query(q, cl.Title, headings(cl.Markdown)) + items, cw := s.Ctx.Search(ctx, c.Workspace, c.Name, query) + ctxMS = time.Since(ct0).Milliseconds() + if cw != "" { + warn = append(warn, cw) + } + ctxItems = items + } + + qLen := len(q) + if qLen > 80 { + qLen = 80 + } + fb, _ := "", false + if digMeta != nil { + if s, ok := digMeta["fallback"].(string); ok { + fb = s + } + } + s.Log.Info("fetch", "caller", c.Name, "host", s.logHost(c, res.FinalURL), "mode", mode, + "q_len", qLen, "raw_tokens", len(res.Body)/4, "returned_tokens", len(content)/4, + "pdf_pages", pdfPages, "digest_ms", digestMS, "fallback", fb) + + src := map[string]any{ + "url": res.URL, + "final_url": res.FinalURL, + "status": res.Status, + "content_type": res.ContentType, + "fetched_at": res.FetchedAt.Format(time.RFC3339), + "hash": res.SHA256(), + "bytes": len(res.Body), + "cached": len(res.Body) == 0, + "kind": "live", + } + if snap != nil { + src["kind"] = "snapshot" + src["via"] = string(snap.Via) + src["original_url"] = snap.OriginalURL + if snap.SnapshotAt != "" { + src["snapshot_at"] = snap.SnapshotAt + } else { + src["snapshot_at"] = nil + warn = append(warn, "snapshot date unknown") + } + } + m := map[string]any{ + "mode": mode, + "untrusted": true, + "source": src, + "warnings": warn, + "sizes": map[string]int{ + "raw_tokens": docLen / 4, + "clean_tokens": digest.EstTokens(cl.Markdown), + "returned_tokens": digest.EstTokens(content), + }, + "timings": map[string]int64{"fetch_ms": fetchMS, "digest_ms": digestMS, "context_ms": ctxMS}, + } + if pdfMap != nil { + m["pdf"] = pdfMap + } + if digMeta != nil { + m["digest"] = digMeta + } + if wantCtx { + m["context"] = ctxItems + } + return batch.Result{Content: content, Meta: m}, nil +} From 72e0828360946ae628c8b786fb2b415c73737733 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 22:43:28 -0700 Subject: [PATCH 06/10] =?UTF-8?q?api:=20NewBatchRunner=20=E2=80=94=20the?= =?UTF-8?q?=20batch=20fetch=20adapter,=20retry=20on=20busy,=20outcome=20me?= =?UTF-8?q?trics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire the batch runner to fetchTarget and feed the sift_batch_* metrics. New internal/api/batch.go: NewBatchRunner overwrites cfg.Hooks (Job, Target, JobSeconds, Depth, Inflight → s.Prom) and sets the runner's FetchFunc to batchFetch, which tags the context as a batch fetch, runs under requestDeadline, and maps fetchTarget's errors to what the runner retries (429/503 → ErrRetryLater) and what the metrics classify (batchOutcome). Added the Batch field to Server (nil → task 07 answers 503 "batch disabled") and, in extractPDF, charged the PDF pool to c.Name+"/batch" under the batch mark so a batch never takes a caller's interactive slot while the single-flight key is unchanged. No HTTP endpoints. Co-Authored-By: ornith-1.5-35b-a3b --- docs/implementer-log.md | 1 + internal/api/api.go | 4 + internal/api/batch.go | 113 +++++++++++++++++++ internal/api/batchrun_test.go | 202 ++++++++++++++++++++++++++++++++++ internal/api/pdf.go | 6 +- 5 files changed, 325 insertions(+), 1 deletion(-) create mode 100644 internal/api/batch.go create mode 100644 internal/api/batchrun_test.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 9dcdbfd..1854bed 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -22,6 +22,7 @@ One row per task session that ends in a commit. Newest last. | p6/03-runner-workers | 2026-09-26 | done | 1 | gate: ok | none | Workers in new internal/batch/worker.go: one rotation of live jobs in submit order, round-robin via an absolute cursor (advances in pick, wraps at access, so it survives a second job being added while the first is in flight — the mod-n cursor from the design stuck at 0 for one job and re-picked it), serial per job (target i+1 never starts before i is stored), one target per fetch under the Close-cancelled context (panic→ErrInternal, never echoed; ErrRetryLater retried after RetryWait, bounded by RetryFor then stored failed with the wrapped text). Terminal status once per job: all done→done, all failed→failed, mix→partial. Cancel of a running job sets cancelled, stores the in-flight target without overwriting cancelled, leaves later targets pending. Close sets closed, cancels the context, broadcasts, waits the workers, idempotent. batch.go: New starts cfg.Workers, Submit appends the liveJob and wakes, Cancel handles the in-flight case, Close stops. Absolute cursor is the one deviation from the design's "index in live" comment. All race tests pass -count=3. | | | p6/04-metrics | 2026-09-26 | done | 1 | gate: ok | none | Batch series in new internal/metrics/batch.go, registered in New: `BatchJobsTotal`/`BatchTargetsTotal` CounterVecs (label "outcome", any value outside the known set folded to "other" via `batchJobOutcomes`/`batchTargetOutcomes` maps so a raw string never becomes a label), `BatchQueueDepth`/`BatchTargetsInflight` gauges, and `BatchJobSeconds`/`BatchTargetSeconds` histograms (durationBuckets) plus `BatchResultBytes` (1K,4K,16K,64K,256K,1M). Nil-receiver-safe `Observe…`/`Set…` methods mirroring ObservePDF/ObserveDigest. Series served through the existing /metrics handler; no new endpoint, no Snapshot change. Given test byte-identical; metrics tests pass under -race -count=3; make gate ok. | | p6/05-fetchtarget | 2026-09-26 | done | 1 | gate: ok | none | Extracted v1fetch's per-target pipeline into new `internal/api/target.go`: `fetchTarget(ctx, c, t batch.Target) (batch.Result, error)`. Body moved unchanged in order — question truncation to 500 runes, mode default/check, wantCtx, doFetch, recover path, PDF branch (render, pages/grep errors, question-chunk limit), clean/raw branch, digest, context search, the fetch log line, and the envelope. `content` is returned as `Result.Content`; every other envelope key (`mode`,`untrusted`,`source`,`warnings`,`sizes`,`timings`, and `pdf`/`digest`/`context` when present) goes in `Result.Meta`, never content. `v1fetch` keeps reqN/caller/decode/400, builds a `batch.Target` (`URL` + `Mode`/`Question`/`Pages`/`Grep`/`JQ`/`Context`/`Recover` into `Options`), calls `fetchTarget`, `writeFetchErr` on error and `writeJSON(w,200,env)` on success where `env` is a fresh copy of `Meta` plus `"content"` (a copy, so `len(env)==len(Meta)+1` holds and Meta is not mutated). The two `writeErr` failures become `*fetch.Error` with the same status/msg/hint: 400 "invalid url" (bad mode), 400 "raw mode does not apply to PDFs". `doFetch` takes `context.Context` instead of `*http.Request` (its three `r.Context()` → `ctx`); `scrape` passes `r.Context()`. Removed now-unused `fmt`/`unicode/utf8`/`recover` imports, added `batch`. All api tests pass under -race -count=3; make gate ok; given test byte-identical. | +| p6/06-batch-wiring | 2026-09-26 | done | 1 | gate: ok | none | New `internal/api/batch.go`: `NewBatchRunner` overwrites `cfg.Hooks` so Job→ObserveBatchJob, Target→ObserveBatchTarget(batchOutcome)+ObserveBatchTargetSeconds (+ObserveBatchResultBytes only when err==nil), and JobSeconds/Depth/Inflight→the matching `s.Prom` methods; the runner's FetchFunc is `batchFetch`, which tags ctx with an unexported `batchCtxKey`, runs under `requestDeadline`, and maps fetchTarget's errors (a *fetch.Error 429/503 → `fmt.Errorf("%s: %w", fe.Msg, batch.ErrRetryLater)` so the runner retries; any other *fetch.Error → TargetError with 502 rewritten to 500 "internal error" and `s.blockN.Add(1)` for 403; anything else → TargetError 500 logged Error with `s.logErr`). `batchOutcome` classifies nil→ok, ErrRetryLater-wrapped→busy, then by TargetError.Status (403 blocked, 429 busy, 504 timeout, 415 unsupported, 422 whose Msg starts "document too long"→too_long, other 4xx→upstream_4xx, 5xx→upstream_5xx, any other error→upstream_5xx). Added `Batch *batch.Runner` Server field (nil → the /v1/batch endpoints answer 503 "batch disabled"). `pdf.go` extractPDF charges the PDF pool to `c.Name+"/batch"` when the ctx carries the batch mark, single-flight key unchanged so a batch and an interactive extract of the same PDF still share one child; the only new log line carries caller and host="-" (never a URL/host). All api tests pass under -race -count=3; make gate ok; given file byte-identical. | ## P5a review (owner, 2026-09-26) diff --git a/internal/api/api.go b/internal/api/api.go index 83c0769..3818f9b 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -73,6 +73,10 @@ type Server struct { pdfPoolOnce sync.Once + // Batch is the batch runner; nil means the /v1/batch endpoints (task 07) answer 503 + // "batch disabled". + Batch *batch.Runner + // uploads bounds how many /v1/extract bodies (up to PDFMaxUpload each) are read into // memory at once, ahead of the PDF pool's own limits. uploads chan struct{} diff --git a/internal/api/batch.go b/internal/api/batch.go new file mode 100644 index 0000000..7dc7c9e --- /dev/null +++ b/internal/api/batch.go @@ -0,0 +1,113 @@ +package api + +import ( + "context" + "errors" + "fmt" + "strings" + "time" + + "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/batch" + "git.wntrmute.dev/kyle/sift/internal/dcache" + "git.wntrmute.dev/kyle/sift/internal/fetch" +) + +// batchCtxKey marks a context as a batch fetch so extractPDF charges the PDF pool to the +// caller's "/batch" slot instead of its interactive one. +type batchCtxKey struct{} + +func isBatchFetch(ctx context.Context) bool { + ok, _ := ctx.Value(batchCtxKey{}).(bool) + return ok +} + +// NewBatchRunner builds the runner that serve (task 07) and the tests use. Its FetchFunc is +// s.batchFetch and its Hooks feed s.Prom; any cfg.Hooks from the caller is overwritten. +func (s *Server) NewBatchRunner(db *dcache.DB, cfg batch.Config) *batch.Runner { + cfg.Hooks = batch.Hooks{ + Job: func(outcome string) { + s.Prom.ObserveBatchJob(outcome) + }, + Target: func(err error, d time.Duration, n int) { + s.Prom.ObserveBatchTarget(batchOutcome(err)) + s.Prom.ObserveBatchTargetSeconds(d) + if err == nil { + s.Prom.ObserveBatchResultBytes(n) + } + }, + JobSeconds: func(d time.Duration) { + s.Prom.ObserveBatchJobSeconds(d) + }, + Depth: func(live int) { + s.Prom.SetBatchQueueDepth(live) + }, + Inflight: func(n int) { + s.Prom.SetBatchTargetsInflight(n) + }, + } + return batch.New(db, s.batchFetch, cfg, s.Log) +} + +// batchFetch is the runner's FetchFunc: one target through s.fetchTarget under the request +// deadline, tagged as a batch fetch. Its errors map to what the runner and the metrics need. +func (s *Server) batchFetch(ctx context.Context, c *auth.Caller, t batch.Target) (batch.Result, error) { + ctx, cancel := context.WithTimeout(ctx, requestDeadline) + defer cancel() + ctx = context.WithValue(ctx, batchCtxKey{}, true) + res, err := s.fetchTarget(ctx, c, t) + if err != nil { + var fe *fetch.Error + if errors.As(err, &fe) { + switch fe.Status { + case 429, 503: + return batch.Result{}, fmt.Errorf("%s: %w", fe.Msg, batch.ErrRetryLater) + } + if fe.Status == 502 { + fe.Status = 500 + fe.Msg = "internal error" + } + if fe.Status == 403 { + s.blockN.Add(1) + } + return batch.Result{}, &batch.TargetError{Status: fe.Status, Msg: fe.Msg, Hint: fe.Hint} + } + s.Log.Error("internal", "caller", c.Name, "host", "-", "err", s.logErr(c, err)) + return batch.Result{}, &batch.TargetError{Status: 500, Msg: "internal error"} + } + return res, nil +} + +// batchOutcome classifies a finished target's error into the sift_batch_targets_total label: +// a nil error is ok, a retry-later the runner gave up on is busy, a TargetError by its status, +// and anything else upstream_5xx so a raw string never becomes a label. +func batchOutcome(err error) string { + switch { + case err == nil: + return "ok" + case errors.Is(err, batch.ErrRetryLater): + return "busy" + } + var te *batch.TargetError + if errors.As(err, &te) { + switch te.Status { + case 403: + return "blocked" + case 429: + return "busy" + case 504: + return "timeout" + case 415: + return "unsupported" + case 422: + if strings.HasPrefix(te.Msg, "document too long") { + return "too_long" + } + return "upstream_4xx" + } + if te.Status >= 400 && te.Status < 500 { + return "upstream_4xx" + } + } + return "upstream_5xx" +} diff --git a/internal/api/batchrun_test.go b/internal/api/batchrun_test.go new file mode 100644 index 0000000..5e5c14c --- /dev/null +++ b/internal/api/batchrun_test.go @@ -0,0 +1,202 @@ +package api + +// P6 task 06: the runner wired to fetchTarget and the metrics, with no HTTP yet. The helpers +// here (newBatchServer, pageServer, fakeDNS) are shared with task 07's batch_test.go. + +import ( + "context" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "net/netip" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/batch" + "git.wntrmute.dev/kyle/sift/internal/dcache" + "git.wntrmute.dev/kyle/sift/internal/fetch" + "git.wntrmute.dev/kyle/sift/internal/guard" +) + +var loopback = []netip.Prefix{netip.MustParsePrefix("127.0.0.0/8")} + +// fakeDNS resolves internal.test to a private address; every other name fails to resolve. +func fakeDNS(_ context.Context, host string) ([]netip.Addr, error) { + if host == "internal.test" { + return []netip.Addr{netip.MustParseAddr("10.0.0.5")}, nil + } + return nil, fmt.Errorf("no such host %s", host) +} + +// newBatchServer builds a Server with the batch runner wired over an in-memory dcache through +// s.NewBatchRunner, exactly as serve does. callers are the names to provision (each gets a key). +func newBatchServer(t *testing.T, callers ...string) (*Server, map[string]string) { + t.Helper() + dir := t.TempDir() + keysPath := filepath.Join(dir, "keys.toml") + keys := map[string]string{} + for _, name := range callers { + out := filepath.Join(dir, name) + if err := auth.Add(keysPath, name, "hermes-fleet", out); err != nil { + t.Fatal(err) + } + b, err := os.ReadFile(out) + if err != nil { + t.Fatal(err) + } + keys[name] = strings.TrimSpace(string(b)) + } + store, err := auth.Load(keysPath) + if err != nil { + t.Fatal(err) + } + g := &guard.Guard{AllowPrefixes: loopback, LookupIP: fakeDNS} + f := &fetch.Fetcher{Guard: g, UA: "sift-test"} + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + s := New("test", g, f, store, log) + db, err := dcache.Open("", 1<<20, log) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + s.Batch = s.NewBatchRunner(db, batch.Config{}) + t.Cleanup(s.Batch.Close) // before db.Close + return s, keys +} + +// pageServer serves a small HTML page at every path; hold, when non-nil, blocks each request +// until it is closed. +func pageServer(t *testing.T, hold chan struct{}) *httptest.Server { + t.Helper() + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if hold != nil { + select { + case <-hold: + case <-r.Context().Done(): + return + } + } + w.Header().Set("Content-Type", "text/html; charset=utf-8") + fmt.Fprintf(w, `T

hello batch %s

`, r.URL.Path) + })) + t.Cleanup(ts.Close) + return ts +} + +func waitJob(t *testing.T, r *batch.Runner, id string) batch.Job { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + j, _, ok := r.Status(id) + if ok && (j.Status == "done" || j.Status == "partial" || j.Status == "failed" || j.Status == "cancelled") { + return j + } + time.Sleep(5 * time.Millisecond) + } + t.Fatal("job never finished") + return batch.Job{} +} + +func promText(t *testing.T, s *Server) string { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/metrics", nil) + req.RemoteAddr, req.Host = "127.0.0.1:9", "127.0.0.1:8890" + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + return rec.Body.String() +} + +// The runner fetches through fetchTarget as the submitting caller; a name that resolves to a +// private address is a per-target "blocked" failure; the hooks reach the metrics. +func TestBatchRunnerWired(t *testing.T) { + ts := pageServer(t, nil) + s, _ := newBatchServer(t, "test") + c := &auth.Caller{Name: "test", Workspace: "hermes-fleet"} + id, reused, err := s.Batch.Submit(context.Background(), c, dcache.ClassFleet, []batch.Target{ + {URL: ts.URL + "/one"}, {URL: "http://internal.test/"}, + }) + if err != nil || reused { + t.Fatalf("submit: %v %v", err, reused) + } + if j := waitJob(t, s.Batch, id); j.Status != "partial" { + t.Fatalf("job %+v", j) + } + _, ts2, _ := s.Batch.Status(id) + if ts2[0].Status != "done" || ts2[0].Result == nil || !strings.Contains(ts2[0].Result.Content, "hello batch /one") { + t.Fatalf("target 0: %+v", ts2[0]) + } + if ts2[0].Result.Meta["mode"] != "clean" || ts2[0].Result.Meta["untrusted"] != true { + t.Fatalf("target 0 meta %v: not the fetchTarget envelope", ts2[0].Result.Meta) + } + if ts2[1].Status != "failed" || !strings.Contains(ts2[1].Error, "blocked") { + t.Fatalf("target 1: %+v", ts2[1]) + } + out := promText(t, s) + for _, want := range []string{ + `sift_batch_jobs_total{outcome="created"} 1`, + `sift_batch_jobs_total{outcome="partial"} 1`, + `sift_batch_targets_total{outcome="ok"} 1`, + `sift_batch_targets_total{outcome="blocked"} 1`, + `sift_batch_result_bytes_count 1`, + `sift_batch_queue_depth 0`, + } { + if !strings.Contains(out, want) { + t.Errorf("metrics lack %q", want) + } + } +} + +// The adapter turns fetch errors into what the runner and the metrics need. +func TestBatchOutcomeClasses(t *testing.T) { + cases := []struct { + err error + want string + }{ + {nil, "ok"}, + {fmt.Errorf("busy: %w", batch.ErrRetryLater), "busy"}, + {&batch.TargetError{Status: 403, Msg: "blocked: private address"}, "blocked"}, + {&batch.TargetError{Status: 429, Msg: "busy"}, "busy"}, + {&batch.TargetError{Status: 504, Msg: "timeout"}, "timeout"}, + {&batch.TargetError{Status: 415, Msg: "unsupported content type"}, "unsupported"}, + {&batch.TargetError{Status: 422, Msg: "document too long for question (6001 chunks); use grep or pages"}, "too_long"}, + {&batch.TargetError{Status: 422, Msg: "unreadable PDF"}, "upstream_4xx"}, + {&batch.TargetError{Status: 404, Msg: "not found"}, "upstream_4xx"}, + {&batch.TargetError{Status: 500, Msg: "internal error"}, "upstream_5xx"}, + {errors.New("something else"), "upstream_5xx"}, + } + for _, c := range cases { + if got := batchOutcome(c.err); got != c.want { + t.Errorf("batchOutcome(%v) = %q, want %q", c.err, got, c.want) + } + } +} + +// batchFetch maps a busy fetch to ErrRetryLater (the runner retries it) and every other +// failure to a TargetError with the /v1/fetch status. +func TestBatchFetchErrors(t *testing.T) { + s, _ := newBatchServer(t, "test") + c := &auth.Caller{Name: "test", Workspace: "hermes-fleet"} + _, err := s.batchFetch(context.Background(), c, batch.Target{URL: "http://internal.test/"}) + var te *batch.TargetError + if !errors.As(err, &te) || te.Status != 403 || !strings.Contains(te.Msg, "blocked") { + t.Fatalf("private name: %v, want a TargetError 403 blocked", err) + } + // Fill the global fetch gate so the next fetch is refused busy. + for i := 0; i < 8; i++ { + if err := s.acquire(fmt.Sprintf("filler%d", i), "http://filler.test/"); err != nil { + t.Fatal(err) + } + } + ts := pageServer(t, nil) + _, err = s.batchFetch(context.Background(), c, batch.Target{URL: ts.URL}) + if !errors.Is(err, batch.ErrRetryLater) { + t.Fatalf("busy gate: %v, want an error wrapping batch.ErrRetryLater", err) + } +} diff --git a/internal/api/pdf.go b/internal/api/pdf.go index d13e313..c141247 100644 --- a/internal/api/pdf.go +++ b/internal/api/pdf.go @@ -58,7 +58,11 @@ func (s *Server) extractPDF(ctx context.Context, c *auth.Caller, res *fetch.Reso return v, err } start := time.Now() - v, err := s.pool().Do(ctx, s.callerClass(c)+"|"+res.SHA256(), c.Name, fn) + caller := c.Name + if isBatchFetch(ctx) { + caller = c.Name + "/batch" + } + v, err := s.pool().Do(ctx, s.callerClass(c)+"|"+res.SHA256(), caller, fn) total := time.Since(start) // The job's own extraction time is what this call measured it to be; the // rest of Do is time spent waiting for a slot. From 692a334c09eac1fb2c1e3bece6fc6818d9536571 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 22:59:05 -0700 Subject: [PATCH 07/10] =?UTF-8?q?api:=20/v1/batch=20=E2=80=94=20submit,=20?= =?UTF-8?q?status=20summary,=20one=20result,=20cancel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The HTTP surface over s.Batch. New internal/api/batch.go: batchSubmit (POST) decodes {"targets":[{url,mode,question,pages,grep,jq,context,recover}]} flat as /v1/fetch does, 400 "invalid batch" on bad json / empty list / empty url / bad mode, then PreCheck per target -> 422 "blocked: target : " with no job, then Submit mapping ErrTooManyTargets->422, ErrCallerBusy->429 Retry-After 30, ErrBusy->429 Retry-After 60, ErrNoTargets->400, and 202 {success,job_id,status,targets,reused}. batchStatus (GET), batchResult (GET {id}/{idx}) and batchCancel (DELETE) share ownership: unknown or not c.Name -> 404, never 403; status is a summary (no inline results unless ?results=1), idx validated 0..targets_total-1 else 404, cancel of a terminal job -> 409 else 200 {success,status:cancelled}. api.go scoped to rule 2: batchMaxBody (256 KiB), the four routes in New, and ServeHTTP applying the per-path cap once and collapsing every batch path to a fixed metrics label so a job id never becomes a label. Each handler logs one batch line, never a URL, hostname or idem_key. Co-Authored-By: ornith-1.5-35b-a3b --- docs/implementer-log.md | 1 + internal/api/api.go | 33 ++++- internal/api/batch.go | 253 +++++++++++++++++++++++++++++++++ internal/api/batch_test.go | 280 +++++++++++++++++++++++++++++++++++++ 4 files changed, 566 insertions(+), 1 deletion(-) create mode 100644 internal/api/batch_test.go diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 1854bed..cd50411 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -23,6 +23,7 @@ One row per task session that ends in a commit. Newest last. | p6/04-metrics | 2026-09-26 | done | 1 | gate: ok | none | Batch series in new internal/metrics/batch.go, registered in New: `BatchJobsTotal`/`BatchTargetsTotal` CounterVecs (label "outcome", any value outside the known set folded to "other" via `batchJobOutcomes`/`batchTargetOutcomes` maps so a raw string never becomes a label), `BatchQueueDepth`/`BatchTargetsInflight` gauges, and `BatchJobSeconds`/`BatchTargetSeconds` histograms (durationBuckets) plus `BatchResultBytes` (1K,4K,16K,64K,256K,1M). Nil-receiver-safe `Observe…`/`Set…` methods mirroring ObservePDF/ObserveDigest. Series served through the existing /metrics handler; no new endpoint, no Snapshot change. Given test byte-identical; metrics tests pass under -race -count=3; make gate ok. | | p6/05-fetchtarget | 2026-09-26 | done | 1 | gate: ok | none | Extracted v1fetch's per-target pipeline into new `internal/api/target.go`: `fetchTarget(ctx, c, t batch.Target) (batch.Result, error)`. Body moved unchanged in order — question truncation to 500 runes, mode default/check, wantCtx, doFetch, recover path, PDF branch (render, pages/grep errors, question-chunk limit), clean/raw branch, digest, context search, the fetch log line, and the envelope. `content` is returned as `Result.Content`; every other envelope key (`mode`,`untrusted`,`source`,`warnings`,`sizes`,`timings`, and `pdf`/`digest`/`context` when present) goes in `Result.Meta`, never content. `v1fetch` keeps reqN/caller/decode/400, builds a `batch.Target` (`URL` + `Mode`/`Question`/`Pages`/`Grep`/`JQ`/`Context`/`Recover` into `Options`), calls `fetchTarget`, `writeFetchErr` on error and `writeJSON(w,200,env)` on success where `env` is a fresh copy of `Meta` plus `"content"` (a copy, so `len(env)==len(Meta)+1` holds and Meta is not mutated). The two `writeErr` failures become `*fetch.Error` with the same status/msg/hint: 400 "invalid url" (bad mode), 400 "raw mode does not apply to PDFs". `doFetch` takes `context.Context` instead of `*http.Request` (its three `r.Context()` → `ctx`); `scrape` passes `r.Context()`. Removed now-unused `fmt`/`unicode/utf8`/`recover` imports, added `batch`. All api tests pass under -race -count=3; make gate ok; given test byte-identical. | | p6/06-batch-wiring | 2026-09-26 | done | 1 | gate: ok | none | New `internal/api/batch.go`: `NewBatchRunner` overwrites `cfg.Hooks` so Job→ObserveBatchJob, Target→ObserveBatchTarget(batchOutcome)+ObserveBatchTargetSeconds (+ObserveBatchResultBytes only when err==nil), and JobSeconds/Depth/Inflight→the matching `s.Prom` methods; the runner's FetchFunc is `batchFetch`, which tags ctx with an unexported `batchCtxKey`, runs under `requestDeadline`, and maps fetchTarget's errors (a *fetch.Error 429/503 → `fmt.Errorf("%s: %w", fe.Msg, batch.ErrRetryLater)` so the runner retries; any other *fetch.Error → TargetError with 502 rewritten to 500 "internal error" and `s.blockN.Add(1)` for 403; anything else → TargetError 500 logged Error with `s.logErr`). `batchOutcome` classifies nil→ok, ErrRetryLater-wrapped→busy, then by TargetError.Status (403 blocked, 429 busy, 504 timeout, 415 unsupported, 422 whose Msg starts "document too long"→too_long, other 4xx→upstream_4xx, 5xx→upstream_5xx, any other error→upstream_5xx). Added `Batch *batch.Runner` Server field (nil → the /v1/batch endpoints answer 503 "batch disabled"). `pdf.go` extractPDF charges the PDF pool to `c.Name+"/batch"` when the ctx carries the batch mark, single-flight key unchanged so a batch and an interactive extract of the same PDF still share one child; the only new log line carries caller and host="-" (never a URL/host). All api tests pass under -race -count=3; make gate ok; given file byte-identical. | +| p6/07-api-endpoints | 2026-09-26 | done | 1 | gate: ok | none | The four /v1/batch handlers in new `internal/api/batch.go`: `batchSubmit` (POST) decodes `{"targets":[{url,mode,question,pages,grep,jq,context,recover}]}` flat like `/v1/fetch`, 400 "invalid batch" on bad json/empty list/empty url/bad mode (only empty,clean,raw,digest), then `PreCheck` per target → 422 "blocked: target : " with no job, then `Submit` mapping ErrTooManyTargets→422 "too many targets (max 32)", ErrCallerBusy→429+Retry-After 30, ErrBusy→429+Retry-After 60, ErrNoTargets→400, and 202 {success,job_id,status,targets,reused} (status "queued" new, current status reused). `batchStatus` (GET) and `batchResult` (GET {id}/{idx}) and `batchCancel` (DELETE) share `ownership` (unknown or not `c.Name` → 404, never 403; nil batch → 503); status is a summary with no results unless `?results=1` inlines the envelope (Meta+content) on done targets, idx validated 0..targets_total-1 else 404, cancel of a terminal job → 409 "job already finished" else 200 {success,status:cancelled}. `api.go` scoped to the rule-2 changes: `batchMaxBody` (256 KiB) const, the four routes in `New`, and `ServeHTTP` applying the per-path cap once (`/v1/batch`→256 KiB, `/v1/extract`→pdfMaxUpload, else 64 KiB) and collapsing every batch path to a fixed metrics label via `batchObservePath` (`/v1/batch` or `/v1/batch/{id}`, never a job id). Each handler logs one `batch` line with caller/job_id/targets/status-or-reused and never a URL/hostname/idem_key. Given test byte-identical; all api tests pass under -race -count=3; make gate ok. | ## P5a review (owner, 2026-09-26) diff --git a/internal/api/api.go b/internal/api/api.go index 3818f9b..61a0a2b 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -37,6 +37,10 @@ const ( writeTimeout = 240 * time.Second // P5a: a PDF may take 120 s to download + 20 s queued + 60 s to extract requestDeadline = 210 * time.Second // P5a: the PDF path's worst case (see writeTimeout); other paths keep their own shorter timeouts + // batchMaxBody is the largest body POST /v1/batch accepts: 32 targets, each with a + // large question, exceed the 64 KiB cap of the other endpoints. + batchMaxBody = 256 << 10 + // DefaultPDFMaxUpload is the largest body POST /v1/extract accepts: an uploaded // PDF, addressed by its hash and extracted once per content. DefaultPDFMaxUpload = 100 << 20 @@ -108,6 +112,10 @@ func New(version string, g *guard.Guard, f *fetch.Fetcher, keys *auth.Store, log s.Mux.HandleFunc("POST /v1/extract", s.v1extract) s.Mux.HandleFunc("POST /v1/search", s.v1search) s.Mux.HandleFunc("POST /v2/search", s.v2search) + s.Mux.HandleFunc("POST /v1/batch", s.batchSubmit) + s.Mux.HandleFunc("GET /v1/batch/{id}", s.batchStatus) + s.Mux.HandleFunc("GET /v1/batch/{id}/{idx}", s.batchResult) + s.Mux.HandleFunc("DELETE /v1/batch/{id}", s.batchCancel) return s } @@ -125,8 +133,11 @@ func (s *Server) HTTPServer(addr string) *http.Server { func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { var limit int64 = maxReqBody - if r.URL.Path == "/v1/extract" { + switch r.URL.Path { + case "/v1/extract": limit = int64(s.pdfMaxUpload()) + case "/v1/batch": + limit = batchMaxBody } r.Body = http.MaxBytesReader(w, r.Body, limit) switch r.URL.Path { @@ -144,9 +155,29 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { s.Prom.Observe(r.URL.Path, time.Since(start), code) return } + if strings.HasPrefix(r.URL.Path, "/v1/batch") { + cw := &codeWriter{ResponseWriter: w} + start := time.Now() + s.Mux.ServeHTTP(cw, r) + code := cw.code + if code == 0 { + code = http.StatusOK + } + s.Prom.Observe(batchObservePath(r.URL.Path), time.Since(start), code) + return + } s.Mux.ServeHTTP(w, r) } +// batchObservePath collapses a batch path to a fixed label so a job id never becomes a +// metric label value: the submit path stays, every {id}/{idx} variant becomes {id}. +func batchObservePath(path string) string { + if path == "/v1/batch" { + return "/v1/batch" + } + return "/v1/batch/{id}" +} + type codeWriter struct { http.ResponseWriter code int diff --git a/internal/api/batch.go b/internal/api/batch.go index 7dc7c9e..1945972 100644 --- a/internal/api/batch.go +++ b/internal/api/batch.go @@ -2,8 +2,11 @@ package api import ( "context" + "encoding/json" "errors" "fmt" + "net/http" + "strconv" "strings" "time" @@ -111,3 +114,253 @@ func batchOutcome(err error) string { } return "upstream_5xx" } + +// The four /v1/batch handlers (task 07). A job that is unknown or owned by another caller is a +// 404 for every GET and DELETE, never a 403, so a caller cannot probe for another caller's jobs. + +// ownership answers the caller for id: a disabled batch is 503, an unknown id or one not owned by +// c is a 404. The empty job returned on either path signals "already answered" to the caller. +func (s *Server) ownership(w http.ResponseWriter, c *auth.Caller, id string) (batch.Job, []batch.TargetStatus) { + if s.Batch == nil { + writeErr(w, 503, "batch disabled", "") + return batch.Job{}, nil + } + j, ts, ok := s.Batch.Status(id) + if !ok || j.Caller != c.Name { + writeErr(w, 404, "not found", "") + return batch.Job{}, nil + } + return j, ts +} + +// logBatch writes the one batch line per handler: caller, job_id, targets, then either status or +// reused. It never carries a target URL, hostname or the idem_key. +func (s *Server) logBatch(c *auth.Caller, jobID string, targets int, extra ...any) { + s.Log.Info("batch", + append([]any{ + "caller", c.Name, + "job_id", jobID, + "targets", targets, + }, extra...)..., + ) +} + +func validBatchMode(m string) bool { + switch m { + case "", "clean", "raw", "digest": + return true + } + return false +} + +func (s *Server) batchSubmit(w http.ResponseWriter, r *http.Request) { + c := s.caller(w, r) + if c == nil { + return + } + var req struct { + Targets []v1Req `json:"targets"` + } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + writeErr(w, 400, "invalid batch: bad json", "") + return + } + if len(req.Targets) == 0 { + writeErr(w, 400, "invalid batch: no targets", "") + return + } + targets := make([]batch.Target, 0, len(req.Targets)) + for _, t := range req.Targets { + if t.URL == "" { + writeErr(w, 400, "invalid batch: empty url", "") + return + } + if !validBatchMode(t.Mode) { + writeErr(w, 400, "invalid batch: bad mode", "") + return + } + targets = append(targets, batch.Target{ + URL: t.URL, + Options: batch.Options{ + Mode: t.Mode, + Question: t.Question, + Pages: t.Pages, + Grep: t.Grep, + JQ: t.JQ, + Context: t.Context, + Recover: t.Recover, + }, + }) + } + for i, t := range targets { + if err := s.Guard.PreCheck(t.URL); err != nil { + s.logBatch(c, "", len(targets), "status", "blocked") + writeErr(w, 422, fmt.Sprintf("blocked: target %d: %v", i, err), "") + return + } + } + id, reused, err := s.Batch.Submit(r.Context(), c, s.callerClass(c), targets) + if err != nil { + s.logBatch(c, "", len(targets), "status", "failed") + switch { + case errors.Is(err, batch.ErrTooManyTargets): + writeErr(w, 422, "too many targets (max 32)", "") + case errors.Is(err, batch.ErrCallerBusy): + w.Header().Set("Retry-After", "30") + writeErr(w, 429, "busy: a batch of yours is still running", "") + case errors.Is(err, batch.ErrBusy): + w.Header().Set("Retry-After", "60") + writeErr(w, 429, "batch jobs busy", "") + case errors.Is(err, batch.ErrNoTargets): + writeErr(w, 400, "invalid batch: no targets", "") + default: + writeErr(w, 500, "internal error", "") + } + return + } + status := "queued" + if reused { + if j, _, ok := s.Batch.Status(id); ok { + status = j.Status + } + } + if reused { + s.logBatch(c, id, len(targets), "reused", true) + } else { + s.logBatch(c, id, len(targets), "status", status) + } + writeJSON(w, 202, map[string]any{ + "success": true, + "job_id": id, + "status": status, + "targets": len(targets), + "reused": reused, + }) +} + +func (s *Server) batchStatus(w http.ResponseWriter, r *http.Request) { + c := s.caller(w, r) + if c == nil { + return + } + id := r.PathValue("id") + j, ts := s.ownership(w, c, id) + if j.ID == "" { + return + } + s.Batch.Touch(id) + s.logBatch(c, id, j.TargetsTotal, "status", j.Status) + writeJSON(w, 200, statusEnvelope(j, ts, r)) +} + +func (s *Server) batchResult(w http.ResponseWriter, r *http.Request) { + c := s.caller(w, r) + if c == nil { + return + } + id := r.PathValue("id") + j, ts := s.ownership(w, c, id) + if j.ID == "" { + return + } + idx, err := strconv.Atoi(r.PathValue("idx")) + if err != nil || idx < 0 || idx >= j.TargetsTotal { + writeErr(w, 404, "not found", "") + return + } + s.Batch.Touch(id) + t := ts[idx] + out := map[string]any{ + "job_id": j.ID, + "idx": t.Idx, + "url": t.URL, + "status": t.Status, + } + if t.Result != nil { + out["result"] = resultEnvelope(t.Result) + } + if t.Error != "" { + out["error"] = t.Error + } + if t.Hint != "" { + out["hint"] = t.Hint + } + s.logBatch(c, id, j.TargetsTotal, "status", t.Status) + writeJSON(w, 200, out) +} + +func (s *Server) batchCancel(w http.ResponseWriter, r *http.Request) { + c := s.caller(w, r) + if c == nil { + return + } + id := r.PathValue("id") + j, _ := s.ownership(w, c, id) + if j.ID == "" { + return + } + switch j.Status { + case "done", "partial", "failed", "cancelled": + s.logBatch(c, id, j.TargetsTotal, "status", "failed") + writeErr(w, 409, "job already finished", "") + return + } + if s.Batch.Cancel(id) { + s.logBatch(c, id, j.TargetsTotal, "status", "cancelled") + writeJSON(w, 200, map[string]any{"success": true, "status": "cancelled"}) + return + } + s.logBatch(c, id, j.TargetsTotal, "status", "failed") + writeErr(w, 409, "job already finished", "") +} + +// statusEnvelope is the summary GET /v1/batch/{id} returns: statuses, never inline results, +// because 32 results can be several megabytes. With ?results=1 each done target carries the +// /v1/fetch envelope (Meta plus content). +func statusEnvelope(j batch.Job, ts []batch.TargetStatus, r *http.Request) map[string]any { + out := map[string]any{ + "job_id": j.ID, + "status": j.Status, + "targets_total": j.TargetsTotal, + "targets_done": j.TargetsDone, + "created_at": time.Unix(j.CreatedAt, 0).UTC().Format(time.RFC3339), + "targets": targetSummaries(ts, r), + } + if j.Error != "" { + out["error"] = j.Error + } + return out +} + +func targetSummaries(ts []batch.TargetStatus, r *http.Request) []map[string]any { + results := r.URL.Query().Get("results") == "1" + out := make([]map[string]any, 0, len(ts)) + for _, t := range ts { + m := map[string]any{ + "idx": t.Idx, + "url": t.URL, + "status": t.Status, + } + if t.Error != "" { + m["error"] = t.Error + } + if t.Hint != "" { + m["hint"] = t.Hint + } + if results && t.Status == "done" && t.Result != nil { + m["result"] = resultEnvelope(t.Result) + } + out = append(out, m) + } + return out +} + +// resultEnvelope is the /v1/fetch shape: every Meta field plus content. +func resultEnvelope(res *batch.Result) map[string]any { + env := make(map[string]any, len(res.Meta)+1) + for k, v := range res.Meta { + env[k] = v + } + env["content"] = res.Content + return env +} diff --git a/internal/api/batch_test.go b/internal/api/batch_test.go new file mode 100644 index 0000000..6b81ad6 --- /dev/null +++ b/internal/api/batch_test.go @@ -0,0 +1,280 @@ +package api + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +// Every test fetches only from local httptest servers or internal.test (fake DNS): no test +// touches the network. newBatchServer, pageServer and fakeDNS come from batchrun_test.go. + +func batchReq(t *testing.T, s *Server, method, path, key string, body any) *httptest.ResponseRecorder { + t.Helper() + var rd io.Reader + if body != nil { + b, err := json.Marshal(body) + if err != nil { + t.Fatal(err) + } + rd = strings.NewReader(string(b)) + } + req := httptest.NewRequest(method, path, rd) + req.Header.Set("Authorization", "Bearer "+key) + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + return rec +} + +func batchDecode(t *testing.T, rec *httptest.ResponseRecorder) map[string]any { + t.Helper() + var m map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &m); err != nil { + t.Fatalf("decode: %v (%d %s)", err, rec.Code, rec.Body.String()) + } + return m +} + +func submit(t *testing.T, s *Server, key string, targets ...map[string]any) map[string]any { + t.Helper() + rec := batchReq(t, s, http.MethodPost, "/v1/batch", key, map[string]any{"targets": targets}) + if rec.Code != 202 { + t.Fatalf("submit: %d %s", rec.Code, rec.Body.String()) + } + return batchDecode(t, rec) +} + +func waitTerminal(t *testing.T, s *Server, key, id string) map[string]any { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + env := batchDecode(t, batchReq(t, s, http.MethodGet, "/v1/batch/"+id, key, nil)) + switch env["status"] { + case "done", "partial", "failed", "cancelled": + return env + } + time.Sleep(5 * time.Millisecond) + } + t.Fatal("batch never reached a terminal status") + return nil +} + +func TestBatchSubmitAndCollect(t *testing.T) { + ts := pageServer(t, nil) + s, keys := newBatchServer(t, "test") + sub := submit(t, s, keys["test"], map[string]any{"url": ts.URL + "/one"}, map[string]any{"url": ts.URL + "/two"}) + id, _ := sub["job_id"].(string) + if sub["success"] != true || id == "" || sub["reused"] != false || sub["targets"] != float64(2) || sub["status"] != "queued" { + t.Fatalf("submit body %v", sub) + } + + // The status body is a summary: statuses, no results. + env := waitTerminal(t, s, keys["test"], id) + if env["status"] != "done" || env["targets_total"] != float64(2) || env["targets_done"] != float64(2) { + t.Fatalf("status %v", env) + } + if _, err := time.Parse(time.RFC3339, fmt.Sprint(env["created_at"])); err != nil { + t.Fatalf("created_at %v is not RFC 3339", env["created_at"]) + } + targets := env["targets"].([]any) + if len(targets) != 2 { + t.Fatalf("targets %d", len(targets)) + } + for i, x := range targets { + tg := x.(map[string]any) + if tg["idx"] != float64(i) || tg["status"] != "done" { + t.Fatalf("target %d: %v", i, tg) + } + if _, has := tg["result"]; has { + t.Fatalf("summary must not carry results: target %d has one", i) + } + } + + // One target's result: the /v1/fetch envelope. + rec := batchReq(t, s, http.MethodGet, "/v1/batch/"+id+"/1", keys["test"], nil) + if rec.Code != 200 { + t.Fatalf("GET idx: %d %s", rec.Code, rec.Body.String()) + } + one := batchDecode(t, rec) + res, _ := one["result"].(map[string]any) + if one["idx"] != float64(1) || one["status"] != "done" || res == nil { + t.Fatalf("GET idx body %v", one) + } + if c, _ := res["content"].(string); !strings.Contains(c, "hello batch /two") { + t.Fatalf("result content %q", c) + } + if res["untrusted"] != true || res["mode"] != "clean" { + t.Fatalf("result is not the /v1/fetch envelope: %v", res) + } + if src, _ := res["source"].(map[string]any); src == nil || src["final_url"] != ts.URL+"/two" { + t.Fatalf("result source %v", res["source"]) + } + + // ?results=1 inlines every result. + full := batchDecode(t, batchReq(t, s, http.MethodGet, "/v1/batch/"+id+"?results=1", keys["test"], nil)) + for i, x := range full["targets"].([]any) { + if _, has := x.(map[string]any)["result"]; !has { + t.Fatalf("results=1: target %d has no result", i) + } + } + + if got := batchReq(t, s, http.MethodGet, "/v1/batch/"+id+"/2", keys["test"], nil).Code; got != 404 { + t.Fatalf("GET idx out of range: %d, want 404", got) + } + if got := batchReq(t, s, http.MethodGet, "/v1/batch/"+id+"/x", keys["test"], nil).Code; got != 404 { + t.Fatalf("GET idx not a number: %d, want 404", got) + } +} + +func TestBatchIdempotentResubmit(t *testing.T) { + ts := pageServer(t, nil) + s, keys := newBatchServer(t, "test") + tg := map[string]any{"url": ts.URL, "mode": "clean"} + a := submit(t, s, keys["test"], tg) + b := submit(t, s, keys["test"], tg) + if a["job_id"] != b["job_id"] || b["reused"] != true { + t.Fatalf("resubmit: %v then %v", a, b) + } +} + +func TestBatchRefusedUpFront(t *testing.T) { + s, keys := newBatchServer(t, "test") + cases := []struct { + name string + body any + code int + want string + }{ + {"guard pre-check (port)", map[string]any{"targets": []any{map[string]any{"url": "http://127.0.0.1/"}, map[string]any{"url": "http://example.com:8080/"}}}, 422, "blocked"}, + {"guard pre-check (private literal)", map[string]any{"targets": []any{map[string]any{"url": "http://10.0.0.5/"}}}, 422, "blocked"}, + {"no targets", map[string]any{"targets": []any{}}, 400, ""}, + {"not json", "nope", 400, ""}, + {"empty url", map[string]any{"targets": []any{map[string]any{"url": ""}}}, 400, ""}, + {"bad mode", map[string]any{"targets": []any{map[string]any{"url": "http://127.0.0.1/", "mode": "shout"}}}, 400, ""}, + } + for _, c := range cases { + rec := batchReq(t, s, http.MethodPost, "/v1/batch", keys["test"], c.body) + if rec.Code != c.code || !strings.Contains(rec.Body.String(), c.want) { + t.Errorf("%s: %d %s, want %d containing %q", c.name, rec.Code, rec.Body.String(), c.code, c.want) + } + } + // The guard refusal names the offending index. + rec := batchReq(t, s, http.MethodPost, "/v1/batch", keys["test"], cases[0].body) + if !strings.Contains(rec.Body.String(), "1") { + t.Errorf("guard refusal does not name target 1: %s", rec.Body.String()) + } + + var many []any + for i := 0; i < 33; i++ { + many = append(many, map[string]any{"url": fmt.Sprintf("http://127.0.0.1/%d", i)}) + } + if rec := batchReq(t, s, http.MethodPost, "/v1/batch", keys["test"], map[string]any{"targets": many}); rec.Code != 422 { + t.Errorf("33 targets: %d, want 422", rec.Code) + } +} + +// 32 targets, each with a 500-rune question, fit the batch body cap even though they are well +// over the 64 KiB cap of the other endpoints. +func TestBatchBodyCap(t *testing.T) { + hold := make(chan struct{}) + defer close(hold) + ts := pageServer(t, hold) + s, keys := newBatchServer(t, "test") + q := strings.Repeat("q", 500) + var many []any + for i := 0; i < 32; i++ { + many = append(many, map[string]any{"url": fmt.Sprintf("%s/%d?pad=%s", ts.URL, i, strings.Repeat("p", 1500)), "question": q}) + } + if rec := batchReq(t, s, http.MethodPost, "/v1/batch", keys["test"], map[string]any{"targets": many}); rec.Code != 202 { + t.Fatalf("32 targets with questions (~%d KiB): %d %s", 32*2100/1024, rec.Code, rec.Body.String()) + } +} + +func TestBatchPartialFailure(t *testing.T) { + ts := pageServer(t, nil) + s, keys := newBatchServer(t, "test") + // Target 1 is a name that resolves to a private address. Names are not resolved at submit, + // so it passes the pre-check; the fetch-time guard blocks it: a per-target failure. + sub := submit(t, s, keys["test"], map[string]any{"url": ts.URL}, map[string]any{"url": "http://internal.test/"}) + env := waitTerminal(t, s, keys["test"], sub["job_id"].(string)) + if env["status"] != "partial" { + t.Fatalf("status %v", env["status"]) + } + second := env["targets"].([]any)[1].(map[string]any) + if second["status"] != "failed" { + t.Fatalf("target 1: %v", second) + } + if e, _ := second["error"].(string); !strings.Contains(e, "blocked") { + t.Fatalf("target 1 error %q", e) + } +} + +func TestBatchOwnership404(t *testing.T) { + hold := make(chan struct{}) + defer close(hold) + ts := pageServer(t, hold) + s, keys := newBatchServer(t, "test", "other") + id := submit(t, s, keys["test"], map[string]any{"url": ts.URL})["job_id"].(string) + for _, p := range []string{"/v1/batch/" + id, "/v1/batch/" + id + "/0"} { + if got := batchReq(t, s, http.MethodGet, p, keys["other"], nil).Code; got != 404 { + t.Fatalf("other caller GET %s: %d (must not reveal the job)", p, got) + } + } + if got := batchReq(t, s, http.MethodDelete, "/v1/batch/"+id, keys["other"], nil).Code; got != 404 { + t.Fatalf("other caller DELETE: %d", got) + } + if got := batchReq(t, s, http.MethodGet, "/v1/batch/"+id, keys["test"], nil).Code; got != 200 { + t.Fatalf("owner GET: %d", got) + } + if got := batchReq(t, s, http.MethodGet, "/v1/batch/no-such-job", keys["test"], nil).Code; got != 404 { + t.Fatalf("unknown job: %d", got) + } + if got := batchReq(t, s, http.MethodGet, "/v1/batch/"+id, "not-a-key", nil).Code; got != 401 { + t.Fatalf("bad key: %d", got) + } +} + +func TestBatchCancel(t *testing.T) { + hold := make(chan struct{}) + ts := pageServer(t, hold) + s, keys := newBatchServer(t, "test") + id := submit(t, s, keys["test"], map[string]any{"url": ts.URL + "/a"}, map[string]any{"url": ts.URL + "/b"})["job_id"].(string) + rec := batchReq(t, s, http.MethodDelete, "/v1/batch/"+id, keys["test"], nil) + if rec.Code != 200 { + t.Fatalf("cancel: %d %s", rec.Code, rec.Body.String()) + } + if b := batchDecode(t, rec); b["success"] != true || b["status"] != "cancelled" { + t.Fatalf("cancel body %v", b) + } + close(hold) + env := waitTerminal(t, s, keys["test"], id) + if env["status"] != "cancelled" { + t.Fatalf("status %v", env["status"]) + } + if tg := env["targets"].([]any)[1].(map[string]any); tg["status"] != "pending" { + t.Fatalf("target 1 after cancel: %v", tg) + } + if got := batchReq(t, s, http.MethodDelete, "/v1/batch/"+id, keys["test"], nil).Code; got != 409 { + t.Fatalf("cancelling a finished job: %d, want 409", got) + } +} + +// The request metrics never get a job id as a label value. +func TestBatchMetricsPathLabel(t *testing.T) { + ts := pageServer(t, nil) + s, keys := newBatchServer(t, "test") + id := submit(t, s, keys["test"], map[string]any{"url": ts.URL})["job_id"].(string) + waitTerminal(t, s, keys["test"], id) + batchReq(t, s, http.MethodGet, "/v1/batch/"+id+"/0", keys["test"], nil) + if strings.Contains(promText(t, s), id) { + t.Fatal("a job id appears in /metrics") + } +} From 484c33f986a05a2972a2cfe7929e19f0f43c3d16 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 23:06:46 -0700 Subject: [PATCH 08/10] serve: [batch] config; open the store, reap jobs killed by a restart, start the runner Add the [batch] config block (Batch type + File.Batch) and ParseSize (binary KiB/MiB/GiB byte counts) in internal/config. serve now opens the dcache store when batch is on, marks the queued/running jobs a restart killed as failed (never resumed) and observes them, builds the runner from the config with the runner's defaults filling zeros, and closes the runner on both shutdown paths. Batch is on by default; disabled = true leaves srv.Batch nil so the endpoints answer 503. Co-Authored-By: ornith-1.5-35b-a3b --- cmd/sift/serve.go | 27 ++++++++++++++++- dist/sift.toml.example | 9 ++++++ docs/implementer-log.md | 1 + internal/config/batch_test.go | 56 +++++++++++++++++++++++++++++++++++ internal/config/config.go | 40 +++++++++++++++++++++++++ 5 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 internal/config/batch_test.go diff --git a/cmd/sift/serve.go b/cmd/sift/serve.go index 6095982..1773304 100644 --- a/cmd/sift/serve.go +++ b/cmd/sift/serve.go @@ -10,9 +10,11 @@ import ( "os/signal" "strings" "syscall" + "time" "git.wntrmute.dev/kyle/sift/internal/api" "git.wntrmute.dev/kyle/sift/internal/auth" + "git.wntrmute.dev/kyle/sift/internal/batch" "git.wntrmute.dev/kyle/sift/internal/config" "git.wntrmute.dev/kyle/sift/internal/dcache" "git.wntrmute.dev/kyle/sift/internal/digest" @@ -119,7 +121,7 @@ func serveMain() { cachePath = "/var/lib/sift/cache.db" } var cache *dcache.DB - if cfg.Dixie.Chat != "" || cfg.Search.AnyEnabled() { + if cfg.Dixie.Chat != "" || cfg.Search.AnyEnabled() || !cfg.Batch.Disabled { c, err := dcache.Open(cachePath, 500<<20, log) if err != nil { log.Warn("cache", "err", err) @@ -127,6 +129,23 @@ func serveMain() { cache = c } } + if !cfg.Batch.Disabled && cache != nil { + now := time.Now().Unix() + if n := cache.MarkInFlightFailed("restart", now); n > 0 { + for i := 0; i < n; i++ { + srv.Prom.ObserveBatchJob("restart") + } + log.Info("batch reaped", "jobs", n) + } + srv.Batch = srv.NewBatchRunner(cache, batch.Config{ + TTL: config.ParseDuration(cfg.Batch.TTL, 0), + TargetsPerJob: cfg.Batch.TargetsPerJob, + JobsPerCaller: cfg.Batch.JobsPerCaller, + JobsTotal: cfg.Batch.JobsTotal, + Workers: cfg.Batch.Workers, + MaxBytes: config.ParseSize(cfg.Batch.MaxBytes, 0), + }) + } if cfg.Dixie.Chat != "" { srv.Digest = &digest.Engine{ ChatURL: cfg.Dixie.Chat, @@ -213,8 +232,14 @@ func serveMain() { case <-sig: stop() _ = hs.Close() + if srv.Batch != nil { + srv.Batch.Close() + } case err := <-errCh: stop() + if srv.Batch != nil { + srv.Batch.Close() + } if err != nil { log.Error("serve", "err", err) os.Exit(1) diff --git a/dist/sift.toml.example b/dist/sift.toml.example index c0b426c..6ff7801 100644 --- a/dist/sift.toml.example +++ b/dist/sift.toml.example @@ -54,3 +54,12 @@ listen = "127.0.0.1:8890" # url = "https://api.tavily.com/search" # daily_credit_cap = 500 # no_rerank = true + +# [batch] +# disabled = false +# targets_per_job = 32 +# jobs_per_caller = 1 +# jobs_total = 8 +# workers = 1 # at most 2: each worker fetches one target at a time +# ttl = "24h" # a job's results live this long after its last read or write +# max_bytes = "64MiB" # stored results; the oldest finished jobs go first diff --git a/docs/implementer-log.md b/docs/implementer-log.md index cd50411..a2823b7 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -24,6 +24,7 @@ One row per task session that ends in a commit. Newest last. | p6/05-fetchtarget | 2026-09-26 | done | 1 | gate: ok | none | Extracted v1fetch's per-target pipeline into new `internal/api/target.go`: `fetchTarget(ctx, c, t batch.Target) (batch.Result, error)`. Body moved unchanged in order — question truncation to 500 runes, mode default/check, wantCtx, doFetch, recover path, PDF branch (render, pages/grep errors, question-chunk limit), clean/raw branch, digest, context search, the fetch log line, and the envelope. `content` is returned as `Result.Content`; every other envelope key (`mode`,`untrusted`,`source`,`warnings`,`sizes`,`timings`, and `pdf`/`digest`/`context` when present) goes in `Result.Meta`, never content. `v1fetch` keeps reqN/caller/decode/400, builds a `batch.Target` (`URL` + `Mode`/`Question`/`Pages`/`Grep`/`JQ`/`Context`/`Recover` into `Options`), calls `fetchTarget`, `writeFetchErr` on error and `writeJSON(w,200,env)` on success where `env` is a fresh copy of `Meta` plus `"content"` (a copy, so `len(env)==len(Meta)+1` holds and Meta is not mutated). The two `writeErr` failures become `*fetch.Error` with the same status/msg/hint: 400 "invalid url" (bad mode), 400 "raw mode does not apply to PDFs". `doFetch` takes `context.Context` instead of `*http.Request` (its three `r.Context()` → `ctx`); `scrape` passes `r.Context()`. Removed now-unused `fmt`/`unicode/utf8`/`recover` imports, added `batch`. All api tests pass under -race -count=3; make gate ok; given test byte-identical. | | p6/06-batch-wiring | 2026-09-26 | done | 1 | gate: ok | none | New `internal/api/batch.go`: `NewBatchRunner` overwrites `cfg.Hooks` so Job→ObserveBatchJob, Target→ObserveBatchTarget(batchOutcome)+ObserveBatchTargetSeconds (+ObserveBatchResultBytes only when err==nil), and JobSeconds/Depth/Inflight→the matching `s.Prom` methods; the runner's FetchFunc is `batchFetch`, which tags ctx with an unexported `batchCtxKey`, runs under `requestDeadline`, and maps fetchTarget's errors (a *fetch.Error 429/503 → `fmt.Errorf("%s: %w", fe.Msg, batch.ErrRetryLater)` so the runner retries; any other *fetch.Error → TargetError with 502 rewritten to 500 "internal error" and `s.blockN.Add(1)` for 403; anything else → TargetError 500 logged Error with `s.logErr`). `batchOutcome` classifies nil→ok, ErrRetryLater-wrapped→busy, then by TargetError.Status (403 blocked, 429 busy, 504 timeout, 415 unsupported, 422 whose Msg starts "document too long"→too_long, other 4xx→upstream_4xx, 5xx→upstream_5xx, any other error→upstream_5xx). Added `Batch *batch.Runner` Server field (nil → the /v1/batch endpoints answer 503 "batch disabled"). `pdf.go` extractPDF charges the PDF pool to `c.Name+"/batch"` when the ctx carries the batch mark, single-flight key unchanged so a batch and an interactive extract of the same PDF still share one child; the only new log line carries caller and host="-" (never a URL/host). All api tests pass under -race -count=3; make gate ok; given file byte-identical. | | p6/07-api-endpoints | 2026-09-26 | done | 1 | gate: ok | none | The four /v1/batch handlers in new `internal/api/batch.go`: `batchSubmit` (POST) decodes `{"targets":[{url,mode,question,pages,grep,jq,context,recover}]}` flat like `/v1/fetch`, 400 "invalid batch" on bad json/empty list/empty url/bad mode (only empty,clean,raw,digest), then `PreCheck` per target → 422 "blocked: target : " with no job, then `Submit` mapping ErrTooManyTargets→422 "too many targets (max 32)", ErrCallerBusy→429+Retry-After 30, ErrBusy→429+Retry-After 60, ErrNoTargets→400, and 202 {success,job_id,status,targets,reused} (status "queued" new, current status reused). `batchStatus` (GET) and `batchResult` (GET {id}/{idx}) and `batchCancel` (DELETE) share `ownership` (unknown or not `c.Name` → 404, never 403; nil batch → 503); status is a summary with no results unless `?results=1` inlines the envelope (Meta+content) on done targets, idx validated 0..targets_total-1 else 404, cancel of a terminal job → 409 "job already finished" else 200 {success,status:cancelled}. `api.go` scoped to the rule-2 changes: `batchMaxBody` (256 KiB) const, the four routes in `New`, and `ServeHTTP` applying the per-path cap once (`/v1/batch`→256 KiB, `/v1/extract`→pdfMaxUpload, else 64 KiB) and collapsing every batch path to a fixed metrics label via `batchObservePath` (`/v1/batch` or `/v1/batch/{id}`, never a job id). Each handler logs one `batch` line with caller/job_id/targets/status-or-reused and never a URL/hostname/idem_key. Given test byte-identical; all api tests pass under -race -count=3; make gate ok. | +| p6/08-serve-config | 2026-09-26 | done | 1 | gate: ok | none | `[batch]` config in `internal/config`: the `Batch` type (disabled/targets_per_job/jobs_per_caller/jobs_total/workers/ttl/max_bytes) and `File.Batch`, plus `ParseSize` (trims spaces, accepts only binary KiB/MiB/GiB suffixes or a plain byte count, zero/negative/other → def). serve.go opens the dcache store when batch is on (`!cfg.Batch.Disabled`) alongside dixie/search — same path, same call, Warn on failure — then, before building the runner, reaps in-flight jobs a restart killed via `MarkInFlightFailed("restart", now)` with one `ObserveBatchJob("restart")` per reaped job and a `batch reaped` info log when n>0; builds `srv.Batch` from the config (TTL via `ParseDuration`, MaxBytes via `ParseSize`, runner defaults fill the zeros) only when enabled and the store opened; closes `srv.Batch` in both shutdown branches when non-nil. Added a commented `[batch]` block to dist/sift.toml.example. Given test byte-identical; all config and cmd tests pass under -race -count=3. | ## P5a review (owner, 2026-09-26) diff --git a/internal/config/batch_test.go b/internal/config/batch_test.go new file mode 100644 index 0000000..b4ea9c1 --- /dev/null +++ b/internal/config/batch_test.go @@ -0,0 +1,56 @@ +package config + +// P6 task 08: the [batch] block and ParseSize. + +import ( + "os" + "path/filepath" + "testing" +) + +func TestBatchBlock(t *testing.T) { + p := filepath.Join(t.TempDir(), "sift.toml") + body := "[batch]\ndisabled = false\ntargets_per_job = 16\njobs_per_caller = 2\njobs_total = 4\nworkers = 2\nttl = \"12h\"\nmax_bytes = \"32MiB\"\n" + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + f, err := Load(p) + if err != nil { + t.Fatal(err) + } + b := f.Batch + if b.Disabled || b.TargetsPerJob != 16 || b.JobsPerCaller != 2 || b.JobsTotal != 4 || b.Workers != 2 || b.TTL != "12h" || b.MaxBytes != "32MiB" { + t.Fatalf("batch block %+v", b) + } + // No block: zero values, and batch is on (the runner applies its own defaults). + empty := filepath.Join(t.TempDir(), "empty.toml") + if err := os.WriteFile(empty, []byte("listen = \":8443\"\n"), 0o600); err != nil { + t.Fatal(err) + } + if f, err := Load(empty); err != nil || f.Batch != (Batch{}) { + t.Fatalf("no [batch] block: %+v %v", f.Batch, err) + } +} + +func TestParseSize(t *testing.T) { + const def = 7 + for _, c := range []struct { + in string + want int64 + }{ + {"64MiB", 64 << 20}, + {"512KiB", 512 << 10}, + {"1GiB", 1 << 30}, + {"1000", 1000}, + {" 2MiB ", 2 << 20}, + {"", def}, + {"junk", def}, + {"-1MiB", def}, + {"0", def}, + {"12MB", def}, // decimal units are not accepted: say MiB + } { + if got := ParseSize(c.in, def); got != c.want { + t.Errorf("ParseSize(%q) = %d, want %d", c.in, got, c.want) + } + } +} diff --git a/internal/config/config.go b/internal/config/config.go index e0205de..796007c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2,6 +2,8 @@ package config import ( "os" + "strconv" + "strings" "time" "github.com/pelletier/go-toml/v2" @@ -17,6 +19,19 @@ type File struct { Digest Digest `toml:"digest"` Context Context `toml:"context"` Search Search `toml:"search"` + Batch Batch `toml:"batch"` +} + +// Batch is the [batch] block. Batch is on by default: no block means the runner runs with its own +// defaults. disabled = true turns it off and the endpoints answer 503. +type Batch struct { + Disabled bool `toml:"disabled"` + TargetsPerJob int `toml:"targets_per_job"` + JobsPerCaller int `toml:"jobs_per_caller"` + JobsTotal int `toml:"jobs_total"` + Workers int `toml:"workers"` + TTL string `toml:"ttl"` + MaxBytes string `toml:"max_bytes"` } type Dixie struct { @@ -110,3 +125,28 @@ func ParseDuration(s string, def time.Duration) time.Duration { } return d } + +// ParseSize parses a positive byte count: a plain integer, or one with a binary suffix KiB, MiB or +// GiB. Anything else, zero or negative returns def. +func ParseSize(s string, def int64) int64 { + s = strings.TrimSpace(s) + if s == "" { + return def + } + var mult int64 + switch { + case strings.HasSuffix(s, "KiB"): + mult, s = 1<<10, s[:len(s)-3] + case strings.HasSuffix(s, "MiB"): + mult, s = 1<<20, s[:len(s)-3] + case strings.HasSuffix(s, "GiB"): + mult, s = 1<<30, s[:len(s)-3] + default: + mult = 1 + } + n, err := strconv.ParseInt(s, 10, 64) + if err != nil || n <= 0 { + return def + } + return n * mult +} From be48ecb550ca4a4be5cb7de2e5e4cdc53a117c59 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 23:13:48 -0700 Subject: [PATCH 09/10] cli: sift batch submit|status|get|cancel; README and skill Add `sift batch` so agents reach the batch endpoint through the CLI like `fetch` and `search`. `batchcmd.go` has `parseBatchArgs` (verb plus the unchanged args after it; submit takes one arg, status a job id optionally preceded by `--results`, get a job id plus a non-negative-integer idx, cancel a job id, else a usage error), `batchMain` dispatch, one small function per verb, and `batchDo` (bearer header, decode the envelope, print it on stdout, on non-2xx the error to stderr and `exitFor`). `submitMain` reads the file or stdin and validates locally: a JSON object with a non-empty `targets` array, or a bare array of targets or URL strings wrapped into `{"targets": [...]}`. README gains a "Batch" section (endpoints, serial-per-job/one-worker/ round-robin model, caps, idempotency, lease/TTL/restart, summary-vs-results, redaction, metrics); SKILL.md gains a "Batch" section. Co-Authored-By: ornith-1.5-35b-a3b --- README.md | 43 ++++++++- cmd/sift/batch_test.go | 46 +++++++++ cmd/sift/batchcmd.go | 200 ++++++++++++++++++++++++++++++++++++++++ cmd/sift/main.go | 3 + docs/implementer-log.md | 1 + skill/SKILL.md | 27 ++++++ 6 files changed, 317 insertions(+), 3 deletions(-) create mode 100644 cmd/sift/batch_test.go create mode 100644 cmd/sift/batchcmd.go diff --git a/README.md b/README.md index 5ffb2c4..b11d079 100644 --- a/README.md +++ b/README.md @@ -34,9 +34,10 @@ sift serve [-listen addr] [-config /etc/sift/config.toml] [-keys /var/lib/sift/ sift fetch [--question q] [--mode clean|raw|digest] [--jq filter] [--pages a-b] [--grep re] [--no-context] [--recover] [--json] sift extract [--pages a-b] [--grep re] [--question q] [--mode clean|digest] [--json] -sift search "" [--k n] [--json] -sift keys add --caller name [--workspace ws] [--out file] | list | revoke --caller name -sift version + sift search "" [--k n] [--json] + sift batch submit|status|get|cancel + sift keys add --caller name [--workspace ws] [--out file] | list | revoke --caller name + sift version ``` `sift fetch`, `sift extract` and `sift search` are clients: they read `url` and `key_file` from @@ -46,6 +47,42 @@ answer exactly as `sift fetch` does for a PDF URL — `--pages`, `--grep` and `- same, and a file over 100 MiB is refused before the upload. The agent-facing guide is [`skill/SKILL.md`](skill/SKILL.md). +## Batch + +`sift batch` runs several fetches as one job, so an agent can read a handful of URLs later instead +of a loop of `sift fetch`. The four endpoints: + +| Endpoint | What | +|---|---| +| `POST /v1/batch` | `{"targets":[…]}` → `202 {"success","job_id","status","targets","reused"}` | +| `GET /v1/batch/{id}` | summary; `?results=1` inlines each done target's `/v1/fetch` envelope | +| `GET /v1/batch/{id}/{idx}` | one target's result, as `sift fetch --json` | +| `DELETE /v1/batch/{id}` | cancel a queued job → `200 {"success","status"}` | + +`sift batch submit ` reads a JSON object with a non-empty `targets` array locally, +or a bare array of targets or URL strings (each string becomes `{"url": s}`), then posts it. `status` +prints the summary (or the body with `?results=1`); `get ` prints one result; `cancel` +stops a queued job. + +A job runs **serially** (target `i+1` never starts before `i` is stored) with **one worker by +default** (see `[batch]` config) and the runner rotates jobs **round-robin** while several are live. +Caps: **32 targets** per job, **1 live job per caller**, **8 live jobs** in all. The posted body is +capped at **256 KiB**. A target that is blocked, times out, or errors is recorded and the job +continues; a job whose targets are all done is `done`, all failed `failed`, mixed `partial`. + +**Idempotency**: the same list from the same caller returns the same job while it is live or already +has results (`reused: true`), so a retry does not start a second job. Each job carries a **lease** +that a status read renews; a job whose lease has expired for more than the **24 h TTL** is swept +along with its targets. On restart, jobs in flight when the server died are marked failed so a retry +re-submits them. + +A `status` read is a **summary** (per-target status, never the content); `?results=1` inlines the +full envelope for each done target, so it can be large — prefer `get ` for one result. +Restricted callers see the same endpoints; their results are **redacted** the same way a restricted +`sift fetch` is. Batch counters are `sift_batch_jobs_total{outcome}`, +`sift_batch_targets_total{outcome}`, `sift_batch_queue_depth`, `sift_batch_targets_inflight`, +`sift_batch_job_seconds`, `sift_batch_target_seconds` and `sift_batch_result_bytes`. + ## Configuration `/etc/sift/config.toml` (`$SIFT_CONFIG`); every key is shown in diff --git a/cmd/sift/batch_test.go b/cmd/sift/batch_test.go new file mode 100644 index 0000000..7315e11 --- /dev/null +++ b/cmd/sift/batch_test.go @@ -0,0 +1,46 @@ +package main + +import ( + "slices" + "testing" +) + +func TestParseBatchArgs(t *testing.T) { + cases := []struct { + args []string + verb string + rest []string + ok bool + }{ + {[]string{"submit", "targets.json"}, "submit", []string{"targets.json"}, true}, + {[]string{"submit", "-"}, "submit", []string{"-"}, true}, + {[]string{"status", "job-123"}, "status", []string{"job-123"}, true}, + {[]string{"status", "--results", "job-123"}, "status", []string{"--results", "job-123"}, true}, + {[]string{"get", "job-123", "4"}, "get", []string{"job-123", "4"}, true}, + {[]string{"cancel", "job-123"}, "cancel", []string{"job-123"}, true}, + {[]string{}, "", nil, false}, + {[]string{"submit"}, "", nil, false}, + {[]string{"status"}, "", nil, false}, + {[]string{"status", "a", "b"}, "", nil, false}, + {[]string{"status", "--results"}, "", nil, false}, + {[]string{"get", "job-123"}, "", nil, false}, + {[]string{"get", "job-123", "x"}, "", nil, false}, + {[]string{"get", "job-123", "-1"}, "", nil, false}, + {[]string{"cancel"}, "", nil, false}, + {[]string{"bogus", "x"}, "", nil, false}, + } + for _, c := range cases { + verb, rest, err := parseBatchArgs(c.args) + if c.ok { + if err != nil { + t.Errorf("%v: unexpected error %v", c.args, err) + continue + } + if verb != c.verb || !slices.Equal(rest, c.rest) { + t.Errorf("%v: got verb=%q rest=%q", c.args, verb, rest) + } + } else if err == nil { + t.Errorf("%v: want an error", c.args) + } + } +} diff --git a/cmd/sift/batchcmd.go b/cmd/sift/batchcmd.go new file mode 100644 index 0000000..ef2986b --- /dev/null +++ b/cmd/sift/batchcmd.go @@ -0,0 +1,200 @@ +package main + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "os" + "strconv" +) + +const batchUsage = `usage: sift batch submit|status|get|cancel + sift batch submit + sift batch status [--results] + sift batch get + sift batch cancel ` + +// parseBatchArgs validates a `sift batch` command line (without "batch"). verb is +// the subcommand; rest is the arguments after it, unchanged. +func parseBatchArgs(args []string) (verb string, rest []string, err error) { + if len(args) == 0 { + return "", nil, fmt.Errorf("%s", batchUsage) + } + verb, rest = args[0], args[1:] + switch verb { + case "submit": + if len(rest) != 1 { + return "", nil, fmt.Errorf("%s", batchUsage) + } + case "status": + switch len(rest) { + case 1: + if rest[0] == "--results" { + return "", nil, fmt.Errorf("%s", batchUsage) + } + case 2: + if rest[0] != "--results" { + return "", nil, fmt.Errorf("%s", batchUsage) + } + default: + return "", nil, fmt.Errorf("%s", batchUsage) + } + case "get": + if len(rest) != 2 { + return "", nil, fmt.Errorf("%s", batchUsage) + } + idx, e := strconv.Atoi(rest[1]) + if e != nil || idx < 0 { + return "", nil, fmt.Errorf("%s", batchUsage) + } + case "cancel": + if len(rest) != 1 { + return "", nil, fmt.Errorf("%s", batchUsage) + } + default: + return "", nil, fmt.Errorf("%s", batchUsage) + } + return verb, rest, nil +} + +func batchMain() { + verb, rest, err := parseBatchArgs(os.Args[1:]) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + switch verb { + case "submit": + submitMain(rest) + case "status": + statusMain(rest) + case "get": + getMain(rest) + case "cancel": + cancelMain(rest) + } +} + +// submitMain reads the targets file (or stdin when the argument is "-") and +// validates it locally before calling the server: a JSON object with a +// non-empty targets array, or a bare array of targets or URL strings wrapped +// into {"targets": [...]}. +func submitMain(rest []string) { + var r io.Reader = os.Stdin + if rest[0] != "-" { + f, err := os.Open(rest[0]) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(5) + } + defer f.Close() + r = f + } + raw, err := io.ReadAll(r) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(5) + } + body, err := buildTargetsBody(raw) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + batchDo(http.MethodPost, "/v1/batch", bytes.NewReader(body)) +} + +// buildTargetsBody accepts the three accepted shapes and returns the +// {"targets": [...]} body to send. A bare array of URL strings gets each string +// wrapped as {"url": s}; bare target objects pass through. +func buildTargetsBody(raw []byte) ([]byte, error) { + var probe any + if err := json.Unmarshal(raw, &probe); err != nil { + return nil, fmt.Errorf("targets: not valid JSON: %w", err) + } + switch p := probe.(type) { + case map[string]any: + arr, ok := p["targets"].([]any) + if !ok || len(arr) == 0 { + return nil, fmt.Errorf("targets: a JSON object needs a non-empty targets array") + } + return raw, nil + case []any: + if len(p) == 0 { + return nil, fmt.Errorf("targets: an empty array") + } + out := make([]any, 0, len(p)) + for _, el := range p { + switch e := el.(type) { + case map[string]any: + out = append(out, e) + case string: + out = append(out, map[string]any{"url": e}) + default: + return nil, fmt.Errorf("targets: array elements must be URLs or objects") + } + } + return json.Marshal(map[string]any{"targets": out}) + default: + return nil, fmt.Errorf("targets: expected a JSON object or array") + } +} + +func statusMain(rest []string) { + id := rest[len(rest)-1] + path := "/v1/batch/" + url.PathEscape(id) + if len(rest) == 2 && rest[0] == "--results" { + path += "?results=1" + } + batchDo(http.MethodGet, path, nil) +} + +func getMain(rest []string) { + path := "/v1/batch/" + url.PathEscape(rest[0]) + "/" + url.PathEscape(rest[1]) + batchDo(http.MethodGet, path, nil) +} + +func cancelMain(rest []string) { + batchDo(http.MethodDelete, "/v1/batch/"+url.PathEscape(rest[0]), nil) +} + +// batchDo calls base+path with the bearer header, decodes the JSON envelope, +// and prints it on stdout. On a non-2xx it writes the error to stderr and exits +// with exitFor, as fetch does. +func batchDo(method, path string, body io.Reader) { + base, key, err := loadClient() + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(5) + } + req, err := http.NewRequest(method, base+path, body) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(5) + } + req.Header.Set("Authorization", "Bearer "+key) + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(5) + } + defer resp.Body.Close() + var env map[string]any + if err := json.NewDecoder(resp.Body).Decode(&env); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(5) + } + if resp.StatusCode != 200 { + msg, _ := env["error"].(string) + fmt.Fprintln(os.Stderr, msg) + os.Exit(exitFor(resp.StatusCode, msg)) + } + enc := json.NewEncoder(os.Stdout) + enc.SetIndent("", " ") + _ = enc.Encode(env) +} diff --git a/cmd/sift/main.go b/cmd/sift/main.go index b42c8fd..a9c83c0 100644 --- a/cmd/sift/main.go +++ b/cmd/sift/main.go @@ -26,6 +26,8 @@ func main() { extractMain() case "search": searchMain() + case "batch": + batchMain() case "keys": keysMain() case "pdftext": @@ -49,6 +51,7 @@ Commands: sift fetch fetch via the service sift extract extract a local PDF via the service sift search "" search via the service + sift batch submit|status|get|cancel run or watch a batch of fetches sift keys add|list|revoke sift version `, version) diff --git a/docs/implementer-log.md b/docs/implementer-log.md index a2823b7..498ca05 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -25,6 +25,7 @@ One row per task session that ends in a commit. Newest last. | p6/06-batch-wiring | 2026-09-26 | done | 1 | gate: ok | none | New `internal/api/batch.go`: `NewBatchRunner` overwrites `cfg.Hooks` so Job→ObserveBatchJob, Target→ObserveBatchTarget(batchOutcome)+ObserveBatchTargetSeconds (+ObserveBatchResultBytes only when err==nil), and JobSeconds/Depth/Inflight→the matching `s.Prom` methods; the runner's FetchFunc is `batchFetch`, which tags ctx with an unexported `batchCtxKey`, runs under `requestDeadline`, and maps fetchTarget's errors (a *fetch.Error 429/503 → `fmt.Errorf("%s: %w", fe.Msg, batch.ErrRetryLater)` so the runner retries; any other *fetch.Error → TargetError with 502 rewritten to 500 "internal error" and `s.blockN.Add(1)` for 403; anything else → TargetError 500 logged Error with `s.logErr`). `batchOutcome` classifies nil→ok, ErrRetryLater-wrapped→busy, then by TargetError.Status (403 blocked, 429 busy, 504 timeout, 415 unsupported, 422 whose Msg starts "document too long"→too_long, other 4xx→upstream_4xx, 5xx→upstream_5xx, any other error→upstream_5xx). Added `Batch *batch.Runner` Server field (nil → the /v1/batch endpoints answer 503 "batch disabled"). `pdf.go` extractPDF charges the PDF pool to `c.Name+"/batch"` when the ctx carries the batch mark, single-flight key unchanged so a batch and an interactive extract of the same PDF still share one child; the only new log line carries caller and host="-" (never a URL/host). All api tests pass under -race -count=3; make gate ok; given file byte-identical. | | p6/07-api-endpoints | 2026-09-26 | done | 1 | gate: ok | none | The four /v1/batch handlers in new `internal/api/batch.go`: `batchSubmit` (POST) decodes `{"targets":[{url,mode,question,pages,grep,jq,context,recover}]}` flat like `/v1/fetch`, 400 "invalid batch" on bad json/empty list/empty url/bad mode (only empty,clean,raw,digest), then `PreCheck` per target → 422 "blocked: target : " with no job, then `Submit` mapping ErrTooManyTargets→422 "too many targets (max 32)", ErrCallerBusy→429+Retry-After 30, ErrBusy→429+Retry-After 60, ErrNoTargets→400, and 202 {success,job_id,status,targets,reused} (status "queued" new, current status reused). `batchStatus` (GET) and `batchResult` (GET {id}/{idx}) and `batchCancel` (DELETE) share `ownership` (unknown or not `c.Name` → 404, never 403; nil batch → 503); status is a summary with no results unless `?results=1` inlines the envelope (Meta+content) on done targets, idx validated 0..targets_total-1 else 404, cancel of a terminal job → 409 "job already finished" else 200 {success,status:cancelled}. `api.go` scoped to the rule-2 changes: `batchMaxBody` (256 KiB) const, the four routes in `New`, and `ServeHTTP` applying the per-path cap once (`/v1/batch`→256 KiB, `/v1/extract`→pdfMaxUpload, else 64 KiB) and collapsing every batch path to a fixed metrics label via `batchObservePath` (`/v1/batch` or `/v1/batch/{id}`, never a job id). Each handler logs one `batch` line with caller/job_id/targets/status-or-reused and never a URL/hostname/idem_key. Given test byte-identical; all api tests pass under -race -count=3; make gate ok. | | p6/08-serve-config | 2026-09-26 | done | 1 | gate: ok | none | `[batch]` config in `internal/config`: the `Batch` type (disabled/targets_per_job/jobs_per_caller/jobs_total/workers/ttl/max_bytes) and `File.Batch`, plus `ParseSize` (trims spaces, accepts only binary KiB/MiB/GiB suffixes or a plain byte count, zero/negative/other → def). serve.go opens the dcache store when batch is on (`!cfg.Batch.Disabled`) alongside dixie/search — same path, same call, Warn on failure — then, before building the runner, reaps in-flight jobs a restart killed via `MarkInFlightFailed("restart", now)` with one `ObserveBatchJob("restart")` per reaped job and a `batch reaped` info log when n>0; builds `srv.Batch` from the config (TTL via `ParseDuration`, MaxBytes via `ParseSize`, runner defaults fill the zeros) only when enabled and the store opened; closes `srv.Batch` in both shutdown branches when non-nil. Added a commented `[batch]` block to dist/sift.toml.example. Given test byte-identical; all config and cmd tests pass under -race -count=3. | +| p6/09-cli-docs | 2026-09-26 | done | 1 | gate: ok | none | `cmd/sift/batchcmd.go`: `parseBatchArgs` (verb + unchanged `rest`; submit takes one arg, status one job id optionally preceded by `--results`, get a job id plus a non-negative-integer idx, cancel one job id, else a usage error), `batchMain` dispatch, one small function per verb, and `batchDo` (bearer header, decode the JSON envelope, print it indented on stdout, on non-2xx print `env["error"]` to stderr and `exitFor`). `submitMain` reads the file or stdin and `buildTargetsBody` accepts a JSON object with a non-empty `targets` array, a bare array of target objects, or a bare array of URL strings (each wrapped `{"url": s}`), validating locally before the call. `main.go`: one `case "batch"` and one help line. README "Batch" section (four endpoints, serial-per-job/one-worker/round-robin model, 32 targets/1-per-caller/8-total caps, 256 KiB body, idempotency, lease/TTL/restart, summary-vs-results, restricted redaction, `sift_batch_*` metrics); SKILL.md "Batch" section. Given test byte-identical; all cmd/sift tests pass under -race -count=3. | ## P5a review (owner, 2026-09-26) diff --git a/skill/SKILL.md b/skill/SKILL.md index 1b3c381..77d5391 100644 --- a/skill/SKILL.md +++ b/skill/SKILL.md @@ -55,6 +55,33 @@ Useful flags: `--mode digest|clean|raw` (default `digest` when `--question` is g else `clean`), `--no-context` (skip Honcho lookup), `--recover` (server-side archive ladder for blocked pages), `--json` (full envelope: content, source, context, sizes, warnings). +## Batch + +When you will read the content of several URLs later (a shortlist to compare), submit them once +instead of a loop of `sift fetch`: + +```bash +# one JSON object, or a bare array of URLs +sift batch submit targets.json +# ["https://a", "https://b"] also works; each string becomes {"url": ...} +``` + +Then poll and read: + +```bash +sift batch status # summary: per-target status, no content +sift batch status --results # everything at once — can be large, avoid unless needed +sift batch get # one target's result, as `sift fetch --json` +sift batch cancel # stop a queued job +``` + +- Submit once; a retry of the same list from this caller returns the same job (`reused`), it does + not start a second. +- Don't use `--results` unless you need every result at once — a batch of 32 can be several + megabytes. Read each piece with `sift batch get `. +- A batch is slow on purpose: it runs one target at a time per job. For one URL you need now, use + `sift fetch`. + ## PDFs PDFs work: `sift fetch` extracts them and returns the text. A short one comes back whole; a From a452aa16a1e532cc8eb28529259bfa27ab7fd6b3 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Sat, 26 Sep 2026 23:27:47 -0700 Subject: [PATCH 10/10] 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 --- cmd/sift/batch_review_test.go | 52 ++++++++ cmd/sift/batchcmd.go | 2 +- internal/api/batch.go | 18 ++- internal/api/batch_review_test.go | 29 +++++ internal/batch/batch.go | 62 +++++----- internal/batch/review_test.go | 177 +++++++++++++++++++++++++++ internal/batch/worker.go | 41 +++++-- internal/config/batch_review_test.go | 10 ++ internal/config/config.go | 3 +- internal/dcache/batch.go | 50 ++++++-- internal/dcache/batch_review_test.go | 25 ++++ 11 files changed, 406 insertions(+), 63 deletions(-) create mode 100644 cmd/sift/batch_review_test.go create mode 100644 internal/api/batch_review_test.go create mode 100644 internal/batch/review_test.go create mode 100644 internal/config/batch_review_test.go create mode 100644 internal/dcache/batch_review_test.go diff --git a/cmd/sift/batch_review_test.go b/cmd/sift/batch_review_test.go new file mode 100644 index 0000000..aeb8a9a --- /dev/null +++ b/cmd/sift/batch_review_test.go @@ -0,0 +1,52 @@ +package main + +// P6 owner review: the batch CLI treats every 2xx as success. POST /v1/batch answers 202, and +// batchDo exited 1 on it with an empty message. + +import ( + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestBatchDoAccepts202(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != "Bearer k" { + w.WriteHeader(401) + _, _ = io.WriteString(w, `{"error":"unauthorized"}`) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = io.WriteString(w, `{"success":true,"job_id":"j1","status":"queued","targets":1,"reused":false}`) + })) + defer srv.Close() + dir := t.TempDir() + keyPath := filepath.Join(dir, "key") + if err := os.WriteFile(keyPath, []byte("k\n"), 0o600); err != nil { + t.Fatal(err) + } + cfg := filepath.Join(dir, "client.toml") + if err := os.WriteFile(cfg, []byte("url = \""+srv.URL+"\"\nkey_file = \""+keyPath+"\"\n"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("SIFT_CLIENT", cfg) + + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + old := os.Stdout + os.Stdout = w + batchDo(http.MethodPost, "/v1/batch", strings.NewReader(`{"targets":[{"url":"https://example.com/"}]}`)) + os.Stdout = old + _ = w.Close() + out, _ := io.ReadAll(r) + if !strings.Contains(string(out), `"job_id": "j1"`) { + t.Fatalf("stdout %q, want the 202 body printed", out) + } +} diff --git a/cmd/sift/batchcmd.go b/cmd/sift/batchcmd.go index ef2986b..2c56d2e 100644 --- a/cmd/sift/batchcmd.go +++ b/cmd/sift/batchcmd.go @@ -189,7 +189,7 @@ func batchDo(method, path string, body io.Reader) { fmt.Fprintln(os.Stderr, err) os.Exit(5) } - if resp.StatusCode != 200 { + if resp.StatusCode < 200 || resp.StatusCode > 299 { // submit answers 202 Accepted msg, _ := env["error"].(string) fmt.Fprintln(os.Stderr, msg) os.Exit(exitFor(resp.StatusCode, msg)) diff --git a/internal/api/batch.go b/internal/api/batch.go index 1945972..50f7aac 100644 --- a/internal/api/batch.go +++ b/internal/api/batch.go @@ -66,14 +66,16 @@ func (s *Server) batchFetch(ctx context.Context, c *auth.Caller, t batch.Target) case 429, 503: return batch.Result{}, fmt.Errorf("%s: %w", fe.Msg, batch.ErrRetryLater) } - if fe.Status == 502 { - fe.Status = 500 - fe.Msg = "internal error" + // Copy, never mutate: the PDF pool's single-flight may share this error with an + // interactive request. + status, msg := fe.Status, fe.Msg + if status == 502 { + status, msg = 500, "internal error" } - if fe.Status == 403 { + if status == 403 { s.blockN.Add(1) } - return batch.Result{}, &batch.TargetError{Status: fe.Status, Msg: fe.Msg, Hint: fe.Hint} + return batch.Result{}, &batch.TargetError{Status: status, Msg: msg, Hint: fe.Hint} } s.Log.Error("internal", "caller", c.Name, "host", "-", "err", s.logErr(c, err)) return batch.Result{}, &batch.TargetError{Status: 500, Msg: "internal error"} @@ -158,6 +160,10 @@ func (s *Server) batchSubmit(w http.ResponseWriter, r *http.Request) { if c == nil { return } + if s.Batch == nil { // [batch] disabled, or the store did not open + writeErr(w, 503, "batch disabled", "") + return + } var req struct { Targets []v1Req `json:"targets"` } @@ -264,7 +270,7 @@ func (s *Server) batchResult(w http.ResponseWriter, r *http.Request) { return } idx, err := strconv.Atoi(r.PathValue("idx")) - if err != nil || idx < 0 || idx >= j.TargetsTotal { + if err != nil || idx < 0 || idx >= j.TargetsTotal || idx >= len(ts) { writeErr(w, 404, "not found", "") return } diff --git a/internal/api/batch_review_test.go b/internal/api/batch_review_test.go new file mode 100644 index 0000000..0e526be --- /dev/null +++ b/internal/api/batch_review_test.go @@ -0,0 +1,29 @@ +package api + +// P6 owner review: with batch off (disabled, or the store did not open) every batch endpoint +// answers 503; none of them panics on the nil runner. + +import ( + "net/http" + "testing" +) + +func TestBatchDisabled(t *testing.T) { + s, keys := newBatchServer(t, "test") + s.Batch.Close() + s.Batch = nil + for _, c := range []struct{ method, path string }{ + {http.MethodPost, "/v1/batch"}, + {http.MethodGet, "/v1/batch/x"}, + {http.MethodGet, "/v1/batch/x/0"}, + {http.MethodDelete, "/v1/batch/x"}, + } { + var body any + if c.method == http.MethodPost { + body = map[string]any{"targets": []any{map[string]any{"url": "http://127.0.0.1/"}}} + } + if rec := batchReq(t, s, c.method, c.path, keys["test"], body); rec.Code != 503 { + t.Errorf("%s %s: %d %s, want 503", c.method, c.path, rec.Code, rec.Body.String()) + } + } +} diff --git a/internal/batch/batch.go b/internal/batch/batch.go index bddc3d2..49677b0 100644 --- a/internal/batch/batch.go +++ b/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() diff --git a/internal/batch/review_test.go b/internal/batch/review_test.go new file mode 100644 index 0000000..2712e3a --- /dev/null +++ b/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) + } +} diff --git a/internal/batch/worker.go b/internal/batch/worker.go index 5cb64df..caf223a 100644 --- a/internal/batch/worker.go +++ b/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) diff --git a/internal/config/batch_review_test.go b/internal/config/batch_review_test.go new file mode 100644 index 0000000..56ff0bb --- /dev/null +++ b/internal/config/batch_review_test.go @@ -0,0 +1,10 @@ +package config + +import "testing" + +// P6 owner review: a size too large for int64 falls back to the default instead of wrapping negative. +func TestParseSizeOverflow(t *testing.T) { + if got := ParseSize("9223372036854775807GiB", 7); got != 7 { + t.Fatalf("ParseSize(huge) = %d, want the default", got) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 796007c..4dc5299 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,6 +1,7 @@ package config import ( + "math" "os" "strconv" "strings" @@ -145,7 +146,7 @@ func ParseSize(s string, def int64) int64 { mult = 1 } n, err := strconv.ParseInt(s, 10, 64) - if err != nil || n <= 0 { + if err != nil || n <= 0 || n > math.MaxInt64/mult { // the last: n*mult would overflow return def } return n * mult diff --git a/internal/dcache/batch.go b/internal/dcache/batch.go index 928113b..8a44759 100644 --- a/internal/dcache/batch.go +++ b/internal/dcache/batch.go @@ -134,8 +134,14 @@ func (d *DB) PutBatchTargets(jobID string, targets []BatchTarget) error { if d == nil { return fmt.Errorf("dcache: nil db") } + // One transaction: a job never has only some of its targets. + tx, err := d.sql.Begin() + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() for _, t := range targets { - if _, err := d.sql.Exec(` + if _, err := tx.Exec(` INSERT INTO batch_target(job_id, idx, url, options, status, result, error, hint) VALUES(?,?,?,?,?,?,?,?) `, t.JobID, t.Idx, t.URL, t.Options, t.Status, @@ -143,7 +149,7 @@ func (d *DB) PutBatchTargets(jobID string, targets []BatchTarget) error { return err } } - return nil + return tx.Commit() } func (d *DB) BatchTargets(jobID string) ([]BatchTarget, bool) { @@ -249,16 +255,25 @@ func (d *DB) SweepBatch(now int64, ttl time.Duration) int { return 0 } ttlSec := int64(ttl.Seconds()) - if _, err := d.sql.Exec(` - DELETE FROM batch_target WHERE job_id IN - (SELECT id FROM batch_job WHERE lease + ? <= ?) - `, ttlSec, now); err != nil { + // A queued or running job is never swept: it is still in a worker's rotation. The restart + // reaper marks such jobs failed, and they are swept after that. Both deletes are one + // transaction, so a job never loses its targets but keeps its row. + tx, err := d.sql.Begin() + if err != nil { return 0 } - res, err := d.sql.Exec(`DELETE FROM batch_job WHERE lease + ? <= ?`, ttlSec, now) + defer func() { _ = tx.Rollback() }() + const expired = `SELECT id FROM batch_job WHERE lease + ? <= ? AND status NOT IN ('queued','running')` + if _, err := tx.Exec(`DELETE FROM batch_target WHERE job_id IN (`+expired+`)`, ttlSec, now); err != nil { + return 0 + } + res, err := tx.Exec(`DELETE FROM batch_job WHERE id IN (`+expired+`)`, ttlSec, now) if err != nil { return 0 } + if err := tx.Commit(); err != nil { + return 0 + } n, _ := res.RowsAffected() return int(n) } @@ -279,10 +294,7 @@ func (d *DB) EvictBatchOldest(maxBytes int64) int { if err != nil { break } - if _, err := d.sql.Exec(`DELETE FROM batch_target WHERE job_id = ?`, id); err != nil { - break - } - if _, err := d.sql.Exec(`DELETE FROM batch_job WHERE id = ?`, id); err != nil { + if err := d.deleteBatchJob(id); err != nil { break } evicted++ @@ -312,3 +324,19 @@ func (d *DB) BatchNbytes() int64 { _ = d.sql.QueryRow(`SELECT IFNULL(SUM(nbytes),0) FROM batch_job`).Scan(&n) return n.Int64 } + +// deleteBatchJob removes a job and its targets in one transaction. +func (d *DB) deleteBatchJob(id string) error { + tx, err := d.sql.Begin() + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + if _, err := tx.Exec(`DELETE FROM batch_target WHERE job_id = ?`, id); err != nil { + return err + } + if _, err := tx.Exec(`DELETE FROM batch_job WHERE id = ?`, id); err != nil { + return err + } + return tx.Commit() +} diff --git a/internal/dcache/batch_review_test.go b/internal/dcache/batch_review_test.go new file mode 100644 index 0000000..841becf --- /dev/null +++ b/internal/dcache/batch_review_test.go @@ -0,0 +1,25 @@ +package dcache + +// P6 owner review: the sweep never removes a live job. + +import ( + "testing" + "time" +) + +func TestSweepSparesLiveJobs(t *testing.T) { + db := newBatchDB(t) + for _, st := range []string{"queued", "running", "done"} { + if err := db.CreateBatchJob(BatchJob{ID: st, Caller: "a", CallerClass: ClassFleet, IDemKey: st, Status: st, TargetsTotal: 1, CreatedAt: 1, Lease: 1}); err != nil { + t.Fatal(err) + } + } + if n := db.SweepBatch(1_000_000, time.Hour); n != 1 { + t.Fatalf("swept %d, want only the done job", n) + } + for _, id := range []string{"queued", "running"} { + if _, ok := db.BatchJob(id); !ok { + t.Fatalf("live job %s was swept", id) + } + } +}