fix(FsspecStore): close owned async filesystem on store.close()#4003
Open
josh-ag2 wants to merge 4 commits into
Open
fix(FsspecStore): close owned async filesystem on store.close()#4003josh-ag2 wants to merge 4 commits into
josh-ag2 wants to merge 4 commits into
Conversation
FsspecStore.from_url() and from_mapper() create their own async filesystem instance that zarr is responsible for — but Store.close() never cleaned it up, leaving the underlying aiohttp ClientSession open until garbage collection. This produced "Unclosed client session" ResourceWarnings from aiohttp, and in environments where the finalizer ran on the wrong event loop (e.g. Python 3.12+ with eager_start=True) it could raise RuntimeError. Changes: - Add _close_fs() async helper: calls fs.set_session() then client.close() for filesystems that expose set_session() (e.g. s3fs); no-op for all others. - Add _owns_fs: bool to FsspecStore.__init__ (default False). Set True in from_url() unconditionally; set True in from_mapper() only when _make_async() produced a new instance (sync→async wrap). Direct construction and from_upath() leave _owns_fs=False — the caller supplied the fs and remains responsible for it. - Override close() to invoke zarr_sync(_close_fs(self.fs)) before calling super().close(), guarded by _owns_fs and a bare except so it can never raise from a destructor path. Tests: - Update pytestmark comment (the filter stays for GC-path warnings). - test_from_url_owns_filesystem / test_from_url_close_releases_store - test_direct_construction_does_not_own_filesystem - test_from_upath_does_not_own_filesystem - test_from_mapper_does_not_own_already_async_filesystem - test_from_mapper_owns_wrapped_sync_filesystem - test_close_fs_closes_s3_client / test_close_fs_no_op_for_fs_without_set_session Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4003 +/- ##
=======================================
Coverage 93.49% 93.50%
=======================================
Files 88 88
Lines 11873 11889 +16
=======================================
+ Hits 11101 11117 +16
Misses 772 772
🚀 New features to boost your workflow:
|
…tation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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
`FsspecStore.from_url()` and `from_mapper()` create their own async filesystem instance, but `Store.close()` never cleaned it up. This left the aiohttp `ClientSession` inside s3fs open until garbage collection, producing:
```
ResourceWarning: Unclosed client session
ResourceWarning: coroutine 'ClientCreatorContext.aexit' was never awaited
```
The test file already had a `# TODO: fix these warnings` comment and a module-level `pytestmark` filter suppressing these warnings — this PR resolves the underlying cause for the `close()`-triggered path.
Changes
`src/zarr/storage/_fsspec.py`
Tests
Eight new tests in `tests/test_store/test_fsspec.py`:
All 105 existing + new tests pass; 6 skipped (version-gated).
Scope
This fix closes the S3 client session that is active at the time `store.close()` is called. It does not prevent warnings that may arise from sessions abandoned earlier — for example, s3fs with `cache_regions=True` internally refreshes and replaces its S3 client during I/O, discarding prior sessions before `close()` is ever invoked. Those intermediate sessions are an upstream s3fs issue addressed in fsspec/s3fs#1028, which fixes:
Notes