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

The problem would manifest as this error message: ``` ❌ Deployment failed: Error: Duplicate use of node id: 07a6878c7a2ec9b49ef3c0ece94cef1c2dd20fba34ca9650dfa6e7e00f2b9961:current_account-current_region-build ``` The problem was that we were using the full asset "destination identifier" for both the build and publish steps, but then were trying to use the `source` object to deduplicate build steps. A more robust solution is to only use the asset identifier (excluding the destination identifier) for the build step, which includes all data necessary to deduplicate the asset. No need to look at the source at all anymore. ---- *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/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;