Skip to content

Fix ZarrParser crash on S3 directory-marker objects#1055

Open
TomNicholas wants to merge 1 commit into
zarr-developers:mainfrom
TomNicholas:fix/directory-marker-chunk-key-crash
Open

Fix ZarrParser crash on S3 directory-marker objects#1055
TomNicholas wants to merge 1 commit into
zarr-developers:mainfrom
TomNicholas:fix/directory-marker-chunk-key-crash

Conversation

@TomNicholas

Copy link
Copy Markdown
Member

Summary

Fixes #1052.

obstore strips the trailing slash from listed S3 object keys, so build_1d_chunk_mapping's endswith("/") check can never filter out zero-byte "directory marker" objects (created by e.g. aws s3 sync, s3fs, or boto3.put_object(Key=prefix + "/")) out of the discovered chunk keys. The marker then fails int() parsing in build_chunk_manifest, crashing with e.g. ValueError: invalid literal for int() with base 10: 'precip'.

This isn't a rare edge case — of 152 real-world public Zarr v2 stores I swept (NOAA/NCAR/weatherbench2 datasets on AWS Open Data), 26 failed with this exact error.

Fix

Filter on whether each listed key actually starts with the full array_chunks_prefix instead of relying on the (now ineffective) trailing-slash check. A directory marker is keyed exactly as the prefix with its trailing slash removed, so it's one character shorter than the prefix and can never satisfy startswith(prefix) — this holds regardless of whether obstore has already stripped the marker's own trailing slash, so it's robust to the underlying cause rather than papering over one specific symptom.

Test plan

  • Added test_build_chunk_manifest_ignores_directory_marker_object in virtualizarr/tests/test_parsers/test_zarr.py. Since no in-memory or local obstore backend reproduces the S3-specific slash-stripping quirk itself (writes and lists are both path-normalized consistently for those backends, so a marker written via MemoryStore.put_async never even collides with a nested-chunk listing), the test fabricates the exact listing shape a real S3 store returns and feeds it to build_chunk_manifest directly.
  • Confirmed the new test fails with the reported ValueError against the pre-fix code, and passes after the fix.
  • Full existing test suite passes locally (pytest virtualizarr/tests), ruff check/ruff format clean, mypy clean on the changed file.
  • Also verified the fix against the real public bucket from the original bug report (s3://noaa-nwm-retrospective-2-1-zarr-pds/precip.zarr), which now opens successfully instead of raising.

[This is Claude Code on behalf of TomNicholas]

obstore strips the trailing slash from listed S3 object keys, so
build_1d_chunk_mapping's endswith("/") check can never filter out
zero-byte "directory marker" objects (created by e.g. `aws s3 sync`,
`s3fs`, or `boto3.put_object(Key=prefix + "/")`) from the discovered
chunk keys. The marker then fails int() parsing in build_chunk_manifest.

Filter on whether each listed key actually starts with the full
array_chunks_prefix instead: a directory marker is keyed exactly as
the prefix with its trailing slash removed, so it's one character
shorter than any genuine nested chunk key and fails that test
regardless of whether obstore has already stripped its own slash.

Fixes zarr-developers#1052

Co-authored-by: Claude <noreply@anthropic.com>
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.39%. Comparing base (117acc7) to head (5cc3370).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1055   +/-   ##
=======================================
  Coverage   90.39%   90.39%           
=======================================
  Files          37       37           
  Lines        2248     2249    +1     
=======================================
+ Hits         2032     2033    +1     
  Misses        216      216           
Files with missing lines Coverage Δ
virtualizarr/parsers/zarr.py 97.41% <100.00%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

ZarrParser crashes on stores with zero-byte S3 'directory marker' objects (invalid literal for int())

1 participant