fix: reject pickling writable ZipStore by AMBRA7592 · Pull Request #4168 · zarr-developers/zarr-python

AMBRA7592 · 2026-07-20T21:43:40Z

Summary

Pickling a ZipStore doesn't serialize the archive; it serializes the parameters to reopen it. So pickling is safe exactly when reopening is safe.

For readers (mode="r"), it is: many independent processes can open the same path and just read. Round-trips fine.

For writers, it isn't. A ZIP has one central directory written at close. Fan out several unpickled writers to one path and each rewrites that directory on close; last close wins, the rest is corruption. Since cross-process fan-out is why you pickle at all, the writer pickle path leads straight into the one thing ZIP can't survive. Rejecting at __getstate__ fails fast instead.

Closes #3516.

For reviewers

Keying on _zmode != "r" (not read_only) is right: an append-mode store can be read_only=True yet still rewrite its directory on reopen. Only "r" reopens harmlessly.

Author attestation

I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

TODO

Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/user-guide/*.md
Changes documented as a new file in changes/
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

codecov · 2026-07-20T21:55:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.23%. Comparing base (5a4767b) to head (83181c1).
⚠️ Report is 48 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4168      +/-   ##
==========================================
+ Coverage   93.99%   94.23%   +0.23%     
==========================================
  Files          91       92       +1     
  Lines       12795    12882      +87     
==========================================
+ Hits        12027    12139     +112     
+ Misses        768      743      -25

Files with missing lines	Coverage Δ
src/zarr/storage/_zip.py	`98.19% <100.00%> (+0.01%)`	⬆️
src/zarr/testing/store.py	`98.33% <100.00%> (ø)`

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…le-guard # Conflicts: # src/zarr/storage/_zip.py

…o codex/zipstore-pickle-guard

AMBRA7592 · 2026-08-14T22:33:53Z

Hi @mkitti, following up once on this PR. After @d-v-b synchronized the branch on August 5, the full check suite passed. When you have time, could you please review whether the _zmode != "r" pickling guard is the right safety boundary? Thanks.

mkitti · 2026-08-15T14:02:04Z

I'm not quite sure I understand what gets pickled here. What if the file changes between when it is pickled and unpickled?

AMBRA7592 · 2026-08-19T17:51:43Z

Thanks, that is the important distinction. The ZIP data itself is not pickled. __getstate__ stores the path and reopening state after removing the live ZipFile and lock; __setstate__ then opens a new ZipFile at that path.

Therefore, if the archive changes between pickling and unpickling, the restored read-only store sees the archive as it exists at unpickle time. If the path was removed or no longer contains a valid ZIP, opening or reading will fail. This is path-reference semantics, not snapshot isolation. I verified that replacing a valid archive between those operations makes the restored store read the replacement contents.

That behavior is non-destructive for mode="r", although callers requiring a stable view must keep the archive unchanged while it is being read. For mode="w" or mode="a", reopening can itself mutate the archive, and independent unpickled writers can overwrite its central directory. That destructive reopening is the boundary this PR rejects.

I can add an explicit documentation sentence or regression test for the current-file semantics if you think it should be part of the contract.

mkitti · 2026-08-20T12:17:16Z

Documentation and a regression test would be appreciated.

AMBRA7592 · 2026-08-20T13:57:40Z

Added in 2b2ebe67.

The ZipStore API notes and pickle user guide now state that a path-backed read-only pickle is a path reference, not an archive snapshot, and that callers requiring a stable view must keep the archive unchanged.
The regression test replaces a valid archive after pickling and verifies that the unpickled reader observes the replacement contents.

Local verification:

complete tests/test_store/test_zip.py: 83 passed, 18 skipped
Ruff check and format check passed
documentation lint passed
git diff --check passed

The updated PR CI is now running.

AMBRA7592 · 2026-09-05T16:42:16Z

Hi @mkitti, following up after adding the requested documentation and regression test in 2b2ebe67. The full PR check suite is green, including Codecov. When you have time, could you please re-review it? Thanks.

mkitti · 2026-09-05T19:40:26Z

I've asked @d-v-b to conduct final review.

Copilot

🟡 Changes recommended

The critical append-mode, read-only regression case is not tested.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Prevents writable ZipStore instances from being pickled, avoiding ZIP corruption when reopened across processes.

Changes:

Rejects pickling in archive-writing modes.
Preserves reader pickling and fixes restored-store testing.
Adds tests, documentation, and release notes.

File summaries

File	Review
`tests/test_store/test_zip.py`	Missing coverage for `mode="a", read_only=True`; add this regression case.
`src/zarr/testing/store.py`	Correctly tests the restored store instance.
`src/zarr/storage/_zip.py`	Enforces safe pickling modes.
`docs/user-guide/performance.md`	Documents pickling constraints.
`changes/4168.bugfix.md`	Records the bug fix.

Review details

Files reviewed: 5/5 changed files
Comments generated: 1
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

AMBRA7592 · 2026-09-06T11:31:55Z

+        store.close()
+        assert store.path is not None
+        with zipfile.ZipFile(store.path, mode="r") as archive:
+            assert archive.read("sentinel") == data.to_bytes()


Addressed in 83181c1f. The new regression test opens an existing archive with mode="a", read_only=True, asserts both pieces of state, and verifies that pickle.dumps raises. It therefore fails if the guard regresses to checking read_only instead of _zmode. Verification: 84 passed, 18 skipped for tests/test_store/test_zip.py; Ruff check, Ruff format check, and git diff --check pass.

read-the-docs-community · 2026-09-06T11:32:44Z

Documentation build overview

📚 zarr-indexing | 🛠️ Build #34416930 | 📁 Comparing 83181c1 against latest (a50d791)

🔍 Preview build

25 files changed · + 1 added · ± 11 modified · - 13 deleted

+ Added

api/composition/index.html

± Modified

- Deleted

read-the-docs-community · 2026-09-06T11:32:44Z

Documentation build overview

📚 zarr-metadata | 🛠️ Build #34416931 | 📁 Comparing 83181c1 against latest (a50d791)

🔍 Preview build

10 files changed · ± 9 modified · - 1 deleted

± Modified

- Deleted

release-notes/index.html

fix: reject pickling writable ZipStore

d014344

github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jul 20, 2026

docs: add ZipStore pickle changelog

64e98cc

github-actions Bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jul 20, 2026

d-v-b requested a review from mkitti July 22, 2026 21:42

d-v-b and others added 5 commits July 22, 2026 23:42

Merge branch 'main' into codex/zipstore-pickle-guard

75046e5

Merge remote-tracking branch 'upstream/main' into codex/zipstore-pick…

9eef8dd

…le-guard # Conflicts: # src/zarr/storage/_zip.py

Merge remote-tracking branch 'origin/codex/zipstore-pickle-guard' int…

b1261f9

…o codex/zipstore-pickle-guard

test: narrow ZipStore path after file-object support

eb3f4d9

Merge branch 'main' into codex/zipstore-pickle-guard

4b485f5

docs: define ZipStore pickle reference semantics

2b2ebe6

mkitti requested a balanced review from Copilot September 5, 2026 19:40

mkitti approved these changes Sep 5, 2026

View reviewed changes

mkitti requested a review from d-v-b September 5, 2026 19:41

Copilot started reviewing on behalf of mkitti September 5, 2026 19:41 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

test: cover read-only append ZipStore pickling

83181c1

Navigation Menu

Uh oh!

Uh oh!

fix: reject pickling writable ZipStore - #4168

fix: reject pickling writable ZipStore#4168
AMBRA7592 wants to merge 9 commits into
zarr-developers:mainfrom
AMBRA7592:codex/zipstore-pickle-guard

AMBRA7592 commented Jul 20, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

AMBRA7592 commented Aug 14, 2026

Uh oh!

mkitti commented Aug 15, 2026 •

edited

Loading

Uh oh!

AMBRA7592 commented Aug 19, 2026

Uh oh!

mkitti commented Aug 20, 2026

Uh oh!

AMBRA7592 commented Aug 20, 2026

Uh oh!

AMBRA7592 commented Sep 5, 2026

Uh oh!

mkitti commented Sep 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

AMBRA7592 Sep 6, 2026

Uh oh!

read-the-docs-community Bot commented Sep 6, 2026

Uh oh!

read-the-docs-community Bot commented Sep 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

AMBRA7592 commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

For reviewers

Author attestation

TODO

Uh oh!

codecov Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AMBRA7592 commented Aug 14, 2026

Uh oh!

mkitti commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AMBRA7592 commented Aug 19, 2026

Uh oh!

mkitti commented Aug 20, 2026

Uh oh!

AMBRA7592 commented Aug 20, 2026

Uh oh!

AMBRA7592 commented Sep 5, 2026

Uh oh!

mkitti commented Sep 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

AMBRA7592 Sep 6, 2026

Choose a reason for hiding this comment

Uh oh!

read-the-docs-community Bot commented Sep 6, 2026

Documentation build overview

Uh oh!

read-the-docs-community Bot commented Sep 6, 2026

Documentation build overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AMBRA7592 commented Jul 20, 2026 •

edited

Loading

codecov Bot commented Jul 20, 2026 •

edited

Loading

mkitti commented Aug 15, 2026 •

edited

Loading