Skip to content

🪲 BUG-#1: Fix RpcClient pending requests leak - #13

Merged
FernandoCelmer merged 4 commits into
masterfrom
feature/1
Aug 15, 2026
Merged

🪲 BUG-#1: Fix RpcClient pending requests leak#13
FernandoCelmer merged 4 commits into
masterfrom
feature/1

Conversation

@FernandoCelmer

@FernandoCelmer FernandoCelmer commented Aug 15, 2026

Copy link
Copy Markdown
Member

Description

  • src/core-client/core-client.ts: Extracted ProcessHandle interface from CoreProcess to decouple RpcClient from the concrete process implementation. Added rejectPending() private method that settles all in-flight requests with a JSON-RPC -32000 error. Hooked it into exit and spawnError event handlers, and replaced the silent pending.clear() in dispose().
  • test/core-client.test.ts: Added 4 unit tests using a FakeProcess (lightweight EventEmitter stub) covering: process exit mid-flight, spawn failure, dispose, and normal response resolution.

Motivation and Context

Closes #1

RpcClient.request() stored promise resolvers in a pending map and only resolved them when a matching JSON-RPC response arrived. If the child process exited or failed to spawn, those promises hung forever — callers (sendPrompt, askAside, etc.) would block indefinitely with no way to recover.

Types of changes

  • Bug fix (change that fixes an issue)
  • New feature (change which adds functionality)
  • Documentation

Checklist

  • I have performed a self-review of my own code
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the CHANGELOG
  • I have updated the documentation accordingly

@FernandoCelmer

Copy link
Copy Markdown
Member Author

Follow-up commit: the original tests spawned a real node -e ... child process to exercise RpcClient, and that made npm test hang indefinitely — a real child process handle keeps Node's own event loop alive until the process actually terminates, independent of whether the JS-level promise already resolved. Refactored RpcClient to accept an optional ProcessHandle (defaults to a real CoreProcess, unchanged behavior in production) so tests can inject a plain EventEmitter-based fake instead. npm test now finishes in ~330ms.

@FernandoCelmer FernandoCelmer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Code Review

Overall this is a solid, well-scoped fix with good test coverage. The ProcessHandle interface abstraction is a clean design that makes the code properly testable without spawning real processes. Two follow-up suggestions below around edge cases in rejectPending.

Code issues found: 2

# Severity Comment
1 [Suggestion] pending.clear() after resolve() risks dropping re-entrant requests
2 [Suggestion] post-exit request() calls hang forever — no dead guard

Comment thread src/core-client/core-client.ts
Comment thread src/core-client/core-client.ts
@FernandoCelmer FernandoCelmer added the bug Something isn't working label Aug 15, 2026
@FernandoCelmer
FernandoCelmer merged commit 4cf6738 into master Aug 15, 2026
2 checks passed
@FernandoCelmer
FernandoCelmer deleted the feature/1 branch August 15, 2026 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RpcClient pending requests leak when process exits mid-flight

1 participant