Skip to content

fix(cli): stop the top-level error handler racing yargs - #378

Draft
so0k wants to merge 1 commit into
fix/outputs-file-write-failuresfrom
fix/toplevel-fail-handler
Draft

fix(cli): stop the top-level error handler racing yargs#378
so0k wants to merge 1 commit into
fix/outputs-file-write-failuresfrom
fix/toplevel-fail-handler

Conversation

@so0k

@so0k so0k commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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.ts and is not touched here.

Third in a stack. Targets fix/outputs-file-write-failures, so the diff here is only this change.
Review order: #375#376 → this.

Description

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. So the error printed once from the handler and again
from Node's unhandled-rejection path, followed by Collecting Debug Information... and a
PromiseRejectionHandledWarning.

Measured, not inferred

Both failure classes were reproduced against a build of this branch's base before any change:

Trigger Before
Unexpected internal error (corrupt cdk.tf.json + --skip-synth) message + stack printed twice; never reached the Debug Information: label — the awaited collection never completed
Terraform failure (TerraformOutput referencing an undeclared resource) undefined / undefined, then a hard ERR_UNHANDLED_REJECTION abort

Both 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-init race described in #361 — no signal timing, no provider,
no credentials.

The undefined / undefined signature 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 fix

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.
  • runCli awaits parseAsync(), reports once through a single awaited path, and makes the one process.exit()
    call. Its try/catch also 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 .argv promise, picked up by Sentry's global unhandled-rejection
integration — which is why the Sentry 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 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 would
let 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.ts calls readConfigSync() at module top level and is unconditionally imported by cdktn.ts,
    so a malformed cdktf.json crashes the CLI at require-time for every command — before .fail() is even
    registered, bypassing all of this. Pre-existing; happy to file it.
  • cdktn.ts never calls .strict(), so unknown flags are silently ignored (cdktn synth --nope exits 0).

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 — n/a, no documented behaviour changes
  • 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

`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
so0k requested a review from a team as a code owner August 7, 2026 15:13
@so0k
so0k marked this pull request as draft August 7, 2026 16:28
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.

1 participant