fix(cli): assets shared between stages lead to an error

packages/aws-cdk/lib/util/work-graph-builder.ts

@@ -19,7 +19,6 @@ export class WorkGraphBuilder {
     'stack': 5,
   };
   private readonly graph = new WorkGraph();
-  private readonly assetBuildNodes = new Map<string, AssetBuildNode>;
 
   constructor(private readonly prebuildAssets: boolean, private readonly idPrefix = '') { }
 
@@ -39,12 +38,16 @@ export class WorkGraphBuilder {
    */
   // eslint-disable-next-line max-len
   private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) {
-    const buildId = `${this.idPrefix}${asset.id}-build`;
+    // Just the artifact identifier
+    const assetId = asset.id.assetId;
+    // Unique per destination where the artifact needs to go
+    const assetDestinationId = `${asset.id}`;
 
-    // Add the build node, but only one per "source"
-    // The genericSource includes a relative path we could make absolute to do more effective deduplication of build steps. Not doing that right now.
-    const assetBuildNodeKey = JSON.stringify(asset.genericSource);
-    if (!this.assetBuildNodes.has(assetBuildNodeKey)) {
+    const buildId = `${this.idPrefix}${assetId}-build`;
+    const publishNodeId = `${this.idPrefix}${assetDestinationId}-publish`;
+
+    // Build node only gets added once because they are all the same
+    if (!this.graph.tryGetNode(buildId)) {
       const node: AssetBuildNode = {
         type: 'asset-build',
         id: buildId,
@@ -60,13 +63,9 @@ export class WorkGraphBuilder {
         deploymentState: DeploymentState.PENDING,
         priority: WorkGraphBuilder.PRIORITIES['asset-build'],
       };
-      this.assetBuildNodes.set(assetBuildNodeKey, node);
       this.graph.addNodes(node);
     }
 
-    // Always add the publish
-    const publishNodeId = `${this.idPrefix}${asset.id}-publish`;
-
     const publishNode = this.graph.tryGetNode(publishNodeId);
     if (!publishNode) {
       this.graph.addNodes({

packages/aws-cdk/test/work-graph-builder.test.ts

@@ -3,6 +3,7 @@ import * as path from 'path';
 import * as cxschema from '@aws-cdk/cloud-assembly-schema';
 import * as cxapi from '@aws-cdk/cx-api';
 import { CloudAssemblyBuilder } from '@aws-cdk/cx-api';
+import { WorkGraph } from '../lib/util/work-graph';
 import { WorkGraphBuilder } from '../lib/util/work-graph-builder';
 import { AssetBuildNode, AssetPublishNode, StackNode, WorkNode } from '../lib/util/work-graph-types';
 
@@ -36,14 +37,14 @@ describe('with some stacks and assets', () => {
 
     expect(graph.node('F1:D1-publish')).toEqual(expect.objectContaining({
       type: 'asset-publish',
-      dependencies: new Set(['F1:D1-build']),
+      dependencies: new Set(['F1-build']),
     } as Partial<AssetPublishNode>));
   });
 
   test('with prebuild off, asset building inherits dependencies from their parent stack', () => {
     const graph = new WorkGraphBuilder(false).build(assembly.artifacts);
 
-    expect(graph.node('F1:D1-build')).toEqual(expect.objectContaining({
+    expect(graph.node('F1-build')).toEqual(expect.objectContaining({
       type: 'asset-build',
       dependencies: new Set(['stack0', 'stack1']),
     } as Partial<AssetBuildNode>));
@@ -52,7 +53,7 @@ describe('with some stacks and assets', () => {
   test('with prebuild on, assets only have their own dependencies', () => {
     const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
 
-    expect(graph.node('F1:D1-build')).toEqual(expect.objectContaining({
+    expect(graph.node('F1-build')).toEqual(expect.objectContaining({
       type: 'asset-build',
       dependencies: new Set(['stack0']),
     } as Partial<AssetBuildNode>));
@@ -138,17 +139,11 @@ describe('tests that use assets', () => {
 
     const assembly = rootBuilder.buildAssembly();
 
-    const traversal: string[] = [];
     const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
-    await graph.doParallel(1, {
-      deployStack: async (node) => { traversal.push(node.id); },
-      buildAsset: async (node) => { traversal.push(node.id); },
-      publishAsset: async (node) => { traversal.push(node.id); },
-    });
+    const traversal = await traverseAndRecord(graph);
 
-    expect(traversal).toHaveLength(4); // 1 asset build, 1 asset publish, 2 stacks
     expect(traversal).toEqual([
-      'work-graph-builder.test.js:D1-build',
+      'work-graph-builder.test.js-build',
       'work-graph-builder.test.js:D1-publish',
       'StackA',
       'StackB',
@@ -171,6 +166,53 @@ describe('tests that use assets', () => {
     // THEN
     expect(graph.findCycle()).toBeUndefined();
   });
+
+  test('the same asset to different destinations is only built once', async () => {
+    addStack(rootBuilder, 'StackA', {
+      environment: 'aws://11111/us-east-1',
+      dependencies: ['StackA.assets'],
+    });
+    addAssets(rootBuilder, 'StackA.assets', {
+      files: {
+        abcdef: {
+          source: { path: __dirname },
+          destinations: {
+            D1: { bucketName: 'bucket1', objectKey: 'key' },
+            D2: { bucketName: 'bucket2', objectKey: 'key' },
+          },
+        },
+      },
+    });
+
+    addStack(rootBuilder, 'StackB', {
+      environment: 'aws://11111/us-east-1',
+      dependencies: ['StackB.assets', 'StackA'],
+    });
+    addAssets(rootBuilder, 'StackB.assets', {
+      files: {
+        abcdef: {
+          source: { path: __dirname },
+          destinations: {
+            D3: { bucketName: 'bucket3', objectKey: 'key' },
+          },
+        },
+      },
+    });
+
+    const assembly = rootBuilder.buildAssembly();
+
+    const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
+    const traversal = await traverseAndRecord(graph);
+
+    expect(traversal).toEqual([
+      'abcdef-build',
+      'abcdef:D1-publish',
+      'abcdef:D2-publish',
+      'StackA',
+      'abcdef:D3-publish',
+      'StackB',
+    ]);
+  });
 });
 
 /**
@@ -251,3 +293,13 @@ function assertableNode<A extends WorkNode>(x: A) {
     dependencies: Array.from(x.dependencies),
   };
 }
+
+async function traverseAndRecord(graph: WorkGraph) {
+  const ret: string[] = [];
+  await graph.doParallel(1, {
+    deployStack: async (node) => { ret.push(node.id); },
+    buildAsset: async (node) => { ret.push(node.id); },
+    publishAsset: async (node) => { ret.push(node.id); },
+  });
+  return ret;
+}

packages/cdk-assets/lib/asset-manifest.ts

@@ -106,7 +106,10 @@ export class AssetManifest {
   }
 
   /**
-   * List of assets, splat out to destinations
+   * List of assets per destination
+   *
+   * Returns one asset for every publishable destination. Multiple asset
+   * destinations may share the same asset source.
    */
   public get entries(): IManifestEntry[] {
     return [
@@ -145,7 +148,7 @@ const ASSET_TYPES: AssetType[] = ['files', 'dockerImages'];
  */
 export interface IManifestEntry {
   /**
-   * The identifier of the asset
+   * The identifier of the asset and its destination
    */
   readonly id: DestinationIdentifier;
 
@@ -209,10 +212,16 @@ export class DockerImageManifestEntry implements IManifestEntry {
 
 /**
  * Identify an asset destination in an asset manifest
+ *
+ * When stringified, this will be a combination of the source
+ * and destination IDs.
  */
 export class DestinationIdentifier {
   /**
    * Identifies the asset, by source.
+   *
+   * The assetId will be the same between assets that represent
+   * the same physical file or image.
    */
   public readonly assetId: string;