[laughingman7743]

WHAT

• An S3File (and AioS3File) finalized without having been closed is closed in another thread instead of in the finalizing thread when that thread is inside a pass of the cyclic garbage collector or is a thread of an S3ThreadPool. It is still committed, unless autocommit is False. A file whose last reference is dropped in any other thread is still closed by its finalizer before the drop returns, so fs.open(path, "wb").write(data) still commits before the statement ends.
• During a collection, the close starts once the collection has ended (queued per thread and started by the gc stop callback), after the other finalizers of the collection, such as that of the TextIOWrapper of a text-mode file, which the collector may run after the finalizer of the file, have written to the file.
• The close runs in a thread of the _thread module, with an empty contextvars context. Thread.start() takes locks of threading that the finalizing thread may hold, and waits for the new thread, which may take another. The interpreter does not join _thread threads, so an atexit handler waits for the handed-over closes, also for those handed over while it waits; a fork clears them in the child, where a close queued in the parent only marks its file closed.
• S3File.close() called during a collection, such as by the finalizer of a TextIOWrapper, is handed over in the same way, only once, and returns before the file is closed. A later close(), commit(), or discard() of the file waits for the handed-over close to end, so that a transaction does not complete the upload without the data that the close uploads, and an explicit close does not race with it. During a collection or in a thread of an S3ThreadPool, where that wait could last forever, close() returns without waiting, and commit() and discard() raise RuntimeError.
• Before the close is handed over, the write buffer is copied, since the collector finalizes, and so closes, the original BytesIO before the collection ends. A file whose buffer is already closed when its finalizer runs is discarded: its data is incomplete, so it is not committed, and its multipart upload is aborted.
• During interpreter finalization, no thread is started, and the file is closed in the finalizing thread, as before: on Python 3.11, Thread.start() waits forever then, and later versions raise. The same applies once the exit has waited for the handed-over closes, such as in an exit handler that runs after that wait. If no thread can be started otherwise, the file is left unclosed with a warning, and the next close(), commit(), or discard() of the file outside a collection and the pool threads closes it first, also one that already waits. A write that fails during the collection that handed over the close of its file leaves the discard of the write to that close.
• S3ThreadPool.in_pool_thread() reports whether the calling thread belongs to any S3ThreadPool.
• docs/filesystem.md describes how an unclosed file is closed when it is finalized, including during a collection, in text mode, at exit, and at interpreter finalization, the later calls, and that the write of the remaining text of a text wrapper in a pool thread can still wait forever for a part queued behind that thread, as before.

WHY

Closes #1364. The design and the decisions on its open questions are in #1364 (comment), with the addenda below it. The maintainer chose to keep the commit at finalization rather than discard the file.

Since #1359, the parts of a write run in the request pool of the filesystem. A file finalized in a pool thread waited forever when one of its parts was queued behind that thread, or when the collection ran inside one of its own parts. This breaks T1 of #1332. Both hangs were reproduced with a stubbed client before the fix.

Release note

An unclosed S3File that the cyclic garbage collector finalizes, or that is finalized in a thread of an S3ThreadPool, is committed from another thread, which an exit waits for, no longer during its finalization, except during interpreter finalization. Before, its finalizer could wait forever in a request pool thread. close() called during a collection, such as by another finalizer, now also returns before the file is closed; a later close(), commit(), or discard() of the file waits for that close, and during a collection or in a thread of an S3ThreadPool, commit() and discard() raise RuntimeError instead. A file whose write buffer the collector has already finalized is discarded, and its multipart upload aborted, instead of failing with ValueError and leaving the upload incomplete.

TEST

At e5eabc4 (on 758adee, after #1378 and #1394; the changed lines are those of d7e9260 on d03e034):

• just lint, just docs lint: passed.
• tests/pyathena/filesystem/test_s3.py -k "finalized_unclosed or handed_over": 21 passed. The tests cover the last reference dropped in a user thread, collection in a user thread (binary and text mode), collection in the only pool thread with the first part queued behind it, collection inside its own part, the last reference dropped in the pool thread, a buffer finalized first, a thread that cannot start, interpreter finalization (with sys.is_finalizing() patched) and after the exit wait, a later close(), commit(), or discard() that waits for a handed-over close, close() and commit() during a collection and in the pool thread, which do not wait, a later close() or commit() after a failed start, also one that waits before the start fails, and in the pool thread, and a write that fails during the collection. Each handed-over close of a collection is checked to start after the collection, and no finalizing thread starts a thread through threading. A fixture fails a test whose handed-over close does not end, instead of leaving the exit to wait for it.
• Mutation check, 30 mutants, each fails at least one of these tests: the hand-overs in a pool thread and of close() in a collection, the once-guard, the buffer copy, the discard of a finalized buffer, the start after the collection, the finalization and exit checks, _thread instead of threading.Thread, an inline close when no thread starts, the handling of a failed start (marked as ended, not waking waiters, not run later, run where it cannot be waited for, kept for the exit), the deferred discard of a failed write, the end of a close, the closer check, the checks of the wait, the waits of close(), commit(), and discard(), and close() raising or closing where it cannot wait. Not covered by a test: the fork guard of a queued close and the repeated pass of the exit wait.
• tests/pyathena/filesystem/ (including S3 integration): 1744 passed, 6 skipped at e5eabc4 (1652 passed, 6 skipped at aac7cf8, before the rebase).
• A script without patches (stubbed client) commits the text of a collected text-mode file in 30 of 30 runs. At 2cc12e8, the finalizer of the file ran first, and its close raced with the wrapper writing the text.
• Interpreter exit, checked with scripts and a stubbed client on CPython 3.11, 3.12, 3.13, 3.14, and 3.15 at aac7cf8 (not a test): all exit. A file whose last reference a pool thread drops during threading._shutdown, a file collected at module teardown with no part left to upload, and a file collected together with its filesystem and request pool are committed, and so is a file collected by an exit handler registered before PyAthena (3.11 and 3.15); a file collected at teardown with a part left to upload is not committed, and its upload is aborted. Without the atexit wait, the first file is lost on 3.11. At d03e034, files that were not committed raised ValueError from their closed buffer and left the upload incomplete.
• Not covered by a test: the empty context of the new thread, the fork guard of a queued close, and the atexit wait itself (covered by the exit scripts).
• Independent review: Codex (static, read-only), one review and four follow-ups of the full diff, then a narrow follow-up of the last change, which was CLEAN. They found the race with the text wrapper, the transaction commit, the explicit close, the waits in collections and pool threads, thread starts from finalizers, the exit and fork handling of handed-over closes, the failed start, and a failed write during a collection; each is repaired and recorded on the PR. One finding, which needs two threads that close and commit the same file at once, is rejected under threadsafety 2, and the pre-existing wait of a text wrapper's write in a pool thread is documented.

🤖 Generated with Claude Code

[laughingman7743]

Self-review round 1 (behavior and implementation): CLEAN

Base 68fc3befd2ea9595d7df0effe462d845f7500351, head 63bede7dd1ab0d9610055abdc42cc410ba14952b.

Covered: S3File.close() / __del__ / _close_later() / _close_collected() / _track_collection() / _in_collection(), the gc.callbacks registration, S3ThreadPool.in_pool_thread(), AioS3File (inherits S3File unchanged; AioS3StreamedFile keeps its own finalizer), the new tests, and docs/filesystem.md.

Checked:

• Only finalizers can call close() while the collection flag is set. The flag is per thread, set by the start phase and cleared by the stop phase, both of which CPython runs in the collecting thread. Explicit closes in normal code, including with blocks and the task-pool threads of put/pipe, are unchanged.
• Objects freed by refcount while a collection runs, such as a wrapper held only by a cycle, are covered by the same flag.
• Resurrection: the finalizer runs once per object, both in a collection and for a refcount drop, because Python classes are GC-tracked. The handed-over thread holds the file until it ends.
• Buffer: getvalue() followed by BytesIO(bytes) shares the bytes, so at most one copy is made. A closed buffer leads to _close_without_commit() (discard and abort), and a read file has no buffer.
• Partially initialized files: closed defaults to True through getattr, as before. Inherited files return without a request.
• Interpreter shutdown: if Thread.start raises, it raises RuntimeError or PythonFinalizationError, a subclass, and the file is left unclosed.
• Mutation check: all 5 guards are killed by the 8 tests.

Pre-existing and out of scope, recorded in the #1364 design comment: a file collected together with its filesystem and pool, a last reference dropped in a thread of an executor passed with executor=, and a text wrapper's flush during a collection.

[laughingman7743]

Self-review round 2 (claims, compatibility, operations): FINDINGS, repaired.

Scope: git diff d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d..2cc12e8f3621374480bf7276d338a8cfcb14a173 (base d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d, head 2cc12e8f3621374480bf7276d338a8cfcb14a173). Round 1 was recorded at 63bede7 on 68fc3be; a range-diff of the branch before and after the rebase onto d03e034 shows both patches unchanged (63bede7 = 8589ecb; the repair was committed before the rebase and is e5911ed after it). The upstream changes of #1395 (S3Executor.run(), _S3TransactionWrite, glob(), batch limits) do not touch the close or finalization of S3File/AioS3File; _S3TransactionWrite discards its files with discard(), not through a finalizer.

Finding 1, interpreter finalization (regression, repaired in e5911ed). At 63bede7, a file collected during interpreter finalization was handed to a new thread. On 3.12 and later, Thread.start() raises then, so the file was left unclosed, where 68fc3be closed it inline. On 3.11, Thread.start() does not raise and waits forever for a thread that never runs: an exit script with an unclosed file in a reference cycle hung at module teardown (faulthandler: __del__ → close → _close_later → Thread.start → Event.wait). The file is now closed here, without starting a thread, when sys.is_finalizing(). No other thread runs requests then, and the request pools take no more calls (checked: S3ThreadPool.submit() raises cannot schedule new futures after shutdown from a finalizer at teardown on 3.11 and 3.15), so the close does not wait for a part queued behind it. Exit scripts after the repair, on 3.11, 3.14, and 3.15, all exit: a file dropped by a pool thread during threading._shutdown is committed by the new thread; a multipart file collected at teardown with all parts sent is committed on 3.11 and 3.14, and discarded with its upload aborted on 3.15, where its buffer is finalized first. test_finalized_unclosed_file_without_thread[True] covers the branch; replacing the check with False fails it, and with True (always inline) fails 8 of the 9 finalization tests.

Claims checked.

• "Last reference dropped in any other thread → committed before the drop returns": test_finalized_unclosed_file[last reference] asserts the commit runs in the test thread.
• "The collector closes the original BytesIO before the new thread runs": the collection cases hold the close until the finalization has returned; removing the copy fails four cases.
• "A file whose buffer is closed is discarded and aborted": test_finalized_unclosed_file_with_finalized_buffer; at teardown on 3.15, 68fc3be/d03e034f raise ValueError from the closed buffer and leave the upload incomplete, which the release note now says.
• "If a thread cannot be started otherwise, the file is left unclosed with a warning": test_finalized_unclosed_file_without_thread[False].
• "AioS3File gets the same behavior": it subclasses S3File without overriding close/__del__ after the rebase.

Deferred, with reason. The empty contextvars context of the new thread has no test; the custom executor= and filesystem-collected-with-its-pool limits stay documented on #1364 as before.

[laughingman7743]

Self-review round 2, finding 2 (documentation, repaired in e5911ed and 2cc12e8).

• The bullet "During interpreter shutdown, a file whose last part can no longer be submitted is closed without committing it, and its multipart upload is aborted" had not been reproduced at 63bede7, and the shutdown it named (threading._shutdown) is not where the pools stop taking calls for a finalizer. It is replaced by the finalization bullet here, which an exit script confirms: with data left to upload as a part at module teardown, the file is not committed and its upload is aborted on 3.11 (the part is rejected) and on 3.14 and 3.15 (its buffer is finalized first).
• The first bullet said that a file whose last reference is dropped outside a pool thread is closed before the drop returns. A drop by another finalizer during a collection is handed over, as close() checks the collection, so the bullets now name "during a pass of the cyclic garbage collector" first and "elsewhere" second.
• The finalized-buffer sentence applies to collections at finalization as well, so it moved out of the first bullet.

just docs lint passes.

[laughingman7743]

Independent review (relayed): Codex CLI 0.166.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1d05b-204e-7983-acb6-018e09215197. Static review only (no tests, builds, network, or GitHub access), of a clean snapshot of 2cc12e8f3621374480bf7276d338a8cfcb14a173 against base d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d, without the PR number, description, commit messages, or prior findings. Result: FINDINGS. Covered: the finalization methods, pool-thread detection, request scopes, write-session completion and abort, explicit close(), with, transactions, autocommit=False, text wrappers, read mode, AioS3File, AioS3StreamedFile, collection flags, fork guards, shutdown paths, docs, and every added test.

Reviewer finding 1 (P1): a deferred close can let a transaction commit incomplete data. A transaction keeps the binary file; the finalizer of a cyclic text wrapper flushes a short tail into it and calls close(), which is handed over and returns. If the transaction commits before the new thread uploads the tail, commit() completes only the submitted parts. The same applies to any close() of a live file during a collection, such as a with block in a finalizer.

Reviewer finding 2 (P2): an explicit close can race with the deferred close. After the wrapper hands over the close, an explicit f.close() enters _close() independently: if the new thread has set fsspec's forced, it skips the flush, marks the file closed, and shuts down the executor while the new thread uploads, so its remaining part fails and the upload is aborted.

Verification: both confirmed by reading close(), commit(), and fsspec's AbstractBufferedFile.close()/flush() at 2cc12e8.

Repair (5c0cbe7): close(), commit(), and discard() wait for a handed-over close (_wait_for_close_later, here), except in the thread that runs it and during a collection, whose end starts it. test_close_handed_over_is_waited_for[close-True/close-False/commit-False] holds the handed-over close and checks that a close() or commit() from another thread is still waiting after 0.2 s, then that the object includes the tail. Removing the wait in close() or in commit() each fails one case (with autocommit=True, the commit inside the flush waits instead, so close-False covers close()).

[laughingman7743]

Reviewer conclusion (docs): "closed in a new thread after the finalization returns" is not established, since Thread.start() gives no ordering, and the text-mode case of test_finalized_unclosed_file passes only because the helper holds the close until the collection returned, a synchronization that production lacks.

Verification: confirmed, and it was a defect, not only a wording issue. A trace at 2cc12e8 without the hold shows that the collector runs the finalizer of the S3File before that of its TextIOWrapper: the new thread started and flushed while the wrapper was still writing its text into the file in the collecting thread.

Repair (5c0cbe7): a close handed over during a collection is queued per thread and started by _track_collection when the collection stops (here), after every finalizer of the collection. The helper no longer holds the close, and it records whether each close was started inside a collection. Starting immediately again fails 6 of the 13 finalization tests; not starting the queued closes fails 10. A production-ordering script (no patches) now commits the text of a collected text-mode file in 30 of 30 runs. docs/filesystem.md now says that the thread starts once the collection has ended, and makes no ordering claim for pool threads.

[laughingman7743]

Reviewer finding 3 (P1, pre-existing at the base): text-wrapper finalization in a pool thread can still deadlock. If the last reference to a TextIOWrapper is dropped in the only request-pool thread outside a collection while a part of its file is queued behind that thread, the wrapper calls S3File.close() inline, which does not check in_pool_thread().

Verification: confirmed as a code path, present at d03e034. Deferred, with reason: close() cannot hand over every call in a pool thread, since the batch operations (pipe, put) open and close their files in pool threads and must commit before returning; their parts run inline there (T1), so they do not wait. The path needs a pool call that holds the last reference to a user's text wrapper, which no PyAthena call does; a collection in a pool thread is handed over. The docs paragraph here now states that the wrapper closes the file in the thread that finalizes it, outside a collection.

Other reviewer conclusions, with dispositions:

• The collection flag is thread-local and not nesting-counted: no change; CPython does not run a collection inside another, or two at once (gcstate->collecting).
• Fork does not reset the thread-local markers: no change; no scenario found (a file inherited by a child is only marked closed).
• Exceptions of a synchronous finalizer are not contained, as at the base: no change. Thread creation failures other than RuntimeError are now also contained, so that a queued close that cannot start is never waited for forever.
• Shutdown behavior was not verifiable statically: exit scripts with a stubbed client on CPython 3.11, 3.12, 3.13, 3.14, and 3.15 at 5c0cbe7 all exit; a file dropped by a pool thread during threading._shutdown and a file collected at module teardown with no part left to upload are committed, and one with a part left to upload is not committed and its upload is aborted.
• test_finalized_unclosed_file_without_thread[True] checks branch selection, not real interpreter finalization: agreed; the exit scripts are the evidence for that, as stated in the PR body.
• The log-text assertions: kept, since the warning is the only signal to the caller that the data was not committed.

[laughingman7743]

Independent follow-up review (relayed): Codex CLI 0.166.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1d083-79ef-7dd3-af4a-3b579fa258f3. Static review only, of a clean snapshot of 5c0cbe713a86a336740f03bd5f6be87f17b5d449 against base d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d (full diff, since 5c0cbe7 expanded the contract), without the PR number, description, commit messages, or prior findings. Result: FINDINGS (5). Covered: the finalization methods, pool and scope scheduling, collection ordering and thread-local state, fork handling, explicit close and with, transactions, text and read modes, both aio file types, docs, and all added tests; it read fsspec's source and the CPython 3.11–3.15 threading sources.

Finding 1 (P1): a commit() during a collection can publish an incomplete multipart object. A finalizer calls file.close(); file.commit() on a file with autocommit=False, a sent part, and a short tail: close() hands the close over, and commit(), which does not wait during a collection, completes only the submitted parts.

Finding 2 (P1): waiting for a handed-over close can deadlock a request-pool worker. If a pool worker calls close(), commit(), or discard() on such a file, it waits for the close, which submits its final part to the same pool behind that worker.

Verification: both confirmed at 5c0cbe7 (_wait_for_close_later skipped the wait in a collection and waited in a pool thread). Repair (00c894f), here: during a collection or in a thread of an S3ThreadPool, close() returns without waiting and without closing, and commit() and discard() raise RuntimeError (_raise_unless_closed_later), instead of committing incomplete data or waiting forever. test_close_handed_over_is_not_waited_for[collection/pool] writes a sent part and a tail, calls close() and commit() from a finalizer during the collection and from the only pool thread, and checks that close() leaves the file open, commit() raises, no upload is completed, and a later close() and commit() from the test thread commit b"abcdefg".

Finding 5 (P2): waiter.join(0.2) does not establish that the caller reached the wait. Confirmed: scheduling-dependent. Repair: test_close_handed_over_is_waited_for now observes the call of wait() on the event of the handed-over close before it releases the close, and covers discard() as well.

Mutation check at 00c894f, 20 mutants of -k "finalized_unclosed or handed_over" (16 tests), each fails at least one test, including: no collection or pool check in the wait, close() closing when it cannot wait, commit() proceeding when it cannot wait, and no wait in close(), commit(), or discard().

[laughingman7743]

Finding 3 (P1): starting a thread inside the gc callback can deadlock on Python 3.11/3.12. A collection can run while the interrupted code holds threading._shutdown_locks_lock; Thread.start() waits for the new thread, whose bootstrap takes that lock.

Verification: confirmed by reading threading.py of 3.11 and 3.12 (_set_tstate_lock takes _shutdown_locks_lock before _started.set(), and _maintain_shutdown_locks allocates under it). Every version also takes the non-reentrant _active_limbo_lock in Thread.start(), which threading.enumerate() holds while it allocates lists. Not reproduced at runtime.

Repair (00c894f), here: a handed-over close runs in a thread of the _thread module, whose start takes no lock of threading and does not wait for the new thread. The interpreter does not join such threads, so an atexit handler waits for the handed-over closes (_wait_for_handed_over_closes). A first variant that started a threading.Thread from a _thread trampoline lost the close of a file dropped by a pool thread during threading._shutdown on 3.11, 3.12, and 3.14 in exit scripts, since the join of non-daemon threads ended before the trampoline started the thread; it was not committed. With the atexit wait, exit scripts on 3.11, 3.12, 3.13, 3.14, and 3.15 all commit that file, and removing the atexit registration loses it again on 3.11. The test helper now records any threading.Thread.start() by a finalizing thread, except a worker of the request pool that the wrapper of a text-mode file starts when it writes; starting the close with threading.Thread fails 7 tests.

[laughingman7743]

Finding 4 (P2): closes queued in a collection and inherited across a fork bypass the process guard. A fork between the hand-over and the end of the collection would run the parent's close in the child, where _close() shuts down an inherited scope that can wait forever.

Verification: confirmed as a code path; it needs a fork from a finalizer or a gc callback. Repair (00c894f), here: _start_close only marks an inherited file closed, and os.register_at_fork clears the set of closes that an exit waits for in the child. No test: it needs a fork during a collection.

Other reviewer conclusions: the log-text assertions are kept, since the warning is the only signal that the data was not committed; test_finalized_unclosed_file_without_thread[True] checks branch selection, and the exit scripts are the evidence for real finalization; the pre-existing text-wrapper path in a pool thread outside a collection stays as recorded in the first review.

[laughingman7743]

Independent follow-up review 2 (relayed): Codex CLI 0.166.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1d0d1-a385-78c1-896f-373358a35263. Static review only, of a clean snapshot of 00c894f34cbab63f9d3b7004c53b0f3070434aed against base d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d (full diff), without the PR number, description, commit messages, or prior findings. Result: FINDINGS (4). Covered: the finalization methods, pool-thread detection, scopes, write-session completion and abort, fsspec's buffered file, text wrapping, transactions, explicit close, with, read mode, autocommit=False, both aio file types, GC ordering, thread-local state, fork, the 3.11–3.15 shutdown order, docs, and every added test.

Finding 1 (P1): a failed hand-over permits a truncated transaction commit. If no thread can be started for a handed-over close of an autocommit=False file with a sent part and a buffered tail, the event was set as if the close had ended, so a later commit() completed only the sent parts.

Verification: confirmed at 00c894f. Repair (here): a close for which no thread could be started is not marked as ended; it is removed from the closes that an exit waits for, and the next close(), commit(), or discard() outside a collection and the pool threads runs it first (_wait_for_close_later). test_close_handed_over_not_started[close/commit] fails the start, then checks that a later close() (autocommit) or commit() (no autocommit) commits b"abcdefg"; marking the failed close as ended fails commit, and not running it later fails both.

[laughingman7743]

Finding 2 (P2): a forked child can block on an inherited event lock. If the collecting thread forks while another thread holds the condition lock of the event of a handed-over close, _start_close in the child set that event and could block forever.

Verification: confirmed as a code path (a fork from a finalizer or a gc callback). Repair (here): for an inherited file, _start_close only marks it closed and leaves the event alone; no thread of the parent waits in the child, a later close() returns at the fork check, and commit()/discard() raise there. No test, as before.

Finding 3 (P2): the exit waited for one snapshot of the handed-over closes. A close handed over while the exit waits, such as by a collection in a closing thread, was not waited for. Verification: confirmed. Repair (here): _wait_for_handed_over_closes waits until the set is empty, and _end_close_later removes a close before it sets its event. Not covered by a unit test; the exit scripts cover the single-close case.

[laughingman7743]

Finding 4 (P2): the deadlock tests do not bound process termination. A regression that leaves an event unset would make the atexit wait hang the test process after the test fails, and a blocked pool worker would hang the exit join of ThreadPoolExecutor.

Verification: confirmed for the event: before this change, a test that replaced the event left four events that the exit waited for. Repair (here): the handed_over_closes_end fixture of the tests that hand over a close waits for each of its closes with a timeout, then clears them, so a close that does not end fails the test instead of hanging the exit; removing the step that drops a failed start from the exit set now fails a test in 10 s instead of hanging. Deferred, with reason: a pool worker that a regression blocks forever still keeps ThreadPoolExecutor's exit join waiting; isolating these tests in subprocesses would change how this suite runs them, and the regressions that would block a worker fail first with a timeout.

Other reviewer conclusions: the text-wrapper flush that waits for the part window inside its own part during a collection, and unraisable exceptions of an inline close, are pre-existing, as recorded before. The log-substring and close-count assertions are kept: the warning is the only signal that the data was not committed, and the count checks that a close is handed over once.

[laughingman7743]

Independent follow-up review 3 (relayed): Codex CLI 0.166.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1d0fe-c25a-7011-84b6-9b6a98cea1fa. Static review only, of a clean snapshot of 4b63a6634f2746373523a74e6fc2c100b2d41a72 against base d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d (full diff), without the PR number, description, commit messages, or prior findings. Result: FINDINGS (4, all P2). Covered: the finalization and waiting methods, buffer preservation, write-session completion and abort, scopes, collection ordering, thread-local state, fork hooks, explicit close, with, transactions, autocommit=False, text wrappers, reads, both aio file types, shutdown ordering, thread creation, docs, and all added tests.

Finding 2: a failed start strands a caller that already waits. A caller that started waiting before the start of the handed-over close failed was never woken, since the failure recorded the close for a later call without setting the event. Verification: confirmed. Repair (aac7cf8, here): the failure sets the event, and a woken caller runs the close where it can wait; during a collection or in a pool thread, close() returns and commit()/discard() raise as before. test_close_handed_over_not_started[commit-True] starts the caller before the failure; [close in pool-False] checks that a pool thread leaves the file open and commit() raises there. Not waking the caller fails 4 tests; running the close in a pool thread fails 1.

[laughingman7743]

Finding 1: a write that fails during the collection loses its abort. After the finalizer of the file hands over its close, a text wrapper writes; the write finds a failed part and calls _close_without_commit(), whose discard() the collection refused, so the abort was only logged, and the handed-over close then saw a closed file. Verification: confirmed. Repair (here): when the discard cannot wait for the handed-over close, _close_without_commit() leaves it to that close, which discards the write once it ends. test_close_handed_over_failed_write_is_discarded fails a part, writes from a finalizer after the hand-over, and checks one abort and no completion; not deferring, or not running the deferred discard, each fails it.

[laughingman7743]

Finding 3: the exit misses closes handed over by exit handlers that run after its wait. An atexit handler registered before PyAthena is imported runs after the wait, so a close that it hands over started a thread that nothing waited for. Verification: confirmed. Repair (here): once the exit has waited, a handed-over close is run in the finalizing thread, as during interpreter finalization (the pools take no more calls by then). test_finalized_unclosed_file_without_thread[exiting] runs the wait, then collects a file and checks that the collecting thread commits it; removing the flag or the check fails it. An exit script on 3.11 and 3.15 with such a handler commits the file before the handler returns.

[laughingman7743]

Finding 4: a regression could hang test_close_handed_over_is_not_waited_for, whose final close() and commit() ran in the test thread. Verification: confirmed. Repair (here): they run in a daemon thread with a bounded join.

Mutation check at aac7cf8: 29 mutants of -k "finalized_unclosed or handed_over" (21 tests), each fails at least one test. Pre-existing, as recorded before: the text-wrapper flush that can wait in a pool worker before it reaches close(), and unraisable exceptions of an inline close.

[laughingman7743]

Independent follow-up review 4 (relayed): Codex CLI 0.166.0, model gpt-6-astra, reasoning effort high, sandbox read-only, session 01a1d138-c23b-7252-bcf8-2190012cc9d2. Static review only, of a clean snapshot of aac7cf8385a8a2c765fe48563f45117895ccebf8 against base d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d (full diff), without the PR number, description, commit messages, or prior findings. Result: FINDINGS (3). Covered: the finalization, hand-over, waiting, commit/discard, and collection-tracking methods, S3ThreadPool, _S3PoolScope, the write session, fsspec's __del__/close/flush/text-wrapper paths, explicit close, transactions, read mode, both aio file types, fork guards, the 3.11–3.15 shutdown paths, docs, and every added test.

Finding 1 (P1): a fallback close is advertised as finished before it runs. After a failed start, caller A claims the close of the file and runs it, while caller B's commit() sees the event set and no unstarted close, and completes the upload with the sent parts only.

Rejected, with reason: the scenario needs two threads that call close() and commit() of the same file at once. PyAthena's threadsafety is 2, and a file object is not safe for concurrent use from several threads, as in fsspec; the PR keeps that contract rather than adding a lock for it. With one caller, as in test_close_handed_over_not_started, the close ends before the call returns. The waits added in this PR serve the thread that PyAthena itself starts for the close, not concurrent callers.

[laughingman7743]

Finding 2 (P2): the test accepted a RuntimeError from close() as the one expected from commit(). Verification: confirmed. Repair (d7e9260, here): close() is checked to return with the file open, separately from the RuntimeError of commit(); making close() raise when it cannot wait now fails both parameters.

Finding 3 (P1, pre-existing): the write of the remaining text of a text wrapper can wait forever in a pool thread for a part queued behind that thread, before the close is reached; the hand-over of the close cannot help. Deferred, with reason, and documented (d7e9260): the docs paragraph on text mode now states it. It is present at d03e034, it needs a text wrapper finalized in a request-pool thread, and handing over the flush would change when the text of every collected wrapper is written. Unraisable exceptions of an inline close are pre-existing in fsspec, as recorded before.

Follow-up review 5 (relayed, narrow): Codex CLI 0.166.0, gpt-6-astra, effort high, read-only, session 01a1d147-60b6-7560-bad2-38119c6a3929, of git diff aac7cf83..d7e92605 (this test and the docs paragraph) on a clean snapshot of d7e9260. Result: CLEAN. It confirmed that closed == [False] checks close() in both parameters (through Future.result() in the pool case), that only commit() is inside the RuntimeError handler, and that the paragraph matches S3File.write() → _upload_chunk() → session.wait_ready().

[laughingman7743]

Self-review of the repairs after the independent reviews (2cc12e8f..d7e92605; base d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d, head d7e926057384566f0567f9679121a445658133e5), both perspectives, run after each repair commit (5c0cbe7, 00c894f, 4b63a66, aac7cf8, d7e9260) and recorded together.

Round 1, behavior: FINDINGS, each repaired in the commit that followed.

• Every state of a handed-over close and the calls that meet it: queued in a collection (started by the stop callback, never in the collecting frame); running in a _thread thread (callers outside a collection and the pool threads wait; inside, close() returns and commit()/discard() raise; the closing thread itself never waits, through _closer_ident); ended (event set, removed from the exit set before it is set); not started (event set to wake waiters, _unstarted_close run by the next safe caller, removed from the exit set); inherited across a fork (only marked closed); after the exit wait or during finalization (run inline).
• Found in self-review, not by the reviewers: a test that replaced the event left four events that the atexit wait waited for, which made each test process exit about 9 s late (repaired in 4b63a66, with a fixture that fails a test whose close does not end); the event was registered for the exit before the buffer copy, so an exception in the copy would have left the exit waiting forever (moved after the copy); a _thread trampoline that started a threading.Thread lost the close of a file dropped by a pool thread during threading._shutdown on 3.11, 3.12, and 3.14 (replaced by closing in the _thread thread with the atexit wait, 00c894f); a thread-creation exception other than RuntimeError in the stop callback would have skipped the remaining queued closes (contained per file).
• Tests: -k "finalized_unclosed or handed_over": 21 passed twice before aac7cf8 was committed, and once at d7e9260. Mutation check: 29 mutants at aac7cf8 plus one at d7e9260 (close() raising when it cannot wait), each fails at least one test; the fork guard of a queued close and the repeated pass of the exit wait are not covered by a test.

Round 2, claims and operations: CLEAN after the docs changes in each repair.

• Callers: close() during a collection now returns before the file is closed, and commit()/discard() of a file whose handed-over close runs raise RuntimeError during a collection or in a pool thread; both are in the docs and the release note. Outside a collection and the pool threads, a with block and an explicit close() are unchanged.
• Operations: an exit waits for every handed-over close that runs, as it waited for the non-daemon thread of the first design; closes handed over after that wait run in the finalizing thread. Exit scripts with a stubbed client on CPython 3.11–3.15 at aac7cf8 (no code change since): a file dropped by a pool thread during threading._shutdown, files collected at teardown with no part left, a file collected with its filesystem and pool, and a file collected by an exit handler registered before PyAthena are committed; a file collected at teardown with a part left is aborted. Without the atexit wait, the first is lost on 3.11. A script without patches commits the text of a collected text-mode file in 30 of 30 runs.
• tests/pyathena/filesystem/ with S3 integration: 1652 passed, 6 skipped at aac7cf8; d7e9260 changes only one test and the docs, and its finalization tests pass.
• Claims: each docs sentence of the finalization section was checked against the code paths above; the Python-version claims (3.11 Thread.start() waits forever during finalization; 3.12 and later raise; _shutdown_locks_lock on 3.11/3.12; _active_limbo_lock on every version) come from the exit scripts and the threading sources of each version.

[laughingman7743]

Rebase onto 758adee (after #1378 and #1394), head e5eabc4e69c5b00c5c3e3b7f8209867276322b26, base 758adee9; previous head d7e926057384566f0567f9679121a445658133e5 on d03e034f947b9ab6ad5696ec2bd942f8a63e7b5d.

• Conflicts: the module constants here (Inject the filesystem's S3 client with client= and retry_config= (GH-1363) #1378 added _UNPICKLABLE_CLIENT_MESSAGE and _CONNECTION_DEPRECATION_MESSAGE above _collecting) and the pyathena.filesystem imports of the tests (Inject the filesystem's S3 client with client= and retry_config= (GH-1363) #1378 added s3_filesystem); both kept.
• git range-diff d03e034f..d7e92605 758adee9..e5eabc4e69c5b00c5c3e3b7f8209867276322b26: the changed lines of the PR are identical, except import sys, which Inject the filesystem's S3 client with client= and retry_config= (GH-1363) #1378 already added upstream.
• Upstream changes checked against the contracts of this PR: Inject the filesystem's S3 client with client= and retry_config= (GH-1363) #1378 (injected client, pickling of filesystems and read files, AioS3FileSystem arguments) and Accept os.PathLike paths in every S3 filesystem method (GH-1380) #1394 (os.PathLike paths) change no code of S3File or s3_executor.py; the last upstream hunk in s3.py is above class S3File. AioS3File still subclasses S3File without overriding close or __del__.
• At e5eabc4: just lint, just docs lint pass; -k "finalized_unclosed or handed_over": 21 passed; tests/pyathena/filesystem/ with S3 integration: 1744 passed, 6 skipped; the offline checks of the PR pass.