diff --git a/docs/source/reference/geotiff_release_contract.md b/docs/source/reference/geotiff_release_contract.md index 9d7787d8..05e81d2e 100644 --- a/docs/source/reference/geotiff_release_contract.md +++ b/docs/source/reference/geotiff_release_contract.md @@ -89,7 +89,7 @@ category. The `Key` column matches the runtime key. | `reader.http` | advanced | Plain HTTP/HTTPS reads; SSRF and private-host filters apply. | | `reader.http_cog` | advanced | HTTP COG with range-request fetching. The transport surface (redirects, retries) is not yet contracted at the stable bar. | | `reader.vrt` | advanced | Simple VRT mosaics. Full GDAL VRT parity is out of scope. | -| `reader.sidecar_ovr` | advanced | External `.tif.ovr` sidecar overviews. | +| `reader.sidecar_ovr` | advanced | External `.tif.ovr` sidecar overviews. A stale or malformed sidecar does not break a base read: the eager CPU, eager GPU, and dask metadata paths warn and fall through to base-file-only behaviour for `overview_level=None`/`0`. Requesting a specific external level surfaces the underlying parse error. Caller-set `max_cloud_bytes` breaches still raise `CloudSizeLimitError` either way. See issue #2416. | | `reader.allow_rotated` | experimental | Opt-in `allow_rotated=True`; drops the axis-aligned `transform` attr in favour of `rotated_affine`. | | `reader.allow_unparseable_crs` | experimental | Opt-in escape hatch for CRS strings pyproj cannot parse. | | `reader.gpu` | experimental | GPU read path; no cross-backend numerical parity claim. | diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index 38574c89..46865468 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -76,7 +76,7 @@ RotatedTransformError, UnknownCRSModelTypeError, UnparseableCRSError, UnsupportedGeoTIFFFeatureError) from ._geotags import RASTER_PIXEL_IS_AREA, RASTER_PIXEL_IS_POINT, GeoTransform # noqa: F401 -from ._reader import _MAX_CLOUD_BYTES_SENTINEL, UnsafeURLError +from ._reader import _MAX_CLOUD_BYTES_SENTINEL, CloudSizeLimitError, UnsafeURLError from ._reader import read_to_array as _read_to_array from ._runtime import (_CRS_WKT_DEPRECATED_SENTINEL, _GPU_DEPRECATED_SENTINEL, # noqa: F401 _MISSING_SOURCES_SENTINEL, _ON_GPU_FAILURE_SENTINEL, GeoTIFFFallbackWarning, @@ -275,22 +275,52 @@ def _read_geo_info(source, *, overview_level: int | None = None, # Append sibling `.tif.ovr` sidecar IFDs onto the pyramid list # so ``overview_level`` indexes both internal and external # overviews (issue #2112). Local file paths only. + # + # A broken sidecar must not break the base read. The release + # contract puts ``reader.local_file`` at the stable tier and + # ``reader.sidecar_ovr`` at advanced; a stale or corrupt + # ``.ovr`` written by an external tool falls back to base-only + # behaviour with a warning. Mirrors the eager CPU path in + # ``_reader._read_to_array`` and the dask metadata helper + # ``_sidecar.discover_remote_sidecar``. Issue #2416. from ._sidecar import attach_sidecar_origin, find_sidecar, load_sidecar sidecar_origin: dict[int, tuple] = {} sidecar_path = find_sidecar(source) if sidecar_path is not None: - sidecar = load_sidecar(sidecar_path) - # The origin mapping is consumed below for georef extraction - # only -- strip/tile bytes are sliced by ``read_to_array`` on - # the actual read. A sidecar IFD that carries its own - # GeoKeyDirectory / ModelPixelScale / ModelTiepoint / - # ModelTransformation needs the sidecar's byte order to - # parse cleanly; without the mapping the helper falls back - # to the base file's bytes (today's default, correct under - # the usual GDAL convention). See issue #2315. - sidecar_origin = attach_sidecar_origin( - sidecar.ifds, sidecar.data, sidecar.header) - ifds = ifds + sidecar.ifds + try: + sidecar = load_sidecar(sidecar_path) + except CloudSizeLimitError: + # Re-raised for symmetry with ``_reader._read_to_array``; + # the byte budget is a caller-set contract. In practice + # this branch is local-file-only (the cloud / HTTP cases + # are handled in the earlier ``_parse_cog_http_meta`` / + # ``_CloudSource`` branch above) so the exception cannot + # fire from a local mmap today, but keeping the explicit + # re-raise prevents the symmetry breaking if a future + # patch routes a cloud-source path through here. + raise + except Exception as exc: + warnings.warn( + f"Ignoring unreadable sidecar {sidecar_path!r}: " + f"{type(exc).__name__}: {exc}. Falling back to " + f"base-file-only read. Delete the .ovr file or pass " + f"overview_level>=1 to surface the parse error.", + RuntimeWarning, + stacklevel=3, + ) + sidecar = None + if sidecar is not None: + # The origin mapping is consumed below for georef extraction + # only -- strip/tile bytes are sliced by ``read_to_array`` on + # the actual read. A sidecar IFD that carries its own + # GeoKeyDirectory / ModelPixelScale / ModelTiepoint / + # ModelTransformation needs the sidecar's byte order to + # parse cleanly; without the mapping the helper falls back + # to the base file's bytes (today's default, correct under + # the usual GDAL convention). See issue #2315. + sidecar_origin = attach_sidecar_origin( + sidecar.ifds, sidecar.data, sidecar.header) + ifds = ifds + sidecar.ifds ifd = select_overview_ifd(ifds, overview_level) # Inherit georef from the level-0 IFD when the overview itself # has no geokeys (issue #1640). Pass-through for level 0. The diff --git a/xrspatial/geotiff/_backends/gpu.py b/xrspatial/geotiff/_backends/gpu.py index 72fb99d4..eecc0bdc 100644 --- a/xrspatial/geotiff/_backends/gpu.py +++ b/xrspatial/geotiff/_backends/gpu.py @@ -23,7 +23,7 @@ from .._attrs import _finalize_eager_read, _finalize_lazy_read_attrs from .._coords import coords_from_geo_info as _coords_from_geo_info from .._nodata import NodataLifecycle as _NL -from .._reader import _MAX_CLOUD_BYTES_SENTINEL, _coerce_path +from .._reader import _MAX_CLOUD_BYTES_SENTINEL, CloudSizeLimitError, _coerce_path from .._reader import read_to_array as _read_to_array from .._runtime import (_GPU_DEPRECATED_SENTINEL, _MISSING_SOURCES_SENTINEL, _ON_GPU_FAILURE_SENTINEL, _geotiff_strict_mode) @@ -406,14 +406,40 @@ def read_geotiff_gpu(source: str, *, # the sidecar's buffers, and we skip the GDS fast path -- GDS # reads the source file path, which would point at the base # file rather than the sidecar. + # + # A broken sidecar must not break a base read here either. The + # release contract puts ``reader.local_file`` at the stable tier + # and ``reader.sidecar_ovr`` at advanced; matches the eager CPU + # path in ``_reader._read_to_array`` and the dask metadata + # helper ``_sidecar.discover_remote_sidecar``. Issue #2416. from .._sidecar import attach_sidecar_origin, close_sidecar, find_sidecar, load_sidecar sidecar_origin: dict[int, tuple] = {} sidecar_path = find_sidecar(source) if sidecar_path is not None: - sidecar = load_sidecar(sidecar_path) - sidecar_origin = attach_sidecar_origin( - sidecar.ifds, sidecar.data, sidecar.header) - ifds = ifds + sidecar.ifds + try: + sidecar = load_sidecar(sidecar_path) + except CloudSizeLimitError: + # Re-raised for symmetry with ``_reader._read_to_array``; + # the byte budget is a caller-set contract. The GPU eager + # path operates on a local mmap source today so the + # exception cannot fire here, but keeping the explicit + # re-raise prevents the symmetry breaking if a future + # patch routes a cloud-source path through here. + raise + except Exception as exc: + warnings.warn( + f"Ignoring unreadable sidecar {sidecar_path!r}: " + f"{type(exc).__name__}: {exc}. Falling back to " + f"base-file-only read. Delete the .ovr file or pass " + f"overview_level>=1 to surface the parse error.", + RuntimeWarning, + stacklevel=3, + ) + sidecar = None + if sidecar is not None: + sidecar_origin = attach_sidecar_origin( + sidecar.ifds, sidecar.data, sidecar.header) + ifds = ifds + sidecar.ifds # Skip mask IFDs (NewSubfileType bit 2) ifd = select_overview_ifd(ifds, overview_level) diff --git a/xrspatial/geotiff/_reader.py b/xrspatial/geotiff/_reader.py index 91e55ca1..7be3d02a 100644 --- a/xrspatial/geotiff/_reader.py +++ b/xrspatial/geotiff/_reader.py @@ -17,6 +17,8 @@ """ from __future__ import annotations +import warnings + import numpy as np # ``urllib3`` is kept as a top-level import here even though the HTTP # source moved to ``_sources`` in #2228. ``test_http_no_stdlib_fallback_2050`` @@ -218,15 +220,43 @@ def _read_to_array(source, *, window=None, overview_level: int | None = None, # enforces (#2121). The sidecar must be loaded before IFD # selection so ``overview_level`` indexes into a unified # pyramid list. + # + # Sidecar load failures must not break a base read. The release + # contract classifies ``reader.local_file`` as stable and + # ``reader.sidecar_ovr`` as advanced (see + # ``docs/source/reference/geotiff_release_contract.md``); a + # stale, truncated, or malformed ``.ovr`` written by an external + # tool should not be able to take the stable surface down. + # ``CloudSizeLimitError`` is the one exception: that signals a + # caller-set byte budget breach which the caller asked to hear + # about. Everything else (bad TIFF header, I/O error, fsspec + # failure) falls back to base-only behaviour with a warning so + # the user can still investigate. Mirrors the contract that + # ``discover_remote_sidecar`` already uses on the dask metadata + # path. Issue #2416. from ._sidecar import attach_sidecar_origin, find_sidecar, load_sidecar sidecar_origin: dict[int, tuple] = {} sidecar_path = find_sidecar(source) if sidecar_path is not None: - sidecar = load_sidecar(sidecar_path, - max_cloud_bytes=cloud_budget) - sidecar_origin = attach_sidecar_origin( - sidecar.ifds, sidecar.data, sidecar.header) - ifds = ifds + sidecar.ifds + try: + sidecar = load_sidecar(sidecar_path, + max_cloud_bytes=cloud_budget) + except CloudSizeLimitError: + raise + except Exception as exc: + warnings.warn( + f"Ignoring unreadable sidecar {sidecar_path!r}: " + f"{type(exc).__name__}: {exc}. Falling back to " + f"base-file-only read. Delete the .ovr file or pass " + f"overview_level>=1 to surface the parse error.", + RuntimeWarning, + stacklevel=3, + ) + sidecar = None + if sidecar is not None: + sidecar_origin = attach_sidecar_origin( + sidecar.ifds, sidecar.data, sidecar.header) + ifds = ifds + sidecar.ifds # Select IFD, skipping any mask IFDs ifd = select_overview_ifd(ifds, overview_level) diff --git a/xrspatial/geotiff/tests/test_sidecar_bad_does_not_break_base_2416.py b/xrspatial/geotiff/tests/test_sidecar_bad_does_not_break_base_2416.py new file mode 100644 index 00000000..9e416e59 --- /dev/null +++ b/xrspatial/geotiff/tests/test_sidecar_bad_does_not_break_base_2416.py @@ -0,0 +1,224 @@ +"""Corrupt ``.ovr`` sidecar must not break a base read (issue #2416). + +The release contract classifies ``reader.local_file`` as stable and +``reader.sidecar_ovr`` as advanced. A stale, truncated, or malformed +sibling ``.ovr`` file -- typically dropped by external tools like +``gdaladdo``, partial downloads, or stray build artifacts -- should not +take the stable base read down. + +These tests pin the contract: + +* A base read against a valid TIFF succeeds when the sibling ``.ovr`` + bytes are garbage. A ``RuntimeWarning`` is emitted so the user can + still investigate. +* Explicitly requesting ``overview_level=0`` is also a base read and + must succeed. +* Explicitly requesting an external overview level (``overview_level=1``) + surfaces the underlying parse error -- silent fallback only applies + when the caller did not ask for the broken surface. +* ``CloudSizeLimitError`` still propagates because it represents a + caller-set byte budget the caller asked to hear about. +* The same contract holds on the GPU eager path + (``read_geotiff_gpu`` via ``open_geotiff(gpu=True)``) which also + walks the sidecar before IFD selection. +* The metadata-only path (``_read_geo_info``) follows the same rule + for the base level. +""" +from __future__ import annotations + +import importlib.util + +import numpy as np +import pytest +import xarray as xr + +from xrspatial.geotiff import open_geotiff +from xrspatial.geotiff._reader import read_to_array +from xrspatial.geotiff._writers.eager import to_geotiff + + +# --------------------------------------------------------------------------- +# Fixtures: build a valid base TIFF and pair it with a chosen sidecar payload. +# --------------------------------------------------------------------------- +def _make_base(tmp_path) -> tuple: + """Write a 4x4 uint16 base TIFF and return (path, expected array).""" + arr = np.arange(16, dtype=np.uint16).reshape(4, 4) + xs = np.arange(4) + 0.5 + ys = np.arange(4) + 0.5 + da = xr.DataArray(arr, dims=("y", "x"), coords={"y": ys, "x": xs}) + da.attrs["crs"] = 4326 + path = tmp_path / "base_2416.tif" + to_geotiff(da, str(path), tiled=False, compression="none") + return path, arr + + +def _make_corrupt_sidecar(base_path, payload: bytes) -> None: + side = str(base_path) + ".ovr" + with open(side, "wb") as f: + f.write(payload) + + +def _gpu_available() -> bool: + if importlib.util.find_spec("cupy") is None: + return False + try: + import cupy + return bool(cupy.cuda.is_available()) + except Exception: + return False + + +# --------------------------------------------------------------------------- +# Eager CPU path: base read survives a broken sidecar. +# --------------------------------------------------------------------------- +def test_open_geotiff_base_read_survives_unreadable_sidecar(tmp_path): + path, expected = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"not a tiff") + + with pytest.warns(RuntimeWarning, match="Ignoring unreadable sidecar"): + da = open_geotiff(str(path)) + + np.testing.assert_array_equal(da.values, expected) + + +def test_open_geotiff_overview_level_zero_survives_unreadable_sidecar(tmp_path): + """``overview_level=0`` is also a base read; the sidecar is not needed.""" + path, expected = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"\x00\x00\x00\x00\x00\x00\x00\x00") + + with pytest.warns(RuntimeWarning, match="Ignoring unreadable sidecar"): + da = open_geotiff(str(path), overview_level=0) + + np.testing.assert_array_equal(da.values, expected) + + +def test_read_to_array_base_read_survives_unreadable_sidecar(tmp_path): + """The lower-level ``read_to_array`` entry point honours the same rule.""" + path, expected = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"this is not even close to a TIFF header") + + with pytest.warns(RuntimeWarning, match="Ignoring unreadable sidecar"): + arr, _ = read_to_array(str(path)) + + np.testing.assert_array_equal(arr, expected) + + +@pytest.mark.parametrize( + "payload", + [ + b"", # empty file + b"x", # too short for any header parse + b"not a tiff", # 10 bytes, looks like text + b"GZIP\x1f\x8b\x08\x00", # gzip magic, wrong file type + b"\x89PNG\r\n\x1a\n", # PNG magic, wrong file type + ], +) +def test_open_geotiff_base_read_survives_various_sidecar_payloads( + tmp_path, payload +): + path, expected = _make_base(tmp_path) + _make_corrupt_sidecar(path, payload) + + with pytest.warns(RuntimeWarning, match="Ignoring unreadable sidecar"): + da = open_geotiff(str(path)) + + np.testing.assert_array_equal(da.values, expected) + + +# --------------------------------------------------------------------------- +# Explicit external-overview requests still surface the error. +# --------------------------------------------------------------------------- +def test_open_geotiff_requesting_sidecar_level_still_raises(tmp_path): + """Asking for ``overview_level=1`` is asking for the sidecar surface; + a parse failure on that surface should not be silently swallowed.""" + path, _ = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"not a tiff") + + # The base IFD list has length 1, sidecar is unreadable -> level 1 + # falls outside the merged list. The reader's ``select_overview_ifd`` + # raises ``ValueError`` for an out-of-range level. The point is that + # the error surfaces (warning + then explicit raise) rather than the + # base read silently failing. + with pytest.warns(RuntimeWarning, match="Ignoring unreadable sidecar"): + with pytest.raises(ValueError, match="out of range"): + open_geotiff(str(path), overview_level=1) + + +# --------------------------------------------------------------------------- +# CloudSizeLimitError still propagates: byte budget is a caller contract. +# --------------------------------------------------------------------------- +def test_cloud_size_limit_error_from_sidecar_is_not_silenced(tmp_path, monkeypatch): + from xrspatial.geotiff import _sidecar as _sidecar_mod + from xrspatial.geotiff._reader import CloudSizeLimitError + + path, _ = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"\x00") # contents do not matter; we stub + + def _budget_breach(_path, *, max_cloud_bytes=None): + raise CloudSizeLimitError("sidecar too big for budget") + + monkeypatch.setattr(_sidecar_mod, "load_sidecar", _budget_breach) + + with pytest.raises(CloudSizeLimitError): + open_geotiff(str(path)) + + +# --------------------------------------------------------------------------- +# Metadata-only path (used by the dask graph builder for local files). +# --------------------------------------------------------------------------- +def test_read_geo_info_base_survives_unreadable_sidecar(tmp_path): + from xrspatial.geotiff import _read_geo_info + + path, _ = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"not a tiff") + + with pytest.warns(RuntimeWarning, match="Ignoring unreadable sidecar"): + geo, h, w, dtype, _n = _read_geo_info(str(path)) + + assert (h, w) == (4, 4) + assert geo is not None + + +def test_read_geo_info_cloud_size_limit_error_is_not_silenced( + tmp_path, monkeypatch +): + """Symmetry with the eager CPU path: the metadata-only helper must + re-raise ``CloudSizeLimitError`` rather than swallow it as a generic + sidecar failure. Cannot fire on a local mmap source today, but the + contract should not silently regress if a future patch widens the + helper to a cloud source.""" + from xrspatial.geotiff import _read_geo_info + from xrspatial.geotiff import _sidecar as _sidecar_mod + from xrspatial.geotiff._reader import CloudSizeLimitError + + path, _ = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"\x00") + + def _budget_breach(_path, *, max_cloud_bytes=None): + raise CloudSizeLimitError("sidecar too big for budget") + + monkeypatch.setattr(_sidecar_mod, "load_sidecar", _budget_breach) + + with pytest.raises(CloudSizeLimitError): + _read_geo_info(str(path)) + + +# --------------------------------------------------------------------------- +# GPU eager path: same contract. +# --------------------------------------------------------------------------- +@pytest.fixture +def _gpu_or_skip(): + if not _gpu_available(): + pytest.skip("cupy + CUDA required") + + +def test_open_geotiff_gpu_base_read_survives_unreadable_sidecar( + tmp_path, _gpu_or_skip +): + path, expected = _make_base(tmp_path) + _make_corrupt_sidecar(path, b"not a tiff") + + with pytest.warns(RuntimeWarning, match="Ignoring unreadable sidecar"): + gpu_da = open_geotiff(str(path), gpu=True) + + np.testing.assert_array_equal(gpu_da.data.get(), expected)