fix(store): don't close a shared filesystem in FsspecStore.close()

[d-v-b]

🤖 AI text below 🤖

Fixes F3 from the v3.3.0 evaluation — high-impact runtime breakage on the default zarr.open("s3://...") path.

The bug

from_url sets _owns_fs = True unconditionally. The premise recorded in zarr-developers#4003 — "True only when FsspecStore itself created the filesystem" — is false: url_to_fs goes through fsspec's _Cached metaclass, which tokenizes on storage options only, not the path. Two stores built from two URLs on one host get the same AbstractFileSystem, and each claims to own it.

close() then kills the session for every sibling store, and because the dead filesystem stays in cls._cache, stores created afterwards are handed the corpse too. The damage outlives the store that caused it.

Two corrections to the finding as written

1. The MVCE in the writeup doesn't actually demonstrate the bug — set_session() is a coroutine and it's never awaited, so assert session.closed was passing on a coroutine object's missing attribute rather than a closed session. The underlying bug is real; I re-derived it with an awaited version.

2. skip_instance_cache=True alone is not sufficient, though the writeup offers it as the fix. _owns_fs is a plain attribute with no __getstate__ anywhere, so it pickles as True while the filesystem is rebuilt cache-served on the far side. On a dask worker the store owns a shared instance again and the bug is back. Detecting a "newly created" instance is also impossible — _fs_token is identical for a fresh and a cache-served instance, and a new cachable instance is inserted into _cache before __call__ returns.

The predicate that actually matters isn't "did I create it" but "can anyone else reach it" — which has to be asked at close() time, not construction time.

The fix (both halves)

1. from_url passes skip_instance_cache=True — the filesystem is genuinely private, so closing it is safe and fix(FsspecStore): close owned async filesystem on store.close() zarr-developers/zarr-python#4003's leak fix keeps working.
2. close() re-checks _fs_is_shared(self.fs) against the live cache — self-correcting after a pickle roundtrip: the fs comes back cache-served, the check turns close() into a no-op, and no sibling is harmed.

Each half covers where the other fails. The helper guards cachable/_cache/_fs_token with getattr and degrades to "assume shared" — the safe answer — if fsspec's internals move.

Also fixes from_mapper, which the writeup doesn't mention. Its fs is not original_fs heuristic assumes any new object is private, but _make_async's sync→async conversion round-trips through to_json/from_json, which the cache serves. A sync s3fs mapper therefore wrongly claimed ownership. It now uses the same check.

A caller who explicitly passes skip_instance_cache=False still gets a cached fs and correctly gets _owns_fs=False — the **opts ordering respects user intent while staying safe.

Tests

Four new tests, all failing on main and passing here:

• test_from_url_filesystem_is_private — distinct URLs, distinct fs
• test_close_does_not_close_a_sibling_stores_session
• test_close_does_not_poison_the_instance_cache — a store created after the close still gets a live session
• test_close_declines_to_close_a_shared_filesystem — the pickle-resurrection case

The three session tests use HTTP rather than S3 deliberately: s3fs/aiobotocore transparently reconnects, so an S3-based assertion passes even with the bug present. I wrote the S3 versions first and they were vacuous — they passed against unfixed main. HTTP's aiohttp session stays dead, which is where the user-visible RuntimeError: Session is closed comes from. No network I/O — set_session() only constructs the session.

That reconnect behavior is also why this went unnoticed: the existing test_from_url_close_releases_store only checks not store._is_open, never that the session survived.

Full suite: 6731 passed, 0 failed. mypy clean.

Known tradeoff

A from_url store no longer compares equal to itself across a pickle roundtrip: skip_instance_cache perturbs the fs token and isn't carried in storage_options, so unpickling reconstructs through the cache with a different token. Stores built from the same URL still compare equal, and no existing test covers the roundtrip case (test_serializable_store's fixture constructs the store directly with a supplied fs). Flagging it explicitly since test_serializable_store's comment calls roundtrip equality "important for being able to compare (de)serialized stores" — happy to pursue a __getstate__ that re-privatizes on unpickle if you'd rather not take this.

Relatedly, unpickled stores on a worker share one cached fs and none will close it, so the session leaks there — safe, but zarr-developers#4003's original complaint in that narrow case.

Left alone

with_read_only() mutating self._owns_fs = False on the source store is still there, per your call — a nominally derive-only method with a side effect that silently changes the source's close() behavior and makes it non-idempotent. Worth a separate look.