Narrow except Exception in read_vrt to OSError/ValueError/struct.error (#1670)#1675
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR narrows the exception handler in the GeoTIFF VRT reader so that default-mode “skip this tile” behavior no longer swallows arbitrary bugs (e.g., RuntimeError, MemoryError), while keeping strict-mode (XRSPATIAL_GEOTIFF_STRICT=1) behavior unchanged.
Changes:
- Narrow
read_vrt’s per-sourceexcept Exceptiontoexcept (OSError, ValueError, struct.error). - Add a regression test module asserting propagation vs warn-and-continue behavior for several exception types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xrspatial/geotiff/_vrt.py |
Narrows the exception allowlist for per-source reads inside VRT assembly. |
xrspatial/geotiff/tests/test_vrt_narrow_except_1670.py |
Adds regression tests for the new exception-handling contract (default vs strict mode). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+367
to
+371
| src.filename, | ||
| window=(src_r0, src_c0, src_r1, src_c1), | ||
| band=src.band - 1, # convert 1-based to 0-based | ||
| ) | ||
| except Exception as e: | ||
| except (OSError, ValueError, struct.error) as e: |
This was referenced May 12, 2026
brendancol
added a commit
that referenced
this pull request
May 12, 2026
Addresses Copilot feedback on PR #1675: the narrowed (OSError, ValueError, struct.error) catch in read_vrt did not cover codec-specific exceptions raised from corrupt compressed tiles, which meant a single bad deflate or ZSTD payload aborted the whole mosaic instead of warning-and-skipping like the historical behaviour. Expand the allowlist via a small helper that returns the tuple of codec-library decode errors worth swallowing (zlib.error always; zstandard.ZstdError when the optional dep is installed). RuntimeError is deliberately left out since lz4 frame, LERC and glymur all raise it for both corruption and real bugs.
#1670) The VRT source-read fallback in ``_vrt.py`` caught every ``Exception`` subclass, so a ``RuntimeError`` or ``MemoryError`` thrown by future code paths would silently turn into a warn-and-skip "hole" in the mosaic. The intent was always "this one source file is unreadable, fall back to fill values"; widen-to-Exception leaked real bugs. Restrict the catch to the families ``read_to_array`` actually documents/raises: ``OSError`` (and ``FileNotFoundError`` / ``PermissionError``) for I/O, ``ValueError`` for the typed parse errors, and ``struct.error`` which still leaks from a couple of parse paths until that gets cleaned up. Strict mode (#1662) and the warning text are unchanged.
Addresses Copilot feedback on PR #1675: the narrowed (OSError, ValueError, struct.error) catch in read_vrt did not cover codec-specific exceptions raised from corrupt compressed tiles, which meant a single bad deflate or ZSTD payload aborted the whole mosaic instead of warning-and-skipping like the historical behaviour. Expand the allowlist via a small helper that returns the tuple of codec-library decode errors worth swallowing (zlib.error always; zstandard.ZstdError when the optional dep is installed). RuntimeError is deliberately left out since lz4 frame, LERC and glymur all raise it for both corruption and real bugs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
read_vrtwrapped each source read inexcept Exception. In default mode the "skip this tile" fallback would swallow anyExceptionsubclass, includingRuntimeErrorandMemoryErrorfrom unrelated bugs. The catch was only ever meant for unreadable source files.Restrict the catch to what
read_to_arrayactually raises for an unreadable or malformed source:OSErrorand subclasses (FileNotFoundError,PermissionError) for I/O failuresValueErrorfor the typed parse errors fromparse_header/parse_ifdstruct.error, which still leaks from a couple of parse paths until those get cleaned upThe strict-mode escape from #1662 and the warning text are unchanged.
Test plan
test_vrt_narrow_except_1670.pywith 9 tests:RuntimeErrorpropagates in both default and strict modes (previously swallowed in default)MemoryErrorpropagates in default modeFileNotFoundErrorwarns and continues in default, re-raises in strictValueErrorwarns and continues in default, re-raises in strictstruct.errorwarns and continues in defaultPermissionErrorwarns and continues in defaultDID NOT RAISE <class 'RuntimeError'>Closes #1670