Skip to content

Cache hits ignore reference_week_day (and refresh_cache bypasses parse-mode guards): cached tibbles served across incompatible fetch settings #388

Description

@docxology

Summary

Cached entries store post-parse tibbles but are keyed by the request URL alone. Parse-relevant fetch_args are therefore invisible to the cache: two calls differing only in reference_week_day (which anchors how epiweeks convert to dates) share one cache entry, so the second call silently receives the first call's dates. Worse, the refresh_cache = TRUE OR-arm skips check_is_cachable()'s parse-mode guards entirely, so refresh_cache = TRUE, disable_date_parsing = TRUE writes a string-dated tibble under the exact key the default parse mode later reads back. I believe the fix is to include reference_week_day (and the parse flags) in the hash input — or to add them to check_is_cachable()'s disabling conditions — and to gate the refresh write on the same predicates.

Evidence (dev tip 87b10e1)

  • Hash input is URL-only — R/epidatacall.R:327-333:
    should_write_cache <- is_cachable ||
      (fetch_args$refresh_cache && is_cache_enabled())     # :327-328
    
    if (should_write_cache) {
      target <- request_url(epidata_call, "json", fetch_args$fields)
      hashed <- openssl::md5(target)                       # :330-333 — URL only
    }
  • check_is_cachable() (R/cache.R:319-342) guards fields, dry_run, base_url, disable_date_parsing, disable_data_frame_parsing, and refresh_cache — but not reference_week_day (nor return_empty).
  • The refresh_cache OR-arm above bypasses all of those guards on the write path.

Maintainer-runnable repro (verified code path)

library(epidatr)
set_cache(cache_dir = tempdir(), confirm = FALSE)

call <- pub_covidcast(
  source = "jhu-csse", signals = "confirmed_7dav_incidence_prop",
  geo_type = "state", time_type = "day", geo_values = "ca",
  time_values = epirange(20200601, 20200801),
  as_of = "2022-01-01"          # pins as_of so the call is cacheable
)

a <- fetch(call, fetch_args = fetch_args_list(reference_week_day = 1))
b <- fetch(call, fetch_args = fetch_args_list(reference_week_day = 7))
identical(a, b)   # TRUE — the second call returns the first call's dates
                  # (epiweek columns shifted up to 6 days), silently

Poison variant (writes a string-dated tibble under the default-mode key):

fetch(call, fetch_args = fetch_args_list(disable_date_parsing = TRUE, refresh_cache = TRUE))
# next default-mode fetch() serves the string-dated tibble from cache

Impact

On epiweek-typed endpoints (pub_fluview, pub_kcdc_ili, pub_fluview_clinical, …), cached values can be served with wrong column types/dates and no signal — the affected fetch_args are silently ignored on cache hits.

Suggested fix

  1. Append reference_week_day (and a parse-flags tuple: disable_date_parsing, disable_data_frame_parsing) to the hash input, e.g. hash paste(request_url(...), reference_week_day, disable_date_parsing, disable_data_frame_parsing); or add them to check_is_cachable()'s disabling conditions so differing settings are never served from cache.
  2. Gate the refresh write on the same predicates so refresh_cache cannot write entries the read path would serve under different parse modes.

Related smaller items in the same area (happy to split into separate issues)

  • refresh_cache = TRUE also persists entries that the read path can never serve for non-cachable calls (the OR-arm writes whenever the cache is enabled, even when fields makes the call non-cachable) — dead entries consuming max_size.
  • The key also omits the package version/schema tag, so a cached parsed shape survives an epidatr upgrade within max_age (default 1 day; the roxygen example suggests 14) — worth mixing packageVersion("epidatr") into the hash when the key is being reworked anyway.
  • reference_week_day itself accepts out-of-range values (0, 9, 2.5, Inf) because it is only guarded by assert_numeric (R/epidatacall.R:251-256) before reaching MMWRweek::MMWRweek2Date(MMWRday = ...) — assert_integerish(..., lower = 1, upper = 7) would catch typos client-side.

I checked for prior reports (open and closed issues/PRs about cache correctness, reference_week_day, or refresh_cache — including #303 "Review automatic weekly data type conversion", which is about conversion semantics rather than the cache key) and found none covering this.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions