Skip to content

[MeshSync] Unsubscribe exec input on teardown; drop leaked drain goroutine#591

Merged
leecalcote merged 2 commits into
masterfrom
fix/meshsync-exec-unsubscribe-teardown
Jul 6, 2026
Merged

[MeshSync] Unsubscribe exec input on teardown; drop leaked drain goroutine#591
leecalcote merged 2 commits into
masterfrom
fix/meshsync-exec-unsubscribe-teardown

Conversation

@leecalcote

@leecalcote leecalcote commented Jul 6, 2026

Copy link
Copy Markdown
Member

Description

Interactive kubectl exec sessions subscribed to a per-session input.<id> subject (client keystrokes) but could not tear that subscription down, because broker.Handler had no Unsubscribe. streamSession worked around the gap by parking a drain goroutine:

go func() {
    <-done
    for range subCh {}
}()

subCh is never closed, so for range subCh never returns: every exec session leaked a goroutine and a NATS subscription for the process lifetime.

Root cause / fix

  • Root cause - the interface gap. MeshKit broker.Handler had no way to release a single subscription short of CloseConnection().
  • Fix - MeshKit v1.0.22 (meshery/meshkit#1056) adds Unsubscribe(subject string) error. terminate() (already the single, sync.Once-guarded teardown path for stream EOF/error, explicit stop, and global stop) now calls Unsubscribe("input.<id>"), which releases the subscription and the broker's delivery goroutine. The drain goroutine is removed.
  • subCh is given a 1-slot buffer so a delivery already in flight at teardown lands in the buffer instead of blocking the broker's delivery goroutine in the narrow window before Unsubscribe takes effect.

What changed

  • go.mod: meshery/meshkit v1.0.20 -> v1.0.22 (schemas bumped transitively).
  • meshsync/exec.go: Unsubscribe on teardown via a new unsubscribeSessionInput helper (the testable seam - streamSession itself needs a live SPDY kube client); drain goroutine removed; subCh buffered; stale //nolint:cyclop TODO (hardcoded an out-of-date complexity number) replaced with a rationale.
  • meshsync/exec_test.go (new): teardown/goroutine-leak regression test and an error-path test, both under -race, using a recordingBroker that wraps a real ChannelBrokerHandler (records the Unsubscribe subject, delegates the actual teardown) plus a compile-time broker.Handler conformance assertion.
  • Docs: docs/agent-instructions/architecture.md gains an Interactive Sessions (exec / log stream) section; docs/design/fd4-jetstream-durable-delivery.md gets a status note - the base Unsubscribe it flagged as an open gap has shipped (as 1-arg Unsubscribe(subject string), not the (subject, queue) it speculated) and this exec leak is fixed; durable JetStream delivery remains the forward work there.

Relationship to #585

Fixes part 2 of #585 (the exec input-subscription / drain-goroutine leak). Part 1 (the channelPool concurrent-map data race) landed in #587. With this, #585 is fully addressed.

Testing

  • go test --short ./... -race - all packages pass.
  • New TestUnsubscribeSessionInputTearsDownSubscription / TestUnsubscribeSessionInputLogsErrorWithoutPanic pass under -race.
  • golangci-lint run --new-from-rev=origin/master ./... - 0 new issues.

Signed commits

  • Yes, I signed my commits.

Closes #585.

…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 #585 (part 1, the channelPool data race, landed in #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 <[email protected]>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the meshkit dependency to v1.0.22 to utilize the new Unsubscribe method on broker.Handler, resolving an exec subscription and goroutine leak during session teardown. Documentation and tests have been updated to reflect and verify this fix. Feedback on the changes highlights a potential nil pointer dereference panic in meshsync/exec.go if subCh is closed and read without checking the channel's open status. Additionally, it is recommended to replace the flaky runtime.NumGoroutine() leak detection in the new tests with a robust library like go.uber.org/goleak.

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.

Comment thread meshsync/exec.go Outdated
Comment thread meshsync/exec_test.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a lifecycle leak in MeshSync interactive kubectl exec sessions by ensuring the per-session stdin subscription (input.<id>) is explicitly torn down on session termination, leveraging the newly added broker.Handler.Unsubscribe(subject string) error in MeshKit v1.0.22.

Changes:

  • Bump MeshKit dependency to v1.0.22 and unsubscribe input.<id> on exec-session teardown (removing the previously leaked drain goroutine).
  • Add regression tests around teardown/unsubscribe behavior using a recording broker wrapper.
  • Update architecture/design docs to reflect the shipped Unsubscribe capability and the exec leak fix.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
meshsync/exec.go Unsubscribes exec stdin subject on teardown, removes drain goroutine, buffers input channel.
meshsync/exec_test.go Adds regression/error-path tests for unsubscribe teardown behavior.
go.mod Bumps github.com/meshery/meshkit to v1.0.22 (and schemas transitively).
go.sum Updates dependency checksums for MeshKit v1.0.22 and schemas v1.3.25.
docs/design/fd4-jetstream-durable-delivery.md Adds status note documenting that base Unsubscribe has shipped and the exec leak is fixed.
docs/agent-instructions/architecture.md Documents interactive session teardown behavior and session-map separation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread meshsync/exec.go Outdated
… test

Review follow-ups on #591:

- Main input loop reads subCh with the comma-ok form and guards the payload
  assertion, so a broker that closes its delivery channel (returns nil,false) or
  a malformed non-string ExecInput message ends/skips the session instead of
  panicking or spinning. (subCh is not closed by the current NATS/channel
  brokers, but the receive is now robust if one ever does.)
- Widen the input channel buffer from 1 to execInputChannelBuffer (256) so a
  burst of stdin - or a delivery already in flight at teardown - cannot
  backpressure the broker's delivery goroutine in the window before Unsubscribe
  stops delivery.
- Replace the flaky runtime.NumGoroutine leak assertion with go.uber.org/goleak
  (VerifyNone + IgnoreCurrent), which retries with backoff and ignores runtime
  background goroutines; verified it fails when teardown is skipped.

Signed-off-by: Lee Calcote <[email protected]>
@leecalcote
leecalcote merged commit 3e04e50 into master Jul 6, 2026
6 checks passed
@leecalcote
leecalcote deleted the fix/meshsync-exec-unsubscribe-teardown branch July 6, 2026 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

language/go Golang related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MeshSync exec: synchronize channelPool access and fix subscription/goroutine teardown

2 participants