fix(paseo-omp): Better child process closing based on stdin.end(), with prevention for starting childprocess on Windows to kill process - #150
Conversation
…with prevention for starting childprocess on Windows to kill process
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved type-checking and shutdown behavior issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (6)
What changed in this PR
Updates OMP and MCP child-process shutdown to end stdin first, wait for closure, and fall back to process-tree cleanup after timeout.
Changes:
- Adds stdin-close timeout handling.
- Applies the shutdown flow to both transports.
- Reuses process cleanup types and timeout behavior.
| File | Summary |
|---|---|
paseo-omp/server/provider/omp-rpc-transport.ts |
Updates OMP shutdown sequencing; unresolved timer typing, cleanup, and regression issues remain. |
paseo-omp/server/provider/mcp-transport.ts |
Mirrors the shutdown strategy; unresolved timer typing, cleanup, notification, lint, and regression issues remain. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const treeCleanup = this.startTreeCleanup(); | ||
| if (!this.exited) { | ||
|
|
||
| if (this.exited) return; |
| } catch (error) { | ||
| if (error instanceof TimeOutError) { | ||
| console.warn(`MCP stdio process did not close in time (${PROCESS_STOP_TIMEOUT_MS}ms), starting tree cleanup`); | ||
| let terminated = await this.startTreeCleanup(); | ||
|
|
||
| if (!terminated) throw new Error("MCP stdio process tree cleanup failed"); | ||
| } | ||
| } |
| throw new Error("MCP stdio process did not close after tree cleanup"); | ||
| } |
| const cleanupPromise = this.startTreeCleanup(); | ||
| if (!this.exited) { | ||
|
|
||
| if (this.exited) return; |
|
Thanks for tracking down the Windows I took the fix in a narrower direction in #152: a real Windows leader-plus-descendant regression test first, followed by the smallest ordering change that keeps the Windows tree root alive until I’m going to close this PR as superseded by #152 to avoid landing two competing shutdown implementations. The combined regression and fix pass Thanks also for the shared base-class suggestion. The duplicated shutdown shape is worth monitoring, but the two transports currently differ in lifecycle and notification semantics; extracting a base class would broaden this race fix unnecessarily. |
This is a fix for the
passeo-ompplugin not working on Windows due to a race-condition with TaskKill, the same fix is also transposed on thecloseTransportmethod inmcp-transport.ts- which I noticed to have very similar code.Suggestion for the future: Create a base-class for childprocess work, to prevent duplicate work and make it easier to implement such fixes.