Go SDK: CreateSession failure leaks the pre-registered session's processEvents goroutine (non-cloud path)

[timreimherr]

Created with AI assistance - Human reviewed.

Summary

On the non-cloud session path, Client.CreateSession leaks the session's processEvents goroutine whenever the session.create RPC (or its response handling) fails. The pre-registered session is removed from the client's session map on the error path without closing its event channel, so neither Client.Stop() nor a caller's own teardown can ever reap the goroutine. Each failed CreateSession call (and each retry) permanently leaks one goroutine plus its Session (which holds a 128-entry buffered channel) for the lifetime of the process.

Verified in v1.0.6 and still present in v1.0.9 (latest stable) — the relevant code is identical.

Root cause (Go SDK, v1.0.9 line numbers)

1. newSession eagerly starts the consumer goroutine: go s.processEvents() (go/session.go:389). processEvents runs for event := range s.eventCh and only returns once eventCh is closed.
2. eventCh is closed only by Session.Disconnect() → s.closeOnce.Do(func() { close(s.eventCh) }) (go/session.go:1724).
3. For non-cloud sessions, CreateSession generates a client-side localSessionID and pre-registers the session in c.sessions before issuing the RPC (initializeSession at go/client.go:915, invoked ~go/client.go:990) so that session-scoped requests emitted during session.create processing can be routed.
4. Every CreateSession error path after pre-registration then does only delete(c.sessions, registeredSessionID) and returns — it never calls Disconnect() / closes eventCh:
   • RPC failure: go/client.go:1029-1034
   • response unmarshal failure: go/client.go:1039-1044
   • sessionId mismatch: go/client.go:1051-1055
   • (the SessionFS validation paths inside initializeSession, go/client.go:964 and :972, have the same shape)
5. Client.Stop() disconnects only sessions still present in c.sessions. Because step 4 already removed the session from the map, Stop() cannot see it, and the goroutine blocks forever on eventCh.

Equivalent locations in v1.0.6: session.go:376, session.go:1662, client.go:920-946, client.go:436-445.

Impact

A long-running client that constructs a client/session per request accumulates one orphaned goroutine + Session per failed CreateSession. During a model/config or runtime outage — exactly when session.create fails and callers retry — goroutine and memory usage grow without bound until the process restarts. Successful sessions are unaffected (they stay in the map and are disconnected by Stop()).

The leaked resource is the in-process goroutine/Session; the CLI child process is still reaped by Stop(). So this is distinct from the child-process leaks in #1804 / #1381, and from the "disconnected sessions linger in the map" case that the (closed, unmerged) PR #1130 targeted.

Reproduction

1. Build a Client over stdio (non-cloud).
2. Cause session.create to fail (e.g. point at an unreachable/invalid model or induce a runtime error) so CreateSession returns an error.
3. Call Client.Stop().
4. Observe via runtime.NumGoroutine() / a goroutine dump that a processEvents goroutine remains parked on chan receive (s.eventCh). Repeat N times → N parked goroutines.

Suggested fix

On every CreateSession failure that occurs after the session has been pre-registered, close/disconnect the pre-registered session before returning (e.g. call session.Disconnect(), or at minimum s.closeOnce.Do(func() { close(s.eventCh) })) in addition to the existing delete(c.sessions, …). A defer-based cleanup that runs on any non-nil error return would cover all of the paths above (including the SessionFS ones) uniformly. PR #1130's OnDisposed wiring is a related, more general approach but was closed unmerged.

Environment

• SDK: github.com/github/copilot-sdk/go v1.0.6 (repro confirmed in source through v1.0.9)
• Language: Go
• Transport: stdio (non-cloud / local session)

[github-actions]

Investigation findings

Verdict: Confirmed bug — the goroutine leak is real and the root cause matches the report exactly.

What I investigated

• go/session.go lines 380–391: newSession always starts go s.processEvents() eagerly, which blocks on for event := range s.eventCh
• go/session.go lines 1718–1724: Disconnect() is the only place that calls s.closeOnce.Do(func() { close(s.eventCh) })
• go/client.go lines 920–984: initializeSession — the two early-return error paths (lines 968–971, 976–980) call delete(c.sessions, sessionID) but never close eventCh
• go/client.go lines 1034–1039: RPC failure path — delete(c.sessions, registeredSessionID), no eventCh close
• go/client.go lines 1044–1049: unmarshal failure path — same issue
• go/client.go lines 1057–1060: sessionID mismatch path — same issue

What I found

Every error path that fires after newSession is called (which starts the goroutine) removes the session from c.sessions but does not call session.Disconnect() or otherwise close eventCh. Since processEvents only exits when eventCh is closed, and Stop() only disconnects sessions still in the map, the goroutine is permanently orphaned.

This affects all non-cloud CreateSession failures — exactly the retry scenario the issue describes. Each failed attempt leaks one goroutine plus a 128-entry buffered channel for the lifetime of the process.

Suggested fix (as noted in the issue)

A defer-based cleanup is the cleanest approach: after the session is pre-registered, install a defer that calls s.closeOnce.Do(func() { close(s.eventCh) }) (or a dedicated internal teardown helper) whenever the function returns an error. This covers all error paths uniformly including the SessionFS validation ones inside initializeSession.

Generated by Bug Handler for #2320 · sonnet46 21.8 AIC · ⌖ 5.21 AIC · ⊞ 5.9K · ◷