Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| it('keeps a bucket non-empty while archived versions or delete markers remain, even though the current listing is empty', async () => { | ||
| const client = createRestClient() | ||
| const token = requireServiceKey() | ||
| const bucketName = uniqueBucketName('vernonempty') | ||
| const key = uniqueObjectKey('nonempty') | ||
|
|
||
| await createRestBucket(bucketName, { versioningStatus: 'ENABLED' }) | ||
| await putObject(client, token, bucketName, key, 'will-be-hidden') | ||
| await deleteObject(client, token, bucketName, key) | ||
|
|
||
| const currentListing = await client.request<ListObjectsV2Response>( | ||
| 'POST', | ||
| `/object/list-v2/${bucketName}`, | ||
| { body: { limit: 100, prefix: '', with_delimiter: false }, expectedStatus: 200, token } | ||
| ) | ||
| expect(currentListing.json?.objects).toEqual([]) | ||
|
|
||
| const denied = await client.request<ErrorResponse>('DELETE', `/bucket/${bucketName}`, { | ||
| expectedStatus: 400, | ||
| token, | ||
| }) | ||
| expect(denied.json?.error).toBe('ResourceNotEmpty') | ||
| expect(denied.json?.statusCode).toBe('409') | ||
|
|
||
| await purgeVersionedBucket(client, token, bucketName) | ||
|
|
||
| const missing = await client.request<ErrorResponse>('GET', `/bucket/${bucketName}`, { | ||
| expectedStatus: 400, | ||
| token, | ||
| }) | ||
| expect(missing.json?.statusCode).toBe('404') | ||
| }) |
There was a problem hiding this comment.
🟡 (optional) This test leaks a bucket with archived versions/delete markers on the target server if any assertion before cleanup fails, unlike every other test in this file. purgeVersionedBucket is called directly at line 730 instead of inside a try/finally, so an early failing expect (e.g. line 721, 727, or 728) throws before cleanup runs. Fix: wrap the test body in try/finally like all 25 other tests in this suite so purgeVersionedBucket(client, token, bucketName) always runs, even on assertion failure.
Extended reasoning...
Every other it(...) block in this file (e.g. lines 282-298, 660-677, 1215-1242) wraps its body in try { ... } finally { await purgeVersionedBucket(...) }. The test starting at line 706 ('keeps a bucket non-empty ...') does not: bucket creation, put, delete, list, and delete-bucket-denied assertions run unguarded, and purgeVersionedBucket is only called at line 730 as a plain statement. If any expect() between lines 706 and 728 throws (e.g. the server does not yet return ResourceNotEmpty/409 for this case, or list-v2 output differs), the test fails and returns immediately, skipping cleanup. The bucket, plus its versioned/delete-marker rows, remains on the server; since these acceptance tests run destructively against a real target (managed local or remote), this accumulates orphaned versioned buckets across repeated CI failures until manually purged.
Verification: nit. The test at lines 706-737 ('keeps a bucket non-empty...') has no try/finally: its body runs unguarded and purgeVersionedBucket(client, token, bucketName) is only a plain statement at line 730. If any assertion before it throws — line 721 expect(currentListing.json?.objects).toEqual([]), line 727 expect(denied.json?.error).toBe('ResourceNotEmpty'), or line 728… | nit. The observation is…
There was a problem hiding this comment.
Fixed — the setup and assertions now run inside try/finally like every other destructive case here. The in-body purge stays because it is part of the assertion (the bucket only becomes deletable once its history is hard-deleted); the finally purge is a no-op once that has run, since purgeVersionedBucket now early-returns when the bucket is already gone.
| async function purgeVersionedBucket( | ||
| client: AcceptanceHttpClient, | ||
| token: string, | ||
| bucketName: string | ||
| ) { | ||
| const rows = await listAllVersions(client, token, bucketName).catch(() => []) | ||
| const prefixes = rows.map((row) => ({ path: row.name, versionId: row.version })) | ||
|
|
||
| if (prefixes.length > 0) { | ||
| await bulkDelete(client, token, bucketName, prefixes).catch(() => undefined) | ||
| } | ||
|
|
||
| await client | ||
| .request('DELETE', `/bucket/${bucketName}`, { expectedStatus: [200, 400, 404], token }) | ||
| .catch(() => undefined) | ||
| } |
There was a problem hiding this comment.
🟡 (optional) Maintainers get silently leaked versioned test buckets (with their full archived history) when cleanup's own enumeration step fails, with no test failure to reveal it — unlike the fixed-key-list cleanup helper used by other specs. listAllVersions(...).catch(() => []) at line 262 turns any list-v2 error into an empty array, so the if (prefixes.length > 0) guard at line 265 skips bulkDelete for every row, and the bucket DELETE at 269-271 accepts a 400 ResourceNotEmpty as success. Fix: log/rethrow a listAllVersions failure instead of swallowing it to empty, and don't let a 400/404 on the bucket delete count as cleanup success when rows were never enumerated.
Extended reasoning...
purgeVersionedBucket is the only cleanup path for all 26 tests in this file (called from every try/finally). listAllVersions issues POST /object/list-v2/:bucket with noncurrentVersions/deleteMarkers include; this is the brand-new query surface this PR itself is exercising, so it is more failure-prone than a plain object delete. If that request throws for any reason (transient 5xx, timeout, or a bug in the new filter combination), .catch(() => []) at line 262 masks it and rows becomes []. prefixes.length is then 0, so bulkDelete at line 265-266 never runs — no hard-deletes are issued for any of the bucket's rows. The bucket still has archived versions and delete markers. DELETE /bucket/:name at 269-271 passes expectedStatus [200,400,404], so the real 400 ResourceNotEmpty response is swallowed as an accepted status too. purgeVersionedBucket returns normally, the enclosing try/finally completes without throwing, and the test passes green while a fully populated versioned bucket is left on the target server with no log line or failed assertion pointing at it — unlike cleanupRestObjects…
Verification: nit (test-hygiene only, no production impact). The mechanism is real and reachable: at acceptance/specs/rest-object-versioning.test.ts:262, const rows = await listAllVersions(client, token, bucketName).catch(() => []) turns any error from the paginated POST /object/list-v2/:bucket (noncurrentVersions/deleteMarkers include, lines 218-237) into an empty array. Then line 265 `if…
There was a problem hiding this comment.
Fixed — purgeVersionedBucket now warns per step (enumeration, hard-delete, bucket delete) instead of swallowing silently, and additionally warns when the final bucket delete returns anything other than success or already-gone (e.g. ResourceNotEmpty), which is exactly the case where rows survived the purge. It still never throws, since it runs from a finally and raising there would replace the real assertion error with a teardown error.
87b5516 to
31c8408
Compare
0483674 to
b3eeea1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
One documented lifecycle contract is untested, and one destructive test can leak resources when it fails.
2 open findings
What changed in this PR
Adds black-box REST acceptance coverage for object versioning.
Changes:
- Adds 26 versioning lifecycle, history, deletion, copy/move, and concurrency tests.
- Introduces a gated versioning capability and bucket creation option.
- Documents versioning acceptance configuration and coverage.
| File | Description |
|---|---|
.env.acceptance.sample |
Enables versioning acceptance tests locally. |
acceptance/API_COVERAGE.md |
Documents covered versioning behavior. |
acceptance/README.md |
Documents the versioning capability flag. |
acceptance/specs/rest-object-versioning.test.ts |
Adds versioning acceptance tests. |
acceptance/support/config.ts |
Registers the versioning capability. |
acceptance/support/resources.ts |
Supports creating versioned buckets. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| bucket = await client.request<BucketResponse>('GET', `/bucket/${bucketName}`, { | ||
| expectedStatus: 200, | ||
| token, | ||
| }) | ||
| expect(bucket.json?.versioning_status).toBe('ENABLED') |
There was a problem hiding this comment.
Good catch — the contract is now actually tested, from both ENABLED and SUSPENDED, asserting the bucket still reads back unchanged afterwards.
Worth recording what it turned out to be: PUT /bucket/:id does not accept DISABLED at all, because updateBucketBodySchema in src/http/routes/bucket/updateBucket.ts declares versioning_status: { enum: ["ENABLED", "SUSPENDED"] }. So the request is refused by request validation (generic 400, message containing must be equal to one of the allowed values) before updateBucket’s transition rule in src/storage/database/pg.ts is ever consulted — that DB-level guard is defense-in-depth for non-REST callers. My first attempt asserted InvalidParameter and failed against the real server, so the test now asserts the validation shape instead.
| await createRestBucket(bucketName, { versioningStatus: 'ENABLED' }) | ||
| await putObject(client, token, bucketName, key, 'will-be-hidden') | ||
| await deleteObject(client, token, bucketName, key) | ||
|
|
||
| const currentListing = await client.request<ListObjectsV2Response>( | ||
| 'POST', | ||
| `/object/list-v2/${bucketName}`, | ||
| { body: { limit: 100, prefix: '', with_delimiter: false }, expectedStatus: 200, token } | ||
| ) | ||
| expect(currentListing.json?.objects).toEqual([]) | ||
|
|
||
| const denied = await client.request<ErrorResponse>('DELETE', `/bucket/${bucketName}`, { | ||
| expectedStatus: 400, | ||
| token, | ||
| }) | ||
| expect(denied.json?.error).toBe('ResourceNotEmpty') | ||
| expect(denied.json?.statusCode).toBe('409') | ||
|
|
||
| await purgeVersionedBucket(client, token, bucketName) |
There was a problem hiding this comment.
Fixed — wrapped in try/finally; see the reply on the other thread covering this same test for why the in-body purge stays.
b3eeea1 to
095cffe
Compare
Coverage Report for CI Build 37929843310Coverage decreased (-0.07%) to 85.19%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions17 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
095cffe to
962878f
Compare
Covers bucket versioning status transitions, per-version upload history, delete markers and undelete, single/bulk delete (including hard-delete by versionId with promotion), copy/move under versioning, and concurrency races - all verified against the REST surface (object/info, list-v2 with noncurrentVersions/deleteMarkers) since acceptance tests are black-box. Adds a gated `versioning` acceptance capability (ACCEPTANCE_ENABLE_VERSIONING) and a versioningStatus option on createRestBucket to support it.
962878f to
7b7b813
Compare
Summary
Rebased onto
masternow that #1347 is merged, so this PR is a single commit containing only the acceptance coverage.acceptance/specs/rest-object-versioning.test.ts: 26 black-box acceptance tests covering bucket versioning status transitions, per-version upload history, SUSPENDED-era mutable-slot semantics, delete markers/undelete, single and bulk delete (including hard-delete byversionIdwith promotion of the previous version), copy/move under versioning, and concurrency races./object/info/...,/object/list-v2/...withnoncurrentVersions/deleteMarkers), consistent with the acceptance suite's black-box design.versioningacceptance capability (ACCEPTANCE_ENABLE_VERSIONING) plus aversioningStatusoption oncreateRestBucket, and documents both in the README/API coverage doc and.env.acceptance.sample.ACCEPTANCE_ENABLE_VERSIONINGinto the remote acceptance job the same way every other gated capability is, so it can be turned on for remote targets via a repo variable.The local CI legs pick the capability up automatically: they copy
.env.acceptance.sample(now setsACCEPTANCE_ENABLE_VERSIONING=true) and.env.test.sample(already setsSTORAGE_VERSIONING_ENABLED=true), and in multitenant mode the app-level flag short-circuits the per-tenantobjectVersioningcheck — so the spec really runs on all 18 local legs rather than silently skipping.Review feedback addressed
ENABLEDandSUSPENDED. Worth noting what it turned out to be:PUT /bucket/:iddoes not acceptDISABLEDat all (updateBucket's schema enum is['ENABLED', 'SUSPENDED']), so the request is refused by validation beforeupdateBucket's transition rule insrc/storage/database/pg.tsis ever consulted. The test asserts that shape and that the status is unchanged afterwards.try/finallylike every other destructive case, so a failing assertion no longer leaks a versioned bucket and its history. Its in-body purge is part of the assertion (the bucket only becomes deletable once history is gone), and thefinallypurge is a no-op after it.purgeVersionedBucketno longer swallows failures silently: it warns per step, and also warns when the final bucket delete comes back as anything other than success or already-gone (e.g.ResourceNotEmpty), which is the case where rows survived the purge. It still never throws, since it runs from afinallyand raising there would replace the real assertion error. It also early-returns when the bucket is already gone, so the intentional double purge stays quiet.Test plan
npx tsc -p acceptance/tsconfig.json --noEmitnpx biome check .andnpx prettier --check .— clean repo-widenpm run acceptance -- --profile full acceptance/specs/rest-object-versioning.test.tsagainst mergedmaster— 26/26 pass, no cleanup warningsnpm run acceptance -- --profile full(whole suite) locally: 76 passed, 27 skipped, 4 failed. The 4 failures areiceberg.test.ts(1) andrender.test.ts(3); they reproduce identically on plainmasterwith this branch checked out away, so they are pre-existing/environmental on my machine (imgproxyfetch failed, iceberg-rest 403) and unrelated to these changes. Both specs pass in CI.🤖 Generated with Claude Code