[laughingman7743]

WHAT

The filesystem's thread pools become one type:

• S3AioThreadPool is renamed S3ThreadPool. S3ThreadPoolExecutor and S3AioThreadPool remain as aliases of it, documented as such in the API reference.
• S3AioExecutor remains as a subclass of S3ThreadPool for compatibility. It no longer dispatches the calls through an asyncio event loop: they run directly in the pool. Its loop argument is ignored, with a DeprecationWarning if one is given, and its default max_workers is unchanged.
• AioS3FileSystem._create_executor(), which created the S3AioExecutor of each file, is removed: AioS3FileSystem.open() takes the executor from the wrapped S3FileSystem._create_executor(), so its files run their range reads and parts in an S3ThreadPool of max_workers threads of the file, as S3File does.
• docs/aio.md, the API reference, the benchmark notes, and the docstrings and comments that described the loop dispatch are updated.

Release note (behavior changes):

• The files of AioS3FileSystem.open(), and S3AioExecutor, no longer need fsspec's event loop to run: a call runs to its end when the loop stops, instead of being cancelled before it starts, and S3AioExecutor.submit() no longer raises RuntimeError without a running loop.
• In a forked child process, the files of an AioS3FileSystem created in the parent no longer hang on their executor, which used the parent's event loop. The docs say to create a new filesystem in the child: one created in the parent keeps the parent's boto3 client, and fsspec's synchronous methods of an AioS3FileSystem created in the parent, such as ls(), raise RuntimeError there, as before.
• S3ThreadPoolExecutor is now S3ThreadPool: its max_workers is optional (default min(32, CPUs + 4)), and it has run(), run_shielded(), wait_shielded(), and release(). S3ThreadPoolExecutor, S3AioThreadPool, and S3AioExecutor are no longer distinct for isinstance(): an S3AioExecutor is an instance of the other two.
• Passing loop to S3AioExecutor emits a DeprecationWarning.
• S3AioExecutor.shutdown(wait=True) waits for the submitted calls also on the thread of the event loop, where it used to return at once because the loop dispatched them; this includes closing an AioS3File, such as at garbage collection, on that thread.

WHY

Part of #1332: step 2a of the implementation decisions, decision 6 of the design.

The loop dispatch of S3AioExecutor added failure modes and no function: it needed a running loop, hung after a fork (#1336), could not be waited for on the loop's thread, and left futures pending when the loop stopped while the write session waited for them. One pool type lets step 2b give each filesystem one request pool that its files share.

TEST

Tested commit: 06ff663 (the full suite at 79129d7; the later commits change docstrings, docs, comments, the removed AioS3FileSystem._create_executor(), and the tests, whose modules were rerun).

• just lint, just benchmark lint, and just docs lint (0 errors): passed. The API reference, rendered from the working tree with sphinx-build, shows S3ThreadPool, the two aliases with their own one-line description, and S3AioExecutor.
• uv run --env-file .env pytest -n 8 tests/pyathena/filesystem/, including the AWS integration tests: 1410 passed, 6 skipped at 79129d7. The unit modules of the filesystem also pass with -W error::DeprecationWarning (379 passed, 3 skipped at 79129d7; test_s3_executor.py, test_s3_async.py, test_s3_writer.py: 201 passed, 3 skipped at 5d56657; test_s3_executor.py, test_s3_async.py: 168 passed, 3 skipped at dbb262c).
• Tests:
  • New: TestS3AioExecutor::test_loop_ignored (the warning, and a call without a running loop) and TestAioS3FileSystem::test_read_in_forked_child (a file opened in a child by a filesystem created in the parent reads its ranges in parallel, counted in the child); it replaces test_open_in_forked_child, which asserted the RuntimeError of Make files usable after fork with either filesystem (GH-1336) #1353.
  • Rewritten: the three submit_to() tests of a callable that the executor drops, fails, or fails after the caller cancelled it, which used S3AioExecutor's loop, now use an executor that holds the submitted calls.
  • Removed, because the behavior no longer exists or no longer depends on the loop: the dispatch, shutdown, fork, and cancellation tests of S3AioExecutor's loop (ten tests), TestS3ThreadPoolExecutor::test_fork (the same class as TestS3ThreadPool::test_fork), and test_discard_on_event_loop_thread (the cancellation of parts that have not started is still covered by test_discard_waits_for_running_parts), and TestAioS3File::test_failed_read_frees_other_ranges, which now ran the same S3File code and pool as TestS3FileSystem::test_failed_read_frees_other_ranges. The parametrizations of TestS3Executor over the two classes, now one, are dropped.
  • TestS3AioThreadPool is renamed TestS3ThreadPool, which changes the IDs of its tests.
• Mutation check: no warning for a given loop, a call to self.loop in _create_executor, and three changes to _S3WorkItem.settle() each fail the corresponding new or rewritten test. Without the after-fork reset of the pool, TestS3ThreadPool::test_fork fails; test_read_in_forked_child still passes because files still create their own pools.
• just benchmark test: not run locally (Three test_fleet.py tests fail locally while passing in CI #1354); the Benchmark tooling workflow runs it.

Not run yet: the AWS CI jobs, which start when the PR is marked Ready.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (implementation behavior): FINDINGS, repaired in 5d56657 and f855e7f.

Scope: git diff c135e214da966bf8788dfd122d6a94a7722468e2..79129d7a (base = merge-base with master), all 14 files: s3_executor.py, s3_async.py, s3.py, s3_writer.py, docs/aio.md, docs/api/filesystem.rst, the benchmark notes, and the tests. Perspectives: behavior and failure paths (calls without a running loop, loop shutdown, fork, close on the loop thread, submit_to() with executors that drop calls), compatibility of the aliases and of S3AioExecutor, simplicity and test quality (/code-review high, then /simplify with four angles), and the removed tests.

Repaired:

• The class docstring of S3ThreadPool named itself as an alias (my global rename); it names S3AioThreadPool now.
• docs/aio.md said the coroutines of a filesystem created in the parent raise in a forked child. Verified: they work there (asyncio.run(fs._info(...)) in a child returned), and only fsspec's synchronous methods (fs.info()) raise "This class is not fork-safe". Corrected.
• AioS3FileSystem._create_executor() only forwarded to the wrapped filesystem, and its docstring said it returned that filesystem's executor, which is created per call; removed, and _open() calls self._sync_fs._create_executor().
• S3FileSystem._create_executor()'s docstring invited asyncio-based overrides; it now says which files and operations take their executors from it.
• The isinstance() change of the aliases is in the release note.

Kept: S3AioExecutor as a subclass, not an alias, keeps S3AioExecutor(loop, max_workers) positional calls and its cpu_count() * 5 default (S3AioExecutor(8) binds loop as before). _S3WorkItem's claim lock stays: an executor passed to S3File(executor=) may cancel its returned future while the call runs, as the old S3AioExecutor did. The alias sentence in the class docstring stays next to the #: entries.

[laughingman7743]

Self-review round 1: repaired in 5d56657. The test counted requests in the parent only; the child now checks that its own read sent more than one ranged request. Mutations: a call to self.loop in the executor path fails this test; removing the pool's after-fork reset fails only TestS3ThreadPool::test_fork, because each file still creates its own pool in the child (step 2b changes that).

[laughingman7743]

Self-review round 1: repaired in 5d56657: the overrides of this double carry @override. Reuse check: _DroppingExecutor (test_s3_writer.py) overrides submit_to itself, and ImmediateExecutor runs calls at once, so neither exercises S3Executor.submit_to()'s handling of a dropped or failed inner future, which these three tests protect. Each fails under its mutation of _S3WorkItem.settle().

[laughingman7743]

Self-review round 2 (claims, callers, evidence): CLEAN after one addition to the release note; no code change.

Scope: git diff c135e214da966bf8788dfd122d6a94a7722468e2..f855e7fe11c7e12368a345b6b692b82f791b00ec, the PR body, the docstrings, comments, docs/aio.md, docs/api/filesystem.rst, the benchmark notes, and the commit messages.

Claims checked:

• Without a running loop, S3AioExecutor.submit() runs the call (test_loop_ignored); queued calls run to their end when a loop stops, since nothing dispatches them through it any more.
• Forked child: a file of a parent-created AioS3FileSystem reads in parallel (test_read_in_forked_child); its coroutines work (asyncio.run(fs._info(...)) in a child), and only fsspec's synchronous wrappers raise; the only self.loop use in s3_async.py is the synchronous mv() wrapper (s3_async.py:445).
• S3ThreadPool defaults to min(32, CPUs + 4) and starts calls in submission order (one ThreadPoolExecutor queue); the API reference renders both aliases with their own line (built with sphinx-build from the working tree, since just docs build renders only master and tags).
• No reference to the removed loop dispatch remains in pyathena/ or docs/*.md outside the compatibility notes; the benchmark history files describe past measurements and are unchanged.

Existing callers: positional S3AioExecutor(loop, max_workers) and its default keep working; code that passes loop gets a DeprecationWarning (an error under -W error); isinstance() distinctions between the three names are gone (in the release note).

Addition to the release note: S3AioExecutor.shutdown(wait=True) now waits for its calls also on the event loop's thread, including when an AioS3File is closed there, where it used to return at once.

Evidence limits: the full suite ran at 79129d7; the later commits change docstrings, docs, comments, the removed override, and tests whose modules were rerun. No AWS request path changes.

[laughingman7743]

Independent review (relayed): Codex CLI 0.166.0, model gpt-6-astra, codex exec -s read-only (session 01a1a230-b700-7310-b686-af873c062562), static review of a detached snapshot of f855e7fe11c7e12368a345b6b692b82f791b00ec against merge base c135e214da966bf8788dfd122d6a94a7722468e2, with the diff and the repository conventions, without the PR text or the self-review conclusions; no edits, tests, or network; both worktrees unchanged afterwards. Covered: constructor signatures, defaults and aliases; submission, shutdown, GC, interpreter exit and fork; file and write-session cancellation and settling; docs and API reference; added, rewritten, and removed tests. Verdict: FINDINGS (three).

Finding 1 (P2, pre-existing client limitation exposed by the docs): a filesystem created in the parent keeps the parent's boto3 client; if a parent thread held, for example, the credential-refresh lock at the fork, a child request that refreshes blocks. The docs promised that the parent's filesystem's open() works in the child, and the mocked fork test does not reach the client.

Verified: only the executors are reset after a fork; S3Core.client is the parent's object. Repaired in dbb262c: the docs tell to create a new filesystem in a forked child and give both reasons (the parent's boto3 client, and fsspec's synchronous methods raising). The release note now says only that the files no longer hang on their executor. test_read_in_forked_child keeps protecting that executor contract with a mocked client.

Finding 3 (P3): TestAioS3File::test_failed_read_frees_other_ranges now ran the same S3File._fetch_range and pool as TestS3FileSystem::test_failed_read_frees_other_ranges, with the same assertions. Verified by comparing both tests; removed in dbb262c, and the synchronous test keeps the coverage.

[laughingman7743]

Independent review (relayed), finding 2 (P3): with several threads, a worker can take the first queued call and be descheduled before it runs it, so a later call may start first; "the calls start in the order they were submitted" overstated it, also in docs/aio.md.

Verified against ThreadPoolExecutor's worker loop (one shared queue, no ordering of starts). Repaired in dbb262c here and in docs/aio.md: the calls are queued in submission order. The same wording in the submit() docstring is pre-existing; its purpose, that a call waiting for earlier ones does not wait for a thread it holds, follows from the queue order, and it is left for step 2b, which rewrites the pool's use.

[laughingman7743]

Repairs and independent follow-ups (relayed).

Self-review of the dbb262c repair, both perspectives: the docs' new reason for creating a filesystem in a forked child matches boto3's guide ("While clients are thread-safe, they cannot be shared across processes due to their networking implementation", Boto3 guide, Low-level clients, "Multithreading or multiprocessing with clients"); the removed test's behavior stays covered by TestS3FileSystem::test_failed_read_frees_other_ranges, which runs the same S3File read on the same pool type.

Follow-up 1 (Codex gpt-6-astra, read-only, session 01a1a23d-3514-7983-811d-ab1f428aaeec, static, f855e7fe..dbb262c1 with docs/aio.md, s3_executor.py, and both test files): findings 1 and 3 resolved; FINDINGS (P2): the submit() docstring (s3_executor.py:258) still promised a start order. I had left it as pre-existing; repaired in 06ff663 instead: the calls are queued in submission order, so the calls submitted before one that waits for them are taken by threads before it.

Follow-up 2 (Codex gpt-6-astra, read-only, session 01a1a23f-b542-7482-929c-041842928510, static, dbb262c1..06ff6632, checked against CPython 3.12's ThreadPoolExecutor worker loop): CLEAN: the statement is accurate, and no sentence in s3_executor.py or docs/aio.md promises a start order any more.