Audit the structure and implementation of the filesystem package #1327

Activity

1. laughingman7743

   added

   enhancement

   on Oct 24, 2026
2. laughingman7743

   added a parent issue on Oct 24, 2026

   • Review the filesystem package before a standalone release #1326
3. laughingman7743 commented on Oct 24, 2026

   

   laughingman7743

   on Oct 24, 2026

   MemberAuthor

   More actions

   Audit report for this issue, at commit 766d79d0 (master after #1321). Five read-only area audits (A: S3FileSystem; B: S3File, write session, executors, ReadAheadStream; C: AioS3FileSystem against S3FileSystem; D: S3Core, models, S3Path, S3PathPairing, errors; E: public surface, docs, coupling to PyAthena) were followed by a re-run of every reproduction offline (fake clients or stubbed S3Core.call, all AWS endpoints pointed at a closed local port). Every defect below reproduced as stated.

   Defects

   #	Defect	Evidence	Reproduced
   1	cp_file/copy/mv/setxattr look up the source without its SSE-C or RequestPayer parameters, so the copy fails with OSError(EINVAL) before CopyObject (sync and aio)	s3.py:1604, :2132, :1440, :1398; s3_async.py:576; S3Core._copy_source_params already maps them	HeadObject carried only Bucket/Key
   2	A read-mode open() replaces the caller's IfMatch with the current ETag	s3.py:2787-2788	'"stale"' sent as "e1", read succeeded
   3	A reopened (unpickled) file loses the request parameters of open() and pins the current ETag while keeping the old size: a silent truncated read of another object	s3.py:2934-2943 (self.kwargs == {}), :2790-2791	reopened read returned b'WXYZ' of the new object with the old size 4
   4	S3File.metadata()/getxattr()/url()/setxattr() drop the file's lookup parameters (SSE-C)	s3.py:3225-3266	open-time HeadObject had SSECustomer*, metadata() had not
   5	An append's part copies are not pinned to the object looked up at open; the sync path re-runs an unpinned lookup above 5 GiB, the aio path uses the open-time size, so they differ	s3.py:3083-3097, :2795-2804; s3_async.py:1526-1529	part copies of the replacement object without version or CopySourceIfMatch
   6	ab and xb on a key that is only a prefix: ab raises FileNotFoundError, xb raises FileExistsError, wb writes	s3.py:2795-2809 against read mode at :2775-2777	yes
   7	commit() on a read-mode file raises AttributeError	s3.py:3180-3181	yes
   8	chmod(recursive=True) skips the object at the key when the key is also a prefix	s3.py:2206, :2232-2235	recursive=False -> ['a'], recursive=True -> ['a/x']
   9	Keys with an empty segment (dir//x): ls(dir) returns itself, find(maxdepth=...) returns the directory five times and misses the object	s3.py:516-518, :585, :857-860	yes
   10	ls/find of a version-qualified path list the key prefix, put the version on directory entries, and cache under a key that invalidation never reaches	s3.py:497-527	stale listing returned after a write below it
   11	_split_version_paths is quadratic (20,000 paths: 1.1 s)	s3.py:963-964	yes
   12	AioS3FileSystem.open() hangs in a forked child: self._loop skips fsspec's fork check, and S3AioExecutor submits to the parent's loop	s3_async.py:852-854	the child was killed by a 5 s alarm; the sync filesystem read
   13	S3ThreadPoolExecutor is not fork-safe, unlike S3AioThreadPool	s3_executor.py:180-202 against :448-482	work submitted in the child never ran
   14	A cancelled _put_file/_pipe_file/_get_file (for example by asyncio.wait_for) keeps uploading and commits; fsspec's fail-fast _put/_get cancel only the coroutines	s3_async.py:271-273; S3AioThreadPool.run	TimeoutError after one part; four more parts and CompleteMultipartUpload followed
   15	Recursive aio put() calls _makedirs per local directory: an extra HeadBucket, two concurrent CreateBucket with allow_bucket_creation=True, PermissionError without it; sync sends only the PutObjects	fsspec AsyncFileSystem._put → s3.py:1199-1259	yes
   16	aio expand_path(**kwargs) raises TypeError: fsspec's AsyncFileSystem._expand_path takes no **kwargs (upstream)	s3_async.py:828-830	refresh=True, prefix=
   17	rm(max_workers=N) and cp_file(on_error=...) are accepted by sync and sent to S3 by aio; cat(batch_size=...) is accepted by aio and sent to GetObject by sync	s3_async.py:372-386, :535-549; s3.py:1104, :1545	ParamValidationError
   18	S3FileSystem(max_workers=None) is accepted, then a multi-block read raises TypeError; aio resolves None	s3.py:173, :3342	yes
   19	A delete-marker version raises PermissionError (405), and exists() raises on it instead of returning False; the S3Core docstring and docs/filesystem.md promise FileNotFoundError	s3_errors.py:90-99, :122-128; s3.py:1000-1006	Stubber, HEAD and GET
   20	S3Core.part_ranges(0, ...) raises ValueError from zip(strict=True) instead of returning []	s3_core.py:1120-1126	yes
   21	S3Core.call() with request_kwargs fails for a callable without __name__ (functools.partial)	s3_core.py:642-644	yes
   22	S3Object stores keys in __dict__: a key named keys, items, get, update, ... breaks dict(o) and o.keys()	s3_object.py:183-224	yes
   23	A listed key ending in a version-like query (data?versionId=1) cannot be addressed: its name parses as version 1 of data	s3_path.py:52-61, s3_object.py:177	yes
   24	After the documented re-registration of another s3 class, PolarsCursor's whole-CSV read passes connection= as an unknown option and reads with ambient credentials	polars/result_set.py:608-623, :874; docs/filesystem.md:29-40; docs/polars.md:410	yes (s3fs emulated)
   25	After import pyathena.pandas/pyathena.polars, fsspec.filesystem("s3", asynchronous=True) returns the sync S3FileSystem, which accepts the flag	filesystem/__init__.py:36-44, s3.py:178-179	yes
   26	S3FileSystem(connection=...) and its read files cannot be pickled	fsspec _Cached stores connection in storage_options	AttributeError: Can't get local object 'lazy_call.<locals>._handler'
   27	S3FileSystem(connection=..., endpoint_url=..., config_kwargs=..., anon=...) silently ignores the client arguments; only requester_pays is honored	s3.py:217-235	same client, pool 10

   Design problems

   1. Executors. Four thread-pool mechanisms (S3ThreadPoolExecutor, S3AioThreadPool, S3AioExecutor, ReadAheadStream's own pool). S3AioExecutor hops through fsspec's loop only to submit to a pool of its own; the hop causes defect 12, waits that never finish on the loop thread, and the special cases of its shutdown; every call is wrapped twice. The sync filesystem creates a pool per open file and per rm, recursive chmod, clear_multipart_uploads and multipart copy; the aio filesystem has one pool plus one per file. Defaults differ (cpu_count() * 5 against min(32, cpu + 4)), and neither is reconciled with botocore's pool of 10. A file shuts down an executor that its caller passed. The fan-out with fail-fast logic is written by hand at five sites, while rm and clear_multipart_uploads wait for everything. aio's delegated put_file/pipe_file/get_file nest a file pool inside each filesystem thread: max_workers ** 2 parts in flight (16 UploadPart and 22 threads at max_workers=4). A call in an S3AioThreadPool thread that waits for work in the same pool starves (max_workers=1: 0 of 2 done).
   2. Sync and aio duplication. AioS3FileSystem mixes four styles: thread delegation, reimplementation, direct sync calls on the caller's thread, and fsspec defaults. It reaches into private sync helpers (_find, _delete_paths, _delete_batches, _delete_batch, _raise_delete_errors, _move_pairs, _copy_pairs, _split_version_paths, S3File._get_ranges, _S3WriteSession). Duplicated nearly line for line: _open (three copies of the open defaults), the two _*_in_transaction methods, _copy_file, the version preambles of copy/get/expand_path, and the read and write drivers of S3File and AioS3StreamedFile. Already diverged: mv stops at the first failure in sync and runs every copy in aio; annotations are copied sequentially in sync and in parallel in aio; appends (defect 5). Abort-failure logging is written four times. AioS3File overrides nothing. aio lacks the s3fs attributes that sync has (default_block_size, max_workers, version_aware, s3_additional_kwargs, ...) and coroutine forms of metadata, tags, xattrs, chmod, and the multipart listings.
   3. Logic on the wrong object. _directory_object, the inline root entry, _extract_parent_directories, _delete_batches, _raise_delete_errors, _get_lookup_kwargs, _get_lookup_cache_id (static methods on S3FileSystem); "the key itself or keys under key/" filtering written three times; the lookup-parameter policy in the adapter while its sibling _copy_source_params is private on S3Core (the cause of defect 1); S3Core._multipart_upload_request; S3PathPairing._is_glob, _target (which re-implements S3Path.target) and delete_paths; two range splitters with different rules (S3File._get_ranges leaves a short tail, S3Core.part_ranges merges it); block size validated in three places.
   4. Models. Response models in two styles and two modules (frozen dataclasses with from_response in s3_core.py, property classes over raw dicts in s3_object.py) with a circular import. S3Object is a MutableMapping, not a dict: no repr, not JSON-serializable, update() does not remap API names, entry shapes differ by type, and building it costs 3.8 µs per listed key against 1.1 µs for a dict subclass built from the typed page (about 3 s of CPU per million keys in find). S3Metadata is both the core's HeadObject result and the user-metadata mapping (an object without user metadata is falsy and equal to {}). S3Core has three **params precedence rules and discards results (copy_object and delete_object return None). to_dict() of S3PutObject and S3CompleteMultipartUpload exposes _etag.
   5. Cache. The dircache key scheme (path, "", (path, "/"), (path, ""), (path, "lookups")) is spread over about nine methods; invalidate_cache re-enumerates every delimiter and query spelling (the cause of defect 10). info() and ls(detail=True) return the cached S3Object by reference, so a caller's mutation changes the cache.
   6. Request parameters and errors. s3_additional_kwargs reaches open, pipe_file and put_file only (documented), not touch, cat_file, cp_file, setxattr or rm; rm_file and mkdir drop per-call parameters; touch(Body=...) writes data. Only mkdir translates ParamValidationError; cat_file and clear_multipart_uploads re-parse __cause__ because the translated OSError carries no S3 code; exists() maps 403 to True for buckets and raises for keys. In append mode, kwargs.update(appended.to_api_repr()) overrides the caller's ContentType and forwards StorageClass, SSE and object-lock fields (documented as keeping the old object's metadata; a design question).
   7. Retries. The default RetryConfig retries ThrottlingException and TooManyRequestsException, Athena's codes; S3's SlowDown, ServiceUnavailable, InternalError and RequestTimeout are retried only by botocore, so the stop scopes (stop_retries_on, unstoppable_retries) govern no S3 retry unless the user adds S3 codes. The docs say only "sent with the retry policy".
   8. Coupling to PyAthena (for step 5 of Benchmark the S3 filesystem against s3fs and decide on a standalone package #1315). The filesystem imports Connection only for two private class variables and re-implements its user-agent merge; importing any filesystem module loads 13 other PyAthena modules through the eager __init__. The retry stop scopes are one ContextVar shared with the cursors (arrow/reader.py, pyathena.aio.util), so a standalone package must own those helpers rather than copy them. S3AioThreadPool.run re-implements pyathena.aio.util.run_in_thread. ReadAheadStream is used only by the Arrow result set. The registry is used only by PolarsCursor's whole-CSV read and by reads with user storage_options. An fsspec.specs entry point named s3 would take the protocol in every process with the package installed, because fsspec registers entry points with clobber=True.
   9. Dead code. S3FileSystem._call and the _client/_retry_config aliases (test seams only), _extra_tokenize_attributes (always None), an unreachable branch of info() (s3.py:698-701), a duplicate invalidation in put_file, the S3Object.get override.
   10. Docs that disagree with the code. docs/aio.md lists "Parallelism: ThreadPoolExecutor" for S3FileSystem, whose batch operations are sequential, and its cancellation paragraph does not say that a delegated upload keeps sending parts and commits; docs/filesystem.md:509-510 says S3File uses S3MultipartWriter (it uses the private session), and :557 mentions asyncio.to_thread; docs/api/filesystem.rst omits S3ClientError, S3Owner and register_s3_filesystem. Undocumented: the s3fs-style client arguments, that connection= ignores them, pool sizing against max_workers, pickling, that S3FileSystem(conn) stays in fsspec's instance cache with its connection, and that only the class form of register_implementation restores another implementation.

   Outside the package

   • Connection exchanges role_arn/serial_number credentials once and stores them as static keys (connection.py:312-344), so every client of the connection, including the shared S3 client, fails with ExpiredToken after duration_seconds.
   • pandas/util.py:273 uses Connection._s3_client_kwargs; result_set.py:336-342 and :1258-1265 call connection.s3_client through retry_api_call without S3Core, so errors are not translated.
   • The register_async_filesystem fixture in tests/pyathena/filesystem/test_s3_async.py:72-79 registers by string and has no effect (input for Audit the filesystem tests changed since the #1239 audit #1328).
4. laughingman7743

   added sub-issues on Oct 26, 2026

   • Pass the source's SSE-C and RequestPayer parameters to the lookups of copies and setxattr #1333
   • Keep the caller's parameters and the open-time pin in S3File #1334
   • Fix recursive chmod, empty key segments, and version paths in listings #1335
   • Make files usable after fork with either filesystem #1336
   • Align AioS3FileSystem with S3FileSystem for cancellation, recursive put, expand_path, and arguments #1337
   • Translate delete-marker versions to FileNotFoundError and fix two S3Core edge cases #1338
   • Fix the filesystem's integration with PolarsCursor, the fsspec registry, pickling, and connection= #1339
   • Consolidate the filesystem executors before parallel batch operations #1332
   • Remove the duplication between S3FileSystem and AioS3FileSystem #1340
   • Settle the response models, S3Object, and the listing cache #1341
   • Decide request-parameter coverage, error translation, and S3 retry codes #1342
   • Document that S3Core copies of a delete-marker source version raise OSError(EINVAL) #1346