diff --git a/R/dft_stac_composite.R b/R/dft_stac_composite.R index 2ef0da5..874d6d5 100644 --- a/R/dft_stac_composite.R +++ b/R/dft_stac_composite.R @@ -313,6 +313,10 @@ dft_stac_composite <- function(aoi, # it, keeping the years already built (and cached) before it. tryCatch( build(), + # before drift_no_items, which it subclasses: more months cannot fix it + drift_no_mask_items = function(e) { + composite_skip(label, "no scene has the cloud-mask asset") + }, drift_no_items = function(e) composite_skip(label, "no scenes"), drift_empty_cube = function(e) composite_skip(label, "no clear pixels") ) @@ -322,8 +326,9 @@ dft_stac_composite <- function(aoi, if (!length(out)) { cli::cli_abort(c( "No year produced a composite.", - "i" = "Every window had no scenes or no clear pixels; try more \\ - {.arg months} or a higher {.arg cloud_cover_max}." + "i" = "Every window was skipped; the warnings say why. For no scenes or \\ + no clear pixels, try more {.arg months} or a higher \\ + {.arg cloud_cover_max}." )) } out diff --git a/R/dft_stac_cube.R b/R/dft_stac_cube.R index c72c84c..93acca9 100644 --- a/R/dft_stac_cube.R +++ b/R/dft_stac_cube.R @@ -524,6 +524,13 @@ stac_cube_items <- function(cfg, aoi_wgs84, datetime, cloud_cover_max, months, rlang::abort(paste0("No STAC items found for ", cfg$collection), class = "drift_no_items") } + # Before is_pre, so the offset split is computed on the features that are read. + items$features <- stac_features_masked(items$features, cfg$roles$mask) + if (length(items$features) == 0) { + rlang::abort(paste0(n_items, " STAC items found for ", cfg$collection, + ", but none has the mask asset '", cfg$roles$mask, "'"), + class = c("drift_no_mask_items", "drift_no_items")) + } # Baseline-conditional offset: split items at the boundary so each side is # corrected with its own offset. The split is by item date, so it is the same @@ -546,6 +553,33 @@ stac_cube_items <- function(cfg, aoi_wgs84, datetime, cloud_cover_max, months, } +#' Drop STAC features that have no mask asset +#' +#' gdalcubes uses an image whose mask band is missing from the collection +#' UNMASKED: it warns `Missing mask band for image ..., mask will be ignored` +#' from C++ and records the chunk as OK (drift#99), so clouds reach a median with +#' nothing R can see. Such a feature is dropped here, before the collection is +#' built, and the drop is announced, so the read has fewer scenes rather than +#' unmasked ones: the same outcome as a scene the mask removes entirely. +#' +#' `mask_asset` `NULL` (a source with no mask role) keeps everything. The asset +#' is matched by its exact name: `[[` on the assets list, never `$`, which would +#' partial-match `SCL_20m` for `SCL`. +#' @noRd +stac_features_masked <- function(features, mask_asset) { + if (is.null(mask_asset) || length(features) == 0) return(features) + has_mask <- vapply(features, function(f) !is.null(f$assets[[mask_asset]]), logical(1)) + if (all(has_mask)) return(features) + n <- sum(!has_mask) # nolint: object_usage_linter. used in the cli template + ids <- vapply(features[!has_mask], function(f) f$id %||% NA_character_, "") # nolint: object_usage_linter. + cli::cli_inform(c( + "i" = "Dropped {n} item{?s} with no {.val {mask_asset}} mask asset, which \ + gdalcubes would read unmasked: {.val {ids}}." + )) + features[has_mask] +} + + #' Re-sign STAC features just before they are read #' #' Planetary Computer SAS tokens last about 45 minutes (measured 2026-09-28: @@ -750,9 +784,10 @@ cube_check_nonempty <- function(stk, collection, datetime, cached) { #' file's close (Rcpp's own header warns of exactly this). Other gdalcubes #' warnings pass through untouched. #' -#' Not every failed merge reaches either signal: a worker chunk file the main -#' process cannot open is skipped with status OK and no warning, a C++ stderr -#' line only (round-2 review of #87; drift#99). +#' Not every failed merge reaches either signal on gdalcubes 0.7.5: a worker +#' chunk file the main process cannot open is skipped with status OK and no +#' warning, a C++ stderr line only (round-2 review of #87). The +#' NewGraphEnvironment/gdalcubes fix for drift#99 records it as ERROR. #' @noRd cube_write_ncdf <- function(cube, out) { unmerged <- character(0) @@ -790,23 +825,26 @@ cube_write_ncdf <- function(cube, out) { #' INCOMPLETE, `128` UNKNOWN (gdalcubes 0.7.5, `src/gdalcubes/src/cube.cpp`). A #' worker carries the status to the main process inside its chunk file. #' -#' **What it records is narrower than "the read failed".** Measured offline -#' (#87): a band image that fails to OPEN marks its chunks INCOMPLETE, which is -#' the expired or refused signed URL. A band image that opens and then fails -#' mid-read (a corrupted tile here; a throttled range request on the network) -#' leaves the status OK, and so does a mask (SCL) image that fails to open, which -#' silently drops that scene. A mask image that opens and then fails mid-read is -#' worse: the scene is used unmasked. gdalcubes sets the status only when a band -#' image will not open; every other I/O step drops its return code, so none of -#' these is visible to this check or to anything else R can see (drift#99; -#' inst/notes/gdalcubes-pc-gotchas.md). +#' **On gdalcubes 0.7.5 it records less than "the read failed".** Measured +#' offline (#87, #99): a band image that fails to OPEN marks its chunks +#' INCOMPLETE, which is the expired or refused signed URL. A deleted mask (SCL) +#' image is caught the same way in drift's reads, because gdalcubes also reads +#' the mask asset as a band there (no `select_bands()` before `apply_pixel()`). +#' An image that opens and then fails mid-read (a corrupted tile here; a +#' throttled range request on the network) leaves the status OK. For a band, +#' the chunk is filled from the scenes that did read; for the mask, the scene +#' is used UNMASKED, so cloud reaches the median. Neither is visible to this +#' check or to anything else R can see on 0.7.5. The NewGraphEnvironment/gdalcubes +#' fix for drift#99 records both, so this check then catches them unchanged +#' (inst/notes/gdalcubes-pc-gotchas.md). #' #' **The netCDF integer fill value means no status was written for that id**, #' not that the chunk was empty: a worker skips writing a chunk that is OK and #' all-`NA` (no image, or fully masked), and a chunk whose merge threw keeps fill #' too. That second case raises an R warning, which cube_write_ncdf() turns into #' an abort, so fill is read as OK here. A worker chunk file the main process -#' cannot open is recorded as OK, with neither signal (drift#99). Anything that is neither OK nor fill fails, so a +#' cannot open is recorded as OK on 0.7.5, with neither signal, and as ERROR +#' with the drift#99 fix. Anything that is neither OK nor fill fails, so a #' status code a later gdalcubes adds aborts rather than passes. #' #' For an open failure, this is the only check that sees it. A median over two @@ -833,10 +871,12 @@ cube_check_chunks <- function(nc_file) { if (failed[["failed"]] == 0L) return(invisible(nc_file)) cli::cli_abort(class = "drift_incomplete_cube", c( "gdalcubes could not read {failed[['failed']]} of {failed[['chunks']]} \\ - chunk{?s}: an image would not open.", + chunk{?s}: an image would not open or read.", "x" = "Those cells are {.val NA}, or computed from only the scenes that \\ - did open.", - "i" = "Usually an expired or refused signed URL. Nothing was cached.", + did read.", + "i" = "Usually an expired or refused signed URL; also a refused range \\ + request mid-read, or a worker chunk file that could not be read \\ + back (drift#99). Nothing was cached.", "i" = "Re-running signs the request again. If the same read fails every \\ time, an asset may be missing from the catalogue." )) diff --git a/inst/notes/gdalcubes-pc-gotchas.md b/inst/notes/gdalcubes-pc-gotchas.md index 748dba1..70e595b 100644 --- a/inst/notes/gdalcubes-pc-gotchas.md +++ b/inst/notes/gdalcubes-pc-gotchas.md @@ -246,11 +246,28 @@ Measured on gdalcubes 0.7.5 (`appelmar/gdalcubes@ed68331`) with a local collecti - **The last row is why a pixel check cannot do this.** A median over two scenes where one failed fills every cell from the survivor, so the holed cube has no NA at all. The issue's third option (require data under every item footprint) would have passed it. - **Read it with ncdf4, not terra.** GDAL does not list a one-dimensional variable as a subdataset, so `terra::rast(f, subds = "chunk_status")` errors; the `NETCDF:"f":chunk_status` form opens but prints a stray `R_nc4_open` error line. ncdf4 is a gdalcubes import. - **The `[WARNING]` line is C++ stdio**, so neither an R handler nor `capture.output()` silences it, even at `parallel = 1`. -- **Only one failure is recorded: a band image that fails to open.** gdalcubes sets `chunk_status` in six places, all in `image_collection_cube::read_chunk()`, and only band `GDALOpen` returning NULL can fire in practice; every other I/O step between `GDALOpen` and the final `nc_close` drops its return code or warns and continues. A code-check enumeration of the write pipeline (#87, round 3, `planning/archive/2026-10-issue-87-silent-chunk-read-failures/review-round3.md`) found 38 failure paths: 25 detected by drift (13 through `chunk_status` or the merge warning, 12 as an R error before anything is cached), 13 not. The ones that matter: - - a band image that opens then fails mid-read: status OK, cells filled from the scenes that read; - - **a mask (SCL) image that opens then fails mid-read: the scene is used UNMASKED** — cloud values in the median, not NA (measured: an all-cloud scene gave 1600 of 1600 cells, status OK); - - a mask image that fails to open, or a scene with no mask asset: dropped, or used unmasked; - - a worker that writes no chunk file, or a partial or unreadable one: status OK or fill, no R warning; - - a worker killed by a signal: `write_ncdf()` hangs. - These are drift#99. A chunk whose merge throws keeps fill and raises an R warning (`Chunk N could not be added to output`), which drift does catch. +- **On 0.7.5 only one failure is recorded: a band image that fails to open.** gdalcubes sets `chunk_status` in six places, all in `image_collection_cube::read_chunk()`, and only band `GDALOpen` returning NULL can fire in practice; every other I/O step between `GDALOpen` and the final `nc_close` drops its return code or warns and continues. A code-check enumeration of the write pipeline (#87, round 3, `planning/archive/2026-10-issue-87-silent-chunk-read-failures/review-round3.md`) found 38 failure paths: 25 detected by drift (13 through `chunk_status` or the merge warning, 12 as an R error before anything is cached), 13 not. Which of them the drift#99 fork fix closes is in the next section. - `cube_write_ncdf()` is drift's one `write_ncdf()` call, in `stac_cube_assemble()` and `fetch_extent_to()`. It notes the merge warning, lets gdalcubes return (aborting from inside an `Rcpp::warning()` handler would skip its C++ cleanup), then aborts on it or on `chunk_status`, so a failed read aborts before anything is cached. Before it, the offline fixture showed both `dft_stac_composite()` and `dft_stac_fetch()` caching the holed result, tiled and untiled. + +## Failed reads after open, mask failures, worker deaths (#99, 2026-10-14) + +Fixed on `NewGraphEnvironment/gdalcubes`, branch `99-chunk-status-read-failures` (off `ed68331`), for upstream. drift keeps `Remotes: appelmar/gdalcubes`; its tests for these cases skip with a drift#99 reason until the installed gdalcubes records them (`skip_if_no_read_failure_status()`, `tests/testthat/helper-gdalcubes.R`). + +| case | 0.7.5 | fork branch | +|---|---|---| +| band image opens, then a tile fails to decode | OK; chunk filled from the scenes that read | INCOMPLETE | +| mask (SCL) opens, then fails to read | OK; **scene used unmasked** (the cloud value reaches the output) | INCOMPLETE; scene dropped | +| mask fails to open, `select_bands()` before the write | OK; scene dropped | INCOMPLETE | +| mask fails to open, **drift's read shape** (`apply_pixel()`, no `select_bands()`) | INCOMPLETE already: SCL is read as a band too, and its band open fails | INCOMPLETE | +| main process cannot open / read back a worker chunk file | OK, C++ stderr only | ERROR | +| worker dies before writing its log (signal at startup) | `write_ncdf()` spins forever at 100% CPU | raises in under a second | +| worker killed mid-computation | raises (`worker process #0 returned 9`) | raises | +| item with no mask asset in the STAC listing | used unmasked, status OK | unchanged; **drift drops the item** (`stac_features_masked()`) | + +- **Every chunk is computed in an Rscript worker, at `parallel = 1` too** (gdalcubes' `.onLoad` and each `gdalcubes_options()` call set the process executor). Workers load gdalcubes from `R_LIBS`, not from the session's `.libPaths()`, so to test a build from a scratch library export `R_LIBS` in the shell; a debugger on the main R process sees nothing of a worker crash. +- **The cause was one unchecked return.** `gdalwarp_client::warp_simple()` (every image with a geotransform) ignored `ChunkAndWarpImage()` and returned the `INIT_DEST=nan` output as data. Returning nullptr reaches `read_chunk()`'s existing `!gdal_out` branches, for band and mask alike. +- **The hang was the log printer, not the supervision loop.** That loop breaks on exit 9 as it should. Then `while (!wlogf.eof()) getline(...)` reads each worker's log, and a worker killed at startup never created one: a failed `ifstream` never reaches `eof()`. Sampled while hung: no worker children, main thread hot in `chunk_processor_multiprocess::apply()`. The #87 round-1 hang was this case; a worker killed mid-write, after its log exists, always raised. +- **A fixture that fails to read but still opens** needs the IFD intact. A GDAL GTiff keeps its IFD at the END, so overwriting the tail tests an open failure; a COG keeps it first, but its tail is the DEFLATE trailer and still decoded. Overwrite the first tile at its `TileOffsets` entry: `tif_corrupt_tile()` in the helper, giving `ZIPDecode ... incorrect header check`. +- **Still unrecorded with the fix:** a worker that cannot create or move its chunk file (`nc_create` / `CPLMoveFile` unchecked in `exec()`) leaves fill and no warning; an `sqlite3_step` error truncates a chunk's image list. +- **Separate, pre-existing:** a two-band image with the mask in band 2 segfaults a worker, at full resolution and through overviews alike, on 0.7.5 too (#92 hit it with `one_band_per_file = FALSE`). drift's Sentinel-2 assets are single-band. + diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/README.md b/planning/archive/2026-10-issue-99-read-failures-after-open/README.md new file mode 100644 index 0000000..6399720 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/README.md @@ -0,0 +1,55 @@ +## Outcome + +The silent gdalcubes failures left over from #87 were fixed at their source, on our fork, for upstream. The cause was small: `gdalwarp_client::warp_simple()` ignored the return of `ChunkAndWarpImage()`, so a read that failed after a successful open came back as an all-NaN "image". A mask that failed to read therefore left its scene unmasked, and cloud reached the median with status OK. + +The fork branch (NewGraphEnvironment/gdalcubes#2, off `ed68331`) does five things: +- makes a failed warp return nullptr, which reaches `read_chunk()`'s existing INCOMPLETE branches; +- records a failed mask open; +- sets ERROR when the main process cannot read back a worker's chunk file; +- opens an overview before closing the source; +- fixes the "worker killed by a signal hangs `write_ncdf()`" case. + +That hang turned out to be the log printer: `while (!eof())` on a log a worker killed at startup never wrote. It was not the supervision loop. The upstream PR is drafted in `upstream-pr-draft.md` and **not posted**. + +drift keeps `Remotes: appelmar/gdalcubes`, as decided at the plan gate. Its changes: +- drops STAC items that have no mask asset (gdalcubes would read them unmasked), with its own error subclass; +- adds fixtures that corrupt a COG tile with the IFD intact; +- adds tests gated on a capability probe, which skip with a drift#99 reason on 0.7.5; +- updates the docs. + +One correction to the issue's own table: in drift's read shape (`apply_pixel()` with no `select_bands()`), gdalcubes also reads SCL as a band. So a *deleted* SCL is already recorded on 0.7.5. The case drift really missed is a *corrupted* SCL. + +## Measurement + +| check | 0.7.5 | fork branch | +|---|---|---| +| `test_chunk_status.R` (32 assertions) | 10 fail: band read, mask open, mask read and strict mask open, each at parallel 1 and 2; the cloud value 900 reached the output | 32 pass | +| `test_worker_failure.R` | both fail at the 120 s timeout | pass in 0.9 s | +| full gdalcubes tinytest | — | 159 / 159 | +| corrupted SCL in drift's shape, failed chunks | 0 | 9 | +| deleted SCL in drift's shape, failed chunks | 9 | 9 | +| deleted SCL with `select_bands()` | 0 | 9 | + +- **drift tests:** gated tests SKIP on 0.7.5. With `DRIFT_TEST_ASSUME_READ_STATUS=true` exactly those tests fail; on the fork build all pass. The full drift suite is green on 0.7.5. +- **Mutations that turned tests red:** + - unwiring the mask-asset drop (5 failures); + - a probe forced to answer FALSE; + - `select_bands()` inserted in `stac_cube_assemble()` (M9); + - the composite handler order swapped. +- **Wrong turns, kept:** + - The first fixture corrupted the end of a GTiff. A GTiff keeps its IFD there, so that tested an open failure, which read as "0.7.5 already records band read failures". Overwriting the tail of a COG still decoded. The tile has to be broken at its `TileOffsets` entry. + - A mid-write SIGKILL never hung. Only a kill at startup does. + - A multi-band "overview" segfault from review round 1 also happens at full resolution on 0.7.5. It is the #92 two-band defect and out of scope. +- **Code-check:** + - Fork: 3 rounds. Round 1 found a NULL dereference inside the plan-review fix. Round 3 ended the loop with a 39-row ownership and state enumeration. + - drift: 3 rounds. Round 3 walked every new assertion and guard with mutations. + - Seven reviewer agents in all, plus one plan reviewer. + +## Evidence + +- Fork branch `NewGraphEnvironment/gdalcubes:99-chunk-status-read-failures`: `inst/tinytest/test_chunk_status.R`, `inst/tinytest/test_worker_failure.R`. +- Review rounds: `review-plan.md`, `review-fork-round[123].md`, `review-drift-round[123].md` in this directory. +- Durable notes: `inst/notes/gdalcubes-pc-gotchas.md`, "Failed reads after open, mask failures, worker deaths (#99)". +- The hang and segfault repros were scratch-only; the hang is now `test_worker_failure.R`. + +Closed by: drift PR for branch `99-gdalcubes-read-failures-after-open-mask` (see `gh pr list --search 99`); fork PR NewGraphEnvironment/gdalcubes#2 diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/findings.md b/planning/archive/2026-10-issue-99-read-failures-after-open/findings.md new file mode 100644 index 0000000..d4b0182 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/findings.md @@ -0,0 +1,152 @@ +# Findings — gdalcubes read failures after open, mask-open failures and worker crashes are still silent (#99) + +## Issue context + +## Problem + +#87 made drift abort on a gdalcubes chunk whose **band image fails to open** (an expired or refused signed URL), by reading the per-chunk `chunk_status` gdalcubes writes into its NetCDF. That is the only failure gdalcubes records. It sets `chunk_status` in six places, all in `image_collection_cube::read_chunk()`, and only band `GDALOpen` returning NULL can fire in practice. **Every other I/O step between `GDALOpen` and the final `nc_close` drops its return code, or warns and continues, leaving the status OK**: `ChunkAndWarpImage`, every netCDF call between worker and main process, `CPLMoveFile`, `sqlite3_step`. + +A code-check enumeration of the write pipeline (#87 round 3: 38 failure paths, 25 detected by drift, 13 not; table in the #87 PR's `planning/archive/2026-10-issue-87-silent-chunk-read-failures/review-round3.md`) and offline probes on gdalcubes 0.7.5 (`appelmar/gdalcubes@ed68331`, `parallel` 1 and 4): + +| case | `chunk_status` | cells | +|---|---|---| +| **mask (SCL) image opens, then a tile read fails** | OK | **scene used UNMASKED**: the mask buffer stays NaN, which never matches a mask value, so cloud values reach the median (all-cloud scene: 1600 of 1600 cells set) | +| scene with no mask asset | OK | scene used unmasked | +| worker writes no chunk file (`nc_create` / `CPLMoveFile` fails; worker exits 0) | fill, no R warning | those cells `NA` | +| worker chunk file partial (missing dims/vars, or `nc_get_var_double` fails unchecked) | OK | empty, or an uninitialised buffer merged as data (read from source) | +| `sqlite3_step` error in `find_range_st` | OK | chunk's image list silently truncated (read from source) | +| band image opens, then a tile read fails (corrupted tile here; a throttled or refused range request on the network) | OK | every cell set, from the scenes that read | +| mask (SCL) image fails to open | OK | that scene silently dropped | +| a worker chunk file the main process cannot open (e.g. truncated when the temp dir fills) | OK, no R warning | those cells `NA` (measured in the #87 round-2 review: 1536 of 6400 cells, a C++ stderr line only; `chunk_data::read_ncdf()` returns on `nc_open` failure without setting a status) | +| a gdalcubes worker killed by a signal (SIGKILL, segfault) | — | `write_ncdf()` hangs; never raises or returns (round-1 review of #87, measured to a 150 s timeout) | + +`gdalcubes_options(log_file =, debug = TRUE)` writes the same 11 lines for clean and failing reads, so it is not a detector either. + +Where in gdalcubes (paths under `src/gdalcubes/src/`, from the #87 plan review): +- `image_collection_cube.cpp:424-437` sets INCOMPLETE only when `GDALOpen` fails or `warp()` returns nullptr; `warp.cpp` writes into a MEM dataset it creates first, does not check `ChunkAndWarpImage` (`:256`), and logs a failure only at `GCBS_DEBUG` (`:650-654`). +- `image_collection_cube.cpp:569-579`: a mask that fails to open warns and `continue`s the image loop without `set_status`. + +## Mitigation already in drift + +#87 added `GDAL_HTTP_MAX_RETRY = 3` and `GDAL_HTTP_RETRY_DELAY = 5` to the cube/composite session and to every `dft_stac_fetch()`, so a transient 429/5xx is retried rather than left as a hole. Per-extent re-signing (#79) keeps tokens live. + +## Options + +1. **Upstream (preferred):** propose to gdalcubes that a failed warp/RasterIO and a failed mask open set the chunk to INCOMPLETE, and that a worker exiting on a signal is treated as a failed chunk rather than waited on. Draft below. **Do not post without approval.** +2. **drift-side worker timeout:** none exists in gdalcubes; a wall-clock deadline around `write_ncdf()` would need a subprocess. Probably not worth it before upstream answers. + +### Draft upstream issue (NOT POSTED) + +> **Title:** Failed image reads after a successful open, and failed mask opens, are reported as OK in `chunk_status` +> +> `write_ncdf()` records a per-chunk `chunk_status`, which is very useful for detecting failed reads from R (the console warning is not visible from worker processes). It records a band image that fails to open as INCOMPLETE. Two other failures are recorded as OK: +> +> 1. An image that opens but whose pixel read fails — for a MASK image this leaves the scene unmasked, since the NaN buffer never matches a mask value (e.g. a corrupted tile, or a throttled HTTP range request on `/vsicurl/`). `warp()` writes into a MEM dataset and the result of `ChunkAndWarpImage` is not checked, so the chunk is computed from the remaining images and marked OK. +> 2. A mask band image that fails to open: the image loop `continue`s without `set_status`. +> +> Reproducible offline: build a collection from local GeoTIFFs, then corrupt one band file's tile data with the IFD intact (or delete one mask file), and `write_ncdf()` a median cube: `chunk_status` is 0 everywhere. Would you accept a PR that sets `chunk_status::INCOMPLETE` in both cases? +> +> Also: a worker chunk file the main process cannot open is skipped with status OK (`chunk_data::read_ncdf()` returns on `nc_open` failure without `set_status`), and a worker process killed by a signal leaves the main `write_ncdf()` waiting indefinitely. + +Relates: #87, #79 + + + + +## Plan-mode reading of gdalcubes ed68331 (2026-10-02) + + +#87 made drift abort on a chunk whose band image fails to OPEN, by reading gdalcubes' +per-chunk `chunk_status`. Several other failures still record OK. The worst is a mask (SCL) image +that opens and then fails to read: the scene is used unmasked and clouds reach the median. +Reading gdalcubes `ed68331` (unchanged upstream as of 2026-10-14) shows the cause is small and +in C++. `gdalwarp_client::warp()` (`warp.cpp:650-655`) logs a failed `ChunkAndWarpImage` at +DEBUG and returns the dataset anyway. `read_chunk()` already sets INCOMPLETE when `warp()` +returns nullptr (band `:516-521`, mask `:627-632`), so returning nullptr there fixes both +mid-read cases. A mask that fails to open (`image_collection_cube.cpp:569-579`) `continue`s +without `set_status`. `chunk_data::read_ncdf()` (`cube.cpp` ~1859-1920) returns on `nc_open`, +missing-dim or missing-var failure without a status, and never checks `nc_get_var_double`. + +Precedent: the filter_geom segfault was fixed on `NewGraphEnvironment/gdalcubes`, and upstream +merged it as appelmar/gdalcubes#111 in two weeks. Same route here. + +- `raster_cube(incomplete_ok = FALSE)` maps to C++ `_strict`: a band open failure then returns an ERROR (empty) chunk. The strict mask-open branch returns a fresh chunk_data with no status set. +- The worker supervision loop (`src/multiprocess.cpp`) breaks on `exit_status > 0`, and TinyProcess returns 9 for SIGKILL, so a hang is not explained by the loop alone. #87 round 1 saw `returned 9` printed only after SIGTERM to the main process. +- `Missing mask band for image ..., mask will be ignored`: gdalcubes deliberately uses an item with no mask asset unmasked. + +## Errors Encountered + +| Error | Resolution | +|-------|------------| + +## Fixture: corrupting a tile with the IFD intact (2026-10-14) + +- A plain GTiff written by GDAL puts its IFD at the END of the file, so overwriting the tail + corrupts the header and tests an OPEN failure. The first probe did exactly that and reported + band-read failure as INCOMPLETE on unpatched 0.7.5, contradicting #87's measurement. +- COG layout puts the IFDs first. Overwriting the last 30 bytes of a 1391-byte COG still + decoded (the bytes hit the DEFLATE tail and the COG trailer). What works: parse + `TileOffsets` (tag 324) from the first IFD and overwrite 64 bytes at the tile start, giving + `ZIPDecode ... incorrect header check` on read while `gdalinfo` still opens the file. +- `write_tif()` returns every file in the directory with the prefix, not just the ones it + wrote; two calls with one prefix gave duplicate paths and `cannot add dataset to gdalrefs`. +- tinyexpr in `apply_pixel` has no `t` variable; build each date from its own dummy cube. + +## Measurement: gdalcubes fork, branch 99-chunk-status-read-failures + +| run | 0.7.5 (`ed68331`) | fork branch | +|---|---|---| +| `test_chunk_status.R`, 30 assertions | 10 fail: band_read / mask_open / mask_read status OK, mask_read lets the cloud value 900 through, strict mask_open not ERROR (each at parallel 1 and 2) | 30 pass | +| `test_worker_failure.R` | both fail at the 120 s `system2` timeout | pass in 0.9 s | +| full tinytest suite | — | 157 / 157 | + +## Worker-crash hang: mechanism (2026-10-14) + +- Killing a worker MID-write (SIGKILL or SIGSEGV, both builds, parallel 2) raised promptly: + `[ERROR] worker process #0 returned 9`, then `one or more worker processes failed`, ~6 s. +- Killing a worker at STARTUP (a swapped worker script that SIGKILLs worker 1 before + `gc_exec_worker()`, the #87 round-1 shape) hung to the 60 s deadline on both builds. Control + (passthrough worker script): returned in 0.7 s. +- `sample` of the hung main process: no worker children left, state R (100% CPU), hot in + `chunk_processor_multiprocess::apply()`. The supervision loop had already broken on exit 9. + The spin is the log printer: `while (!wlogf.eof()) getline(...)` on a worker log that was never + created; a failed ifstream never reaches eof. The "returned 9" line was buffered and only + appeared once SIGTERM ended the process. +- Fix: `while (std::getline(wlogf, msg))`. Startup kill now raises in 0.4 s. +- Repro scripts were scratch-only: `hang_main.R`/`hang_run.sh` (mid-write kill), + `hang_early.R` + `worker_kill9.R` (startup kill). The startup case is now + `inst/tinytest/test_worker_failure.R` on the fork branch. + +## drift's read shape decides which SCL failures 0.7.5 records (2026-10-14) + +Measured with `chunk_fixture()` (SCL broken on one of two dates, masked median), failed chunks: + +| SCL | shape | 0.7.5 | fork | +|---|---|---|---| +| deleted | `apply_pixel()` (drift's shape) | 9 | 9 | +| deleted | `select_bands("B04")` | 0 | 9 | +| corrupted | `apply_pixel()` | **0** | 9 | +| corrupted | `select_bands("B04")` | 0 | 9 | + +With no `select_bands()`, gdalcubes reads the mask asset as a band too, so a deleted SCL fails a +band open and is already recorded. The issue table's "mask fails to open -> OK" row holds only +when the mask is selected away. The case drift actually misses on 0.7.5 is the corrupted (read +failure) SCL, which leaves the scene unmasked. So the drift test for a deleted SCL runs ungated, in +drift's shape; the corrupted-SCL and corrupted-band tests are gated. + +Drift test runs (`test-dft_stac_cube.R`, `test-dft_stac_composite.R`): +- 0.7.5: gated tests SKIP "installed gdalcubes records a failed read after open as OK (drift#99)" +- 0.7.5 with `DRIFT_TEST_ASSUME_READ_STATUS=true`: exactly the gated tests fail (cube 4 = 2 tests + x parallel 1/4; composite 2) +- fork build (`R_LIBS=/lib`): all pass + +## Fork code-check round 1 (review-fork-round1.md) + +- Bug inside the plan-review fix (G1): after a failed overview reopen, `in->GetRasterCount()` + dereferenced NULL before the new guard. Fixed by opening the overview into its own handle and + closing the source only on success (fork 7afdd3e). +- Vacuous test: the reprojection control's extent missed the data (0 non-NA). Fixed. +- Chasing the reviewer's repro (two-band COG, mask band 2) found it segfaults at full + resolution too, on 0.7.5 and the fork alike: the #92 two-band segfault, out of scope. lldb on a + worker (swap the worker command for `lldb -- bin/exec/R --file=worker.R --args`) ran clean, so + timing/layout dependent. diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/progress.md b/planning/archive/2026-10-issue-99-read-failures-after-open/progress.md new file mode 100644 index 0000000..b2e14f6 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/progress.md @@ -0,0 +1,33 @@ +# Progress — gdalcubes read failures after open, mask-open failures and worker crashes are still silent (#99) + +## Session 2026-10-14 + +- Plan-mode exploration (started 2026-10-02): phases and three gate decisions approved by user +- Created branch `99-gdalcubes-read-failures-after-open-mask` off main +- Scaffolded PWF baseline from issue #99 with approved phases +- Next: start Phase 1 (gdalcubes fork fix) +- Plan review (Plan agent) returned: 3 blockers (1 already handled, 2 adopted), gaps folded + into the fork patch; written to `review-plan.md` +- Phase 1: fork cloned, branch `99-chunk-status-read-failures` off upstream `ed68331`; patch + + tinytest (30 assertions, 10 fail on 0.7.5, all pass patched); 157/157 suite +- Phase 2: hang reproduced only for a worker killed at startup; mechanism = log-printing loop; + fixed + tested on the fork branch (fork commits 83eea4c..4175ee1, not yet pushed) +- Phase 3: `stac_features_masked()` wired into `stac_cube_items()`; mutation (unwired) -> 5 fails +- Fork `/code-check` round 1 running +- Fork code-check round 1: 2 findings, both fixed (one inside the G1 fix); round 2 spawned +- Phase 4: fixture writes DEFLATE COGs, can corrupt or delete B04 or SCL; lazy memoised probe at + parallel 1 and 2 with a forced override; gated tests + ungated production-shape mask-open test + + composite SCL test; abort text, roxygen and gotchas notes updated +- Fork code-check round 2: 1 pre-existing finding (warp_complex overview transformer type), out + of scope, in the upstream draft. Round 3: mechanism + 39-row enumeration; every branch-introduced + row consistent -> loop ended by enumeration. Fork `master` synced with upstream. +- Drift code-check round 1: 3 findings fixed — probe cross-checked against an independent SCL read + (mutant probe -> fails), `drift_no_mask_items` subclass with its own composite skip reason, + `tif_corrupt_tile()` asserts classic TIFF and one tile +- Drift code-check round 2: 1 finding (message test matched fixed text, not the id) fixed; no defect inside round-1 fixes; reviewer's mutations (probe FALSE, handler order) went red +- Drift code-check round 3: mechanism (checks written from the same belief they test) + a walk of + every new assertion/guard with mutations; 2 rows passed with their defect: the probe's + parallel 1-vs-2 comparison (workers run at every parallel, so it cannot fire) — removed; the + ungated deleted-SCL test used the helper's copy of drift's shape — added a real-path composite + test, which fails under M9 (select_bands() in stac_cube_assemble()). Loop ended by that + enumeration. diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round1.md b/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round1.md new file mode 100644 index 0000000..64452f3 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round1.md @@ -0,0 +1,67 @@ +# Review: drift #99, round 1 + +Branch `99-gdalcubes-read-failures-after-open-mask`, diff vs merge base with origin/main (planning/ excluded). + +## What was run + +- `NOT_CRAN=true devtools::test(filter = "dft_stac_(cube|composite|fetch)")` on installed gdalcubes 0.7.5: + `FAIL 0 | SKIP 13 | PASS 537`. The three new gated tests skip with the drift#99 reason; the #87 + tests (including `"9 of 279"`) pass on the COG fixtures. +- Same with `R_LIBS=/lib` (patched gdalcubes): `FAIL 0 | SKIP 10 | PASS 543`. The gated + tests run and pass. +- `DRIFT_TEST_ASSUME_READ_STATUS=true` on 0.7.5 (cube + composite): `FAIL 6`. That is the band-corrupt + test x2 parallel, the SCL-corrupt test x2, and the composite SCL test (expect_error + cache-empty). + So the gated tests fail without the fix and are not vacuous. +- The cli drop message renders correctly: `Dropped 1 item ... : "b".` and `Dropped 3 items ... : "a", + "b", and "c".` The `{?s}` binds to `{n}`, and the single-backslash continuation renders as one line. +- The roxygen claim that a deleted SCL is recorded on 0.7.5 in drift's read shape was checked for the + count path too. `reduce_time(cube, "count(B04)")` with no `select_bands()` gives 9 of 9 chunks failed + at parallel 1 and 4, so the claim holds for all three of drift's pixel functions. +- is_pre alignment and the re-sign source are both correct. The drop runs before `is_pre`, and + `items$features` is reassigned, so `fetched$items` carries the filtered set. The new + `stac_cube_items` test would fail if the drop moved after `is_pre`. +- Probe scoping is correct. The tempdir is deferred to the probe closure's frame and removed after the + answer is computed. A forced env value is returned before the memo, so it never leaks into the + cached answer. `withr::local_envvar` restores the variable at the end of the test. + +## Findings + +- **[fragile]** tests/testthat/test-dft_stac_cube.R:1213-1220 — The test is titled "the capability + probe agrees with the installed gdalcubes's own record", but it only checks the + `DRIFT_TEST_ASSUME_READ_STATUS` override. The real probe path (no env var, the corrupted-band write at + parallel 1 and 2) has no test. + - The gate fails toward pass. If the probe ever returns FALSE on a patched gdalcubes, all three + gated tests SKIP silently and the run stays green. That could happen if the corrupted fixture stops + failing under a later GDAL, or if the probe's fixture drifts from the tests' fixtures. + - The premise test (`:1202`) only partly covers this: it shows terra cannot decode the tile, not that + gdalcubes records it. + - Related: helper-gdalcubes.R:289 accepts only the exact string `"true"`. `TRUE` or `1` read as FALSE + and skip the tests, which is the opposite of what someone forcing them wants. + - Either retitle the test to what it checks, or have it assert that the probe agrees with an + independent read of the same fixture. Normalise the env value with `tolower()` and reject anything + other than true or false. + +- **[fragile]** R/dft_stac_cube.R:528-533 together with R/dft_stac_composite.R:316 — An all-dropped + listing raises `drift_no_items`. `dft_stac_composite()` catches that class and turns it into + `composite_skip(label, "no scenes")`, which throws away the new message ("N STAC items found ..., but + none has the mask asset 'SCL'"). + - A catalogue or config that systematically lacks the mask asset therefore drops every year as + "no scenes". It ends in "No year produced a composite ... try more `months` or a higher + `cloud_cover_max`", which is a remedy that cannot work. + - Nothing is cached and no wrong pixels result, so this is a misdiagnosis rather than data loss. The + per-item `cli_inform` drop lines do still print beforehand. + - Fix with a subclass (e.g. `c("drift_no_mask_items", "drift_no_items")`) that the composite lets + through or reports with its own reason, or have `composite_skip` use `conditionMessage(e)`. + +- **[fragile, latent]** tests/testthat/helper-gdalcubes.R:233-250 — `tif_corrupt_tile()` treats the + TileOffsets value field as the first tile's offset. That is true only while the tag's count is 1, + which means one tile. + - All current fixtures are single-tile: 40x40 px, and the composite AOI is about 326x313 px, under the + COG's 512 block. + - A larger `ext` would make the value a pointer to the offsets array, so the 64 bytes would overwrite + offsets rather than tile data. The `stopifnot` would not notice. + - The function also checks `"II"` but not the classic-TIFF magic (42 vs BigTIFF 43). + - Asserting count == 1 (read the 4-byte count instead of skipping it) and magic == 42 keeps the + fixture's meaning fixed. + +No bugs or security issues found in the R/ changes themselves. diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round2.md b/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round2.md new file mode 100644 index 0000000..ce3451c --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round2.md @@ -0,0 +1,53 @@ +# Review: drift #99, round 2 + +Branch `99-gdalcubes-read-failures-after-open-mask`, diff vs merge base with origin/main (planning/ excluded), +with most attention on the round-1 fixes in a26b292. + +## What was run + +- `NOT_CRAN=true devtools::test(filter = "dft_stac_(cube|composite)")`: + - On installed gdalcubes 0.7.5: `FAIL 0 | SKIP 9 | PASS 381`. The three gated tests skip with the drift#99 reason. + - With `R_LIBS=/lib` (patched build): `FAIL 0 | SKIP 6 | PASS 387`. The gated tests run. +- Mutation 1 was run in a copy of the repo against the patched build. The probe was made to answer FALSE + (`answer <<- FALSE`). The ungated cross-check test failed (`test-dft_stac_cube.R:1228`), so a probe that wrongly + says FALSE no longer turns into a silent skip. +- Mutation 2 was run in a copy of the repo on 0.7.5. The `drift_no_items` handler was put before + `drift_no_mask_items` in `dft_stac_composite()`. The new composite test failed on both its warning assertions + (`:183`, `:184`), so it exercises the real handler. + +## Round-1 fixes: checked, no defect + +- **tryCatch ordering.** Within one `tryCatch`, the first handler listed whose class the condition inherits wins. + So `drift_no_mask_items` listed first is correct, and mutation 2 confirms it. +- **Callers and catchers.** There are only two callers of `stac_cube_items()`. `dft_stac_cube()` does not catch the + class, so the new message propagates. `dft_stac_composite()` has the new handler. Nothing else in R/, data-raw/ or + the vignettes catches `drift_no_items`. `dft_stac_fetch()` has its own plain `stop()`. +- **The cross-check cannot pass vacuously in the direction that matters.** + - On 0.7.5 both sides are FALSE, which is the correct answer. + - On the patched build both are TRUE. + - For both sides to be FALSE on a fixed gdalcubes, the corruption would have to stop failing to decode. That is + ruled out: 0xAB as a zlib CMF byte has CM=11 (not 8), so every zlib rejects it, and the ungated premise test + checks it through terra. +- **Memoisation.** A forced value returns before the memo is read, so the memo only ever holds a real answer. + `withr::local_envvar(X = NA)` unsets the variable. The loop of `local_envvar` calls in the forced-answer test + restores LIFO to the original state. +- **Missing `chunk_status`.** On a gdalcubes without `chunk_status`, the probe gets `NA` and errors at + `if (seen[1] != seen[2])`. That fails loudly (an error, not a skip), and production aborts on the same case. + Acceptable. +- **`tif_corrupt_tile()` asserts.** The new asserts on magic 42 and on the TileOffsets count are correct for + GDAL's LONG-typed field. Every fixture is a single 512-block tile. + +## Findings + +- **[fragile]** tests/testthat/test-dft_stac_cube.R:1146-1148: the assertion + `expect_message(..., "1 item.*SCL.*b")` cannot check that the dropped item's id is named. + - The trailing `.*b` is already satisfied by the `b` in "gdal**c**u**b**es" in the fixed text of the message. + - Measured: `grepl("1 item.*SCL.*b", 'Dropped 1 item with no "SCL" mask asset, which gdalcubes would read unmasked: .')` + is TRUE with no id at all. + - So the part of the message the user needs most (which items were dropped) has no test. The drop itself is + pinned by the `expect_equal` on the ids. + - A distinctive id (e.g. `"no_scl_item"`) would make the assertion real. + +No bugs or security issues found in the R/ changes. + +/Users/airvine/Projects/repo/drift/planning/active/review-drift-round2.md diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round3.md b/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round3.md new file mode 100644 index 0000000..6dcc175 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/review-drift-round3.md @@ -0,0 +1,78 @@ +# Code-check round 3: drift changes for #99 + +Reviewed at e975940 (`99-gdalcubes-read-failures-after-open-mask`), diff against the merge base with origin/main, planning/ excluded. Both suites green at HEAD: unpatched 0.7.5 `[ FAIL 0 | SKIP 9 | PASS 381 ]`, patched (`R_LIBS=/lib`) `[ FAIL 0 | SKIP 6 | PASS 387 ]`. Every mutation below ran in a `cp -r` copy under `scratchpad/r3/`. The repo was not edited. + +## Mechanism + +All four earlier findings share one shape. Each check encodes a belief about something outside the code: how gdalcubes runs, how a TIFF is laid out, which handler a condition reaches, what a message's fixed text contains. The check was then written from that same belief, so it agrees with itself and nothing makes it face the case where the belief is wrong: + +- the probe's FALSE was never compared with a read the probe did not perform; +- the "no scenes" routing was never fed the new class; +- `tif_corrupt_tile()` assumed its input's layout instead of checking it; +- the regex `.*b` was never run against a message that lacked the id. + +The fix in every case was to make the check answer to something independent of the belief. That same mechanism reaches two more places in this diff: + +1. **`read_failures_recorded()` assumes `parallel = 1` reads in-process.** Its comparison of parallel 1 with parallel 2 is built on that assumption. gdalcubes' `.onLoad` and `gdalcubes_options()` both call `gc_set_process_execution()`, so every read at every `parallel` value, 1 included, runs in Rscript workers. The comparison therefore always measures the same library. +2. **The ungated "drift's read shape" test assumes `production_pixel` is drift's read shape.** It is the helper's copy of that shape. The test reads the copy, not `stac_cube_assemble()`, so it cannot see drift moving away from the shape. + +## Assertions and guards (table) + +How each check was tested: + +- **M1 to M9:** a mutant run through `devtools::test(filter = ...)` in a temp copy. +- **P:** a direct probe script. +- **R:** reasoning only, for a check whose defect is unreachable or trivially covered. + +| location | guards against | passes with the defect? | evidence | +|---|---|---|---| +| R/dft_stac_cube.R:570 `is.null(mask_asset) \|\| length==0` | NULL mask role / empty list erroring in `[[` | no | test :1147-1148; `list()[[NULL]]` errors without the guard (R) | +| R/dft_stac_cube.R:571 `[[mask_asset]]` exact | partial match of `SCL_20m` | no | M8 (`exact = FALSE`): :1154, :1155 FAIL | +| R/dft_stac_cube.R:572 `all(has_mask)` early return | a spurious message or copy when nothing is dropped | no | :1141-1142 (`expect_no_message`, `expect_identical`) | +| R/dft_stac_cube.R:530 drop placed before `is_pre` | `is_pre` misaligned with the read features | no | M3 (mask after `is_pre`): :1184 FAIL (plus :1191/1193/1194) | +| R/dft_stac_cube.R:531 all-dropped abort + `drift_no_mask_items` class | an all-no-mask listing reported as "no scenes" | no | M2 (class `drift_no_items` only): :1193 ERROR | +| R/dft_stac_composite.R:317 handler ordered before `drift_no_items` | the subclass caught by the "no scenes" handler | no | M1 (order swapped): composite :183 and :184 FAIL | +| test-composite :170 mocked condition class | stac_cube_items' real class drifting from the composite's handler | **yes on its own** (it builds its own class vector) | the routing is covered by cube :1193 through the real function (M2), so the pair holds. Not a finding | +| test-composite :173 `"the warnings say why"` | the old abort text | no | regex matches only the new text (R) | +| test-composite :183 / :184 | the wrong skip reason | no | M1 | +| test-cube :1133 regex `1 item.*SCL.*no_scl_item` | the id missing from the message | no | round-2 fix; the id is distinctive (R) | +| test-cube :1136, :1183, :1186 | the wrong features kept, or the items/features split diverging | no | M3; a copy-only mask would fail :1186 (R) | +| test-cube :1194 `"mask asset"` | the old "No STAC items" text | no | fixed text that only the new message has (R) | +| helper :53 `unlink(f)` (delete mode) | (no guard) a typo'd `break_asset`/date leaves the fixture clean, silently | would pass silently, **but** every delete-mode consumer expects an abort, so a clean fixture fails loud | R. Not a finding | +| helper :67 `stopifnot` `II` + 42 | BigTIFF / big-endian layout | no | P: a BigTIFF COG stops at the 42 check | +| helper :82 `stopifnot(count == 1L)` | a multi-tile file (offset array) | no | P: a 600x40 COG stops | +| helper :86 `stopifnot(!is.na(tile))` | a striped TIFF with no TileOffsets | no | P: a striped GTiff stops | +| helper :133-135 env override + validation | a typo'd override silently ignored | no | :1236-1244 (`"yes"` errors) | +| **helper :143 `seen[1] != seen[2]`** | **the session and workers loading different gdalcubes** | **yes: it cannot fire** | **P: patched session via `.libPaths()` with unpatched workers gives `FALSE` and no error (parallel 1 and 2 both report 0 of 279 failed); unpatched session with `R_LIBS=patched` workers gives `TRUE` and no error** | +| helper :155 `skip_if_no_read_failure_status()` | running gated tests on 0.7.5 | no | unpatched: :1248, :1274, composite :774 SKIP; patched: they run and pass | +| test-cube :1213 `file.exists(f)` | the `fixture_dir` attribute pointing elsewhere | no | R | +| test-cube :1215 premise `expect_error(values)` | the corruption not breaking the decode | no | M4 (write at tile+100000): :1215 FAIL | +| test-cube :1229 probe cross-check | the probe wrongly FALSE on a fixed gdalcubes | no (on the patched build) | M5 (probe view ends 07-10, before the broken scene): :1229 FAIL under the patched build. On 0.7.5 both sides are FALSE by design (accepted) | +| test-cube :1237/:1241/:1244 forced answer | the override broken | no | R; direct | +| test-cube :1253 band read-after-open | gdalcubes / `cube_check_chunks` missing it | no (patched); SKIP on 0.7.5, accepted | patched run | +| **test-cube :1267 "mask image that fails to open … in drift's read shape" (ungated)** | **drift's reads failing to record a deleted SCL on 0.7.5** | **yes** | **M9: added `select_bands(cube, band_assets)` before `pixel_fn` in `stac_cube_assemble()`. The suite stays `[ FAIL 0 \| PASS 256 ]` on 0.7.5. The same mutant through drift's real path (`composite_on_fixture(list("2021-07-13"), break_asset = "SCL", break_mode = "delete")`) FAILS: no `drift_incomplete_cube`, the scene is dropped silently. That real-path check passes at HEAD** | +| test-cube :1280 mask read-after-open | gdalcubes / `cube_check_chunks` missing it | no (patched); SKIP on 0.7.5 | patched run | +| test-cube :1289 clean control `expect_silent` | a false refusal | no | an abort would fail it (R) | +| test-composite :776 / :781 corrupt SCL, real path | a holed read cached | no (patched); SKIP on 0.7.5 | patched run; caching before the abort fails :781 (R) | + +Also measured: the count path (`reduce_time("count(B04)")`) records a deleted SCL on 0.7.5, 9 of 279 chunks at parallel 1 and 4, so the roxygen claim also holds there. The composite fixture extent is 331 x 318 px, a single 512 tile, so the composite gated test is within `tif_corrupt_tile()`'s supported layout. + +## Findings + +### 1. `read_failures_recorded()`: the session-vs-worker guard can never fire (tests/testthat/helper-gdalcubes.R:124-147) + +The comment says the probe runs "at parallel 1 and 2: worker processes load gdalcubes from R_LIBS, not from this session's .libPaths(), so the two can disagree, and that is an error rather than a skip". That rests on `parallel = 1` reading in-process, and it does not. gdalcubes calls `gc_set_process_execution(.pkgenv$parallel, ...)` in `.onLoad` with `parallel = 1` and again in every `gdalcubes_options()`, so both writes run in Rscript workers. + +Measured with a patched session loaded through `.libPaths()` and no `R_LIBS`: `getNamespaceInfo("gdalcubes", "path")` is the patched lib, yet parallel 1 and parallel 2 both give `failed 0 / 279`, and the probe returns `FALSE` with no error. The reverse configuration (unpatched session, `R_LIBS` patched) returns `TRUE`, again with no error. The `stop()` at :143 is unreachable. + +**Impact.** It is benign today, because drift's own reads also run in those workers, so the probe still reports the gdalcubes that actually does drift's reading. The cost is a promised protection that does not exist, a comment that misstates gdalcubes' execution model, and a wasted second fixture write. + +**Fix.** Drop the parallel-1 write and the `seen` comparison. Say instead that every read runs in R_LIBS-loaded workers, so the probe measures the library drift's reads use. If a session/worker mismatch should be an error, compare something that actually differs: `find.package("gdalcubes")` in the session against one resolved in a subprocess carrying the same environment the workers get. + +### 2. The ungated "drift's read shape" test reads the helper's copy of the shape, not drift's (tests/testthat/test-dft_stac_cube.R:1257-1269; helper :174) + +The test, the `cube_check_chunks()` roxygen ("A deleted mask (SCL) image is caught the same way in drift's reads, because gdalcubes also reads the mask asset as a band there") and the notes table row all rest on drift never calling `select_bands()` before `pixel_fn`. The test pins that through `production_pixel`, a helper reconstruction, so it cannot see drift's real path change. + +Measured (M9): inserting `gdalcubes::select_bands(cube, band_assets)` before `pixel_fn` in `stac_cube_assemble()` leaves the whole cube suite green on 0.7.5. Through drift's real path, the same deleted-SCL fixture then aborts nothing: the scene is dropped silently, the 0.7.5 behaviour the notes record for that shape. + +**Fix.** Pin it through the real path instead of, or as well as, the helper shape. An ungated `composite_on_fixture(list("2021-07-13"), cache = cache, break_asset = "SCL", break_mode = "delete")` expecting `drift_incomplete_cube` plus an empty cache passes at HEAD on 0.7.5 and fails under M9. diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round1.md b/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round1.md new file mode 100644 index 0000000..fd5b98e --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round1.md @@ -0,0 +1,86 @@ +# Review: gdalcubes branch `99-chunk-status-read-failures`, round 1 + +Reviewed: `git diff upstream/master...HEAD` (5 commits, 83eea4c..4175ee1). All changed files were read in full where touched. +Runs: patched lib (scratch `lib`) and unpatched 0.7.5 (default user lib, plus a tinytest-only lib of my own). + +## Findings + +- **[bug] src/gdalcubes/src/warp.cpp: the new `in == NULL` guard is dead code. The reopen fallback still segfaults (warp_simple, and warp_complex the same way).** + The fallback sets `in = GDALOpen(descr)` and only warns when that returns NULL. The next + use is `psWarpOptions->nBandCount = in->GetRasterCount();`, about 50 lines further down, + and it dereferences NULL before `bool warp_failed = (in == NULL);` is ever reached. + So the path the guard was written for still crashes the worker. + + The path is reachable. A mask band in a multi-band image with overviews gives a mask + `bandsel_vrt` made with `GDALTranslate("", ...)`. Its description is `""`, so both the + `OVERVIEW_LEVEL` open and the full-resolution reopen of `""` fail. + + Reproduced on the patched build: a 2-band 600x600 px COG with overviews, mask = band 2, + and a 40 m view. The worker log shows `Failed to open GDAL overview dataset for ''`, then + the new `GDAL could not reopen ''`, then `*** caught segfault *** address 0x70`. + `write_ncdf()` stops with "one or more worker processes failed". Unpatched 0.7.5 + segfaults the same way, so the branch does not make this worse. But the guard reads as + handling the case and does not, and the mask read is one of the failures the branch is + meant to record. + + Fix: before any use of `in` in the else-branch, clean up and `return nullptr`. That means + destroying the transform, the warp options and `wkt_out`, and `GDALClose(out)`. Better + still, open the overview dataset *before* closing the original, and close the original + only when that open succeeds. Then no reopen is needed and the `""`-description VRT keeps + working at full resolution instead of becoming INCOMPLETE on every chunk. The probe + script is `scratchpad/rv1/ovprobe.R`. + +- **[fragile] inst/tinytest/test_chunk_status.R: the "reprojected read is OK" control reads no pixels, so it passes without testing anything.** + The fixture is EPSG:32610 x/y 0..400. In EPSG:32609 that sits at x ≈ 668,185..668,584, + y ≈ 0..399. The view extent `c(-4500000, -4499000, 0, 1000)` does not overlap it. + Measured: 0 of 20,000 values are non-NA, so no reprojecting warp ever runs. + + It cannot catch a spurious `ChunkAndWarpImage` failure under reprojection, and that + false positive is the main risk of the warp change. With `extent = c(668000, 669000, -300, 700)`: + - the clean case reads 1,600 values and every chunk is OK, so the code itself is fine; + - `band_read` under reprojection gives status 2 on the affected chunks. + + Fix: use an overlapping extent, and add `any(!is.na(r$values))` to this control as the + first control already has. + +## Checked and fine + +- Ownership on every warp path: `in` is closed exactly once inside `warp()` on success and + on failure. `out` is closed only on failure and returned otherwise. The transform and the + warp options are destroyed before return. No caller closes `g` or `bandsel_vrt` after + `warp()`, so there is no double close. In warp_complex, `tmparg` is restored before + `GDALDestroyGenImgProjTransformer` on the failure path too. In the overview-reopen + fallback, the transformer built from the old `in` holds copied geotransform values (or a + GenImgProj info that is still valid), not the closed handle. That is an improvement on + upstream, where `hSrcDS` pointed at a closed dataset. +- `in->GetDescription()` in the new WARN runs only when `!warp_failed`, so `in` is non-NULL there. +- `read_ncdf`: + - `buf(nullptr)` frees the buffer because `size()` is still non-zero at that point, and + `size({0,0,0,0})` then makes the chunk `empty()`. + - Every early return closes the file. The branch adds the missing close on the empty-chunk return. + - Non-OK chunks are written by workers even when they are empty or all NaN + (`chunk_data::write_ncdf` skips only OK + all-NaN), so ERROR and INCOMPLETE status + reaches the main process. +- multiprocess log loop: a missing file fails the first `getline`. A final line with no + newline is still printed because `getline` sets eof, not fail. +- Restore-the-bug, measured: + - test_chunk_status.R: 30/30 pass on the patched build and 10/30 fail on unpatched 0.7.5. + Those 10 are every status assertion for band_read, mask_open and mask_read, the + mask_read 900 check and the strict check, at both parallel settings. + - test_worker_failure.R: 2/2 pass in about 1 s on the patched build. The child prints + `worker process #1 returned 9` and then `RESULT raised`. On unpatched 0.7.5 both + assertions fail on the 120 s timeout (status 124, about 140 s wall). Afterwards no + orphan main or worker R processes were left. +- The netCDF fill (-2147483647, NC_INT) is mapped to OK only for chunks that were never + written. The clean controls would fail if the mapping were wrong. The overview control + really does use an overview: the 600 px COG has 300 and 150 px overviews. +- The `sf`-absent skip of the "file still opens" check only weakens the check that + band_read is not an open failure. In this environment the status assertions fail + pre-branch, so they are not vacuous here. + +## Noted, not findings + +- Now that `warp()` can return nullptr, the `!gdal_out` `continue` in read_chunk's band loop + is reachable, and it skips `filesystem::remove(bandsel_vrt_name)`. Each failed warp of a + multi-band image leaks a few KB of `/vsimem` VRT. This is the same class as the accepted + out-of-scope RasterIO-failure leak, and small. diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round2.md b/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round2.md new file mode 100644 index 0000000..bbd0b85 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round2.md @@ -0,0 +1,72 @@ +# Review round 2: gdalcubes `99-chunk-status-read-failures` (HEAD 7afdd3e vs upstream/master ed68331) + +## Findings + +- **[severity: bug, pre-existing, not a regression]** src/gdalcubes/src/warp.cpp:574-580 (`warp_complex`, overview-success branch) + When the overview opens, this branch treats `psWarpOptions->pTransformerArg` as a + `gdalcubes_transform_info`, but in `warp_complex` it is a `GDALGenImgProjTransformInfo` + (from `GDALCreateGenImgProjTransformer2`, line ~525). Three things go wrong: + 1. `destroy_transform()` `delete`s the GDAL struct through the wrong type. + 2. `create_transform()` installs a `gdalcubes_transform_info` while `pfnTransformer` is still + `GDALGenImgProjTransform`. + 3. At the end, line ~683 casts that struct back to `GDALGenImgProjTransformInfo`, writes + `pReprojectArg = tmparg` into it and calls `GDALDestroyGenImgProjTransformer()` on it. + + Result: heap corruption or a segfault every time a GCP- or geolocation-georeferenced image + (no geotransform, for example Sentinel-1 GRD COGs) has overviews and `use_overview_images` + is TRUE (the default). + + Reproduced on both builds. The image is a GCP-only GTiff (4 GCPs, EPSG:32609), read through + a `stac_image_collection(url_fun = identity)`, with an 80 m view over the 10 m image: + - With `gdal_addo` overviews, the worker dies with `caught segfault` / `worker process #0 + returned 11`, on HEAD 7afdd3e and on unpatched 0.7.5 alike. + - Without overviews, the same image returns status 0 and values 7 on both builds. + + So the overview-first change in 7afdd3e fixes the overview-open **failure** path in both + functions. In `warp_complex` it keeps the wrong transformer on the **success** path, which + is the only path that still differs from `warp_simple`. That is the answer to "does the + overview-first change leave the transformer wrong on any path". + + Fix options: + - On success, set `psInfo->pReprojectArg = tmparg`, call + `GDALDestroyGenImgProjTransformer(psInfo)`, rebuild the transformer with + `GDALCreateGenImgProjTransformer2(in, out, trnsfrm_opts.List())`, save the new `tmparg`, + and re-apply the cache override. + - Or skip overview selection in `warp_complex` altogether. + + Out of scope for #99 if you prefer, but it sits inside the block this branch rewrote, so it + is worth an upstream issue at least. Repro script: + `/private/tmp/claude-501/-Users-airvine-Projects-repo-drift/385a1665-8b28-46b0-9826-7e0ccfdfac92/scratchpad/r2/gcp2.R` + (args: `1 ov` or `1 noov`). + +## Checked and found correct + +- **`warp_simple`, overview failure** (for example the mask band-select VRT, whose description + is `""`): `in`, `hSrcDS` and the transformer all still refer to the original dataset, and the + original is closed exactly once at the end. +- **`warp_simple`, overview success:** the original is closed once (as on upstream). `in_ov` + becomes `in`, with a matching transformer and `hSrcDS`, and is closed once at the end. +- **Caller-created VRT ownership** is unchanged from upstream: `warp()` owns and closes its + input on every path. The /vsimem band-select VRT (band path) still opens its overview while + the original is open. A 3-band COG with one band selected at 80 m gave status 0 and values 5, + with no "Failed to open GDAL overview" warning, on both builds. +- **`warp_failed`:** + - It is set only on an `Initialize` or `ChunkAndWarpImage` failure. + - `ChunkAndWarpImage` is skipped after an `Initialize` failure. + - `in` is non-null where its description is logged. + - `out` is closed before `nullptr` is returned. + - Every `warp()` caller (`image_collection_cube.cpp` lines 500/505 and 614/619) already + handles `nullptr` (INCOMPLETE, or ERROR when strict). + - Clean controls stay OK, including `warp_complex` without overviews (status 0 above). +- **`read_ncdf`:** the new ERROR paths free the buffer correctly. `buf(nullptr)` frees while + size is still set, and size is then zeroed. The merge lambda writes the status for an empty + chunk. +- **`multiprocess.cpp`:** the `getline` loop terminates on a missing file and still prints a + final line that has no trailing newline. +- **Tests:** `test_chunk_status.R` passes 32/32 and `test_worker_failure.R` passes 2/2 against + the patched library (R_LIBS = scratch lib). + +## Verdict + +No regression introduced by the branch. One pre-existing segfault remains in the code that +7afdd3e touched, on the `warp_complex` overview-success path. diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round3.md b/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round3.md new file mode 100644 index 0000000..13501d8 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/review-fork-round3.md @@ -0,0 +1,149 @@ +# Review round 3: gdalcubes fork `99-chunk-status-read-failures` (HEAD 7afdd3e vs upstream/master ed68331) + +Reviewer: subagent, round 3. Read in full: warp.cpp (warp, warp_simple, create_transform, +destroy_transform, warp_complex), image_collection_cube.cpp read_chunk, cube.cpp +chunk_data::write_ncdf / read_ncdf and the write_ncdf merge callback, cube.h chunk_data, +multiprocess.cpp apply/exec, tiny-process `try_get_exit_status`, both new tinytest files. + +## Mechanism + +Every finding so far comes from one assumption: **a handle and the state that refers to it +change together on every exit path.** The handle is `in`, the overview handle, `out`, the +transformer arg, a chunk buffer or the netCDF id. The state is `psWarpOptions->hSrcDS`, +`pTransformerArg`, `_size`, `chunk_status`, or what the caller assumes it still owns. The +plan-review bug: `in` was released before its replacement existed, so `hSrcDS` pointed at a +closed dataset. The round-1 bug, inside the fix for that: `in` was rebound to NULL and then +read (`GetRasterCount`) before the guard. The round-2 note: the transformer was swapped for +one of a different *type*, but the release code still assumed the original type. The tell +in each case is a reassignment or release that the code path after it does not see. + +## Enumeration (table) + +Every exit path, and every reassignment or release of a tracked object, in the functions the +diff touches. "Branch?" says whether the row is new or newly reachable on this branch. + +### warp_simple + +| # | path | what is held | released / set | consistent? | branch? | +|---|------|--------------|----------------|-------------|---------| +| S1 | MEM driver missing → `throw` | `in`, `wkt_out` | nothing | no (leaks) | pre-existing | +| S2 | `create_transform` throws (non-invertible geotransform), first call | `in`, `out`, `psWarpOptions`, `wkt_out` | nothing | no (leaks) | pre-existing | +| S3 | overview: `in_ov = GDALOpenEx(descr)` succeeds | `in`, `in_ov`, transformer(in) | `GDALClose(in)`; `in = in_ov`; transformer destroyed and rebuilt from `in_ov`; `hSrcDS = in` | yes: all three refer to `in_ov` | rewritten | +| S4 | overview: `in_ov == NULL` (e.g. mask VRT with description `""`) | `in`, transformer(in) | nothing changes; WARN | yes: `in`, `hSrcDS` and the transformer still match. Measured: mask path warns 18x, output correct | new (was a NULL deref) | +| S5 | S3, then `create_transform(in_ov)` throws | `in_ov` (as `in`), `out`, options holding a destroyed transformer | nothing | no (leaks; dangling arg never used because of the throw) | same order as before, pre-existing | +| S6 | `oo` | CSL list | `CSLDestroy(oo)` after either branch | yes | rewritten | +| S7 | `x`, `y`, `succ` | malloc buffers | freed after the overview block | yes | unchanged | +| S8 | nodata count mismatch | `src_nodata*` (CPLMalloc) | `std::free`, never assigned to the options | yes (CPLMalloc/free mix, pre-existing) | unchanged | +| S9 | `Initialize` fails | options, transformer, `wkt_out`, `in`, `out` | transformer, options, `wkt_out` freed; `in` closed; `out` closed; returns nullptr | yes | new | +| S10 | `ChunkAndWarpImage` fails | as S9 | WARN reads `in->GetDescription()` *before* `in` is closed; then as S9 | yes | new | +| S11 | success | as S9 | transformer, options, `wkt_out` freed; `in` closed; `out` returned | yes (ownership of `in` passes to warp, as upstream) | unchanged | +| S12 | `oOperation` destructor runs after `in`/`out` are closed (S9/S10: `out` too) | its own cloned options | WipeOptions / thread teardown, no dataset access | yes | `out` closed before dtor is new; harmless | + +### warp_complex + +| # | path | what is held | released / set | consistent? | branch? | +|---|------|--------------|----------------|-------------|---------| +| C1 | MEM missing → throw | `in`, `wkt_out` | nothing | no | pre-existing | +| C2 | `psInfo == NULL` → throw | `in`, `out`, options, `wkt_out` | nothing | no | pre-existing | +| C3 | overview `in_ov` succeeds | `in`, `in_ov`, GenImgProj `psInfo` with a cached reproj arg swapped in | `in` closed, `in = in_ov`; `psInfo` freed via `destroy_transform` (wrong type); a `gdalcubes_transform_info` is installed, but `pfnTransformer` is still `GDALGenImgProjTransform`, and the end-of-function `static_cast` writes through it | no | pre-existing (round 2); reachability unchanged, since the open succeeds under the same conditions as before | +| C4 | overview `in_ov == NULL` | `in`, `psInfo` | nothing changes; WARN | yes: `psInfo` was built from `in` and is restored and destroyed normally | new (was a NULL deref) | +| C5 | `oo`, `x/y/succ` | as S6/S7 | as S6/S7 | yes | rewritten | +| C6 | `Initialize` fails | `psInfo`, options, `wkt_out`, `in`, `out` | `pReprojectArg = tmparg`; `GDALDestroyGenImgProjTransformer`; options and `wkt_out` freed; `in` and `out` closed; nullptr | yes (off the C3 path) | new | +| C7 | `ChunkAndWarpImage` fails | as C6 | WARN before `in` is closed; then as C6 | yes | new | +| C8 | success | as C6 | as C6, `out` returned | yes | unchanged | + +### image_collection_cube::read_chunk (paths that consume the changed warp contract or were edited) + +| # | path | what is held | released / set | consistent? | branch? | +|---|------|--------------|----------------|-------------|---------| +| R1 | band dataset: warp returns nullptr, strict | `img_buf`, `mask_buf`, `agg`, `out` (calloc buf), `g` when bandsel is used, the /vsimem VRT file | bufs freed, `agg` deleted; `out` replaced by an empty chunk with status ERROR, and the old chunk's dtor frees its buffer; warp already closed `in` | yes for buffers and status; `g`/vsimem leak is pre-existing | newly reachable | +| R2 | band dataset: warp nullptr, non-strict | same | INCOMPLETE; `continue` (the `for` over datasets); the band slice stays NaN; image still fed to `agg`, `count_success++` | status yes; see P5 for `count_success` | newly reachable | +| R3 | the caller's `g` after warp | the caller's pointer | warp closed it (or closed the VRT); neither branch touches `g` after warp | yes (same contract as upstream, measured) | unchanged | +| R4 | mask open fails, strict | `img_buf`, `mask_buf`, `agg`, `out` | freed; `out` = empty chunk with ERROR (was OK) | yes | new | +| R5 | mask open fails, non-strict | as R4; `g` NULL | INCOMPLETE; `continue` (the `while`, so the image is skipped and not counted) | yes; matches the mask-warp and mask-RasterIO paths | new | +| R6 | mask warp nullptr, strict / non-strict | as R4; mask `g` when bandsel is used | as R4 / R5 | yes (`g` leak pre-existing) | newly reachable | +| R7 | end: INCOMPLETE and `count_success == 0` → ERROR; all-NaN → empty chunk keeping its status | `out` | status carried across the replacement | yes | unchanged | + +### chunk_data::read_ncdf (multiprocess master) + +| # | path | what is held | released / set | consistent? | branch? | +|---|------|--------------|----------------|-------------|---------| +| N1 | `nc_open` fails | nothing | ERROR; return | yes | new | +| N2 | dimension id missing | `ncfile` | ERROR (overrides the status read from the file); `nc_close` | yes on NetCDF-4 builds; see note A | status is new | +| N3 | variable missing | `ncfile` | ERROR; `nc_close` | yes | status is new | +| N4 | any `nc_inq_dimlen` fails | `ncfile` | ERROR; `nc_close` | yes | new | +| N5 | zero-length dimension (empty non-OK chunk) | `ncfile` | `nc_close` (was leaked); status from the file attribute | yes; measured an empty INCOMPLETE chunk reading back as 2 under `parallel = 2` | close is new | +| N6 | `nc_get_var_double` fails | `ncfile`, `_buf` (malloc), `_size` set | `buf(nullptr)` frees (size still > 0), then size zeroed, ERROR, `nc_close` | yes; buffer and size agree, and `empty()` is true for the merge callback | new | +| N7 | success | `ncfile`, `_buf`, `_size` | `nc_close` | yes | unchanged | +| N8 | malloc returns NULL on a huge chunk | NULL `_buf` | passed to `nc_get_var_double` | no | pre-existing | + +### chunk_processor_multiprocess::apply, worker-log loop + +| # | path | what is held | released / set | consistent? | branch? | +|---|------|--------------|----------------|-------------|---------| +| L1 | log file missing | failed `ifstream` | `getline` is false at once; dtor closes | yes (was an infinite loop) | new | +| L2 | log present, last line has no newline | stream | `getline` returns the line, then false | yes (that line is still printed) | new | +| L3 | read error (badbit) | stream | `getline` false | yes | new | +| L4 | merge callback on an ERROR chunk from N1–N6 | empty `dat` | status written; data write skipped because `empty()`; file removed | yes | newly reachable | + +## Clean + +None of the rows the branch introduced or made reachable is inconsistent. Every +inconsistent row is pre-existing: S1, S2, S5, C1, C2, C3 and N8. + +What I checked: +- Both new tinytest files pass against the patched build: test_chunk_status.R 32/32, + test_worker_failure.R 2/2. +- **Band-selection VRT and overview** (a 2-band 600 px COG with overviews; B04 requested + alone, so it goes through `/vsimem/*.vrt`; SCL mask in band 1). On the patched build, + with and without the mask and at `parallel` 1 and 2: status 0 and values 500. The debug + log shows the "Failed to open GDAL overview dataset for ''" fallback 18 times, which is + exactly 9 chunks x 2 dates on the mask path only. The `/vsimem` band VRT still opens + its overview *before* the source is closed, because GDALTranslate flushes a named VRT. + So the new open-then-close order did not cost the band path its overviews (rows S3/S4). + The unpatched 0.7.5 build segfaults on the masked case (`address 0x70`, worker returned + 11): this is the bug the plan review found, and S4/C4 fix it. +- **Empty INCOMPLETE chunk through the multiprocess master** (rows N2/N5). One image is all + nodata, the other is deleted after the collection is built. The output is 0 non-NA + cells, and chunk_status reads 2 at both `parallel = 1` and `parallel = 2`, on both the + patched and unpatched builds. +- The test fixtures do reach their failure modes: the corrupted COG still opens; the + 6000 m / dx 40 case does select overview level 1; the killed worker leaves no log; and + under tiny-process a SIGKILL'd child of `sh -c` reports exit 137 > 0, so `any_failed` is set. + +## Pre-existing (not findings) + +- P1 (S1, S2, C1, C2, S5): leaks of `in`, `out`, options and `wkt_out` on throw paths in + warp_simple and warp_complex. +- P2 (C3): the warp_complex overview success path replaces a GenImgProj transformer with a + `gdalcubes_transform_info` without changing `pfnTransformer`, then + `static_cast` writes `pReprojectArg` into it. It also + `delete`s the original as the wrong type. Round 2, accepted, noted for upstream. +- P3: in the band path, when a band-selection VRT is used, `g` is never closed, and the + `/vsimem` VRT file is not removed on any `continue`. The mask path leaks `g` the same way. +- P4 (N8): a NULL malloc in read_ncdf is passed straight to `nc_get_var_double`. +- P5: in read_chunk, `count_success` counts images *processed*, not images read. A band + open, warp or RasterIO failure `continue`s only the inner `for`, so the image is still + counted. As a result `INCOMPLETE && count_success == 0 → ERROR` fires only for mask + failures. A chunk whose every band read failed is recorded INCOMPLETE, not ERROR. The + branch follows the existing shape (the upstream open-failure path is identical), so the + only effect is that warp failures inherit it. The tests assert `status != 0`, which is + why they cannot see it. +- P6: the src_nodata mismatch branch frees CPLMalloc memory with `std::free`. + +## Notes (behaviour changes, not defects) + +- A. N2 under classic-model netCDF: only `USE_NCDF4 == 0` builds are affected, which + `cube.cpp` sets only on Solaris. A classic file allows one unlimited dimension; I + measured `nc_def_dim` returning -54 for the second zero-length dimension. So an empty + non-OK chunk is written without `t`/`y`/`x`, and N2 now reads it back as ERROR instead + of its recorded INCOMPLETE. This does not reproduce on NetCDF-4 builds (measured above). +- B. `ChunkAndWarpImage` also returns `CE_Failure` when GDAL cannot compute a source window. + Two ways that happens: "Too many points failed to transform", where + `ERROR_OUT_IF_EMPTY_SOURCE_WINDOW` defaults to TRUE (the option is in the linked GDAL + 3.13); and gdalcubes' own `transform()` returning 0 when OGR reports a partial + reprojection failure. Before this branch, such an image silently contributed NaN with + status OK. Now it is INCOMPLETE, and under `incomplete_ok = FALSE` it is ERROR, which + discards the whole chunk, including the other images' data. This matches the PR's intent + (a dropped image is now reported), but the PR body could say it, since strict users near + a CRS domain edge would see chunks that used to succeed now fail. diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/review-plan.md b/planning/archive/2026-10-issue-99-read-failures-after-open/review-plan.md new file mode 100644 index 0000000..bf55136 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/review-plan.md @@ -0,0 +1,64 @@ +# Plan review — #99 (Plan agent, 2026-10-14) + +Read-only agent; findings returned as reply text and written here by the session. +Read against appelmar/gdalcubes@ed68331 via `gh api`. + +## Blocker +- **B1** `.libPaths()` inside R never reaches gdalcubes workers (spawned `Rscript` inherits + env, not the session lib path). Use `R_LIBS=/lib` exported in the shell; run + probes at parallel >= 2. — *Already the method used; the parallel-2 tinytest passed + patched / failed unpatched, which shows workers loaded the right build.* +- **B2** Dropping items in `stac_cube_assemble()` breaks the #87 integration tests (features + with no `assets`), `composite_offset_check()` runs on the undropped `is_pre`, the offset-split + message would be stale, and an all-dropped year would abort a multi-year composite. Do it + in `stac_cube_items()` after the month filter, as a pure helper, raising `drift_no_items` + when all are dropped. — *Adopted.* +- **B3** The upstream tinytest cannot depend on terra/withr. — *Adopted: fixture built with + `write_tif(COG = TRUE)` from dummy cubes, base `writeBin` for corruption.* + +## Answers +- Q1: `warp()` dispatches to `warp_simple` (`:73`, every geotransformed image, `ChunkAndWarpImage` + unchecked at `:256`) and `warp_complex` (`:436`, GCP/RPC only, DEBUG-logged at `:650`). All four + callers already handle nullptr. Cleanup on failure must keep transformer/tmparg restore, + `CPLFree(wkt_out)`, `GDALClose(in)` (warp owns `in`), then `GDALClose(out)`. Treat + `Initialize()` failure as failure too. — *Adopted.* +- Q2: aggregation (`agg->update`, `:657`) follows the mask block, so mask-path `continue`s drop + the scene (not unmasked); band-path `continue`s keep the image with NaN for that band. +- Q3: an INCOMPLETE all-NA chunk survives: the worker only skips `OK && all_nan()`, read_ncdf + sets status before dimension checks, and derived cubes propagate status. +- Q4: `fetched` comes from `stac_cube_items()`; re-signing is not a constraint. +- Q5: tinytest is right; upstream tests use dummy cubes, so none exercises `warp()`. + +## Gap +- **G1** Failed overview reopen (`warp.cpp:160-169`, `:553-562`) leaves `hSrcDS` on a closed + dataset. — *Adopted: reopen at full resolution, else fail.* +- **G2** `nc_inq_dimlen` unchecked; empty-chunk return leaks the handle; uninitialised buffer on + `nc_get_var_double` failure. — *Adopted.* +- **G3** Fork `master` is behind upstream; sync it and PR into `master`. — *Adopted.* +- **G4** Corruption recipe must be recorded (DEFLATE tile at a `TileOffsets` entry); assert + `failed >= 1`. — *Adopted.* +- **G5** Abort text and roxygen must cover the read-back cause. — *Phase 4.* +- **G6** Capability probe lazy and memoised. — *Phase 4.* + +## Ordering +- O1 open the fork PR after Phase 2. O2 mutation check by forcing the probe on against 0.7.5. + O3 Phase 3 can go first. — *All adopted.* + +## Assumption +- A1 a failed warp may not always be a failed read (source-window failures in exotic + reprojections). — *Clean controls added (other CRS, partial footprint, overviews).* +- A2 partial warps are now discarded — intended; say so in the PR. +- A3 the supervision loop does not explain the hang; sample the hung process. — *Done: the + cause is the log-printing loop (`!wlogf.eof()` on a missing file).* +- A4 a capability gate turns a future regression into SKIPs — accepted at the gate. + +## Scope +- S1 worker-side write failures (`nc_create`/`nc_put_vara`/`nc_close` unchecked) — left out, + named in the upstream draft. +- S2 pre-existing leaks on `continue` paths — out, mention in the PR. +- S3 items missing a band asset (not the mask) — out of scope. +- S4 the fix reaches the user's runs only once installed — note in the report. + +## Acceptance +- AC1 clean controls; AC2 cloud value visible in the output; AC3 strict branch — *all added to + the tinytest.* AC4 read_ncdf has no test — *say so in the PR.* diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/task_plan.md b/planning/archive/2026-10-issue-99-read-failures-after-open/task_plan.md new file mode 100644 index 0000000..d528a88 --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/task_plan.md @@ -0,0 +1,113 @@ +# Task: gdalcubes read failures after open, mask-open failures and worker crashes are still silent (#99) + +## Problem + +#87 made drift abort on a gdalcubes chunk whose **band image fails to open** (an expired or refused signed URL), by reading the per-chunk `chunk_status` gdalcubes writes into its NetCDF. That is the only failure gdalcubes records. It sets `chunk_status` in six places, all in `image_collection_cube::read_chunk()`, and only band `GDALOpen` returning NULL can fire in practice. **Every other I/O step between `GDALOpen` and the final `nc_close` drops its return code, or warns and continues, leaving the status OK**: `ChunkAndWarpImage`, every netCDF call between worker and main process, `CPLMoveFile`, `sqlite3_step`. + +A code-check enumeration of the write pipeline (#87 round 3: 38 failure paths, 25 detected by drift, 13 not; table in the #87 PR's `planning/archive/2026-10-issue-87-silent-chunk-read-failures/review-round3.md`) and offline probes on gdalcubes 0.7.5 (`appelmar/gdalcubes@ed68331`, `parallel` 1 and 4): + +| case | `chunk_status` | cells | +|---|---|---| +| **mask (SCL) image opens, then a tile read fails** | OK | **scene used UNMASKED**: the mask buffer stays NaN, which never matches a mask value, so cloud values reach the median (all-cloud scene: 1600 of 1600 cells set) | +| scene with no mask asset | OK | scene used unmasked | +| worker writes no chunk file (`nc_create` / `CPLMoveFile` fails; worker exits 0) | fill, no R warning | those cells `NA` | +| worker chunk file partial (missing dims/vars, or `nc_get_var_double` fails unchecked) | OK | empty, or an uninitialised buffer merged as data (read from source) | +| `sqlite3_step` error in `find_range_st` | OK | chunk's image list silently truncated (read from source) | +| band image opens, then a tile read fails (corrupted tile here; a throttled or refused range request on the network) | OK | every cell set, from the scenes that read | +| mask (SCL) image fails to open | OK | that scene silently dropped | +| a worker chunk file the main process cannot open (e.g. truncated when the temp dir fills) | OK, no R warning | those cells `NA` (measured in the #87 round-2 review: 1536 of 6400 cells, a C++ stderr line only; `chunk_data::read_ncdf()` returns on `nc_open` failure without setting a status) | +| a gdalcubes worker killed by a signal (SIGKILL, segfault) | — | `write_ncdf()` hangs; never raises or returns (round-1 review of #87, measured to a 150 s timeout) | + +`gdalcubes_options(log_file =, debug = TRUE)` writes the same 11 lines for clean and failing reads, so it is not a detector either. + +Where in gdalcubes (paths under `src/gdalcubes/src/`, from the #87 plan review): +- `image_collection_cube.cpp:424-437` sets INCOMPLETE only when `GDALOpen` fails or `warp()` returns nullptr; `warp.cpp` writes into a MEM dataset it creates first, does not check `ChunkAndWarpImage` (`:256`), and logs a failure only at `GCBS_DEBUG` (`:650-654`). +- `image_collection_cube.cpp:569-579`: a mask that fails to open warns and `continue`s the image loop without `set_status`. + + +## Phase 1: gdalcubes fork fix (NewGraphEnvironment/gdalcubes) +- [x] Clone the fork to `~/Projects/repo/gdalcubes` (no local clone exists). Branch + `99-chunk-status-read-failures` off **upstream** `appelmar/master` (`ed68331`), kept clean of + CLAUDE.md/planning so it can go upstream as is, the same way #111 was +- [x] Write the failing tinytest first: `inst/tinytest/test_chunk_status.R`, a local-GeoTIFF + collection (port of drift's `chunk_fixture()`). It covers (a) a band file whose tile data is + corrupted with the IFD intact, (b) a deleted SCL, (c) an SCL with corrupted tiles. Each must + give `chunk_status != 0` in `write_ncdf()` output, at `parallel` 1 and 2 +- [x] `warp.cpp`: on a failed `ChunkAndWarpImage`, warn, clean up (close `out`, free warp options), and + return nullptr in both warp overloads (`:256` and `:650`) +- [x] `image_collection_cube.cpp`: a mask that fails to open sets INCOMPLETE before `continue`. The + `_strict` branch sets ERROR, like the band path +- [x] `cube.cpp` `chunk_data::read_ncdf()`: set ERROR on `nc_open` / missing dim / missing var + failure and check `nc_get_var_double`'s return. The legitimate empty-chunk return stays OK +- [x] Build into a scratch library (`R CMD INSTALL -l /lib`), so the machine's + installed gdalcubes is untouched. tinytest green, existing gdalcubes tests green +- [x] Commit, push the branch to the fork, and open a PR **within the fork** (NewGraphEnvironment/gdalcubes#2, into synced `master`) + +- [x] Plan-review folds (review-plan.md): Initialize/overview-reopen failures, nc_inq_dimlen, + clean controls (other CRS, partial footprint, overviews), cloud value visible, strict branch +- [x] `/code-check` on the fork branch (3 rounds, ended by a 39-row enumeration); sync fork + `master` with upstream +- [x] Push the fork branch; PR into fork `master` after Phase 2 (review G3/O1) + +## Phase 2: worker-crash hang (time-boxed diagnosis) +- [x] Reproduce with the scratch-lib build: SIGKILL one worker mid-`write_ncdf()` at `parallel = 2` + with an external deadline (`timeout`/`with_deadline`). Find where the main process blocks: the + `multiprocess.cpp` loop, `kill(true)` on the siblings, the TinyProcess destructor, or the + sh-wrapper pid vs. the R pid +- [x] If the fix is contained, add it with a test to the fork branch. If not, record the mechanism + and repro in findings and file a drift issue for it +- [x] Either way, record the outcome in `inst/notes/gdalcubes-pc-gotchas.md` + +## Phase 3: drift: drop items without a mask asset +- [x] Failing test first, in `test-dft_stac_cube.R`. A feature list where one item lacks the + mask asset: the item is dropped and the message gives the count. Cover all items lacking it + (that leaves nothing to read, so expect a legible error, not an empty cube) +- [x] Implement it in `stac_cube_items()` (moved there by plan review B2; was `stac_cube_assemble()`) (`R/dft_stac_cube.R`, before + `stac_image_collection()`) or wherever `fetched$features` is built, keeping `is_pre` + aligned. This covers both `dft_stac_cube()` and `dft_stac_composite()`; `dft_stac_fetch()` + has no mask role and is unaffected + +## Phase 4: drift: tests and docs that follow the fork fix +- [x] Capability probe helper in `helper-gdalcubes.R`: does the installed gdalcubes record a + corrupted-tile read in `chunk_status`? (Test the capability, not a version.) +- [x] Offline tests through `cube_write_ncdf()`: corrupted band tile, deleted SCL and + corrupted SCL each abort `drift_incomplete_cube` with nothing cached. They skip with a + drift#99 reason when the probe says the installed gdalcubes lacks the fix. Run them green + against the scratch-lib fork build, and as SKIP against the installed 0.7.5 +- [x] Mutation check (as review O2: probe forced on against 0.7.5 -> exactly the gated tests fail; the tinytest was run against unpatched 0.7.5 too) in the scratch build and confirm the tests go red +- [x] `cube_check_chunks()` abort text: change "an image would not open" to cover open **or + read** failures, without claiming coverage 0.7.5 lacks. Revise the roxygen for + `cube_check_chunks()` / `cube_write_ncdf()` and `inst/notes/gdalcubes-pc-gotchas.md` + ("Detecting failed chunk reads") to say what is recorded with and without the fix +- [x] NEWS.md entry — left to `/gh-pr-merge` (drift writes NEWS in the release commit; #87 did) + +## Phase 5: upstream draft (not posted) +- [x] Update the draft in the #99 body into a PR description for the fork branch: what changes, + the offline repro, and the tinytest. Include the worker-hang outcome, and the cases left + unfixed (worker-side `nc_create`/`CPLMoveFile`, `sqlite3_step`) +- [x] Edit the #99 body to match what was built (body edited, not appended) — done right after the drift PR opens, so it links both PRs + +## Validation +- [x] `devtools::test()` green (new tests SKIP on 0.7.5); new tests green on the fork build +- [x] `lintr::lint_package()` (211 on main and branch: none added), `devtools::document()`, `pkgdown::check_pkgdown()` +- [x] `/code-check`: fork 3 rounds and drift 3 rounds, each ended by an enumeration +- [x] PWF checkboxes match landed work +- [x] `/planning-archive`, then `/gh-pr-push` (drift PR). The fork PR is linked; the upstream post is + left to the user + +## Decisions taken at the plan gate (2026-10-02) + +- drift keeps `Remotes: appelmar/gdalcubes`. New tests that need the fix are gated on a + capability probe and SKIP on 0.7.5, naming drift#99. +- STAC items with no mask asset are **dropped, with a message** before the collection is built. +- Worker-killed-by-signal hang: **time-boxed diagnosis**. Fix it in the fork branch if the fix is + contained; otherwise put it in the upstream draft and a new drift issue. +- Upstream PR/issue: **drafted, not posted**. The user posts it. + +## Verification +- Fork: `Rscript -e '.libPaths(c("/lib", .libPaths())); tinytest::run_test_file(...)'` + gives a nonzero `chunk_status` for all three cases, and 0 with the patch reverted +- drift: `NOT_CRAN=true` test run under the scratch lib (the new tests pass) and under the default + lib (the new tests SKIP with a drift#99 reason) +- No BULK RSS run needed: the drift-side changes read no pixels. The fork patch changes error + paths only, and the #87 cost measurement (2.3 ms per 7,936 chunks) still holds diff --git a/planning/archive/2026-10-issue-99-read-failures-after-open/upstream-pr-draft.md b/planning/archive/2026-10-issue-99-read-failures-after-open/upstream-pr-draft.md new file mode 100644 index 0000000..de666ad --- /dev/null +++ b/planning/archive/2026-10-issue-99-read-failures-after-open/upstream-pr-draft.md @@ -0,0 +1,89 @@ +# DRAFT — upstream PR to appelmar/gdalcubes (NOT POSTED) + +Head: `NewGraphEnvironment:99-chunk-status-read-failures` → base: `appelmar:master` (`ed68331`). +Posting needs the user's explicit approval. Nothing below has been sent anywhere. + +--- + +**Title:** Record failed image reads and mask failures in `chunk_status`; fix hang when a worker dies early + +`write_ncdf()` writes a per-chunk `chunk_status`, which is the only signal R can rely on for failed +reads (the `[WARNING] n out of m chunks` line comes from C++ stdio and never reaches R from a worker +process). Today it records one failure: a band image that fails to open. Several others leave the +chunk OK. The worst is a mask image that opens and then fails to read: the scene is used +**unmasked**, since a NaN mask value never matches a mask class. + +### Changes + +- **`warp_simple()` / `warp_complex()`: a failed warp returns `nullptr`.** `ChunkAndWarpImage()`'s + result was ignored in `warp_simple()` (every image with a geotransform), and only logged at DEBUG + in `warp_complex()`, so the `INIT_DEST=nan` output came back as if it were data. Now a failed + `Initialize()` or `ChunkAndWarpImage()` warns and returns `nullptr`, after the existing cleanup. + `read_chunk()`'s existing `!gdal_out` branches then mark the chunk INCOMPLETE (ERROR when + strict), for band and mask images alike. A partially warped image is discarded rather than + merged, which is the intended INCOMPLETE behaviour. +- **Overview open no longer closes the source first.** If opening the overview level failed, the + source had already been closed and `hSrcDS` pointed at it. The overview is now opened into its own + handle, and the source is closed only when that succeeds. +- **`read_chunk()`: a mask image that fails to open sets INCOMPLETE** (the strict branch returns an + ERROR chunk, like the band path). It used to `continue` without a status. +- **`chunk_data::read_ncdf()`: failures set ERROR.** A worker chunk file the main process cannot + open, whose dimensions or variable are missing, whose dimension sizes cannot be read, or whose + `nc_get_var_double()` fails, was merged with status OK (and in the last case an uninitialised + buffer). These set ERROR and discard the buffer. The empty-chunk return keeps its recorded status + and now closes the file. +- **`chunk_processor_multiprocess::apply()`: no more hang when a worker dies before writing its + log.** After a worker fails, each worker's log is printed with `while (!wlogf.eof())`; for a + worker killed by a signal at startup the log file never exists, the stream is in a failed state + and never reaches `eof()`, and the main process spins at 100% CPU forever. Loop on `getline()`. + +### Behaviour changes worth knowing + +- `ChunkAndWarpImage()` also fails when GDAL cannot compute a source window (e.g. + `ERROR_OUT_IF_EMPTY_SOURCE_WINDOW`, or the transformer failing). Such an image used to give + silent NaN with status OK; it is now INCOMPLETE, and with `incomplete_ok = FALSE` its chunk is + ERROR. +- `read_chunk()` escalates INCOMPLETE to ERROR only when `count_success == 0`, and + `count_success` counts images processed, not images read; so a chunk where every band read + failed is INCOMPLETE rather than ERROR. Warp failures inherit that existing rule. +- Opening the overview before closing the source also fixes a worker segfault on 0.7.5 when a + band read through a band-selection VRT (one band of a multi-band file) goes through overviews + with a mask (measured in review: 0.7.5 worker returns 11; this branch gives status 0). + +### Tests + +- `inst/tinytest/test_chunk_status.R` builds a two-date collection of single-band COGs (a band and + an SCL-style mask) from dummy cubes, and breaks the second date three ways: a band file with its + first tile corrupted (IFD intact, so it opens and then fails to decode), a deleted mask file, and + a corrupted mask file. At `parallel` 1 and 2, each must give a non-OK status, and the cloud value + of the second scene must never reach the output; strict mode with a missing mask must give + ERROR. Clean controls must stay OK: same CRS, a source in another CRS, a view only partly covered, + and a coarser view read through overviews. 32 assertions: **10 fail on 0.7.5, all pass here.** +- `inst/tinytest/test_worker_failure.R` runs `write_ncdf()` in a child R process, with a worker + script that SIGKILLs worker 1 at startup, under `system2(timeout = 120)`: on 0.7.5 it times out, + here it raises in about a second. Unix only. +- Full suite: 159 / 159 (macOS arm64, GDAL 3.13.0, R 4.5). + +### Not covered + +- The `read_ncdf()` ERROR paths have no test: there is no public hook to hand the main process a + damaged chunk file. +- A worker that cannot create or move its chunk file (`nc_create` / `CPLMoveFile` unchecked in + `exec()`) still leaves the chunk at fill with no warning, and an `sqlite3_step` error in + `find_range_st` still truncates a chunk's image list silently. +- Pre-existing, separate: a two-band image with the mask in band 2 segfaults a worker (at full + resolution and through overviews alike, on 0.7.5 too). Happy to open an issue with the repro. +- Pre-existing, separate: in `warp_complex()`, a successful overview open replaces a + `GDALGenImgProjTransformInfo` through `destroy_transform()`/`create_transform()` (the + `warp_simple()` transformer type), so a GCP- or geolocation-georeferenced image with overviews + segfaults a worker when `use_overview_images` is on (the default), on 0.7.5 too. Not changed + here; happy to open an issue with the repro (a GCP-only GTiff with `gdal_addo` overviews, 80 m + view). +- Existing leaks on `read_chunk()`'s `continue` paths (the `/vsimem` band-selection VRT is not + removed and `gdal_out` is not closed after a failed RasterIO) are left as they are, to keep this + diff small. + +--- + +Fork PR (our repo, inside the mandate): opened against `NewGraphEnvironment/gdalcubes:master` +after syncing fork `master` with upstream. diff --git a/tests/testthat/helper-gdalcubes.R b/tests/testthat/helper-gdalcubes.R index 6ea5977..65a1821 100644 --- a/tests/testthat/helper-gdalcubes.R +++ b/tests/testthat/helper-gdalcubes.R @@ -1,27 +1,39 @@ -# A local, dated gdalcubes collection for the #87 chunk-status tests: one GeoTIFF -# per band per item, in EPSG:32609, over `ext` (xmin, xmax, ymin, ymax) at `res`. -# B04 is a constant reflectance, SCL is 4 (vegetation, never masked). `dates` are -# ISO dates, one item each. +# A local, dated gdalcubes collection for the #87 / #99 chunk-status tests: one +# GeoTIFF per band per item, in EPSG:32609, over `ext` (xmin, xmax, ymin, ymax) at +# `res`. B04 is a constant reflectance, SCL is 4 (vegetation, never masked). +# `dates` are ISO dates, one item each. # -# `break_dates` names items whose files are DELETED after the collection is -# built, so gdalcubes knows the image but cannot open it: the same chunk-read -# failure as an expired signed URL or a refused request, without a network. +# `break_dates` names items whose `break_asset` file (B04 or SCL) is broken after +# the collection is built, so gdalcubes knows the image but cannot read it: +# * `break_mode = "delete"` removes the file, so it fails to OPEN: the same +# failure as an expired signed URL or a refused request, without a network. +# * `break_mode = "corrupt"` overwrites the start of its first tile (see +# tif_corrupt_tile()), so it opens and then fails to READ: the throttled or +# refused range request that 0.7.5 records as OK (drift#99). # -# Separate files per band: two-band files with one_band_per_file = FALSE -# segfaulted a gdalcubes 0.7.5 worker (#92). +# Files are DEFLATE COGs: the compression is what makes a corrupted tile fail to +# decode (raw tiles would read as garbage), and COG layout keeps the IFD ahead of +# the tile data, so the corrupted file still opens. Separate files per band: +# two-band files with one_band_per_file = FALSE segfaulted a gdalcubes 0.7.5 +# worker (#92). chunk_fixture <- function(dates, ext = c(0, 400, 0, 400), res = 10, - break_dates = character(0), + break_dates = character(0), break_asset = "B04", + break_mode = c("delete", "corrupt"), envir = parent.frame()) { + break_mode <- match.arg(break_mode) d <- withr::local_tempdir(.local_envir = envir) r <- terra::rast(xmin = ext[1], xmax = ext[2], ymin = ext[3], ymax = ext[4], resolution = res, crs = "EPSG:32609") + cog <- c("COMPRESS=DEFLATE", "OVERVIEWS=NONE") paths <- character(0) for (dt in dates) { stem <- file.path(d, paste0(format(as.Date(dt), "%Y%m%d"), "_a")) b <- terra::init(r, 500) s <- terra::init(r, 4) - terra::writeRaster(b, paste0(stem, "_B04.tif"), datatype = "INT2U") - terra::writeRaster(s, paste0(stem, "_SCL.tif"), datatype = "INT1U") + terra::writeRaster(b, paste0(stem, "_B04.tif"), datatype = "INT2U", + filetype = "COG", gdal = cog) + terra::writeRaster(s, paste0(stem, "_SCL.tif"), datatype = "INT1U", + filetype = "COG", gdal = cog) paths <- c(paths, paste0(stem, c("_B04.tif", "_SCL.tif"))) } fmt_file <- file.path(d, "format.json") @@ -37,11 +49,46 @@ chunk_fixture <- function(dates, ext = c(0, 400, 0, 400), res = 10, paths, format = fmt_file, out_file = file.path(d, "col.db") ) for (dt in break_dates) { - unlink(file.path(d, paste0(format(as.Date(dt), "%Y%m%d"), "_a_B04.tif"))) + f <- file.path(d, paste0(format(as.Date(dt), "%Y%m%d"), "_a_", break_asset, ".tif")) + if (break_mode == "delete") unlink(f) else tif_corrupt_tile(f) } + attr(col, "fixture_dir") <- d col } +# Overwrite the start of the first tile's compressed stream, located from the +# TileOffsets tag (324) of the first IFD of a little-endian classic TIFF. Not the +# end of the file: a plain GTiff keeps its IFD there (so that tests an open +# failure), and in a COG the tail is the DEFLATE trailer, which still decoded. +tif_corrupt_tile <- function(f) { + con <- file(f, "r+b") + on.exit(close(con)) + # little-endian classic TIFF (42; BigTIFF, 43, has 8-byte offsets) + stopifnot(identical(readBin(con, "raw", 2), charToRaw("II")), + readBin(con, "integer", size = 2, signed = FALSE, endian = "little") == 42L) + seek(con, 4) + ifd <- readBin(con, "integer", size = 4, endian = "little") + seek(con, ifd) + n <- readBin(con, "integer", size = 2, signed = FALSE, endian = "little") + tile <- NA + for (i in seq_len(n)) { + tag <- readBin(con, "integer", size = 2, signed = FALSE, endian = "little") + seek(con, 2, origin = "current") + count <- readBin(con, "integer", size = 4, endian = "little") + val <- readBin(con, "integer", size = 4, endian = "little") + # one tile: the value IS its offset; more and it points at an array of them, + # and overwriting there would corrupt the directory, not the pixels + if (tag == 324) { + stopifnot(count == 1L) + tile <- val + } + } + stopifnot(!is.na(tile)) + seek(con, tile, rw = "write") + writeBin(as.raw(rep(0xAB, 64)), con) + invisible(f) +} + # Write a cube over a chunk_fixture() collection with gdalcubes itself, at a given # worker count, and return the NetCDF path. `chunking` small enough that a # fixture spans several chunks. @@ -49,6 +96,7 @@ chunk_fixture_write <- function(col, ext = c(0, 400, 0, 400), res = 10, t0 = "2021-07-01", t1 = "2021-07-31", dt = "P1D", aggregation = "first", parallel = 1, chunking = c(1, 16, 16), + mask = NULL, pixel = NULL, envir = parent.frame()) { old <- gdalcubes::gdalcubes_options()$parallel gdalcubes::gdalcubes_options(parallel = parallel) @@ -62,6 +110,50 @@ chunk_fixture_write <- function(col, ext = c(0, 400, 0, 400), res = 10, out <- withr::local_tempfile(fileext = ".nc", .local_envir = envir) # A broken fixture prints gdalcubes' "[WARNING] n out of m chunks" line from # C++ stdio, which no R handler or capture.output() can silence (#87). - gdalcubes::write_ncdf(gdalcubes::raster_cube(col, v, chunking = chunking), out) + cube <- gdalcubes::raster_cube(col, v, mask = mask, chunking = chunking) + # `pixel`, e.g. production_pixel, is applied before the write + if (!is.null(pixel)) cube <- pixel(cube) + gdalcubes::write_ncdf(cube, out) out } + +# Does the installed gdalcubes record a read that fails AFTER its image opened in +# chunk_status? 0.7.5 does not (drift#99); the NewGraphEnvironment/gdalcubes fix +# does. The tests that need it skip without it, naming drift#99. Asked of the +# capability, not a version. gdalcubes computes every chunk in Rscript worker +# processes, at parallel = 1 too, and they load gdalcubes from R_LIBS rather +# than from this session's .libPaths(); so this answers for the library that +# actually does drift's reads, which is the one that matters. Lazy and memoised: +# one small write per session, only in files that ask. +# DRIFT_TEST_ASSUME_READ_STATUS=true|false overrides it, which is how the tests +# are shown to fail against a gdalcubes without the fix. +read_failures_recorded <- local({ + answer <- NULL + function() { + forced <- tolower(Sys.getenv("DRIFT_TEST_ASSUME_READ_STATUS")) + if (forced %in% c("true", "1")) return(TRUE) + if (forced %in% c("false", "0")) return(FALSE) + if (nzchar(forced)) stop("DRIFT_TEST_ASSUME_READ_STATUS must be true or false, not ", forced) + if (is.null(answer)) { + col <- chunk_fixture(c("2021-07-03", "2021-07-13"), break_dates = "2021-07-13", + break_mode = "corrupt") + nc <- suppressWarnings(chunk_fixture_write(col, parallel = 2)) + answer <<- drift:::chunk_status_failed(nc)[["failed"]] > 0 + } + answer + } +}) + +skip_if_no_read_failure_status <- function() { + testthat::skip_if_not_installed("gdalcubes") + if (!read_failures_recorded()) { + testthat::skip("installed gdalcubes records a failed read after open as OK (drift#99)") + } +} + +# The shape drift's cube and composite reads take: a masked raster_cube, then +# apply_pixel() on the band expression with no select_bands(). gdalcubes then reads +# the mask asset (SCL) as a BAND as well as the mask, which decides which SCL +# failures 0.7.5 already records (#99): a deleted SCL fails its band open and is +# recorded; a corrupted SCL is not. +production_pixel <- function(cube) gdalcubes::apply_pixel(cube, "B04 / 10000", names = "red") diff --git a/tests/testthat/test-dft_stac_composite.R b/tests/testthat/test-dft_stac_composite.R index 25995cc..570ee41 100644 --- a/tests/testthat/test-dft_stac_composite.R +++ b/tests/testthat/test-dft_stac_composite.R @@ -160,6 +160,30 @@ test_that("composite_layers_order selects layers by name, not position", { "do not match") }) +test_that("a year with no masked scene is skipped for that reason, not as 'no scenes' (#99)", { + skip_if_not_installed("gdalcubes") + aoi <- aoi_pkg() + testthat::local_mocked_bindings( + stac_cube_items = function(...) { + rlang::abort("2 STAC items found for sentinel-2-l2a, but none has the mask asset 'SCL'", + class = c("drift_no_mask_items", "drift_no_items")) + } + ) + w <- character(0) + expect_error( + withCallingHandlers( + suppressMessages(dft_stac_composite(aoi, years = 2017, cache_dir = withr::local_tempdir())), + warning = function(cnd) { + w <<- c(w, conditionMessage(cnd)) + invokeRestart("muffleWarning") + } + ), + "the warnings say why" + ) + expect_true(any(grepl("no scene has the cloud-mask asset", w, fixed = TRUE))) + expect_false(any(grepl(": no scenes.", w, fixed = TRUE))) +}) + test_that("an empty year is dropped with a warning and the others are kept", { skip_if_not_installed("gdalcubes") aoi <- aoi_pkg() @@ -687,12 +711,14 @@ aoi_fixture_ext <- function(aoi) { # `break_dates` holds one entry per year: the dates to break in that year's # collection. Years are 2021, 2022, ...; each has scenes on 07-03 and 07-13. composite_on_fixture <- function(break_dates, tile_size = NULL, cache, + break_asset = "B04", break_mode = "delete", env = parent.frame()) { aoi <- aoi_pkg() years <- 2020 + seq_along(break_dates) cols <- lapply(seq_along(years), function(i) { chunk_fixture(paste0(years[i], c("-07-03", "-07-13")), ext = aoi_fixture_ext(aoi), break_dates = break_dates[[i]], + break_asset = break_asset, break_mode = break_mode, envir = env) }) year_now <- 0L @@ -743,6 +769,33 @@ test_that("a composite over a readable collection is built and cached as before } }) +test_that("a composite whose mask (SCL) image is gone aborts and caches nothing (#99)", { + # Not gated: through drift's own read path (stac_cube_assemble(), no + # select_bands() before the pixel function) gdalcubes also reads SCL as a band, + # so 0.7.5 records the failed open. If drift ever selected bands first, the + # scene would be dropped silently on 0.7.5, and this would fail. + skip_if_not_installed("gdalcubes") + cache <- withr::local_tempdir() + expect_error( + suppressWarnings(composite_on_fixture(list("2021-07-13"), cache = cache, + break_asset = "SCL", break_mode = "delete")), + class = "drift_incomplete_cube" + ) + expect_length(list.files(cache, recursive = TRUE, all.files = TRUE), 0L) +}) + +test_that("a composite whose mask (SCL) read failed aborts and caches nothing (#99)", { + # On gdalcubes 0.7.5 the scene is used unmasked and the chunk is recorded OK. + skip_if_no_read_failure_status() + cache <- withr::local_tempdir() + expect_error( + suppressWarnings(composite_on_fixture(list("2021-07-13"), cache = cache, + break_asset = "SCL", break_mode = "corrupt")), + class = "drift_incomplete_cube" + ) + expect_length(list.files(cache, recursive = TRUE, all.files = TRUE), 0L) +}) + test_that("a failed year is an abort, not a skipped year (#87)", { # The year loop turns drift_no_items and drift_empty_cube into a warning and a # dropped year. A holed read must not take that exit: it would return the diff --git a/tests/testthat/test-dft_stac_cube.R b/tests/testthat/test-dft_stac_cube.R index dfb5d1c..f891652 100644 --- a/tests/testthat/test-dft_stac_cube.R +++ b/tests/testthat/test-dft_stac_cube.R @@ -1117,3 +1117,175 @@ test_that("a failed read on the post-boundary side of an offset split aborts (#8 # both sides were built: the abort is the post side's, not a short-circuit expect_equal(calls, 2L) }) + + +# --- #99: an item with no mask asset is dropped, not read unmasked ----------- + +# gdalcubes warns "Missing mask band for image ..., mask will be ignored" and +# uses such a scene UNMASKED, so clouds reach the median with the chunk status OK. +mask_feature <- function(id, assets = c("B04", "SCL"), datetime = "2023-07-01T00:00:00Z") { + list(id = id, properties = list(datetime = datetime), + assets = stats::setNames(lapply(assets, function(a) list(href = a)), assets)) +} + +test_that("stac_features_masked drops only the features without the mask asset", { + feats <- list(mask_feature("a"), mask_feature("no_scl_item", "B04"), mask_feature("c")) + # the id is distinctive so the match cannot land in the message's fixed text + expect_message(out <- drift:::stac_features_masked(feats, "SCL"), + "1 item.*SCL.*no_scl_item") + expect_equal(vapply(out, `[[`, "", "id"), c("a", "c")) +}) + +test_that("stac_features_masked passes everything through when nothing is missing", { + feats <- list(mask_feature("a"), mask_feature("c")) + expect_no_message(out <- drift:::stac_features_masked(feats, "SCL")) + expect_identical(out, feats) +}) + +test_that("stac_features_masked is a no-op for a source with no mask role", { + feats <- list(mask_feature("a", "B04")) + expect_identical(drift:::stac_features_masked(feats, NULL), feats) + expect_identical(drift:::stac_features_masked(list(), "SCL"), list()) +}) + +test_that("stac_features_masked matches the asset name exactly, not by prefix", { + # `$` would partial-match SCL_20m for SCL; the asset must exist under its own name + feats <- list(mask_feature("a", c("B04", "SCL_20m"))) + expect_message(out <- drift:::stac_features_masked(feats, "SCL"), "1 item") + expect_length(out, 0) +}) + +# rstac mocked as test-dft_stac_fetch.R does, so stac_cube_items() runs for real +stac_items_mocked <- function(feats, months = NULL) { + testthat::local_mocked_bindings( + stac = function(...) list(), + stac_search = function(q, ...) q, + ext_filter = function(q, ...) q, + post_request = function(q, ...) list(type = "FeatureCollection", features = feats), + items_fetch = function(items, ...) items, + items_sign = function(items, ...) items, + .package = "rstac" + ) + aoi <- sf::st_as_sf(sf::st_sfc(sf::st_point(c(-126, 54)), crs = 4326)) + suppressMessages(drift:::stac_cube_items( + drift::dft_stac_config("sentinel-2-l2a"), aoi, "2021-01-01/2023-12-31", + cloud_cover_max = 20, months = months, sign_fn = function(x) x + )) +} + +test_that("stac_cube_items drops items without the mask asset, keeping is_pre aligned", { + feats <- list( + mask_feature("pre_ok", datetime = "2021-07-01T00:00:00Z"), + mask_feature("pre_bad", "B04", datetime = "2021-08-01T00:00:00Z"), + mask_feature("post_ok", datetime = "2023-07-01T00:00:00Z") + ) + out <- stac_items_mocked(feats) + expect_equal(vapply(out$features, `[[`, "", "id"), c("pre_ok", "post_ok")) + expect_equal(out$is_pre, c(TRUE, FALSE)) + # the re-sign source carries the same features as the read + expect_equal(vapply(out$items$features, `[[`, "", "id"), c("pre_ok", "post_ok")) +}) + +test_that("stac_cube_items raises drift_no_items when no item has the mask asset", { + feats <- list(mask_feature("a", "B04"), mask_feature("b", "B04")) + expect_error(stac_items_mocked(feats), class = "drift_no_items") + # its own subclass, so a composite does not report it as "no scenes" + expect_error(stac_items_mocked(feats), class = "drift_no_mask_items") + expect_error(stac_items_mocked(feats), "mask asset") +}) + + +# --- #99: failed reads after open, and failed mask reads --------------------- +# +# gdalcubes 0.7.5 records only a band image that fails to OPEN. A read that fails +# after the open, and a mask (SCL) image that fails to open or read, leave the +# chunk OK; the fork fix records them (NewGraphEnvironment/gdalcubes). These tests +# need it and skip without it (skip_if_no_read_failure_status(), helper-gdalcubes.R). + +scl_mask <- function() gdalcubes::image_mask("SCL", values = c(8, 9)) + +test_that("a corrupted fixture tile opens and then fails to read (#99 premise)", { + # Without this, a 'read failure' test could be testing an open failure. + skip_if_not_installed("gdalcubes") + col <- chunk_fixture(c("2021-07-03", "2021-07-13"), break_dates = "2021-07-13", + break_mode = "corrupt") + f <- file.path(attr(col, "fixture_dir"), "20210713_a_B04.tif") + expect_true(file.exists(f)) + r <- terra::rast(f) # opens + expect_error(suppressWarnings(terra::values(r))) # but its tile does not decode +}) + +test_that("the capability probe agrees with an independent read failure (#99)", { + # Not gated, and asked without the override: the probe (a corrupted BAND) + # must give the same answer as a corrupted MASK read in drift's shape, so a + # probe that wrongly says FALSE on a fixed gdalcubes fails here instead of + # silently skipping the tests below. + skip_if_not_installed("gdalcubes") + withr::local_envvar(DRIFT_TEST_ASSUME_READ_STATUS = NA) + col <- chunk_fixture(c("2021-07-03", "2021-07-13"), break_dates = "2021-07-13", + break_asset = "SCL", break_mode = "corrupt") + nc <- suppressWarnings(chunk_fixture_write(col, parallel = 2, mask = scl_mask(), + pixel = production_pixel)) + expect_identical(read_failures_recorded(), + drift:::chunk_status_failed(nc)[["failed"]] > 0) +}) + +test_that("the capability probe honours a forced answer (#99)", { + skip_if_not_installed("gdalcubes") + for (v in c("true", "TRUE", "1")) { + withr::local_envvar(DRIFT_TEST_ASSUME_READ_STATUS = v) + expect_true(read_failures_recorded()) + } + for (v in c("false", "0")) { + withr::local_envvar(DRIFT_TEST_ASSUME_READ_STATUS = v) + expect_false(read_failures_recorded()) + } + withr::local_envvar(DRIFT_TEST_ASSUME_READ_STATUS = "yes") + expect_error(read_failures_recorded(), "true or false") +}) + +test_that("a band read that fails after open aborts the chunk check (#99)", { + skip_if_no_read_failure_status() + col <- chunk_fixture(c("2021-07-03", "2021-07-13"), break_dates = "2021-07-13", + break_mode = "corrupt") + for (p in c(1, 4)) { + nc <- suppressWarnings(chunk_fixture_write(col, parallel = p, mask = scl_mask())) + expect_error(drift:::cube_check_chunks(nc), class = "drift_incomplete_cube") + } +}) + +test_that("a mask image that fails to open aborts the chunk check, in drift's read shape (#99)", { + # Not gated: in drift's shape (production_pixel) gdalcubes also reads SCL as a + # band, so a deleted SCL fails a BAND open and 0.7.5 already records it. With + # select_bands() it would reach only the mask path, which the fork fix records. + skip_if_not_installed("gdalcubes") + col <- chunk_fixture(c("2021-07-03", "2021-07-13"), break_dates = "2021-07-13", + break_asset = "SCL", break_mode = "delete") + for (p in c(1, 4)) { + nc <- suppressWarnings(chunk_fixture_write(col, parallel = p, mask = scl_mask(), + pixel = production_pixel)) + expect_error(drift:::cube_check_chunks(nc), class = "drift_incomplete_cube") + } +}) + +test_that("a mask image that fails to read aborts the chunk check, in drift's read shape (#99)", { + # On 0.7.5 this scene is used UNMASKED with status OK, in this shape and with + # select_bands() alike. + skip_if_no_read_failure_status() + col <- chunk_fixture(c("2021-07-03", "2021-07-13"), break_dates = "2021-07-13", + break_asset = "SCL", break_mode = "corrupt") + for (p in c(1, 4)) { + nc <- suppressWarnings(chunk_fixture_write(col, parallel = p, mask = scl_mask(), + pixel = production_pixel)) + expect_error(drift:::cube_check_chunks(nc), class = "drift_incomplete_cube") + } +}) + +test_that("a clean masked read in drift's read shape passes the chunk check (#99 control)", { + skip_if_not_installed("gdalcubes") + col <- chunk_fixture(c("2021-07-03", "2021-07-13")) + for (p in c(1, 4)) { + nc <- chunk_fixture_write(col, parallel = p, mask = scl_mask(), pixel = production_pixel) + expect_silent(drift:::cube_check_chunks(nc)) + } +})