[MeshSync] Fix channelPool data race: isolate exec/log-stream sessions behind a mutex - #587
Conversation
…lPool channelPool held both the fixed system channels (Stop/OS/ReSync) and dynamic per-request exec/log-stream session channels. Session goroutines mutated the map (add/delete) while other goroutines read it (system-channel selects, getActiveChannels) - a data race that can panic the process. Dynamic sessions now live in a dedicated mutex-guarded sessions map; channelPool is read-only after construction. The session helpers return the channel and release the lock before any channel send/receive, so the mutex is never held across a blocking channel op. Also fixes getActiveChannels, which previously ranged the whole pool and reported system-channel keys as active sessions. Addresses the channelPool race flagged on #573 (tracked in #585). The exec input-subscription teardown (the other half of #585) still needs a MeshKit broker Unsubscribe and is not covered here. Signed-off-by: marblom007 <158522975+marblom007@users.noreply.github.com> Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors session management for interactive exec and log-stream sessions in MeshSync, moving them from the shared channelPool map to a dedicated, mutex-guarded sessions map to prevent data races. It also adds unit tests to verify concurrent safety. The review feedback highlights critical issues in logstream.go, including a potential deadlock from a blocking channel send on stop requests, a goroutine leak when log streams terminate normally, and an infinite loop bug when handling stream read errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
… error loop) Three pre-existing issues in streamLogs, surfaced while moving sessions out of channelPool: - Stop requests now send to the session channel non-blockingly, so a duplicate stop or an already-closing stream cannot freeze the log-request loop. - The waiter goroutine now selects on a done channel closed when streamLogs exits, so it no longer leaks when the stream ends on its own (EOF/error). Session removal and stream close move to defers covering every exit path. - A non-EOF read error now breaks the loop (checked before the zero-byte continue), avoiding an infinite read/log loop on a failed stream. Addresses review feedback on #587. Signed-off-by: marblom007 <158522975+marblom007@users.noreply.github.com> Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
There was a problem hiding this comment.
Pull request overview
This PR refactors MeshSync’s exec/log-stream session channel handling to eliminate a channelPool concurrent map read/write race by moving dynamic per-session channels into a dedicated, mutex-guarded sessions map, leaving channelPool read-only after initialization.
Changes:
- Introduces
sessions+sessionsMuonHandlerand adds helper methods for add/get/delete/enumerate session channels. - Updates exec and log-stream session management to use the sessions helpers (and fixes
getActiveChannelsto report only active session IDs). - Adds a concurrency-focused unit test intended to pass under
-racefor overlapping add/read/enumerate/delete patterns.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
meshsync/sessions.go |
Adds mutex-guarded session map and helper APIs to prevent races with the fixed channelPool. |
meshsync/sessions_test.go |
Adds unit tests for idempotent session creation and concurrent access under -race. |
meshsync/meshsync.go |
Extends Handler with sessions + sessionsMu and initializes the sessions map in New(). |
meshsync/logstream.go |
Switches log streaming start/stop to sessions helpers and ensures cleanup on all exit paths. |
meshsync/exec.go |
Switches exec session lifecycle to sessions helpers and fixes active session enumeration to exclude system channels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Session stop signals are sent non-blocking from exec/log-stream, so an unbuffered session channel dropped a stop that arrived before the session goroutine started receiving, leaving the session running. Session channels are now 1-buffered so a single stop is reliably recorded. addSession also lazily initializes the sessions map so a Handler built outside New does not panic on the write. Adds regression tests. Addresses review feedback on #587. Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
|
@gemini /review |
There was a problem hiding this comment.
Code Review
This pull request refactors session management for interactive exec and log-stream sessions in MeshSync. It moves dynamic sessions out of the shared channelPool map into a dedicated, mutex-guarded sessions map to prevent concurrent map read/write data races. It also introduces thread-safe helper methods and corresponding unit tests. The review feedback suggests ensuring that log streaming sessions gracefully terminate on a global application stop by checking the global stop channel, which also requires retaining the channels package import.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The log-stream waiter goroutine now also selects on the global channels.Stop channel, so a long-running log stream is closed promptly on shutdown instead of blocking graceful termination until the stream ends on its own (matching the exec session loop). channelPool holds only the fixed system channels and is read-only after init, so the read needs no lock. Addresses review feedback on #587. Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
Move the waiter goroutine's select into awaitLogStreamStop so streamLogs stays under the cyclomatic-complexity limit (golangci-lint cyclop) after adding the global-stop case. No behavior change. Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
…utine
streamSession subscribed to input.<id> for a session's stdin but could not tear
that subscription down, because broker.Handler had no Unsubscribe. It worked
around that by parking a `<-done; for range subCh {}` drain goroutine - which
never exited (subCh is never closed) and never released the subscription,
leaking a goroutine and a NATS subscription per exec session for the process
lifetime.
MeshKit v1.0.22 (meshery/meshkit#1056) adds broker.Handler.Unsubscribe.
terminate() now calls Unsubscribe("input.<id>"), which releases the
subscription and the broker's delivery goroutine, and the drain goroutine is
removed. subCh gets a 1-slot buffer so a delivery already in flight at teardown
cannot block the broker's delivery goroutine in the window before Unsubscribe
takes effect.
Fixes part 2 of meshery#585 (part 1, the channelPool data race, landed in meshery#587).
- go.mod: meshkit v1.0.20 -> v1.0.22
- meshsync/exec.go: Unsubscribe on teardown via unsubscribeSessionInput; remove
drain goroutine; buffer subCh(1); replace stale cyclop TODO with rationale
- meshsync/exec_test.go: teardown/leak regression + error-path tests (-race)
- docs: architecture.md interactive-sessions section; fd4 status note (base
Unsubscribe shipped as 1-arg in v1.0.22, exec leak fixed)
Signed-off-by: Lee Calcote <lee.calcote@layer5.io>
Description
Fixes the
channelPooldata race flagged by review on #573 (tracked in #585). This is the concurrency refactor that #573 deliberately scoped out.channelPoolmixed the fixed system channels (Stop/OS/ReSync) with dynamic per-request exec and log-stream session channels. Session goroutines added/deleted session entries while other goroutines read the same map (system-channel selects,getActiveChannels) - a concurrent map read/write that can panic the process.sessions map[string]channels.StructChannelguarded bysessionsMu(newmeshsync/sessions.gohelpers);channelPoolis read-only after construction.exec.go) and log-stream (logstream.go) session management.getActiveChannels, which previously ranged the whole pool and reported the system-channel keys (stop/os/resync) as active exec sessions.Notes for Reviewers
masteronce [MeshSync] Fix resourceVersion comparison, dead list filter, and exec session leaks #573 merges.go build ./...,go vet ./meshsync/..., andgo test -race ./meshsync/... ./internal/pipeline/...pass, including a new 16-goroutine concurrency test over the sessions map.input.<id>subscription/drain teardown, which requires a MeshKitbroker.Handler.Unsubscribe.Signed commits