diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index de51ce3c281ef..7c5a0d0ff3762 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/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[]) { diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index 792b858a4d168..c51a9a2b3f4e8 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/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(); + }); } }); } diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index 4df8b12baa1db..39e01cc77c911 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/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 */ diff --git a/packages/aws-cdk/test/work-graph.test.ts b/packages/aws-cdk/test/work-graph.test.ts index 015ccca4152fc..5b7a2d1235ec8 100644 --- a/packages/aws-cdk/test/work-graph.test.ts +++ b/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 {