P6 plan, owner review folded in: caller travels with the job, 1–2 round-robin workers, busy retry, summary reads, local-only tests, 8 tasks
Kyle: "Fold them into the plan." What changed and why is in docs/P6.md §14. In short:
- FetchFunc gets the *auth.Caller (the given api test could not compile, and a fetch with no
caller would bypass per-caller gates, the pool limit, the cache class and redaction);
- store reads never bump the lease (task 01's rule contradicted its own test); the API Touches;
- 1-2 workers round-robin over live jobs, so a batch holds at most 2 of the 8 global slots;
busy targets retry in the runner (bounded); batch PDFs use their own pool name; 210 s deadline;
- the api tests no longer touch the network or race the cancel; private names via fake DNS;
- failed/cancelled jobs are not reused; GET is a summary plus GET …/{idx} and ?results=1;
- the 256 KiB body cap moves into ServeHTTP; a fixed metrics label for /v1/batch/{id};
- guard.PreCheck (given verbatim) replaces guard.ParseURL at submit;
- 5 tasks became 8 (refactor, wiring, endpoints and serve/config split), each restating the
APIs it uses; every given test compiles against a panic-only skeleton and fails on it.
P6 starts after P5c: task 04 moves v1fetch, which P5b changes. Recheck the api given tests
against post-P5c master before the run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,149 @@
|
||||
# P6 task 05: the runner wired to `fetchTarget` and the metrics
|
||||
|
||||
**Branch:** `p6` (`git switch p6`; `git status --short` must be empty, otherwise stop)
|
||||
**Commit subject:** `api: NewBatchRunner — the batch fetch adapter, retry on busy, outcome metrics; guard.PreCheck`
|
||||
|
||||
## Goal
|
||||
|
||||
Build the production runner: its `FetchFunc` is an adapter over `fetchTarget` (task 04), and its
|
||||
hooks feed the `sift_batch_*` metrics (task 03). **No HTTP endpoints yet** (task 06). Also add
|
||||
`guard.PreCheck`, which task 06 uses to refuse a target at submit time.
|
||||
|
||||
## Files
|
||||
|
||||
- Copy: `_files/api/batchrun_test.go` → `internal/api/batchrun_test.go`, and
|
||||
`_files/guard/precheck_test.go` → `internal/guard/precheck_test.go`
|
||||
- Create: `internal/api/batch.go` (`NewBatchRunner`, `batchFetch`, `batchOutcome`)
|
||||
- Modify:
|
||||
- `internal/api/api.go`: only the `Batch` field.
|
||||
- `internal/api/pdf.go`: only the pool caller name (rule 3).
|
||||
- `internal/guard/guard.go`: add **exactly** this method, verbatim. It is the only change allowed
|
||||
in the guard package.
|
||||
- `docs/implementer-log.md`
|
||||
|
||||
```go
|
||||
// PreCheck is CheckURL without name resolution: scheme, userinfo, host and port, and an IP
|
||||
// literal checked against the address rules. A hostname is not resolved here; the fetch-time
|
||||
// CheckURL and the dial-time Control still apply to it.
|
||||
func (g *Guard) PreCheck(raw string) error {
|
||||
_, literal, err := ParseURL(raw)
|
||||
if err != nil && !(literal.IsValid() && g.allows(literal)) {
|
||||
return err
|
||||
}
|
||||
if literal.IsValid() {
|
||||
return g.CheckAddr(literal)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
```
|
||||
|
||||
## What already exists
|
||||
|
||||
```go
|
||||
// internal/batch (task 02)
|
||||
type Options struct{ Mode, Question, Pages, Grep, JQ string; Context *bool; Recover bool }
|
||||
type Target struct{ URL string; Options Options }
|
||||
type Result struct{ Content string; Meta map[string]any }
|
||||
type TargetError struct{ Status int; Msg, Hint string }
|
||||
type FetchFunc func(ctx context.Context, c *auth.Caller, t Target) (Result, error)
|
||||
type Job struct{ ID, Caller, Status, Error string; TargetsTotal, TargetsDone int; CreatedAt int64 }
|
||||
type TargetStatus struct{ Idx int; URL, Status string; Result *Result; Error, Hint string }
|
||||
type Hooks struct {
|
||||
Job func(outcome string)
|
||||
Target func(err error, d time.Duration, resultBytes int)
|
||||
JobSeconds func(d time.Duration)
|
||||
Depth func(live int)
|
||||
Inflight func(n int)
|
||||
}
|
||||
type Config struct{ TTL time.Duration; TargetsPerJob, JobsPerCaller, JobsTotal, Workers int; MaxBytes int64; RetryWait, RetryFor time.Duration; Hooks Hooks }
|
||||
var ErrNoTargets, ErrTooManyTargets, ErrCallerBusy, ErrBusy, ErrRetryLater error
|
||||
func New(db *dcache.DB, fetch FetchFunc, cfg Config, log *slog.Logger) *Runner
|
||||
func (r *Runner) Submit(ctx context.Context, c *auth.Caller, callerClass string, targets []Target) (string, bool, error)
|
||||
func (r *Runner) Cancel(id string) bool
|
||||
func (r *Runner) Status(id string) (Job, []TargetStatus, bool)
|
||||
func (r *Runner) Touch(id string)
|
||||
func (r *Runner) Close()
|
||||
|
||||
// internal/metrics (task 03), on s.Prom
|
||||
ObserveBatchJob(outcome string); ObserveBatchTarget(outcome string)
|
||||
SetBatchQueueDepth(n int); SetBatchTargetsInflight(n int)
|
||||
ObserveBatchJobSeconds(d time.Duration); ObserveBatchTargetSeconds(d time.Duration); ObserveBatchResultBytes(n int)
|
||||
|
||||
// internal/api (task 04 and before)
|
||||
func (s *Server) fetchTarget(ctx context.Context, c *auth.Caller, t batch.Target) (batch.Result, error)
|
||||
func (s *Server) caller(w http.ResponseWriter, r *http.Request) *auth.Caller // nil after writing 401/403
|
||||
func (s *Server) callerClass(c *auth.Caller) string
|
||||
func (s *Server) logErr(c *auth.Caller, err error) any
|
||||
// requestDeadline (210 s), maxReqBody (64 KiB), writeErr, writeJSON
|
||||
```
|
||||
|
||||
## Interfaces added here
|
||||
|
||||
```go
|
||||
// in Server (after pdfPoolOnce):
|
||||
Batch *batch.Runner // nil: the /v1/batch endpoints (task 06) answer 503 "batch disabled"
|
||||
|
||||
// NewBatchRunner builds the runner that serve (task 07) and the tests use: its FetchFunc is
|
||||
// batchFetch and its Hooks feed s.Prom. cfg.Hooks is overwritten.
|
||||
func (s *Server) NewBatchRunner(db *dcache.DB, cfg batch.Config) *batch.Runner
|
||||
|
||||
func (s *Server) batchFetch(ctx context.Context, c *auth.Caller, t batch.Target) (batch.Result, error)
|
||||
func batchOutcome(err error) string
|
||||
```
|
||||
|
||||
## Rules
|
||||
|
||||
1. **`batchFetch`**, the runner's `FetchFunc`:
|
||||
- `context.WithTimeout(ctx, requestDeadline)`, mark the context as a batch fetch (an unexported
|
||||
context key), and call `s.fetchTarget`.
|
||||
- Map its error:
|
||||
- a `*fetch.Error` with status `429` or `503` → `fmt.Errorf("%s: %w", fe.Msg, batch.ErrRetryLater)`,
|
||||
so the runner retries it;
|
||||
- any other `*fetch.Error` → `&batch.TargetError{Status, Msg, Hint}`, with `502` rewritten to
|
||||
`500 internal error` as `writeFetchErr` does, and `s.blockN.Add(1)` for a `403`;
|
||||
- anything else → `&batch.TargetError{Status: 500, Msg: "internal error"}`, logged `Error`
|
||||
with `s.logErr(c, err)`.
|
||||
2. **Hooks** (in `NewBatchRunner`):
|
||||
- `Job` → `ObserveBatchJob`;
|
||||
- `Target(err, d, n)` → `ObserveBatchTarget(batchOutcome(err))`, `ObserveBatchTargetSeconds(d)`, and
|
||||
`ObserveBatchResultBytes(n)` when `err == nil`;
|
||||
- `JobSeconds`, `Depth`, `Inflight` → the matching `s.Prom` methods.
|
||||
- `batchOutcome(err)` is:
|
||||
- `ok` for nil;
|
||||
- `busy` for an error wrapping `ErrRetryLater` (the runner gave up retrying);
|
||||
- otherwise, by `TargetError.Status`: `403` `blocked`, `429` `busy`, `504` `timeout`,
|
||||
`415` `unsupported`, `422` whose message starts `document too long` `too_long`, other
|
||||
4xx `upstream_4xx`, 5xx `upstream_5xx`; any other error `upstream_5xx`.
|
||||
3. **The PDF pool under a batch:** in `extractPDF`, the pool's caller argument is `c.Name+"/batch"`
|
||||
when the context carries the batch mark, else `c.Name` as today. A batch extraction then
|
||||
never takes the caller's own interactive slot. The single-flight key is unchanged, so a batch
|
||||
and an interactive extraction of the same PDF still share one child.
|
||||
4. Log lines added here carry the caller and the job id, **never a target URL or hostname**. The
|
||||
per-target `fetch` line inside `fetchTarget` is unchanged.
|
||||
|
||||
## Facts
|
||||
|
||||
- The given api test builds the server through `s.NewBatchRunner` over an in-memory `dcache`
|
||||
(`newBatchServer`). Targets are local `httptest` servers, or `internal.test`, which the test's
|
||||
`LookupIP` resolves to `10.0.0.5`.
|
||||
- `TestBatchFetchErrors` fills the 8-slot global gate with `s.acquire` so the next fetch is
|
||||
refused busy after `acquire`'s 5 s wait.
|
||||
- `batchrun_test.go` also holds the helpers task 06's test uses; it is a given file like the others.
|
||||
|
||||
## Steps
|
||||
|
||||
- [ ] **1.** `git switch p6`; copy the two given tests. **2. See them fail.**
|
||||
- [ ] **3.** `PreCheck` (verbatim). **4.** `batch.go`. **5.** The `Batch` field and the pool name.
|
||||
- [ ] **6.** `gofmt -w`; `go test -race -count=3 ./internal/api/ ./internal/guard/` → `ok`.
|
||||
**7.** `make gate`.
|
||||
- [ ] **8.** Row `p6/05-batch-wiring`; commit.
|
||||
|
||||
```sh
|
||||
git add internal/api internal/guard/guard.go internal/guard/precheck_test.go docs/implementer-log.md
|
||||
git commit -F .state/commit-msg.txt
|
||||
```
|
||||
|
||||
## Done when
|
||||
|
||||
- The given tests and every existing api and guard test pass under `-race -count=3`; `make gate`
|
||||
ok; the given files are byte-identical.
|
||||
Reference in New Issue
Block a user