Skip to content

fix: close in-flight connects on session_shutdown - #3

Open
levitte wants to merge 2 commits into
porameht:mainfrom
levitte:fix/session-shutdown-in-flight-connect
Open

fix: close in-flight connects on session_shutdown#3
levitte wants to merge 2 commits into
porameht:mainfrom
levitte:fix/session-shutdown-in-flight-connect

Conversation

@levitte

@levitte levitte commented Aug 8, 2026

Copy link
Copy Markdown

Stacked on #1 — only the top commit is new here; this PR's diff shrinks to that commit once #1 merges.

#1's session_shutdown handler misses one race: if shutdown lands while connect() is still in flight, client is still undefined, so the handler no-ops and the in-flight spawn completes anyway — its stdio pipes then hold the event loop open just the same, and pi -p still hangs.

Fix: record the shutdown and close the freshly connected client instead of adopting it.

Verified that normal shutdown still exits cleanly: pi -p returns in ~5 s with the extension loaded (pi 0.84.0).

anan and others added 2 commits August 1, 2026 21:03
The extension spawns the codebase-memory-mcp child process at load time
but never closes it. The child's stdio pipes keep the parent event loop
alive, so pi in non-interactive print mode (-p / --mode json) hangs
forever after answering instead of exiting.

Register an idempotent session_shutdown handler (the documented hook for
session-scoped resources) that closes the MCP client, letting the event
loop drain and the process exit normally.

Repro: pi -p "ping" with this extension loaded never exits.
After: exits cleanly; interactive sessions are unaffected because
session_shutdown only fires at session end/switch, not per turn.
If shutdown lands while connect() is still pending, client is
undefined and the handler no-ops; the spawn then completes into
a leak that holds the event loop open just the same.

Assisted-by: Pi:moonshot/kimi-k3
@levitte

levitte commented Aug 8, 2026

Copy link
Copy Markdown
Author

My agent initially discovered the exact issue that PR #1 solves, and did propose exactly that fix before discovering that PR #1 already existed.
(if you need a vouch for #1, @porameht, this is it 😉)

This PR merely completes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant