Repository navigation
fix(cli): don't downgrade prebuilt providers on a transient registry failure - #298
Conversation
so0k
left a comment
There was a problem hiding this comment.
Improves reliability & correctness: it stops transient registry failures from being misclassified as “version absent,” avoids silent provider downgrades, and surfaces a detailed External error instead.
Concerns checked and resolved
-
Is
fetchWithRetrylogic already pre-existing in CLI/package-manager?
Resolved: no. Search only did not surface existing shared CLI/package-manager fetch retry helper to reuse. -
Are
promise-retry/@gar/promise-retryalready dependencies?
Resolved: they are present only as transitive dev/tooling dependencies, not as direct@cdktn/cli-coredependencies. They come from npm/lerna/pacote tooling paths such aslerna -> pacote,npm-registry-fetch,make-fetch-happen,@npmcli/git, andsigstore.
@cdktn/cli-coredirectly depends onnode-fetch, not these retry packages. So the localfetchWithRetryhelper earns its keep and does not duplicate an existing project-level dependency-manager abstraction. -
Is a local
delayhelper duplicating an existing CLI helper?
Resolved: no shared reusable CLI delay helper was found. There are isolatedsetTimeout/sleeppatterns elsewhere, but nothing obvious incli-corethat this code should import. The tiny local helper is acceptable. -
Does the retry logic already use exponential backoff?
Resolved: yes. It uses bounded exponential backoff:REGISTRY_PROBE_ATTEMPTS = 3 REGISTRY_PROBE_BACKOFF_MS = 250 delay = 250ms, then 500ms
That is appropriate for this CLI path.
-
Should
429 Retry-Afterbe honored?
Considered a non-issue for this PR. For an interactive CLI, failing fast with a clearExternalerror after bounded retries is acceptable — often preferable to silently blocking for5s/60s. Not honoringRetry-Afterdoes not reintroduce the correctness bug and is not worth a follow-up cleanup here. -
Do the tests provide meaningful coverage?
Resolved: yes. The tests exercise definitive success/absence and transient-failure paths across Maven, GitHub/Go, PyPI, and NuGet. They are behavior-focused and run in the normal test path.
### 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 #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 `ELOOP`s 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 `copyFileSync` → `EISDIR`) 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.ts` → `toPlanSuccessfully` failure shells out to a real `terraform plan` and fails identically without this change). - Issue #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 - [x] I have updated the PR title to match [CDKTN's style guide](https://github.com/open-constructs/cdk-terrain/blob/main/CONTRIBUTING.md#pull-requests-1) - [x] I have run the linter on my code locally - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation if applicable - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works if applicable - [x] New and existing unit tests pass locally with my changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Related issue
Fixes #297
Description
When
cdktn provider addresolves which prebuilt-provider version to install, it walks the npm candidate versions highest-to-lowest and installs the first thatisNpmVersionAvailablereports as present. The problem: for most languages a transient registry failure (network error, rate limit, 5xx) was indistinguishable from "this version doesn't exist", so the resolver skipped the present newest version and silently installed an older one — no error, no warning.The probes conflated the two failure modes, and the resolution loop's
catchswallowed anything that threw:response.json()succeeded,json.refwasundefined, and the probe returnedfalse. A throttled request read as "version absent".fetchwith no status check; a non-JSON error body maderesponse.json()throw.DependencyManager.getLanguageSpecificPackageVersionwrapped the body intry { … } catch { logger.info("… Skipping…") }, so any throw was logged atinfoand the loop moved on to an older version.This PR gives the probes a shared, deliberate transient-vs-definitive distinction and decides at the loop level what a transient failure should do:
package-manager.ts): a newfetchWithRetryclassifies transient failures — network errors and HTTP408/429/5xx— and retries them (3 attempts, short exponential backoff), while definitive responses (2xx,404, other4xx) return immediately. If the registry stays unreachable after all attempts, it throws anExternalerror rather than reporting the version as absent.isNpmVersionAvailablenow routes throughfetchWithRetryand keys off the HTTP status instead of inferring absence from a missing field. Go checks200vs404(fixing the rate-limit-reads-as-absent bug), Python and .NET check404explicitly, and Java (already probingrepo1.maven.orgdirectly) shares the same retry behaviour.catch(dependency-manager.ts): a registry-unreachable (External) error now propagates and aborts the resolution, so the CLI never silently downgrades on an infrastructure hiccup. A definitive "not available" (afalsereturn, no throw) still advances to the next-oldest candidate as before.Why abort rather than retry-forever or degrade silently: installing an older version than is actually available is a silent correctness problem that's hard to notice after the fact. Surfacing a clear "could not reach the registry" error lets the user retry against a recovered network and get the version they expect.
Checklist