diff --git a/src/core/storage/S3.ts b/src/core/storage/S3.ts index 94fd1a9..f781d20 100644 --- a/src/core/storage/S3.ts +++ b/src/core/storage/S3.ts @@ -81,38 +81,36 @@ export default class S3Storage extends StorageBase { async rm(repoId: string, dir: string = ""): Promise { if (!config.S3_BUCKET) throw new Error("S3_BUCKET not set"); this.assertSafePath(dir); - const data = await this.client(200000).listObjectsV2({ - Bucket: config.S3_BUCKET, - Prefix: join(this.repoPath(repoId), dir), - MaxKeys: 100, - }); - - const params = { - Bucket: config.S3_BUCKET, - Delete: { Objects: new Array<{ Key: string }>() }, - }; - - data.Contents?.forEach(function (content) { - if (content.Key) { - params.Delete.Objects.push({ Key: content.Key }); - } - }); - - if (params.Delete.Objects.length == 0) { - // nothing to remove - return; - } - const result = await this.client(200000).deleteObjects(params); - if (result.Errors?.length) { - throw new AnonymousError("storage_delete_failed", { - httpStatus: 502, - object: result.Errors, + const prefix = join(this.repoPath(repoId), dir).replace(/\/$/, ""); + const client = this.client(200000); + let continuationToken: string | undefined; + do { + const data = await client.listObjectsV2({ + Bucket: config.S3_BUCKET, + Prefix: prefix, + MaxKeys: 1000, + ContinuationToken: continuationToken, }); - } - - if (data.IsTruncated) { - await this.rm(repoId, dir); - } + const objects = (data.Contents || []) + .filter(({ Key }) => Key === prefix || Key?.startsWith(prefix + "/")) + .map(({ Key }) => ({ Key: Key! })); + if (objects.length) { + const result = await client.deleteObjects({ + Bucket: config.S3_BUCKET, + Delete: { Objects: objects }, + }); + if (result.Errors?.length) { + throw new AnonymousError("storage_delete_failed", { + httpStatus: 502, + object: result.Errors, + }); + } + } + continuationToken = data.IsTruncated ? data.NextContinuationToken : undefined; + if (data.IsTruncated && !continuationToken) { + throw new Error("S3 returned a truncated listing without a continuation token"); + } + } while (continuationToken); } /** @override */ diff --git a/test/production-regressions.test.js b/test/production-regressions.test.js index 1e575ee..e23f5c3 100644 --- a/test/production-regressions.test.js +++ b/test/production-regressions.test.js @@ -85,6 +85,20 @@ describe("production regressions", function () { const storage = new S3(); storage.client = () => client; storage.repoPath = () => "repo"; return storage; } + it("deletes only exact S3 keys and descendants across pages", async function () { + const deleted = []; const tokens = []; + const storage = s3({ + listObjectsV2: async ({ ContinuationToken }) => { + tokens.push(ContinuationToken); + return ContinuationToken ? { Contents: [{ Key: "repo/data/a" }, { Key: "repo/database" }] } : + { Contents: [{ Key: "repo/data.csv" }, { Key: "repo/data" }], IsTruncated: true, NextContinuationToken: "next" }; + }, + deleteObjects: async ({ Delete }) => { deleted.push(...Delete.Objects.map(x => x.Key)); return {}; }, + }); + await storage.rm("repo", "data"); + expect(tokens).to.deep.equal([undefined, "next"]); + expect(deleted).to.deep.equal(["repo/data", "repo/data/a"]); + }); it("rejects S3 per-object deletion failures", async function () { const storage = s3({ listObjectsV2: async () => ({ Contents: [{ Key: "repo/data" }] }),