From 3615c01fe3b822445514ad299ce6f31fdf93f4bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= <zoltan.pmail@gmail.com>
Date: Thu, 27 Aug 2026 11:27:25 +0200
Subject: [PATCH] fix(client): keep the run's exit signal readable after
 supervisor Stop
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Stop cleared the done channel, so a waiter that raced a teardown got nil
from Done() and selected on a nil channel: an Up racing a Down hung in
waitForUp for its full 50s timeout where it previously failed fast with
"client gave up to connect". The same clearing made Alive() report a
timed-out run dead while its goroutine was still tearing down, letting
Up start an overlapping second run.

Keep the channel in place instead: the exiting run closes it, so late
waiters observe the real exit and Alive() stays true until the run is
actually gone — the lifecycle clientGiveUpChan used to have. Stop still
drops the current client and invalidates the generation.

The stale-running-chan test begins a fresh run before waiting, as Up
does, so the kept exit signal does not race the stale clientRunningChan.
---
 client/internal/run_supervisor.go      |  7 ++++---
 client/internal/run_supervisor_test.go | 17 +++++++++++++++++
 client/server/server_connect_test.go   |  5 +++++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/client/internal/run_supervisor.go b/client/internal/run_supervisor.go
index 4a4c51b7e..43131e1fb 100644
--- a/client/internal/run_supervisor.go
+++ b/client/internal/run_supervisor.go
@@ -72,8 +72,10 @@ func (s *RunSupervisor) Current() *ConnectClient {
 	return s.current
 }
 
-// Done returns the channel the current run closes when it exits, or nil when no
-// run has been started. Callers that only need a yes/no answer should use Alive.
+// Done returns the channel the most recent run closes when it exits, or nil
+// when no run has been started. It stays readable after Stop so a waiter that
+// raced a teardown still observes the run's exit instead of a nil channel.
+// Callers that only need a yes/no answer should use Alive.
 func (s *RunSupervisor) Done() <-chan struct{} {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -110,7 +112,6 @@ func (s *RunSupervisor) Stop(ctx context.Context) error {
 	cc := s.current
 	done := s.done
 	s.current = nil
-	s.done = nil
 	s.mu.Unlock()
 
 	if cc != nil {
diff --git a/client/internal/run_supervisor_test.go b/client/internal/run_supervisor_test.go
index c4f5c9a24..85593157b 100644
--- a/client/internal/run_supervisor_test.go
+++ b/client/internal/run_supervisor_test.go
@@ -96,6 +96,23 @@ func TestRunSupervisorStopGivesUpWaitOnContext(t *testing.T) {
 	assert.ErrorIs(t, s.Stop(ctx), context.DeadlineExceeded)
 }
 
+func TestRunSupervisorDoneSurvivesStop(t *testing.T) {
+	var s RunSupervisor
+
+	_, done := s.Begin()
+	close(done)
+
+	require.NoError(t, s.Stop(context.Background()))
+
+	runDone := s.Done()
+	require.NotNil(t, runDone, "a waiter that raced Stop must still observe the run's exit")
+	select {
+	case <-runDone:
+	default:
+		t.Fatal("the last run's exit signal should remain readable after Stop")
+	}
+}
+
 func TestRunSupervisorAliveTracksRun(t *testing.T) {
 	var s RunSupervisor
 
diff --git a/client/server/server_connect_test.go b/client/server/server_connect_test.go
index ba320ccbc..541c11af8 100644
--- a/client/server/server_connect_test.go
+++ b/client/server/server_connect_test.go
@@ -190,6 +190,11 @@ func TestDownThenUp_StaleRunningChan(t *testing.T) {
 	assert.False(t, s.clientRunning, "clientRunning should be cleared by cleanupConnection (intent = down)")
 	s.mutex.Unlock()
 
+	// A fresh Up begins a new run before waiting; without it the previous
+	// run's kept exit signal would race the stale clientRunningChan below.
+	_, nextDone := s.runs.Begin()
+	defer close(nextDone)
+
 	// waitForUp() returns immediately due to stale closed clientRunningChan
 	waitCtx, ctxCancel := context.WithTimeout(context.Background(), 2*time.Second)
 	defer ctxCancel()