Repository navigation
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`).
|
Absorbed into #62. The error-handling shape (synchronous |
|
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 #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