fix(core): cdk deploy stops early if 2 stacks with a dependency between them share an asset (#25719)

Fixes cdk deploy not completing for some apps by removing cycles from core's worker graph between asset publishing steps & stacks. These cycles may be introduced when there dependencies between stacks and these stacks share assets with the same contents.

Additionally fixes a bug in the worker graph's error handling for cycles. 

Closes #25714

----

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

Author: liamoneill

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

@@ -75,7 +75,10 @@ export class WorkGraphBuilder {
         // This is purely cosmetic: if we don't do this, the progress printing of asset publishing
         // is going to interfere with the progress bar of the stack deployment. We could remove this
         // for overall faster deployments if we ever have a better method of progress displaying.
-        ...this.getDepIds(parentStack.dependencies),
+        // Note: this may introduce a cycle if one of the parent's dependencies is another stack that
+        // depends on this asset. To workaround this we remove these cycles once all nodes have
+        // been added to the graph.
+        ...this.getDepIds(parentStack.dependencies.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact)),
       ]),
       parentStack,
       assetManifestArtifact: assetArtifact,
@@ -114,6 +117,10 @@ export class WorkGraphBuilder {
     }
 
     this.graph.removeUnavailableDependencies();
+
+    // Remove any potentially introduced cycles between asset publishing and the stacks that depend on them.
+    this.removeStackPublishCycles();
+
     return this.graph;
   }
 
@@ -129,6 +136,24 @@ export class WorkGraphBuilder {
     }
     return ids;
   }
+
+  private removeStackPublishCycles() {
+    const stacks = this.graph.nodesOfType('stack');
+    for (const stack of stacks) {
+      for (const dep of stack.dependencies) {
+        const node = this.graph.nodes[dep];
+
+        if (!node || node.type !== 'asset-publish' || !node.dependencies.has(stack.id)) {
+          continue;
+        }
+
+        // Delete the dependency from the asset-publish onto the stack.
+        // The publish -> stack dependencies are purely cosmetic to prevent publish output
+        // from interfering with the progress bar of the stack deployment.
+        node.dependencies.delete(stack.id);
+      }
+    }
+  }
 }
 
 function stacksFromAssets(artifacts: cxapi.CloudArtifact[]) {

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

@@ -165,17 +165,19 @@ export class WorkGraph {
       function startOne(x: WorkNode) {
         x.deploymentState = DeploymentState.DEPLOYING;
         active[x.type]++;
-        void fn(x).then(() => {
-          active[x.type]--;
-          graph.deployed(x);
-          start();
-        }).catch((err) => {
-          active[x.type]--;
-          // By recording the failure immediately as the queued task exits, we prevent the next
-          // queued task from starting.
-          graph.failed(x, err);
-          start();
-        });
+        void fn(x)
+          .finally(() => {
+            active[x.type]--;
+          })
+          .then(() => {
+            graph.deployed(x);
+            start();
+          }).catch((err) => {
+            // By recording the failure immediately as the queued task exits, we prevent the next
+            // queued task from starting.
+            graph.failed(x, err);
+            start();
+          });
       }
     });
   }

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

@@ -109,6 +109,50 @@ test('dependencies on unselected artifacts are silently ignored', async () => {
   }));
 });
 
+test('assets with shared contents between dependant stacks', async () => {
+  const files = {
+    // Referencing an existing file on disk is important here.
+    // It means these two assets will have the same AssetManifest
+    // and the graph will merge the two into a single asset.
+    'work-graph-builder.test.js': {
+      source: { path: __dirname },
+      destinations: {
+        D1: { bucketName: 'bucket', objectKey: 'key' },
+      },
+    },
+  };
+
+  addStack(rootBuilder, 'StackA', {
+    environment: 'aws://11111/us-east-1',
+    dependencies: ['StackA.assets'],
+  });
+  addAssets(rootBuilder, 'StackA.assets', { files });
+
+  addStack(rootBuilder, 'StackB', {
+    environment: 'aws://11111/us-east-1',
+    dependencies: ['StackB.assets', 'StackA'],
+  });
+  addAssets(rootBuilder, 'StackB.assets', { files });
+
+  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); },
+  });
+
+  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:D1-publish',
+    'StackA',
+    'StackB',
+  ]);
+});
+
 /**
  * Write an asset manifest file and add it to the assembly builder
  */

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

@@ -249,7 +249,7 @@ describe('WorkGraph', () => {
     expect(actionedAssets).toEqual(['a-build', 'a-publish', 'A']);
   });
 
-  // Failure
+  // Failure Concurrency
   test.each([
     // Concurrency 1
     { scenario: 'A (error)', concurrency: 1, toDeploy: createArtifacts([{ id: 'A', type: 'stack', displayName: 'A' }]), expectedError: 'A', expectedStacks: [] },
@@ -320,6 +320,37 @@ describe('WorkGraph', () => {
 
     expect(actionedAssets).toStrictEqual(expectedStacks);
   });
+
+  // Failure Graph Circular Dependencies
+  test.each([
+    {
+      scenario: 'A -> A',
+      toDeploy: createArtifacts([
+        { id: 'A', type: 'stack', stackDependencies: ['A'] },
+      ]),
+    },
+    {
+      scenario: 'A -> B, B -> A',
+      toDeploy: createArtifacts([
+        { id: 'A', type: 'stack', stackDependencies: ['B'] },
+        { id: 'B', type: 'stack', stackDependencies: ['A'] },
+      ]),
+    },
+    {
+      scenario: 'A, B -> C, C -> D, D -> B',
+      toDeploy: createArtifacts([
+        { id: 'A', type: 'stack' }, // Add a node to visit first so the infinite loop occurs deeper in the traversal callstack.
+        { id: 'B', type: 'stack', stackDependencies: ['C'] },
+        { id: 'C', type: 'stack', stackDependencies: ['D'] },
+        { id: 'D', type: 'stack', stackDependencies: ['B'] },
+      ]),
+    },
+  ])('Failure - Graph Circular Dependencies - $scenario', async ({ toDeploy }) => {
+    const graph = new WorkGraph();
+    addTestArtifactsToGraph(toDeploy, graph);
+
+    await expect(graph.doParallel(1, callbacks)).rejects.toThrowError(/Unable to make progress anymore/);
+  });
 });
 
 interface TestArtifact {