fix: use parseAsync to propagate async handler errors to exit code#46
Merged
nazarhussain merged 2 commits intoChainSafe:mainfrom Mar 30, 2026
Merged
Conversation
The CLI used `void yargs(...).parse()` which discards the Promise from async command handlers. When a benchmark run throws (e.g. all benchmarks fail), the rejection is unhandled and the process exits with code 0 instead of 1. Switch to `.parseAsync()` so the async handler's rejection is properly caught by yargs and routed to the `.fail()` handler which calls `process.exit(1)`. The `.catch()` on parseAsync prevents an unhandled rejection warning since the `.fail()` handler already calls `process.exit(1)`. Ref: ChainSafe/lodestar#7484
Wrap postGaComment in try-catch so that HttpError (e.g. fork PRs lack pull-requests:write permission) logs a warning instead of propagating as a fatal error. The benchmark results themselves are unaffected. This is the same fix applied in PR ChainSafe#45 (fix/fail-on-benchmark-errors).
nazarhussain
approved these changes
Mar 30, 2026
nazarhussain
left a comment
There was a problem hiding this comment.
@lodekeeper void is just syntax sugar from TS, it does not impact on behavior of promise. Actual change is the replacing parse with parseAsync.
Author
|
Good point, thanks for the correction @nazarhussain! You're right — |
This was referenced Mar 30, 2026
nflaig
pushed a commit
to ChainSafe/lodestar
that referenced
this pull request
Mar 30, 2026
## Motivation Bumps `@chainsafe/benchmark` from `1.2.3` to `2.0.2`, which includes fixes for error handling that were silently swallowing benchmark failures. ### Fixes included in 2.0.x - **[PR #45](ChainSafe/benchmark#45 — Benchmark failures are now logged to stderr and cause CI to exit non-zero instead of being silently dropped - **[PR #46](ChainSafe/benchmark#46 — CLI uses `parseAsync()` to properly propagate async handler errors to exit code ### Verification Ran fork-choice benchmarks locally to confirm they pass with the new version: - `computeDeltas` — 8/8 passing - `forkChoice/updateHead` — 9/9 passing - `forkChoice/onAttestation` — 1/1 passing - `utils/bytes` — 20/20 passing Resolves #7484 Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
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.
Problem
The CLI entry point uses
void yargs(...).parse()which discards the Promise from async command handlers. When a benchmark run throws (e.g. all benchmarks fail), the rejection is unhandled and the process exits with code 0 instead of code 1.The
.fail()handler exists and callsprocess.exit(1), but with synchronous.parse()the async rejection never reaches it.Reproduced locally:
Fix
Replace
void yargs(...).parse()with.parseAsync()so the async handler's rejection is properly caught by yargs and routed to the.fail()handler.The
.catch()onparseAsync()prevents an unhandled rejection warning since the.fail()handler already callsprocess.exit(1)before the promise settles.Ref: ChainSafe/lodestar#7484