[dodoshell]

Related issue

Fixes #582

Description

TerraformAsset._onSynthesize removed the asset's whole assets/<id>/ folder before staging, including the artifact for the current hash, so every synth reran every bundler. It now removes only the folders of other hashes, and a bundled asset whose hash is taken before the build (SOURCE, the default, or CUSTOM) keeps the artifact already staged for its hash instead of building it again. Unbundled assets are still re-copied, and OUTPUT is unchanged since it has already built in the constructor. As the bundlerKey docs say, a bundler whose key doesn't capture its build config now reuses the old artifact when that config changes.

This is a behaviour change in three cases, which used to rebuild on every synth and now keep the old artifact until cdktf.out is deleted:

• a hand-written bundler without a bundlerKey whose code or options change, or which is swapped for another bundler without a key;
• a build that reads inputs outside the asset path (a LocalBundler workingDirectory above the source, a monorepo import, a mutable Docker tag like node:20);
• a fixed assetHash (e.g. "v1") on a bundled asset, which keeps its first artifact until the string changes.

Set extraHash to fold such inputs into the hash (or change assetHash when it is set), or delete cdktf.out. The extraHash and bundler TSDoc now say this, and open-constructs/cdk-terrain-docs#41 adds a "Rebuilding Bundled Assets" section to the assets page.

A bundled asset whose hash is known before the build stages into a sibling <hash>.staging folder, which is renamed into place once it succeeds. The rename, and the removal of the old hash folder, the staging folder and other hashes' folders, retry briefly on the errors Windows returns while antivirus or the indexer has a file inside open. If the build fails, the original error is reported even when removing the staging folder fails too; the next synth sweeps the leftover. A failed or interrupted build therefore never leaves a folder a later synth would reuse. Since the hash doesn't depend on the asset type, assets/<id>/<hash>.json records the type of the staged artifact, and its file name for FILE and ARCHIVE. It is written after the rename, and the artifact is reused only when it matches, so changing type (e.g. ARCHIVE → DIRECTORY) rebuilds, while moving a directory asset's source does not. Unbundled and OUTPUT assets are staged in place as before. A folder left half-built by an older version would still be reused; deleting cdktf.out clears it.

Building the same hash once across stacks within one synth (the third suggestion in the issue) is left out to keep this small; it could be a follow-up.

New tests in canonical-asset-hash.test.ts synthesize twice into the same outdir with a bundler that counts its runs:

• an unchanged hash doesn't rerun it;
• a changed hash reruns it and removes the old folder;
• a failed build leaves no artifact behind;
• a failed build whose cleanup also fails still reports the build error, and the next synth rebuilds;
• changing type from ARCHIVE or FILE to DIRECTORY rebuilds;
• moving a directory asset's source doesn't rebuild;
• a rename that fails once with EPERM is retried.

All but the changed-hash case fail without this change.

On Windows, matchers.test.ts, archive-symlink.test.ts and asset-hash.test.ts fail locally with and without this change (no terraform on the PATH, symlinks); everything else in packages/cdktn passes.

Checklist

• I have updated the PR title to match CDKTN's style guide
• I have run the linter on my code locally
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation if applicable
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works if applicable
• New and existing unit tests pass locally with my changes

[jsteinich]

Thanks, this is a clean fix for #582. Staging into a .staging sibling and renaming it into place is a good addition. A folder left over from an interrupted run is swept on the next synth, since its name isn't the hash. Two things before it lands:

1. Changing an asset's type can reuse an artifact of the wrong shape (packages/cdktn/src/terraform-asset.ts:293)

The hash doesn't depend on packaging. A bundled asset hashes the same as ARCHIVE and as DIRECTORY, with or without canonicalAssetHashes (checked locally). A directory packaging's targetPath is the hash folder itself, so existsSync(targetPath) is true whenever any artifact for that hash is there:

1. Synth with type: AssetType.ARCHIVE and a bundler: assets/asset/<H>/archive.zip.
2. Change to type: AssetType.DIRECTORY and synth again: the hash is still <H>, and assets/asset/<H>/ exists, so staging is skipped.
3. The directory asset now contains only archive.zip, not the built tree, and nothing fails.

FILE → DIRECTORY hits the same problem. The other directions rebuild correctly, because they look for a file path that isn't there. On main this can't happen, because the whole folder is wiped each time. One option is to record the packaging (and file name) in a sidecar next to the hash folder, such as assets/<id>/<hash>.json. Keep it in the cleanup loop, and reuse only when it matches. Please add a test for the ARCHIVE → DIRECTORY case.

2. On Windows, renaming a directory fails with EPERM while any file inside it is open (terraform-asset.ts:310)

I checked this locally: with a handle open on a file inside <hash>.staging, fs.renameSync throws EPERM. Antivirus and the search indexer briefly open newly written files, which is why graceful-fs retries rename on Windows. This path now runs for every asset on every synth, unbundled ones included. Before, staging wrote in place and never renamed. Could the rename retry briefly on EPERM/EACCES/EBUSY (a few attempts with a short backoff)? Another option is to use the staging-and-rename path only for bundled assets, where it matters. Unbundled assets are always restaged anyway, so they gain nothing from it.

On the bundlerKey trade-off: I think it's acceptable. It matches the asset-pipeline design (#380: the build is deferred to onSynthesize so it can be skipped, and the hash is taken at construction). IAssetBundler.bundlerKey already documents that an incomplete key "silently reuses a stale artifact". LocalBundler and DockerBundler both build their key from their config. It is still a behaviour change users will notice in two cases:

• a hand-written bundler with no bundlerKey, whose code or options change;
• a build that reads inputs outside the asset path: a LocalBundler workingDirectory above the source, a monorepo import, or a mutable Docker tag like node:20.

These used to rebuild on every synth; now they keep the old artifact until cdktf.out is deleted. Please call it out in the release notes and the assets docs (the docs box is unchecked). Mention extraHash or deleting cdktf.out as the fix.

Not blocking: within one synth, two stacks with the same hash still build twice. Leaving that out is fine, as the PR says; the next synth reuses both.

[dodoshell]

Thanks for checking these locally. Pushed d31763d:

1. Packaging: there's now an assets/<id>/<hash>.json next to the hash folder with the asset type and file name. The cleanup loop keeps it, and an artifact is reused only when it matches. It's removed before any rebuild and written only after the staged folder has been renamed into place, so an interrupted build isn't picked up. New tests cover ARCHIVE → DIRECTORY and FILE → DIRECTORY; both fail on the previous commit.
2. Windows rename: I did both. Only bundled SOURCE/CUSTOM assets go through <hash>.staging and the rename; everything else stages in place again, as on main. The rename retries on EPERM/EACCES/EBUSY, up to 5 attempts with a 50ms doubling backoff, and a test makes renameSync throw EPERM once.
3. Docs: the extraHash and bundler TSDoc now say when a bundled asset keeps its old artifact and that extraHash or deleting cdktf.out forces a rebuild, and the PR description spells out the two cases. I'll open a PR in cdk-terrain-docs adding a "Rebuilding bundled assets" section to content/concepts/assets.mdx. For the release notes, release-please only picks up a BREAKING CHANGE: footer. I can add one to the description (and a ! in the title), as in refactor(lib)!: Use declared targetVersions for synth-time validation of resource moves #453–refactor(lib)!: Use declared targetVersions for synth-time validation of the cloud backend #456, or leave it as a note in the description if you'd rather not list it under breaking changes. Which do you prefer?

[jsteinich]

Thanks, both fixes look right.

• Packaging: the <hash>.json sidecar closes the type-switch case. It is removed before any rebuild and written only after the rename, so a crash anywhere in between just rebuilds on the next synth. The two new → DIRECTORY tests cover what I hit.
• Windows rename: limiting the staging-and-rename path to bundled SOURCE/CUSTOM assets puts unbundled assets back on main's behaviour. I checked the retry against a real handle: another process held a file inside <hash>.staging open for 300ms, and renameSyncWithRetry succeeded after about 370ms. Plain renameSync fails with EPERM there.

On release notes: please don't add a BREAKING CHANGE: footer. This follows the contract the bundlerKey docs already describe, so I'd rather not list it under breaking changes. The behaviour-change paragraph in the description is the right place for it. Could you add one more case there: a fixed assetHash (e.g. "v1") on a bundled asset now keeps its first artifact until the string changes. Before, it rebuilt on every synth.

[eduardomourar]

It would be nice to have those fixed before merging.

[eduardomourar]

Stale artifact reuse — bundler swaps without bundlerKey silently serve old build output

[dodoshell]

This is the trade-off jsteinich and I went through in the first review. The hash is taken at construction, and the only thing that identifies a bundler in it is its bundlerKey. Without one there's nothing stable to tell two bundlers apart: an object literal or a minified class has no reliable name. #582 asks for exactly this reuse (its repro bundler has no key), AWS CDK does the same, and IAssetBundler.bundlerKey already says an incomplete key reuses a stale artifact. LocalBundler, DockerBundler and ChainBundler all set one. Turning reuse off for keyless bundlers would bring back the rebuild-every-synth behaviour for the case in the issue, so I left the code as is. I did add the choice of bundler to the inputs the extraHash docs list, and the description calls this out as a behaviour change, with extraHash or deleting cdktf.out as the fix. Happy to revisit if you see it differently.

[eduardomourar]

It sounds good.

[eduardomourar]

Missing retry on cleanup rmSync (also on line 347) — inconsistent with the new renameSyncWithRetry, can cause spurious Windows failures.

[dodoshell]

Done in 6b5ac3a. There's now an rmSyncWithRetry next to renameSyncWithRetry that uses Node's built-in maxRetries/retryDelay (it retries EBUSY/EPERM/ENOTEMPTY), with the same attempt count and base delay as the rename. It's used for the hash folder before the rename, for the staging-folder cleanup, and for the sweep of other hashes' folders. So a staging folder left behind by a failed cleanup is also removed with retries on the next synth.

[eduardomourar]

This finally cleanup can mask real build errors — a locked-file cleanup failure overwrites the original error.

[dodoshell]

Good catch. The finally is now a catch that tries to remove the staging folder and then rethrows the original error. If that removal fails too, it's ignored: the next synth sweeps <hash>.staging anyway, since its name isn't the hash. On success the rename has already moved the folder, so there's nothing to clean up. New test: the build throws and removing the staging folder throws EBUSY; the synth reports the build error, and the next synth removes the leftover and rebuilds.

[eduardomourar]

Directory asset sidecar includes fileName — causes unnecessary rebuilds on rename/move despite unchanged hash.

[dodoshell]

Right. For a directory the file name was just the source's basename and isn't part of path, so a renamed or moved source rebuilt for nothing. The sidecar now records fileName only for FILE and ARCHIVE. For those the name is part of the path anyway, so the ARCHIVE/FILE → DIRECTORY cases jsteinich raised still rebuild. New test: moving the source directory keeps the artifact.