fix(floating): retain PTY context after launch

commit: bb3b15df4b520aeb414b1aab638c809dca818181
author: brice · 3mo ago · 2026-07-13 10:01 UTC
parent: 0a42b0f
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +88 -8

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


@@ -300,7 +300,7 @@ func (d Daemon) launchFloating(sess session, tb *tab, cfg domain.FloatingConfi
300	300		// before a late Open completion has either been rejected or fully joined.
301	301		d.sessWg.Go(func() {
302	302		defer sess.finishFloatingLaunch(launch)
303		-	d.openAndInstallFloating(sess, tb, spec, generation, launch)
	303	+	d.openAndInstallFloating(sess, tb, spec, generation)
304	304		})
305	305		}
306	306

@@ -357,7 +357,7 @@ func (d Daemon) newFloatingLaunchSpec(sess session, tb *tab, cfg domain.Floati
357	357
358	358		// openAndInstallFloating runs entirely in the launch worker. No PTY operation
359	359		// occurs under tb.mu; the pane is fully initialized before publication.
360		-	func (d Daemon) openAndInstallFloating(sess session, tb tab, spec floatingLaunchSpec, generation uint64, launch floatingLaunch) {
	360	+	func (d Daemon) openAndInstallFloating(sess session, tb *tab, spec floatingLaunchSpec, generation uint64) {
361	361		// Open is context-aware and is intentionally called without any daemon,
362	362		// session, tab, or launch-ownership lock. Cancellation makes teardown
363	363		// bounded even if the adapter is waiting to create the child.

@@ -366,9 +366,12 @@ func (d Daemon) openAndInstallFloating(sess session, tb *tab, spec floatingLau
366	366		}
367	367		openCtx, cancelOpen := context.WithCancel(spec.parentCtx)
368	368		stopSessionCancel := context.AfterFunc(sess.ctx, cancelOpen)
	369	+	ownedByPane := false
369	370		defer func() {
370		-	stopSessionCancel()
371		-	cancelOpen()
	371	+	if !ownedByPane {
	372	+	stopSessionCancel()
	373	+	cancelOpen()
	374	+	}
372	375		}()
373	376		pty, err := d.ptys.Open(openCtx, spec.command, spec.args, spec.env, spec.cwd, spec.size)
374	377		if err != nil {

@@ -379,7 +382,14 @@ func (d Daemon) openAndInstallFloating(sess session, tb *tab, spec floatingLau
379	382		p.rect = spec.geometry.Inner
380	383		p.popupGeometry = spec.geometry
381	384		p.title.displayFallback = spec.fallback
382		-	p.ctx, p.cancel = context.WithCancel(spec.parentCtx)
	385	+	// CommandContext owns the child through openCtx, so retain both this context
	386	+	// and its cancellation until the installed pane is reaped or torn down.
	387	+	p.ctx = openCtx
	388	+	p.cancel = func() {
	389	+	stopSessionCancel()
	390	+	cancelOpen()
	391	+	}
	392	+	ownedByPane = true
383	393		// The reader may run as soon as install returns; make its exit policy
384	394		// immutable before publishing the pane to the slot.
385	395		p.onExit = func() { d.reapFloating(sess, tb, p, generation) }

 	// before a late Open completion has either been rejected or fully joined.
 	d.sessWg.Go(func() {
 		defer sess.finishFloatingLaunch(launch)
-		d.openAndInstallFloating(sess, tb, spec, generation, launch)
+		d.openAndInstallFloating(sess, tb, spec, generation)
 	})
 }
 // openAndInstallFloating runs entirely in the launch worker. No PTY operation
 // occurs under tb.mu; the pane is fully initialized before publication.
-func (d *Daemon) openAndInstallFloating(sess *session, tb *tab, spec floatingLaunchSpec, generation uint64, launch *floatingLaunch) {
+func (d *Daemon) openAndInstallFloating(sess *session, tb *tab, spec floatingLaunchSpec, generation uint64) {
 	// Open is context-aware and is intentionally called without any daemon,
 	// session, tab, or launch-ownership lock. Cancellation makes teardown
 	// bounded even if the adapter is waiting to create the child.
 	}
 	openCtx, cancelOpen := context.WithCancel(spec.parentCtx)
 	stopSessionCancel := context.AfterFunc(sess.ctx, cancelOpen)
+	ownedByPane := false
 	defer func() {
-		stopSessionCancel()
+		if !ownedByPane {
-		cancelOpen()
+			stopSessionCancel()
+			cancelOpen()
+		}
 	}()
 	pty, err := d.ptys.Open(openCtx, spec.command, spec.args, spec.env, spec.cwd, spec.size)
 	if err != nil {
 	p.rect = spec.geometry.Inner
 	p.popupGeometry = spec.geometry
 	p.title.displayFallback = spec.fallback
-	p.ctx, p.cancel = context.WithCancel(spec.parentCtx)
+	// CommandContext owns the child through openCtx, so retain both this context
+	// and its cancellation until the installed pane is reaped or torn down.
+	p.ctx = openCtx
+	p.cancel = func() {
+		stopSessionCancel()
+		cancelOpen()
+	}
+	ownedByPane = true
 	// 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) }

M internal/usecase/daemon/floating_open_test.go +73 -3


@@ -54,12 +54,19 @@ func (p *contextAwareFloatingPTY) Close() error {
54	54		type contextAwareFloatingFactory struct {
55	55		pty *contextAwareFloatingPTY
56	56		opened chan context.Context
	57	+	err error
	58	+	onOpen func()
57	59		}
58	60
59	61		func (f *contextAwareFloatingFactory) Open(ctx context.Context, _ string, _ []string, _ []string, _ string, _ domain.Size) (ports.PTY, error) {
60		-	f.pty.ctx = ctx
	62	+	if f.pty != nil {
	63	+	f.pty.ctx = ctx
	64	+	}
61	65		f.opened <- ctx
62		-	return f.pty, nil
	66	+	if f.onOpen != nil {
	67	+	f.onOpen()
	68	+	}
	69	+	return f.pty, f.err
63	70		}
64	71
65	72		func TestFloatingSuccessfulOpenTransfersContextOwnershipToPane(t *testing.T) {

@@ -109,7 +116,7 @@ func TestFloatingSuccessfulOpenTransfersContextOwnershipToPane(t *testing.T) {
109	116		geometry: floatingGeometry{Inner: domain.Rect{Width: 20, Height: 8}},
110	117		paneStableID: "p_floating",
111	118		parentCtx: tabCtx,
112		-	}, generation, nil)
	119	+	}, generation)
113	120
114	121		openCtx := <-factory.opened
115	122		require.NoError(t, openCtx.Err(), "the PTY context must outlive the launch worker")

@@ -147,6 +154,69 @@ func TestFloatingSuccessfulOpenTransfersContextOwnershipToPane(t *testing.T) {
147	154		}
148	155		}
149	156
	157	+	func TestFloatingFailedAndStaleOpenCancelContext(t *testing.T) {
	158	+	tests := []struct {
	159	+	name string
	160	+	setup func(tab, contextAwareFloatingFactory)
	161	+	}{
	162	+	{
	163	+	name: "failed Open",
	164	+	setup: func(_ tab, factory contextAwareFloatingFactory) {
	165	+	factory.err = context.Canceled
	166	+	},
	167	+	},
	168	+	{
	169	+	name: "stale install",
	170	+	setup: func(tb tab, factory contextAwareFloatingFactory) {
	171	+	factory.onOpen = func() {
	172	+	tb.mu.Lock()
	173	+	tb.takeFloatingLocked()
	174	+	tb.mu.Unlock()
	175	+	}
	176	+	},
	177	+	},
	178	+	}
	179	+	for _, tt := range tests {
	180	+	t.Run(tt.name, func(t *testing.T) {
	181	+	pty := newContextAwareFloatingPTY()
	182	+	factory := &contextAwareFloatingFactory{pty: pty, opened: make(chan context.Context, 1)}
	183	+	d := newTestDaemon(t, factory, stubClock{})
	184	+	tb := newFloatingTestTab(t)
	185	+	sess := &session{name: "work", tabs: []*tab{tb}, ctx: t.Context()}
	186	+	tb.mu.Lock()
	187	+	generation := tb.beginFloatingWarmLocked(true)
	188	+	tb.mu.Unlock()
	189	+	tt.setup(tb, factory)
	190	+
	191	+	d.openAndInstallFloating(sess, tb, floatingLaunchSpec{
	192	+	sessionName: "work",
	193	+	size: domain.Size{Cols: 20, Rows: 8},
	194	+	geometry: floatingGeometry{Inner: domain.Rect{Width: 20, Height: 8}},
	195	+	paneStableID: "p_floating",
	196	+	parentCtx: tb.ctx,
	197	+	}, generation)
	198	+
	199	+	openCtx := <-factory.opened
	200	+	select {
	201	+	case <-openCtx.Done():
	202	+	case <-time.After(time.Second):
	203	+	t.Fatal("failed or stale floating Open retained its context")
	204	+	}
	205	+	tb.mu.Lock()
	206	+	require.Nil(t, tb.floating.pane)
	207	+	tb.mu.Unlock()
	208	+	if tt.name == "stale install" {
	209	+	select {
	210	+	case <-pty.closed:
	211	+	case <-time.After(time.Second):
	212	+	t.Fatal("stale floating PTY was not closed")
	213	+	}
	214	+	require.Equal(t, 1, pty.closeCount)
	215	+	}
	216	+	})
	217	+	}
	218	+	}
	219	+
150	220		func TestFloatingOpenCancellationBoundsSessionTeardownAndClosesLatePTY(t *testing.T) {
151	221		factory := portsmocks.NewMockPTYFactory(t)
152	222		opened := make(chan struct{})


@@ -54,12 +54,19 @@ func (p *contextAwareFloatingPTY) Close() error {
54		type contextAwareFloatingFactory struct {	54		type contextAwareFloatingFactory struct {
55		pty *contextAwareFloatingPTY	55		pty *contextAwareFloatingPTY
56		opened chan context.Context	56		opened chan context.Context
			57	+	err error
			58	+	onOpen func()
57		}	59		}
58			60
59		func (f *contextAwareFloatingFactory) Open(ctx context.Context, _ string, _ []string, _ []string, _ string, _ domain.Size) (ports.PTY, error) {	61		func (f *contextAwareFloatingFactory) Open(ctx context.Context, _ string, _ []string, _ []string, _ string, _ domain.Size) (ports.PTY, error) {
60	-	f.pty.ctx = ctx	62	+	if f.pty != nil {
			63	+	f.pty.ctx = ctx
			64	+	}
61		f.opened <- ctx	65		f.opened <- ctx
62	-	return f.pty, nil	66	+	if f.onOpen != nil {
			67	+	f.onOpen()
			68	+	}
			69	+	return f.pty, f.err
63		}	70		}
64			71
65		func TestFloatingSuccessfulOpenTransfersContextOwnershipToPane(t *testing.T) {	72		func TestFloatingSuccessfulOpenTransfersContextOwnershipToPane(t *testing.T) {

@@ -109,7 +116,7 @@ func TestFloatingSuccessfulOpenTransfersContextOwnershipToPane(t *testing.T) {
109		geometry: floatingGeometry{Inner: domain.Rect{Width: 20, Height: 8}},	116		geometry: floatingGeometry{Inner: domain.Rect{Width: 20, Height: 8}},
110		paneStableID: "p_floating",	117		paneStableID: "p_floating",
111		parentCtx: tabCtx,	118		parentCtx: tabCtx,
112	-	}, generation, nil)	119	+	}, generation)
113			120
114		openCtx := <-factory.opened	121		openCtx := <-factory.opened
115		require.NoError(t, openCtx.Err(), "the PTY context must outlive the launch worker")	122		require.NoError(t, openCtx.Err(), "the PTY context must outlive the launch worker")

@@ -147,6 +154,69 @@ func TestFloatingSuccessfulOpenTransfersContextOwnershipToPane(t *testing.T) {
147		}	154		}
148		}	155		}
149			156
			157	+	func TestFloatingFailedAndStaleOpenCancelContext(t *testing.T) {
			158	+	tests := []struct {
			159	+	name string
			160	+	setup func(tab, contextAwareFloatingFactory)
			161	+	}{
			162	+	{
			163	+	name: "failed Open",
			164	+	setup: func(_ tab, factory contextAwareFloatingFactory) {
			165	+	factory.err = context.Canceled
			166	+	},
			167	+	},
			168	+	{
			169	+	name: "stale install",
			170	+	setup: func(tb tab, factory contextAwareFloatingFactory) {
			171	+	factory.onOpen = func() {
			172	+	tb.mu.Lock()
			173	+	tb.takeFloatingLocked()
			174	+	tb.mu.Unlock()
			175	+	}
			176	+	},
			177	+	},
			178	+	}
			179	+	for _, tt := range tests {
			180	+	t.Run(tt.name, func(t *testing.T) {
			181	+	pty := newContextAwareFloatingPTY()
			182	+	factory := &contextAwareFloatingFactory{pty: pty, opened: make(chan context.Context, 1)}
			183	+	d := newTestDaemon(t, factory, stubClock{})
			184	+	tb := newFloatingTestTab(t)
			185	+	sess := &session{name: "work", tabs: []*tab{tb}, ctx: t.Context()}
			186	+	tb.mu.Lock()
			187	+	generation := tb.beginFloatingWarmLocked(true)
			188	+	tb.mu.Unlock()
			189	+	tt.setup(tb, factory)
			190	+
			191	+	d.openAndInstallFloating(sess, tb, floatingLaunchSpec{
			192	+	sessionName: "work",
			193	+	size: domain.Size{Cols: 20, Rows: 8},
			194	+	geometry: floatingGeometry{Inner: domain.Rect{Width: 20, Height: 8}},
			195	+	paneStableID: "p_floating",
			196	+	parentCtx: tb.ctx,
			197	+	}, generation)
			198	+
			199	+	openCtx := <-factory.opened
			200	+	select {
			201	+	case <-openCtx.Done():
			202	+	case <-time.After(time.Second):
			203	+	t.Fatal("failed or stale floating Open retained its context")
			204	+	}
			205	+	tb.mu.Lock()
			206	+	require.Nil(t, tb.floating.pane)
			207	+	tb.mu.Unlock()
			208	+	if tt.name == "stale install" {
			209	+	select {
			210	+	case <-pty.closed:
			211	+	case <-time.After(time.Second):
			212	+	t.Fatal("stale floating PTY was not closed")
			213	+	}
			214	+	require.Equal(t, 1, pty.closeCount)
			215	+	}
			216	+	})
			217	+	}
			218	+	}
			219	+
150		func TestFloatingOpenCancellationBoundsSessionTeardownAndClosesLatePTY(t *testing.T) {	220		func TestFloatingOpenCancellationBoundsSessionTeardownAndClosesLatePTY(t *testing.T) {
151		factory := portsmocks.NewMockPTYFactory(t)	221		factory := portsmocks.NewMockPTYFactory(t)
152		opened := make(chan struct{})	222		opened := make(chan struct{})