fix: half-written asset zips can be uploaded if process is interrupted (

#22393)

Here's a possible scenario that can lead to "Uploaded file must be a non-empty zip":

- Bundling starts
- A partial zip file is written
- The process is interrupted (`^C` or similar) while writing.
- On next run, the destination file already exists, is taken to be complete, and uploaded.

In this fix, we'll write the zip to a temporary file and atomically rename it into place once finished, thereby avoiding this possible scenario.

Attempts to fix #18459.

I don't have tests for this, I don't know how to write them properly. To my mind, if the code looks good and we pass integration tests, that should be sufficient. Add a global asset salting mechanism so that we know all asset hashes are always unique.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/@aws-cdk/core/lib/asset-staging.ts

@@ -15,6 +15,8 @@ import { Stage } from './stage';
 
 const ARCHIVE_EXTENSIONS = ['.tar.gz', '.zip', '.jar', '.tar', '.tgz'];
 
+const ASSET_SALT_CONTEXT_KEY = '@aws-cdk/core:assetHashSalt';
+
 /**
  * A previously staged asset
  */
@@ -160,8 +162,13 @@ export class AssetStaging extends Construct {
   constructor(scope: Construct, id: string, props: AssetStagingProps) {
     super(scope, id);
 
+    const salt = this.node.tryGetContext(ASSET_SALT_CONTEXT_KEY);
+
     this.sourcePath = path.resolve(props.sourcePath);
-    this.fingerprintOptions = props;
+    this.fingerprintOptions = {
+      ...props,
+      extraHash: props.extraHash || salt ? `${props.extraHash ?? ''}${salt ?? ''}` : undefined,
+    };
 
     if (!fs.existsSync(this.sourcePath)) {
       throw new Error(`Cannot find asset at ${this.sourcePath}`);
@@ -329,6 +336,7 @@ export class AssetStaging extends Construct {
 
     // Calculate assetHash afterwards if we still must
     assetHash = assetHash ?? this.calculateHash(this.hashType, bundling, bundledAsset.path);
+
     const stagedPath = path.resolve(this.assetOutdir, renderAssetFilename(assetHash, bundledAsset.extension));
 
     this.stageAsset(bundledAsset.path, stagedPath, 'move');

packages/@aws-cdk/core/test/staging.test.ts

@@ -262,6 +262,24 @@ describe('staging', () => {
     expect(withExtra.assetHash).toEqual('c95c915a5722bb9019e2c725d11868e5a619b55f36172f76bcbcaa8bb2d10c5f');
   });
 
+  test('can specify extra asset salt via context key', () => {
+    // GIVEN
+    const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');
+
+    const app = new App();
+    const stack = new Stack(app, 'stack');
+
+    const saltedApp = new App({ context: { '@aws-cdk/core:assetHashSalt': 'magic' } });
+    const saltedStack = new Stack(saltedApp, 'stack');
+
+    // WHEN
+    const asset = new AssetStaging(stack, 'X', { sourcePath: directory });
+    const saltedAsset = new AssetStaging(saltedStack, 'X', { sourcePath: directory });
+
+    // THEN
+    expect(asset.assetHash).not.toEqual(saltedAsset.assetHash);
+  });
+
   test('with bundling', () => {
     // GIVEN
     const app = new App({ context: { [cxapi.NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: false } });

packages/aws-cdk/test/integ/cli/app/app.js

@@ -369,7 +369,11 @@ class BuiltinLambdaStack extends cdk.Stack {
   }
 }
 
-const app = new cdk.App();
+const app = new cdk.App({
+  context: {
+    '@aws-cdk/core:assetHashSalt': `${Date.now()}`, // Force all assets to be unique
+  },
+});
 
 const defaultEnv = {
   account: process.env.CDK_DEFAULT_ACCOUNT,

packages/cdk-assets/lib/private/archive.ts

@@ -6,7 +6,17 @@ import * as glob from 'glob';
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 const archiver = require('archiver');
 
-export function zipDirectory(directory: string, outputFile: string): Promise<void> {
+type Logger = (x: string) => void;
+
+export async function zipDirectory(directory: string, outputFile: string, logger: Logger): Promise<void> {
+  // We write to a temporary file and rename at the last moment. This is so that if we are
+  // interrupted during this process, we don't leave a half-finished file in the target location.
+  const temporaryOutputFile = `${outputFile}._tmp`;
+  await writeZipFile(directory, temporaryOutputFile);
+  await moveIntoPlace(temporaryOutputFile, outputFile, logger);
+}
+
+function writeZipFile(directory: string, outputFile: string): Promise<void> {
   return new Promise(async (ok, fail) => {
     // The below options are needed to support following symlinks when building zip files:
     // - nodir: This will prevent symlinks themselves from being copied into the zip.
@@ -44,6 +54,46 @@ export function zipDirectory(directory: string, outputFile: string): Promise<voi
     }
 
     await archive.finalize();
-
   });
+}
+
+/**
+ * Rename the file to the target location, taking into account that we may see EPERM on Windows
+ * while an Antivirus scanner still has the file open, so retry a couple of times.
+ */
+async function moveIntoPlace(source: string, target: string, logger: Logger) {
+  let delay = 100;
+  let attempts = 5;
+  while (true) {
+    try {
+      if (await pathExists(target)) {
+        await fs.unlink(target);
+      }
+      await fs.rename(source, target);
+      return;
+    } catch (e) {
+      if (e.code !== 'EPERM' || attempts-- <= 0) {
+        throw e;
+      }
+      logger(e.message);
+      await sleep(Math.floor(Math.random() * delay));
+      delay *= 2;
+    }
+  }
+}
+
+function sleep(ms: number) {
+  return new Promise(ok => setTimeout(ok, ms));
+}
+
+async function pathExists(x: string) {
+  try {
+    await fs.stat(x);
+    return true;
+  } catch (e) {
+    if (e.code === 'ENOENT') {
+      return false;
+    }
+    throw e;
+  }
 }

packages/cdk-assets/lib/private/handlers/files.ts

@@ -143,7 +143,7 @@ export class FileAssetHandler implements IAssetHandler {
       }
 
       this.host.emitMessage(EventType.BUILD, `Zip ${fullPath} -> ${packagedPath}`);
-      await zipDirectory(fullPath, packagedPath);
+      await zipDirectory(fullPath, packagedPath, (m) => this.host.emitMessage(EventType.DEBUG, m));
       return { packagedPath, contentType };
     } else {
       const contentType = mime.getType(fullPath) ?? 'application/octet-stream';

packages/cdk-assets/test/archive.test.ts

@@ -10,13 +10,18 @@ import { rmRfSync } from '../lib/private/fs-extra';
 const exec = promisify(_exec);
 const pathExists = promisify(exists);
 
+function logger(x: string) {
+  // eslint-disable-next-line no-console
+  console.log(x);
+}
+
 test('zipDirectory can take a directory and produce a zip from it', async () => {
   const stagingDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test.archive'));
   const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test.archive.extract'));
   try {
     const zipFile = path.join(stagingDir, 'output.zip');
     const originalDir = path.join(__dirname, 'test-archive');
-    await zipDirectory(originalDir, zipFile);
+    await zipDirectory(originalDir, zipFile, logger);
 
     // unzip and verify that the resulting tree is the same
     await exec(`unzip ${zipFile}`, { cwd: extractDir });
@@ -46,9 +51,9 @@ test('md5 hash of a zip stays consistent across invocations', async () => {
   const zipFile1 = path.join(stagingDir, 'output.zip');
   const zipFile2 = path.join(stagingDir, 'output.zip');
   const originalDir = path.join(__dirname, 'test-archive');
-  await zipDirectory(originalDir, zipFile1);
+  await zipDirectory(originalDir, zipFile1, logger);
   await new Promise(ok => setTimeout(ok, 2000)); // wait 2s
-  await zipDirectory(originalDir, zipFile2);
+  await zipDirectory(originalDir, zipFile2, logger);
 
   const hash1 = contentHash(await fs.readFile(zipFile1));
   const hash2 = contentHash(await fs.readFile(zipFile2));
@@ -75,7 +80,7 @@ test('zipDirectory follows symlinks', async () => {
     const originalDir = path.join(__dirname, 'test-archive-follow', 'data');
     const zipFile = path.join(stagingDir, 'output.zip');
 
-    await expect(zipDirectory(originalDir, zipFile)).resolves.toBeUndefined();
+    await expect(zipDirectory(originalDir, zipFile, logger)).resolves.toBeUndefined();
     await expect(exec(`unzip ${zipFile}`, { cwd: extractDir })).resolves.toBeDefined();
     await expect(exec(`diff -bur ${originalDir} ${extractDir}`)).resolves.toBeDefined();
   } finally {