test(events): regression guard for #5056 AbortSignal exit-hang#5190
Conversation
events.once / events.on with an AbortSignal used to leave a leaked keepalive (pending promise / abort listener) parking the event loop, so the program produced correct output then never exited. The runtime fix landed incidentally with the events.on async-iterator rework (#5099); this adds an explicit guard so it cannot silently regress. The node-suite differential runner does check exit codes, but a hang only surfaces as a timeout there and that runner is not part of the per-PR gate. This stand-alone script (same shape as tests/test_issue_3730_timers_promises_unref_await.sh) compiles both abort fixtures and asserts each exits 0 within a timeout with the expected stdout — a hang trips the timeout (exit 124) and fails. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughA new standalone Bash regression test ChangesAbortSignal exit regression test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_issue_5056_events_abort_exit.sh`:
- Around line 39-50: The exit code normalization in the
wait_for_output_or_timeout function currently only maps SIGTERM (exit code 143)
to the timeout indicator 124, but does not handle SIGKILL (exit code 137).
Modify the condition that checks whether rc equals 143 to also check for 137, so
both SIGTERM and SIGKILL process terminations are normalized to return 124,
ensuring consistent timeout/hang classification regardless of which termination
signal was ultimately required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65468740-93d6-4bc7-8aae-cf8f1db3f302
📒 Files selected for processing (1)
tests/test_issue_5056_events_abort_exit.sh
| ( sleep "$secs" && kill -TERM "$pid" 2>/dev/null && sleep 1 && kill -KILL "$pid" 2>/dev/null ) & | ||
| local watcher=$! | ||
| if wait "$pid" 2>/dev/null; then | ||
| kill -TERM "$watcher" 2>/dev/null || true | ||
| wait "$watcher" 2>/dev/null || true | ||
| return 0 | ||
| fi | ||
| local rc=$? | ||
| kill -TERM "$watcher" 2>/dev/null || true | ||
| wait "$watcher" 2>/dev/null || true | ||
| [[ "$rc" == "143" ]] && return 124 | ||
| return "$rc" |
There was a problem hiding this comment.
Normalize SIGKILL timeout exits to 124 as well.
The fallback timeout path currently maps only 143 (SIGTERM) to 124. On hangs that need SIGKILL, wait returns 137, so the script reports a generic non-zero exit instead of a timeout/hang classification.
Suggested patch
- [[ "$rc" == "143" ]] && return 124
+ [[ "$rc" == "143" || "$rc" == "137" ]] && return 124
return "$rc"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ( sleep "$secs" && kill -TERM "$pid" 2>/dev/null && sleep 1 && kill -KILL "$pid" 2>/dev/null ) & | |
| local watcher=$! | |
| if wait "$pid" 2>/dev/null; then | |
| kill -TERM "$watcher" 2>/dev/null || true | |
| wait "$watcher" 2>/dev/null || true | |
| return 0 | |
| fi | |
| local rc=$? | |
| kill -TERM "$watcher" 2>/dev/null || true | |
| wait "$watcher" 2>/dev/null || true | |
| [[ "$rc" == "143" ]] && return 124 | |
| return "$rc" | |
| ( sleep "$secs" && kill -TERM "$pid" 2>/dev/null && sleep 1 && kill -KILL "$pid" 2>/dev/null ) & | |
| local watcher=$! | |
| if wait "$pid" 2>/dev/null; then | |
| kill -TERM "$watcher" 2>/dev/null || true | |
| wait "$watcher" 2>/dev/null || true | |
| return 0 | |
| fi | |
| local rc=$? | |
| kill -TERM "$watcher" 2>/dev/null || true | |
| wait "$watcher" 2>/dev/null || true | |
| [[ "$rc" == "143" || "$rc" == "137" ]] && return 124 | |
| return "$rc" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_issue_5056_events_abort_exit.sh` around lines 39 - 50, The exit
code normalization in the wait_for_output_or_timeout function currently only
maps SIGTERM (exit code 143) to the timeout indicator 124, but does not handle
SIGKILL (exit code 137). Modify the condition that checks whether rc equals 143
to also check for 137, so both SIGTERM and SIGKILL process terminations are
normalized to return 124, ensuring consistent timeout/hang classification
regardless of which termination signal was ultimately required.
Summary
events.once/events.onwith anAbortSignalused to leave a leaked keepalive (a pending promise / abort listener) parking the event loop — programs printed the correct output, then never exited (#5056).Re-verified on current
main: both repros now exit0and matchnode --experimental-strip-typesbyte-for-byte. The runtime fix landed incidentally with theevents.onasync-iterator rework (#5099); the underlyingevents.{once,on}abort paths clean up their listeners and don't register an active handle. So no runtime change is needed here — this PR adds the regression guard that was missing.Why a dedicated guard
The node-suite differential runner (
scripts/node_suite_run.py) does compare exit codes and time out on hangs, but:perry_err, andSo an exit-hang regression on these two fixtures could merge green. This stand-alone script — same shape as
tests/test_issue_3730_timers_promises_unref_await.sh— pins the behaviour directly: it compiles both abort fixtures and asserts each exits 0 within a timeout with the expected stdout. A hang trips the timeout (exit 124) and fails the test.Fixtures guarded:
test-parity/node-suite/events/once/abort-cleans-pending-error.tstest-parity/node-suite/events/on/async-iterator-abort.tsTest
Closes #5056.
🤖 Generated with Claude Code
Summary by CodeRabbit