fix(daemon): surface pane resize failures
- commit
- 25f341a386d81fabfdb29cce0b48fec2260b0b8a
- author
- brice · · 2026-07-21 05:02 UTC
- parent
- a2b1d86
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +66 -1
M internal/usecase/daemon/performance_benchmark_test.go +1 -1
| @@ -132,7 +132,7 @@ func TestTransactionalResizeMetrics(t *testing.T) { | |||
| 132 | 132 | require.Equal(t, uint64(1), metrics.resizeCommits) | |
| 133 | 133 | require.Equal(t, uint64(1), metrics.ptyFailures) | |
| 134 | 134 | require.Equal(t, uint64(1), metrics.ptyRetries) | |
| 135 | - | require.Equal(t, uint64(2), metrics.outputFrames, "commit plus successful full-reset retry") | |
| 135 | + | require.Equal(t, uint64(3), metrics.outputFrames, "commit, resize-failure notice toast, and successful full-reset retry") | |
| 136 | 136 | require.Zero(t, metrics.frameGapEpochs, "every committed epoch emitted its required frame") | |
| 137 | 137 | require.Equal(t, []domain.Size{{Cols: 50, Rows: 28}, {Cols: 50, Rows: 28}}, fixture.pty.requested()) | |
| 138 | 138 | require.Equal(t, domain.Size{Cols: 100, Rows: 30}, fixture.ac.size) | |
| @@ -132,7 +132,7 @@ func TestTransactionalResizeMetrics(t *testing.T) { | |||||
| 132 | require.Equal(t, uint64(1), metrics.resizeCommits) | 132 | require.Equal(t, uint64(1), metrics.resizeCommits) | ||
| 133 | require.Equal(t, uint64(1), metrics.ptyFailures) | 133 | require.Equal(t, uint64(1), metrics.ptyFailures) | ||
| 134 | require.Equal(t, uint64(1), metrics.ptyRetries) | 134 | require.Equal(t, uint64(1), metrics.ptyRetries) | ||
| 135 | - | require.Equal(t, uint64(2), metrics.outputFrames, "commit plus successful full-reset retry") | 135 | + | require.Equal(t, uint64(3), metrics.outputFrames, "commit, resize-failure notice toast, and successful full-reset retry") |
| 136 | require.Zero(t, metrics.frameGapEpochs, "every committed epoch emitted its required frame") | 136 | require.Zero(t, metrics.frameGapEpochs, "every committed epoch emitted its required frame") | ||
| 137 | require.Equal(t, []domain.Size{{Cols: 50, Rows: 28}, {Cols: 50, Rows: 28}}, fixture.pty.requested()) | 137 | require.Equal(t, []domain.Size{{Cols: 50, Rows: 28}, {Cols: 50, Rows: 28}}, fixture.pty.requested()) | ||
| 138 | require.Equal(t, domain.Size{Cols: 100, Rows: 30}, fixture.ac.size) | 138 | require.Equal(t, domain.Size{Cols: 100, Rows: 30}, fixture.ac.size) | ||
M internal/usecase/daemon/transactional_resize.go +14 -0
| @@ -71,6 +71,11 @@ func (d *Daemon) prepareResize(sess *session, size domain.Size) resizePlan { | |||
| 71 | 71 | // applyResize deliberately holds only a pane's resizeMu around its PTY call; | |
| 72 | 72 | // no daemon, session, tab, or pane lock crosses the external boundary. | |
| 73 | 73 | func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |
| 74 | + | // Collect degradation across the loop and report once after it returns to | |
| 75 | + | // lock-free code: notify acquires sess.mu/d.mu and repaints, so it must never | |
| 76 | + | // run while a pane's resizeMu is held across the external PTY call below. | |
| 77 | + | failed := 0 | |
| 78 | + | var lastErr error | |
| 74 | 79 | for i := range plan.members { | |
| 75 | 80 | if len(current) != 0 && !current[0]() { | |
| 76 | 81 | return false | |
| @@ -91,6 +96,8 @@ func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |||
| 91 | 96 | m.ok = true | |
| 92 | 97 | } else if err := pty.Resize(rectSize(m.rect)); err != nil { | |
| 93 | 98 | d.log.Warn("pty resize failed", "err", err) | |
| 99 | + | failed++ | |
| 100 | + | lastErr = err | |
| 94 | 101 | d.replayResizePending(m.session, m.tab, m.pane, false, m.rect) | |
| 95 | 102 | } else { | |
| 96 | 103 | m.ok = true | |
| @@ -99,6 +106,13 @@ func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |||
| 99 | 106 | } | |
| 100 | 107 | m.pane.resizeMu.Unlock() | |
| 101 | 108 | } | |
| 109 | + | if failed > 0 { | |
| 110 | + | // Every resizeMu (and pane/session/daemon lock) is released here: each was | |
| 111 | + | // bracketed inside the loop. plan.members is non-empty because a failure | |
| 112 | + | // came from one of its members, so members[0] is always safe. | |
| 113 | + | d.notify(plan.members[0].session, domain.NoticeWarn, domain.NoticeResizeFailed, | |
| 114 | + | "pane resize failed; retrying in background", lastErr) | |
| 115 | + | } | |
| 102 | 116 | return true | |
| 103 | 117 | } | |
| 104 | 118 | ||
| @@ -71,6 +71,11 @@ func (d *Daemon) prepareResize(sess *session, size domain.Size) resizePlan { | |||||
| 71 | // applyResize deliberately holds only a pane's resizeMu around its PTY call; | 71 | // applyResize deliberately holds only a pane's resizeMu around its PTY call; | ||
| 72 | // no daemon, session, tab, or pane lock crosses the external boundary. | 72 | // no daemon, session, tab, or pane lock crosses the external boundary. | ||
| 73 | func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | 73 | func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | ||
| 74 | + | // Collect degradation across the loop and report once after it returns to | |||
| 75 | + | // lock-free code: notify acquires sess.mu/d.mu and repaints, so it must never | |||
| 76 | + | // run while a pane's resizeMu is held across the external PTY call below. | |||
| 77 | + | failed := 0 | |||
| 78 | + | var lastErr error | |||
| 74 | for i := range plan.members { | 79 | for i := range plan.members { | ||
| 75 | if len(current) != 0 && !current[0]() { | 80 | if len(current) != 0 && !current[0]() { | ||
| 76 | return false | 81 | return false | ||
| @@ -91,6 +96,8 @@ func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |||||
| 91 | m.ok = true | 96 | m.ok = true | ||
| 92 | } else if err := pty.Resize(rectSize(m.rect)); err != nil { | 97 | } else if err := pty.Resize(rectSize(m.rect)); err != nil { | ||
| 93 | d.log.Warn("pty resize failed", "err", err) | 98 | d.log.Warn("pty resize failed", "err", err) | ||
| 99 | + | failed++ | |||
| 100 | + | lastErr = err | |||
| 94 | d.replayResizePending(m.session, m.tab, m.pane, false, m.rect) | 101 | d.replayResizePending(m.session, m.tab, m.pane, false, m.rect) | ||
| 95 | } else { | 102 | } else { | ||
| 96 | m.ok = true | 103 | m.ok = true | ||
| @@ -99,6 +106,13 @@ func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |||||
| 99 | } | 106 | } | ||
| 100 | m.pane.resizeMu.Unlock() | 107 | m.pane.resizeMu.Unlock() | ||
| 101 | } | 108 | } | ||
| 109 | + | if failed > 0 { | |||
| 110 | + | // Every resizeMu (and pane/session/daemon lock) is released here: each was | |||
| 111 | + | // bracketed inside the loop. plan.members is non-empty because a failure | |||
| 112 | + | // came from one of its members, so members[0] is always safe. | |||
| 113 | + | d.notify(plan.members[0].session, domain.NoticeWarn, domain.NoticeResizeFailed, | |||
| 114 | + | "pane resize failed; retrying in background", lastErr) | |||
| 115 | + | } | |||
| 102 | return true | 116 | return true | ||
| 103 | } | 117 | } | ||
| 104 | 118 | ||||
M internal/usecase/daemon/transactional_resize_test.go +51 -0
| @@ -204,6 +204,57 @@ func TestTransactionalResizeApplyDoesNotHoldTabOrPaneLocks(t *testing.T) { | |||
| 204 | 204 | require.True(t, paneUnlocked, "PTY.Resize must run outside pane.mu") | |
| 205 | 205 | } | |
| 206 | 206 | ||
| 207 | + | // P3 degradation: a failing PTY resize is surfaced as exactly one warn notice | |
| 208 | + | // per applyResize call, collected inside the resizeMu loop and reported once | |
| 209 | + | // after it returns to lock-free code. The count is the meaningful assertion: | |
| 210 | + | // notify acquires sess.mu/d.mu and must never run while any resizeMu is held. | |
| 211 | + | func TestTransactionalResizeApplyReportsFailureOncePerCall(t *testing.T) { | |
| 212 | + | countResizeFailed := func(d *Daemon) (int, domain.Notification) { | |
| 213 | + | var last domain.Notification | |
| 214 | + | count := 0 | |
| 215 | + | for _, h := range d.notices.history() { | |
| 216 | + | if h.Code == domain.NoticeResizeFailed { | |
| 217 | + | count++ | |
| 218 | + | last = h | |
| 219 | + | } | |
| 220 | + | } | |
| 221 | + | return count, last | |
| 222 | + | } | |
| 223 | + | ||
| 224 | + | t.Run("one failed member yields one notice after return", func(t *testing.T) { | |
| 225 | + | failing := &transactionalResizePTY{errs: []error{errors.New("scripted resize failure")}} | |
| 226 | + | d, sess, _, _ := newManualSessionWithPTYs(t, failing) | |
| 227 | + | tb := sess.activeTab() | |
| 228 | + | p := tb.focusedPane() | |
| 229 | + | plan := resizePlan{members: []resizeMember{{session: sess, tab: tb, pane: p, rect: domain.Rect{Width: 100, Height: 20}, retry: true}}} | |
| 230 | + | ||
| 231 | + | require.Empty(t, d.notices.history(), "no notice is recorded before applyResize") | |
| 232 | + | require.True(t, d.applyResize(&plan)) | |
| 233 | + | ||
| 234 | + | count, n := countResizeFailed(d) | |
| 235 | + | require.Equal(t, 1, count, "exactly one NoticeResizeFailed per applyResize call") | |
| 236 | + | require.Equal(t, domain.NoticeWarn, n.Severity) | |
| 237 | + | require.Equal(t, "pane resize failed; retrying in background", n.Message) | |
| 238 | + | require.False(t, plan.members[0].ok, "failed member is not marked ok") | |
| 239 | + | }) | |
| 240 | + | ||
| 241 | + | t.Run("two failed members still yield one notice per call", func(t *testing.T) { | |
| 242 | + | first := &transactionalResizePTY{errs: []error{errors.New("first fails")}} | |
| 243 | + | second := &transactionalResizePTY{errs: []error{errors.New("second fails")}} | |
| 244 | + | d, sess, _, _ := newManualSessionWithPTYs(t, first, second) | |
| 245 | + | tabs := sess.tabs | |
| 246 | + | plan := resizePlan{members: []resizeMember{ | |
| 247 | + | {session: sess, tab: tabs[0], pane: tabs[0].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |
| 248 | + | {session: sess, tab: tabs[1], pane: tabs[1].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |
| 249 | + | }} | |
| 250 | + | ||
| 251 | + | require.True(t, d.applyResize(&plan)) | |
| 252 | + | ||
| 253 | + | count, _ := countResizeFailed(d) | |
| 254 | + | require.Equal(t, 1, count, "one notice per applyResize call, not per failed member") | |
| 255 | + | }) | |
| 256 | + | } | |
| 257 | + | ||
| 207 | 258 | // S3 acceptance: a failed member never publishes speculative parser/screen or | |
| 208 | 259 | // rectangle state. Successful members and the client layout commit together; | |
| 209 | 260 | // composition clips/pads the retained failed screen against that new layout. | |
| @@ -204,6 +204,57 @@ func TestTransactionalResizeApplyDoesNotHoldTabOrPaneLocks(t *testing.T) { | |||||
| 204 | require.True(t, paneUnlocked, "PTY.Resize must run outside pane.mu") | 204 | require.True(t, paneUnlocked, "PTY.Resize must run outside pane.mu") | ||
| 205 | } | 205 | } | ||
| 206 | 206 | ||||
| 207 | + | // P3 degradation: a failing PTY resize is surfaced as exactly one warn notice | |||
| 208 | + | // per applyResize call, collected inside the resizeMu loop and reported once | |||
| 209 | + | // after it returns to lock-free code. The count is the meaningful assertion: | |||
| 210 | + | // notify acquires sess.mu/d.mu and must never run while any resizeMu is held. | |||
| 211 | + | func TestTransactionalResizeApplyReportsFailureOncePerCall(t *testing.T) { | |||
| 212 | + | countResizeFailed := func(d *Daemon) (int, domain.Notification) { | |||
| 213 | + | var last domain.Notification | |||
| 214 | + | count := 0 | |||
| 215 | + | for _, h := range d.notices.history() { | |||
| 216 | + | if h.Code == domain.NoticeResizeFailed { | |||
| 217 | + | count++ | |||
| 218 | + | last = h | |||
| 219 | + | } | |||
| 220 | + | } | |||
| 221 | + | return count, last | |||
| 222 | + | } | |||
| 223 | + | ||||
| 224 | + | t.Run("one failed member yields one notice after return", func(t *testing.T) { | |||
| 225 | + | failing := &transactionalResizePTY{errs: []error{errors.New("scripted resize failure")}} | |||
| 226 | + | d, sess, _, _ := newManualSessionWithPTYs(t, failing) | |||
| 227 | + | tb := sess.activeTab() | |||
| 228 | + | p := tb.focusedPane() | |||
| 229 | + | plan := resizePlan{members: []resizeMember{{session: sess, tab: tb, pane: p, rect: domain.Rect{Width: 100, Height: 20}, retry: true}}} | |||
| 230 | + | ||||
| 231 | + | require.Empty(t, d.notices.history(), "no notice is recorded before applyResize") | |||
| 232 | + | require.True(t, d.applyResize(&plan)) | |||
| 233 | + | ||||
| 234 | + | count, n := countResizeFailed(d) | |||
| 235 | + | require.Equal(t, 1, count, "exactly one NoticeResizeFailed per applyResize call") | |||
| 236 | + | require.Equal(t, domain.NoticeWarn, n.Severity) | |||
| 237 | + | require.Equal(t, "pane resize failed; retrying in background", n.Message) | |||
| 238 | + | require.False(t, plan.members[0].ok, "failed member is not marked ok") | |||
| 239 | + | }) | |||
| 240 | + | ||||
| 241 | + | t.Run("two failed members still yield one notice per call", func(t *testing.T) { | |||
| 242 | + | first := &transactionalResizePTY{errs: []error{errors.New("first fails")}} | |||
| 243 | + | second := &transactionalResizePTY{errs: []error{errors.New("second fails")}} | |||
| 244 | + | d, sess, _, _ := newManualSessionWithPTYs(t, first, second) | |||
| 245 | + | tabs := sess.tabs | |||
| 246 | + | plan := resizePlan{members: []resizeMember{ | |||
| 247 | + | {session: sess, tab: tabs[0], pane: tabs[0].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |||
| 248 | + | {session: sess, tab: tabs[1], pane: tabs[1].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |||
| 249 | + | }} | |||
| 250 | + | ||||
| 251 | + | require.True(t, d.applyResize(&plan)) | |||
| 252 | + | ||||
| 253 | + | count, _ := countResizeFailed(d) | |||
| 254 | + | require.Equal(t, 1, count, "one notice per applyResize call, not per failed member") | |||
| 255 | + | }) | |||
| 256 | + | } | |||
| 257 | + | ||||
| 207 | // S3 acceptance: a failed member never publishes speculative parser/screen or | 258 | // S3 acceptance: a failed member never publishes speculative parser/screen or | ||
| 208 | // rectangle state. Successful members and the client layout commit together; | 259 | // rectangle state. Successful members and the client layout commit together; | ||
| 209 | // composition clips/pads the retained failed screen against that new layout. | 260 | // composition clips/pads the retained failed screen against that new layout. | ||