fix(client): keep picker input released when an attachment run is cancelled (#303)

A cancelled run could settle through the attachment context (derived from
the parent) before the ctx.Done case was selected. It was then treated as a
plain end, and the deferred teardown handed terminal input back to the
retired picker. Parent cancellation now wins after settlement, and a
terminated run no longer re-acquires the picker. The test asserts ownership
after Run returns so the regression is deterministic.

commit: 80d04fec7fbed319a77f2dcda333898a0ca76ca9
author: bnema · 23d ago · 2026-09-29 19:42 UTC
committer: GitHub · 23d ago · 2026-09-29 19:42 UTC
parent: b90b5f2
signature: GPG UnverifiedB5690EEEBB952194 The key is unknown

2 files · +18 -1

M internal/usecase/client/supervisor_attach.go +15 -1


@@ -560,7 +560,7 @@ func (s Supervisor) runCommittedAttachment(ctx context.Context, input terminal
560	560		return s.runResolvedAttachment(ctx, input, service, pickerAttachmentTarget{request: request, tab: tab}, SessionEnvironmentLocalPicker)
561	561		}
562	562
563		-	func (s Supervisor) runResolvedAttachment(ctx context.Context, input terminalInputLifetime, service ports.BrokerNavigator, target pickerAttachmentTarget, localProvenance SessionEnvironmentProvenance) (bool, error) {
	563	+	func (s Supervisor) runResolvedAttachment(ctx context.Context, input terminalInputLifetime, service ports.BrokerNavigator, target pickerAttachmentTarget, localProvenance SessionEnvironmentProvenance) (terminated bool, termErr error) {
564	564		picker := s.cfg.Picker
565	565		if picker == nil {
566	566		return false, nil

@@ -568,6 +568,11 @@ func (s Supervisor) runResolvedAttachment(ctx context.Context, input terminalI
568	568		picker.SetOwnsInput(false)
569	569		input.releasePicker()
570	570		defer func() {
	571	+	// A terminated run never returns to the picker: handing it input back
	572	+	// during teardown would let a retired picker consume keys.
	573	+	if terminated {
	574	+	return
	575	+	}
571	576		// Whoever owned the terminal since the last picker frame, the next
572	577		// frame must draw the whole box again.
573	578		s.invalidatePickerPresentation()

@@ -811,6 +816,15 @@ settlement:
811	816		}
812	817		}
813	818
	819	+	// Parent cancellation wins over a settlement that raced it: the attachment
	820	+	// context derives from ctx, so cancellation can settle the run before the
	821	+	// ctx.Done case is selected.
	822	+	if !terminated {
	823	+	if cause := ctx.Err(); cause != nil {
	824	+	terminated, termErr = true, cause
	825	+	}
	826	+	}
	827	+
814	828		// The attachment settled, so its foreground (and overlay slot) is gone.
815	829		overlay.drop()
816	830		if !terminated && !brokerLost {

 	return s.runResolvedAttachment(ctx, input, service, pickerAttachmentTarget{request: request, tab: tab}, SessionEnvironmentLocalPicker)
 }
-func (s *Supervisor) runResolvedAttachment(ctx context.Context, input *terminalInputLifetime, service ports.BrokerNavigator, target pickerAttachmentTarget, localProvenance SessionEnvironmentProvenance) (bool, error) {
+func (s *Supervisor) runResolvedAttachment(ctx context.Context, input *terminalInputLifetime, service ports.BrokerNavigator, target pickerAttachmentTarget, localProvenance SessionEnvironmentProvenance) (terminated bool, termErr error) {
 	picker := s.cfg.Picker
 	if picker == nil {
 		return false, nil
 	picker.SetOwnsInput(false)
 	input.releasePicker()
 	defer func() {
+		// A terminated run never returns to the picker: handing it input back
+		// during teardown would let a retired picker consume keys.
+		if terminated {
+			return
+		}
 		// Whoever owned the terminal since the last picker frame, the next
 		// frame must draw the whole box again.
 		s.invalidatePickerPresentation()
 		}
 	}
+	// Parent cancellation wins over a settlement that raced it: the attachment
+	// context derives from ctx, so cancellation can settle the run before the
+	// ctx.Done case is selected.
+	if !terminated {
+		if cause := ctx.Err(); cause != nil {
+			terminated, termErr = true, cause
+		}
+	}
 	// The attachment settled, so its foreground (and overlay slot) is gone.
 	overlay.drop()
 	if !terminated && !brokerLost {

M internal/usecase/client/supervisor_attach_test.go +3 -0


@@ -1729,6 +1729,9 @@ func TestSupervisorAttachmentConcurrentCommitAndCancellation(t *testing.T) {
1729	1729		require.Equal(t, resolved, picker.resolveCount(), "revoked picker operations must not resolve")
1730	1730		require.Equal(t, consumed, picker.consumedCount(), "revoked input must not be consumed")
1731	1731		require.ErrorIs(t, harness.waitRun(t), context.Canceled)
	1732	+	// Teardown has fully run: a cancelled attachment must not hand input
	1733	+	// back to the picker on its way out.
	1734	+	require.False(t, picker.owns(), "a terminated run must not re-acquire the picker")
1732	1735		require.True(t, stream.closedNow(), "the admitted stream must be released")
1733	1736		require.NotEqual(t, PresentAttached, harness.sup.State().Presentation)
1734	1737		})


@@ -1729,6 +1729,9 @@ func TestSupervisorAttachmentConcurrentCommitAndCancellation(t *testing.T) {
1729	require.Equal(t, resolved, picker.resolveCount(), "revoked picker operations must not resolve")	1729		require.Equal(t, resolved, picker.resolveCount(), "revoked picker operations must not resolve")
1730	require.Equal(t, consumed, picker.consumedCount(), "revoked input must not be consumed")	1730		require.Equal(t, consumed, picker.consumedCount(), "revoked input must not be consumed")
1731	require.ErrorIs(t, harness.waitRun(t), context.Canceled)	1731		require.ErrorIs(t, harness.waitRun(t), context.Canceled)
		1732	+	// Teardown has fully run: a cancelled attachment must not hand input
		1733	+	// back to the picker on its way out.
		1734	+	require.False(t, picker.owns(), "a terminated run must not re-acquire the picker")
1732	require.True(t, stream.closedNow(), "the admitted stream must be released")	1735		require.True(t, stream.closedNow(), "the admitted stream must be released")
1733	require.NotEqual(t, PresentAttached, harness.sup.State().Presentation)	1736		require.NotEqual(t, PresentAttached, harness.sup.State().Presentation)
1734	})	1737		})