zarrs / zarrs/zarrs-python

Filesystem store is not wrapped in AtomicWriteStorageAdapter, so a killed write leaves a truncated file

Open
#185 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
80
Forks
8
Avg merge
21h 44m
Merged PRs (30d)
5

Description

Summary

The filesystem store is constructed bare, so writes are not atomic:

// src/store/filesystem.rs
impl TryInto<ReadableWritableListableStorage> for &FilesystemStoreConfig {
    fn try_into(self) -> Result<ReadableWritableListableStorage, Self::Error> {
        let store = Arc::new(
            FilesystemStore::new_with_options(self.root.clone(), self.opts.clone())
                .map_py_err::<PyRuntimeError>()?,
        );
        Ok(store)
    }
}

zarrs/zarrs#421 added AtomicWriteStorageAdapter, which writes through <key>.tmp and renames. Wrapping the store in it would mean a process killed mid-write leaves the previous complete value rather than a truncated one.

Why this matters to us

We run per-position reconstruction jobs on a preemptible Slurm partition, writing sharded OME-Zarr to Lustre through zarr-python with this codec pipeline. A job killed mid-write left a truncated shard, and since our shard grid rounds up past the array bound, every later write to that shard had to read it back first and died on its checksum:

RuntimeError: the checksum is invalid
RuntimeError: The encoded shard is smaller than the expected size of its index.

Deterministic, so retries and resumes failed identically — one preemption could take out a whole run. Details in czbiohub-sf/iohub#415.

We have worked around it downstream (deleting the shards a write owns before writing, czbiohub-sf/iohub#455), but atomic writes fix it at the source: the truncated file would never exist.

Suggested change

Wrap the store, probably behind an option rather than unconditionally:

use zarrs::storage::storage_adapter::atomic_write::AtomicWriteStorageAdapter;

let store = Arc::new(FilesystemStore::new_with_options(...)?);
Ok(if self.atomic_writes { Arc::new(AtomicWriteStorageAdapter::new(store)) } else { store })

surfaced the same way as the existing knobs (validate_checksums, direct_io, chunk_concurrent_*) via CodecPipelineImpl and zarr.config.

No dependency bump appears necessary: zarrs = "0.23.6" requires zarrs_storage = "^0.4.3", and the adapter shipped in zarrs_storage 0.4.5, so it already resolves.

Things worth weighing

  • Cost. Every write becomes a temp write plus a rename, and #421 notes an atomic full-value fallback for partial writes. For whole-shard writes like ours that is close to free, but it is not free in general — which argues for opt-in, at least initially.
  • direct_io. Worth checking the interaction, since that path is O_DIRECT with page-size alignment and the temp-then-rename indirection is new to it.
  • Other backends. obstore.rs and http.rs are out of scope here; AtomicRenameStorageTraits is implemented for FilesystemStore, which is the backend where a torn file is observable this way.

Happy to open a PR if the shape above looks right — mainly wanted to check whether opt-in or default-on is preferred before writing it.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/store/filesystem.rs at the TryInto implementation, then inspect AtomicWriteStorageAdapter and the existing option flow through CodecPipelineImpl and zarr.config. Check the interaction with direct_io before choosing the opt-in shape. Done means the filesystem store can use atomic writes without changing other backends or the existing behavior when disabled.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, rust
Domain
backend
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.