[yuxin00j]

Summary

Prevents ZonalFile.__init__ from leaking a checked-out MRD pool or leaving a partially initialized file armed for deferred close when opening fails.

Changes

• gcsfs/zonal_file.py:
  • Check out an MRD pool only when mode == "rb" and accept only ("wb", "ab", "xb") as write modes, rejecting + modes ("r+", "rb+", "w+", etc.) before pool checkout or super().__init__.
  • Wrap super().__init__ in try ... except BaseException so a failure after pool checkout (such as an unknown cache_type) marks self.closed = True, clears self.mrd_pool, and closes the checked-out pool via sync_teardown (logging a warning if teardown fails so the initialization error is preserved).
• gcsfs/tests/test_zonal_file.py:
  • Add test_zonal_file_failed_init_closes_mrd_pool, test_zonal_file_failed_init_preserves_exception_when_pool_close_fails, and test_zonal_file_plus_modes_rejected_before_pool_or_super_init.

[gemini-code-assist]

Code Review

This pull request ensures that if ZonalFile.init fails after the MRD pool is checked out, the pool is properly closed and the file is marked as closed. It also restricts supported modes to explicit binary modes (rb, wb, ab, xb) and adds corresponding unit tests. A review comment suggests wrapping the sync_teardown call in a try...except block during cleanup to prevent any secondary exceptions from masking the original initialization failure.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.29%. Comparing base (c783299) to head (e09f0db).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1139      +/-   ##
==========================================
+ Coverage   92.24%   92.29%   +0.04%     
==========================================
  Files          20       20              
  Lines        4047     4060      +13     
==========================================
+ Hits         3733     3747      +14     
+ Misses        314      313       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

[zhixiangli]

Thanks for the fix. I checked it against a real zonal bucket. On main, a failed open() (e.g. consistency="crc32c" without crcmod, invalid fixed_key_metadata, an unknown cache_type, or r+/w+/a+) leaks the MRD pool every time: the refcount stays pinned and the deferred close fails with AttributeError: 'ZonalFile' object has no attribute 'timeout'. With this PR the pool is released in all of those cases. A few follow-ups inline.

[zhixiangli]

The adaptive cache's prefetcher is also left running here. GCSFile.__init__ creates it, sets cache.close = lambda: None (core.py L2733), and leaves stopping it to _close_impl(). Once this block sets self.closed = True, __del__ never calls _close_impl(), so any failure after L2733 leaves PrefetchProducer._loop() pending. For example, fs.open(path, "rb", fixed_key_metadata=123) logs Task was destroyed but it is pending! ... PrefetchProducer._loop(). This also happens on main, but it fits the scope of this PR. Suggest stopping it the same way GCSFile._close_impl does:

self.closed = True
cache = getattr(self, "cache", None)
prefetcher = getattr(cache, "_prefetcher", None)
if prefetcher is not None:
    cache._prefetcher = None
    try:
        prefetcher.close()
    except Exception as e:
        logger.warning(
            "Failed to close prefetcher during failed initialization cleanup: %s", e
        )

[zhixiangli]

Nit: _get_prefetcher_and_cache_config doesn't check cache_type, so a typo costs an _info RPC and a new gRPC stream before fsspec raises KeyError. After cleanup, that open stream sits in MRDPoolCache under (..., "no-such-cache") and uses up an LRU slot. Suggest failing before checking out the pool:

if resolved_cache_type not in fsspec.caching.caches:
    raise KeyError(resolved_cache_type)

[zhixiangli]

cache_type="no-such-cache" raises before self.cache exists, so the "failure after the cache is built" path isn't covered. This test (and the one below) would also stop testing anything if cache_type were checked before the pool checkout. fixed_key_metadata=123 raises TypeError in GCSFile.__init__ after the cache is built and works as a trigger; with the prefetcher change above this test could also assert zf.cache._prefetcher is None.

[zhixiangli]

fs.open(path, "r+") normalizes the mode to "r+b" before it reaches ZonalFile. Could we add "r+b", "w+b", "a+b" and "x+b" so the test covers the strings callers actually send?

[zhixiangli]

getattr(zf, "closed", True) is True always passes, because AbstractBufferedFile.closed already defaults to True when _closed isn't set. To check that __del__ was never armed:

assert "_closed" not in zf.__dict__