Merge pull request #26 from NewGraphEnvironment/25-dft-stac-fetch-cache-key-omits-aoi-secon

Key STAC cache by AOI and fetch parameters (fixes silent cross-AOI collision)

Author: NewGraphEnvironment

DESCRIPTION

@@ -1,7 +1,7 @@
 Package: drift
 Title: Detecting Riparian and Inland Floodplain Transitions
-Version: 0.2.2
-Date: 2026-04-14
+Version: 0.2.3
+Date: 2026-07-06
 Authors@R: c(
     person("Allan", "Irvine", , "al@newgraphenvironment.com", role = c("aut", "cre"),
            comment = c(ORCID = "0000-0002-3495-2128")),

NEWS.md

@@ -1,3 +1,8 @@
+# drift 0.2.3
+
+- Fix silent cross-AOI cache collision in `dft_stac_fetch()` (#25). Cache files were keyed by source + year only, so fetching a second AOI with the same source/year silently returned the first AOI's raster masked to the second AOI's extent. Cache filenames now include a hash of the AOI geometry and all fetch-affecting parameters (`res`, `crs`, `dt`, `aggregation`, `resampling`, `stac_url`, `collection`, `asset`). Existing caches re-fetch on first use after upgrading; `dft_cache_clear()` reclaims the orphaned old-format files.
+- `force = TRUE` now overwrites the cached file instead of erroring with "File already exists" (#25).
+
 # drift 0.2.2
 
 - Startup quote pool expanded to 113. Adds 52 domain-expert quotes from 11 voices across floodplain/river process (David Montgomery, Ellen Wohl), Indigenous stewardship (Robin Wall Kimmerer, Kyle Whyte, Nancy Turner, Jeannette Armstrong), ecosystem valuation (Kai Chan), Canadian public voices (David Suzuki, Wade Davis), and legacy conservation (Aldo Leopold, Wendell Berry).

R/dft_stac_fetch.R

@@ -5,6 +5,13 @@
 #' collection hosting single-band classified rasters (IO LULC, ESA WorldCover,
 #' custom COGs).
 #'
+#' Fetched rasters are cached under [dft_cache_path()] as
+#' `<source>/<year>_<key>.nc`, where `key` is a hash of the AOI geometry and
+#' every fetch parameter that affects the output (`res`, `crs`, `dt`,
+#' `aggregation`, `resampling`, `stac_url`, `collection`, `asset`). Repeat
+#' calls with the same AOI and parameters reuse the cache; changing any of
+#' them re-fetches.
+#'
 #' @param aoi An `sf` polygon defining the area of interest.
 #' @param source Character. A known source name passed to [dft_stac_config()].
 #'   Ignored when `stac_url`, `collection`, and `asset` are all provided.
@@ -24,7 +31,10 @@
 #'   for categorical data).
 #' @param cache_dir Character. Cache directory path. When `NULL`, uses
 #'   [dft_cache_path()].
-#' @param force Logical. Re-fetch even if cached (default `FALSE`).
+#' @param force Logical. Re-fetch even if cached, overwriting the cached file
+#'   (default `FALSE`). A raster returned by an earlier call with the same
+#'   parameters is backed by that file and may silently pick up the rewritten
+#'   contents.
 #' @param sign_fn A signing function for STAC assets. Default is
 #'   [rstac::sign_planetary_computer()].
 #'
@@ -97,10 +107,14 @@ dft_stac_fetch <- function(aoi,
   source_label <- if (!is.null(source)) source else "custom"
   cache_source_dir <- file.path(cache_base, source_label)
   dir.create(cache_source_dir, recursive = TRUE, showWarnings = FALSE)
+  cache_key <- stac_cache_key(
+    aoi_target, res, target_crs, dt, aggregation, resampling,
+    stac_url, collection, asset
+  )
 
   # Fetch per year
   result <- lapply(years, function(yr) {
-    cache_file <- file.path(cache_source_dir, paste0(yr, ".nc"))
+    cache_file <- file.path(cache_source_dir, paste0(yr, "_", cache_key, ".nc"))
 
     if (!force && file.exists(cache_file)) {
       message("  ", yr, ": cached")
@@ -123,7 +137,7 @@ dft_stac_fetch <- function(aoi,
         resampling = resampling
       )
       cube <- gdalcubes::raster_cube(col, v)
-      gdalcubes::write_ncdf(cube, cache_file)
+      gdalcubes::write_ncdf(cube, cache_file, overwrite = TRUE)
       r <- terra::rast(cache_file)
     }
 
@@ -136,6 +150,29 @@ dft_stac_fetch <- function(aoi,
 }
 
 
+#' Cache key for one STAC fetch parameter set
+#'
+#' Hashes everything that changes the written raster except year, which stays
+#' as the readable filename prefix (all years of one call share a key). The
+#' geometry is hashed as WKB so sf attribute columns and PROJ-version CRS
+#' representation differences can't change the key; the CRS enters separately
+#' as `target_crs`. `res` is coerced to double so `10L` and `10` key alike.
+#' Callers must pass post-resolution `stac_url`/`collection`/`asset`, never
+#' the raw possibly-NULL arguments.
+#' @noRd
+stac_cache_key <- function(aoi_target, res, target_crs, dt, aggregation,
+                           resampling, stac_url, collection, asset) {
+  geom_wkb <- sf::st_as_binary(sf::st_geometry(aoi_target), endian = "little")
+  substr(
+    rlang::hash(list(
+      geom_wkb, as.numeric(res), target_crs, dt, aggregation,
+      resampling, stac_url, collection, asset
+    )),
+    1, 12
+  )
+}
+
+
 #' Auto-detect UTM EPSG code from sf geometry
 #' @noRd
 auto_utm_epsg <- function(x) {

man/dft_stac_fetch.Rd

@@ -0,0 +1,21 @@
+# Issue #25 — dft_stac_fetch cache key omits AOI
+
+## Outcome
+
+Fixed a silent wrong-data bug: `dft_stac_fetch()` cached NetCDFs as `<source>/<year>.nc`, so a
+second AOI with the same source/year silently received the first AOI's raster masked to its own
+extent (real occurrence: MORR got Neexdzii's rasters). Added internal `stac_cache_key()` —
+`rlang::hash()` over the AOI geometry as WKB plus every fetch-affecting parameter (`res` coerced
+to double, target CRS, `dt`, `aggregation`, `resampling`, post-resolution
+`stac_url`/`collection`/`asset`) — giving filenames `<year>_<key>.nc`; also made `force = TRUE`
+overwrite via `write_ncdf(..., overwrite = TRUE)` instead of erroring. Key learnings: hash sf
+geometry as WKB, not the sfc object (PROJ-version CRS WKT drift causes spurious misses, and sf
+attribute columns would leak into the key); coerce numerics before hashing (`10L` vs `10` hash
+differently under `rlang::hash()`); hash post-default-resolution values, never possibly-NULL
+args; and skip extent-containment checks on cache hits — gdalcubes only ever enlarges extents,
+so the check validates nothing. Verified end-to-end against live Planetary Computer STAC (two
+AOIs → two distinct cache files, correct extents, cache hit, force overwrite). Released as
+v0.2.3.
+
+Closed by: commits 9e2816a / 352aec9 / fc7c4e2 / b09c0fb, PR pending (branch
+`25-dft-stac-fetch-cache-key-omits-aoi-secon`)

planning/archive/2026-07-issue-25-cache-key-aoi/README.md

@@ -0,0 +1,114 @@
+# Findings — dft_stac_fetch cache key omits AOI (#25)
+
+## Issue context
+
+**Repo:** NewGraphEnvironment/drift · **Severity:** high (silent wrong data, no error) · **Version seen:** 0.2.2
+
+### Summary
+
+`dft_stac_fetch()` caches fetched rasters at `file.path(cache_source_dir, paste0(yr, ".nc"))`
+(`R/dft_stac_fetch.R:103`) — keyed only by **source** and **year**, with **no AOI component**. Any
+two calls with the same `source`/`year` but different `aoi` collide: the second call finds the
+first call's NetCDF, skips the fetch (when `force = FALSE`, the default), and returns the **first
+AOI's raster masked to the second AOI**. No warning, no error — just wrong data.
+
+### Evidence (real occurrence)
+
+Running two BC watershed areas through a floodplain/LULC pipeline that calls
+`dft_stac_fetch(source = "io-lulc", years = c(2017, 2020, 2023))`:
+
+1. Area A (Neexdzii, a reach of the Bulkley) ran first → populated
+   `~/Library/Caches/drift/io-lulc/{2017,2020,2023}.nc` with Neexdzii's extent. Correct output.
+2. Area B (MORR / Morice, ~80 km west, larger) ran second → `dft_stac_fetch` found the cache files
+   and returned **Neexdzii's** rasters, masked to the MORR floodplain.
+
+Cache extent vs. AOIs (EPSG:32609, metres):
+
+| | E min–max | N min–max |
+|---|---|---|
+| cache `io-lulc/*.nc` | 645443–696463 | 6000758–6056578 |
+| **Neexdzii** fp bbox | 645444–696461 | 6000762–6056573 | ← cache == Area A |
+| **MORR** fp bbox | 566715–651331 | 5948369–6035818 | ← what Area B should have gotten |
+
+Result: MORR's land cover was classified over only the ~3% where the Neexdzii cached extent
+overlaps the MORR floodplain (near the shared Bulkley/Morice confluence); "tree loss" came out
+22 ha of Bulkley-valley agricultural transitions instead of the true MORR figure.
+
+### Secondary bug: `force = TRUE` cannot overwrite
+
+`force = TRUE` routes to the fetch branch and calls `gdalcubes::write_ncdf(cube, cache_file)`
+without removing the existing file first. When the cache file exists, `write_ncdf` errors:
+
+```
+Error: File already exists, please change the output filename or set overwrite = TRUE
+```
+
+So `force = TRUE` cannot be used to bypass a stale/colliding cache — the user must manually delete
+the file (or call `dft_cache_clear()`).
+
+### Fix (from issue)
+
+1. **Put the AOI in the cache key.** Hash the AOI (bbox + geometry) into the filename. Preserves
+   caching for repeat runs of the *same* AOI while eliminating cross-AOI collisions. (Also fold
+   `res`, `crs`, `aggregation` into the key, since they change the output too.)
+2. **Fix `force = TRUE`** to overwrite instead of erroring.
+3. **Defensive check (optional):** on a cache hit, verify the cached raster's extent covers the
+   requested AOI bbox; if not, re-fetch.
+
+### Minimal repro
+
+```r
+library(drift)
+a <- sf::st_as_sf(sf::st_sfc(sf::st_buffer(sf::st_point(c(-126.75, 54.41)), 0.1), crs = 4326))
+b <- sf::st_as_sf(sf::st_sfc(sf::st_buffer(sf::st_point(c(-127.75, 54.05)), 0.1), crs = 4326))  # ~65 km west
+ra <- dft_stac_fetch(a, source = "io-lulc", years = 2020)  # fetches
+rb <- dft_stac_fetch(b, source = "io-lulc", years = 2020)  # returns a's cached raster, masked to b -> mostly NA
+# terra::ext(rb[["2020"]]) matches a, not b
+```
+
+## Plan-mode exploration (2026-07-06)
+
+### Code facts
+
+- Only place the `<year>.nc` filename is constructed: `R/dft_stac_fetch.R:103`. Written at :126,
+  read at :107/:127. No other code, test, vignette, or data-raw script assumes the pattern.
+- `dft_cache_clear()` / `dft_cache_info()` (`R/dft_cache.R`) are filename-agnostic
+  (`list.files(recursive = TRUE)` / `unlink(recursive = TRUE)`) — unaffected by a filename change.
+  `dft_cache_clear(source=)` assumes only the per-source subdirectory, which is kept.
+- Fetch-affecting params NOT in the current key: `aoi`, `res`, `crs`, `dt`, `aggregation`,
+  `resampling`, `stac_url`, `collection`, `asset`. All must enter the hash. `sign_fn` doesn't
+  affect pixels; `source` remains the directory.
+- `rlang` and `sf` are already in Imports; `digest` is not a dependency and isn't needed —
+  `rlang::hash()` (XXH128) works. No hashing exists anywhere in the package yet.
+- Existing tests never exercise the network fetch path (only `auto_utm_epsg` and the
+  missing-gdalcubes error). Cache-key helper is unit-testable fully offline via `drift:::`.
+
+### Design decisions (validated against installed sf 1.1.0 / rlang 1.2.0 / gdalcubes 0.7.3)
+
+- **Hash WKB (`sf::st_as_binary(sf::st_geometry(x), endian = "little")`), not the sfc object.**
+  sfc carries a PROJ-generated CRS WKT that drifts across PROJ versions → spurious cache misses.
+  WKB is coordinates + geometry type only; CRS enters the key separately as `target_crs`.
+  Also immune to sf attribute columns (verified: sf-with-attributes and bare sfc hash identically
+  via WKB).
+- **`as.numeric(res)`** — `10L` vs `10` serialize differently under `rlang::hash()`; identical
+  fetches would get different keys without coercion.
+- **Hash post-resolution `stac_url`/`collection`/`asset`** (after the `%||%` config resolution),
+  never the raw possibly-NULL args — otherwise `dft_stac_fetch(aoi)` and an explicit-but-identical
+  call hash differently. Bonus: also fixes a latent collision where a custom collection with
+  default `source = "io-lulc"` landed in the io-lulc dir keyed only by year.
+- **Year stays out of the hash** — key computed once before the per-year `lapply`; filename
+  `<year>_<key>.nc` groups all years of one call under a shared readable suffix.
+- **`write_ncdf(..., overwrite = TRUE)` over bare `unlink()`** — gdalcubes 0.7.3 signature is
+  `write_ncdf(x, fname, overwrite = FALSE, ...)`. Bare `unlink()` fails *silently* on Windows when
+  a prior SpatRaster holds a GDAL handle on the file, reproducing the original confusing error.
+- **Extent check (issue's optional item 3) skipped** — user confirmed 2026-07-06. gdalcubes'
+  `cube_view` only ever *enlarges* extents to fit the pixel grid, so a containment check with
+  one-pixel tolerance would validate nothing; post-hash-fix, legacy `<year>.nc` files can never
+  match the new pattern anyway.
+- **Old-format cache files become dead weight** — correct behavior; do NOT auto-delete (can't
+  attribute them to an AOI). NEWS notes existing caches refetch and `dft_cache_clear()` reclaims
+  space.
+- **POSIX silent-swap caveat** — with `force = TRUE`, a SpatRaster returned by an earlier call and
+  backed by the same cache file may lazily reopen and see the new content. Newly reachable (the
+  old behavior errored first), but benign under the hash key: the overwritten file corresponds to
+  the identical parameter set. Documented in `@param force` rather than engineered around.

planning/archive/2026-07-issue-25-cache-key-aoi/findings.md

@@ -0,0 +1,15 @@
+# Progress — dft_stac_fetch cache key omits AOI (#25)
+
+## Session 2026-07-06
+
+- Plan-mode exploration — phases approved by user (extent check explicitly skipped)
+- Created branch `25-dft-stac-fetch-cache-key-omits-aoi-secon` off main
+- Scaffolded PWF baseline from issue #25 with approved phases
+- Phase 1 complete: `stac_cache_key()` helper + `<year>_<key>.nc` filenames + 5 local key tests
+  (suite: 192 pass, 0 fail; lint clean)
+- Phase 2 complete: `write_ncdf(..., overwrite = TRUE)` + `@param force` doc caveat
+- Phase 3 complete: cache-keying note in roxygen, NEWS 0.2.3, version bump
+- E2E verified against live Planetary Computer STAC: two AOIs ~64 km apart produced two
+  distinct cache files (`2020_622235623d95.nc`, `2020_d631fe72d838.nc`), each raster matched
+  its own AOI extent, re-run hit the cache, `force = TRUE` overwrote without error
+- Next: /planning-archive + /gh-pr-push

planning/archive/2026-07-issue-25-cache-key-aoi/progress.md

@@ -0,0 +1,32 @@
+# Task: dft_stac_fetch cache key omits AOI -> second area silently gets first area's raster (#25)
+
+`dft_stac_fetch()` caches fetched rasters at `file.path(cache_source_dir, paste0(yr, ".nc"))`
+(`R/dft_stac_fetch.R:103`) — keyed only by **source** and **year**, with **no AOI component**. Any
+two calls with the same `source`/`year` but different `aoi` collide: the second call finds the
+first call's NetCDF, skips the fetch (when `force = FALSE`, the default), and returns the **first
+AOI's raster masked to the second AOI**. No warning, no error — just wrong data.
+
+## Phase 1: Cache key includes AOI + fetch parameters
+
+- [x] Add internal `stac_cache_key()` helper in `R/dft_stac_fetch.R` — WKB geometry + `as.numeric(res)` + `target_crs` + `dt` + `aggregation` + `resampling` + post-resolution `stac_url`/`collection`/`asset`, `rlang::hash()`, 12-char prefix
+- [x] Compute key once after AOI/CRS resolution; cache filename becomes `<year>_<key>.nc` (`R/dft_stac_fetch.R:103`)
+- [x] Unit tests in `tests/testthat/test-dft_stac_fetch.R` (all local, no network): determinism; shifted geometry → different key; different `res`/`crs`/`collection`/`asset`/`stac_url` → different keys; `res = 10` vs `10L` → same key; sf-with-attributes vs bare sfc → same key; key matches `^[0-9a-f]{12}$`
+
+## Phase 2: force = TRUE overwrites cleanly
+
+- [x] `gdalcubes::write_ncdf(cube, cache_file, overwrite = TRUE)` (`R/dft_stac_fetch.R:126`)
+- [x] Update `@param force` roxygen — overwrites the cached file; note that a SpatRaster returned earlier and backed by the same file may silently see new content on POSIX
+
+## Phase 3: Docs + release
+
+- [x] Roxygen note in `dft_stac_fetch` docs: cache entries keyed by AOI geometry + fetch parameters; `devtools::document()`
+- [x] NEWS.md entry for 0.2.3: bug + fix, existing caches will refetch, `dft_cache_clear()` reclaims space
+- [x] `lintr::lint_package()` clean + full `devtools::test()` pass (one pre-existing vignette lint, untouched by this branch)
+- [x] Version bump to 0.2.3 in DESCRIPTION as final commit
+
+## Validation
+
+- [x] Tests pass
+- [x] `/code-check` clean on each commit
+- [x] PWF checkboxes match landed work
+- [x] `/planning-archive` on completion

planning/archive/2026-07-issue-25-cache-key-aoi/task_plan.md

@@ -28,3 +28,55 @@ test_that("dft_stac_fetch requires gdalcubes", {
   )
   expect_error(dft_stac_fetch(aoi), "gdalcubes")
 })
+
+# helpers for stac_cache_key tests: a unit-square polygon (optionally shifted)
+# and a key call with fixed defaults so each test varies one input at a time
+square_aoi <- function(dx = 0) {
+  sf::st_sfc(
+    sf::st_polygon(list(rbind(
+      c(0 + dx, 0), c(1 + dx, 0), c(1 + dx, 1), c(0 + dx, 1), c(0 + dx, 0)
+    ))),
+    crs = 32609
+  )
+}
+
+cache_key <- function(aoi = square_aoi(), res = 10, target_crs = "EPSG:32609",
+                      dt = "P1Y", aggregation = "first", resampling = "near",
+                      stac_url = "https://example.com/stac",
+                      collection = "test-collection", asset = "data") {
+  drift:::stac_cache_key(aoi, res, target_crs, dt, aggregation, resampling,
+                         stac_url, collection, asset)
+}
+
+test_that("stac_cache_key is deterministic and 12-char hex", {
+  k1 <- cache_key(square_aoi())
+  k2 <- cache_key(square_aoi())
+  expect_equal(k1, k2)
+  expect_match(k1, "^[0-9a-f]{12}$")
+})
+
+test_that("stac_cache_key changes when the AOI geometry changes", {
+  expect_false(cache_key(square_aoi()) == cache_key(square_aoi(dx = 0.5)))
+})
+
+test_that("stac_cache_key changes with each fetch-affecting parameter", {
+  base <- cache_key()
+  expect_false(cache_key(res = 20) == base)
+  expect_false(cache_key(target_crs = "EPSG:32610") == base)
+  expect_false(cache_key(dt = "P2Y") == base)
+  expect_false(cache_key(aggregation = "median") == base)
+  expect_false(cache_key(resampling = "bilinear") == base)
+  expect_false(cache_key(stac_url = "https://other.com/stac") == base)
+  expect_false(cache_key(collection = "other-collection") == base)
+  expect_false(cache_key(asset = "other-asset") == base)
+})
+
+test_that("stac_cache_key treats integer and double res alike", {
+  expect_equal(cache_key(res = 10L), cache_key(res = 10))
+})
+
+test_that("stac_cache_key ignores sf attribute columns", {
+  bare <- square_aoi()
+  with_attrs <- sf::st_sf(name = "a", area = 1.5, geometry = bare)
+  expect_equal(cache_key(with_attrs), cache_key(bare))
+})