Task 3 review — RedisStore (durable, HA-safe jobs.Store)

Base 9869f5e, Head bf9af49. Verified against source (internal/jobs/redisstore.go,
internal/jobs/redisstore_test.go), the plan's Task 3 spec + Global Constraints, and
internal/jobs/store.go (the interface + LocalStore it must match). Ran
go build ./..., go vet ./..., and go test -race -count=3 -v ./internal/jobs/...
myself — all green, all 11 Redis tests + 6 LocalStore tests pass 3× under -race, no
skips.

Spec Compliance — ⚠️

Implements every Store method with the specified key layout (job:{id} hash,
job:{id}:payload, jobs:ready:{class}, jobs:processing, idem:{tenant}:{key},
job:{parent}:children), SETNX dedup, RPOPLPUSH claim, reclaim-and-requeue, TTLs.
Builds/vets/tests clean, race-clean 3×. One real gap found by hand-tracing (see
Issues): an unclaimed queued job that outlives jobTTL can be claimed into a
corrupted, blank record rather than erroring — a TTL/list-lifetime mismatch the
EXISTS-guard pattern used elsewhere in this same file should have covered. Not a
duplicate-run bug (the core at-most-once invariant holds), but it's exactly the kind
of durability edge case this task exists to close, so it keeps this from a clean ✅.

Strengths

• Dedup atomicity is correctly reasoned and correctly implemented. Traced the
  full race: writeRecord (hash+payload) always completes before the same
  goroutine attempts SETNX, so by the time any SETNX succeeds, the winner's
  record is guaranteed fully written — a loser's Get on the winner's id can never
  observe a half-written job. The loser's own orphan hash is deleted (redisstore.go:184)
  and, critically, the loser returns (redisstore.go:186) before reaching the
  LPush enqueue step (redisstore.go:201) — so an orphan is never pushed onto a
  ready list and is structurally unclaimable, even in the narrow window before the
  Del runs. TestRedisCreateDedupAtomic (16 concurrent creates, race-checked)
  confirms exactly one job is created and exactly one is claimable.
• Claim-once is a genuine single EVAL. claimScript (redisstore.go:101-108) does
  RPOPLPUSH + the processing-stamp + HINCRBY atomically in one round trip; Redis's
  single-threaded script execution makes two concurrent claimers structurally unable
  to receive the same id. TestRedisClaimOnce (8 claimers, 20 jobs, race-checked)
  confirms exactly 20 claims total, each id exactly once.
• ReclaimStale idempotency is correctly guarded. The LREM ... > 0 check
  (redisstore.go:125) means a second scan (whether from the same replica or a
  concurrent one — Lua scripts serialize on Redis) can't re-requeue an id already
  moved; the boundary (heartbeat < cutoff, strict) matches LocalStore's
  Heartbeat.Before(cutoff) exactly.
• Restart/HA durability is actually tested, not just asserted: TestRedisRestartDurability
  builds a second RedisStore against the same miniredis instance and confirms
  record, payload, status, timestamps, and attempts all survive.
• UpdateStatus WATCH/MULTI loop is correct and bounded (updateStatusMaxRetries = 50,
  redisstore.go:65/288-298) — no infinite spin; on exhaustion it returns an explicit
  error rather than silently dropping the update.
• Heartbeat/SetResult correctly guard against resurrecting expired jobs via an
  EXISTS check before the HSET (redisstore.go:139-151) — this is exactly the right
  pattern; see Issues for where it's missing.
• Good behavior parity with LocalStore: same dedup-returns-full-existing-record
  semantics, same terminal-state shape, same ClaimNext(ctx, class) signature with
  the interface doc (store.go:30-34) explicitly noting the class arg is advisory
  for Redis and that convergence with LocalStore's priority ordering is a Task 7
  obligation (manager loops classes interactive→system→batch) — this is stated
  correctly in both the interface comment and the implementation comment, not just
  asserted in the report.
• Test suite deviates from the brief's literal "skip if absent" wording but matches
  the actual, more-recent precedent (internal/pool/redispool_test.go already uses
  miniredis.RunT(t) unconditionally, no gate/skip) — this is the right call, not a
  deviation worth flagging.

Issues

Important — redisstore.go:101-108 (claimScript), :116-135 (reclaimScript): missing EXISTS guard lets a claim resurrect a TTL-expired job as a corrupted record.
job:{id} and job:{id}:payload get an absolute jobTTL (24h) set once, at
creation (redisstore.go:219,:224) — nothing refreshes it on claim, heartbeat, or
status update. But jobs:ready:{class} list entries have no TTL at all. If a
job sits unclaimed past jobTTL (batch-class starvation, a manager outage, etc. —
plausible over the lifetime of a durable HA queue, which is the entire point of this
task), its hash and payload expire while its id is still sitting in the ready list.
When it's eventually popped by claimScript, RPOPLPUSH succeeds (line 102) and the
script proceeds straight to HSET k 'status' 'processing' ... (line 105) — Redis
HSET auto-vivifies a hash that doesn't exist, silently creating a new one with
only status/started_at/heartbeat/updated_at/attempts and none of the real fields
(tenant, model, doc_hash, etc. are all ""). ClaimNext's subsequent s.Get
(line 250) then succeeds (HGetAll is non-empty) instead of returning ErrNotFound,
so the manager receives a claimed, "processing" job with blank tenant/model/payload.
Worse: Payload() (redisstore.go:339-355) checks Exists(jobKey(id)) — which is now
true, since HSET just recreated it — then does Get(payloadKey(id)), which returns
redis.Nil because the payload key really is gone, and that's mapped to
return []byte{}, nil (not an error). So the manager could dispatch an OCR call
with an empty payload for a job with no identifying fields, rather than failing
loudly. The exact same asymmetry exists in reclaimScript's HSET at line 128 (lower
risk there, since a false HGET status on a vanished hash just fails the
== 'processing' check and the id is skipped/leaked in jobs:processing, not
corrupted). This is the same class of hazard the author explicitly guarded against in
heartbeatScript/setResultScript (comment at line 43-44: "a plain HSET would
otherwise create it") — the guard just wasn't extended to claimScript/reclaimScript.
Not a duplicate-run bug (at-most-once claim still holds structurally); it's a
silent-data-corruption / silent-empty-payload bug under a real (if currently
low-probability, given the 24h TTL vs. the 30-min GPU_QUEUE_WAIT_MS) durability
edge case. Recommend: add an EXISTS check in claimScript before the HSET
stamp — on miss, LREM the id back out of jobs:processing (it's already
unrecoverable) and return false so ClaimNext treats it as "try again," rather
than handing back a corrupted job.

Minor — redisstore.go:184, :209: two fire-and-forget Redis calls with unchecked errors.
s.rdb.Del(ctx, jobKey(job.ID), payloadKey(job.ID)) (orphan cleanup on dedup loss)
and s.rdb.Expire(ctx, ck, s.jobTTL) (children-set TTL) both ignore their .Err().
Neither is a correctness issue today (the orphan is unclaimable regardless of whether
Del succeeds, since it's never enqueued; a missing children-set TTL just means it
lives to jobTTL off the last SAdd instead of refreshing) — worth a //nolint-style
comment or a debug log so a real failure isn't completely invisible, but not
blocking.

Minor — no active reaper for jobs stuck past GPU_QUEUE_WAIT_MS while still queued.
Already called out in the report as a known P2 gap ("no Delete/GC of terminal jobs
beyond TTL"). Combined with the Important issue above, this means the only thing
currently preventing the corrupted-claim scenario is "in practice nothing sits
unclaimed for 24h." Worth a one-line note in the Task 7 handoff, not a Task 3 blocker.

Cross-task note (ClaimNext error handling / class-scoping for Task 7)

• ClaimNext swallowing all Redis errors as "empty" (redisstore.go:245-249) is real
  and matches the report's own caveat. I traced it: claimScript.Run(...).Text()
  returns the same (nil, false) whether the ready list is genuinely empty or Redis
  is unreachable — there is no way for a caller to distinguish "no work" from
  "infrastructure is down" from ClaimNext's return value alone. Note this is
  qualitatively different from redispool.go's fail-open pattern (which the report
  cites as precedent): redispool's fail-open still admits work via a local
  fallback pool when Redis errors; RedisStore.ClaimNext's "swallow as empty" means
  no work is claimed at all — for an HA pair, a real Redis outage would silently
  stall every job across every replica, with nothing in the job-store layer
  distinguishing that from a quiet queue. Given CONVENTIONS' explicit requirement
  that queue state be user-visible ("never a bare spinner") and that 429/503 are
  graceful degradation rather than silent drops, a silent stall at the store layer
  is the failure mode CONVENTIONS is trying to avoid, just one layer down.
• My call: don't change the Store interface for this alone (it would touch
  LocalStore too, for no behavioral benefit there — LocalStore has no analogous
  failure mode). Instead, make it an explicit Task 7 obligation: (a) the manager
  must not treat "N consecutive empty claims across all classes" the same as "queue
  is empty, back off normally" without also tracking whether those empties are
  correlated with actual Redis errors; (b) add an independent Redis health signal
  (e.g., a periodic PING alongside the pool's own health tracking, or reuse
  whatever redispool.go already surfaces for its fail-open decision) so
  operators/alerts can distinguish "no work" from "store unreachable" — this should
  not rely on ClaimNext's return value. This should be written into the Task 7
  section of the plan (or CONVENTIONS if it's meant to bind future stores too), not
  left as an implicit assumption.
• Class-scoping: confirmed correct and honestly documented — ClaimNext is
  genuinely per-class (one readyKey(class)), and both the Store interface doc
  (store.go:30-34) and the implementation comment state plainly that priority
  ordering across classes only holds once Task 7's manager loops
  interactive→system→batch per call. No parity gap today; just flagging that Task 7
  must actually implement that loop order for the "interactive beats batch" invariant
  to hold under Redis coordination — LocalStore's in-process priority scan doesn't
  need this, so it's easy to forget when wiring the Redis path.

Assessment — Task quality: Needs fixes

The concurrency/atomicity primitives that were the explicit point of this task —
dedup-once, claim-once, idempotent reclaim, WATCH/MULTI optimistic updates, restart
durability — are all correctly reasoned, correctly implemented, and genuinely
exercised by race-checked concurrent tests (verified independently, not just taking
the report's word for it). That's the hard part and it's solid.

What holds this back from Approved is the claimScript/reclaimScript missing the
same EXISTS guard the author correctly applied to heartbeatScript/setResultScript
— a real, traceable path to a corrupted "processing" job with a silently-empty
payload, in a file whose entire mandate is durability under exactly this kind of
edge case (jobs that sit around for a while under HA/outage conditions). It's a
small, mechanical fix (one more EXISTS check + a cleanup branch in claimScript,
mirroring the pattern already in the same file), not a redesign. Recommend fixing
that before merge, and carrying the ClaimNext-error-handling note into Task 7 as an
explicit obligation rather than an implicit assumption.