fix(cli): stop the top-level error handler racing yargs - #378
Draft
so0k wants to merge 1 commit into
Draft
Conversation
`cdktn.ts` registered an `async` `.fail()` handler that yargs does not await - the code's own comment said so - and called `yargs.exit(1, error)` as its first statement. The error was therefore printed once by the handler and again by Node's unhandled-rejection path, followed by "Collecting Debug Information..." and a PromiseRejectionHandledWarning. Measured against the current build, the outcome varied by how the run failed: * an unexpected internal error printed the message and stack twice and never reached the "Debug Information:" label at all - the awaited collection never completed; * a terraform failure printed `undefined` / `undefined` and then hard-aborted with ERR_UNHANDLED_REJECTION. Both are deterministic, not timing-dependent, which makes them testable. The second case had a second cause: `terraform-cli.ts` threw a raw string, so `error.message` and `error.stack` were both `undefined`. It now throws an Error. Deliberately a plain Error rather than `Errors.External`: a non-zero terraform exit is exactly what debug collection exists for, and the `Errors` factories fire an unawaited telemetry POST that a terraform failure has no reason to send. The handler moves to `bin/error-handling.ts`. `.fail()` is now synchronous and only records the failure - it still calls `yargs.exit(1, error)` for its load-bearing `hasOutput` side effect - and `runCli` awaits `parseAsync()`, reports once through a single awaited path, and makes the one `process.exit()` call. The try/catch around `parseAsync` also catches synchronous handler throws, which yargs 17 never routes to `.fail()` at all. Crash reporting needed care. Nothing in the old handler reported to Sentry: the `yargs.exit()` on the first line exits the process, so the handler's own tail never ran. What reached Sentry was the *orphaned rejection* from the un-awaited `.argv` promise, picked up by the global unhandled-rejection integration - which is why the report for the crash fixed in #375 carries `mechanism: onunhandledrejection`. Handling the rejection properly removes that channel, so `reportFailure` now captures explicitly before flushing: for unexpected errors and for External, but not for Usage, which cli-core's `beforeSend` already drops. Verified end to end rather than by mock - a test points a real Sentry client at a local sink in a child process and asserts the envelope arrives before exit, since asserting only that `close()` was called is exactly what would let a silently dead reporting path pass. Exit codes are unchanged; the measured baseline for every failure class was recorded before the change and re-checked after. Fixes #361. Fixes #360 (the trailing rejection dump; the duplicated Terraform error block in that issue is a separate defect in `util.ts`).
so0k
marked this pull request as draft
August 7, 2026 16:28
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
Fixes #361
Fixes #360 — the trailing unhandled-rejection dump. The duplicated Terraform error block also reported there
is a separate defect in
@cdktn/commons/src/util.tsand is not touched here.Description
cdktn.tsregistered anasync.fail()handler that yargs does not await — the code's own comment said so —and called
yargs.exit(1, error)as its first statement. So the error printed once from the handler and againfrom Node's unhandled-rejection path, followed by
Collecting Debug Information...and aPromiseRejectionHandledWarning.Measured, not inferred
Both failure classes were reproduced against a build of this branch's base before any change:
cdk.tf.json+--skip-synth)Debug Information:label — the awaited collection never completedTerraformOutputreferencing an undeclared resource)undefined/undefined, then a hardERR_UNHANDLED_REJECTIONabortBoth are deterministic rather than timing-dependent, which is what made them testable. The second is also a
much cheaper reproduction than the CTRL+C-during-
initrace described in #361 — no signal timing, no provider,no credentials.
The
undefined/undefinedsignature had a second cause:terraform-cli.tsthrew a raw string, soerror.messageanderror.stackwere both undefined. It now throws anError. Deliberately a plainErrorrather than
Errors.External— a non-zero terraform exit is exactly what debug collection exists for, and theErrorsfactories fire an unawaited telemetry POST that a terraform failure has no reason to send.The fix
The handler moves to
bin/error-handling.ts:.fail()is now synchronous and only records the failure. It still callsyargs.exit(1, error)for itsload-bearing
hasOutputside effect.runCliawaitsparseAsync(), reports once through a single awaited path, and makes the oneprocess.exit()call. Its
try/catchalso catches synchronous handler throws, which yargs 17 never routes to.fail()at all.Crash reporting needed care
This is the part worth reviewing closely. Nothing in the old handler reported to Sentry: the
yargs.exit()on its first line exits the process, so the handler's own tail never ran. What actually reported crashes was the
orphaned rejection from the un-awaited
.argvpromise, picked up by Sentry's global unhandled-rejectionintegration — which is why the Sentry report for the crash fixed in #375 carries
mechanism: onunhandledrejection.Handling the rejection properly removes that channel. So
reportFailurenow captures explicitly beforeflushing: for unexpected errors and for
External, but not forUsage, which cli-core'sbeforeSendalreadydrops deliberately.
Verified end to end rather than by mock — a test points a real Sentry client at a local sink in a child process
and asserts the envelope arrives before exit. Asserting only that
close()was called is precisely what wouldlet a silently dead reporting path pass review, and an earlier revision of this branch did exactly that.
Exit codes
Unchanged. The baseline for every failure class was measured before the change and re-checked after.
Out of scope, found along the way
cmds/get.tscallsreadConfigSync()at module top level and is unconditionally imported bycdktn.ts,so a malformed
cdktf.jsoncrashes the CLI at require-time for every command — before.fail()is evenregistered, bypassing all of this. Pre-existing; happy to file it.
cdktn.tsnever calls.strict(), so unknown flags are silently ignored (cdktn synth --nopeexits 0).Checklist