Remove and rewrite unnecessary async filesystem and core tests by laughingman7743 · Pull Request #1259 · pyathena-dev/PyAthena

laughingman7743 · 2026-10-17T14:18:58Z

WHAT

Apply the rows of #1241 to the asynchronous filesystem, core, executor, path-pairing, path and error tests: filesystem/test_s3_async.py, test_s3_core.py, test_s3_executor.py, test_s3_path_pairing.py, test_s3_path.py and test_s3_errors.py. Several rows cite the synchronous tests that #1254 (#1240) rewrote as covers; this branch is rebased onto master after its merge. Each row was re-checked on that head. For each REMOVE or MERGE row, the behavior was broken in pyathena/ and a remaining test had to fail offline; the source changes were reverted, and this diff changes no file under pyathena/.

Remove the real-S3 TestAioS3FileSystem tests of methods that are one-line delegations to the synchronous filesystem (_info, _ls, _exists, _cat_file, _rm_file, _touch, _get_file, _mkdir, checksum, sign, the metadata and tag methods, put_file/pipe_file outside a transaction), together with the checksum multipart cases, small writes and appends that never use the aio executor, and test_invalidate_cache, which passed with invalidation broken. Keep one size of test_write_transaction (10 MiB, the only real-S3 multipart write through S3AioExecutor) and of test_upload_cp_file, and 2 files instead of 10 in test_rm_recursive.
Rewrite the offline async tests to go through public calls and request-level fakes: the single-request multipart copy and test_rm_maxdepth use Stubber, test_put_file_in_transaction_open_parameters calls fs.put_file() inside fs.transaction, test_expand_path_glob_lists_stem_prefix asserts the ListObjectsV2 Prefix, and test_cancelled_read_frees_ranges cancels the real f.read() task instead of patching asyncio.wait. test_copy_object_with_multipart_upload_max_workers no longer waits out a 5 s timeout, and the queued-copies test no longer polls ThreadPoolExecutor._work_queue.
Rewrite the executor tests that relied on timing or private state: test_run_cancelled_after_return (which failed about 1 in 4 runs under -n 8) and test_run_cancelled_before_start synchronize on a spy of pool.submit, and test_fork runs a real os.fork() child while another thread of the parent holds the pool lock.
Check the documented S3AioThreadPool default max_workers against ThreadPoolExecutor's, the code, message and http_status_code of S3ClientError, and synchronize test_cancelled_read_frees_ranges and the max_workers copy test on the work they wait for; the held range of the cancelled read now retries until the cancellation stops it.
Check that S3Core.upload_part() sends an explicit part checksum, and presign with a 60 s expiry and a parameter that only PutObject accepts.
Await the one-worker copies and the delegations with await_bounded() (new in tests/pyathena/util.py), which fails at EVENT_TIMEOUT without waiting for the cancellation, so that a copy that never gets a worker fails the test instead of hanging it.
Synchronize test_copy_object_with_multipart_upload_waits_for_running_parts, which failed when part 2 failed before part 1 had started (about 1 in 6 runs under load, and once in a live run): part 2 now fails once part 1 is running.
Let overlapping_calls() show an exceeded bound: the calls of the first overlap wait up to 0.5 s for one more, so that a pool one worker too large fails test_run_beyond_default_executor and test_read_parallel, which it passed before.
Merge the S3Core path-rejection, ACL, multipart-identity, part-upload and checksum tests into their table tests; rewrite test_generate_presigned_url to sign locally and assert the parsed URL; check in test_get_object that a parameter of the call takes precedence over request_kwargs, as S3Core.call() documents (no test failed when the order was swapped).
Merge test_os_error_unknown_code into the errno table, and test_move_pairs into test_move_pairs_null_version_bucket_state.

These rows changed their verdict because breaking the behavior did not fail the cited cover, or because the contract is documented:

Row	Issue	Applied	Reason
`TestAioS3FileSystem::test_find_maxdepth`	REMOVE	MERGE	Async `_find` forwards `maxdepth` itself; dropping it failed no other test. `test_find` now asserts a `maxdepth=1` listing.
`TestAioS3FileSystem::test_put_file_mode`	REMOVE	KEEP	Async `_put_file` dropping `mode` failed only its `[create]` case.
`TestAioS3FileSystem::test_pipe_file_compression` (`intrans=False`)	REWRITE	KEEP	Async `_pipe_file` dropping its keyword arguments outside a transaction failed only these cases.
`TestAioS3File::test_open_version_id`, `::test_open_lookup_parameters`	REMOVE	MERGE	Async `_open` dropping `version_id` or the lookup parameters failed only these tests; they are one test now.
`TestAioS3StreamedFile::test_commit_failed_part_first`	MERGE	KEEP	`_discard` not cancelling unstarted parts failed only this test, and no public path leaves a part queued deterministically.
`TestS3PathPairing::test_conflict_candidates_one_per_pair`	REMOVE	KEEP	`conflict_candidates()` documents one candidate per pair in pair order; deduplicating them failed only this test.
`TestS3Core::test_properties`	REMOVE	KEEP, as `test_request_kwargs_is_a_copy`	The `request_kwargs` docstring says it returns a copy, and `S3File` changes the dict it gets; returning the internal dict failed only this test.
`TestS3Core::test_replace_object_metadata_omits_unset_fields`	REMOVE	MERGE	The cited `setxattr()` cover fails without the None filtering, but passing the metadata mapping unchanged failed nothing; `test_replace_object_metadata` now passes an `S3Metadata`.
`TestS3Executor::test_submit_to_executor_shut_down`	REMOVE	KEEP	`submit_to()` documents that the future gets the error that the executor reported; replacing it failed only this test. Its wait is now bounded.
`TestS3Executor::test_submit_to_cancelled_executor_shut_down`	REMOVE	KEEP	A `settle()` that did not notify a future the caller had cancelled failed only this test, and `wait()` on that future would never return.
`TestS3AioThreadPool::test_shutdown`	REMOVE	KEEP	The pool shutting down its executor only with `wait=True` failed only this test; the cited `test_shutdown_before_dispatch` checks `S3AioExecutor`'s own guard.
`TestAioS3FileSystem::test_multipart_copy_uses_creation_algorithm`	REMOVE	KEEP	The async copy rebuilding the completed parts without their checksums failed only this test; the sync test runs the sync aggregation.
`TestS3Core::test_complete_multipart_upload_preserves_part_checksums`	MERGE	MERGE, with a part checksum in `test_upload_part_uses_creation_algorithm_and_identity`	Dropping an explicit part checksum from `upload_part()` failed only this test.
`TestS3AioThreadPool::test_init`	REMOVE	REMOVE, with `test_default_max_workers`	A wrong default `max_workers` failed no other test; the new test compares it with `ThreadPoolExecutor`'s default, which the docstring names, instead of restating the formula.
`TestS3ClientError::test_properties`	REMOVE	MERGE	`http_status_code` returning None failed no other test; the status code, code and message are asserted in the `os_error` tests.

The issue named TestAioS3File::test_open_invalid_max_workers as the cover of TestS3AioThreadPool::test_init for max_workers=0; it does not fail under that mutation, but TestAioS3StreamedFile::test_open_async_invalid does. test_generate_presigned_url_signs_locally, not listed in the issue, is folded into the rewritten test_generate_presigned_url.

The removed async tests of methods that run a method of the synchronous filesystem also checked, in passing, that those methods pass their arguments through; dropping the range of _cat_file, the mode of _pipe_file or the maxdepth of _mv then failed nothing. test_delegations_send_the_requests_of_the_sync_methods calls 16 of these methods and their synchronous methods with the same arguments on filesystems that answer from the same keys, and compares the requests sent, the results and the stored keys; dropping any of their arguments fails its case.

	Before	After
Test functions	278	201
Collected tests	599	478

The removed real-S3 cases send about 420-450 S3 requests and transfer about 200 MiB per run, estimated from the code; the 16 checksum multipart cases account for about 170 requests and 80-100 MiB.

WHY

Closes #1241. Part of #1239: these tests repeated synchronous tests through the same code, restated constructors and Python behavior, depended on timing or private state, or repeated a contract that another test checks with the same kind of evidence, as described in docs/testing.md ("Choose what to test", #1250).

TEST

Python 3.15.0. At da1f35f (on 12859a4, which merged #1254; a trial merge into 59e00fb, which merged #1262, passes the same offline checks):

just format: no changes. just lint: passed.
Live: uv run --env-file .env pytest -n 4 tests/pyathena/filesystem gives 1124 passed and 6 skipped in 219 s. No Test workflow with AWS jobs and no other local live run was running. An earlier run, before the last two repairs, failed only test_copy_object_with_multipart_upload_waits_for_running_parts, which they synchronize.
Offline, with --noconftest and placeholder settings: pytest -n 8 tests/pyathena/filesystem gives 1030 passed, 6 skipped, and 94 failed; the 94 failures are the AWS tests kept in test_s3.py and test_s3_async.py, which fail without credentials, a subset of the set that fails at the base. 6 repeated runs gave the same set, and the synchronized abort test passed 30 repeated runs.
Mutation checks, offline with the same settings: each mutation of pyathena/filesystem/ named in the table above, in Remove and rewrite unnecessary tests in the async S3 filesystem and S3 core tests #1241 and in the review records failed the kept, merged, rewritten or added test that covers it, and git diff -- pyathena/ was empty after each. Covers that run only against S3 were judged by reading s3_async.py.

Not covered: a regression that blocks a worker thread forever can still hang the cancelled copy's cleanup in the teardown after await_bounded() has failed the test; ending it needs a process-level timeout, which the suite does not have.

🤖 Generated with Claude Code

laughingman7743 · 2026-10-17T14:50:55Z

-        assert events == ["pending finished"]
-
+    @pytest.mark.skipif(not hasattr(os, "fork"), reason="os.fork() is not available.")
+    @pytest.mark.filterwarnings("ignore:.*fork\\(\\) may lead to deadlocks:DeprecationWarning")


Self-review round one (implementation: /code-review high), reviewed at 25be2de on base 9fe991b; repairs in 87dbe2e, and the branch is now rebased onto #1254's head (base 87a1a66, head fd5bd60) with an unchanged patch series. Covered: every changed file (test_s3_async.py, test_s3_core.py, test_s3_executor.py, test_s3_path_pairing.py, test_s3_path.py, test_s3_errors.py) and the pyathena/filesystem code that the removed tests exercised. FINDINGS: 10; 7 accepted and repaired, 3 rejected. Each finding was checked with an offline mutation (--noconftest, placeholder settings).

Accepted. The test_fork rewrite dropped the held lock: removing pool._lock = threading.Lock() from _reset_after_fork (s3_executor.py:390) passed every test. A thread of the parent now holds the pool lock while the main thread forks (no public call holds it across a fork); under that mutation the child blocks and the test fails at its deadline. Rejected in part: keeping the copied executor in _forked_executors (s3_executor.py:389) guards against a collection that touches a lock a parent thread held; the child cannot reproduce that deterministically, and asserting the list is private state.

/simplify (3efd0d4). Applied: overlapping_calls() waits for the peak instead of the active calls, which removed three 5 s timeouts on the passing path; test_cancelled_read_frees_ranges uses wait_until() and checks that its task filter finds the held read. Skipped: replacing the barrier of the max_workers copy test with overlapping_calls (the barrier makes peak == 3 deterministic), mock.patch.object for the pool.submit spies, pytest.param IDs, a shared multipart-copy mock helper (a broader refactor of about 10 tests), shrinking test_write_transaction to 5 MiB + 1 (#1241 keeps its 10 MiB case and ID), and the formula instead of ThreadPoolExecutor in test_default_max_workers (the docstring names ThreadPoolExecutor's default).

laughingman7743 · 2026-10-17T14:50:55Z

            executor.submit_to(future, sum, [1, 2])
            with pytest.raises(RuntimeError, match="after shutdown"):
-                await asyncio.wrap_future(future)
+                await asyncio.wait_for(asyncio.wrap_future(future), EVENT_TIMEOUT)


Round one finding. Accepted. Without test_submit_to_cancelled_executor_shut_down, a settle() that skipped set_running_or_notify_cancel() for a future that the caller had cancelled (s3_executor.py:96-103) passed every test, and wait() on that future would never return. The test is restored (KEEP), with its wait bounded by EVENT_TIMEOUT; the mutation fails it.

laughingman7743 · 2026-10-17T14:50:55Z

-        assert S3AioThreadPool(None).max_workers == min(32, (cpus or 1) + 4)
-        with pytest.raises(ValueError, match="max_workers must be greater than 0"):
-            S3AioThreadPool(0)
+    def test_default_max_workers(self):


Round one finding. Accepted. With test_init removed, max_workers = 1 instead of ThreadPoolExecutor's default (s3_executor.py:246) passed every test. test_default_max_workers compares S3AioThreadPool().max_workers with a ThreadPoolExecutor(), which the docstring names as the default, instead of restating the formula; the mutation fails it.

laughingman7743 · 2026-10-17T14:50:55Z

    def test_os_error_by_http_status_code(self, status_code, expected_errno):
-        actual = S3ClientError(_client_error("UnknownCode", status_code=status_code)).os_error
+        error = S3ClientError(_client_error("UnknownCode", status_code=status_code))
+        assert error.http_status_code == status_code


Round one finding. Accepted. Removing test_properties left http_status_code (s3_errors.py:141) untested: returning None passed. test_os_error_by_http_status_code asserts the status code and test_os_error_by_error_code the code and message; returning None or an empty message fails them.

laughingman7743 · 2026-10-17T14:50:55Z

+        held = threading.Event()
+        release = threading.Event()
+
+        async def others_done():


Round one finding. Accepted. The held range waited for one event-loop pass, which can run before the other ranges' tasks take their results. It now waits until no other S3AioThreadPool.run task is pending before the read is cancelled; dropping clear_frames or keeping the read tasks fails it in 3 of 3 runs each.

laughingman7743 · 2026-10-17T14:50:55Z

+        condition = threading.Condition()
+        state = {"calls": 0, "active": 0, "peak": 0}
+        # The first three part copies run at once.
+        first_three = threading.Barrier(3, timeout=EVENT_TIMEOUT)


Round one finding. Accepted. overlapping_calls(4, timeout=0.5) needed three part copies to start within 0.5 s. The first three now meet at a barrier (EVENT_TIMEOUT), then wait up to 0.5 s for a fourth; peak == 3 is deterministic, and Semaphore(max_workers + 1) fails it in 3 of 3 runs.

laughingman7743 · 2026-10-17T14:50:55Z

-        )
-
-    def test_generate_presigned_url_signs_locally(self):
+    def test_generate_presigned_url(self, path, client_method, params, key, query):


Round one finding. Accepted in part. The URL checks no longer depend on botocore's signature version or addressing style (either Expires or X-Amz-Expires, any X-Amz-*/SigV2 field removed, the key at the end of the path); dropping the version, swapping the precedence or a fixed ExpiresIn fails it. Rejected: the dropped VersionId=None override case passed only because the old test mocked call; botocore rejects a None parameter when it signs.

laughingman7743 · 2026-10-17T14:50:55Z

    @pytest.mark.asyncio
-    async def test_get_version(self, tmp_path):
-        # GH-979: a version is downloaded to a local path named after its key.
+    async def test_get_version(self, tmp_path, destination, expected):


Round one finding. Accepted (convention). The destinations are built in the test body from declarative "directory"/"path" parameters instead of lambdas; the IDs are unchanged.

laughingman7743 · 2026-10-17T14:50:56Z

-                await asyncio.sleep(0.01)
+            assert await asyncio.to_thread(started.wait, EVENT_TIMEOUT)
+            # The call waits for the only thread.
+            await asyncio.wait_for(submitted.wait(), EVENT_TIMEOUT)


Round one finding. Rejected (finding at the removed TestS3AioThreadPool::test_shutdown). executor.shutdown(wait=wait, cancel_futures=True) (s3_executor.py:378) fails test_s3_async.py::TestAioS3StreamedFile::test_write_cancelled[2] deterministically (3 of 3 runs in the slice, and again here).

laughingman7743 · 2026-10-17T14:50:56Z

+            "upload_part-UploadId",
+            "upload_part_copy-UploadId",
+            "complete_multipart_upload-Key",
+            "abort_multipart_upload-Bucket",


Round one finding. Rejected. #1241 reduces test_multipart_upload_requires_identity to the 3 missing fields on one method plus one case per other method, because the 4 methods share _multipart_upload_request(). A method that stopped using the helper and checked only some fields is a rewrite that this test is not meant to pin.

laughingman7743 · 2026-10-17T14:59:27Z

-            condition.wait_for(lambda: state["active"] >= calls, timeout=timeout)
+            # The peak, unlike the active calls, stays reached when the last
+            # call returns before the others wake up.
+            condition.wait_for(lambda: state["peak"] >= calls, timeout=timeout)


Self-review round two (claims, callers, AWS effects, evidence), base 87a1a66, head 3efd0d4. Covered: the PR body and commit messages, the callers of the changed helpers, and the live tests whose code changed. FINDINGS: 1 corrected in the PR body; the rest checked as stated.

Checked (callers). overlapping_calls() now wakes its calls once the peak is reached instead of while that many are active; the peak it records is unchanged. Its callers are test_s3_executor.py (test_run_beyond_default_executor, test_submit_beyond_default_executor) and test_s3_async.py (test_copy_object_with_multipart_upload_shares_threads, test_read_parallel); none asserts on the time a call waited, and the negative-overlap callers, which never reach the target, still wait their timeout. Corrected: the PR body gave the counts and TEST results at 25be2de; it now lists the round-one repairs and their verdicts (test_submit_to_cancelled_executor_shut_down KEEP, test_default_max_workers, the S3ClientError properties), 198 test functions and 456 collected tests, and the TEST section at this head.

laughingman7743 · 2026-10-17T14:59:27Z

        result = await fs._find(f"{dir_}/prefix/test_")
        assert len(result) == 0

+        # maxdepth reaches the listing: only prefix2 is one level down.


Round two. Checked (AWS effects). Changed live tests: test_find (one more listing for the merged maxdepth=1 assertion), test_write_transaction[10485760], test_rm_recursive (2 files), test_upload_cp_file[1-1024], and the kept AWS tests now take the parameterless fs fixture; all are in the one live run before Ready. The savings (about 420-450 requests and 200 MiB per run) are estimated from the removed code, not measured. The flakiness claim for test_run_cancelled_after_return (about 1 in 4 runs under -n 8) comes from 6 of 25 and 2 of 9 failing runs on the base tree during the slice work.

laughingman7743 · 2026-10-17T15:46:16Z

-        assert query["versionId"] == ["v1"]
-        # request_kwargs are not signed.
-        assert not any(k.lower() == "x-amz-request-payer" for k in query)
+            url = core.generate_presigned_url(path, client_method, expires_in=3600, **params)


Independent review (relayed): Codex CLI 0.163.0 (codex exec -s read-only, model gpt-6-astra, reasoning effort max), session 01a14a60-684c-77a0-ab1d-940c0c0f6184. Static review of a detached snapshot of head 3efd0d4 against base 87a1a66 with the literal diff and docs/testing.md "Choose what to test"; no PR text, commit messages or prior findings; no edits, builds, tests, network or GitHub. Covered (reviewer): all seven changed files and the removed base tests; the replacement coverage in test_s3.py and the object/writer/executor tests; the filesystem code and docs/filesystem.md, docs/aio.md; the four callers of overlapping_calls; cancellation synchronization, the fork watchdog, parameter coverage, marks, fixture scopes and cleanup. Result: FINDINGS (10). The reviewer reasoned the mutations; I ran each offline.

Finding 1 (P2): generate_presigned_url hard-coding ClientMethod="get_object" or ExpiresIn=3600 passes the rewritten test, which signs with the default expiry and does not tell PUT from GET; the live test_sign uses GET and a lower bound only.

Accepted, repaired in 82df7e2: the test presigns with expires_in=60 (60 s window, or X-Amz-Expires=60) and signs put_object with ContentType, which get_object rejects; hard-coding either the method or the expiry now fails it (offline). SigV2 puts the content type in the query, which the test accepts for the case that passes it.

laughingman7743 · 2026-10-17T15:46:16Z

-
-        assert events == ["pending finished"]
-
+    @pytest.mark.skipif(not hasattr(os, "fork"), reason="os.fork() is not available.")


Independent review (relayed). Finding 2 (P2, removed TestS3AioThreadPool::test_shutdown): shutting the executor down only when wait is true (s3_executor.py:377) passes; the kept shutdown tests check S3AioExecutor's own guard or an unstarted pool.

Accepted, repaired in 82df7e2: test_shutdown is restored (KEEP); the mutation fails only it (offline).

laughingman7743 · 2026-10-17T15:46:17Z

            ("put_object_acl", (S3Path("bucket", "key"), "invalid")),
            # An object ACL that a bucket does not accept.
            ("put_bucket_acl", ("bucket", "bucket-owner-full-control")),
+            ("create_bucket", ("bucket", "invalid")),


Independent review (relayed). Finding 3 (P2, removed test_complete_multipart_upload_preserves_part_checksums): filtering Checksum* out of the per-call parameters of upload_part (s3_core.py:980) passes; no remaining test sends an explicit part checksum.

Accepted, repaired in 82df7e2: test_upload_part_uses_creation_algorithm_and_identity sends ChecksumSHA256 and expects it in the request; the mutation fails it.

laughingman7743 · 2026-10-17T15:46:17Z

        for fs in (fs1, fs2, fs3):
            assert fs._sync_fs not in S3FileSystem._cache.values()

+    @pytest.fixture(scope="class")


Independent review (relayed). Finding 4 (P2, removed async test_multipart_copy_uses_creation_algorithm): rebuilding each completed part from its number and ETag in the async aggregation (s3_async.py:699) drops the checksums and passes; the sync tests cannot see it.

Accepted, repaired in 82df7e2: the async test_multipart_copy_uses_creation_algorithm is restored (KEEP, 4 cases); the mutation fails its three checksum cases.

laughingman7743 · 2026-10-17T15:46:17Z

-            condition.wait_for(lambda: state["active"] >= calls, timeout=timeout)
+            # The peak, unlike the active calls, stays reached when the last
+            # call returns before the others wake up.
+            condition.wait_for(lambda: state["peak"] >= calls, timeout=timeout)


Independent review (relayed). Finding 5 (P2): once peak >= calls, later calls return at once, so an oversized pool can still report the expected peak in test_run_beyond_default_executor and test_read_parallel.

Accepted, repaired in 82df7e2: the overlapping_calls() change from round one's /simplify is reverted. The upper-bound callers rely on the first calls still waiting while later ones start; the 5 s waits on their passing path come back.

laughingman7743 · 2026-10-17T15:46:17Z

+                    # Waits briefly for a fourth overlapping call, which only a
+                    # copy that exceeds max_workers runs.
+                    with condition:
+                        condition.wait_for(lambda: state["active"] > 3, timeout=0.5)


Independent review (relayed). Finding 6 (P2): with a semaphore that admits four copies, a fourth that starts after the 0.5 s window leaves peak == 3.

Rejected. The passing path is deterministic (the barrier makes the first three overlap), and a fourth copy that a broken semaphore admits starts as soon as one is submitted, inside the 0.5 s window; detection depends on timing only on the failure side, as #1241's row intends ("pass timeout=0.5 ... and keep the GH-1159 assertion"). Semaphore(max_workers + 1) failed it in 3 of 3 runs.

laughingman7743 · 2026-10-17T15:46:17Z

+        task = asyncio.ensure_future(f.read())
+        try:
+            assert await asyncio.to_thread(held.wait, EVENT_TIMEOUT)
+            task.cancel()


Independent review (relayed). Finding 7 (P2): reads.pop() without cancel() in the cancellation handler (s3_async.py:1727) passes; the test never observes that the held read was cancelled.

Accepted, repaired in 82df7e2: the held range throttles and would retry after EVENT_TIMEOUT; the test asserts one attempt and that the cancelled read finished within EVENT_TIMEOUT. reads.pop() without cancel() fails it, and so does dropping clear_frames.

laughingman7743 · 2026-10-17T15:46:17Z

+            # Without them, the file gets the defaults of the filesystem.
+            fs.put_file(str(lpath), "s3://bucket/key", mode=mode)
+
+        assert fs.open.call_args_list == [


Independent review (relayed). Finding 8 (P3): the transaction rewrite asserts only the fs.open arguments; removing _write_file_and_close(...) from _put_file_in_transaction would leave it passing.

Rejected. Removing _write_file_and_close(...) fails test_transaction_pipe_put_file[True] and test_transaction_put_file_failed_write[PermissionError, RuntimeError] (offline); this test checks only the open() parameters, as #1241's row asks.

laughingman7743 · 2026-10-17T15:46:17Z

+            "upload_part-UploadId",
+            "upload_part_copy-UploadId",
+            "complete_multipart_upload-Key",
+            "abort_multipart_upload-Bucket",


Independent review (relayed). Finding 9 (P3): the identity, ACL and path cases still match message text ("no bucket", "no key", "no upload ID").

Rejected. ruff PT011 requires match for a bare ValueError; these strings are unchanged from the base tests and name the missing field, which is what tells the cases apart.

laughingman7743 · 2026-10-17T15:46:17Z

-            ("upload_part_copy", (1, S3Path("source", "key"))),
-            ("complete_multipart_upload", ([],)),
-            ("abort_multipart_upload", ()),
+        ids=[


Independent review (relayed). Finding 10 (P3): the six kept identity cases get new explicit IDs instead of their generated ones.

Rejected. The generated IDs embedded positional indexes (args0, args1) of the 12-case product, which the reduction to 6 cases would have renumbered anyway; the explicit IDs name the method and the missing field.

laughingman7743 · 2026-10-17T16:06:28Z

+                # Retrying when the read is cancelled, once the tasks of the
+                # other ranges have taken their results.
+                asyncio.run_coroutine_threadsafe(others_done(), loop).result(EVENT_TIMEOUT)
+                held.set()


Independent follow-up (relayed): Codex CLI 0.163.0 (codex exec -s read-only, gpt-6-astra, effort max), session 01a14a8c-1151-7e43-a629-c2956ad1bffb. Static review of the repair 3efd0d4..82df7e2 on a snapshot of 82df7e2 with the earlier findings and docs/testing.md; no edits, builds, tests, network or GitHub. Covered: the six repairs, their filesystem code, the overlapping_calls callers, botocore and tenacity sources. FINDINGS (6).

Finding 1 (P2): held is set before throttled(), so a stop that arrives before the first failure is classified raises through tenacity with a traceback cycle that keeps get_object's data; the weak-reference check could fail on correct code unless the cyclic GC runs.

Accepted, repaired in 7170d90: the held range creates its data after the stopped retry, so the retry's traceback holds nothing of it; 10 repeated runs passed, and dropping cancel() or clear_frames still fails the test.

laughingman7743 · 2026-10-17T16:06:28Z


        assert events == ["pending finished"]

+    @pytest.mark.skipif(not hasattr(os, "fork"), reason="os.fork() is not available.")


Independent follow-up (relayed). Finding 2 (P2, tests/pyathena/util.py:222, unchanged by this PR after the revert): with overlapping_calls(2), later calls can enter and leave one at a time, so ThreadPoolExecutor(max_workers=self.max_workers + 1) can still report the expected peak.

Deferred (pre-existing). overlapping_calls and its upper-bound callers test_run_beyond_default_executor/test_read_parallel are unchanged from the base after the revert; making the helper detect an oversized pool deterministically is outside this PR. Recorded here for a follow-up.

laughingman7743 · 2026-10-17T16:06:29Z


        assert events == ["pending finished"]

+    @pytest.mark.skipif(not hasattr(os, "fork"), reason="os.fork() is not available.")


Independent follow-up (relayed). Finding 3 (P2, restored test_shutdown, test_s3_executor.py:199): shutdown() always delegating with wait=True passes after the first call's EVENT_TIMEOUT.

Accepted, repaired in 7170d90: the test asserts that the pending call has not run when shutdown(wait=False) returns; always waiting fails it (after the first call's EVENT_TIMEOUT).

laughingman7743 · 2026-10-17T16:06:29Z

+            expires = int(signed_query["Expires"][0])
+            assert signed + 60 - 30 < expires <= signed + 60
+        # SigV2 also signs the content type in the query.
+        assert signed_query.pop("content-type", [params.get("ContentType")]) == [


Independent follow-up (relayed). Finding 4 (P2): dropping ContentType before signing passes: the SigV2 check supplies the expected value when the field is absent, and the SigV4 signed header is discarded.

Accepted, repaired in 7170d90: with SigV2 the content-type query field must equal the given type, and with SigV4 content-type must be in X-Amz-SignedHeaders exactly when a type is given; dropping ContentType fails it.

laughingman7743 · 2026-10-17T16:06:29Z

-            actual = f.read()
-            assert len(actual) == len(data)
-            assert actual == data
+    def fs(self):


Independent follow-up (relayed). Finding 5 (P2): the restored one-worker test_multipart_copy_uses_creation_algorithm hangs if the copy semaphore gets 0 permits, and the cancelled-read test awaits its task and the blocking pool shutdown without deadlines (test_s3_async.py:2700-2702).

Accepted in part, repaired in 7170d90: the cancelled-read test waits for the task and the pool shutdown with asyncio.wait_for(..., EVENT_TIMEOUT). Deferred: the restored test_multipart_copy_uses_creation_algorithm is the base test unchanged, and a zero-permit semaphore hanging it is a pre-existing property shared by the other one-worker copy tests.

laughingman7743 · 2026-10-17T16:06:29Z


        assert events == ["pending finished"]

+    @pytest.mark.skipif(not hasattr(os, "fork"), reason="os.fork() is not available.")


Independent follow-up (relayed). Finding 6 (P3, test_s3_executor.py:200): the restored test_shutdown matches "after shutdown", which is not documented.

Accepted, repaired in 7170d90: pytest.raises(RuntimeError) without match (ruff PT011 does not require one for RuntimeError).

laughingman7743 · 2026-10-17T16:25:16Z

+                asyncio.run_coroutine_threadsafe(others_done(), loop).result(EVENT_TIMEOUT)
+                held.set()
+                with contextlib.suppress(Exception):
+                    throttled()


Independent follow-up (relayed): Codex CLI 0.163.0 (codex exec -s read-only, gpt-6-astra, effort max), session 01a14a9d-e969-7232-9c94-c3ca471ea663. Static review of the repair 82df7e2..7170d90 (snapshot at 7170d90); no edits, builds, tests, network or GitHub. Covered: the cancelled-read allocation, cleanup, retry-stop mutations, synchronization and wait bounds; the shutdown always-wait mutation and message removal; the SigV2/SigV4 content-type checks. FINDINGS (1): P2, a cancellation that arrives before tenacity evaluates the first failure re-raises it with a reference cycle (error, traceback, retry state) that contains the get_object frame, so the data assigned later in that frame stays alive until the cyclic collector runs and the weak-reference check can fail on correct code. The other repaired hunks were found to fail under their mutations and to bound their waits.

Accepted, repaired in 07f38b8: the held range clears the frames of the caught error's traceback, as S3Core.call() does, which breaks the cycle; 10 repeated runs passed, and dropping cancel() or clear_frames in _fetch_range still fails the test.

Independent follow-up of the repair (relayed): Codex CLI 0.163.0 (codex exec -s read-only, gpt-6-astra, effort max), session 01a14aae-3db1-73f2-8eea-817fe0969084. Static review of 7170d90..07f38b8 on a snapshot of 07f38b8; no edits, builds, tests, network or GitHub. Result: CLEAN. Covered: a stop before the first failure is classified and a stop during the backoff (both reach the handler and the cleared frames drop the cycle), a successful retry (retry_api_call() clears its failures), and the two production mutations (dropping the cancel fails the timing and call-count checks; dropping clear_frames keeps the completed ranges and fails the weak-reference check).

…ver (GH-1239) - Fold the unknown error code into the errno table, which now asserts that an error without a status has no http_status_code, and remove the test of the S3ClientError properties. - Remove the version_id spelling test of S3Path, which the parse and name tests imply. - Fold the ordinary move of test_move_pairs into the null-version bucket-state test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- Narrow test_properties to the documented copy of request_kwargs. - Remove the per-operation request_kwargs and error translation tests, whose rule every operation shares through call(). - Fold the S3Metadata mapping of test_replace_object_metadata_omits_ unset_fields into test_replace_object_metadata; setxattr() checks the omitted fields. - Fold the keyless path into TestS3DeleteBatch::test_from_paths and the storage class into test_list_objects, and remove the frozen dataclass test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- Fold the per-method path checks into test_object_operations_reject_ paths and the create_bucket ACLs into test_acl_validation. - Keep one missing identity field per multipart method beyond the three of upload_part. - Fold test_upload_part into the creation-algorithm test, the ClientError cause and InvalidRange of get_object into test_call_translates_errors, and the precedence of the given parts into test_complete_multipart_ upload; the per-field checksum mapping is S3MultipartUploadPart's. - Sign presigned URLs locally and assert the URL instead of the arguments of a mocked call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Apply the offline TestAioS3FileSystem, TestAioS3File and TestAioS3StreamedFile rows of #1241: remove the tests whose contract the shared sync, core and executor tests check, check the async single-request copy, recursive rm with maxdepth and glob listing at the request level, drive the transaction put_file() through the public call, and cancel a real read instead of patching asyncio.wait. Keep test_put_file_mode, the non-transaction cases of test_pipe_file_compression and test_commit_failed_part_first, and merge test_open_version_id into test_open_lookup_parameters: each is the only test that fails when its async code is broken. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Most AioS3FileSystem methods run the S3FileSystem method of the same name in a thread of the pool, and AioS3File differs from S3File only in its executor. Remove the real-S3 tests that repeat the sync tests through those delegations: listing, info, exists, find, du, touch, cat, put, get, rm_file, checksum, sign, mkdir, metadata and tags, the small writes and appends, the checksum multipart writes, the read through core and _fetch_range, and the sync wrapper tests. Check that find() passes maxdepth to the listing in test_find, which test_find_maxdepth checked; glob() still checks withdirs. Keep only the multipart case of test_write_transaction and one size of test_upload_cp_file, and remove 2 files instead of 10 in test_rm_recursive. The fs fixture no longer takes parameters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…1239) S3Core.call() documents that a parameter of the request takes precedence over the core's request_kwargs, and no test failed when the order was swapped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Apply the test_s3_executor.py rows of #1241: remove the tests that restate constructor defaults and ThreadPoolExecutor semantics, the private-callback poll of wait_shielded(), and the dispatch-failure copies; merge the loop shutdown of submit() into test_submit_to_loop_shutdown; keep test_submit_to_executor_shut_down, the only test of the documented error that submit_to() sets when the executor drops a callable, with a bounded wait. Rewrite the cancellation tests without sleeps or private queues: the cancellation after the call has returned now comes from a loop callback that blocks until the call returns, and the cancellation before the start waits for a spy on submit(). test_fork runs a real forked child that submits to the pool. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…H-1239) - test_fork: a thread of the parent holds the pool lock while the main thread forks, so that the child creates its own lock. - Restore test_submit_to_cancelled_executor_shut_down: a future that the caller cancelled before the dispatch failed must still be notified for wait(). - Check the code, message and HTTP status code of S3ClientError. - Compare the default max_workers of S3AioThreadPool with ThreadPoolExecutor's, which its docstring documents. - test_cancelled_read_frees_ranges cancels the read once the tasks of the other ranges have taken their results, and the max_workers copy test makes its first three part copies meet at a barrier. - Sign the presigned URLs without depending on botocore's signature version or addressing style, and build the test_get_version destinations in the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

overlapping_calls() let the last call return before the others woke, which then waited out the whole timeout although the overlap had been reached; three executor and read tests waited 5 s each. Wait for the peak instead of the active calls. Also use wait_until() in test_cancelled_read_frees_ranges and check that its filter finds the held read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…H-1239) - Restore TestS3AioThreadPool::test_shutdown: the pool refuses new calls after shutdown(wait=False) and runs the queued ones. - Restore the async test_multipart_copy_uses_creation_algorithm: the async copy keeps the checksums of the copied parts in the completion. - Send an explicit part checksum through S3Core.upload_part(). - The held range of test_cancelled_read_frees_ranges retries until the cancellation stops it, so that a read left uncancelled fails the test. - Presign with a 60 s expiry and a parameter that only PutObject accepts, so that the method and the expiry reach the signer. - Revert the overlapping_calls() change: callers that check an upper bound rely on the first calls waiting while later ones start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

- test_cancelled_read_frees_ranges creates the data of the held range after its stopped retry, whose traceback would otherwise keep it, and bounds its waits for the cancelled read and the pool shutdown. - test_shutdown checks that shutdown(wait=False) does not wait for the running call, and no longer matches the error message. - test_generate_presigned_url checks that the content type is signed, in the query with SigV2 and as a signed header with SigV4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…test (GH-1239) The error of a retry that the cancellation stops is kept in a cycle through the frames of its traceback, which kept the frame of the held range and its data until the cyclic collector ran. Clear those frames, as S3Core.call() does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…eded bound (GH-1239) - Await the copies of the tests that run with one worker within EVENT_TIMEOUT, so that a copy that never gets a worker fails the test instead of hanging it. - overlapping_calls() waits for the peak instead of the active calls, and then up to a grace period for one more call, which only a caller that runs more calls at once makes; a pool one worker too large now fails test_run_beyond_default_executor and test_read_parallel. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The async methods that run a method of the synchronous filesystem had no test of the arguments they pass: dropping the range of _cat_file, the mode of _pipe_file or the maxdepth of _mv failed nothing, because the sync tests cannot see the async forwarding. One parametrized test calls each of them and its synchronous method with the same arguments on filesystems that answer from the same keys, and compares the requests sent, the results and the stored keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Round one of the review of the new checks found: - asyncio.wait_for() waits for the cancelled copy, whose cleanup a stuck worker can hold; await_bounded() fails the test at the bound instead. The elapsed-time checks that it makes redundant are gone. - The delegation fake answered copies of deleted keys and was used from several threads without a lock; it now rejects a copy of a missing source and serializes its requests. Each case passes a request parameter, so that dropping **kwargs fails it, and object_version_info is called with delete_markers. - overlapping_calls() makes only the calls of the first overlap wait for one more. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

laughingman7743 · 2026-10-18T00:52:58Z



-def overlapping_calls(calls, timeout=5):
+async def await_bounded(awaitable, timeout=EVENT_TIMEOUT):


Self-review round one of the new commits (implementation, /code-review high), range 5dc59fc..7a5f120 after the rebase onto master (12859a4, which merged #1254; the rebased patch series is unchanged, and master's tests/pyathena/util.py changes from #1251/#1253 add helpers that these tests do not use). Repairs in 6e22345. FINDINGS (10): 8 accepted, 1 rejected, 1 accepted in part.

asyncio.wait_for waits for the cancelled copy, whose cleanup a stuck worker can hold, so a hang was not bounded. Repaired: await_bounded() awaits with asyncio.wait(timeout=...) and fails without waiting for the cancellation; the elapsed-time checks it made redundant (two copy tests) are removed.

laughingman7743 · 2026-10-18T00:52:58Z

        assert info == {"name": "bucket/key"}

+    @staticmethod
+    def _serve_keys(fs, keys):


Round one findings 2-4 (the delegation test and its fake): (2) the fake copied a key that had been deleted, so an _mv that deleted before copying passed; (3) many cases passed no keyword arguments, so dropping **kwargs passed, and delete_markers was never passed; (4) the fake was used from several threads (the async _mv gathers its copies) without a lock and iterated the shared set. Repaired in 6e22345: CopyObject of a missing source raises FileNotFoundError, the fake serializes its requests and lists a snapshot, every case that forwards keyword arguments passes one that reaches a request (ExpectedBucketOwner, ContentType, MetadataDirective, delete_markers=True with a delete marker in the listing). Dropping the keyword arguments of _info, _cat_file, _exists, _put_file, _get_file, _mv's copies, metadata or object_version_info, or deleting before copying in _mv, fails its case (offline). rm_file documents that it uses no keyword arguments, so its case passes none. 5 repeated -n 8 runs passed.

laughingman7743 · 2026-10-18T00:52:58Z

+    return task.result()
+
+
+def overlapping_calls(calls, timeout=5, grace=0.5):


Round one findings 5-6 (overlapping_calls): (5) an exceeded bound shows only if the extra call starts within grace; accepted as failure-side timing only, since correct code passes deterministically, and an oversized pool failed both upper-bound tests in 3 of 3 runs. (6) Every call after the peak waited the full grace. Repaired in 6e22345: only the calls of the first overlap wait for one more, and the docstring says so; the upper-bound tests take about 0.5 s each.

laughingman7743 · 2026-10-18T00:52:58Z

+        ],
+    )
+    @pytest.mark.asyncio
+    async def test_delegations_send_the_requests_of_the_sync_methods(


Round one findings 7-8: (7) accepted with finding 1 (redundant elapsed-time checks removed). (8) Rejected: the expected values come from the synchronous method, which is the contract of these methods (their docstrings say "See S3FileSystem.x", and each runs it or its own gather around it); the synchronous methods are tested on their own in test_s3.py with independent expected values, so a defect shared by both is caught there.

laughingman7743 · 2026-10-18T00:53:43Z



-def overlapping_calls(calls, timeout=5):
+async def await_bounded(awaitable, timeout=EVENT_TIMEOUT):


Self-review round two of the new commits (claims, callers, evidence), head 6e22345. Corrected: the PR body still said the branch was stacked on #1254 and that the async forwarding and the two helper gaps were not covered; it now describes the delegation test, await_bounded() and the overlapping_calls() grace, and gives 201 test functions and 477 collected tests. Callers: await_bounded is new; overlapping_calls keeps its signature (a grace keyword is added) and its four callers pass on correct code in about 0.5 s each. The live run and the TEST section at the final head are still to do before Ready.

…1239) The independent review found forwarding that the comparison missed: - a wrong download destination, or a dropped progress callback, of _get_file and _put_file: the downloaded files and the callback updates are compared too; - a dropped truncate of _touch: a case touches an existing key with truncate=False, and a raised error is compared by its type; - metadata() returning a plain mapping: S3Metadata and the listed versions compare by their public properties. A blocked thread can also hang a test after await_bounded() fails it, while pytest-asyncio cancels the task in the teardown. A per-test faulthandler watchdog ends the process after 300 s instead, which fails the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

laughingman7743 · 2026-10-18T01:19:26Z

+    Returns:
+        The result of the awaitable.
+    """
+    task = asyncio.ensure_future(awaitable)


Independent review (relayed): Codex CLI 0.163.0 (codex exec -s read-only, gpt-6-astra, effort max), session 01a14c80-738c-7421-b75b-ff9f6ae92a74. Static review of a snapshot of 6e22345 with the diff 5dc59fc..6e22345 and docs/testing.md; no edits, builds, tests, network or GitHub. Covered: each delegation case against dropped or changed arguments, the fake (dircache, concurrency, ordering, exceptions), the four overlapping_calls callers, await_bounded and what it leaves pending. FINDINGS (7).

Finding 1 (P1): on a timeout await_bounded leaves its task pending; pytest-asyncio cancels it in the teardown and waits, so a copy whose cancellation waits for a blocked worker hangs the teardown, and the cleanup can outlive the Stubber context.

Accepted, repaired in 5dc3ca6: a regression that keeps a thread blocked cannot be ended from inside the test, so test_s3_async.py arms a faulthandler watchdog for each test (300 s, rearmed per test so that it covers the teardown, cancelled when the module ends) that dumps the stacks and ends the process; under xdist the test is reported as failed ("worker crashed"), which I checked with a scratch test. await_bounded still fails the test at its bound without waiting for the cancellation.

Correction: the watchdog was removed in f878518 (see the follow-up on the watchdog). This finding is now a recorded limitation: a thread that a regression blocks forever needs a process-level timeout, which the suite does not have.

laughingman7743 · 2026-10-18T01:19:26Z

+            local = {"local-source": str(source), "local-destination": str(tmp_path / run)}
+            call_args = [local.get(a, a) if isinstance(a, str) else a for a in args]
+            if run == "sync":
+                result = getattr(fs._sync_fs, name)(*call_args, **kwargs)


Independent review (relayed). Finding 2 (P2): the synchronous reference calls and non-coroutine methods such as recursive chmod run on the event-loop thread, where no timeout of the test can end a hang.

Accepted, repaired in 5dc3ca6 by the same watchdog, which also covers calls made on the event-loop thread.

Correction: the watchdog was removed in f878518 (see the follow-up on the watchdog). This finding is now a recorded limitation: a thread that a regression blocks forever needs a process-level timeout, which the suite does not have.

laughingman7743 · 2026-10-18T01:19:26Z

+            if not state["released"] and condition.wait_for(
+                lambda: state["peak"] >= calls, timeout=timeout
+            ):
+                condition.wait_for(lambda: state["peak"] > calls, timeout=grace)


Independent review (relayed). Finding 3 (P2): an extra worker that starts after the 0.5 s grace leaves the peak at the bound, and test_submit_beyond_default_executor submits only three calls.

Rejected (failure-side timing only). Correct code passes deterministically; an oversized pool failed both upper-bound tests in 3 of 3 runs. test_submit_beyond_default_executor checks that three calls run at once despite a one-thread default executor (GH-1159), not an upper bound.

laughingman7743 · 2026-10-18T01:19:26Z

+                result = getattr(fs, name)(*call_args, **kwargs)
+                if asyncio.iscoroutine(result):
+                    result = await await_bounded(result)
+            outcomes.append((comparable(result), sorted(map(repr, requests)), sorted(store)))


Independent review (relayed). Finding 4 (P2): _get_file forwarding lpath + ".wrong" keeps the requests, the None result and the stored keys.

Accepted, repaired in 5dc3ca6: each run downloads into its own directory, and the downloaded files and their contents are compared; lpath + ".wrong" fails the case (offline).

laughingman7743 · 2026-10-18T01:19:26Z

+                ("s3://bucket/key?versionId=v1", "local-destination"),
+                {"ExpectedBucketOwner": "111122223333"},
+            ),
+            ("_touch", "touch", ("s3://bucket/new",), {"ContentType": "text/csv"}),


Independent review (relayed). Finding 5 (P2): dropping truncate=truncate from _touch, or the callbacks of _put_file/_get_file, passes because the cases use the defaults.

Accepted, repaired in 5dc3ca6: a case touches the existing key with truncate=False (the error is compared by type), and the upload and download cases pass a recording progress callback whose updates are compared; dropping truncate or either callback fails its case.

laughingman7743 · 2026-10-18T01:19:26Z

+                return [comparable(v) for v in value]
+            if isinstance(value, S3ObjectVersion):
+                return vars(value)
+            return value


Independent review (relayed). Finding 6 (P2): metadata() returning dict(...) passes because S3Metadata compares by its user metadata, which the fake leaves empty.

Accepted, repaired in 5dc3ca6: S3Metadata results compare by type and public properties; dict(...) fails the case.

laughingman7743 · 2026-10-18T01:19:26Z

+            if isinstance(value, list):
+                return [comparable(v) for v in value]
+            if isinstance(value, S3ObjectVersion):
+                return vars(value)


Independent review (relayed). Finding 7 (P3): vars(value) compares the private fields of the listed versions instead of their public properties.

Accepted, repaired in 5dc3ca6: the listed versions also compare by type and public properties.

pytest cancels a faulthandler timer when it reports a failure, before the teardown that the watchdog was meant to cover, and a timer left armed by one test could end a slow session fixture of the next. A thread that a regression blocks forever needs a process-level timeout, which the suite does not have. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

laughingman7743 · 2026-10-18T01:32:11Z

+@pytest.fixture(autouse=True)
+def watchdog():
+    # Rearmed for each test, so that it also covers the teardown.
+    faulthandler.dump_traceback_later(WATCHDOG_TIMEOUT, exit=True)


Independent follow-up (relayed): Codex CLI 0.163.0 (codex exec -s read-only, gpt-6-astra, effort max), session 01a14c98-3b34-7dd1-8fd6-56b1d46e063e. Static review of 6e22345..5dc3ca6 on a snapshot of 5dc3ca6; no edits, builds, tests, network or GitHub. It found that the destination, callback, truncate and metadata comparisons fail under their mutations and match on correct code. FINDINGS (2): (1, P1) pytest's faulthandler plugin cancels dump_traceback_later when it reports a failure, so the watchdog is gone before the pytest-asyncio teardown it was meant to cover; (2, P2) with AWS_ATHENA_S3_VERSIONING_TESTS=1, a timer left armed by the previous test ends the session-scoped versioning_buckets fixture, which waits up to 900 s, and skips its cleanup.

Accepted, repaired in f878518 by removing the watchdog. A thread that a regression blocks forever cannot be ended from inside a pytest test; it needs a process-level timeout such as pytest-timeout, which the suite does not use. This stays a limitation: await_bounded() fails the test at its bound, and only a regression that also blocks the cancelled copy's cleanup can still hang the teardown.

test_copy_object_with_multipart_upload_waits_for_running_parts let part 2 fail before part 1 had started, which then cancelled part 1 instead of waiting for it; it failed about 1 in 6 runs under load and once in the live run. Part 2 now fails once part 1 is running. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…H-1239) Part 1 of test_copy_object_with_multipart_upload_waits_for_running_parts ended a fixed 50 ms after part 2 failed, so a delayed failure handling let it free its worker and part 3 start on correct code. It now retries until the failure stops it, then ends a little later, and the test also checks that it was stopped. test_close_cancelled bounds its held completion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

laughingman7743 · 2026-10-18T01:59:29Z

            if part_number == 1:
-                failed.wait(5)
+                started.set()
+                failed.wait(EVENT_TIMEOUT)


Independent follow-up (relayed): Codex CLI 0.163.0 (codex exec -s read-only, gpt-6-astra, effort max), session 01a14cb2-840c-7322-b79e-d05faecea0b7. Static review of 5dc3ca6..888f55d (watchdog removal, synchronized abort test) on a snapshot of 888f55d; no edits, builds, tests, network or GitHub. FINDINGS (2).

Finding 1 (P2): the synchronized test still depends on scheduling: part 1 ended 50 ms after part 2 failed, so if part 2's thread is descheduled before it raises, part 1 frees its worker and part 3 starts on correct code; and a delayed event loop lets an abort that does not wait pass.

Accepted, repaired in da1f35f: part 1 retries until the failure stops it (the copy sets failed before releasing the semaphore, and stops the retries after cancelling the queued parts), so part 3 can no longer start, and the test checks one attempt. A short delay after the stop keeps an abort that does not wait from passing: that mutation failed 5 of 5 runs, and the test passed 30 repeated runs.

laughingman7743 · 2026-10-18T01:59:29Z

        # GH-1186: a cancelled parallel read cancels the reads that have not
-        # finished and frees the data of the others, also while the
-        # cancellation is held, as a failed read does.
+        # finished, which stops their retries, and frees the data of the


Independent follow-up (relayed). Finding 2 (P2): test_close_cancelled waits for the held completion without a bound, so a close() that waits for the worker would hang before release.set().

Accepted, repaired in da1f35f: the held completion waits for release with EVENT_TIMEOUT.

laughingman7743 · 2026-10-18T02:03:56Z

+                finally:
+                    # Ends a little after the stop, so that an abort that
+                    # does not wait for it is recorded first.
+                    time.sleep(0.05)


Independent follow-up (relayed): Codex CLI 0.163.0 (codex exec -s read-only, gpt-6-astra, effort max), session 01a14cbc-9378-7220-a6e9-2f13e2c077c0. Static review of 888f55d..da1f35f on a snapshot of da1f35f; no edits, builds, tests, network or GitHub. It found the other orderings deterministic on correct code (the failure flag keeps part 3 from starting, part 2 waits for part 1's first attempt, part 2's error is raised, a missing stop fails the call count or the bound). FINDINGS (1, P2): an abort that does not wait for the running parts still passes if the cleanup is scheduled more than 50 ms after the stop.

Rejected (failure-side timing only). On correct code the abort waits for part 1, so the order is deterministic (30 of 30 runs); without the wait, the mutation failed 5 of 5 runs. Seeing that the abort waits without relying on the delay would need a patch of the private wait in _abort, which docs/testing.md allows only for failure points no public object reaches.

laughingman7743 added the enhancement label Oct 17, 2026

laughingman7743 force-pushed the test/1241-filesystem-async-core-tests branch from 87dbe2e to fd5bd60 Compare October 17, 2026 14:50

laughingman7743 commented Oct 17, 2026

View reviewed changes

laughingman7743 marked this pull request as ready for review October 17, 2026 16:40

laughingman7743 marked this pull request as draft October 18, 2026 00:23

Base automatically changed from test/1240-filesystem-sync-tests to master October 18, 2026 00:28

laughingman7743 and others added 14 commits October 18, 2026 09:29

laughingman7743 force-pushed the test/1241-filesystem-async-core-tests branch from 07f38b8 to 7a5f120 Compare October 18, 2026 00:36

laughingman7743 commented Oct 18, 2026

View reviewed changes

laughingman7743 and others added 2 commits October 18, 2026 10:48

laughingman7743 commented Oct 18, 2026

View reviewed changes

laughingman7743 marked this pull request as ready for review October 18, 2026 02:09

laughingman7743 merged commit 82754d2 into master Oct 18, 2026
11 checks passed

laughingman7743 deleted the test/1241-filesystem-async-core-tests branch October 18, 2026 02:21

This was referenced Oct 18, 2026

Build the pandas result-set tests through the constructor #1268

Merged

Audit the filesystem tests changed since the #1239 audit #1328

Open


		assert events == ["pending finished"]

		@pytest.mark.skipif(not hasattr(os, "fork"), reason="os.fork() is not available.")



		def overlapping_calls(calls, timeout=5):
		async def await_bounded(awaitable, timeout=EVENT_TIMEOUT):

		return task.result()


		def overlapping_calls(calls, timeout=5, grace=0.5):

Conversation

laughingman7743 commented Oct 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHAT

WHY

TEST

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

laughingman7743 commented Oct 17, 2026 •

edited

Loading