fix(core): pressing Ctrl-C when content is bundled leaves broken asset (#33692)

When a bundling command is interrupted with Ctrl-C, the asset output directory has already been created. On the next synthesis, we assume the asset has already successfully been produced, don't do any bundling, and upload it.

We will then have produced and uploaded a broken asset.

Instead, the common pattern to handle this is:

- Do the work into a temporary directory
- Rename the temporary directory to the target directory only if the work succeeds.

Closes #33201, closes #32869, relates to #14474.

----

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

Author: rix0rrr

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

@@ -438,19 +438,23 @@ export class AssetStaging extends Construct {
   private bundle(options: BundlingOptions, bundleDir: string) {
     if (fs.existsSync(bundleDir)) { return; }
 
-    fs.ensureDirSync(bundleDir);
+    const tempDir = `${bundleDir}-building`;
+    // Remove the tempDir if it exists, then recreate it
+    fs.rmSync(tempDir, { recursive: true, force: true });
+
+    fs.ensureDirSync(tempDir);
     // Chmod the bundleDir to full access.
-    fs.chmodSync(bundleDir, 0o777);
+    fs.chmodSync(tempDir, 0o777);
 
     let localBundling: boolean | undefined;
     try {
       process.stderr.write(`Bundling asset ${this.node.path}...\n`);
 
-      localBundling = options.local?.tryBundle(bundleDir, options);
+      localBundling = options.local?.tryBundle(tempDir, options);
       if (!localBundling) {
         const assetStagingOptions = {
           sourcePath: this.sourcePath,
-          bundleDir,
+          bundleDir: tempDir,
           ...options,
         };
 
@@ -464,18 +468,11 @@ export class AssetStaging extends Construct {
             break;
         }
       }
-    } catch (err) {
-      // When bundling fails, keep the bundle output for diagnosability, but
-      // rename it out of the way so that the next run doesn't assume it has a
-      // valid bundleDir.
-      const bundleErrorDir = bundleDir + '-error';
-      if (fs.existsSync(bundleErrorDir)) {
-        // Remove the last bundleErrorDir.
-        fs.removeSync(bundleErrorDir);
-      }
 
-      fs.renameSync(bundleDir, bundleErrorDir);
-      throw new Error(`Failed to bundle asset ${this.node.path}, bundle output is located at ${bundleErrorDir}: ${err}`);
+      // Success, rename the tempDir into place
+      fs.renameSync(tempDir, bundleDir);
+    } catch (err) {
+      throw new Error(`Failed to bundle asset ${this.node.path}, bundle output is located at ${tempDir}: ${err}`);
     }
 
     if (FileSystem.isEmpty(bundleDir)) {

packages/aws-cdk-lib/core/lib/assets.ts

@@ -53,8 +53,6 @@ export interface AssetOptions {
    *
    * @default - uploaded as-is to S3 if the asset is a regular file or a .zip file,
    * archived into a .zip file and uploaded to S3 otherwise
-   *
-   *
    */
   readonly bundling?: BundlingOptions;
 }

packages/aws-cdk-lib/core/test/app-that-is-interrupted-during-staging.ts

@@ -0,0 +1,22 @@
+/**
+ * This is a CDK app that is guaranteed to kill itself during bundling
+ */
+import * as path from 'path';
+import { App, AssetStaging, DockerImage, Stack } from '../lib';
+
+const app = new App();
+const stack = new Stack(app, 'stack');
+const directory = path.join(__dirname, 'fs', 'fixtures', 'test1');
+
+const pid = process.pid;
+
+// WHEN
+new AssetStaging(stack, 'Asset', {
+  sourcePath: directory,
+  bundling: {
+    image: DockerImage.fromRegistry('alpine'),
+    command: ['DOCKER_STUB_EXEC', 'kill', `${pid}`],
+  },
+});
+
+app.synth();

packages/aws-cdk-lib/core/test/docker-stub.sh

@@ -49,5 +49,14 @@ if echo "$@" | grep "DOCKER_STUB_SINGLE_FILE"; then
   exit 0
 fi
 
-echo "Docker mock only supports one of the following commands: DOCKER_STUB_SUCCESS_NO_OUTPUT,DOCKER_STUB_FAIL,DOCKER_STUB_SUCCESS,DOCKER_STUB_MULTIPLE_FILES,DOCKER_SINGLE_ARCHIVE"
+if echo "$@" | grep "DOCKER_STUB_EXEC"; then
+  while [[ "$1" != "DOCKER_STUB_EXEC" ]]; do
+    shift
+  done
+  shift
+
+  exec "$@" # Execute what's left
+fi
+
+echo "Docker mock only supports one of the following commands: DOCKER_STUB_SUCCESS_NO_OUTPUT,DOCKER_STUB_FAIL,DOCKER_STUB_SUCCESS,DOCKER_STUB_MULTIPLE_FILES,DOCKER_SINGLE_ARCHIVE,DOCKER_STUB_EXEC, got '$@'"
 exit 1

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

@@ -1,3 +1,4 @@
+import { execSync } from 'child_process';
 import * as os from 'os';
 import * as path from 'path';
 import { testDeprecated } from '@aws-cdk/cdk-build-tools';
@@ -610,13 +611,13 @@ describe('staging', () => {
         image: DockerImage.fromRegistry('alpine'),
         command: [DockerStubCommand.FAIL],
       },
-    })).toThrow(/Failed.*bundl.*asset.*-error/);
+    })).toThrow(/Failed.*bundl.*asset.*-building/);
 
     // THEN
     const assembly = app.synth();
 
     const dir = fs.readdirSync(assembly.directory);
-    expect(dir.some(entry => entry.match(/asset.*-error/))).toEqual(true);
+    expect(dir.some(entry => entry.match(/asset.*-building/))).toEqual(true);
   });
 
   test('bundler re-uses assets from previous synths', () => {
@@ -675,6 +676,44 @@ describe('staging', () => {
     ]);
   });
 
+  test('if bundling is interrupted, target asset directory is not produced', () => {
+    // GIVEN
+    const TEST_OUTDIR = path.join(__dirname, 'cdk.out');
+    if (fs.existsSync(TEST_OUTDIR)) {
+      fs.removeSync(TEST_OUTDIR);
+    }
+
+    // WHEN
+    try {
+      execSync(`npx ts-node ${__dirname}/app-that-is-interrupted-during-staging.ts`, {
+        env: {
+          ...process.env,
+          CDK_OUTDIR: TEST_OUTDIR,
+        },
+      });
+      throw new Error('We expected the above command to fail');
+    } catch (e) {
+      // We expect the command to be terminated with a signal, which sometimes shows
+      // as 'signal' is set to SIGTERM, and on some Linuxes as exitCode = 128 + 15 = 143
+      if (e.signal === 'SIGTERM' || e.status === 143) {
+        // pass
+      } else {
+        throw e;
+      }
+    }
+
+    // THEN
+    const generatedFiles = fs.readdirSync(TEST_OUTDIR);
+    // We expect a 'building' asset directory...
+    expect(generatedFiles).toContainEqual(
+      expect.stringMatching(/^asset\.[0-9a-f]+-building$/),
+    );
+    // ...not a complete asset directory
+    expect(generatedFiles).not.toContainEqual(
+      expect.stringMatching(/^asset\.[0-9a-f]+$/),
+    );
+  });
+
   test('bundler re-uses assets from previous synths, ignoring tokens', () => {
     // GIVEN
     const TEST_OUTDIR = path.join(__dirname, 'cdk.out');