fix: resolve child_process shell close without exit #998
Conversation
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
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughListen for both Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ShellSvc as ShellExecutionService
participant Child as ChildProcess
Note over ShellSvc,Child: One-time listeners + hasResolved guard ensure single resolution
Caller->>ShellSvc: run command → returns Promise
ShellSvc->>Child: attach child.once('exit', handleExit)
ShellSvc->>Child: attach child.once('close', handleExit)
alt Child emits 'exit' or 'close' first
Child-->>ShellSvc: 'exit'/'close'
ShellSvc->>ShellSvc: handleExit (if !hasResolved) → set hasResolved = true
ShellSvc->>ShellSvc: collect stdout/stderr, append truncation warning if truncated
ShellSvc-->>Caller: resolve Promise with ShellExecutionResult
else Duplicate event (second emit)
Child-->>ShellSvc: duplicate 'exit'/'close'
ShellSvc->>ShellSvc: handleExit sees hasResolved → no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
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 |
Contributor
|
I'll analyze this PR that addresses shell tool hanging when child_process emits close without exit. Let me first understand what changes were made. Looking at this PR, I need to focus on the shell execution fix that addresses issue #997 where the shell tool hangs when child_process emits Now I'll search for the specific test that verifies the new behavior (resolving on close event): |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/services/shellExecutionService.test.ts (1)
494-502: Excellent test coverage for the close-only scenario.This test directly validates the fix for issue #997 by simulating a child process that emits
closewithoutexit. The assertions correctly verify that:
- The promise resolves (doesn't hang)
- The exit code is captured correctly
- The stdout data is accumulated properly
Optional: Consider adding a test for non-zero exit code with close event
While the current test covers the primary scenario, you could add a complementary test case for non-zero exit codes:
it('should resolve when only the close event fires with non-zero exit code', async () => { const { result } = await simulateExecution('failing-command', (cp) => { cp.stderr?.emit('data', Buffer.from('error occurred')); cp.emit('close', 1, null); }); expect(result.exitCode).toBe(1); expect(result.stderr).toBe('error occurred'); });This would provide more comprehensive coverage, but the existing test is sufficient for validating the fix.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/services/shellExecutionService.test.tspackages/core/src/services/shellExecutionService.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/services/shellExecutionService.test.tspackages/core/src/services/shellExecutionService.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: E2E Test (macOS)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
🔇 Additional comments (3)
packages/core/src/services/shellExecutionService.ts (3)
266-267: LGTM! Well-placed guard flag.The
hasResolvedflag is correctly scoped within the promise to track whether the exit handler has already executed, preventing double-resolution when bothexitandcloseevents fire.
272-276: LGTM! Robust double-resolution guard.The check-and-set pattern ensures that the exit handling logic (cleanup, resolution) executes at most once, even if multiple lifecycle events fire. This is the correct fix for the reported hanging issue.
330-335: Correct fix for the close-without-exit hanging issue.The changes properly address the root cause:
- Using
once()provides automatic cleanup after the first event fires- Listening to both
exitandcloseensures resolution regardless of which event fires- The
hasResolvedguard safely handles the case where both events fire (the typical scenario)Code follows all guidelines: proper type specifications, no
anytypes, and no console logging.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/services/shellExecutionService.test.ts (2)
441-442: Bindingontoonceshadows EventEmitter's nativeoncebehavior.
EventEmitteralready provides a nativeoncemethod. By bindingontoonce, the listener won't be automatically removed after the first event emission (which is the purpose ofonce). This works for these tests because events are emitted once and the promise resolves immediately, but it's semantically inconsistent with howonceshould behave.Consider using the native
oncemethod instead, which is already available onEventEmitter:- mockChildProcess.once = mockChildProcess.on.bind(mockChildProcess); + // EventEmitter already provides `once` natively - no binding neededIf you explicitly need to verify that the production code handles missing
oncegracefully, create a separate test case for that scenario rather than altering the default mock behavior.
846-846: Same binding pattern as noted earlier.Same consideration applies here -
EventEmitternatively providesonce, so this binding is technically unnecessary unless testing a specific fallback scenario.packages/core/src/services/shellExecutionService.ts (1)
330-344: Theonceexistence check is defensive but always true for Node's ChildProcess.Node.js
ChildProcessinherits fromEventEmitter, which always has aoncemethod. The fallback toon(lines 337-344) will never execute in production. This check exists solely to accommodate test mocks.The implementation correctly listens for both
exitandcloseevents, ensuring resolution regardless of which event the child process emits first (or exclusively). Combined with thehasResolvedguard, this properly fixes issue #997.Consider adding a brief comment explaining the fallback is for test compatibility:
+ // Use once() to auto-remove listeners; fallback to on() for test mocks + // that may not implement once() if (child.once) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/services/shellExecutionService.test.tspackages/core/src/services/shellExecutionService.tspackages/core/src/services/shellExecutionService.windows.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/services/shellExecutionService.windows.test.tspackages/core/src/services/shellExecutionService.test.tspackages/core/src/services/shellExecutionService.ts
🧠 Learnings (2)
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: Shell tool (packages/core/src/tools/shell.ts) always applies tool-output-max-tokens via limitOutputTokens to the model-facing llmContent at the end of execute(), formatting a warning when truncated.
Applied to files:
packages/core/src/services/shellExecutionService.ts
📚 Learning: 2026-01-03T00:20:08.040Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-03T00:20:08.040Z
Learning: ShellExecutionService applies a hard cap only in the child_process path (appendAndTruncate, “16MB” message) and currently has no truncation in the PTY path, which can lead to unbounded memory before tool-level token limiting runs.
Applied to files:
packages/core/src/services/shellExecutionService.ts
🧬 Code graph analysis (1)
packages/core/src/services/shellExecutionService.ts (1)
scripts/start.js (1)
child(105-105)
🪛 GitHub Actions: LLxprt Code CI
packages/core/src/services/shellExecutionService.windows.test.ts
[error] 1-1: Prettier formatting issue detected by format step. Differences shown in diff; please run 'prettier --write' to fix code style in this file.
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
🔇 Additional comments (2)
packages/core/src/services/shellExecutionService.test.ts (1)
496-504: Good test coverage for the close-only completion scenario.This test directly validates the fix for issue #997, ensuring the shell execution resolves correctly when only the
closeevent fires without a precedingexitevent.packages/core/src/services/shellExecutionService.ts (1)
266-275: Double-resolution guard is correctly implemented.The
hasResolvedflag properly preventshandleExitfrom resolving the promise multiple times when bothexitandcloseevents fire (or whenerroralso triggershandleExit). This is the core fix for issue #997.
packages/core/src/services/shellExecutionService.windows.test.ts
Outdated
Show resolved
Hide resolved
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.
Summary
closewithoutexitTesting
Fixes #997
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.