fix(ngff): sync a unit's shards before recording it as done - #476

Related issue

None filed. Found by debugging a pipeline that failed repeatedly on the same shards. Related upstream report on the layer below: zarrs/zarrs#462.

Summary

WriteUnit.complete recorded a unit as finished if its shard files merely existed:

written = [self._key(path) for path, _ in self.shards if path.exists()] if wrote else []

Nothing forced those bytes to disk. Neither iohub, nor zarr, nor the zarrs codec pipeline calls fsync, and Rust's File discards the result of close when it drops, so a write-back error is dropped rather than raised. The marker is tens of bytes and commits almost immediately; the shards it vouches for are hundreds of megabytes still draining to a networked filesystem. A kill in between — or a swallowed EIO — leaves a .done marker beside data that never landed.

unit_is_complete cannot catch that after the fact. Its probe reads one element per shard, which decodes a single inner chunk: on the geometries our reconstruction pipelines produce that is 1 of 354 (0.28%) for one store and 1 of 132 (0.76%) for another. Damage anywhere but the shard origin reads back clean, so the unit is skipped for good and the corruption becomes permanent.

This PR:

• Syncs a unit's shards before recording it. An interruption before the barrier leaves no record, so the unit is recomputed. An I/O error during it propagates instead of being recorded as success.
• Records completion only after the store is closed, so nothing the codec pipeline buffered in user space sits outside the barrier.

_fsync_file opens read-write rather than read-only because Windows refuses to commit a handle not opened for writing; _fsync_dir is a no-op off POSIX, where losing a marker rename costs a recomputation at worst and never a wrong skip.

The change is to the write path only. Recovering stores already damaged this way is deliberately out of scope here.

What this looked like in practice

Shards of several hundred MB, each full length with a valid CRC'd shard index and a handful of garbled inner chunks. Each was written by a job that was preempted or cancelled, or that exited 0 while the storage was returning EIO to other jobs. In one case the retry "completed" the position in 2m28s, skipping 2280 units — including the corrupt ones — and the damage then survived several runs before a downstream stage finally read those voxels.

Testing

pytest tests/ngff/test_ngff_utils.py — 77 passed. Two failures, test_apply_transform_to_czyx_and_save and test_apply_transform_to_tczyx_and_save, are pre-existing on main and unrelated; I confirmed they fail identically on an unmodified tree.

Three new tests. The first two fail on main and pass here:

• test_complete_syncs_every_shard_before_recording_the_unit — the unit's own marker does not exist while its shards are being synced
• test_a_shard_that_cannot_be_synced_is_not_recorded_as_done — an EIO during the barrier propagates and leaves no .done
• test_resume_skips_corruption_away_from_the_shard_origin — pins the default probe's limit deliberately. It passes before and after, and is here so the assumption is written down: the probe is not an integrity check, which is why the barrier is needed rather than a post-hoc verification.

The existing _tear helper truncates a shard, which destroys the trailing index and so is caught by the origin probe immediately. The new _corrupt_inner_chunk_away_from_origin helper reproduces the real failure instead: full length, index intact, one inner chunk garbled. It selects its target by position in the shard index — C order over the inner chunk grid per the v3 sharding spec, so entry zero is always the origin's chunk — because choosing by file offset is unstable when the pipeline assembles chunks concurrently.

Ran the new tests repeatedly and the full module several times to check for order dependence and flakiness.

Also verified downstream: biahub's suite passes against this commit (242 passed, 1 skipped).

Checklist

• I added or updated tests, or this change does not require tests.
• I updated the documentation, or this change does not require documentation.
• I confirmed this change does not include confidential or sensitive information.

🤖 Generated with Claude Code