Skip to content

Commit

Permalink
fix: delete unused [site] assets
Browse files Browse the repository at this point in the history 
We discovered critical issues with the way we expire unused assets with `[site]` (see #666, cloudflare/wrangler-legacy#2224), that we're going back to the legacy manner of handling unused assets, i.e- deleting unused assets.

Fixes #666
  • Loading branch information
@threepointone
threepointone committed Mar 22, 2022
1 parent d504e34 commit 206d319
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 180 deletions.
9 changes: 9 additions & 0 deletions .changeset/ten-llamas-boil.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"wrangler": patch
---

fix: delete unused `[site]` assets

We discovered critical issues with the way we expire unused assets with `[site]` (see https://github.com/cloudflare/wrangler2/issues/666, https://github.com/cloudflare/wrangler/issues/2224), that we're going back to the legacy manner of handling unused assets, i.e- deleting unused assets.

Fixes https://github.com/cloudflare/wrangler2/issues/666
203 changes: 63 additions & 140 deletions packages/wrangler/src/__tests__/publish.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import * as TOML from "@iarna/toml";
import { mockAccountId, mockApiToken } from "./helpers/mock-account-id";
import {
createFetchResult,
setMockFetchKVGetValue,
setMockRawResponse,
setMockResponse,
unsetAllMocks,
Expand Down Expand Up @@ -1238,150 +1237,56 @@ export default{
`);
});

describe("expire unused assets", () => {
it("should expire uploaded assets that aren't included anymore", async () => {
const assets = [
{ filePath: "assets/file-1.txt", content: "Content of file-1" },
{ filePath: "assets/file-2.txt", content: "Content of file-2" },
];
const kvNamespace = {
title: "__test-name-workers_sites_assets",
id: "__test-name-workers_sites_assets-id",
};
writeWranglerToml({
main: "./index.js",
site: {
bucket: "assets",
},
});
writeWorkerSource();
writeAssets(assets);
mockUploadWorkerRequest();
mockSubDomainRequest();
mockListKVNamespacesRequest(kvNamespace);
mockKeyListRequest(kvNamespace.id, [
// Put file-1 in the KV namespace
{ name: "assets/file-1.2ca234f380.txt" },
// As well as a couple from a previous upload
{ name: "assets/file-3.somehash.txt" },
{ name: "assets/file-4.anotherhash.txt" },
]);

// Check that we pull down the values of file-3.txt and file-4.txt
setMockFetchKVGetValue(
"some-account-id",
"__test-name-workers_sites_assets-id",
"assets/file-3.somehash.txt",
Buffer.from("Content of file-3", "utf8").toString("base64")
);
setMockFetchKVGetValue(
"some-account-id",
"__test-name-workers_sites_assets-id",
"assets/file-4.anotherhash.txt",
Buffer.from("Content of file-4", "utf8").toString("base64")
);

// and send them back up
mockUploadAssetsToKVRequest(kvNamespace.id, [
...assets.filter((a) => a.filePath !== "assets/file-1.txt"),
{
filePath: "assets/file-3.txt",
content: "Content of file-3",
// expect to expire this asset 300 seconds from now
expiration_ttl: 300,
},
{
filePath: "assets/file-4.txt",
content: "Content of file-4",
// expect to expire this asset 300 seconds from now
expiration_ttl: 300,
},
]);

await runWrangler("publish");

expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
skipping - already uploaded
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
expiring unused assets/file-3.somehash.txt...
expiring unused assets/file-4.anotherhash.txt...
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
it("should delete uploaded assets that aren't included anymore", async () => {
const assets = [
{ filePath: "assets/file-1.txt", content: "Content of file-1" },
{ filePath: "assets/file-2.txt", content: "Content of file-2" },
];
const kvNamespace = {
title: "__test-name-workers_sites_assets",
id: "__test-name-workers_sites_assets-id",
};
writeWranglerToml({
main: "./index.js",
site: {
bucket: "assets",
},
});
writeWorkerSource();
writeAssets(assets);
mockUploadWorkerRequest();
mockSubDomainRequest();
mockListKVNamespacesRequest(kvNamespace);
mockKeyListRequest(kvNamespace.id, [
// Put file-1 in the KV namespace
{ name: "assets/file-1.2ca234f380.txt" },
// As well as a couple from a previous upload
{ name: "assets/file-3.somehash.txt" },
{ name: "assets/file-4.anotherhash.txt" },
]);

it("should not try to expire uploaded assets that are already set to expire", async () => {
const assets = [
{
filePath: "assets/file-1.txt",
content: "Content of file-1",
},
{ filePath: "assets/file-2.txt", content: "Content of file-2" },
];
const kvNamespace = {
title: "__test-name-workers_sites_assets",
id: "__test-name-workers_sites_assets-id",
};
writeWranglerToml({
main: "./index.js",
site: {
bucket: "assets",
},
});
writeWorkerSource();
writeAssets(assets);
mockUploadWorkerRequest();
mockSubDomainRequest();
mockListKVNamespacesRequest(kvNamespace);
// These return with an expiration added on each of them
mockKeyListRequest(kvNamespace.id, [
// Put file-1 in the KV namespace, with an expiration
{ name: "assets/file-1.2ca234f380.txt", expiration: 10000 },
// As well as a couple from a previous upload
// This one's set to expire so it won't be uploaded back
{ name: "assets/file-3.somehash.txt", expiration: 10000 },
// This one will be sent back with an expiration value
{ name: "assets/file-4.anotherhash.txt" },
]);

setMockFetchKVGetValue(
"some-account-id",
"__test-name-workers_sites_assets-id",
"assets/file-4.anotherhash.txt",
Buffer.from("Content of file-4", "utf8").toString("base64")
);
// we upload only file-1.txt
mockUploadAssetsToKVRequest(kvNamespace.id, [
...assets.filter((a) => a.filePath !== "assets/file-1.txt"),
]);

mockUploadAssetsToKVRequest(kvNamespace.id, [
// so it'll re-upload file-1 because it's set to expire
// and file-2 because it hasn't been uploaded yet
...assets,
// and it'll expire file-4 since it's just not included anymore
{
filePath: "assets/file-4.txt",
content: "Content of file-4",
// expect to expire this asset 300 seconds from now
expiration_ttl: 300,
},
]);
mockDeleteUnusedAssetsRequest(kvNamespace.id, [
"assets/file-3.somehash.txt",
"assets/file-4.anotherhash.txt",
]);

await runWrangler("publish");
await runWrangler("publish");

expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
uploading as assets/file-1.2ca234f380.txt...
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
expiring unused assets/file-4.anotherhash.txt...
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});
expect(std.out).toMatchInlineSnapshot(`
"reading assets/file-1.txt...
skipping - already uploaded
reading assets/file-2.txt...
uploading as assets/file-2.5938485188.txt...
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
test-name.test-sub-domain.workers.dev"
`);
expect(std.err).toMatchInlineSnapshot(`""`);
});
});

Expand Down Expand Up @@ -2759,3 +2664,21 @@ function mockUploadAssetsToKVRequest(
}
);
}

/** Create a mock handler for thr request that does a bulk delete of unused assets */
function mockDeleteUnusedAssetsRequest(
expectedNamespaceId: string,
assets: string[]
) {
setMockResponse(
"/accounts/:accountId/storage/kv/namespaces/:namespaceId/bulk",
"DELETE",
([_url, accountId, namespaceId], { body }) => {
expect(accountId).toEqual("some-account-id");
expect(namespaceId).toEqual(expectedNamespaceId);
const deletes = JSON.parse(body as string);
expect(assets).toEqual(deletes);
return null;
}
);
}
53 changes: 13 additions & 40 deletions packages/wrangler/src/sites.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,20 @@ import ignore from "ignore";
import xxhash from "xxhash-wasm";
import {
createNamespace,
getKeyValue,
listNamespaceKeys,
listNamespaces,
putBulkKeyValue,
deleteBulkKeyValue,
} from "./kv";
import type { Config } from "./config";
import type { KeyValue, NamespaceKeyInfo } from "./kv";
import type { KeyValue } from "./kv";
import type { XXHashAPI } from "xxhash-wasm";

/** Paths to always ignore. */
const ALWAYS_IGNORE = new Set(["node_modules"]);
const HIDDEN_FILES_TO_INCLUDE = new Set([
".well-known", // See https://datatracker.ietf.org/doc/html/rfc8615
]);
const FIVE_MINUTES = 60 * 5; // expressed in seconds, since that's what the api expects

async function* getFilesInFolder(dirPath: string): AsyncIterable<string> {
const files = await readdir(dirPath, { withFileTypes: true });
Expand Down Expand Up @@ -125,10 +124,7 @@ export async function syncAssets(

// let's get all the keys in this namespace
const result = await listNamespaceKeys(accountId, namespace);
const keyMap = result.reduce<Record<string, NamespaceKeyInfo>>(
(km, key) => Object.assign(km, { [key.name]: key }),
{}
);
const keys = new Set(result.map((x) => x.name));

const manifest: Record<string, string> = {};
const toUpload: KeyValue[] = [];
Expand All @@ -153,7 +149,7 @@ export async function syncAssets(
validateAssetKey(assetKey);

// now put each of the files into kv
if (!(assetKey in keyMap) || keyMap[assetKey].expiration) {
if (!keys.has(assetKey)) {
console.log(`uploading as ${assetKey}...`);
toUpload.push({
key: assetKey,
Expand All @@ -163,43 +159,20 @@ export async function syncAssets(
} else {
console.log(`skipping - already uploaded`);
}
// remove the key from the set so we know we've seen it
delete keyMap[assetKey];
manifest[path.relative(siteAssets.baseDirectory, file)] = assetKey;
}

// `keyMap` now contains the assets that we need to expire
// remove the key from the set so we know what we've already uploaded
keys.delete(assetKey);

let toExpire: KeyValue[] = [];
for (const asset of Object.values(keyMap)) {
if (!asset.expiration) {
console.log(`expiring unused ${asset.name}...`);
toExpire.push({
key: asset.name,
value: "", // we'll fill all the values in one go
// we use expiration_ttl, since we can't trust the time
// that a deploy source might provide (eg - https://github.com/cloudflare/wrangler/issues/2224)
expiration_ttl: FIVE_MINUTES,
base64: true,
});
}
manifest[path.relative(siteAssets.baseDirectory, file)] = assetKey;
}

// TODO: batch these in groups if it causes problems
toExpire = await Promise.all(
toExpire.map(async (asset) => ({
...asset,
// it would be great if we didn't have to do this fetch at all
value: await getKeyValue(accountId, namespace, asset.key),
}))
);
await Promise.all([
// upload all the new assets
putBulkKeyValue(accountId, namespace, toUpload, () => {}),
// delete all the unused assets
deleteBulkKeyValue(accountId, namespace, Array.from(keys), () => {}),
]);

await putBulkKeyValue(
accountId,
namespace,
toUpload.concat(toExpire),
() => {}
);
return { manifest, namespace };
}

Expand Down

0 comments on commit 206d319

Please sign in to comment.