fix(daemon): synchronize floating pane ownership

commit: 5fdd7fed1dab406f342e6f9131739b7863db9bfe
author: brice · 3mo ago · 2026-07-10 05:39 UTC
committer: brice · 3mo ago · 2026-07-10 05:40 UTC
parent: c376548
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

3 files · +105 -21

M internal/usecase/daemon/floating.go +27 -15


@@ -101,12 +101,23 @@ func (tb tab) clearFloatingLocked(p pane, generation uint64) bool {
101	101		(tb.floating.state != floatingHidden && tb.floating.state != floatingVisible) {
102	102		return false
103	103		}
	104	+	tb.takeFloatingLocked()
	105	+	return true
	106	+	}
	107	+
	108	+	// takeFloatingLocked invalidates the slot and detaches its installed pane. The
	109	+	// caller must hold tb.mu and must close the returned pane after unlocking.
	110	+	func (tb tab) takeFloatingLocked() pane {
	111	+	if tb == nil {
	112	+	return nil
	113	+	}
	114	+	p := tb.floating.pane
104	115		tb.floating.generation++
105	116		tb.floating.state = floatingUninitialized
106	117		tb.floating.pane = nil
107	118		tb.floating.desiredVisible = false
108	119		tb.floating.launch = domain.FloatingConfig{}
109		-	return true
	120	+	return p
110	121		}
111	122
112	123		// terminalTargetLocked chooses the visible floating terminal ahead of the

@@ -220,7 +231,10 @@ func (d Daemon) launchFloating(sess session, tb *tab, cfg domain.FloatingConfi
220	231		args = []string{"-lc", cfg.Command}
221	232		}
222	233		fallback := floatingCommandFallback(cfg.Command, d.shell)
223		-	go func() {
	234	+	// Count the launch before its goroutine starts. It remains counted through
	235	+	// install, including the reader/scheduler Adds, so shutdown cannot return
	236	+	// before a late Open completion has either been rejected or fully joined.
	237	+	d.sessWg.Go(func() {
224	238		pty, openErr := d.ptys.Open(command, args, env, cwd, sz)
225	239		if openErr != nil {
226	240		d.failFloatingLaunch(tb, generation, userOpen, name, openErr)

@@ -228,15 +242,19 @@ func (d Daemon) launchFloating(sess session, tb *tab, cfg domain.FloatingConfi
228	242		}
229	243		p := newPaneWithStableID(layout.PaneID("floating"), paneStableID, pty, sz)
230	244		p.title.displayFallback = fallback
231		-	if tabCtx == nil {
232		-	tabCtx = sess.ctx
	245	+	paneCtx := tabCtx
	246	+	if paneCtx == nil {
	247	+	paneCtx = sess.ctx
233	248		}
234		-	if tabCtx == nil {
235		-	tabCtx = context.Background()
	249	+	if paneCtx == nil {
	250	+	paneCtx = context.Background()
236	251		}
237		-	p.ctx, p.cancel = context.WithCancel(tabCtx)
	252	+	p.ctx, p.cancel = context.WithCancel(paneCtx)
	253	+	// The reader may run as soon as install returns; make its exit policy
	254	+	// immutable before publishing the pane to the slot.
	255	+	p.onExit = func() { d.reapFloating(sess, tb, p, generation) }
238	256		d.installFloating(sess, tb, p, generation)
239		-	}()
	257	+	})
240	258		}
241	259
242	260		func (d Daemon) failFloatingLaunch(tb tab, generation uint64, userOpen bool, sessionName string, err error) {

@@ -263,7 +281,6 @@ func (d Daemon) installFloating(sess session, tb tab, p pane, generation uin
263	281		closeFloatingPane(p)
264	282		return
265	283		}
266		-	p.onExit = func() { d.reapFloating(sess, tb, p, generation) }
267	284		d.startPaneGoroutines(sess, tb, p)
268	285		if visible {
269	286		sess.mu.Lock()

@@ -302,12 +319,7 @@ func (d Daemon) teardownFloating(tb tab) {
302	319		return
303	320		}
304	321		tb.mu.Lock()
305		-	p := tb.floating.pane
306		-	tb.floating.generation++
307		-	tb.floating.state = floatingUninitialized
308		-	tb.floating.pane = nil
309		-	tb.floating.desiredVisible = false
310		-	tb.floating.launch = domain.FloatingConfig{}
	322	+	p := tb.takeFloatingLocked()
311	323		tb.mu.Unlock()
312	324		closeFloatingPane(p)
313	325		}

 		(tb.floating.state != floatingHidden && tb.floating.state != floatingVisible) {
 		return false
 	}
+	tb.takeFloatingLocked()
+	return true
+}
+// takeFloatingLocked invalidates the slot and detaches its installed pane. The
+// caller must hold tb.mu and must close the returned pane after unlocking.
+func (tb *tab) takeFloatingLocked() *pane {
+	if tb == nil {
+		return nil
+	}
+	p := tb.floating.pane
 	tb.floating.generation++
 	tb.floating.state = floatingUninitialized
 	tb.floating.pane = nil
 	tb.floating.desiredVisible = false
 	tb.floating.launch = domain.FloatingConfig{}
-	return true
+	return p
 }
 // terminalTargetLocked chooses the visible floating terminal ahead of the
 		args = []string{"-lc", cfg.Command}
 	}
 	fallback := floatingCommandFallback(cfg.Command, d.shell)
-	go func() {
+	// Count the launch before its goroutine starts. It remains counted through
+	// install, including the reader/scheduler Adds, so shutdown cannot return
+	// before a late Open completion has either been rejected or fully joined.
+	d.sessWg.Go(func() {
 		pty, openErr := d.ptys.Open(command, args, env, cwd, sz)
 		if openErr != nil {
 			d.failFloatingLaunch(tb, generation, userOpen, name, openErr)
 		}
 		p := newPaneWithStableID(layout.PaneID("floating"), paneStableID, pty, sz)
 		p.title.displayFallback = fallback
-		if tabCtx == nil {
+		paneCtx := tabCtx
-			tabCtx = sess.ctx
+		if paneCtx == nil {
+			paneCtx = sess.ctx
 		}
-		if tabCtx == nil {
+		if paneCtx == nil {
-			tabCtx = context.Background()
+			paneCtx = context.Background()
 		}
-		p.ctx, p.cancel = context.WithCancel(tabCtx)
+		p.ctx, p.cancel = context.WithCancel(paneCtx)
+		// The reader may run as soon as install returns; make its exit policy
+		// immutable before publishing the pane to the slot.
+		p.onExit = func() { d.reapFloating(sess, tb, p, generation) }
 		d.installFloating(sess, tb, p, generation)
-	}()
+	})
 }
 func (d *Daemon) failFloatingLaunch(tb *tab, generation uint64, userOpen bool, sessionName string, err error) {
 		closeFloatingPane(p)
 		return
 	}
-	p.onExit = func() { d.reapFloating(sess, tb, p, generation) }
 	d.startPaneGoroutines(sess, tb, p)
 	if visible {
 		sess.mu.Lock()
 		return
 	}
 	tb.mu.Lock()
-	p := tb.floating.pane
+	p := tb.takeFloatingLocked()
-	tb.floating.generation++
-	tb.floating.state = floatingUninitialized
-	tb.floating.pane = nil
-	tb.floating.desiredVisible = false
-	tb.floating.launch = domain.FloatingConfig{}
 	tb.mu.Unlock()
 	closeFloatingPane(p)
 }

M internal/usecase/daemon/floating_test.go +77 -0


@@ -160,6 +160,80 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi
160	160		require.Equal(t, domain.Size{Cols: 48, Rows: 18}, gotSize)
161	161		d.teardownFloating(tb)
162	162		unblock()
	163	+	d.sessWg.Wait()
	164	+	}
	165	+
	166	+	func TestFloatingLaunchOwnershipJoinsShutdown(t *testing.T) {
	167	+	floatingPTY := portsmocks.NewMockPTY(t)
	168	+	readerStarted := make(chan struct{})
	169	+	readerRelease := make(chan struct{})
	170	+	var releaseOnce sync.Once
	171	+	releaseReader := func() { releaseOnce.Do(func() { close(readerRelease) }) }
	172	+	var closeCount int
	173	+	floatingPTY.EXPECT().Read(mock.Anything).RunAndReturn(func([]byte) (int, error) {
	174	+	close(readerStarted)
	175	+	<-readerRelease
	176	+	return 0, io.EOF
	177	+	}).Once()
	178	+	floatingPTY.EXPECT().Close().RunAndReturn(func() error {
	179	+	closeCount++
	180	+	releaseReader()
	181	+	return nil
	182	+	}).Once()
	183	+	factory := portsmocks.NewMockPTYFactory(t)
	184	+	opened := make(chan struct{})
	185	+	releaseOpen := make(chan struct{})
	186	+	factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn(
	187	+	func(string, []string, []string, string, domain.Size) (ports.PTY, error) {
	188	+	close(opened)
	189	+	<-releaseOpen
	190	+	return floatingPTY, nil
	191	+	}).Once()
	192	+	d := newTestDaemon(t, factory, stubClock{})
	193	+	tb := newFloatingTestTab(t)
	194	+	sessCtx, cancel := context.WithCancel(t.Context())
	195	+	sess := &session{id: "floating-ownership", name: "work", tabs: []*tab{tb}, ctx: sessCtx, cancel: cancel}
	196	+	d.mu.Lock()
	197	+	d.sessions[sess.id] = sess
	198	+	d.mu.Unlock()
	199	+
	200	+	// Keep a known worker alive while the waiter starts. Releasing it after
	201	+	// Open is blocked deterministically exposes an uncounted launch worker.
	202	+	d.sessWg.Add(1)
	203	+	joined := waitGroupDone(&d.sessWg)
	204	+	d.startFloating(sess, tb, true)
	205	+	select {
	206	+	case <-opened:
	207	+	case <-time.After(time.Second):
	208	+	t.Fatal("floating Open did not start")
	209	+	}
	210	+	d.sessWg.Done()
	211	+	// Waiting while Open is blocked must retain the launch worker. Otherwise
	212	+	// daemon shutdown can finish and a late install can Add reader goroutines.
	213	+	launchUnaccounted := false
	214	+	select {
	215	+	case <-joined:
	216	+	launchUnaccounted = true
	217	+	case <-time.After(time.Second):
	218	+	}
	219	+	close(releaseOpen)
	220	+	select {
	221	+	case <-readerStarted:
	222	+	case <-time.After(time.Second):
	223	+	t.Fatal("floating reader did not start")
	224	+	}
	225	+	require.NoError(t, d.killSession(sess, ports.ReasonSessionKilled, false))
	226	+	select {
	227	+	case <-waitGroupDone(&d.sessWg):
	228	+	case <-time.After(time.Second):
	229	+	t.Fatal("floating launch, reader, and scheduler did not join")
	230	+	}
	231	+	tb.mu.Lock()
	232	+	require.Nil(t, tb.floating.pane, "shutdown leaked an installed floating pane")
	233	+	require.Equal(t, floatingUninitialized, tb.floating.state)
	234	+	tb.mu.Unlock()
	235	+	require.False(t, launchUnaccounted, "blocked floating launch was not accounted by sessWg")
	236	+	require.Equal(t, 1, closeCount, "floating PTY must close exactly once")
163	237		}
164	238
165	239		func TestFloatingLifecycleStaleSuccessAndOldExitCannotReplaceCurrentSlot(t *testing.T) {

@@ -187,6 +261,7 @@ func TestFloatingLifecycleStaleSuccessAndOldExitCannotReplaceCurrentSlot(t *test
187	261		case <-time.After(time.Second):
188	262		t.Fatal("stale completion did not close its PTY")
189	263		}
	264	+	d.sessWg.Wait()
190	265
191	266		first := &pane{}
192	267		second := &pane{}

@@ -389,6 +464,7 @@ func TestFloatingLaunchUsesLiveOrValidatedSessionCwd(t *testing.T) {
389	464		case <-time.After(time.Second):
390	465		t.Fatal("floating PTY was not opened")
391	466		}
	467	+	d.sessWg.Wait()
392	468		})
393	469		}
394	470		}

@@ -445,6 +521,7 @@ func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testi
445	521		}
446	522		close(stopRename)
447	523		<-renameDone
	524	+	d.sessWg.Wait()
448	525		require.True(t, strings.Contains(logs.String(), "session=captured"), logs.String())
449	526		}
450	527


@@ -160,6 +160,80 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi
160	require.Equal(t, domain.Size{Cols: 48, Rows: 18}, gotSize)	160		require.Equal(t, domain.Size{Cols: 48, Rows: 18}, gotSize)
161	d.teardownFloating(tb)	161		d.teardownFloating(tb)
162	unblock()	162		unblock()
		163	+	d.sessWg.Wait()
		164	+	}
		165	+
		166	+	func TestFloatingLaunchOwnershipJoinsShutdown(t *testing.T) {
		167	+	floatingPTY := portsmocks.NewMockPTY(t)
		168	+	readerStarted := make(chan struct{})
		169	+	readerRelease := make(chan struct{})
		170	+	var releaseOnce sync.Once
		171	+	releaseReader := func() { releaseOnce.Do(func() { close(readerRelease) }) }
		172	+	var closeCount int
		173	+	floatingPTY.EXPECT().Read(mock.Anything).RunAndReturn(func([]byte) (int, error) {
		174	+	close(readerStarted)
		175	+	<-readerRelease
		176	+	return 0, io.EOF
		177	+	}).Once()
		178	+	floatingPTY.EXPECT().Close().RunAndReturn(func() error {
		179	+	closeCount++
		180	+	releaseReader()
		181	+	return nil
		182	+	}).Once()
		183	+	factory := portsmocks.NewMockPTYFactory(t)
		184	+	opened := make(chan struct{})
		185	+	releaseOpen := make(chan struct{})
		186	+	factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn(
		187	+	func(string, []string, []string, string, domain.Size) (ports.PTY, error) {
		188	+	close(opened)
		189	+	<-releaseOpen
		190	+	return floatingPTY, nil
		191	+	}).Once()
		192	+	d := newTestDaemon(t, factory, stubClock{})
		193	+	tb := newFloatingTestTab(t)
		194	+	sessCtx, cancel := context.WithCancel(t.Context())
		195	+	sess := &session{id: "floating-ownership", name: "work", tabs: []*tab{tb}, ctx: sessCtx, cancel: cancel}
		196	+	d.mu.Lock()
		197	+	d.sessions[sess.id] = sess
		198	+	d.mu.Unlock()
		199	+
		200	+	// Keep a known worker alive while the waiter starts. Releasing it after
		201	+	// Open is blocked deterministically exposes an uncounted launch worker.
		202	+	d.sessWg.Add(1)
		203	+	joined := waitGroupDone(&d.sessWg)
		204	+	d.startFloating(sess, tb, true)
		205	+	select {
		206	+	case <-opened:
		207	+	case <-time.After(time.Second):
		208	+	t.Fatal("floating Open did not start")
		209	+	}
		210	+	d.sessWg.Done()
		211	+	// Waiting while Open is blocked must retain the launch worker. Otherwise
		212	+	// daemon shutdown can finish and a late install can Add reader goroutines.
		213	+	launchUnaccounted := false
		214	+	select {
		215	+	case <-joined:
		216	+	launchUnaccounted = true
		217	+	case <-time.After(time.Second):
		218	+	}
		219	+	close(releaseOpen)
		220	+	select {
		221	+	case <-readerStarted:
		222	+	case <-time.After(time.Second):
		223	+	t.Fatal("floating reader did not start")
		224	+	}
		225	+	require.NoError(t, d.killSession(sess, ports.ReasonSessionKilled, false))
		226	+	select {
		227	+	case <-waitGroupDone(&d.sessWg):
		228	+	case <-time.After(time.Second):
		229	+	t.Fatal("floating launch, reader, and scheduler did not join")
		230	+	}
		231	+	tb.mu.Lock()
		232	+	require.Nil(t, tb.floating.pane, "shutdown leaked an installed floating pane")
		233	+	require.Equal(t, floatingUninitialized, tb.floating.state)
		234	+	tb.mu.Unlock()
		235	+	require.False(t, launchUnaccounted, "blocked floating launch was not accounted by sessWg")
		236	+	require.Equal(t, 1, closeCount, "floating PTY must close exactly once")
163	}	237		}
164		238
165	func TestFloatingLifecycleStaleSuccessAndOldExitCannotReplaceCurrentSlot(t *testing.T) {	239		func TestFloatingLifecycleStaleSuccessAndOldExitCannotReplaceCurrentSlot(t *testing.T) {

@@ -187,6 +261,7 @@ func TestFloatingLifecycleStaleSuccessAndOldExitCannotReplaceCurrentSlot(t *test
187	case <-time.After(time.Second):	261		case <-time.After(time.Second):
188	t.Fatal("stale completion did not close its PTY")	262		t.Fatal("stale completion did not close its PTY")
189	}	263		}
		264	+	d.sessWg.Wait()
190		265
191	first := &pane{}	266		first := &pane{}
192	second := &pane{}	267		second := &pane{}

@@ -389,6 +464,7 @@ func TestFloatingLaunchUsesLiveOrValidatedSessionCwd(t *testing.T) {
389	case <-time.After(time.Second):	464		case <-time.After(time.Second):
390	t.Fatal("floating PTY was not opened")	465		t.Fatal("floating PTY was not opened")
391	}	466		}
		467	+	d.sessWg.Wait()
392	})	468		})
393	}	469		}
394	}	470		}

@@ -445,6 +521,7 @@ func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testi
445	}	521		}
446	close(stopRename)	522		close(stopRename)
447	<-renameDone	523		<-renameDone
		524	+	d.sessWg.Wait()
448	require.True(t, strings.Contains(logs.String(), "session=captured"), logs.String())	525		require.True(t, strings.Contains(logs.String(), "session=captured"), logs.String())
449	}	526		}
450		527

M internal/usecase/daemon/session.go +1 -6


@@ -373,14 +373,9 @@ func (tb tab) panesSnapshot() []pane {
373	373		func (tb *tab) closeAllPanes() {
374	374		tb.mu.Lock()
375	375		panes := tb.panesSnapshot()
376		-	floating := tb.floating.pane
377	376		// Invalidate before cancellation so a concurrently unblocked floating
378	377		// reader cannot reap a subsequently reused slot.
379		-	tb.floating.generation++
380		-	tb.floating.state = floatingUninitialized
381		-	tb.floating.pane = nil
382		-	tb.floating.desiredVisible = false
383		-	tb.floating.launch = domain.FloatingConfig{}
	378	+	floating := tb.takeFloatingLocked()
384	379		tb.mu.Unlock()
385	380		if floating != nil {
386	381		panes = append(panes, floating)

 func (tb *tab) closeAllPanes() {
 	tb.mu.Lock()
 	panes := tb.panesSnapshot()
-	floating := tb.floating.pane
 	// Invalidate before cancellation so a concurrently unblocked floating
 	// reader cannot reap a subsequently reused slot.
-	tb.floating.generation++
+	floating := tb.takeFloatingLocked()
-	tb.floating.state = floatingUninitialized
-	tb.floating.pane = nil
-	tb.floating.desiredVisible = false
-	tb.floating.launch = domain.FloatingConfig{}
 	tb.mu.Unlock()
 	if floating != nil {
 		panes = append(panes, floating)