Skip to content

Pin HTTP/range COG byte-budget contract (#2293)#2298

Merged
brendancol merged 2 commits into
xarray-contrib:mainfrom
brendancol:worktree-agent-ac03a7733d4d60c45
May 22, 2026
Merged

Pin HTTP/range COG byte-budget contract (#2293)#2298
brendancol merged 2 commits into
xarray-contrib:mainfrom
brendancol:worktree-agent-ac03a7733d4d60c45

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Part of #2286 (COG readiness rollout). Adds xrspatial/geotiff/tests/test_http_cog_range_contract_2286.py to lock down the HTTP COG reader's transport behaviour with explicit byte-count and range-count assertions. Tests only; no production code changes.

Coverage rows

  • Windowed tile and multi-tile reads fetch only intersecting tiles. No read_all fallback, total bytes stay below the file size, and the byte budget is bounded by the windowed footprint.
  • Overview reads pull the overview IFD's tiles, not the full-res pixel data. For a factor-2 overview the byte budget lands under 75% of the base read.
  • band= on multi-band chunky COGs returns the same pixels as the local path with bounded reads, both alone and combined with window=.
  • Dask COG reads parse IFDs once across all chunk tasks (O(1) header GETs in chunk count), checked across two chunk granularities.
  • Truncated buffers, malformed IFD chains, and short pixel bodies all close the HTTP source via the try/finally guard from geotiff: _read_cog_http skips source.close() when tile fetch raises #1816 and raise a clear exception rather than hanging.
  • coalesce_ranges respects the configured max_coalesced_range_bytes and the default cap (COG range coalescing can turn safe tile reads into huge over-fetches #2266). split_coalesced_bytes round-trips bytes under the cap. The real _HTTPSource.read_ranges_coalesced path propagates the cap to wire-level GETs.

Fixture choices

  • 256x256 random-pixel COGs so deflate cannot collapse the file below the 16 KiB header probe. Otherwise the "windowed read < file size" assertion would be vacuous.
  • Sidecar discovery is monkeypatched off so the contract tests count exactly the GETs the issue is about. Sidecar behaviour for the chunked HTTP path is already covered by test_remote_sidecar_chunked_2239.py.
  • Reuses the _RecordingHTTPSource / _CloseCountingSource patterns from the sibling test files referenced in the issue (test_http_cog_coalesce.py, test_cog_http_close_on_error_1816.py, test_http_stripped_window_max_pixels_issue_A_1842.py).

Test plan

  • pytest xrspatial/geotiff/tests/test_http_cog_range_contract_2286.py -v -- 15 passed locally
  • Smoke run with neighbouring HTTP files (test_http_cog_coalesce.py, test_http_window_band_planar_1669.py) -- 46 passed, no regressions

Closes #2293.

Add test_http_cog_range_contract_2286.py to lock down the HTTP COG
reader's transport behaviour with explicit byte-count and
range-count assertions. Tests only; no production code changes.

Coverage rows:

* Windowed tile and multi-tile reads fetch only intersecting tiles
  (no read_all fallback; total bytes below file size and bounded by
  the windowed footprint).
* Overview reads pull the overview IFD's tiles, not the full-res
  pixel data.
* band= on multi-band chunky COGs returns correct pixels with
  bounded reads, alone and combined with window=.
* Dask graphs parse IFDs once across all chunk tasks (O(1) header
  GETs in chunk count).
* Truncated buffers, malformed IFD chains, and short pixel bodies
  close the HTTP source via the try/finally guard and raise a
  clear exception rather than hanging.
* coalesce_ranges respects the configured and default
  max-merged-range caps; split_coalesced_bytes round-trips bytes
  under the cap.

Closes xarray-contrib#2293.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 22, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review: PR #2298

Single new test file, 864 lines. All 15 tests pass locally. Read the file in full; here is what stood out.

Blockers

None.

Suggestions

  • xrspatial/geotiff/tests/test_http_cog_range_contract_2286.py:43 -- _parse_cog_http_meta is imported but never used. pyflakes flags it. Drop the import to keep the test file lint-clean.
  • xrspatial/geotiff/tests/test_http_cog_range_contract_2286.py:112-120 -- _serve is defined but never called. The two tests that stand up loopback servers (test_short_body_during_pixel_fetch_closes_source and test_loopback_end_to_end_windowed_byte_budget) each inline their own handler/server setup. Either delete the helper or refactor the two sites to use it. Leaving it as dead code adds 9 lines of misleading scaffolding.

Nits (stale comments after the fixture resize)

The fixtures were bumped from 64x64 / tile=16 to 256x256 / tile=32 to clear the 16 KiB header probe, but a handful of docstrings and inline comments still reference the old sizes. Worth scrubbing:

  • test_http_cog_range_contract_2286.py:247 -- docstring says "A 16x16 window aligned to one tile" but the window is 32x32.
  • test_http_cog_range_contract_2286.py:292 -- "A window that touches 2x2=4 tiles must not fetch all 16 tiles" but the file has 64 tiles, not 16. The comment two lines below (>=64 separate GETs) is correct; the docstring is stale.
  • test_http_cog_range_contract_2286.py:421 -- "16x16 window aligned to one tile" but the window is 32x32.
  • test_http_cog_range_contract_2286.py:483 -- docstring says "two chunk granularities (8 and 16)" but the run uses chunks=32 and chunks=64.

Unused locals

  • test_band_selection_multiband_chunky_bounded_reads (line 381) and test_band_selection_with_window_bounded_range_count (line 409) unpack expected from the fixture but never use it. Replace with _expected or drop the unpack to silence linters.

What looks good

  • Coverage matches the issue checklist row-for-row: windowed reads, overview reads, band+window, dask once-per-graph, close-on-error (truncated buffer, malformed IFD, short pixel body), and the coalesce cap. All six required assertions have at least one test row.
  • Fixture sizing is deliberate (256x256 random pixels so deflate cannot collapse below the header probe) and the rationale is documented in the docstrings.
  • _no_sidecar_probe autouse fixture is the right call here -- sidecar discovery on a 200-everything mock server would skew the byte-count assertions, and the issue explicitly says sidecar behaviour belongs in test_remote_sidecar_chunked_2239.py.
  • _RecordingHTTPSource extends _HTTPSource so it inherits read_ranges_coalesced, which is what makes test_coalesced_get_size_capped_on_real_http_source exercise the real code path instead of a stub.
  • The expected = _Handler.payload[:64] style upper bounds (file size, header + tile budget, 75% of base for overviews) are tight enough to catch a regression that pulls everything but loose enough to survive codec/compression changes. The hard bound is always "less than file size"; the soft bound is the early-warning indicator.
  • Short-body test accepts both OSError and urllib3.exceptions.ProtocolError so the test does not become brittle if read_range's wrapping changes.

Checklist

  • Tests only; no production code touched (per #2293 scope).
  • All six required assertions present.
  • Reuses fixtures from the sibling files listed in the issue.
  • Deterministic (in-process mock + loopback server, no real network).
  • Suite green on CPU CI -- 15 passed locally; 46 passed across this file + two sibling files combined.
  • No real production bugs uncovered; no xfail rows needed.

Drop unused _parse_cog_http_meta import (flagged by pyflakes), drop
the unused _serve helper that the two loopback tests bypassed inline,
update stale docstrings/comments left over from the 64x64 -> 256x256
fixture resize, and rename unused `expected` unpacks to `_expected`.

No assertion changes; all 15 tests still pass.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round 2 (post-fix)

All findings from round 1 addressed in 1a82640:

  • Dropped unused _parse_cog_http_meta import. pyflakes now clean.
  • Removed unused _serve helper. The two tests that stand up loopback servers (test_short_body_during_pixel_fetch_closes_source, test_loopback_end_to_end_windowed_byte_budget) both inline their setup so the helper was dead code.
  • Fixed four stale docstrings/comments referring to the old 64x64 / tile=16 fixture sizes (lines 247, 292, 421, 483). All four now match the actual 256x256 / tile=32 fixture and the chunks=32/64 dask runs.
  • Renamed expected to _expected in the two band-selection tests where the fixture's reference array is not used (the tests compare against open_geotiff(path, band=...) directly).

15 tests still pass locally; no assertion logic touched.

Nothing else to flag. Ready for CI.

@brendancol brendancol marked this pull request as ready for review May 22, 2026 12:41
@brendancol brendancol merged commit b0131f7 into xarray-contrib:main May 22, 2026
4 of 5 checks passed
brendancol added a commit that referenced this pull request May 22, 2026
* Promote local COG contract to stable (#2300)

Flip the local COG read and write paths to the stable tier in
``xrspatial.geotiff.SUPPORTED_FEATURES``:

- ``SUPPORTED_FEATURES['writer.cog']``: advanced -> stable.
- ``SUPPORTED_FEATURES['reader.local_cog']``: advanced -> stable.
- ``SUPPORTED_FEATURES['reader.http_cog']`` stays advanced; the inline
  comment now spells out why (range fetching, redirect handling, SSRF
  filter, cache / retry behaviour not yet contracted).

Document the stable COG contract:

- ``xrspatial/geotiff/_attrs.py`` carries a block comment above
  ``SUPPORTED_FEATURES`` describing what the stable contract guarantees
  and what stays advanced.
- ``docs/source/reference/geotiff.rst`` grows a *Stable COG contract*
  section at the top of the page plus an *Outside the stable contract*
  list.
- The COG overview notebook (``examples/user_guide/52_COG_Overview_
  Generation.ipynb``) carries a short note that the examples sit
  inside the stable contract while HTTP / GPU / BigTIFF stay outside.
- ``CHANGELOG.md`` records the promotion under Unreleased.

Backed by the writer compliance suite (#2292), the cross-backend
parity gate (#2293), and the per-tile byte-budget contract
(#2294 / #2298). The full 5169-test geotiff suite passes locally.

Closes #2300.

* Address self-review: bump RST underline above title length
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

COG readiness: HTTP/range byte-budget contract tests (#2286 PR 3/6)

1 participant