fix(http): raise the CFBD retry budget so a throttle stops looking like no data (#148)

cfbfastR_cfb_pbp published nothing from 2026-07-01 onward and has no
play_by_play_2026 asset, while the cfbfastR-data job that builds it reported
success every run. In one run cfbd_play_stats_player() hit HTTP 429 sixty-three
times, so sack_player_id never materialised and the roster join in week.R:349
aborted the script 628 lines before the release upload.

httr2 was already doing the right things -- retry_is_transient() retries 429/503
by default and retry_after() honours a Retry-After header, falling back to our
backoff. Only the budget was wrong: 3 tries and roughly 1-9s of backoff covers
an incidental 429, not the sustained throttling a parallel caller produces.

When the budget ran out, req_error(is_error = ~FALSE) handed the still-429
response back and check_status() raised "The CFBD API returned HTTP 429", which
every cfbd_*() tryCatch reports as "... or no data available" -- a throttle
became indistinguishable from an empty result.

max_tries 3 -> 6 with max_seconds = 120. The cap matters: without it a hard-down
API hangs a caller for the full tries * backoff product. All 28 callers of
get_req() inherit this.

Also splits build_req() out of get_req() so the policy is assertable without
network. Signature unchanged, so tests/testthat/setup-cfbd-throttle.R still
works -- which is also why the test targets build_req() and why nothing asserts
the delegation: the throttle hot-swaps get_req() in the namespace for the whole
session.

build_req() is title-free in roxygen so no .Rd is generated for an internal
helper. No other generated file is touched: the committed docs predate the local
roxygen and regenerating rewrites ~150 unrelated man/*.Rd plus a hand-formatted
NAMESPACE.

Verified: 6/6 assertions pass offline, the same assertions fail against the old
3-try/no-max-wait policy, and R CMD check passes on ubuntu release, ubuntu
oldrel-1 and windows.

Follow-up: CLAUDE.md line 133 still says wrappers "don't hot-retry"; that is now
stale and needs updating.

Author: saiemgilani

R/utils.R

@@ -247,9 +247,14 @@ rbindlist_with_attrs <- function(dflist){
 }
 
 # Request Functions ----
+# build_req() is split out from get_req() so the retry/timeout policy can be
+# asserted in a test without performing a request (see
+# tests/testthat/test-utils_request_policy.R). get_req() remains the only entry
+# point callers use. Deliberately kept title-free so roxygen generates no .Rd
+# for an internal helper.
 #' @keywords Internal
 #' @importFrom httr2 request req_headers req_timeout req_retry req_error req_perform req_proxy
-get_req <- function(full_url, proxy = NULL) {
+build_req <- function(full_url, proxy = NULL) {
   req <- httr2::request(full_url) |>
     httr2::req_headers(Authorization = paste("Bearer", cfbd_key())) |>
     httr2::req_timeout(60)
@@ -280,13 +285,37 @@ get_req <- function(full_url, proxy = NULL) {
     }
   }
 
+  # Retry budget. httr2 retries 429/503 by default (`retry_is_transient`) and
+  # honours a `Retry-After` header when the server sends one, falling back to
+  # `backoff` when it does not.
+  #
+  # The previous budget -- 3 tries, ~1-9s of backoff -- is enough for an
+  # incidental 429 but not for sustained throttling, which is what a parallel
+  # caller produces. When it ran out, `req_error(is_error = ~FALSE)` handed the
+  # still-429 response back and `check_status()` turned it into a plain error,
+  # which every cfbd_*() tryCatch reports as "no data available". A throttle
+  # then looks exactly like an empty result. That is how cfbfastR_cfb_pbp
+  # published nothing from 2026-07-01 onward while its job stayed green:
+  # `cfbd_play_stats_player()` 429'd 63 times in one run, `sack_player_id`
+  # never materialised, and the downstream join aborted the whole script.
+  #
+  # `max_seconds` bounds the total wait so a hard-down API still fails in
+  # bounded time rather than hanging a caller for max_tries * backoff.
+  #
+  # Note this is the production path only -- tests already slow themselves via
+  # the `get_req()` wrapper in tests/testthat/setup-cfbd-throttle.R.
   req |>
     httr2::req_retry(
-      max_tries = 3,
-      backoff   = function(i) stats::runif(1, 0.5, 1.5) * (2 ^ i)
+      max_tries   = 6,
+      max_seconds = 120,
+      backoff     = function(i) stats::runif(1, 0.5, 1.5) * (2 ^ i)
     ) |>
-    httr2::req_error(is_error = function(resp) FALSE) |>
-    httr2::req_perform()
+    httr2::req_error(is_error = function(resp) FALSE)
+}
+
+#' @keywords Internal
+get_req <- function(full_url, proxy = NULL) {
+  httr2::req_perform(build_req(full_url, proxy = proxy))
 }
 
 #' Drop NULL entries from a list (internal)

tests/testthat/test-utils_request_policy.R

@@ -0,0 +1,39 @@
+test_that("build_req() carries a retry budget that survives sustained CFBD throttling", {
+  # Offline by design: build_req() only constructs the request, and cfbd_key()
+  # returns NA rather than erroring when CFBD_API_KEY is unset, so this needs
+  # neither network nor credentials.
+  req <- cfbfastR:::build_req("https://api.collegefootballdata.com/games")
+
+  # httr2 already retries 429/503 (retry_is_transient) and honours a
+  # Retry-After header; what this package controls is the budget. The previous
+  # 3 tries / ~1-9s of backoff covered an incidental 429 but not the sustained
+  # throttling a parallel caller produces -- when it ran out, check_status()
+  # turned the surviving 429 into an error that every cfbd_*() tryCatch reports
+  # as "no data available", making a throttle indistinguishable from an empty
+  # result.
+  expect_gte(req$policies$retry_max_tries, 6L)
+  expect_gte(req$policies$retry_max_wait, 120)
+
+  # A backoff must still exist for servers that send no Retry-After header.
+  expect_type(req$policies$retry_backoff, "closure")
+  expect_true(is.finite(req$policies$retry_backoff(1)))
+})
+
+test_that("build_req() leaves HTTP status handling to check_status()", {
+  req <- cfbfastR:::build_req("https://api.collegefootballdata.com/games")
+
+  # req_error(is_error = ~FALSE) is deliberate: the response is handed back so
+  # check_status() can raise the "The CFBD API returned HTTP ..." message the
+  # cfbd_*() family reports. If this ever flips to TRUE, httr2 would abort
+  # first and that message would never be produced.
+  expect_type(req$policies$error_is_error, "closure")
+  expect_false(req$policies$error_is_error(
+    structure(list(status_code = 429L), class = "httr2_response")
+  ))
+})
+
+# No test asserts that get_req() delegates to build_req(): setup-cfbd-throttle.R
+# hot-swaps get_req() in the cfbfastR namespace for the whole test session, so
+# anything inspecting it here sees the throttle wrapper rather than the package
+# function. build_req() is not swapped, which is why the policy assertions above
+# target it directly.