Conversation
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.
![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…
| 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…
This branch has not been deployed
1 participant
Summary
acceptance/specs/rest-object-versioning.test.ts: 26 black-box acceptance tests covering bucket versioning status transitions, per-version upload history, 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.Test plan
npx tsc -p acceptance/tsconfig.json --noEmitnpx biome check/npx prettier --checkon all changed filesSTORAGE_VERSIONING_ENABLED=true(npm run acceptance -- --profile full acceptance/specs/rest-object-versioning.test.ts) - all 26 tests pass