Task 8 Review — Fan-out job groups (children + max_parallel + merge)

Commit reviewed: 438cdba (branch feat/gateway-p2-jobs-ocr, on top of f767251)
Verified independently: go vet ./... clean; go test -race -count=1 ./internal/jobs/ ./internal/server/ green.

Spec Compliance — ⚠️

The observable behavior matches the Task 8 deliverable (parent/child records, StatusHeld, max_parallel
gating via a coordinator, collect-all merge, 1 job + N job_child audit events, GET /jobs/{parent} and
GET /jobs/{childId} both work) and the concurrency-cap test genuinely gates (4 pool slots, max_parallel=2,
asserts peak concurrent Run ≤ 2). However, two gaps keep this from a clean ✅:

A real correctness bug in fan-out group creation atomicity (finding 1 below) that the "idempotency" bullet
of the plan implicitly promises against and does not hold.
The Redis store — the actual HA/production backend, and the one this task explicitly calls out as the
concurrency risk — has zero new test coverage for any of the three methods this task added to it.

Strengths

The max_parallel cap genuinely cannot be exceeded, including across concurrent/redundant coordinators —
I traced this by hand through several adversarial interleavings (two coordinators ticking simultaneously on
the same parent, one coordinator's snapshot going stale mid-scan while another releases/completes children
concurrently) and could not construct an overshoot. The mechanism relies on three properties working
together: (a) ListChildren/ListActiveParents sort children in a fixed, deterministic order (CreatedAt,
then ID) so any two independent readers pick the same "next N held children" for a given budget; (b) job
status only ever moves forward (held→queued→processing→terminal) and every Redis read is a live round-trip
(no caching), so a stale composite read can only overcount in-flight children (conservative — causes
under-utilization, never overshoot), never undercount in a way that authorizes too many releases; (c)
releaseChildScript is a single atomic, idempotent Lua CAS (EXISTS guard + status=='held' check), so two
replicas racing to release the same child collapse into exactly one real transition. This is a genuinely
well-designed property — worth documenting explicitly in the source, since it's non-obvious and the next
person to touch coordinate() could easily break it without realizing.
FinalizeParent (both stores) is correctly once-only: Redis via a CAS Lua script (EXISTS guard + terminal
check + SREM from the parents index in one atomic step), Local via a mutex + terminal check. The coordinator
only calls emitParent when won==true, so exactly one job audit event fires per group regardless of how
many replicas' coordinators race to finalize it — confirmed by trace, not just by reading the comment.
StatusHeld children are excluded from ClaimNext by construction, not by a bolted-on filter: RedisStore.Create
simply never LPUSHes a held child (or a parent) onto jobs:ready:{class}, and LocalStore.ClaimNext filters
on both Kind==KindParent and Status!=Queued. Both stores agree.
Parent-status derivation (deriveParentStatus) correctly handles all-fail, mixed, and single-child cases via
a single terminal/failed counter pass; collect-all is real (TestFanoutOneChildFailsCollectAll proves no
sibling cancellation and that all 4 results are retained).
Slot-release/panic-recover/on-prem enforcement are provably unaffected: manager.go's diff against f767251
touches only ManagerConfig (new field), Start() (spawns coordinateLoop), and audit-event field
population (JobID/ParentJobID) — the entire process() slot-admission/release/panic-recover/on-prem body
is byte-identical, so a released fan-out child inherits every P0/P1 guarantee for free, no re-implementation
to audit.
parseFanout's footgun (a single-job JSON body that happens to contain a top-level "fanout" key would be
misread as a group) is real but the report's own "not a concern for OCR/document payloads" assessment is
reasonable given this gateway's actual payload shapes (multipart documents or opaque JSON without that key) —
I agree with the severity call, but flag it as a latent landmine for any future adapter whose payload schema
legitimately uses fanout (see Issues, Minor).

Issues

Important — fan-out group creation is not atomic against store failures, and idempotent resubmit cannot
recover from a partial failure (internal/jobs/api.go:247-277, submitFanout)

existing, err := a.Store.Create(r.Context(), parent, nil)
...
if !existing {
    for i, p := range prep {
        child := &Job{... Status: StatusHeld}
        if _, cerr := a.Store.Create(r.Context(), child, p.payload); cerr != nil {
            writeErr(w, http.StatusServiceUnavailable, "STORE_UNAVAILABLE", "could not enqueue child job")
            return   // <-- parent already exists; children[i:] never created
        }
    }
}

The report's atomicity claim ("an invalid group fails atomically — no orphan parent") is true only for
validation failures (model resolution, base64 decode, on-prem), which are all checked before any Create
call. It does not hold for a transient store error mid-loop: if child i's Create fails (Redis timeout,
connection blip — realistic for the ~200-doc batches the design doc itself cites), the parent is left with only
i of N children, permanently. The client's only prescribed recovery (retry with the same Idempotency-Key,
per CONVENTIONS §3/idempotency) makes it worse, not better: Store.Create(parent, ...) returns
existing=true on the retry, so the for i, p := range prep loop is skipped entirely — no error, no
completion of the missing children, just a 202 with a children count in the response that no longer matches
reality. The group silently under-delivers (e.g., a 200-doc OCR batch that quietly only processes 140 documents,
with no error surfaced to the caller) and there is no way to fix it short of a fresh Idempotency-Key (a new group).

Concrete fix: don't gate the children-creation loop on !existing. Since each child's own Create is
independently idempotent (childIdemKey(parentKey, i)), it's safe to run the loop unconditionally on every
submit (including resubmits) — a child that already exists is a no-op dedup, and a genuinely-missing child from
a prior partial failure gets backfilled. (Minor extra cost: a fully-successful resubmit does N redundant
idempotent Create calls instead of 0 — cheap compared to silently losing data.)

Important — the actual concurrency-critical code path (RedisStore's three new Store methods) has zero test
coverage (internal/jobs/redisstore.go, internal/jobs/redisstore_test.go)

git diff f767251..438cdba -- internal/jobs/redisstore_test.go is empty — this task added ~116 lines to
redisstore.go (three new Lua scripts: releaseChildScript, finalizeParentScript, plus ListActiveParents,
and changes to Create/ClaimNext for held-child/parent handling) but did not add a single test exercising any
of it against RedisStore. Every fan-out test in fanout_test.go runs exclusively against LocalStore
(fanoutFixture calls NewLocal()). Given the task's own framing ("concurrency correctness is the core risk"
and Redis is the HA/production backend), this is the largest gap in the review: the "atomic Lua" claims for
ReleaseChild/FinalizeParent are correct by inspection (I traced them by hand above and by reading the Lua),
but that inspection is exactly the kind of reasoning a test should exist to catch regressions in — e.g. a future
refactor that changes finalizeParentScript's CAS condition, or a change to ListActiveParents's pruning, would
sail through go test ./... with this task's test suite. Also worth noting: the report's "Redis-gated ones
which ran against a reachable Redis" is a slight overstatement — newRedisStore in redisstore_test.go always
spins up an in-process miniredis unconditionally (not gated on a real, reachable Redis per Task 3's own stated
intent of mirroring redispool_test.go's skip-if-absent pattern), so there is no real-redis-server Lua
verification anywhere in this suite, fan-out or otherwise.

Concrete fix: add at minimum (a) a single-store correctness test for ListActiveParents/ReleaseChild/
FinalizeParent against RedisStore (mirrors the existing Local-only fan-out tests), and (b) a genuine
multi-replica test: two Managers (two coordinateLoops) sharing one RedisStore/miniredis, racing to
release/finalize the same parent's children, asserting peak in-flight ≤ max_parallel and exactly one job
audit event. That test is the one that would actually validate the "safe across replicas" claim this report and
I both arrived at only via manual proof.

Minor — ReleaseChild's atomicity signal is discarded, both in the Lua wrapper and the coordinator
(internal/jobs/redisstore.go:451-458, internal/jobs/coordinator.go:62-68)

func (s *RedisStore) ReleaseChild(ctx context.Context, id string) error {
    ...
    _, err := releaseChildScript.Run(...).Result()   // discards the 0/1 "did it actually flip" return
    ...
}

if err := m.store.ReleaseChild(m.ctx, children[i].ID); err == nil {
    toRelease--   // decrements even when ReleaseChild was a no-op (child already released by someone else)
}

Harmless today (per the Strengths analysis, redundant no-op releases self-correct because all coordinators
converge on the same deterministic target order), but it means the coordinator can never distinguish "I
released N children" from "I attempted N, several were already-done no-ops" — this makes the accounting
unverifiable by log/metric and would silently mask a future bug in the release-ordering invariant that Strengths
relies on. Suggest having the Lua scripts' int result propagate through ReleaseChild/FinalizeParent (Redis
already computes it) so the coordinator's bookkeeping — and any future test — can assert on real transition
counts, not just "no transport error."

Minor — max_parallel: 0 (and negative) silently means "uncapped," not "paused" (internal/jobs/api.go:227-230,
internal/jobs/coordinator.go:57-61)

maxPar := spec.MaxParallel
if maxPar < 0 {
    maxPar = 0
}
...
cap := parent.MaxParallel
toRelease := len(children) // effectively unbounded when cap <= 0
if cap > 0 {
    toRelease = cap - inflight
}

A caller sending max_parallel: 0 (a plausible mistake, or an attempt to mean "run serially"/"don't start yet")
gets full concurrency bounded only by the pool, not by their own cap. This is documented in a code comment but
not surfaced in the API response or validated/rejected, and no test pins down that this is deliberate rather
than an oversight (0 was never exercised in fanout_test.go). Worth at least a one-line note in the
POST /jobs API doc/README (Task 9) so operators don't get surprised.

Minor / style — shadowing the builtin cap (internal/jobs/coordinator.go:57): cap := parent.MaxParallel
shadows Go's builtin cap() in a file that's meant to get the most scrutiny in this codebase. Not a bug, just
worth a rename (e.g. capN/limit) for readability given how subtle the surrounding logic already is.

Assessment — Task quality: Needs fixes

The hard part — proving max_parallel can't be exceeded under concurrent/redundant coordinators — is actually
solved correctly, and that's the part I was most worried about walking in. But two things block approval as-is:

The fan-out creation path has a genuine, plan-relevant data-integrity bug (partial creation + idempotency
short-circuit that prevents recovery) that will silently under-deliver large batches under transient store
errors — exactly the kind of "collect-all, nothing silently lost" guarantee this task is supposed to provide.
The new Redis store methods — the ones actually carrying the concurrency risk in production (HA/coordination: redis) — ship with no tests at all, single-threaded or concurrent. The single-store fan-out test suite
(fanout_test.go) only ever runs against LocalStore.

Fix 1 (unconditional idempotent child-creation loop) and add the Redis-backed tests called out in Issue 2 before
merging; the coordinator's concurrency logic itself does not need to change.