You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(data): GeoTIFF and COG rasters the machine can't hold are errors, not crashes (#1291 step 1) - #1370
Part of #1291, step 1 (fallible allocation): the GeoTIFF and COG readers. They follow #1328 and #1338, and come after #1210, which changed the same reader code.
What
Opening a GeoTIFF, a COG window or a turned picture whose bands can't be allocated now fails with "not enough memory for a W × H raster" instead of aborting the process. This holds in the browser and on Windows; on Linux it catches impossible requests, while overcommit can still let a reservation succeed and fail later (as #1291 says). The buffers that now reserve their memory first:
GeoTIFF, read whole: the samples turned into f32, and each band split out of them (bands stored apart or interleaved).
GeoTIFF, read a chunk at a time or decimated: the output bands, on one thread or several, and each chunk's samples turned into f32. A failure on a worker thread stops the read and is returned, the same way a chunk that doesn't decode is.
cargo xtask ci passes locally (fmt, clippy, tests, layers), apart from two tests that fail only on this machine (GDAL and a damaged-Shapefile fixture).
Summary by CodeRabbit
Bug Fixes
Raster and GeoTIFF operations now handle memory allocation failures more reliably, returning an error instead of risking a crash.
Allocation errors identify the raster dimensions involved, making failures clearer.
Raster transformations, GeoTIFF sample conversion, and tile reads use allocation paths that can report failures.
Successful reads and transformations retain their existing results, including output dimensions and initial values.
…, not crashes (opengeos#1291 step 1, GeoTIFF and COG)
The buffers a GeoTIFF read allocates at the raster's size are reserved first, through
raster::{filled, room} (opengeos#1338): a decimated read's bands, the samples turned into f32, the bands
split out of a whole image, a COG window's values (opengeos#1210), and a turned picture or skewed
image's bands. The decoder's own buffer stays bounded by its decoding_buffer_size limit, and a
COG tile by MAX_TILE_SAMPLES. polygon_mask's docs name its new error (from opengeos#1338's review).
No actionable comments were generated in the recent review.
Walkthrough
Raster allocation paths now return errors when output memory cannot be reserved. GeoTIFF sample conversion and decoding propagate allocation errors, and raster builders and COG reads use fallible output allocation.
Changes: Raster allocation handling
Layer / File(s)
Summary
GeoTIFF conversion and decoding crates/data/src/raster/geotiff.rs
Sample conversion reserves output memory using raster dimensions and returns errors on allocation failure. Decimation and whole-image reads use fallible allocations. Parallel decoding propagates conversion errors through its worker failure path.
Other raster output allocations crates/data/src/raster/mod.rs, crates/tiles/src/cog.rs, crates/analysis/src/rasterops.rs
rotate, north_up, and COG reads use fallible output allocation. The polygon_mask documentation now describes the allocation error.
opengeos/RustGIS#1210: Both changes affect read_values_counted_blocking and its output allocation path.
opengeos/RustGIS#1338: This change uses the raster::filled and raster::room helpers and documents the allocation error introduced by that PR.
Suggested reviewers:giswqs, elpidioassinaturas
Merge Risk: Minimal · up to 0556f
No actionable merge-blocking issue is established for the fallible raster allocation changes; mergeability remains subject to normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Title check
The title names the main change: GeoTIFF and COG allocations that fail now return errors instead of crashing the process. The changeset matches this. It converts GeoTIFF sample conversion, band creati…
Description check
The description follows the repository template. It has a Summary (the What and How sections), a Testing section (the cargo xtask ci run, existing reader tests, and a note on the helper test), and a R…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main change: GeoTIFF and COG allocations that fail now return errors instead of crashing the process. The changeset matches this. It converts GeoTIFF sample conversion, band creation, and COG window reads to fallible allocation through raster::filled and raster::room. The title uses a 'feat(data)' prefix and a reference to #1291 step 1, which are acceptable. It is long (97 characters), but it stays specific and descriptive.
Full details: Description check
Explanation
The description follows the repository template. It has a Summary (the What and How sections), a Testing section (the cargo xtask ci run, existing reader tests, and a note on the helper test), and a Related issue reference (Part of #1291). The UI evidence section is missing. The template requires that section, but this change has no visible UI change, and the template says to keep the 'No visible UI change.' line. The author kept that line. The description also lists related PRs and explains scope clearly.
Full details: Docstring Coverage
Explanation
Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 4 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the raster rows, And finds where memory’s limit shows. The bands request their space with care, Errors return when none is there. The tiles begin with NaNs in place, Then hop through reads at measured pace.
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 9978bb05-0527-47ac-88b8-df4d0378f973
Commits
Reviewing files that changed from the base of the PR and between 432e475 and 0556fe7.
Files selected for processing: 1
crates/data/src/raster/geotiff.rs
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.
I read the full diff against the call sites, the filled/grid/room helpers, polygon_mask's mask allocation, and the decimate_parallel error path. I found no crash, silent-loss or wasm-breaking bug. The one inline comment is about error-message accuracy.
Bugs
None found. The allocation changes keep pixel values, sizes and the MAX_SAMPLES limits unchanged. The planar and interleaved band copies reserve exactly n and fill exactly n, so the reserved capacity holds. decimate_parallel propagates a chunk conversion error through fail, the same way a decode error is handled.
Security
None found.
Performance
No regression. The reservation roughly doubles the peak while the source buffer and the converted copy are both alive, but the old code had the same peak (.to_vec() and vec![…; n] already copied). Nothing new is added on the hot path.
Quality
Low, confidence medium: geotiff.rs:874 (posted inline). decimate passes the output size (ow, oh) to to_f32, while the doc says the error names the w × h raster being read.
Nits, grouped and not posted inline: to_f32 binds let at = (w, h); and then passes at everywhere, which could be (w, h) directly. cog.rs uses the fully qualified rustgis_data::raster::filled while the rest of the file imports from rustgis_data::raster. Both are cosmetic.
AGENTS.md
Low, confidence medium: "every crash fix gets a regression test." The helper test a_band_too_large_to_hold_is_an_error covers grid/filled/room, but none of the new call-site error paths (to_f32 in the whole, chunked and parallel reads, the planar/interleaved room calls, rotate, north_up, COG window) is exercised. The PR says this is because no reader can reach the error without a huge file. That is a reasonable explanation, but a test that reaches to_f32's reservation error (for example through a small injectable reservation length) would meet the rule.
Met: no unwrap/expect in non-test code, no unsafe, wasm gating holds (the COG call is behind cfg(not(wasm32)), and filled and room are plain Rust), no new dependency, and the layering is unchanged (tiles already depends on rustgis_data). No changed line exceeds the 140-column rustfmt.toml limit.
Note, not a finding: the PR description says the fix stops the process "aborting". The code comment in raster/mod.rs correctly says Linux overcommit may still succeed and fail later, so the PR body overstates the Linux guarantee.
Prior reviews: none on this PR, so no earlier concerns were skipped.
Change size
Compared: the whole PR against main (merge base e03a6b1 to PR head 432e475).
Reviewed the four changed files (geotiff.rs, raster/mod.rs, tiles/src/cog.rs, analysis/src/rasterops.rs) against the base branch and the staged AGENTS.md. No inline comments posted: nothing I found is worth an inline comment.
Bugs: None found. I checked:
Error propagation in to_f32, in the serial decimate path, and in the decimate_parallel worker (fail then return).
The planar and interleaved branches of read_limited_with_nodata, which keep the same lengths and pixel order.
That n is still used in cog.rs after the change, so no unused-variable warning.
That raster::filled is pub and room is pub(crate), so the tiles crate call and the geotiff calls both resolve. tiles already depends on rustgis_data.
That polygon_mask already returns Result through grid(...)?, so the doc change is accurate.
Security: None found.
Performance: None found. Allocation now happens through try_reserve_exact. Peak memory during conversion is unchanged: the source buffer is still alive while the f32 copy is reserved. (Confidence: high.)
Quality:
geotiff.rs:1218: values.chunks(n) panics if n == 0 (zero-size chunks). This is pre-existing, but the line is rewritten by this PR. I did not verify whether a zero-dimension TIFF can reach it, so treat it as unconfirmed. Confidence: low (about 25%).
AGENTS.md:
"Every crash fix gets a regression test": the new error sites in geotiff.rs, mod.rs and cog.rs have no end-to-end test. The author explained in the PR body that no file reaches them without first hitting MAX_SAMPLES or the window limits, and the helper test covers the error text. This is a reasoned gap rather than a violation. Confidence: medium.
Interface text, asset, layering, and quality-gate rules: no violations seen. The only new text is an error message built with format!, which is the same as the existing no_memory helper and not user interface text.
Earlier review threads: 1 thread (geotiff.rs:874, the to_f32 doc naming the decimated size). It is resolved, and the author's reply addresses it in 0556fe7. I left it alone and did not re-raise the test-coverage point it touched.
Change size
Compared: the whole PR against main (merge base e03a6b1 to PR head 0556fe7).
On the review of 0556fe7: the values.chunks(n) point (n == 0) can't be reached. The tiff decoder refuses an image with a zero width or height when it reads the IFD (InvalidDimensions, tiff 0.11.3, decoder/image.rs:151), before read_limited_with_nodata gets its dimensions, so n is at least 1 there. Leaving it as it is.
Brings in main's 32 commits. Three blocks, each combined:
- tiles cog.rs: main's bounded `decode_tile_counted` (opengeos#1370), then this
branch's login check on the tile read.
- engine project.rs: the recorded (history/task/model) addresses are put
back first, then main's early return and project-size limit (opengeos#1364),
so a refused save never leaves the live project's bindings stripped.
- engine layer.rs: layer.addUrl lists main's `bbox`/`cellSize` (opengeos#1210)
and this branch's `connection`.
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
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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Part of #1291, step 1 (fallible allocation): the GeoTIFF and COG readers. They follow #1328 and #1338, and come after #1210, which changed the same reader code.
What
Opening a GeoTIFF, a COG window or a turned picture whose bands can't be allocated now fails with "not enough memory for a W × H raster" instead of aborting the process. This holds in the browser and on Windows; on Linux it catches impossible requests, while overcommit can still let a reservation succeed and fail later (as #1291 says). The buffers that now reserve their memory first:
f32, and each band split out of them (bands stored apart or interleaved).f32. A failure on a worker thread stops the read and is returned, the same way a chunk that doesn't decode is.layer.addUrl {"bbox"}(#1204) #1210): the window's values inread_values_counted_blocking, which every band of alayer.addUrlwindow goes through.raster/mod.rs.Pixels, sizes and the
MAX_SAMPLESlimit are unchanged.How
to_f32returns aResult: it reserves the converted samples throughraster::room(feat(data): ASCII grids, Zarr stores and pictures the machine can't hold are errors, not crashes (#1291 step 1, part 2) #1338), and theFloat32case still passes the buffer through without copying.raster::filledandraster::room: one implementation for the error message, as in feat(data): ASCII grids, Zarr stores and pictures the machine can't hold are errors, not crashes (#1291 step 1, part 2) #1338.polygon_mask's docs now name its error, as promised in feat(data): ASCII grids, Zarr stores and pictures the machine can't hold are errors, not crashes (#1291 step 1, part 2) #1338's review.Not in this part
tiffcrate allocates it itself, and thedecoding_buffer_sizelimit already bounds it, asmax_allocdoes for the image decoder in feat(data): ASCII grids, Zarr stores and pictures the machine can't hold are errors, not crashes (#1291 step 1, part 2) #1338.MAX_TILE_SAMPLESbounds it.Tests
MAX_SAMPLESand the window limits refuse any file large enough first.a_band_too_large_to_hold_is_an_error(feat(data): ASCII grids, Zarr stores and pictures the machine can't hold are errors, not crashes (#1291 step 1, part 2) #1338) covers the helpers themselves.cargo xtask cipasses locally (fmt, clippy, tests, layers), apart from two tests that fail only on this machine (GDAL and a damaged-Shapefile fixture).Summary by CodeRabbit