Fix null-version moves in versioning-enabled S3 buckets - #1084
Conversation
| ( | ||
| p1, | ||
| p2, | ||
| self._move_target(p1, versioning_enabled=paths[p1].bucket in versioned_buckets), |
There was a problem hiding this comment.
Self-review round one (implementation behavior): CLEAN.
Frozen scope: 0c19c8d..570ed72; all six changed files. Traced path parsing and aliases, move expansion, self-move filtering, destination/source conflicts, the async caller, copy success/failure, explicit VersionId deletion, and cache invalidation. Also inspected the new integration fixture's exact-key assertions, opt-in gate, first-enable wait, pagination, and cleanup after failures.
The enabled-null regression exercises the original skipped-copy behavior; the Stubber regression checks the copy and deletion request's VersionId=null. State comparisons are shared within one move and remain fresh across calls. No actionable implementation defects found and no repairs were needed in this round.
Evidence: just format, just lint, docs lint, and both documentation builds passed; 51 existing/mocked move cases have passed in the active targeted run. The nine real AWS versioning cases are still waiting for propagation. This is a source review, not a claim that pending validation has passed.
| versioned_buckets = { | ||
| bucket | ||
| for bucket in buckets | ||
| if self._call(self._client.get_bucket_versioning, Bucket=bucket).get("Status") |
There was a problem hiding this comment.
Self-review round two (claims, compatibility, and AWS operation): CLEAN.
Frozen scope: 0c19c8d..570ed72; all six changed files. Audited the PR explanation and changed documentation/comments before tracing the corresponding implementation and tests.
Confirmed unchanged public signatures and version_aware defaults, version-query/protocol normalization, and preservation of original copy/delete paths. The planner observes bucket state per move; it does not introduce persistent configuration caching. Each relevant bucket has one logical lookup through the existing S3Core request/retry/error-translation path, so service retries may send additional requests. Permission failures remain visible before copying or deleting. S3 moves remain copy/delete operations, without an atomicity guarantee.
Checked AWS's documented Enabled, Suspended, and absent-Status behavior and GetBucketVersioning permission, the first-enable propagation wait, local fixture permissions and cleanup, the ordinary-CI skip gate, and Test workflow path selection (PyAthena suite, with unrelated SQLAlchemy/Spark suites skipped by conditions). The targeted tests cover aliases, multiple keys/buckets, state freshness, conflicts, and lookup/copy failures. No actionable compatibility or operational regression found.
Corrected the PR validation record to name the full published SHA, dependency versions, and completed documentation builds, including the 205 current-tree Sphinx warnings; none names the changed Markdown or move docstrings. The active real AWS versioning cases remain pending and are not represented as passed.
| for p in paths.values() | ||
| if p.version_id == "null" | ||
| and p.name in unversioned | ||
| and not self.core._is_directory_bucket(p.bucket) |
There was a problem hiding this comment.
Self-review round one, bounded repair follow-up: CLEAN. Base: 0c19c8d; old head: 570ed72; new head: d680586. Verified the old commit objects and git range-diff: the initial patch is unchanged, with one repair commit added. Covered all five repaired files and traced the shared planner and existing S3Core._is_directory_bucket predicate through both sync and async callers. Directory null versions retain the previous identity, without an unsupported lookup, while general-purpose bucket handling is unchanged. Added regressions assert both same-key no-op and separate-destination moves without lookup, plus async no-op behavior. just format, just lint, docs lint, and the current-tree Sphinx build passed; the repaired move selection passed 54 cases and intentionally skipped the nine opt-in cases. The initial reviewed general-purpose bucket run passed all 60 cases, including the nine real AWS cases, with cleanup completed.
| uv run --env-file .env pytest -n 1 tests/pyathena/sqlalchemy/ tests/pyathena/aio/sqlalchemy/ -v | ||
| ``` | ||
|
|
||
| ### Other validation and reporting |
There was a problem hiding this comment.
Self-review round two, bounded repair follow-up: CLEAN. Same old/new frozen scope as the repair implementation record. Independently audited the repaired claims and operator/reader consequences: the existing --x-s3 predicate excludes only directory buckets from the versioning lookup, matching AWS's unsupported-operation contract; no public parameters or general-purpose bucket copy/delete behavior changed. The filesystem documentation now explains this exception. General tox and evidence-reporting guidance is under its own heading, outside the SQLAlchemy subsection. Runtime limits remain explicit: general-purpose bucket states were tested on real AWS at 570ed72; directory-bucket preservation is covered by mocks at d680586, without an S3 Express runtime claim.
| for p in paths.values() | ||
| if p.version_id == "null" | ||
| and p.name in unversioned | ||
| and not self.core._is_directory_bucket(p.bucket) |
There was a problem hiding this comment.
Relayed independent bounded static follow-up: CLEAN. Reviewer: Claude Code claude-opus-5-5, verified first-party Max (not Enterprise), effort high; session 77be42ea-ce2d-4566-9889-c92c73f3db39. Old/new base: 0c19c8d; old head: 570ed72; new head: d680586. Supplied the bounded patch-series comparison without commit subjects, the literal repair diff, and a tracked-source snapshot. Only Read/Grep/Glob were enabled; no commands, edits, validation, network, PR context, commit messages, or memory access were authorized. Snapshot hashes and author worktree were unchanged afterward.
Coverage: lookup selection; the existing directory-bucket predicate and core property; null/key identity and conflicts; both shared-planner callers; affected docstrings; all three added directory-bucket test cases; filesystem documentation; testing-guide headings and related anchors. The reviewer confirmed the unsupported directory-bucket lookup is removed without changing general-purpose bucket lookups, and the general tox/reporting guidance has its own section. Both prior findings are resolved. No actionable defects found within the repaired scope.
Static review only: the reviewer ran no tests or builds. Directory-bucket preservation has mock coverage, without a real S3 Express runtime claim.
d680586 to
ca44c77
Compare
| return pairing.move_pairs( | ||
| pairs, missing=missing, versioning_enabled_buckets=versioned_buckets | ||
| ) | ||
|
|
There was a problem hiding this comment.
Rebased implementation self-review (round one): CLEAN.
Frozen full diff: e41fe33..ca44c77; all nine changed files covered. The old base/head objects were verified and the patch-series comparison inspected. This is a full pass because the upstream refactor changes the planner's contract.
Checked selective bucket-state lookup, scheme/version normalization, per-call freshness, directory-bucket exclusion, enabled/suspended/unversioned identities, overlap and duplicate-destination guards, missing-directory candidates, and exact-version copy/delete. Traced both synchronous mv and async _mv/wrapper into the extracted S3PathPairing and shared core. Bucket status is passed identically to candidate discovery and final conflict checks; defaults and the pure model's lack of I/O remain compatible. Both lookup and copy failures prevent source deletion.
Regression coverage includes real-client Stubber assertions, status/error/request-count cases, pure-model bucket isolation and same-version no-op cases, and opt-in live coverage with exact-key version comparisons and exhaustive fixture cleanup. Preserved #1082's du tests and offline/docs organization.
Validation: just format, just lint, just docs lint and a build of the current Sphinx source passed; 224 self-contained path/pairing/pandas/Polars tests passed. Real AWS tests and current CI are still in progress. No S3 Express runtime claim. Static review does not replace those pending results.
| missing: The :meth:`conflict_candidates` without an object at their | ||
| key, in any form that names them; needed when there are | ||
| candidates. | ||
| versioning_enabled_buckets: Buckets whose versioning is enabled. |
There was a problem hiding this comment.
Rebased compatibility, operational and claim self-review (round two): CLEAN.
Frozen full diff: e41fe33..ca44c77; all nine changed files and upstream contract changes inspected separately from round one.
Claims checked: enabled null-version moves preserve the selected copy and delete only that version; never-enabled/suspended cases remain no-ops; conflict checks share the same identity; only null-versus-bare-key comparisons need status; state is shared within a move and fresh on the next call; failed planning cannot copy/delete; directory buckets skip the unsupported operation; the test fixture waits after first enablement and cleans only its own generated buckets. Traced public sync/async signatures, default pure-model arguments, version forwarding in CopySource and delete batches, S3Core request filtering/error translation and existing SDK/application retries. Logical per-bucket lookup counts are mock evidence, not a measurement of retry attempts.
Preserved upstream copy/get/rm pairing and #1082's offline command, parameter cases, du cleanup, SQLAlchemy/tox/reporting headings. Checked unchanged CI IAM and path filters: temporary-bucket provisioning remains opt-in; SQLAlchemy/Spark suites are outside the changed contracts. Existing static API docs and the explicit Enabled context agree.
Current evidence: format/lint/docs lint passed, current-source Sphinx build passed with 209 API/reference warnings (none on the changed Markdown, move methods, or target docstring), and 224 offline path/pairing/pandas/Polars tests passed. AWS regression and current CI are pending; prior-head runtime results will not be represented as results on this rebased head. No S3 Express live test or concurrent versioning-change guarantee is claimed.
| TypeError: If ``versioning_enabled_buckets`` is a string. | ||
| """ | ||
| if isinstance(versioning_enabled_buckets, str): | ||
| raise TypeError( |
There was a problem hiding this comment.
Implementation repair self-review: CLEAN. Covered all six repair files plus shared callers and contracts using the verified old objects and patch-series comparison. The shared _moves entry rejects a string bucket context before target membership in both public methods; tuple/set/default behavior and internal set calls are preserved. The two regressions exercise the misleading logs/logs-archive substring case. The public model docs describe both required inputs and their default; sync/async exception and private helper docstrings match actual propagation. The valid AWS move path is unchanged by this guard; the opt-in run started on the old head remains in progress. Format/lint/docs lint/build passed and 226 self-contained tests passed.
Frozen repair: ca44c77..4a3edf6; PR base e41fe33. Patch series compared as e41fe33..ca44c77 versus e41fe33..4a3edf6.
| Pass a collection of bucket names, such as a set, rather than a single string. | ||
| The default empty collection treats null versions as their keys, as in unversioned | ||
| or suspended buckets. The model makes no AWS requests. | ||
|
|
There was a problem hiding this comment.
Compatibility, operational and claim repair self-review: CLEAN. Covered all six repair files separately from the implementation pass. Existing positional arguments, valid collection contexts, default no-op semantics and pure model/no-I/O behavior remain compatible. Both public methods now clearly raise TypeError for a bucket-name string; OSError accurately includes S3Core-translated permission failures through sync/async planners and wrapper. Public documentation states that callers supply the same enabled-bucket collection to candidate discovery and final checks; default-empty behavior and opt-in permissions are explicit. Tests verify both method entry points. Format/lint/docs checks and 226 offline tests passed; live results started at the earlier head will be identified by that revision, while current CI is still pending. No new AWS calls, permissions or runtime claims are introduced by this repair.
Frozen repair: ca44c77..4a3edf6; PR base e41fe33. Patch series compared as e41fe33..ca44c77 versus e41fe33..4a3edf6.
| TypeError: If ``versioning_enabled_buckets`` is a string. | ||
| """ | ||
| if isinstance(versioning_enabled_buckets, str): | ||
| raise TypeError( |
There was a problem hiding this comment.
Relayed independent repair review: CLEAN (no defects found). Claude Code claude-opus-5-5, verified first-party Max profile, high effort; session 180b3bea-3d55-4627-bdaa-8fe4c0d8090e, completed in 94257 ms. Model metadata confirms claude-opus-5-5 / firstParty; no permission denials.
Verified old/new series: e41fe33..ca44c77 versus e41fe33..4a3edf6. All six repair files and affected callers/contracts covered using the subject-free range-diff and literal repair diff. Read/Grep/Glob only; no edits, commands, builds/tests, network/GitHub or memory access. The exported snapshot remained unchanged, and the author worktree is clean.
The reviewer confirms the shared _moves guard rejects strings before any membership lookup in both public entry points, including empty pairs; valid collection and default behavior is preserved. Public docs describe the same collection for candidate discovery/final checks and the empty default. Private helper Args/Returns, sync/async OSError propagation, and the unversioned mock comment are accurate. The two tests exercise the substring failure scenario. No further actionable findings.
Static review only; runtime/lint/docs results are author validation. On the repaired head, format/lint/docs checks and 226 offline tests passed. At ca44c77, the serial real AWS selection passed all 66 tests, including nine opt-in cases, and temporary-bucket cleanup completed. The repair only rejects invalid string context and updates prose; the internal AWS caller continues to pass a set. S3 Express remains mock-only coverage. Current-head AWS CI will follow Ready.
| } | ||
| return pairing.move_pairs(pairs, missing=missing) | ||
| return pairing.move_pairs( | ||
| pairs, missing=missing, versioning_enabled_buckets=versioned_buckets |
There was a problem hiding this comment.
Additional upstream integration check: master advanced from e41fe33 to 855d4a7 while validation ran. Inspected that literal upstream diff: it extracts S3File buffered multipart writing and updates its tests/docs; S3Core copy/delete, S3PathPairing, sync/async move planners and versioning lookups are unchanged. No move-contract repair or branch rewrite is needed. The frozen review merge-base remains e41fe33, the published/reviewed head is 4a3edf6, offline checks pass and GitHub reports a clean merge. Ready CI will validate integration with the new base.
| source | ||
| for source in pairing.conflict_candidates(pairs) | ||
| for source in pairing.conflict_candidates( | ||
| pairs, versioning_enabled_buckets=versioned_buckets |
There was a problem hiding this comment.
Validation complete at published/reviewed head 4a3edf6: Ready-triggered Test run 37208194062 passed against merge candidate eec166a (head plus master 855d4a7). Python 3.14: 2572 passed, 10 skipped, 13 warnings. All applicable current-head checks are green; SQLAlchemy jobs and Spark coverage were excluded by unchanged-path conditions, and nine temporary-bucket tests remain intentionally opt-in. Local opt-in run at ca44c77 passed all 66 cases, including nine real AWS versioning cases, and cleanup completed. The final repair preserves that valid set-context AWS path; current-head offline tests passed 226 cases. Required self-reviews and Opus 5.5 / first-party Max / high follow-up are complete and CLEAN; all verified findings are repaired and resolved. PR state rechecked: Ready, MERGEABLE, CLEAN.
CI: https://github.com/pyathena-dev/PyAthena/actions/runs/37208194062
| ) | ||
| def test_move_null_version_onto_key(self, fs, versioning_buckets, status): | ||
| client, buckets = versioning_buckets | ||
| bucket = buckets[status] |
There was a problem hiding this comment.
Test-layout repair self-review (implementation): CLEAN.
Verified old objects and frozen patch-series comparison: e41fe33..4a3edf6 versus e41fe33..d4d7fe5. Covered docs/testing.md, both filesystem test files, the deleted standalone test module, and its moved bucket fixture in filesystem/conftest.py.
The nine cases now belong to the existing S3FileSystem/AioS3FileSystem test classes and use their class-scoped fs fixtures. Native async mv and the synchronous wrapper have distinct methods; no method shadows the existing mock regressions. Backend/state parameter IDs are preserved, and the exact content/version assertions and exact-key filtering remain unchanged. The original bucket setup, first-enable wait and all-bucket/version/delete-marker cleanup were retained. The bucket fixture intentionally changes module to session scope so both modules share the original three-bucket setup once per worker; no autouse fixture or production code changes.
Local validation: just format/lint/docs lint/current-source Sphinx build passed. Collection selects exactly the nine cases, and opt-out with AWS session hooks excluded skips all nine before filesystem/bucket setup. Real S3 validation is pending serialization with another session's AWS test; no prior-head live result is claimed on this head. S3 Express coverage remains mock-only.
| VERSIONING_TEST_KEYS = ("sync", "async", "async-wrapper") | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") |
There was a problem hiding this comment.
Test-layout repair self-review (compatibility, operational effects and claims): CLEAN.
Frozen patch series: e41fe33..4a3edf6 versus e41fe33..d4d7fe5; all moved/changed/deleted test and documentation surfaces audited separately from implementation review.
The documented -k selector collects exactly 9 cases (the same sync, async and wrapper times None/Enabled/Suspended matrix); IDs retain their backend/state labels. Only the native coroutine carries asyncio marking now, and both synchronous APIs run as normal tests. Existing class fixture signatures, scope and registration are unchanged. The session-scoped resource fixture is lazy, checks opt-in before creating a client, and shares 3 buckets/one propagation wait per single worker across the two files. Separate keys prevent cross-backend mutation and async-prefix collisions. Scope widening moves cleanup to session teardown, with all owned bucket cleanup preserved; neither default CI nor unrelated tests request it when opt-out markers skip the cases.
Docs explain the new paths, per-worker scope, serial invocation and separate API methods. No source/API/permission/retry changes. Format/lint/docs checks passed, collection reports 9/719 cases, opt-out reports 9 skipped/710 deselected with AWS hooks excluded. Live regression and new CI are pending; previous runtime results retain their original revisions.
| VERSIONING_TEST_KEYS = ("sync", "async", "async-wrapper") | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") |
There was a problem hiding this comment.
Independent test-layout repair review: CLEAN.
Reviewer: Claude Code claude-opus-5-5, verified claude.ai first-party Max profile, high effort; session edf8dcb6-5d61-4512-9d3a-5920778b802d. The returned model metadata reports claude-opus-5-5/provider firstParty, with no permission denials.
Frozen series: e41fe33..4a3edf6 versus e41fe33..d4d7fe5. The reviewer inspected the literal repair diff, range-diff, full patch context and relevant source/fixture contracts from an archived snapshot, without author review records or PR metadata.
No defects found after tracing all five changed/deleted paths: session-scoped bucket preparation and cleanup, reuse of class-scoped filesystem fixtures, exact-key isolation between async and async-wrapper, sync/native-async/wrapper calls, markers and the nine-case selector, documentation and license headers. No production changes in this repair.
Static review only: no builds, tests, AWS calls, or repository edits. Read/Grep/Glob-only tool access; no network, shell, memory, or GitHub access. Snapshot hashes remained unchanged across all 292 files, and the author's worktree stayed clean. Real-S3 validation is recorded separately when it completes. Reviewer's stated limitation: fsspec sync and pytest teardown semantics were assessed from code and general library behavior rather than vendored library source.
| VERSIONING_TEST_KEYS = ("sync", "async", "async-wrapper") | ||
|
|
||
|
|
||
| @pytest.fixture(scope="session") |
There was a problem hiding this comment.
Current-head test-layout runtime validation: 9 passed in 953.94s, exit 0.
Head d4d7fe5; command: AWS_ATHENA_S3_VERSIONING_TESTS=1 uv run --env-file .env pytest -n 1 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k move_null_version_onto_key -v.
All cases passed in their new TestS3FileSystem/TestAioS3FileSystem locations: sync, native async and sync wrapper, each against None/Enabled/Suspended bucket versioning. This validates reuse of the existing class filesystem fixtures and the shared session fixture with one preparation/propagation wait. Normal session teardown deleted all owned object versions/delete markers and temporary buckets; no cleanup errors.
Current-head format, lint, docs lint and rendered docs build passed; offline collection found exactly 9 selected cases, and opt-out skipped all 9 before their fixture setup. Both distinct bounded self-reviews and the requested Opus 5.5/first-party Max/high independent repair review are CLEAN. Ready-triggered AWS CI will be recorded separately at its actual revision. No previous CI result is substituted for this head.
| ) | ||
| def test_move_null_version_onto_key(self, fs, versioning_buckets, status): | ||
| client, buckets = versioning_buckets | ||
| bucket = buckets[status] |
There was a problem hiding this comment.
Final test-layout delivery validation: all applicable current-head checks passed; PR Ready and mergeable (CLEAN).
Reviewed/published head: d4d7fe5; review merge-base: e41fe33. Worktree remains clean.
Ready-triggered Test run 37211599010: https://github.com/pyathena-dev/PyAthena/actions/runs/37211599010 . Python 3.14 AWS test job 111463756889 passed in 9m14s; pytest reports 2,572 passed, 10 skipped, 13 warnings in 450.30s. The log confirms checkout 004ef9e, whose parents are master 855d4a7 and this reviewed head. Changes/lint jobs passed; SQLAlchemy jobs are skipped by path selection. Docs lint/build, license and offline checks are green on this same head.
Separately, the documented one-worker opt-in versioning command passed all 9 moved sync/async/wrapper × None/Enabled/Suspended cases in 953.94s, exit 0, including owned temporary-bucket cleanup. Those nine cases remain intentionally skipped in ordinary CI.
Both distinct repair self-reviews and the requested Claude Opus 5.5/verified first-party Max/high independent follow-up are CLEAN and recorded inline. No production changes in the layout repair. No merge performed.
1 participant
WHAT
Use the bucket's versioning state when a move compares a
nullversion with the same key without a version ID. With versioning enabled,mv("s3://bucket/key?versionId=null", "s3://bucket/key")copies the selected version and then deletes that source version. Unversioned and suspended buckets keep the source in place.Apply the same distinction to conflict checks and the shared synchronous/asynchronous move planner. Only comparisons involving both a null version and its unversioned key look up bucket versioning; results are shared within one move and are not cached between moves. These lookups require
s3:GetBucketVersioning. Directory buckets retain their previous behavior without an unsupported versioning lookup.The extracted
S3PathPairingaccepts the versioning-enabled buckets as explicit context for both candidate discovery and final conflict checks, retaining its pure default behavior. Both methods reject a single bucket-name string to prevent substring matches; callers pass a collection of bucket names. Keep the opt-in versioning regressions in the existingTestS3FileSystemandTestAioS3FileSystemclasses, with separate sync, async and wrapper methods. Reuse their filesystem fixtures and share bucket preparation/cleanup in a session-scopedtests/pyathena/filesystem/conftest.pyfixture. No standalone versioning test module remains. Ordinary CI does not enable the temporary-bucket tests or require additional provisioning permissions.WHY
Fixes #1083. A null version may be noncurrent after bucket versioning is enabled. Treating it as the unversioned key skips a requested move and incorrectly rejects moves whose sources do not conflict. Retaining the existing behavior for unversioned and suspended buckets avoids deleting a copy that replaced the null source version.
TEST
Current head:
d4d7fe5bd3ccb414c8b161c14453a3a1a15e4718; frozen review merge-base:e41fe33cf0182166cdb1424d4b08e512a713ef79. Master tip:855d4a7b652d6dad106c27ee7112264ca45b923e. This repair changes only test organization and its documented invocation; the production implementation is unchanged from4a3edf616af79ebba843d43b2f779fe92788ef1f.Local tools: Python 3.13.1, fsspec 2026.9.0, boto3/botocore 1.43.102.
just format,just lint,just docs lint: passed.uv run sphinx-build -b html docs docs/_build/current: passed.AWS_ATHENA_S3_VERSIONING_TESTS=0 uv run --env-file .env pytest --noconftest -p no:rerunfailures --collect-only -q tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k move_null_version_onto_key: 9 cases collected / 710 deselected. Backend/state IDs are retained in the new class locations.--collect-only: 9 skipped / 710 deselected, confirming opt-out before fixture setup; AWS session hooks were excluded.AWS_ATHENA_S3_VERSIONING_TESTS=1 uv run --env-file .env pytest -n 1 tests/pyathena/filesystem/test_s3.py tests/pyathena/filesystem/test_s3_async.py -k move_null_version_onto_key -v: 9 passed in 953.94s, exit 0, on this head with one worker. The documented invocation exercised all three API methods and all three versioning states using the shared session fixture; all temporary bucket/version/delete-marker cleanup completed without errors.edf8dcb6-5d61-4512-9d3a-5920778b802d), recorded inline. Prior production-change reviews and repairs remain recorded at their actual revisions.004ef9e4a5e6dcaf696d51656402428e4dee7da9, whose parents are current master855d4a7b652d6dad106c27ee7112264ca45b923eand reviewed headd4d7fe5bd3ccb414c8b161c14453a3a1a15e4718. All applicable current PR checks are green. SQLAlchemy/Spark contracts are unchanged, so the SQLAlchemy jobs are skipped by workflow path filters and Spark coverage retains its existing test filtering. Temporary-bucket tests are intentionally opt-in; ordinary CI does not need their provisioning permissions. S3 Express remains mock-only coverage.