Skip to content

Record failed image reads and mask failures in chunk_status; fix hang when a worker dies early - #2

Closed
NewGraphEnvironment wants to merge 6 commits into
masterfrom
99-chunk-status-read-failures
Closed

NewGraphEnvironment wants to merge 6 commits into
masterfrom
99-chunk-status-read-failures

Conversation

@NewGraphEnvironment

Copy link
Copy Markdown
Owner

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.

Built and reviewed here first (as #1 was for appelmar#110) so it can go upstream as is; the upstream PR is drafted, not posted.

Relates to NewGraphEnvironment/drift#99
Relates to NewGraphEnvironment/sred-2025-2026#16

🤖 Generated with Claude Code

https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN

NewGraphEnvironment and others added 6 commits October 14, 2026 05:43
write_ncdf() records a per-chunk status, but only a band image that fails
to OPEN was ever recorded as non-OK. Three other failures produced a chunk
computed from partial input with status OK:

- gdalwarp_client::warp() ignored the return of ChunkAndWarpImage() (and
  logged a failure only at DEBUG), returning the INIT_DEST=nan output as
  if it were data. A band whose tile read fails after a successful open
  (corrupted tile, refused or throttled HTTP range request) was silently
  dropped from the chunk; for a MASK band the scene was used unmasked,
  since a NaN mask value never matches a mask class. warp() now warns and
  returns nullptr, so read_chunk()'s existing `!gdal_out` branches mark
  the chunk INCOMPLETE (ERROR when strict).
- A mask image that fails to open `continue`d without set_status(); it now
  sets INCOMPLETE, and the strict branch returns an ERROR chunk like the
  band path does.
- chunk_data::read_ncdf() returned without a status when the main process
  could not open a worker's chunk file, found its dimensions or variable
  missing, or failed nc_get_var_double() (whose return was unchecked, so
  an uninitialised buffer could be merged as data). These now set ERROR
  and discard the buffer. The legitimately empty chunk still returns its
  recorded status, and now closes the file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
Builds a two-date collection of single-band COGs (a band and an SCL-style
mask) from dummy cubes, then breaks the second date three ways: a band
file whose first tile is corrupted with the IFD intact, a deleted mask
file, and a mask file with a corrupted tile. Each must yield a non-OK
chunk_status from write_ncdf() at parallel 1 and 2, and a clean read must
stay OK. The corrupted files are asserted to still open (via sf, when
installed), so the read cases cannot silently test an open failure.

Against 0.7.5 the six broken-read expectations fail; with the previous
commit all 12 pass, and the full suite is 137/137.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
Follow-up hardening in the same failure paths:

- warp_simple()/warp_complex(): a failed GDALWarpOperation::Initialize()
  now counts as a failed warp instead of logging and running
  ChunkAndWarpImage() on an uninitialised operation.
- When reopening the source at an overview level fails, the source had
  already been closed, leaving hSrcDS pointing at freed memory. Reopen
  it at full resolution, as the existing warning says; if that also
  fails, the warp fails (nullptr) rather than reading from the closed
  dataset.
- chunk_data::read_ncdf(): check nc_inq_dimlen(); a failed query left
  the sizes at (size_t)-1 and passed the emptiness check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
- clean controls at parallel 1 and 2 must stay OK: a source in another
  CRS (EPSG:32610 into an EPSG:32609 view), a view only partly covered by
  the images, and a coarser view that reads overviews, so the stricter
  warp error handling cannot refuse good reads
- the second date is now a cloud scene (B04 = 900, SCL = 9), so a failed
  mask read that leaks unmasked values is visible in the output, not
  only in the status: under 0.7.5 the corrupted-mask case returns 900
- strict mode (incomplete_ok = FALSE) with a mask that fails to open must
  give ERROR
- fixture files are named explicitly: write_tif() returns every file in
  the directory with the prefix

30 assertions; 10 fail against 0.7.5, all pass with this branch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
After a worker fails, chunk_processor_multiprocess::apply() prints each
worker's log with `while (!wlogf.eof()) getline(...)`. A worker that died
before creating its log file (killed by a signal at startup) leaves the
ifstream in a failed state that never reaches eof(), so the main process
spun at 100% CPU forever: write_ncdf() neither returned nor raised.
Sampled while hung: no worker processes left, main thread looping in
apply(). Loop on getline() instead.

A worker killed mid-computation (after its log exists) already raised
promptly; only the early death hung. The test runs the computation in a
child R process with a worker script that SIGKILLs worker 1 at startup,
under system2(timeout =), so a regression fails instead of hanging.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
… test

Review round 1:

- The previous commit reopened the source at full resolution when the
  overview could not be opened, but a band-selection VRT (empty
  description) cannot be reopened either, and `in` was dereferenced
  (GetRasterCount) before the new NULL check ran. Open the overview into
  its own handle and close the original only when that succeeds, so the
  original is never lost; the NULL check is no longer needed.
- The "reprojected read is OK" control used a view that did not overlap
  the EPSG:32610 fixture (0 non-NA cells), so no reprojecting warp ran.
  Use the overlapping extent and assert that the read has data.

A two-band image with the mask in band 2 still segfaults a worker, at
full resolution as well as through overviews and on 0.7.5 alike; that is
a separate, pre-existing defect and is not addressed here.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PRhUJsuKABLfpBGktPoiBN
@NewGraphEnvironment

Copy link
Copy Markdown
Owner Author

Superseded by #3, #4 and #5: the same fixes split by concern so each can go upstream on its own, with its own test. The chunk_data::read_ncdf() hardening in cube.cpp is not carried into any of them (no test can reach it); it stays on this branch, 99-chunk-status-read-failures, which is kept.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant