close
Skip to content

fix(FsspecStore): close owned async filesystem on store.close()#4003

Open
josh-ag2 wants to merge 4 commits into
zarr-developers:mainfrom
josh-ag2:fix/fsspec-store-owns-filesystem-lifecycle
Open

fix(FsspecStore): close owned async filesystem on store.close()#4003
josh-ag2 wants to merge 4 commits into
zarr-developers:mainfrom
josh-ag2:fix/fsspec-store-owns-filesystem-lifecycle

Conversation

@josh-ag2
Copy link
Copy Markdown

@josh-ag2 josh-ag2 commented May 23, 2026

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`

  • Add `_close_fs(fs)` — async helper that calls `fs.set_session()` then `client.close()` for filesystems that expose `set_session()` (i.e. s3fs / aiobotocore); silently skips all others.
  • Add `_owns_fs: bool` to `FsspecStore.init` (default `False`). Set `True` in `from_url()` unconditionally (zarr created the fs). Set `True` in `from_mapper()` only when `_make_async()` produced a new instance (sync→async conversion). Direct construction and `from_upath()` leave `_owns_fs=False` — the caller supplied the fs and remains responsible for its lifecycle.
  • Override `close()` to call `zarr_sync(_close_fs(self.fs))` before `super().close()` when `_owns_fs is True`. Wrapped in `contextlib.suppress(Exception)` so it can never raise from a destructor context.

Tests

Eight new tests in `tests/test_store/test_fsspec.py`:

  • `test_from_url_owns_filesystem` — `_owns_fs=True` after `from_url()`
  • `test_from_url_close_releases_store` — `close()` completes without error and sets `_is_open=False`
  • `test_direct_construction_does_not_own_filesystem` — `_owns_fs=False` when fs supplied directly
  • `test_from_upath_does_not_own_filesystem` — `_owns_fs=False` for `from_upath()`
  • `test_from_mapper_does_not_own_already_async_filesystem` — `_owns_fs=False` when mapper's fs is already async (identity case in `_make_async`)
  • `test_from_mapper_owns_wrapped_sync_filesystem` — `_owns_fs=True` when sync fs is wrapped
  • `test_close_fs_closes_s3_client` — unit test (mock): `_close_fs()` calls `set_session()` then `client.close()`
  • `test_close_fs_no_op_for_fs_without_set_session` — unit test (mock): `_close_fs()` is a no-op for fs without `set_session`

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:

  • stale `S3BucketRegionCache` not closed on refresh
  • vacuous-truth stale-session check on an empty `_sessions` dict
  • concurrent `asyncio.gather` callers each creating a separate creator and tearing down each other's in-flight sessions

Notes

  • The `pytestmark` `ResourceWarning` filter is kept: it still applies to stores GC'd without an explicit `close()` call (e.g. `test_basic`) and to the residual aiobotocore `ClientCreatorContext.aexit` issue. The filter comment is updated to reflect what's now fixed vs what's still outstanding.
  • `from_upath()` and direct `FsspecStore(fs=..., ...)` construction leave `_owns_fs=False` — the caller owns the fs in those cases, matching the existing ownership model.

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>
@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label May 23, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.50%. Comparing base (c0e2afa) to head (f4d4517).

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           
Files with missing lines Coverage Δ
src/zarr/storage/_fsspec.py 92.06% <100.00%> (+0.73%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot removed the needs release notes Automatically applied to PRs which haven't added release notes label May 23, 2026
…tation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

1 participant