Retry transient reservation-fetch failures; split lease denial from unreachable

[agt]

Two targeted resilience fixes in reservation_client.py, arising from an outside review of hand-rolled retry code in this repo.

Why not the reviewer's proposal

The review recommended replacing all backoff with tenacity/stamina plus httpx.AsyncHTTPTransport(retries=...). That is not adopted, for three reasons:

• The eleven next_attempt_at sites in main.py are not call-retry loops. They are requeue delays in a level-triggered reconciler. Eight of them follow a guard denial (main.py:700, :711, :725, :745, :755 — schedule verdict pending, stuck-holder interlock, class overcommitted, no single-node fit, unknown class id) where nothing raised and there is no call to re-drive. Retry libraries retry a callable that threw; that is not what these sites do.
• "No attempt cap" is a requirement, not a defect. A pod waiting for GPU capacity must be re-evaluated indefinitely. Capping attempts would permanently strand Pending pods whose capacity frees up later.
• httpx transport retries= only covers connection establishment — not read timeouts, not 5xx — so it could not have replaced the hand-rolled paths anyway.

Two of the review's four cited sites (controller_push.py, email_service.py) do not exist in this repo; they appear to describe the sibling reservation app. Of what remained, the two fixes below were genuine.

Fix 1 — retry each page of fetch_reservations

A network error or 5xx on page 3 of 5 aborted the entire refresh cycle, leaving the controller on stale reservation state for a full RESERVATION_FETCH_INTERVAL (300 s default).

_get_reservations_page now retries such a failure up to 3 times with a 1 s / 2 s backoff, resuming at the same offset so already-accumulated pages are not lost.

• A 4xx is not retried — a bad key or bad filter will not fix itself, and retrying only delays the cycle's failure.
• A failure outliving the retries still raises. That fail-safe is load-bearing: acting on a partial page set would wholesale-replace state.reservations with an under-count.

Fix 2 — distinguish lease failure modes

create_ondemand_reservation collapsed a 409 denial and an unreachable app into a bare None, so _grant_and_admit gave both the same 2–5 min cooldown.

It now returns a LeaseResult carrying a LeaseOutcome:

outcome	cause	caller cooldown
GRANTED	lease issued	—
DENIED	4xx, typically 409 — the app answered "no"	2–5 min jittered
UNAVAILABLE	network error or 5xx — the app failed, it did not decide	30 s short retry
MALFORMED	2xx whose body would not parse (likely deterministic)	2–5 min jittered

Re-asking on UNAVAILABLE is safe because the request is idempotent on the pod UID — a lease created behind a lost response is handed back on the retry rather than duplicated.

Scoping note, stated plainly: the cooldown split is bounded by the queue-processor tick. next_attempt_at gates eligibility only, so at the default QUEUE_PROCESSOR_INTERVAL (300 s) a 30 s and a 2–5 min cooldown often both resolve to "next tick". The distinction bites when the interval is tuned down, and on ADDED-triggered admission batches that run between ticks. This is a real but modest improvement, not a large one.

Testing

962 pass (baseline 951; 11 added). Both fixes were mutation-tested to confirm the new tests fail without them:

• setting _FETCH_MAX_ATTEMPTS = 1 fails all 4 retry tests
• forcing the caller back to the jittered cooldown fails the transient-retry test

New coverage: retry on transient error and on 5xx, retry preserving earlier pages mid-pagination (asserts offsets ["0", "200", "200"] — the retry resumes rather than restarting), no-retry on 4xx, raise after exhausting attempts, all four LeaseOutcome classifications, and the caller-side cooldown split.

Docs

The log-grammar build gate (tests/test_log_grammar.py) requires these:

• docs/LOG-FIELDS.md — new attempt field, new lease.denied reason enum. ⚠️ This file is duplicated verbatim in the reservation app; the sibling copy needs the same two edits.
• OBSERVABILITY.md — new api.reservations_fetch_retry event, updated lease.denied row.
• CLAUDE.md — new Transient-failure retry policy section recording why the other nine sites stay as they are, so this question does not get re-litigated from scratch.

No new dependencies, no new RBAC, no config changes.

---

Generated by Claude Code