fix!: cloud assemblies are not locked (#344)

Cloud Assemblies are supposed to be read-locked as they are being used,
so that concurrent writers into the same directories do not trample on
the `cdk.out` directory as we are deploying it.

This is being done with the class `RWLock` which represents a read/write
lock. A lock is associated to a directory, which can have either at most
one writer or multiple readers.

This PR is a synthesis of #335 and #339.

Closes aws/aws-cdk#32964, closes #335, closes
#339.

# Design

Because we need to add the concept of a lock to a Cloud Assembly, and we
want that resource to be disposable, we introduce the class
`IReadableCloudAssembly`. This interface models both a cloud assembly
and a dispose function which releases the lock and optionally cleans up
the backing directory of the cloud assembly.

Cloud Assembly Sources (`ICloudAssemblySource.produce()`) now returns a
`IReadableCloudAssembly` with a lock associated. The factory functions
start off by acquiring write locks on the backing directory during
synthesis, which are converted to read locks after a successful
synthesis.

* All toolkit functions take an `ICloudAssemblySource`, and dispose of
the produced `IReadableCloudAssembly` after use;
* Except `synth()` now returns a `CachedCloudAssembly`. That class
implements both `IReadableCloudAssembly` (as in, it is a cloud assembly
that is locked and ready to be read), as well as `ICloudAssemblySource`
so that it can be passed into other toolkit functions. The
`CachedCloudAssembly.produce()` returns borrowed versions of
`IReadableCloudAssembly` with dispose functions that don't do anything:
only the top-level dispose actually cleans anything up. This is so that
if the result of `synth()` is passed into (multiple) other toolkit
functions, their dispose calls don't accidentally release the lock. This
could have been done with reference counting, but is currently being
done by giving out a "borrowed" Cloud Assembly which doesn't clean up at
all.
* The locking itself is managed by `ExecutionEnvironment`; this is now a
disposable object which will hold a lock that either gets cleaned up
automatically with the object, or get converted to a reader lock when
the `ExecutionEnvironment` is explicitly marked as having successfully
completed. That same change now allows us to automatically clean up
temporary directories if synthesis fails, or when the CloudAssembly in
them is disposed of.

# Supporting changes

Supporting changes in this PR, necessary to make the above work:

- `StackAssembly`, which is a helper class to query a Cloud Assembly,
used to be an `ICloudAssemblySource` but it no longer is. That
functionality isn't used anywhere.
- Rename `CachedCloudAssemblySource` to `CachedCloudAssembly` and make
`synth()` return this concretely. It makes more sense to think of this
class as a Cloud Assembly (that happens to be usable as a source), than
the reverse.
- Locks can now be released more than once; calls after the first won't
do anything. This is to prevent an `_unlock()` followed by a `dispose()`
from doing something unexpected, and it also shouldn't fail.
- Rename `ILock` => `IReadLock` to contrast it more clearly with an
`IWriteLock`.
- Remove `IdentityCloudAssemblySource`, since it fills much the same
role as `CachedCloudAssembly`.
- `ExecutionEnvironment` is now disposable, and its sync constructor has
been changed to an async factory function.
- A lot of tests that called `cx.produce()` directory now have to use an
`await using` statement to make sure the read locks are cleaned up,
otherwise they get added to the GitHub repo.

# New tests for new contracts

- Cloud Assemblies are properly disposed by all toolkit methods that
take a CloudAssemblySource
- The return value of `toolkit.synth()` can be passed to other toolkit
methods, but its contents are only disposed when the object is
explicitly disposed, not when it's passed to toolkit methods.
- When Cloud Assembly producers fail, they should leave the directory
unlocked if given, or no directory at all if they are temporary.

# Breaking change

BREAKING CHANGE: this change changes the return type of
`toolkit.synth()`: it no longer returns an arbitrary Assembly Source (by
interface), but a specific Assembly, by class, that can also be used as
a source. The return type of `ICloudAssemblySource.produce()` has been
changed to `IReadableCloudAssembly`. This will only affect consumers
with custom implementations of that interface, the factory function APIs
are unchanged.

---
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/tmp-toolkit-helpers/src/api/rwlock.ts

@@ -29,19 +29,24 @@ export class RWLock {
    *
    * No other readers or writers must exist for the given directory.
    */
-  public async acquireWrite(): Promise<IWriterLock> {
+  public async acquireWrite(): Promise<IWriteLock> {
     await this.assertNoOtherWriters();
 
-    const readers = await this.currentReaders();
+    const readers = await this._currentReaders();
     if (readers.length > 0) {
       throw new ToolkitError(`Other CLIs (PID=${readers}) are currently reading from ${this.directory}. Invoke the CLI in sequence, or use '--output' to synth into different directories.`);
     }
 
     await writeFileAtomic(this.writerFile, this.pidString);
 
+    let released = false;
     return {
       release: async () => {
-        await deleteFile(this.writerFile);
+        // Releasing needs a flag, otherwise we might delete a file that some other lock has created in the mean time.
+        if (!released) {
+          await deleteFile(this.writerFile);
+          released = true;
+        }
       },
       convertToReaderLock: async () => {
         // Acquire the read lock before releasing the write lock. Slightly less
@@ -58,7 +63,7 @@ export class RWLock {
    *
    * Will fail if there are any writers.
    */
-  public async acquireRead(): Promise<ILock> {
+  public async acquireRead(): Promise<IReadLock> {
     await this.assertNoOtherWriters();
     return this.doAcquireRead();
   }
@@ -77,27 +82,37 @@ export class RWLock {
   /**
    * Do the actual acquiring of a read lock.
    */
-  private async doAcquireRead(): Promise<ILock> {
+  private async doAcquireRead(): Promise<IReadLock> {
     const readerFile = this.readerFile();
     await writeFileAtomic(readerFile, this.pidString);
+
+    let released = false;
     return {
       release: async () => {
-        await deleteFile(readerFile);
+        // Releasing needs a flag, otherwise we might delete a file that some other lock has created in the mean time.
+        if (!released) {
+          await deleteFile(readerFile);
+          released = true;
+        }
       },
     };
   }
 
   private async assertNoOtherWriters() {
-    const writer = await this.currentWriter();
+    const writer = await this._currentWriter();
     if (writer) {
       throw new ToolkitError(`Another CLI (PID=${writer}) is currently synthing to ${this.directory}. Invoke the CLI in sequence, or use '--output' to synth into different directories.`);
     }
   }
 
   /**
    * Check the current writer (if any)
+   *
+   * Publicly accessible for testing purposes. Do not use.
+   *
+   * @internal
    */
-  private async currentWriter(): Promise<number | undefined> {
+  public async _currentWriter(): Promise<number | undefined> {
     const contents = await readFileIfExists(this.writerFile);
     if (!contents) {
       return undefined;
@@ -115,8 +130,12 @@ export class RWLock {
 
   /**
    * Check the current readers (if any)
+   *
+   * Publicly accessible for testing purposes. Do not use.
+   *
+   * @internal
    */
-  private async currentReaders(): Promise<number[]> {
+  public async _currentReaders(): Promise<number[]> {
     const re = /^read\.([^.]+)\.[^.]+\.lock$/;
     const ret = new Array<number>();
 
@@ -151,18 +170,21 @@ export class RWLock {
 /**
  * An acquired lock
  */
-export interface ILock {
+export interface IReadLock {
+  /**
+   * Release the lock. Can be called more than once.
+   */
   release(): Promise<void>;
 }
 
 /**
  * An acquired writer lock
  */
-export interface IWriterLock extends ILock {
+export interface IWriteLock extends IReadLock {
   /**
    * Convert the writer lock to a reader lock
    */
-  convertToReaderLock(): Promise<ILock>;
+  convertToReaderLock(): Promise<IReadLock>;
 }
 
 /* c8 ignore start */ // code paths are unpredictable

packages/@aws-cdk/toolkit-lib/lib/actions/bootstrap/index.ts

@@ -25,7 +25,7 @@ export class BootstrapEnvironments {
   static fromCloudAssemblySource(cx: ICloudAssemblySource): BootstrapEnvironments {
     return new BootstrapEnvironments(async (ioHost: IIoHost) => {
       const ioHelper = asIoHelper(ioHost, 'bootstrap');
-      const assembly = await assemblyFromSource(ioHelper, cx);
+      await using assembly = await assemblyFromSource(ioHelper, cx);
       const stackCollection = await assembly.selectStacksV2(ALL_STACKS);
       return stackCollection.stackArtifacts.map(stack => stack.environment);
     });

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/cached-source.ts

@@ -0,0 +1,56 @@
+import type * as cxapi from '@aws-cdk/cx-api';
+import { BorrowedAssembly } from './private/borrowed-assembly';
+import type { ICloudAssemblySource, IReadableCloudAssembly } from './types';
+
+/**
+ * A CloudAssemblySource that is caching its result once produced.
+ *
+ * Most Toolkit interactions should use a cached source. Not caching is
+ * relevant when the source changes frequently and it is to expensive to predict
+ * if the source has changed.
+ *
+ * The `CachedCloudAssembly` is both itself a readable CloudAssembly, as well as
+ * a Cloud Assembly Source. The lifetimes of cloud assemblies produced by this
+ * source are coupled to the lifetime of the `CachedCloudAssembly`. In other
+ * words: the `dispose()` functions of those cloud assemblies don't do anything;
+ * only the `dispose()` function of the `CachedCloudAssembly` will be used.
+ *
+ * NOTE: if we are concerned about borrowed assemblies outliving the parent
+ * (i.e. the parent getting disposed while someone is still working with the
+ * borrowed copies), we could consider referencing counting here. That seems
+ * unnecessarily complicated for now, we will just assume that everyone is
+ * being a good citizen an borrowed copies are only used by the toolkit and
+ * immediately disposed of.
+ *
+ * Because `dispose()` is a no-op on the borrowed assembly, you can omit it
+ * without changing behavior, but that would turn into a leak if we ever introduced
+ * reference counting. Failing to dispose the result if a `produce()` call of a
+ * `CachedCloudAssembly` is considered a bug.
+ */
+export class CachedCloudAssembly implements ICloudAssemblySource, IReadableCloudAssembly {
+  private asm: IReadableCloudAssembly;
+
+  public constructor(asm: IReadableCloudAssembly) {
+    this.asm = asm;
+  }
+
+  public get cloudAssembly(): cxapi.CloudAssembly {
+    return this.asm.cloudAssembly;
+  }
+
+  public async produce(): Promise<IReadableCloudAssembly> {
+    return new BorrowedAssembly(this.asm.cloudAssembly);
+  }
+
+  public _unlock() {
+    return this.asm._unlock();
+  }
+
+  public dispose(): Promise<void> {
+    return this.asm.dispose();
+  }
+
+  public [Symbol.asyncDispose](): Promise<void> {
+    return this.dispose();
+  }
+}

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/index.ts

@@ -1,4 +1,5 @@
 export { StackSelectionStrategy, StackSelector } from '../../api/shared-public';
+export * from './cached-source';
 export * from './source-builder';
 export * from './types';
 

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/borrowed-assembly.ts

@@ -0,0 +1,22 @@
+import type * as cxapi from '@aws-cdk/cx-api';
+import type { IReadableCloudAssembly } from '../types';
+
+/**
+ * An implementation of `IReadableCloudAssembly` that does nothing except hold on to the CloudAssembly object
+ *
+ * It does not own a lock, and it does not clean the underlying directory.
+ */
+export class BorrowedAssembly implements IReadableCloudAssembly {
+  constructor(public readonly cloudAssembly: cxapi.CloudAssembly) {
+  }
+
+  public async _unlock(): Promise<void> {
+  }
+
+  public async dispose(): Promise<void> {
+  }
+
+  public async [Symbol.asyncDispose](): Promise<void> {
+  }
+}
+

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/cached-source.ts

@@ -1,11 +1,10 @@
 import type { MissingContext } from '@aws-cdk/cloud-assembly-schema';
-import type * as cxapi from '@aws-cdk/cx-api';
 import type { ToolkitServices } from '../../../toolkit/private';
 import { IO } from '../../io/private';
 import { contextproviders } from '../../shared-private';
 import { PROJECT_CONTEXT, type Context, type IoHelper } from '../../shared-private';
 import { ToolkitError } from '../../shared-public';
-import type { ICloudAssemblySource } from '../types';
+import type { ICloudAssemblySource, IReadableCloudAssembly } from '../types';
 
 export interface ContextAwareCloudAssemblyProps {
   /**
@@ -37,9 +36,23 @@ export interface ContextAwareCloudAssemblyProps {
 }
 
 /**
- * Represent the Cloud Executable and the synthesis we can do on it
+ * A CloudAssemblySource that wraps another CloudAssemblySource and runs a lookup loop on it
+ *
+ * This means that if the underlying CloudAssemblySource produces a manifest
+ * with provider queries in it, the `ContextAwareCloudAssemblySource` will
+ * perform the necessary context lookups and invoke the underlying
+ * `CloudAssemblySource` again with thew missing context information.
+ *
+ * This is only useful if the underlying `CloudAssemblySource` can respond to
+ * this new context information (it must be a CDK app source); if it is just a
+ * static directory, then the contents of the assembly won't change in response
+ * to context.
+ *
+ * The context is passed between `ContextAwareCloudAssemblySource` and the wrapped
+ * cloud assembly source via a contex file on disk, so the wrapped assembly source
+ * should re-read the context file on every invocation.
  */
-export class ContextAwareCloudAssembly implements ICloudAssemblySource {
+export class ContextAwareCloudAssemblySource implements ICloudAssemblySource {
   private canLookup: boolean;
   private context: Context;
   private contextFile: string;
@@ -55,15 +68,16 @@ export class ContextAwareCloudAssembly implements ICloudAssemblySource {
   /**
    * Produce a Cloud Assembly, i.e. a set of stacks
    */
-  public async produce(): Promise<cxapi.CloudAssembly> {
+  public async produce(): Promise<IReadableCloudAssembly> {
     // We may need to run the cloud assembly source multiple times in order to satisfy all missing context
     // (When the source producer runs, it will tell us about context it wants to use
     // but it missing. We'll then look up the context and run the executable again, and
     // again, until it doesn't complain anymore or we've stopped making progress).
     let previouslyMissingKeys: Set<string> | undefined;
     while (true) {
-      const assembly = await this.source.produce();
+      const readableAsm = await this.source.produce();
 
+      const assembly = readableAsm.cloudAssembly;
       if (assembly.manifest.missing && assembly.manifest.missing.length > 0) {
         const missingKeysSet = missingContextKeys(assembly.manifest.missing);
         const missingKeys = Array.from(missingKeysSet);
@@ -99,12 +113,14 @@ export class ContextAwareCloudAssembly implements ICloudAssemblySource {
           }));
           await this.context.save(this.contextFile);
 
-          // Execute again
+          // Execute again. Unlock the assembly here so that the producer can acquire
+          // a read lock on the directory again.
+          await readableAsm._unlock();
           continue;
         }
       }
 
-      return assembly;
+      return readableAsm;
     }
   }
 }

packages/@aws-cdk/toolkit-lib/lib/api/cloud-assembly/private/context-aware-source.ts

@@ -1,6 +1,4 @@
 export * from './context-aware-source';
-export * from './cached-source';
-export * from './identity-source';
 export * from './stack-assembly';
 export * from './exec';
 export * from './prepare-source';