Repository navigation
fix(cli): forward launcher signals to native child - #1667
Conversation
Forward catchable launcher termination signals to the native child, ignore duplicate deliveries per signal, and remove the listeners when the child exits or errors. This covers the catchable signal path only; SIGKILL and platform-native parent-death handling remain out of scope. Co-authored-by: devinsays <[email protected]>
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ccusage-guide | f9f3ecc | Commit Preview URL Branch Preview URL |
Aug 30 2026, 03:32 AM |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe CLI native spawner now accepts injectable dependencies, forwards platform-specific signals to the native child on every occurrence, and removes listeners after exit or error. Tests cover signal selection, repeated forwarding, result handling, and cleanup. ChangesNative signal forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The npm launcher now forwards catchable termination signals to its native child and cleans up handlers when the child exits or fails to start. This is a localized behavior fix with no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant signalSource
participant createNativeSpawner
participant nativeChild
signalSource->>createNativeSpawner: Emit supported signal
createNativeSpawner->>nativeChild: Call child.kill(signal)
nativeChild-->>createNativeSpawner: Exit or error
createNativeSpawner->>signalSource: Remove signal listeners
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Repeated termination signals can still leave the launcher and native child running when the first signal is handled or ignored.
Reviewed changes
The review covers the new launcher signal forwarding and its lifecycle tests.
- Signal forwarding: Adds platform-specific termination signal handlers that forward signals to the native child.
- Listener cleanup: Removes forwarding handlers when the child exits or fails to spawn, with focused tests for both paths.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Luna (free via Pullfrog for OSS) ｜ 𝕏
ccusage
@ccusage/ccusage-darwin-arm64
@ccusage/ccusage-darwin-x64
@ccusage/ccusage-linux-arm64
@ccusage/ccusage-linux-x64
@ccusage/ccusage-win32-x64
commit: |
ccusage performance comparisonPR SHA: This compares the Rust PR release binary against the configured base package on the same CI runner. Package runtime diagnosticsCompares the PR package wrapper, the installed native optional dependency binary, and the workspace release binary on the same large fixture. This identifies whether slow package results come from JavaScript wrapper overhead, the published native binary build, or the Rust core itself. Fixtures: Claude
Committed fixture performanceCommitted small fixtures for stable PR-to-PR feedback and explicit Claude/Codex command coverage. Fixtures: Claude
Large real-world-shaped fixture performanceGenerated fixtures shaped from aggregate local log statistics: thousands of JSONL files, many small sessions, and a long tail of larger sessions. No real prompts, paths, or outputs are stored in the fixtures. Fixtures: Claude
Artifact size
Lower medians and smaller artifacts are better. CI runner noise still applies; use same-run ratios as directional PR feedback, not release guarantees. |
ccusage performance comparisonPR SHA: This compares the PR package against the configured base package on the same CI runner. Package runtime diagnosticsCompares the PR package wrapper, the installed native optional dependency binary, and the workspace release binary on the same large fixture. This identifies whether slow package results come from JavaScript wrapper overhead, the published native binary build, or the Rust core itself. Fixtures: Claude
Committed fixture performanceCommitted small fixtures for stable PR-to-PR feedback and explicit Claude/Codex command coverage. Fixtures: Claude
Large real-world-shaped fixture performanceGenerated fixtures shaped from aggregate local log statistics: thousands of JSONL files, many small sessions, and a long tail of larger sessions. No real prompts, paths, or outputs are stored in the fixtures. Fixtures: Claude
Artifact size
Lower medians and smaller artifacts are better. CI runner noise still applies; use same-run ratios as directional PR feedback, not release guarantees. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Forward every catchable launcher signal delivery without suppressing repeated deliveries. Use SIGINT, SIGBREAK, and SIGHUP on Windows while retaining the Unix signal set, and cover both behaviors in focused tests. Co-authored-by: devinsays <[email protected]>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The review covers the commits added since the prior Pullfrog review, including repeated signal delivery and Windows console signal handling.
- Removed repeat suppression — Forwarded every supported signal delivery while the native child is running.
- Added Windows signal coverage — Replaced the unsupported Windows
SIGTERMlistener withSIGBREAKandSIGHUP, and added focused tests for the platform-specific set.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using GPT Luna (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/ccusage/src/cli.js">
<violation number="1" location="apps/ccusage/src/cli.js:168">
P2: On Windows these signals never reach the native child as a catchable signal: Node's `subprocess.kill()` coerces `SIGINT`, `SIGBREAK`, and `SIGHUP` into an abrupt force-terminate (TerminateProcess / SIGKILL-like), so it cannot deliver an interrupt the Rust binary can handle. The win32 test mocks `kill` and therefore does not reflect the real behavior, and a Ctrl+C / console-close during a write leaves no graceful-shutdown path for the child. This is a Windows design limitation rather than a regression (SIGINT was already forwarded this way), but it runs counter to the PR's stated 'catchable-signal' premise, so it's worth confirming the intent before relying on it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| */ | ||
| function getForwardedSignals(platform = process.platform) { | ||
| if (platform === 'win32') { | ||
| return ['SIGINT', 'SIGBREAK', 'SIGHUP']; |
There was a problem hiding this comment.
P2: On Windows these signals never reach the native child as a catchable signal: Node's subprocess.kill() coerces SIGINT, SIGBREAK, and SIGHUP into an abrupt force-terminate (TerminateProcess / SIGKILL-like), so it cannot deliver an interrupt the Rust binary can handle. The win32 test mocks kill and therefore does not reflect the real behavior, and a Ctrl+C / console-close during a write leaves no graceful-shutdown path for the child. This is a Windows design limitation rather than a regression (SIGINT was already forwarded this way), but it runs counter to the PR's stated 'catchable-signal' premise, so it's worth confirming the intent before relying on it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/ccusage/src/cli.js, line 168:
<comment>On Windows these signals never reach the native child as a catchable signal: Node's `subprocess.kill()` coerces `SIGINT`, `SIGBREAK`, and `SIGHUP` into an abrupt force-terminate (TerminateProcess / SIGKILL-like), so it cannot deliver an interrupt the Rust binary can handle. The win32 test mocks `kill` and therefore does not reflect the real behavior, and a Ctrl+C / console-close during a write leaves no graceful-shutdown path for the child. This is a Windows design limitation rather than a regression (SIGINT was already forwarded this way), but it runs counter to the PR's stated 'catchable-signal' premise, so it's worth confirming the intent before relying on it.</comment>
<file context>
@@ -165,7 +165,7 @@ function isMainModule({ argvEntry = process.argv[1], moduleUrl, realpathPath = r
function getForwardedSignals(platform = process.platform) {
if (platform === 'win32') {
- return ['SIGINT', 'SIGTERM'];
+ return ['SIGINT', 'SIGBREAK', 'SIGHUP'];
}
</file context>
ccusage performance comparisonPR SHA: This compares the Rust PR release binary against the configured base package on the same CI runner. Package runtime diagnosticsCompares the PR package wrapper, the installed native optional dependency binary, and the workspace release binary on the same large fixture. This identifies whether slow package results come from JavaScript wrapper overhead, the published native binary build, or the Rust core itself. Fixtures: Claude
Committed fixture performanceCommitted small fixtures for stable PR-to-PR feedback and explicit Claude/Codex command coverage. Fixtures: Claude
Large real-world-shaped fixture performanceGenerated fixtures shaped from aggregate local log statistics: thousands of JSONL files, many small sessions, and a long tail of larger sessions. No real prompts, paths, or outputs are stored in the fixtures. Fixtures: Claude
Artifact size
Lower medians and smaller artifacts are better. CI runner noise still applies; use same-run ratios as directional PR feedback, not release guarantees. |
ccusage performance comparisonPR SHA: This compares the PR package against the configured base package on the same CI runner. Package runtime diagnosticsCompares the PR package wrapper, the installed native optional dependency binary, and the workspace release binary on the same large fixture. This identifies whether slow package results come from JavaScript wrapper overhead, the published native binary build, or the Rust core itself. Fixtures: Claude
Committed fixture performanceCommitted small fixtures for stable PR-to-PR feedback and explicit Claude/Codex command coverage. Fixtures: Claude
Large real-world-shaped fixture performanceGenerated fixtures shaped from aggregate local log statistics: thousands of JSONL files, many small sessions, and a long tail of larger sessions. No real prompts, paths, or outputs are stored in the fixtures. Fixtures: Claude
Artifact size
Lower medians and smaller artifacts are better. CI runner noise still applies; use same-run ratios as directional PR feedback, not release guarantees. |
Addresses #1596\n\nThe npm launcher forwards every catchable termination signal to the native child and removes the forwarding listeners when the child exits or fails to spawn.\n\nThis covers the catchable-signal orphan path. SIGKILL cannot be intercepted by Node, and platform-native parent-death handling remains out of scope, so this PR must not auto-close the issue.\n\nUnix signals: SIGINT, SIGTERM, SIGHUP, SIGQUIT.\nWindows console signals: SIGINT, SIGBREAK, SIGHUP.\n\nValidation:\n- 34 launcher tests passed\n- oxlint passed\n- treefmt passed\n\nThe issue reporter is included as a commit co-author.