Task 5 review — AzureDIAdapter (Azure DI async submit/poll adapter)

Commit reviewed: 57e973d (base 7465a56). Files: internal/jobs/adapter_azuredi.go,
internal/jobs/adapter_azuredi_test.go. Verified by reading source directly (not just the
diff) and running go build ./..., go vet ./..., go test -race ./internal/jobs/... —
all green; the 8 claimed tests pass under -race.

Spec Compliance — ✅

All Task 5 bullets are met: submit to ep+SubmitPath with {model} substitution, terminal
status handling (succeeded/failed), bounded poll honoring ctx, JobResult-compatible
return, server-side key injection, and the 5 required test scenarios (running×2→succeeded
with progress, failed, poll timeout — plus extra 429/503/missing-header/key-injection cases
beyond the minimum ask).

Strengths

• Exact Adapter interface conformance. Run signature matches internal/jobs/adapter.go
  verbatim; var _ Adapter = (*AzureDIAdapter)(nil) compile-time check present. AdapterError
  conventions (Retryable/Code/Status/RetryAfter) are used identically to
  SyncHTTPAdapter — a manager written against Task 4 drives this adapter with no special-casing.
• Protocol correctness. Operation-Location is read from the response header
  (resp.Header.Get("Operation-Location"), adapter_azuredi.go:182), not the body.
  succeeded/failed are terminal; running/notstarted/empty continue; anything else hits
  AZURE_DI_UNKNOWN_STATUS (line 247) — no infinite loop, no nil-deref (PercentCompleted is
  nil-checked before dereference at adapter_azuredi.go:253-254).
• ctx handling is correct and leak-free. ctx is checked before each attempt (line 212),
  after a failed Do() (line 224), and via select{<-ctx.Done(); <-timer.C} on the inter-poll
  wait (lines 282-287) with timer.Stop() on the cancel branch. Critically, the timer is only
  constructed at the bottom of the loop body, after all terminal/error returns for that
  iteration — so none of the early-return paths (succeeded, failed, unknown status, bad JSON,
  429/503, transport error) ever leave an unstopped timer. TestAzureDIAdapter_PollTimeout
  exercises this concurrently and returns well inside its 2s guard.
• Poll interval is bounded, no busy-spin risk. pollInterval() defaults to 1s and clamps
  to 5s max regardless of config (lines 80-89); there's no path where a 0/unset value produces
  a tight loop.
• Key injection is correct and isolated. injectKey() sets Ocp-Apim-Subscription-Key
  (not Authorization) from os.Getenv(Upstream.APIKeyEnv) on both submit (line 168) and
  poll (line 220); absent when APIKeyEnv is unset or the env var is empty; no client-supplied
  header is ever read or forwarded. Confirmed by TestAzureDIAdapter_APIKeyInjected and
  TestAzureDIAdapter_NoKeyWhenUnset (the latter also asserts Authorization is empty).
• Bounded reads + close on every path. readCapped wraps io.LimitReader(rc, maxBytes) +
  rc.Close() and is used for submit (line 178, drained/discarded before the status switch) and
  poll (line 231, before the status switch) — every response, on every status branch, gets its
  body capped and closed, matching the Task 4 fix.
• Tests are namespace-safe (t5-* tenants/ids), use per-test httptest.Server instances (no
  shared global state to race on), and assert on filtered/local state, not bare counts — meets
  the P0 test rule.

Issues

Minor:

1. Read errors are still silently discarded (readCapped, adapter_azuredi.go:137-140:
   body, _ := io.ReadAll(...)), repeating the Task-4 pattern the report says it addresses.
   Functionally this is safer here than in Task 4: a truncated/partial body almost always
   fails json.Unmarshal and maps to AZURE_DI_BAD_RESPONSE rather than a "fake success" (the
   Task-4 risk), and on submit the body is discarded anyway. However, there's a real (if small)
   downside: a transient network blip mid-read of a running/succeeded poll response now
   surfaces as non-retryable AZURE_DI_BAD_RESPONSE, permanently failing the job, when the
   more correct classification for a mid-poll transport hiccup would arguably be retryable (the
   loop is already built to keep polling). Not a correctness bug — no leak, no fake success —
   but worth a one-line fix (check the read error explicitly and return a retryable code, or at
   least log it) before this sees heavy live traffic.
2. PollPath (config.Upstream) is unused. The plan text says "poll PollPath/that URL";
   this adapter only ever polls the dynamic Operation-Location header value and never
   references Upstream.PollPath. That's defensible — DI's protocol returns a
   per-operation URL, so a static configured path doesn't apply the way it might for a
   different async protocol — but it's worth an explicit note in the plan/config docs that
   PollPath is presently a no-op for the azure-di adapter, so a future config author doesn't
   assume setting it changes behavior.
3. Default submitPath() for an unset SubmitPath is / (adapter_azuredi.go:106-108), whereas
   SyncHTTPAdapter defaults to /ocr. Inconsequential (config always sets SubmitPath for a
   real upstream) but a minor cross-adapter inconsistency if someone goes looking for a pattern.

No Critical or Important issues found.

Cross-task note — Operation-Location rewrite for Task 7/9 live-fire

Confirmed: the adapter polls the Operation-Location header value verbatim — no scheme/host/path
rewriting of any kind (opLoc flows straight from resp.Header.Get(...) into
http.NewRequestWithContext(ctx, http.MethodGet, opLoc, nil), adapter_azuredi.go:182-186 and
216). If the on-prem nginx-fronted DI container returns a mangled prefix, the failure mode is
clean rather than silent: the poll GET will most likely 404/5xx against the wrong path, which
maps deterministically to AZURE_DI_POLL_<status> (non-retryable for 404, retryable for
429/503) rather than hanging or looping forever — so this is not a correctness bug in Task 5,
and deferring is acceptable for this task.

Recommendation for Task 7/9: don't leave this to "deployment/config" as a vague TODO — before
the Task 9 live-fire against the real on-prem box, either (a) confirm empirically that the
on-prem instance's Operation-Location is well-formed end-to-end (no rewrite needed), or (b)
add a small, explicit normalization hook — e.g. when the Operation-Location host/scheme differs
from the resolved upstream endpoint's host/scheme, rewrite host+scheme to the endpoint's while
keeping DI's path+query verbatim (a bounded, safe default that doesn't require a new config
field). This should be a named checklist item in Task 9's live-fire step, not something
discovered mid-drill.

Assessment — Task quality: Approved

Correct, interface-conformant, leak-free implementation with thorough test coverage (8 tests,
race-clean, exceeding the plan's 3 required scenarios). The two Minor issues (read-error
classification, unused PollPath) are worth a follow-up note but do not block merging Task 5;
neither is a correctness or safety defect. The nginx Operation-Location concern is correctly
identified and safely deferred, but should be explicitly tracked into Task 9's live-fire plan
rather than left implicit.