Rework save system to use ZarrV3#260
Open
JoeyBF wants to merge 9 commits into
Open
Conversation
Rebases the zarrs-save-rework branch (6 save-system commits) onto master. Master had since split ext::secondary into per-primary submodules (SpectralSequences#254) and made the secondary lift entry points return Result (SpectralSequences#241, SpectralSequences#242), so the conflicts were confined to ext/src/secondary.rs. Resolution: - Adopt the store-based save API (store.read/write/exists/delete) in the shared SecondaryHomotopy / SecondaryLift code while preserving master's Result-returning try_compute_homotopy_step. - Drop the stale monolithic duplicates of SecondaryResolution, SecondaryResolutionHomomorphism, and SecondaryChainHomotopy; those types now live in submodules under resolution.rs / chain_homotopy.rs / resolution_homomorphism.rs. - Remove the per-new() SaveKind::secondary_data() create_dir loops from the moved constructors: the reworked store creates its shard arrays lazily on first write, and create_dir no longer exists on SaveKind. Dropped from the original branch: the throwaway "experimental rayon build" commit (patched rayon to a personal fork) and the earlier "tracing to parallel guard" commit (master ships a superior version). All 7 save_load_resolution tests and the ext lib unit tests pass.
Two hot-path improvements to ZarrSaveStore that remove overhead the rebase inherited, with no change to the on-disk format or public API. 1. Cache opened Array handles (`arrays: DashMap<SaveKind, Arc<ShardArray>>`). Previously every read/write/delete called `zarrs::array::Array::open`, which re-reads and re-parses the array's `zarr.json` through the storage backend on each call. The handle holds no chunk data and its methods take `&self`, so a cached `Arc<Array>` is safe to share for concurrent reads and (shard-serialized) writes. Now the metadata is parsed at most once per kind per store. Read/delete of a not-yet-created kind still returns None/Ok as before. 2. Key the write lock by `(kind, shard coords)` instead of by kind alone. The zarrs read-modify-write contract only requires serializing writes that share a shard; the old per-kind lock needlessly serialized every write of a kind across the whole store. Writes to different shards touch disjoint chunk files, so per-shard locking restores the cross-shard parallelism a parallel resolution depends on while still honouring the contract. Kept `with_concurrent_target(1)` so the sharding codec stays sequential and the rayon-join deadlock the original guarded against cannot occur. Verified on S_2 through-stem (n=70, s=40), release + concurrent, 4 cores: best-of-3 wall time 15.1s -> 13.0s (~14% faster) with identical Ext output. Save+resume roundtrip re-checked for both the standard and nassau backends; all lib and save_load_resolution tests pass.
…_LEVEL The rebased branch hardcoded zstd level 19 for the stream tier (res_qi, nassau_qi), which is on the hot write path — every differential quasi-inverse is compressed there. Benchmarked against master's uncompressed old format (S_2 through-stem n=70 s=40, release + concurrent, 4 cores, best of 3): master (old, uncompressed): 2.55s write, 51M on disk (real bytes) zarr, zstd level 19: 13.0s write, 23M zarr, zstd level 3: 3.65s write, 24M Level 19 was ~5x slower than master for a 4% smaller store than level 3 on this dense, high-entropy data. Level 3 (zstd's own default) keeps nearly all the compression — still less than half of master's on-disk size — at a fraction of the write cost, cutting the regression from ~5x to ~1.4x. The read/resume path is unchanged and already at parity with master (~0.10s). Compression still matters for very large runs, so the level is read once from the EXT_SAVE_ZSTD_LEVEL environment variable (clamped to zstd's [1, 22]; unparseable values warn and fall back to the default). Set e.g. EXT_SAVE_ZSTD_LEVEL=19 when the on-disk footprint, not save time, is the binding constraint.
📝 WalkthroughWalkthroughThis PR replaces file-based persistence with a Zarr v3-backed store, updates resolution, homotopy, secondary, and Nassau save/load paths to use store reads and writes, and adjusts tests and dependencies for the new format. ChangesZarr save store migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Resolution
participant ZarrSaveStore
participant Bitcode
Resolution->>ZarrSaveStore: read(SaveKind::Differential, b)
ZarrSaveStore-->>Resolution: bytes
Resolution->>Bitcode: deserialize DifferentialPayload
Resolution->>ZarrSaveStore: write_res_qi(...)
Resolution->>Bitcode: serialize kernel / augmentation
Resolution->>ZarrSaveStore: write(SaveKind::Kernel, ...)
Resolution->>ZarrSaveStore: delete(previous kernel)
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
A line beginning with "+ serde_json parse" was parsed as a markdown "+" list marker, so clippy (-D warnings in CI's lint job) flagged the following lines as list items without indentation. Reword to "and a serde_json parse". No code change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/src/chain_complex/chain_homotopy.rs`:
- Around line 52-59: The subgroup path built in chain_homotopy::save_dir
currently interpolates raw values from left.name() and right.name(), which can
alter the Store layout or collide with other pairs. Update the path construction
in the homotopy save logic to encode/sanitize both names before passing them
into parent.subgroup, and keep the change localized around the save_dir creation
so the homotopy lookup and persistence use the same safe naming scheme.
- Around line 181-187: The cached homotopy payload handling in `ChainHomotopy`
is unsafe because `save_dir.store()` reads raw bytes without validating the
encoded shape. Update the `read` path to store and verify `num_gens` and
`target_dim` alongside the payload, then in the decode block reject/recompute on
any mismatch before constructing `outputs`. Also replace the `unwrap()`-based
decoding in this `SaveKind::ChainHomotopy` branch with explicit error handling
and ensure the cursor has no trailing bytes after `FpVector::from_bytes`
completes.
In `@ext/src/resolution_homomorphism.rs`:
- Around line 237-257: The chain-map cache serialization in the load/save path
is missing the saved row width, so `FpVector::to_bytes` data is being read back
with the current `fx_dimension` instead of the original one. Update the
`save_dir.store()` logic in the chain-map handling to persist a small header
with the saved `fx_dimension` before the row bytes, and in the corresponding
read branch deserialize that header first and use it to parse each `FpVector`;
if the stored width differs from the current target, skip or invalidate the
cached entry. Apply the same header format in both write locations around the
`SaveKind::ChainMap` calls so `add_generators_from_rows_ooo` always sees
correctly framed rows.
In `@ext/src/resolution.rs`:
- Around line 140-142: The cache binding in resolution setup only uses
bind_to_algebra, so a store can be reused across different ChainComplex
instances with the same algebra metadata. Update the save_dir.store() path to
bind and validate against the resolved complex as well, using a
ChainComplex/resolution fingerprint (or equivalent per-complex subgroup key)
before any reads or writes, and keep the existing algebra metadata check in
place.
- Around line 413-426: The differential shard handling in
resolution::add_generators_from_rows currently only checks target_cc_dimension
after deserializing DifferentialPayload, so stale or corrupt data can still be
applied. In the logic that reads DifferentialPayload, add validation for the
full payload before mutating state: verify target_res_dimension matches the
expected resolution dimension, and ensure the differential and chain_map row
vectors are consistent with num_gens and the block size before calling
self.add_generators, current_differential.add_generators_from_rows, and
current_chain_map.add_generators_from_rows. Use the existing DifferentialPayload
and add_generators_from_rows flow to locate the checks.
In `@ext/src/save.rs`:
- Around line 377-386: The subgroup path construction in subgroup() is using raw
name components, which can allow "/" or ".." to alter the intended Zarr
hierarchy. Update subgroup() to encode or strictly validate the supplied name
before building group and group_path, and make sure any downstream use in
ensure_intermediate_groups() and GroupBuilder::new().build() operates on the
sanitized component rather than the raw input.
- Around line 1228-1232: Reject malformed signature payloads before decoding in
the signature parsing path. In the code that builds sig from payload using
chunks_exact(2), add an explicit payload length validation for even length and
return an error if payload.len() % 2 != 0, so a corrupt Signature command cannot
be partially decoded. Keep the fix local to the parsing logic in save.rs around
the signature payload handling.
- Around line 1047-1048: The ResQi row handling in `apply()` and
`into_quasi_inverse()` is still using `assert!` on the result of `next_row`,
which can panic on truncated or partially written data. Replace the panic path
with `anyhow::ensure!` (or equivalent fallible error propagation) at the `got`
check so these APIs return a proper error through the existing bidegree/load
flow instead of aborting. Keep the change localized to the `next_row`/pivot-row
validation logic in the affected ResQi reader paths.
- Around line 1401-1405: The SaveDirectory conversion currently panics in the
From<Option<PathBuf>> implementation by calling
ZarrSaveStore::create(...).expect(...), which prevents new_with_save and related
anyhow::Result flows from reporting store creation failures. Replace this with a
fallible conversion path such as TryFrom<Option<PathBuf>> or a dedicated
constructor, and update the SaveDirectory / new_with_save call site to propagate
the ZarrSaveStore::create error instead of unwrapping it.
- Around line 574-578: The stream-kind guard currently uses debug_assert! in the
write/read/delete paths, so misuse of SaveKind::ResQi or SaveKind::NassauQi is
only caught in debug builds. Replace these with runtime validation in the
affected save methods (including write, read, delete, and the exists probe) so
fallible operations return an error instead of silently targeting the wrong
shard-tier layout. Use the existing SaveKind checks in the relevant methods to
reject stream kinds explicitly and keep exists() from probing the shard layout
when given a stream kind.
- Around line 337-356: The store validation in the save/load path only checks
algebra_magic before reusing cached data, so it can accept metadata created for
a different prime. Update the logic in the algebra_magic handling block to also
compare the stored prime from attrs against the current prime before returning
Ok(()), and include the prime mismatch in the same bailout path used for the
existing algebra mismatch; use the existing symbols algebra_magic,
algebra_prefix, prime, and the attrs lookups in ext/src/save.rs to locate the
check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3bd243b-fb2c-425d-a092-fc42f1917e6c
📒 Files selected for processing (11)
ext/Cargo.tomlext/crates/fp/src/matrix/quasi_inverse.rsext/crates/fp/src/vector/fp_wrapper/mod.rsext/src/chain_complex/chain_homotopy.rsext/src/chain_complex/mod.rsext/src/nassau.rsext/src/resolution.rsext/src/resolution_homomorphism.rsext/src/save.rsext/src/secondary.rsext/tests/save_load_resolution.rs
💤 Files with no reviewable changes (1)
- ext/src/chain_complex/mod.rs
Comment on lines
+52
to
+59
| let save_dir = if !left.name().is_empty() | ||
| && !right.name().is_empty() | ||
| && let Some(parent) = left.source.save_dir().store() | ||
| { | ||
| let mut save_dir = left.source.save_dir().clone(); | ||
| save_dir.push(format!("massey/{},{}/", left.name(), right.name())); | ||
|
|
||
| SaveKind::ChainHomotopy | ||
| .create_dir(save_dir.write().unwrap()) | ||
| .unwrap(); | ||
|
|
||
| save_dir | ||
| SaveDirectory::Store( | ||
| parent | ||
| .subgroup(&format!("homotopies/{}__{}", left.name(), right.name())) | ||
| .expect("Failed to create homotopies subgroup"), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify subgroup path semantics and whether homomorphism names are already constrained/encoded.
set -euo pipefail
CHAIN_HOMOTOPY_RS="$(fd '^chain_homotopy\.rs$' ext/src | head -n1)"
SAVE_RS="$(fd '^save\.rs$' ext/src | head -n1)"
HOM_RS="$(fd '^resolution_homomorphism\.rs$' ext/src | head -n1)"
rg -n -C4 'subgroup\(|homotopies/.*__|pub fn subgroup|fn subgroup' "$CHAIN_HOMOTOPY_RS" "$SAVE_RS"
rg -n -C4 'name:\s*String|pub fn name\(|fn name\(|new\(' "$HOM_RS"Repository: SpectralSequences/sseq
Length of output: 13535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SAVE_RS="$(fd '^save\.rs$' ext/src | head -n1)"
CHAIN_HOMOTOPY_RS="$(fd '^chain_homotopy\.rs$' ext/src | head -n1)"
HOM_RS="$(fd '^resolution_homomorphism\.rs$' ext/src | head -n1)"
echo "== subgroup implementation =="
sed -n '360,405p' "$SAVE_RS"
echo
echo "== chain homotopy constructor =="
sed -n '48,62p' "$CHAIN_HOMOTOPY_RS"
echo
echo "== homomorphism naming / constructors =="
rg -n -C3 'pub fn name\(|pub fn new\(|name:\s*String' "$HOM_RS"Repository: SpectralSequences/sseq
Length of output: 4571
Encode the homomorphism names before building the subgroup path.
left.name() and right.name() are inserted raw into homotopies/{left}__{right}. A name containing / or __ can change the subgroup layout or collide with a different pair, which can overwrite or reload the wrong homotopy data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext/src/chain_complex/chain_homotopy.rs` around lines 52 - 59, The subgroup
path built in chain_homotopy::save_dir currently interpolates raw values from
left.name() and right.name(), which can alter the Store layout or collide with
other pairs. Update the path construction in the homotopy save logic to
encode/sanitize both names before passing them into parent.subgroup, and keep
the change localized around the save_dir creation so the homotopy lookup and
persistence use the same safe naming scheme.
Comment on lines
+181
to
+187
| if let Some(store) = self.save_dir.store() | ||
| && let Some(data) = store.read(SaveKind::ChainHomotopy, source).unwrap() | ||
| { | ||
| let mut cursor = &data[..]; | ||
| let mut outputs = Vec::with_capacity(num_gens); | ||
| for _ in 0..num_gens { | ||
| outputs.push(FpVector::from_bytes(p, target_dim, &mut f).unwrap()); | ||
| outputs.push(FpVector::from_bytes(p, target_dim, &mut cursor).unwrap()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect FpVector byte format and similar cache readers for existing framing helpers.
set -euo pipefail
FP_VECTOR_FILES="$(fd -i '.*vector.*' ext/crates/fp/src)"
CHAIN_HOMOTOPY_RS="$(fd '^chain_homotopy\.rs$' ext/src | head -n1)"
SECONDARY_RS="$(fd '^secondary\.rs$' ext/src | head -n1)"
rg -n -C4 'fn from_bytes|fn to_bytes|from_bytes\(p, target_dim|to_bytes\(&mut' $FP_VECTOR_FILES "$CHAIN_HOMOTOPY_RS" "$SECONDARY_RS"Repository: SpectralSequences/sseq
Length of output: 7207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ext/src/chain_complex/chain_homotopy.rs"
sed -n '150,290p' "$FILE" | cat -nRepository: SpectralSequences/sseq
Length of output: 6118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the save/load APIs and any cache-key definitions used by chain homotopy.
rg -n -C3 'SaveKind::ChainHomotopy|save_dir\.store\(|store\.read\(|store\.write\(|target_dim|num_gens|source\.s\(\)|source\.t\(\)' ext/src ext/crates | sed -n '1,220p'Repository: SpectralSequences/sseq
Length of output: 15937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect SaveDirectory and SaveKind to see what the cache key includes.
rg -n -C4 'enum SaveKind|struct SaveDirectory|fn read\(|fn write\(|impl SaveDirectory|ChainHomotopy|ChainMap' ext/src ext/crates | sed -n '1,260p'Repository: SpectralSequences/sseq
Length of output: 15119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'save.*\.rs$|directory.*\.rs$|persist.*\.rs$' ext/src ext/cratesRepository: SpectralSequences/sseq
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the save directory implementation and inspect the keying logic.
FILES="$(git ls-files 'ext/src/**/*.rs' 'ext/crates/**/*.rs' | rg 'save|directory|persist|cache')"
printf '%s\n' "$FILES"Repository: SpectralSequences/sseq
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find definitions related to SaveDirectory / SaveKind.
rg -n -C4 'enum SaveKind|struct SaveDirectory|impl SaveDirectory|fn read\(|fn write\(' ext/src ext/crates | sed -n '1,260p'Repository: SpectralSequences/sseq
Length of output: 1308
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the save directory implementation file directly.
fd -a 'save_directory\.rs$|save.*dir.*\.rs$|directory.*save.*\.rs$' ext/src ext/cratesRepository: SpectralSequences/sseq
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the save directory implementation and nearby types.
FILES="$(rg -l 'struct SaveDirectory|enum SaveKind' ext/src ext/crates)"
printf '%s\n' "$FILES"
for f in $FILES; do
echo "=== $f ==="
rg -n -C3 'struct SaveDirectory|enum SaveKind|impl SaveDirectory|fn read\(|fn write\(' "$f"
doneRepository: SpectralSequences/sseq
Length of output: 531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any existing framing/validation on cached byte payloads.
rg -n -C3 'read_u64|write_u64|cursor|remaining|consum|assert.*cursor|from_bytes\(p, .*cursor|to_bytes\(&mut buf' ext/src ext/crates | sed -n '1,260p'Repository: SpectralSequences/sseq
Length of output: 18759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ext/src/save.rs"
# Read the SaveDirectory / ZarrSaveStore read-write implementation and the cache path layout.
sed -n '1,140p' "$FILE" | cat -n
printf '\n====\n'
sed -n '1384,1495p' "$FILE" | cat -n
printf '\n====\n'
sed -n '1495,1625p' "$FILE" | cat -nRepository: SpectralSequences/sseq
Length of output: 8450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ext/src/save.rs"
# Inspect the store keying and read/write methods used by SaveKind payloads.
rg -n -C6 'fn read\(|fn write\(|SaveKind::ChainHomotopy|SaveKind::ChainMap|path|group|key|zarr' "$FILE" | sed -n '1,260p'Repository: SpectralSequences/sseq
Length of output: 13293
Frame cached homotopy payloads. Store num_gens and target_dim with the bytes, then reject/recompute on mismatch and fail if any trailing bytes remain after decoding. Right now a shorter target_dim can silently accept stale data, while truncated/corrupt payloads only panic via unwrap().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext/src/chain_complex/chain_homotopy.rs` around lines 181 - 187, The cached
homotopy payload handling in `ChainHomotopy` is unsafe because
`save_dir.store()` reads raw bytes without validating the encoded shape. Update
the `read` path to store and verify `num_gens` and `target_dim` alongside the
payload, then in the decode block reject/recompute on any mismatch before
constructing `outputs`. Also replace the `unwrap()`-based decoding in this
`SaveKind::ChainHomotopy` branch with explicit error handling and ensure the
cursor has no trailing bytes after `FpVector::from_bytes` completes.
Comment on lines
+237
to
+257
| if let Some(store) = self.save_dir.store() | ||
| && let Some(data) = store.read(SaveKind::ChainMap, input).unwrap() | ||
| { | ||
| let mut cursor = &data[..]; | ||
| let mut outputs = Vec::with_capacity(num_gens); | ||
|
|
||
| if let Some(mut f) = self | ||
| .source | ||
| .save_file(SaveKind::ChainMap, input) | ||
| .open_file(dir.to_owned()) | ||
| { | ||
| let fx_dimension = f.read_u64::<LittleEndian>().unwrap() as usize; | ||
| for _ in 0..num_gens { | ||
| outputs.push(FpVector::from_bytes(p, fx_dimension, &mut f).unwrap()); | ||
| } | ||
| return f_cur.add_generators_from_rows_ooo(input.t(), outputs); | ||
| for _ in 0..num_gens { | ||
| outputs.push(FpVector::from_bytes(p, fx_dimension, &mut cursor).unwrap()); | ||
| } | ||
| return f_cur.add_generators_from_rows_ooo(input.t(), outputs); | ||
| } | ||
|
|
||
| if output.s() == 0 { | ||
| let outputs = | ||
| extra_images.unwrap_or_else(|| vec![FpVector::new(p, fx_dimension); num_gens]); | ||
|
|
||
| if let Some(dir) = self.save_dir.write() { | ||
| let mut f = self | ||
| .source | ||
| .save_file(SaveKind::ChainMap, input) | ||
| .create_file(dir.clone(), false); | ||
| f.write_u64::<LittleEndian>(fx_dimension as u64).unwrap(); | ||
| if let Some(store) = self.save_dir.store() { | ||
| let mut buf = Vec::new(); | ||
| for row in &outputs { | ||
| row.to_bytes(&mut f).unwrap(); | ||
| row.to_bytes(&mut buf).unwrap(); | ||
| } | ||
| store.write(SaveKind::ChainMap, input, &buf).unwrap(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the saved chain-map row width.
FpVector::to_bytes does not encode the vector length, but the load path now parses rows using the current fx_dimension. If a store is resumed with a different target dimension, row boundaries become misaligned. Store the saved fx_dimension once before the rows and read with that value, or explicitly invalidate mismatched cache entries.
Suggested shape header
if let Some(store) = self.save_dir.store()
&& let Some(data) = store.read(SaveKind::ChainMap, input).unwrap()
{
let mut cursor = &data[..];
+ let (dim_bytes, rest) = cursor.split_at(size_of::<u64>());
+ let saved_fx_dimension = u64::from_le_bytes(dim_bytes.try_into().unwrap()) as usize;
+ cursor = rest;
let mut outputs = Vec::with_capacity(num_gens);
for _ in 0..num_gens {
- outputs.push(FpVector::from_bytes(p, fx_dimension, &mut cursor).unwrap());
+ outputs.push(FpVector::from_bytes(p, saved_fx_dimension, &mut cursor).unwrap());
}
return f_cur.add_generators_from_rows_ooo(input.t(), outputs);
} if let Some(store) = self.save_dir.store() {
let mut buf = Vec::new();
+ buf.extend_from_slice(&(fx_dimension as u64).to_le_bytes());
for row in &outputs {
row.to_bytes(&mut buf).unwrap();
}
store.write(SaveKind::ChainMap, input, &buf).unwrap();
}Apply the same header before the rows in the later write block on Line 325.
Also applies to: 325-330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext/src/resolution_homomorphism.rs` around lines 237 - 257, The chain-map
cache serialization in the load/save path is missing the saved row width, so
`FpVector::to_bytes` data is being read back with the current `fx_dimension`
instead of the original one. Update the `save_dir.store()` logic in the
chain-map handling to persist a small header with the saved `fx_dimension`
before the row bytes, and in the corresponding read branch deserialize that
header first and use it to parse each `FpVector`; if the stored width differs
from the current target, skip or invalidate the cached entry. Apply the same
header format in both write locations around the `SaveKind::ChainMap` calls so
`add_generators_from_rows_ooo` always sees correctly framed rows.
Comment on lines
+140
to
+142
| if let Some(store) = save_dir.store() { | ||
| store.bind_to_algebra(algebra.magic(), algebra.prime().as_u32(), algebra.prefix())?; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bind save stores to the resolved complex, not just the algebra.
bind_to_algebra only rejects different algebra metadata, but kernels, differentials, and QIs also depend on the target ChainComplex. Reusing a store for another complex over the same algebra can load wrong-but-structurally-valid cache data. Add a complex/resolution fingerprint to the root attrs or require a per-complex subgroup before reads/writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext/src/resolution.rs` around lines 140 - 142, The cache binding in
resolution setup only uses bind_to_algebra, so a store can be reused across
different ChainComplex instances with the same algebra metadata. Update the
save_dir.store() path to bind and validate against the resolved complex as well,
using a ChainComplex/resolution fingerprint (or equivalent per-complex subgroup
key) before any reads or writes, and keep the existing algebra metadata check in
place.
Comment on lines
+413
to
+426
| let payload: DifferentialPayload = bitcode::deserialize(&data) | ||
| .with_context(|| format!("Failed to deserialize differential at {b}")) | ||
| .unwrap(); | ||
|
|
||
| let num_new_gens = payload.num_gens; | ||
| assert_eq!( | ||
| target_cc_dimension, | ||
| f.read_u64::<LittleEndian>().unwrap() as usize, | ||
| target_cc_dimension, payload.target_cc_dimension, | ||
| "Malformed data: mismatched augmentation target dimension" | ||
| ); | ||
|
|
||
| self.add_generators(b, num_new_gens); | ||
|
|
||
| let mut d_targets = Vec::with_capacity(num_new_gens); | ||
| let mut a_targets = Vec::with_capacity(num_new_gens); | ||
|
|
||
| for _ in 0..num_new_gens { | ||
| d_targets | ||
| .push(FpVector::from_bytes(p, saved_target_res_dimension, &mut f).unwrap()); | ||
| } | ||
| for _ in 0..num_new_gens { | ||
| a_targets.push(FpVector::from_bytes(p, target_cc_dimension, &mut f).unwrap()); | ||
| } | ||
| drop(f); | ||
| current_differential.add_generators_from_rows(b.t(), d_targets); | ||
| current_chain_map.add_generators_from_rows(b.t(), a_targets); | ||
| current_differential.add_generators_from_rows(b.t(), payload.differentials); | ||
| current_chain_map.add_generators_from_rows(b.t(), payload.chain_maps); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the full differential payload before applying it.
target_res_dimension is persisted but never checked, and the row vectors are trusted to match num_gens. A stale/corrupt shard can mutate the resolution with mismatched rows before the later QI assertions run.
Suggested validation
let num_new_gens = payload.num_gens;
assert_eq!(
target_cc_dimension, payload.target_cc_dimension,
"Malformed data: mismatched augmentation target dimension"
);
+ assert_eq!(
+ target_res_dimension, payload.target_res_dimension,
+ "Malformed data: mismatched resolution target dimension"
+ );
+ assert_eq!(
+ payload.differentials.len(),
+ num_new_gens,
+ "Malformed data: mismatched differential row count"
+ );
+ assert_eq!(
+ payload.chain_maps.len(),
+ num_new_gens,
+ "Malformed data: mismatched chain-map row count"
+ );
self.add_generators(b, num_new_gens);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let payload: DifferentialPayload = bitcode::deserialize(&data) | |
| .with_context(|| format!("Failed to deserialize differential at {b}")) | |
| .unwrap(); | |
| let num_new_gens = payload.num_gens; | |
| assert_eq!( | |
| target_cc_dimension, | |
| f.read_u64::<LittleEndian>().unwrap() as usize, | |
| target_cc_dimension, payload.target_cc_dimension, | |
| "Malformed data: mismatched augmentation target dimension" | |
| ); | |
| self.add_generators(b, num_new_gens); | |
| let mut d_targets = Vec::with_capacity(num_new_gens); | |
| let mut a_targets = Vec::with_capacity(num_new_gens); | |
| for _ in 0..num_new_gens { | |
| d_targets | |
| .push(FpVector::from_bytes(p, saved_target_res_dimension, &mut f).unwrap()); | |
| } | |
| for _ in 0..num_new_gens { | |
| a_targets.push(FpVector::from_bytes(p, target_cc_dimension, &mut f).unwrap()); | |
| } | |
| drop(f); | |
| current_differential.add_generators_from_rows(b.t(), d_targets); | |
| current_chain_map.add_generators_from_rows(b.t(), a_targets); | |
| current_differential.add_generators_from_rows(b.t(), payload.differentials); | |
| current_chain_map.add_generators_from_rows(b.t(), payload.chain_maps); | |
| let payload: DifferentialPayload = bitcode::deserialize(&data) | |
| .with_context(|| format!("Failed to deserialize differential at {b}")) | |
| .unwrap(); | |
| let num_new_gens = payload.num_gens; | |
| assert_eq!( | |
| target_cc_dimension, payload.target_cc_dimension, | |
| "Malformed data: mismatched augmentation target dimension" | |
| ); | |
| assert_eq!( | |
| target_res_dimension, payload.target_res_dimension, | |
| "Malformed data: mismatched resolution target dimension" | |
| ); | |
| assert_eq!( | |
| payload.differentials.len(), | |
| num_new_gens, | |
| "Malformed data: mismatched differential row count" | |
| ); | |
| assert_eq!( | |
| payload.chain_maps.len(), | |
| num_new_gens, | |
| "Malformed data: mismatched chain-map row count" | |
| ); | |
| self.add_generators(b, num_new_gens); | |
| current_differential.add_generators_from_rows(b.t(), payload.differentials); | |
| current_chain_map.add_generators_from_rows(b.t(), payload.chain_maps); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext/src/resolution.rs` around lines 413 - 426, The differential shard
handling in resolution::add_generators_from_rows currently only checks
target_cc_dimension after deserializing DifferentialPayload, so stale or corrupt
data can still be applied. In the logic that reads DifferentialPayload, add
validation for the full payload before mutating state: verify
target_res_dimension matches the expected resolution dimension, and ensure the
differential and chain_map row vectors are consistent with num_gens and the
block size before calling self.add_generators,
current_differential.add_generators_from_rows, and
current_chain_map.add_generators_from_rows. Use the existing DifferentialPayload
and add_generators_from_rows flow to locate the checks.
Comment on lines
+377
to
+386
| pub fn subgroup(&self, name: &str) -> anyhow::Result<Self> { | ||
| let group = format!("{}/{}", self.group, name); | ||
| let group_path = self.path.join(group.trim_start_matches('/')); | ||
| if !group_path.join("zarr.json").exists() { | ||
| // Ensure each path component exists as a group so that intermediate | ||
| // levels (e.g. `products/`) are valid zarr groups, not just dirs. | ||
| self.ensure_intermediate_groups(&group)?; | ||
| GroupBuilder::new() | ||
| .build(self.store.clone(), &group)? | ||
| .store_metadata()?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Encode subgroup names before using them as path components.
subgroup() accepts names that are later built from homomorphism names, so raw /, .., or path separators can create unintended nested groups or escape the intended store prefix on filesystem-backed stores. Percent-encode or otherwise validate each user/name component before constructing the zarr group path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext/src/save.rs` around lines 377 - 386, The subgroup path construction in
subgroup() is using raw name components, which can allow "/" or ".." to alter
the intended Zarr hierarchy. Update subgroup() to encode or strictly validate
the supplied name before building group and group_path, and make sure any
downstream use in ensure_intermediate_groups() and GroupBuilder::new().build()
operates on the sanitized component rather than the raw input.
Comment on lines
+574
to
+578
| debug_assert!( | ||
| !matches!(kind, SaveKind::ResQi | SaveKind::NassauQi), | ||
| "write() is only for sharded kinds, got {:?}", | ||
| kind | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make stream-kind misuse fail in release builds.
These debug_assert! checks disappear in release, so an accidental write/read/delete(SaveKind::ResQi | SaveKind::NassauQi, ...) would operate on a shard-tier array with the same kind name instead of the stream-tier layout. Return an error for fallible methods and avoid silently probing the wrong layout in exists().
Also applies to: 609-613, 629-633, 643-647
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ext/src/save.rs` around lines 574 - 578, The stream-kind guard currently uses
debug_assert! in the write/read/delete paths, so misuse of SaveKind::ResQi or
SaveKind::NassauQi is only caught in debug builds. Replace these with runtime
validation in the affected save methods (including write, read, delete, and the
exists probe) so fallible operations return an error instead of silently
targeting the wrong shard-tier layout. Use the existing SaveKind checks in the
relevant methods to reject stream kinds explicitly and keep exists() from
probing the shard layout when given a stream kind.
Address the low-risk, verified subset of CodeRabbit's review on SpectralSequences#260: - ResQiReader::apply / into_quasi_inverse: replace assert!(got, ...) with anyhow::ensure!(...). Both are already anyhow::Result-returning, so a truncated stream now propagates an error instead of aborting the caller. - NassauQiReader::parse: reject odd-length Signature payloads before chunks_exact(2) silently drops a trailing byte. - Stream-kind guards in write/read/exists/delete: promote debug_assert! to assert! so misusing a stream kind (ResQi/NassauQi) on the shard-tier API is caught in release too, not just debug. Deliberately not applied from the same review: - "Also assert target_res_dimension in the differential load path" — this is a false positive: test_load_smaller legitimately resumes into a smaller target range, so the read-time resolution dimension differs from the saved one (the rows carry their own widths). The original code asserts only target_cc_dimension for exactly this reason. - Prime check in bind_to_algebra — redundant: magic() already encodes the prime (p << 16), so a prime mismatch already fails the existing magic check.
…From) Addresses the remaining actionable review finding on SpectralSequences#260. Store creation can fail (bad path, permissions, unreadable existing metadata), but the old From<Option<PathBuf>> swallowed that with .expect(), so new_with_save's anyhow::Result could never surface it. Replace the impl with TryFrom<Option<PathBuf>> (Error = anyhow::Error) and thread the fallible conversion through the entry points that take a save dir: Resolution / nassau::Resolution::new_with_save and utils::construct{,_nassau, _standard} now take `impl TryInto<SaveDirectory, Error = anyhow::Error>` and `?`-propagate. Every existing call site passes Option<PathBuf>, so this is source-compatible; a bad save path now returns an Err instead of panicking. Not applied from the same review (left for the author's call): name sanitization for subgroup paths, dimension-framing headers for chain-map/homotopy payloads, and per-complex store fingerprinting.
CI's lint job runs clippy on nightly, whose chunks_exact_to_as_chunks lint (not yet in the stable clippy I checked locally) rejects chunks_exact(2) with a constant size. Switch the NassauQi signature decode to as_chunks::<2>(); the even-length check just above guarantees an empty remainder, so this is equivalent. as_chunks is stable since 1.88, well under the toolchains CI tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/src/nassau.rs (1)
1127-1131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard against stored QI images wider than the current input.
Line 1129 only handles
inputbeing longer thanscratch1. If a stored QI was written with a largertarget_dim, this panics inside the fallible replay path instead of returning an error.Fail before slicing past the input
- input - .slice_mut(0, scratch1.len()) - .add(scratch1.as_slice(), 1); + anyhow::ensure!( + scratch1.len() <= input.len(), + "Stored Nassau QI image dimension {} exceeds current input dimension {} at {b}", + scratch1.len(), + input.len(), + ); + input + .slice_mut(0, scratch1.len()) + .add(scratch1.as_slice(), 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/src/nassau.rs` around lines 1127 - 1131, Guard the replay path in the resolve_through_stem logic so it fails gracefully when a stored QI image is wider than the current input. In the code that slices and adds `scratch1` into `input`, check both length directions before calling `slice_mut`/`add`, and return an error if `scratch1.len()` exceeds `input.len()` instead of panicking. Update the fallback handling around `resolve_through_stem` to use the existing fallible error path in `nassau.rs` rather than assuming `input` is always at least as wide as `scratch1`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/src/utils.rs`:
- Line 143: The save_dir parameter bounds in the affected constructors are too
strict and reject callers that already have a SaveDirectory because the
reflexive TryInto implementation uses Infallible rather than anyhow::Error.
Update the save_dir generic constraint in the relevant utility constructors in
ext/src/utils.rs and the matching new_with_save signatures in ext/src/nassau.rs
and ext/src/resolution.rs so they accept any TryInto<SaveDirectory> without
forcing Error = anyhow::Error, keeping the existing API behavior while allowing
existing SaveDirectory values.
---
Outside diff comments:
In `@ext/src/nassau.rs`:
- Around line 1127-1131: Guard the replay path in the resolve_through_stem logic
so it fails gracefully when a stored QI image is wider than the current input.
In the code that slices and adds `scratch1` into `input`, check both length
directions before calling `slice_mut`/`add`, and return an error if
`scratch1.len()` exceeds `input.len()` instead of panicking. Update the fallback
handling around `resolve_through_stem` to use the existing fallible error path
in `nassau.rs` rather than assuming `input` is always at least as wide as
`scratch1`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a7ae8d6-31e1-46b9-8330-1d030eb7f438
📒 Files selected for processing (4)
ext/src/nassau.rsext/src/resolution.rsext/src/save.rsext/src/utils.rs
Follow-up to the TryFrom change: the bound `TryInto<SaveDirectory, Error = anyhow::Error>` was stricter than the original `Into<SaveDirectory>` because the reflexive `TryInto<SaveDirectory>` for `SaveDirectory` has `Error = Infallible`, so a caller passing an already-built `SaveDirectory` was rejected. Widen the bound to `Error: Into<anyhow::Error>` (accepts both `Option<PathBuf>` with `anyhow::Error` and `SaveDirectory` with `Infallible`) and convert via `.map_err(Into::into)`, restoring the original API surface while keeping the fallible path for store creation.
hoodmane
reviewed
Jul 8, 2026
Comment on lines
+32
to
+36
| /// Save directory for this homotopy and any secondary lift built from it. | ||
| /// Set to a `homotopies/{left}__{right}` subgroup of the source's save_dir | ||
| /// when both `left` and `right` are named, `None` otherwise so that | ||
| /// homotopies between anonymous homs don't pollute the store. | ||
| save_dir: SaveDirectory, |
hoodmane
reviewed
Jul 8, 2026
| //! [`zarrs::storage::store::MemoryStore`] instead — `zarrs_filesystem` transitively pulls in | ||
| //! `positioned-io::RandomAccessFile`, which is gated to `cfg(any(windows, unix))` and doesn't | ||
| //! compile for WASM. The WASM frontend has no filesystem to persist to anyway, so the memory | ||
| //! store just acts as a no-op sink that's dropped at session end; the same code paths |
Contributor
There was a problem hiding this comment.
positioned-io::RandomAccessFile, which is gated tocfg(any(windows, unix))and doesn't
compile for WASM.
Maybe we should say here wasm32-unknown-unknown. wasm32-unknown-emscripten is unix.
- chain_homotopy.rs: restore the original ChainHomotopy field order (homotopies before save_dir). The rework had swapped them for no functional reason (field order here doesn't affect drop semantics or the constructor); revert to minimize churn. - save.rs: the wasm rationale said the filesystem store "doesn't compile for WASM", but that's specific to wasm32-unknown-unknown — wasm32-unknown-emscripten is `unix` and unaffected. Reword the module doc and the parallel inline comment to name the target precisely.
hoodmane
reviewed
Jul 8, 2026
Comment on lines
+84
to
+93
| #[cfg(not(target_arch = "wasm32"))] | ||
| use zarrs::array::codec::ZstdCodec; | ||
| // The concrete backing store depends on the target: native uses a `FilesystemStore` for real | ||
| // on-disk persistence; `wasm32-unknown-unknown` uses an in-memory `MemoryStore` so | ||
| // `zarrs_filesystem` (which pulls in `positioned-io::RandomAccessFile`, gated to windows/unix) | ||
| // doesn't need to compile. The web frontend has no filesystem to persist to anyway, so the | ||
| // memory store just serves as a no-op sink and is dropped at session end. | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| use zarrs::filesystem::FilesystemStore; | ||
| #[cfg(target_arch = "wasm32")] |
Contributor
There was a problem hiding this comment.
Likewise I think these could change to any(not(target_arch = "wasm32"), unix) or emscripten.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/src/resolution.rs (1)
865-871: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate store errors from
existsIfreadfails, this returnsfalseand the caller treats the differential as missing. Return aResult<bool>here so transient store errors don’t trigger redundant recomputation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/src/resolution.rs` around lines 865 - 871, The differential cache check in `Resolution::has_computed_bidegree` is swallowing store failures by treating `save_dir.store().exists(...)` as a plain boolean, so transient `read` errors look like “missing data.” Update this check to propagate storage failures instead of collapsing them into `false`, likely by making the `exists` path return a `Result<bool>` and threading that through the caller in `resolution.rs`. Keep the logic around `check_b`, `has_computed_bidegree`, and the `save_dir.store()` access intact, but ensure any error from `exists` is surfaced so the caller can distinguish “not present” from “store unavailable.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ext/src/resolution.rs`:
- Around line 865-871: The differential cache check in
`Resolution::has_computed_bidegree` is swallowing store failures by treating
`save_dir.store().exists(...)` as a plain boolean, so transient `read` errors
look like “missing data.” Update this check to propagate storage failures
instead of collapsing them into `false`, likely by making the `exists` path
return a `Result<bool>` and threading that through the caller in
`resolution.rs`. Keep the logic around `check_b`, `has_computed_bidegree`, and
the `save_dir.store()` access intact, but ensure any error from `exists` is
surfaced so the caller can distinguish “not present” from “store unavailable.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7c0c592-a06a-47a9-9607-e769dad83ec0
📒 Files selected for processing (5)
ext/src/chain_complex/chain_homotopy.rsext/src/nassau.rsext/src/resolution.rsext/src/save.rsext/src/utils.rs
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Replaces the ad-hoc file-per-value save format with a ZarrV3-backed layout via the
zarrscrate, rebased onto currentmaster.Motivations for the format change:
to_bytes/from_bytesplumbing is replaced by zarr arrays whose shape matches the data.This is a clean break — no migration from the old format.
Layout
One zarr v3 store on a
FilesystemStore(native) /MemoryStore(wasm), with two tiers:kernel,differential,augmentation_qi, secondary data, chain maps/homotopies): one shardedvlen-bytesarray per kind, shard shape(8, 8[, 8]), inner chunk[1,1[,1]], CRC32C, no zstd. Tens of thousands of elements collapse into hundreds of shard files per kind.res_qi,nassau_qi): per-bidegree zarr groups with kind-specific sub-arrays, read/written a chunk at a time so peak memory is bounded regardless of payload size. Afinishedgroup attribute is the atomicity source of truth — a writer dropped beforefinish()is treated as missing on read.Coordinates are
(n, s[, idx])(MultiDegree<2/3>::coords()), with a fixed internalN_MIN = -1024offset so negative stems map into zarr's unsigned index space. Named homs and chain homotopies get per-name subgroups (products/{name}/,homotopies/{l}__{r}/) sharing the underlying store viaArc.bitcode(serde) replaces hand-rolled framing for the shard tier; thefptypes gainedSerialize/Deserializein #229.QuasiInverse::stream_quasi_inverse,MilnorSubalgebra::{to,from}_bytes, theMagicenum, andSaveDirectory::Splitare removed.Rebase onto master
Rebased from the original merge-base onto current
master. Since master splitext::secondaryinto per-primary submodules (#254) and made the secondary lifts returnResult(#241/#242), conflicts were confined tosecondary.rs: the store-based save API is applied to the shared secondary machinery while preserving theResult-returning entry points, the stale monolithic type duplicates are dropped, and the per-new()create_dirloops are removed (shard arrays are now created lazily). Two throwaway commits from the original branch (an experimental rayon fork patch and an early tracing tweak that master supersedes) were dropped.Performance
Benchmarked against master's old (uncompressed) format — S_2 through-stem n=70 s=40, release +
concurrent, 4 cores, best of 3:Three fixes over the initial rework, all confined to
save.rswith no format/API change:Arrayhandles perSaveKind— the initial version re-parsed each array'szarr.jsonon every read/write/delete.with_concurrent_target(1)to avoid the sharding-codec rayon-join deadlock.EXT_SAVE_ZSTD_LEVEL(clamped to[1, 22]) so very large runs can trade write time for maximum compression, e.g.EXT_SAVE_ZSTD_LEVEL=19.Net: writes are ~1.4× master (down from ~5×), the read/resume path is at parity, and the store is less than half master's real on-disk size.
Test plan
tests/save_load_resolution.rstests pass (debug + release, incl.--features concurrent,nassau)cargo test --libgreenEXT_SAVE_ZSTD_LEVELoverride verified end-to-endKnown follow-ups
zarrs'FilesystemStorerewrites chunk files in place (no temp+rename), and reads are unlocked, so a read concurrent with a same-shard write could observe a torn shard — CRC32C turns this into a detectable error, not silent corruption, and it hasn't surfaced in practice. A per-shardRwLockwould close it if it ever does.🤖 Generated with Claude Code
https://claude.ai/code/session_01AJt3nb5Fp6okurqLzcQbCa
Generated by Claude Code
Summary by CodeRabbit