fix(daemon): keep pty reader non-blocking on attention

commit: 050ecf4860ef8c67da07bbe988d524dcf3af2d0f
author: brice · 3mo ago · 2026-07-03 14:45 UTC
committer: brice · 3mo ago · 2026-07-03 15:33 UTC
parent: e557ce2
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +55 -1

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


@@ -33,7 +33,12 @@ func (d Daemon) noteAttention(sess session, tb *tab) {
33	33		tb.attention = true
34	34		sess.mu.Unlock()
35	35
36		-	d.repaintAllAttachedClients()
	36	+	// Do not repaint here: this runs on the PTY reader goroutine, and paint
	37	+	// can block on a slow client's socket (Transport.Send has no deadline) —
	38	+	// that would let one wedged client stall a different session's reader.
	39	+	// The animator's first tick (<=120ms) delivers the repaint, and pulse
	40	+	// frame 0 renders the bell as blank anyway, so nothing is visibly lost
	41	+	// by not painting immediately.
37	42		d.pokeAttentionTicker()
38	43		}
39	44

 	tb.attention = true
 	sess.mu.Unlock()
-	d.repaintAllAttachedClients()
+	// Do not repaint here: this runs on the PTY reader goroutine, and paint
+	// can block on a slow client's socket (Transport.Send has no deadline) —
+	// that would let one wedged client stall a different session's reader.
+	// The animator's first tick (<=120ms) delivers the repaint, and pulse
+	// frame 0 renders the bell as blank anyway, so nothing is visibly lost
+	// by not painting immediately.
 	d.pokeAttentionTicker()
 }

M internal/usecase/daemon/attention_test.go +49 -0


@@ -7,10 +7,12 @@ import (
7	7		"testing"
8	8		"time"
9	9
	10	+	"github.com/stretchr/testify/mock"
10	11		"github.com/stretchr/testify/require"
11	12
12	13		"github.com/bnema/vev/internal/domain"
13	14		"github.com/bnema/vev/internal/ports"
	15	+	portsmocks "github.com/bnema/vev/internal/ports/mocks"
14	16		"github.com/bnema/vev/internal/usecase/keys"
15	17		"github.com/bnema/vev/pkg/renderer"
16	18		"github.com/bnema/vev/pkg/vt"

@@ -78,6 +80,53 @@ func TestPTYReaderBellMarksBackgroundTab(t *testing.T) {
78	80		d.sessWg.Wait()
79	81		}
80	82
	83	+	// TestNoteAttentionDoesNotBlockOnWedgedOtherClient is the regression test for
	84	+	// the reader-blocks-on-a-slow-client bug: noteAttention runs on the PTY
	85	+	// reader goroutine (see ptyReader in render.go), so it must never repaint
	86	+	// synchronously — paint ends in Transport.Send, which has no deadline, and a
	87	+	// stalled client's socket would otherwise stall the reader of an unrelated
	88	+	// session. A second session's client whose Send never returns must not stop
	89	+	// noteAttention from returning promptly.
	90	+	func TestNoteAttentionDoesNotBlockOnWedgedOtherClient(t *testing.T) {
	91	+	d, sess, _, _, releases := newManualTabSession(t, 2)
	92	+	defer releases[0]()
	93	+	defer releases[1]()
	94	+	sess.active = 0
	95	+
	96	+	trW := portsmocks.NewMockTransport(t)
	97	+	block := make(chan struct{})
	98	+	t.Cleanup(func() { close(block) })
	99	+	trW.EXPECT().Send(mock.Anything).RunAndReturn(func(ports.Frame) error {
	100	+	<-block
	101	+	return nil
	102	+	}).Maybe()
	103	+	trW.EXPECT().Close().Return(nil).Maybe()
	104	+	acW := &attachedClient{tr: trW, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}}
	105	+	sctxW, cancelW := context.WithCancel(d.serveCtx)
	106	+	t.Cleanup(cancelW)
	107	+	tabW := &tab{screen: vt.NewScreen(80, 23), dirty: make(chan struct{}, 1), size: domain.Size{Cols: 80, Rows: 23}, ctx: sctxW, cancel: cancelW}
	108	+	sessW := &session{id: "wedged", name: "wedged", ctx: sctxW, cancel: cancelW, tabs: []*tab{tabW}, client: acW}
	109	+	acW.setSession(sessW)
	110	+	acW.keys = keys.NewRouter(d.clock, daemonKeyHandler{d: d, ac: acW})
	111	+	d.sessions[sessW.id] = sessW
	112	+
	113	+	done := make(chan struct{})
	114	+	go func() {
	115	+	d.noteAttention(sess, sess.tabs[1])
	116	+	close(done)
	117	+	}()
	118	+
	119	+	select {
	120	+	case <-done:
	121	+	case <-time.After(2 * time.Second):
	122	+	t.Fatal("noteAttention blocked on a wedged other-session client's Send")
	123	+	}
	124	+
	125	+	sess.mu.Lock()
	126	+	defer sess.mu.Unlock()
	127	+	require.True(t, sess.tabs[1].attention)
	128	+	}
	129	+
81	130		type scriptPTY struct {
82	131		mu sync.Mutex
83	132		reads [][]byte


@@ -7,10 +7,12 @@ import (
7	"testing"	7		"testing"
8	"time"	8		"time"
9		9
		10	+	"github.com/stretchr/testify/mock"
10	"github.com/stretchr/testify/require"	11		"github.com/stretchr/testify/require"
11		12
12	"github.com/bnema/vev/internal/domain"	13		"github.com/bnema/vev/internal/domain"
13	"github.com/bnema/vev/internal/ports"	14		"github.com/bnema/vev/internal/ports"
		15	+	portsmocks "github.com/bnema/vev/internal/ports/mocks"
14	"github.com/bnema/vev/internal/usecase/keys"	16		"github.com/bnema/vev/internal/usecase/keys"
15	"github.com/bnema/vev/pkg/renderer"	17		"github.com/bnema/vev/pkg/renderer"
16	"github.com/bnema/vev/pkg/vt"	18		"github.com/bnema/vev/pkg/vt"

@@ -78,6 +80,53 @@ func TestPTYReaderBellMarksBackgroundTab(t *testing.T) {
78	d.sessWg.Wait()	80		d.sessWg.Wait()
79	}	81		}
80		82
		83	+	// TestNoteAttentionDoesNotBlockOnWedgedOtherClient is the regression test for
		84	+	// the reader-blocks-on-a-slow-client bug: noteAttention runs on the PTY
		85	+	// reader goroutine (see ptyReader in render.go), so it must never repaint
		86	+	// synchronously — paint ends in Transport.Send, which has no deadline, and a
		87	+	// stalled client's socket would otherwise stall the reader of an unrelated
		88	+	// session. A second session's client whose Send never returns must not stop
		89	+	// noteAttention from returning promptly.
		90	+	func TestNoteAttentionDoesNotBlockOnWedgedOtherClient(t *testing.T) {
		91	+	d, sess, _, _, releases := newManualTabSession(t, 2)
		92	+	defer releases[0]()
		93	+	defer releases[1]()
		94	+	sess.active = 0
		95	+
		96	+	trW := portsmocks.NewMockTransport(t)
		97	+	block := make(chan struct{})
		98	+	t.Cleanup(func() { close(block) })
		99	+	trW.EXPECT().Send(mock.Anything).RunAndReturn(func(ports.Frame) error {
		100	+	<-block
		101	+	return nil
		102	+	}).Maybe()
		103	+	trW.EXPECT().Close().Return(nil).Maybe()
		104	+	acW := &attachedClient{tr: trW, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}}
		105	+	sctxW, cancelW := context.WithCancel(d.serveCtx)
		106	+	t.Cleanup(cancelW)
		107	+	tabW := &tab{screen: vt.NewScreen(80, 23), dirty: make(chan struct{}, 1), size: domain.Size{Cols: 80, Rows: 23}, ctx: sctxW, cancel: cancelW}
		108	+	sessW := &session{id: "wedged", name: "wedged", ctx: sctxW, cancel: cancelW, tabs: []*tab{tabW}, client: acW}
		109	+	acW.setSession(sessW)
		110	+	acW.keys = keys.NewRouter(d.clock, daemonKeyHandler{d: d, ac: acW})
		111	+	d.sessions[sessW.id] = sessW
		112	+
		113	+	done := make(chan struct{})
		114	+	go func() {
		115	+	d.noteAttention(sess, sess.tabs[1])
		116	+	close(done)
		117	+	}()
		118	+
		119	+	select {
		120	+	case <-done:
		121	+	case <-time.After(2 * time.Second):
		122	+	t.Fatal("noteAttention blocked on a wedged other-session client's Send")
		123	+	}
		124	+
		125	+	sess.mu.Lock()
		126	+	defer sess.mu.Unlock()
		127	+	require.True(t, sess.tabs[1].attention)
		128	+	}
		129	+
81	type scriptPTY struct {	130		type scriptPTY struct {
82	mu sync.Mutex	131		mu sync.Mutex
83	reads [][]byte	132		reads [][]byte