Skip to content

fix(lib)!: preserve symlinks in TerraformAsset walkers#321

Merged
so0k merged 2 commits into
open-constructs:mainfrom
vincenthsh:fix/320-asset-archive-symlinks
Jul 12, 2026
Merged

fix(lib)!: preserve symlinks in TerraformAsset walkers#321
so0k merged 2 commits into
open-constructs:mainfrom
vincenthsh:fix/320-asset-archive-symlinks

Conversation

@vincenthsh

@vincenthsh vincenthsh commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #320.

TerraformAsset walked source trees with fs.statSync, which follows symlinks. On symlinked trees (e.g. pnpm's node_modules) this meant:

  • Archive bloat: a target reachable through N symlink paths was copied N times into the zip (15 MB → 34 MB on a real Lambda bundle for byte-identical logical content).
  • No symlink fidelity: archives contained zero symlink entries — links became full copies.
  • Hard synth crash: circular symlinks (legitimate in pnpm trees) hit ELOOP. Notably this crashed for every asset type, not just ARCHIVE, because hashPath runs in the TerraformAsset constructor.

This is a fork regression, not inherited from cdktf: upstream archives with archiver, which walks with lstat and emits real symlink entries (cycles are structurally impossible there). The regression entered when archiver was replaced by yazl in #95 — yazl has no symlink awareness at all (addFile stats-and-follows) — and survived the yazl → fflate rewrite in #148, which kept the hand-rolled statSync walk.

The fix

All three walkers in packages/cdktn/src/private/fs.ts now use lstatSync and treat symlinks first-class, restoring archiver/cdktf parity:

  • archiveSync emits real symlink entries: the readlink target as STORED entry data with S_IFLNK | perms in the unix external attributes and version-made-by host = Unix — exactly what archiver produces and what Info-ZIP unzip, Go archive/zip, and the AWS Lambda runtime (verified live on nodejs22.x in TerraformAsset: AssetType.ARCHIVE follows symlinks (statSync), duplicating content and crashing on cyclic links #320) recreate as symlinks. Never recursing through links makes cycles unreachable by construction. fflate 0.8.2 supports this natively via per-file [data, { os, attrs, level }] tuples (attrs must be caller-pre-shifted, (mode << 16) >>> 0).
  • hashPath hashes a symlink by its target path instead of following it — retargeting a link still changes the asset hash, but shared targets are no longer double-counted and cycles no longer crash the constructor.
  • copySync (AssetType.DIRECTORY) recreates symlinks with fs.symlinkSync.

Two latent zip-metadata gaps fixed along the way (both also archiver/cdktf parity):

  • Regular-file unix modes are now preserved (executable bits were silently stripped — matters for Lambda binaries and .bin scripts).
  • Entry mtimes are pinned to a fixed 1980 date, making archives byte-reproducible across synths (fflate defaults mtime to Date.now(), so every synth previously produced different zip bytes — perpetual drift for anyone hashing the archive, e.g. filebase64sha256). The pin uses the local-time Date constructor deliberately: fflate encodes DOS dates from local getters and rejects years < 1980, so a UTC midnight date would underflow to 1979 in timezones west of UTC.

Prior art considered and rejected: terraform-provider-archive always dereferences and has no cycle detection — it accumulated the opt-in exclude_symlink_directories band-aid (v2.4.0, hashicorp/terraform-provider-archive#183; follow-up defect fixed in v2.4.2, #298) and still ELOOPs on cycles. Reverting to archiver would reintroduce the execSync child-process bridge that #148 removed (its API is async-only).

Hash compatibility (updated after review — downscoped)

Review surfaced a domain collision in the first cut of the hashPath change (file bytes and symlink-target bytes shared one unframed stream). Per the requested downscope this PR now carries only the compatibility-preserving resolution (reviewer's option 3):

  • the legacy content hash is kept byte-identical for symlink-free trees (proven by a test that recomputes the historical md5 independently);
  • symlink metadata (relative path + target) is framed into a separate digest and combined under a tagged outer hash only when symlinks exist — so hashes still change only for symlink-bearing trees, which today either bloat 2-3× or crash outright.

The canonical entry-framed scheme and its canonicalAssetHashes feature flag moved to the stack: design issue #322 → implementation #323 → docs #324.

TerraformModuleAsset stays in scope per review: it shares the corrected copier (its private copier sent directory symlinks into copyFileSyncEISDIR) with a regression test.

Testing

  • packages/cdktn/test/archive-symlink.test.ts: 10 tests — the issue's repros (file/dir symlink preservation via system-unzip round-trip + lstat, dedup of shared targets, ELOOP on cycles for both archiveSync and hashPath, hash-changes-on-retarget), copySync symlink recreation, executable-bit preservation, and byte-identical archives across runs. 5 of the 6 core repros fail on main.
  • Full cdktn package suite green (453 passed; the one pre-existing matchers.test.tstoPlanSuccessfully failure shells out to a real terraform plan and fails identically without this change).
  • Issue TerraformAsset: AssetType.ARCHIVE follows symlinks (statSync), duplicating content and crashing on cyclic links #320's literal repro script against the built lib: zipinfo shows lrwxr-xr-x ... stor entries for link-a/link-b, one payload copy (538-byte zip vs 3× 200 KiB before), and the cyclic tree archives to a 120-byte zip instead of aborting synth.

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

🤖 Generated with Claude Code

archiveSync, hashPath, and copySync all walked with fs.statSync, which
follows symlinks: shared targets were duplicated N times into archives
(15 MB -> 34 MB on a real pnpm Lambda bundle), symlink entries were
never emitted, and circular links (legitimate in pnpm node_modules)
crashed synth with ELOOP -- for every asset type, since hashPath runs
in the TerraformAsset constructor.

Walk with lstatSync instead and treat symlinks first-class, restoring
the behavior cdktf gets from archiver (dropped in open-constructs#95 when archiver was
replaced by yazl, kept broken through the fflate rewrite in open-constructs#148):

- archiveSync emits real symlink entries (readlink target as STORED
  data, S_IFLNK unix mode in the external attrs, host os Unix), now
  also preserves regular-file modes (executable bits were stripped),
  and pins entry mtimes to a fixed date so archives are
  byte-reproducible (fflate defaulted to Date.now() per synth)
- hashPath hashes a symlink by its target path, so retargeting still
  changes the asset hash without following (or double-counting) links
- copySync recreates symlinks for AssetType.DIRECTORY assets

Fixes open-constructs#320

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vincenthsh vincenthsh requested a review from a team as a code owner July 11, 2026 07:05
@vincenthsh vincenthsh changed the title fix(lib): preserve symlinks in TerraformAsset walkers fix(lib)!: preserve symlinks in TerraformAsset walkers Jul 11, 2026
@so0k

so0k commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

flagged as breaking change as it changes the checksums / SHA created by the TerraformAsset Archiver, and I agree that is should not be put behind a feature flag because:

Asset hashes change only for trees that contain symlinks — trees that today either bloat 2-3× or crash outright.

Altho I'm not sure if we want this (or it should be possible to opt-in / default opt-out to this mtime behavior)

Entry mtimes are pinned to a fixed 1980 date, making archives byte-reproducible across synths (fflate defaults mtime to Date.now(), so every synth previously produced different zip bytes — perpetual drift for anyone hashing the archive, e.g. filebase64sha256). The pin uses the local-time Date constructor deliberately: fflate encodes DOS dates from local getters and rejects years < 1980, so a UTC midnight date would underflow to 1979 in timezones west of UTC.

@sakul-learning sakul-learning left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

The symlink fix itself is well targeted, and the new tests pass the behavior-oriented/value assessment: each core test exercises an observable regression rather than implementation text. In particular, the extraction tests prove real symlink fidelity; the shared-target size test catches archive materialization/bloat; the circular-link tests protect all three walkers from ELOOP; retargeting checks asset invalidation; and the directory-copy test proves links are recreated rather than dereferenced. The executable-mode test protects a real Lambda/CLI artifact property, while the byte-identical archive test protects deterministic output (subject to resolving the maintainer tradeoff already raised about forcing mtimes). These tests are included by the normal cdktn Nx/Jest target, and the configured full run passed: 39 suites, 454 tests, and 288 snapshots. The focused validations suite and nx build cdktn also passed.

I found one blocking correctness issue in the new hash representation: regular-file bytes and symlink-target bytes occupy the same unframed hash domain, so materially different artifacts can receive the same assetHash and synthesized path. See the inline comment for options and compatibility tradeoffs.

I also think this small PR should cover TerraformModuleAsset's separate source copier. packages/cdktn/src/terraform-module-asset.ts:136-144 sends every non-directory, including a directory symlink, to copyFileSync, which can fail with EISDIR before the corrected TerraformAsset walkers are reached. Reusing the corrected helper (or adding equivalent symlink handling) plus one module-asset regression would address the same class of synthesis failures without materially expanding the PR. GitHub cannot attach a review comment to those unchanged lines, so this item is recorded here and alongside the shared copier.

No workflow files changed, and I found no security concern in the touched code. Ponytail/artifact-value pass: the core regression tests earn their maintenance cost; no speculative abstraction or low-signal static test was added.

Comment thread packages/cdktn/src/private/fs.ts Outdated
Comment thread packages/cdktn/src/private/fs.ts
vincenthsh added a commit to vincenthsh/cdk-terrain that referenced this pull request Jul 11, 2026
…ware copier

Review follow-ups on open-constructs#321:

- hashPath fed symlink targets and file contents into one unframed
  stream, so a file containing 'foo' and a symlink targeting 'foo'
  could produce the same assetHash for materially different artifacts.
  The default scheme now keeps the legacy content hash byte-identical
  for symlink-free trees and, only when symlinks exist, combines it
  with separately-framed symlink metadata under a tagged outer hash.

- New canonicalAssetHashes feature flag (FUTURE_FLAGS, on for new
  projects): every entry is hashed as (type, relative path, payload
  size, payload) in sorted order -- the construction git trees and Nix
  NAR use -- which also makes renames, entry-boundary shifts, and
  file-vs-symlink swaps visible to the hash. Graduates to the default
  at the next major.

- TerraformModuleAsset dropped its private copier, which routed
  directory symlinks into copyFileSync (EISDIR) before the fixed
  walkers could run; it now shares the symlink-aware copySync and the
  flag-aware hashPath.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vincenthsh

Copy link
Copy Markdown
Contributor Author

Altho I'm not sure if we want this (or it should be possible to opt-in / default opt-out to this mtime behavior)

Keeping the pin, on prior art: Go module zips do exactly this — the golang.org/x/mod/zip format spec says "File permissions and timestamps are also ignored" (entries get the zero DOS date; the format bans symlinks outright, so timestamps are its only nondeterminism to kill). Python wheels likewise clamp entry mtimes to 1980 for reproducibility (the zip format can't represent earlier dates). The reproducible-builds ecosystem treats embedded wall-clock timestamps in archives as a defect.

Two things bound the blast radius here: assetHash (and therefore the asset path) comes from hashPath over the source tree, never from the zip bytes, so the pin changes no paths; and the previous behavior was fflate's Date.now() default — meaning nobody can be depending on meaningful mtimes inside these archives today, because every synth already produced fresh ones. The pin only removes noise for anyone hashing the artifact (filebase64sha256 on the archive path currently drifts on every synth).

If someone does turn up with a real need for source mtimes we can add a knob then, but I'd rather not grow the JSII surface for it preemptively.

@sakul-learning

sakul-learning commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Requested downscope: keep option 3 here; move canonical hashing out

The compatibility-preserving option 3 now resolves the blocker from the earlier review: trees without symlinks keep their historical content hash, while symlink-bearing trees combine separately framed symlink metadata under a tagged outer digest. That is sufficient for #320 and should remain in this PR.

Please remove the additional canonicalAssetHashes implementation, feature flag, and canonical-only tests from this PR. I opened #322 for the implementer to take as that independently reviewable, feature-flagged follow-up:

#322

The reason to separate it is not only scope. The current canonical representation has a metadata hole: it frames file/symlink type, relative path, payload length, and payload, but regular-file records omit the mode that archiveSync preserves, and directories contribute no records. I reproduced 06440755 producing identical canonical hashes while producing different archive bytes; adding an empty directory is likewise invisible even where the asset copier emits it.

The Git/Nix precedent did not expose this problem because their canonical formats serialize more than payload framing:

  • Git tree entries include mode, name, and object ID; executable files (100755), ordinary files (100644), symlinks (120000), and subtrees are structurally distinct.
  • Nix NAR represents directory entries/node types and an executable marker.

So the framing pattern was borrowed without the complete metadata model that makes those formats canonical. #322 captures the needed contract and regressions for modes, empty directories, paths, entry boundaries, and symlink metadata.

The TerraformModuleAsset copier fix should stay in this PR. It is not speculative canonical-hash work: its existing copier is another active walker over the same asset source class, and a directory symlink reaches copyFileSync and fails with EISDIR before the corrected TerraformAsset walkers run. Reusing the shared symlink-aware copier is a small deletion/consolidation that fixes the same concrete synthesis-failure class and now has a regression test.

…ware copier

Review follow-ups on open-constructs#321, downscoped per review to the
compatibility-preserving hash only (canonical hashing moved to open-constructs#322):

- hashPath fed symlink targets and file contents into one unframed
  stream, so a file containing 'foo' and a symlink targeting 'foo'
  could produce the same assetHash for materially different artifacts.
  The scheme now keeps the legacy content hash byte-identical for
  symlink-free trees and, only when symlinks exist, combines it with
  separately-framed symlink metadata (relative path + target) under a
  tagged outer hash.

- TerraformModuleAsset dropped its private copier, which routed
  directory symlinks into copyFileSync (EISDIR) before the fixed
  walkers could run; it now shares the symlink-aware copySync.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vincenthsh

Copy link
Copy Markdown
Contributor Author

Downscoped as requested — the branch was force-pushed so this PR is now exactly: the three symlink-aware walkers, the option-3 compatibility-preserving hash, and the shared TerraformModuleAsset copier with its EISDIR regression (kept in scope per your note). The canonicalAssetHashes flag, canonical scheme, and canonical-only tests are gone from this diff.

The follow-up is a stack, review-ready:

Thanks for the git/Nix observation — you were right that the first draft borrowed the framing without the object model that makes those formats canonical.

@sakul-learning sakul-learning left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after the downscope and successful checks on ff817b2. The option-3 hash fix and TerraformModuleAsset copier regression are in scope; canonical hashing remains correctly split into the stacked follow-up.

Documentation follow-up for open-constructs/cdk-terrain-docs: please explain the deterministic ZIP timestamp policy and cite golang.org/x/mod/zip as precedent (its format says “File permissions and timestamps are also ignored”). Phrase the comparison precisely: TerraformAsset archives similarly avoid meaningful source timestamps by pinning ZIP mtimes, but unlike Go module ZIPs they preserve regular-file permission bits and support symlinks.

@so0k so0k merged commit 6360e20 into open-constructs:main Jul 12, 2026
259 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TerraformAsset: AssetType.ARCHIVE follows symlinks (statSync), duplicating content and crashing on cyclic links

4 participants