fix(daemon): close notice routing race windows
- commit
- 1dc131d09436335432d9c117d35b3f15ba95fe84
- author
- brice · · 2026-07-21 12:05 UTC
- committer
- brice · · 2026-07-21 12:06 UTC
- parent
- 41eb8b0
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
6 files · +202 -34
M internal/usecase/client/reconnect_toast_test.go +18 -8
| @@ -337,15 +337,25 @@ func TestAttachAttemptOfflineLinkEventReturnsReconnectableError(t *testing.T) { | |||
| 337 | 337 | } | |
| 338 | 338 | ||
| 339 | 339 | func TestDetachedResultReplacementAndCleanDetach(t *testing.T) { | |
| 340 | - | if err := detachedResult(ports.ReasonDetach); err != nil { | |
| 341 | - | t.Fatalf("clean detach = %v, want nil", err) | |
| 342 | - | } | |
| 343 | - | err := detachedResult(ports.ReasonReplaced) | |
| 344 | - | var detached *DetachedError | |
| 345 | - | if !errors.As(err, &detached) { | |
| 346 | - | t.Fatalf("replacement error = %T, want *DetachedError", err) | |
| 340 | + | for _, tt := range []struct { | |
| 341 | + | name string | |
| 342 | + | reason uint8 | |
| 343 | + | wantText string | |
| 344 | + | }{ | |
| 345 | + | {name: "clean detach", reason: ports.ReasonDetach}, | |
| 346 | + | {name: "replacement", reason: ports.ReasonReplaced, wantText: "session taken over by another client"}, | |
| 347 | + | } { | |
| 348 | + | t.Run(tt.name, func(t *testing.T) { | |
| 349 | + | err := detachedResult(tt.reason) | |
| 350 | + | if tt.wantText == "" { | |
| 351 | + | require.NoError(t, err) | |
| 352 | + | return | |
| 353 | + | } | |
| 354 | + | var detached *DetachedError | |
| 355 | + | require.ErrorAs(t, err, &detached) | |
| 356 | + | require.Equal(t, tt.wantText, detached.Text) | |
| 357 | + | }) | |
| 347 | 358 | } | |
| 348 | - | require.Equal(t, "session taken over by another client", detached.Text) | |
| 349 | 359 | } | |
| 350 | 360 | ||
| 351 | 361 | func TestReconnectToastBoundsUseCenterAnchor(t *testing.T) { | |
| @@ -337,15 +337,25 @@ func TestAttachAttemptOfflineLinkEventReturnsReconnectableError(t *testing.T) { | |||||
| 337 | } | 337 | } | ||
| 338 | 338 | ||||
| 339 | func TestDetachedResultReplacementAndCleanDetach(t *testing.T) { | 339 | func TestDetachedResultReplacementAndCleanDetach(t *testing.T) { | ||
| 340 | - | if err := detachedResult(ports.ReasonDetach); err != nil { | 340 | + | for _, tt := range []struct { |
| 341 | - | t.Fatalf("clean detach = %v, want nil", err) | 341 | + | name string |
| 342 | - | } | 342 | + | reason uint8 |
| 343 | - | err := detachedResult(ports.ReasonReplaced) | 343 | + | wantText string |
| 344 | - | var detached *DetachedError | 344 | + | }{ |
| 345 | - | if !errors.As(err, &detached) { | 345 | + | {name: "clean detach", reason: ports.ReasonDetach}, |
| 346 | - | t.Fatalf("replacement error = %T, want *DetachedError", err) | 346 | + | {name: "replacement", reason: ports.ReasonReplaced, wantText: "session taken over by another client"}, |
| 347 | + | } { | |||
| 348 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 349 | + | err := detachedResult(tt.reason) | |||
| 350 | + | if tt.wantText == "" { | |||
| 351 | + | require.NoError(t, err) | |||
| 352 | + | return | |||
| 353 | + | } | |||
| 354 | + | var detached *DetachedError | |||
| 355 | + | require.ErrorAs(t, err, &detached) | |||
| 356 | + | require.Equal(t, tt.wantText, detached.Text) | |||
| 357 | + | }) | |||
| 347 | } | 358 | } | ||
| 348 | - | require.Equal(t, "session taken over by another client", detached.Text) | |||
| 349 | } | 359 | } | ||
| 350 | 360 | ||||
| 351 | func TestReconnectToastBoundsUseCenterAnchor(t *testing.T) { | 361 | func TestReconnectToastBoundsUseCenterAnchor(t *testing.T) { | ||
M internal/usecase/daemon/client.go +10 -2
| @@ -762,15 +762,23 @@ func (d *Daemon) resize(sess *session, ac *attachedClient, sz domain.Size) { | |||
| 762 | 762 | } | |
| 763 | 763 | ||
| 764 | 764 | // handleClientNotice maps the closed client-event enum to daemon-owned notice | |
| 765 | - | // content. The connection loop has already bound sess and ac to this transport; | |
| 766 | - | // verify that ownership is still current before changing presentation state. | |
| 765 | + | // content. routingMu makes ownership validation and toast mutation one atomic | |
| 766 | + | // attachment-routing operation: replacement also takes routingMu before | |
| 767 | + | // publishing sess.client. Never retain sess.mu while touching notice or overlay | |
| 768 | + | // state, and retain the routingMu -> sess.mu order used by attachment paths. | |
| 767 | 769 | func (d *Daemon) handleClientNotice(sess *session, ac *attachedClient, notice ports.ClientNotice) { | |
| 770 | + | d.notices.routingMu.Lock() | |
| 771 | + | defer d.notices.routingMu.Unlock() | |
| 772 | + | ||
| 768 | 773 | sess.mu.Lock() | |
| 769 | 774 | current := sess.client == ac | |
| 770 | 775 | sess.mu.Unlock() | |
| 771 | 776 | if !current { | |
| 772 | 777 | return | |
| 773 | 778 | } | |
| 779 | + | if d.notices.beforeClientNoticeMutation != nil { | |
| 780 | + | d.notices.beforeClientNoticeMutation() | |
| 781 | + | } | |
| 774 | 782 | ||
| 775 | 783 | switch notice.Action { | |
| 776 | 784 | case ports.ClientNoticeClipboardFallback: | |
| @@ -762,15 +762,23 @@ func (d *Daemon) resize(sess *session, ac *attachedClient, sz domain.Size) { | |||||
| 762 | } | 762 | } | ||
| 763 | 763 | ||||
| 764 | // handleClientNotice maps the closed client-event enum to daemon-owned notice | 764 | // handleClientNotice maps the closed client-event enum to daemon-owned notice | ||
| 765 | - | // content. The connection loop has already bound sess and ac to this transport; | 765 | + | // content. routingMu makes ownership validation and toast mutation one atomic |
| 766 | - | // verify that ownership is still current before changing presentation state. | 766 | + | // attachment-routing operation: replacement also takes routingMu before |
| 767 | + | // publishing sess.client. Never retain sess.mu while touching notice or overlay | |||
| 768 | + | // state, and retain the routingMu -> sess.mu order used by attachment paths. | |||
| 767 | func (d *Daemon) handleClientNotice(sess *session, ac *attachedClient, notice ports.ClientNotice) { | 769 | func (d *Daemon) handleClientNotice(sess *session, ac *attachedClient, notice ports.ClientNotice) { | ||
| 770 | + | d.notices.routingMu.Lock() | |||
| 771 | + | defer d.notices.routingMu.Unlock() | |||
| 772 | + | ||||
| 768 | sess.mu.Lock() | 773 | sess.mu.Lock() | ||
| 769 | current := sess.client == ac | 774 | current := sess.client == ac | ||
| 770 | sess.mu.Unlock() | 775 | sess.mu.Unlock() | ||
| 771 | if !current { | 776 | if !current { | ||
| 772 | return | 777 | return | ||
| 773 | } | 778 | } | ||
| 779 | + | if d.notices.beforeClientNoticeMutation != nil { | |||
| 780 | + | d.notices.beforeClientNoticeMutation() | |||
| 781 | + | } | |||
| 774 | 782 | ||||
| 775 | switch notice.Action { | 783 | switch notice.Action { | ||
| 776 | case ports.ClientNoticeClipboardFallback: | 784 | case ports.ClientNoticeClipboardFallback: | ||
M internal/usecase/daemon/notify.go +3 -0
| @@ -41,6 +41,9 @@ type noticeCenter struct { | |||
| 41 | 41 | // beforeGlobalDelivery is a deterministic test seam after targets have been | |
| 42 | 42 | // selected and before their toast stacks are touched. | |
| 43 | 43 | beforeGlobalDelivery func() | |
| 44 | + | // beforeClientNoticeMutation is a deterministic test seam after a client | |
| 45 | + | // notice's ownership validation and before its toast state is changed. | |
| 46 | + | beforeClientNoticeMutation func() | |
| 44 | 47 | } | |
| 45 | 48 | ||
| 46 | 49 | func newNoticeCenter() *noticeCenter { return ¬iceCenter{} } | |
| @@ -41,6 +41,9 @@ type noticeCenter struct { | |||||
| 41 | // beforeGlobalDelivery is a deterministic test seam after targets have been | 41 | // beforeGlobalDelivery is a deterministic test seam after targets have been | ||
| 42 | // selected and before their toast stacks are touched. | 42 | // selected and before their toast stacks are touched. | ||
| 43 | beforeGlobalDelivery func() | 43 | beforeGlobalDelivery func() | ||
| 44 | + | // beforeClientNoticeMutation is a deterministic test seam after a client | |||
| 45 | + | // notice's ownership validation and before its toast state is changed. | |||
| 46 | + | beforeClientNoticeMutation func() | |||
| 44 | } | 47 | } | ||
| 45 | 48 | ||||
| 46 | func newNoticeCenter() *noticeCenter { return ¬iceCenter{} } | 49 | func newNoticeCenter() *noticeCenter { return ¬iceCenter{} } | ||
M internal/usecase/daemon/notify_test.go +99 -0
| @@ -40,6 +40,105 @@ func TestClientNoticeMapsFixedActionsAndDismissesOnlyConnectionToast(t *testing. | |||
| 40 | 40 | } | |
| 41 | 41 | } | |
| 42 | 42 | ||
| 43 | + | func TestHandleClientNoticeSerializesReplacement(t *testing.T) { | |
| 44 | + | d, sess, old, _ := newNoticeFixture(t, newNoticeClock()) | |
| 45 | + | ||
| 46 | + | enteredMutation := make(chan struct{}) | |
| 47 | + | releaseMutation := make(chan struct{}) | |
| 48 | + | d.notices.beforeClientNoticeMutation = func() { | |
| 49 | + | close(enteredMutation) | |
| 50 | + | <-releaseMutation | |
| 51 | + | } | |
| 52 | + | t.Cleanup(func() { | |
| 53 | + | select { | |
| 54 | + | case <-releaseMutation: | |
| 55 | + | default: | |
| 56 | + | close(releaseMutation) | |
| 57 | + | } | |
| 58 | + | }) | |
| 59 | + | ||
| 60 | + | noticeDone := make(chan struct{}) | |
| 61 | + | go func() { | |
| 62 | + | d.handleClientNotice(sess, old, ports.ClientNotice{Action: ports.ClientNoticeClipboardFallback}) | |
| 63 | + | close(noticeDone) | |
| 64 | + | }() | |
| 65 | + | <-enteredMutation // old has validated ownership and holds routingMu | |
| 66 | + | if d.notices.routingMu.TryLock() { | |
| 67 | + | d.notices.routingMu.Unlock() | |
| 68 | + | t.Fatal("client notice mutation must retain routingMu through publication") | |
| 69 | + | } | |
| 70 | + | ||
| 71 | + | tr, _ := newCapturingTransport(t) | |
| 72 | + | replaced := make(chan *attachedClient, 1) | |
| 73 | + | go func() { | |
| 74 | + | current, _ := d.attachClient(sess, tr, domain.Size{Cols: 80, Rows: 24}, attachClientOptions{}) | |
| 75 | + | replaced <- current | |
| 76 | + | }() | |
| 77 | + | ||
| 78 | + | // Releasing the mutation lets the already-validated notice publish before | |
| 79 | + | // replacement can own the session. Thus no stale attachment can mutate a | |
| 80 | + | // toast stack after sess.client changes. | |
| 81 | + | close(releaseMutation) | |
| 82 | + | <-noticeDone | |
| 83 | + | current := <-replaced | |
| 84 | + | ||
| 85 | + | history := d.notices.history() | |
| 86 | + | require.Len(t, history, 1) | |
| 87 | + | require.Equal(t, domain.NoticeClipboard, history[0].Code) | |
| 88 | + | oldToasts, _ := visibleToasts(old) | |
| 89 | + | require.Len(t, oldToasts, 1) | |
| 90 | + | require.Equal(t, domain.NoticeClipboard, oldToasts[0].Code) | |
| 91 | + | newToasts, _ := visibleToasts(current) | |
| 92 | + | require.Empty(t, newToasts) | |
| 93 | + | sess.mu.Lock() | |
| 94 | + | require.Same(t, current, sess.client) | |
| 95 | + | sess.mu.Unlock() | |
| 96 | + | } | |
| 97 | + | ||
| 98 | + | func TestHandleClientNoticeRejectsReplacedClient(t *testing.T) { | |
| 99 | + | for _, tt := range []struct { | |
| 100 | + | name string | |
| 101 | + | setup func(*Daemon, *attachedClient, domain.SessionID) | |
| 102 | + | notice ports.ClientNotice | |
| 103 | + | wantToast domain.NoticeCode | |
| 104 | + | }{ | |
| 105 | + | { | |
| 106 | + | name: "record", | |
| 107 | + | notice: ports.ClientNotice{Action: ports.ClientNoticeClipboardFallback}, | |
| 108 | + | }, | |
| 109 | + | { | |
| 110 | + | name: "dismiss", | |
| 111 | + | setup: func(d *Daemon, ac *attachedClient, sid domain.SessionID) { | |
| 112 | + | d.showToast(ac, notice(domain.NoticeConnection, "connection degraded", sid)) | |
| 113 | + | }, | |
| 114 | + | notice: ports.ClientNotice{Action: ports.ClientNoticeLinkConnected}, | |
| 115 | + | wantToast: domain.NoticeConnection, | |
| 116 | + | }, | |
| 117 | + | } { | |
| 118 | + | t.Run(tt.name, func(t *testing.T) { | |
| 119 | + | d, sess, old, _ := newNoticeFixture(t, newNoticeClock()) | |
| 120 | + | tr, _ := newCapturingTransport(t) | |
| 121 | + | current, _ := d.attachClient(sess, tr, domain.Size{Cols: 80, Rows: 24}, attachClientOptions{}) | |
| 122 | + | if tt.setup != nil { | |
| 123 | + | tt.setup(d, old, sess.id) | |
| 124 | + | } | |
| 125 | + | ||
| 126 | + | d.handleClientNotice(sess, old, tt.notice) | |
| 127 | + | ||
| 128 | + | require.Empty(t, d.notices.history(), "a replaced client must not record notices") | |
| 129 | + | oldToasts, _ := visibleToasts(old) | |
| 130 | + | if tt.setup == nil { | |
| 131 | + | require.Empty(t, oldToasts) | |
| 132 | + | } else { | |
| 133 | + | require.Len(t, oldToasts, 1) | |
| 134 | + | require.Equal(t, tt.wantToast, oldToasts[0].Code, "a replaced client must not dismiss toasts") | |
| 135 | + | } | |
| 136 | + | currentToasts, _ := visibleToasts(current) | |
| 137 | + | require.Empty(t, currentToasts) | |
| 138 | + | }) | |
| 139 | + | } | |
| 140 | + | } | |
| 141 | + | ||
| 43 | 142 | func TestMalformedClientNoticeIsIgnored(t *testing.T) { | |
| 44 | 143 | d, _, ac, _ := newNoticeFixture(t, ¬iceClock{}) | |
| 45 | 144 | tr, ok := ac.tr.(*portsmocks.MockTransport) | |
| @@ -40,6 +40,105 @@ func TestClientNoticeMapsFixedActionsAndDismissesOnlyConnectionToast(t *testing. | |||||
| 40 | } | 40 | } | ||
| 41 | } | 41 | } | ||
| 42 | 42 | ||||
| 43 | + | func TestHandleClientNoticeSerializesReplacement(t *testing.T) { | |||
| 44 | + | d, sess, old, _ := newNoticeFixture(t, newNoticeClock()) | |||
| 45 | + | ||||
| 46 | + | enteredMutation := make(chan struct{}) | |||
| 47 | + | releaseMutation := make(chan struct{}) | |||
| 48 | + | d.notices.beforeClientNoticeMutation = func() { | |||
| 49 | + | close(enteredMutation) | |||
| 50 | + | <-releaseMutation | |||
| 51 | + | } | |||
| 52 | + | t.Cleanup(func() { | |||
| 53 | + | select { | |||
| 54 | + | case <-releaseMutation: | |||
| 55 | + | default: | |||
| 56 | + | close(releaseMutation) | |||
| 57 | + | } | |||
| 58 | + | }) | |||
| 59 | + | ||||
| 60 | + | noticeDone := make(chan struct{}) | |||
| 61 | + | go func() { | |||
| 62 | + | d.handleClientNotice(sess, old, ports.ClientNotice{Action: ports.ClientNoticeClipboardFallback}) | |||
| 63 | + | close(noticeDone) | |||
| 64 | + | }() | |||
| 65 | + | <-enteredMutation // old has validated ownership and holds routingMu | |||
| 66 | + | if d.notices.routingMu.TryLock() { | |||
| 67 | + | d.notices.routingMu.Unlock() | |||
| 68 | + | t.Fatal("client notice mutation must retain routingMu through publication") | |||
| 69 | + | } | |||
| 70 | + | ||||
| 71 | + | tr, _ := newCapturingTransport(t) | |||
| 72 | + | replaced := make(chan *attachedClient, 1) | |||
| 73 | + | go func() { | |||
| 74 | + | current, _ := d.attachClient(sess, tr, domain.Size{Cols: 80, Rows: 24}, attachClientOptions{}) | |||
| 75 | + | replaced <- current | |||
| 76 | + | }() | |||
| 77 | + | ||||
| 78 | + | // Releasing the mutation lets the already-validated notice publish before | |||
| 79 | + | // replacement can own the session. Thus no stale attachment can mutate a | |||
| 80 | + | // toast stack after sess.client changes. | |||
| 81 | + | close(releaseMutation) | |||
| 82 | + | <-noticeDone | |||
| 83 | + | current := <-replaced | |||
| 84 | + | ||||
| 85 | + | history := d.notices.history() | |||
| 86 | + | require.Len(t, history, 1) | |||
| 87 | + | require.Equal(t, domain.NoticeClipboard, history[0].Code) | |||
| 88 | + | oldToasts, _ := visibleToasts(old) | |||
| 89 | + | require.Len(t, oldToasts, 1) | |||
| 90 | + | require.Equal(t, domain.NoticeClipboard, oldToasts[0].Code) | |||
| 91 | + | newToasts, _ := visibleToasts(current) | |||
| 92 | + | require.Empty(t, newToasts) | |||
| 93 | + | sess.mu.Lock() | |||
| 94 | + | require.Same(t, current, sess.client) | |||
| 95 | + | sess.mu.Unlock() | |||
| 96 | + | } | |||
| 97 | + | ||||
| 98 | + | func TestHandleClientNoticeRejectsReplacedClient(t *testing.T) { | |||
| 99 | + | for _, tt := range []struct { | |||
| 100 | + | name string | |||
| 101 | + | setup func(*Daemon, *attachedClient, domain.SessionID) | |||
| 102 | + | notice ports.ClientNotice | |||
| 103 | + | wantToast domain.NoticeCode | |||
| 104 | + | }{ | |||
| 105 | + | { | |||
| 106 | + | name: "record", | |||
| 107 | + | notice: ports.ClientNotice{Action: ports.ClientNoticeClipboardFallback}, | |||
| 108 | + | }, | |||
| 109 | + | { | |||
| 110 | + | name: "dismiss", | |||
| 111 | + | setup: func(d *Daemon, ac *attachedClient, sid domain.SessionID) { | |||
| 112 | + | d.showToast(ac, notice(domain.NoticeConnection, "connection degraded", sid)) | |||
| 113 | + | }, | |||
| 114 | + | notice: ports.ClientNotice{Action: ports.ClientNoticeLinkConnected}, | |||
| 115 | + | wantToast: domain.NoticeConnection, | |||
| 116 | + | }, | |||
| 117 | + | } { | |||
| 118 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 119 | + | d, sess, old, _ := newNoticeFixture(t, newNoticeClock()) | |||
| 120 | + | tr, _ := newCapturingTransport(t) | |||
| 121 | + | current, _ := d.attachClient(sess, tr, domain.Size{Cols: 80, Rows: 24}, attachClientOptions{}) | |||
| 122 | + | if tt.setup != nil { | |||
| 123 | + | tt.setup(d, old, sess.id) | |||
| 124 | + | } | |||
| 125 | + | ||||
| 126 | + | d.handleClientNotice(sess, old, tt.notice) | |||
| 127 | + | ||||
| 128 | + | require.Empty(t, d.notices.history(), "a replaced client must not record notices") | |||
| 129 | + | oldToasts, _ := visibleToasts(old) | |||
| 130 | + | if tt.setup == nil { | |||
| 131 | + | require.Empty(t, oldToasts) | |||
| 132 | + | } else { | |||
| 133 | + | require.Len(t, oldToasts, 1) | |||
| 134 | + | require.Equal(t, tt.wantToast, oldToasts[0].Code, "a replaced client must not dismiss toasts") | |||
| 135 | + | } | |||
| 136 | + | currentToasts, _ := visibleToasts(current) | |||
| 137 | + | require.Empty(t, currentToasts) | |||
| 138 | + | }) | |||
| 139 | + | } | |||
| 140 | + | } | |||
| 141 | + | ||||
| 43 | func TestMalformedClientNoticeIsIgnored(t *testing.T) { | 142 | func TestMalformedClientNoticeIsIgnored(t *testing.T) { | ||
| 44 | d, _, ac, _ := newNoticeFixture(t, ¬iceClock{}) | 143 | d, _, ac, _ := newNoticeFixture(t, ¬iceClock{}) | ||
| 45 | tr, ok := ac.tr.(*portsmocks.MockTransport) | 144 | tr, ok := ac.tr.(*portsmocks.MockTransport) | ||
M internal/usecase/daemon/transactional_resize.go +13 -10
| @@ -71,13 +71,22 @@ 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. | |
| 74 | + | // Collect degradation across the loop and report once from the explicit | |
| 75 | + | // lock-free return points. notify acquires sess.mu/d.mu and repaints, so it | |
| 76 | + | // must never run while a pane's resizeMu is held. | |
| 77 | 77 | failed := 0 | |
| 78 | 78 | var lastErr error | |
| 79 | + | reportFailure := func() { | |
| 80 | + | if failed == 0 { | |
| 81 | + | return | |
| 82 | + | } | |
| 83 | + | // A failure can only come from a plan member, so members[0] is safe. | |
| 84 | + | d.notify(plan.members[0].session, domain.NoticeWarn, domain.NoticeResizeFailed, | |
| 85 | + | "pane resize failed; retrying in background", lastErr) | |
| 86 | + | } | |
| 79 | 87 | for i := range plan.members { | |
| 80 | 88 | if len(current) != 0 && !current[0]() { | |
| 89 | + | reportFailure() | |
| 81 | 90 | return false | |
| 82 | 91 | } | |
| 83 | 92 | m := &plan.members[i] | |
| @@ -106,13 +115,7 @@ func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |||
| 106 | 115 | } | |
| 107 | 116 | m.pane.resizeMu.Unlock() | |
| 108 | 117 | } | |
| 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 | - | } | |
| 118 | + | reportFailure() | |
| 116 | 119 | return true | |
| 117 | 120 | } | |
| 118 | 121 | ||
| @@ -71,13 +71,22 @@ 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 | 74 | + | // Collect degradation across the loop and report once from the explicit |
| 75 | - | // lock-free code: notify acquires sess.mu/d.mu and repaints, so it must never | 75 | + | // lock-free return points. notify acquires sess.mu/d.mu and repaints, so it |
| 76 | - | // run while a pane's resizeMu is held across the external PTY call below. | 76 | + | // must never run while a pane's resizeMu is held. |
| 77 | failed := 0 | 77 | failed := 0 | ||
| 78 | var lastErr error | 78 | var lastErr error | ||
| 79 | + | reportFailure := func() { | |||
| 80 | + | if failed == 0 { | |||
| 81 | + | return | |||
| 82 | + | } | |||
| 83 | + | // A failure can only come from a plan member, so members[0] is safe. | |||
| 84 | + | d.notify(plan.members[0].session, domain.NoticeWarn, domain.NoticeResizeFailed, | |||
| 85 | + | "pane resize failed; retrying in background", lastErr) | |||
| 86 | + | } | |||
| 79 | for i := range plan.members { | 87 | for i := range plan.members { | ||
| 80 | if len(current) != 0 && !current[0]() { | 88 | if len(current) != 0 && !current[0]() { | ||
| 89 | + | reportFailure() | |||
| 81 | return false | 90 | return false | ||
| 82 | } | 91 | } | ||
| 83 | m := &plan.members[i] | 92 | m := &plan.members[i] | ||
| @@ -106,13 +115,7 @@ func (d *Daemon) applyResize(plan *resizePlan, current ...func() bool) bool { | |||||
| 106 | } | 115 | } | ||
| 107 | m.pane.resizeMu.Unlock() | 116 | m.pane.resizeMu.Unlock() | ||
| 108 | } | 117 | } | ||
| 109 | - | if failed > 0 { | 118 | + | reportFailure() |
| 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 | - | } | |||
| 116 | return true | 119 | return true | ||
| 117 | } | 120 | } | ||
| 118 | 121 | ||||
M internal/usecase/daemon/transactional_resize_test.go +59 -14
| @@ -238,21 +238,66 @@ func TestTransactionalResizeApplyReportsFailureOncePerCall(t *testing.T) { | |||
| 238 | 238 | require.False(t, plan.members[0].ok, "failed member is not marked ok") | |
| 239 | 239 | }) | |
| 240 | 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)) | |
| 241 | + | for _, tt := range []struct { | |
| 242 | + | name string | |
| 243 | + | firstErr error | |
| 244 | + | secondErr error | |
| 245 | + | current func() bool | |
| 246 | + | wantResult bool | |
| 247 | + | wantCount int | |
| 248 | + | }{ | |
| 249 | + | { | |
| 250 | + | name: "normal completion reports one combined failure", | |
| 251 | + | firstErr: errors.New("first fails"), | |
| 252 | + | secondErr: errors.New("second fails"), | |
| 253 | + | wantResult: true, | |
| 254 | + | wantCount: 1, | |
| 255 | + | }, | |
| 256 | + | { | |
| 257 | + | name: "stale current after failure still reports once", | |
| 258 | + | firstErr: errors.New("first fails"), | |
| 259 | + | secondErr: errors.New("must not be resized"), | |
| 260 | + | current: func() func() bool { | |
| 261 | + | checks := 0 | |
| 262 | + | return func() bool { | |
| 263 | + | checks++ | |
| 264 | + | return checks == 1 | |
| 265 | + | } | |
| 266 | + | }(), | |
| 267 | + | wantResult: false, | |
| 268 | + | wantCount: 1, | |
| 269 | + | }, | |
| 270 | + | { | |
| 271 | + | name: "normal completion without failure does not report", | |
| 272 | + | wantResult: true, | |
| 273 | + | wantCount: 0, | |
| 274 | + | }, | |
| 275 | + | } { | |
| 276 | + | t.Run(tt.name, func(t *testing.T) { | |
| 277 | + | first := &transactionalResizePTY{errs: []error{tt.firstErr}} | |
| 278 | + | second := &transactionalResizePTY{errs: []error{tt.secondErr}} | |
| 279 | + | d, sess, _, _ := newManualSessionWithPTYs(t, first, second) | |
| 280 | + | tabs := sess.tabs | |
| 281 | + | plan := resizePlan{members: []resizeMember{ | |
| 282 | + | {session: sess, tab: tabs[0], pane: tabs[0].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |
| 283 | + | {session: sess, tab: tabs[1], pane: tabs[1].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |
| 284 | + | }} | |
| 285 | + | ||
| 286 | + | var got bool | |
| 287 | + | if tt.current == nil { | |
| 288 | + | got = d.applyResize(&plan) | |
| 289 | + | } else { | |
| 290 | + | got = d.applyResize(&plan, tt.current) | |
| 291 | + | } | |
| 292 | + | require.Equal(t, tt.wantResult, got) | |
| 252 | 293 | ||
| 253 | - | count, _ := countResizeFailed(d) | |
| 254 | - | require.Equal(t, 1, count, "one notice per applyResize call, not per failed member") | |
| 255 | - | }) | |
| 294 | + | count, _ := countResizeFailed(d) | |
| 295 | + | require.Equal(t, tt.wantCount, count, "one notice per applyResize call, including stale returns") | |
| 296 | + | if !tt.wantResult { | |
| 297 | + | require.Equal(t, []domain.Size(nil), second.requested(), "stale current check must stop before the next member") | |
| 298 | + | } | |
| 299 | + | }) | |
| 300 | + | } | |
| 256 | 301 | } | |
| 257 | 302 | ||
| 258 | 303 | // S3 acceptance: a failed member never publishes speculative parser/screen or | |
| @@ -238,21 +238,66 @@ func TestTransactionalResizeApplyReportsFailureOncePerCall(t *testing.T) { | |||||
| 238 | require.False(t, plan.members[0].ok, "failed member is not marked ok") | 238 | require.False(t, plan.members[0].ok, "failed member is not marked ok") | ||
| 239 | }) | 239 | }) | ||
| 240 | 240 | ||||
| 241 | - | t.Run("two failed members still yield one notice per call", func(t *testing.T) { | 241 | + | for _, tt := range []struct { |
| 242 | - | first := &transactionalResizePTY{errs: []error{errors.New("first fails")}} | 242 | + | name string |
| 243 | - | second := &transactionalResizePTY{errs: []error{errors.New("second fails")}} | 243 | + | firstErr error |
| 244 | - | d, sess, _, _ := newManualSessionWithPTYs(t, first, second) | 244 | + | secondErr error |
| 245 | - | tabs := sess.tabs | 245 | + | current func() bool |
| 246 | - | plan := resizePlan{members: []resizeMember{ | 246 | + | wantResult bool |
| 247 | - | {session: sess, tab: tabs[0], pane: tabs[0].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | 247 | + | wantCount int |
| 248 | - | {session: sess, tab: tabs[1], pane: tabs[1].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | 248 | + | }{ |
| 249 | - | }} | 249 | + | { |
| 250 | - | 250 | + | name: "normal completion reports one combined failure", | |
| 251 | - | require.True(t, d.applyResize(&plan)) | 251 | + | firstErr: errors.New("first fails"), |
| 252 | + | secondErr: errors.New("second fails"), | |||
| 253 | + | wantResult: true, | |||
| 254 | + | wantCount: 1, | |||
| 255 | + | }, | |||
| 256 | + | { | |||
| 257 | + | name: "stale current after failure still reports once", | |||
| 258 | + | firstErr: errors.New("first fails"), | |||
| 259 | + | secondErr: errors.New("must not be resized"), | |||
| 260 | + | current: func() func() bool { | |||
| 261 | + | checks := 0 | |||
| 262 | + | return func() bool { | |||
| 263 | + | checks++ | |||
| 264 | + | return checks == 1 | |||
| 265 | + | } | |||
| 266 | + | }(), | |||
| 267 | + | wantResult: false, | |||
| 268 | + | wantCount: 1, | |||
| 269 | + | }, | |||
| 270 | + | { | |||
| 271 | + | name: "normal completion without failure does not report", | |||
| 272 | + | wantResult: true, | |||
| 273 | + | wantCount: 0, | |||
| 274 | + | }, | |||
| 275 | + | } { | |||
| 276 | + | t.Run(tt.name, func(t *testing.T) { | |||
| 277 | + | first := &transactionalResizePTY{errs: []error{tt.firstErr}} | |||
| 278 | + | second := &transactionalResizePTY{errs: []error{tt.secondErr}} | |||
| 279 | + | d, sess, _, _ := newManualSessionWithPTYs(t, first, second) | |||
| 280 | + | tabs := sess.tabs | |||
| 281 | + | plan := resizePlan{members: []resizeMember{ | |||
| 282 | + | {session: sess, tab: tabs[0], pane: tabs[0].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |||
| 283 | + | {session: sess, tab: tabs[1], pane: tabs[1].focusedPane(), rect: domain.Rect{Width: 100, Height: 20}, retry: true}, | |||
| 284 | + | }} | |||
| 285 | + | ||||
| 286 | + | var got bool | |||
| 287 | + | if tt.current == nil { | |||
| 288 | + | got = d.applyResize(&plan) | |||
| 289 | + | } else { | |||
| 290 | + | got = d.applyResize(&plan, tt.current) | |||
| 291 | + | } | |||
| 292 | + | require.Equal(t, tt.wantResult, got) | |||
| 252 | 293 | ||||
| 253 | - | count, _ := countResizeFailed(d) | 294 | + | count, _ := countResizeFailed(d) |
| 254 | - | require.Equal(t, 1, count, "one notice per applyResize call, not per failed member") | 295 | + | require.Equal(t, tt.wantCount, count, "one notice per applyResize call, including stale returns") |
| 255 | - | }) | 296 | + | if !tt.wantResult { |
| 297 | + | require.Equal(t, []domain.Size(nil), second.requested(), "stale current check must stop before the next member") | |||
| 298 | + | } | |||
| 299 | + | }) | |||
| 300 | + | } | |||
| 256 | } | 301 | } | ||
| 257 | 302 | ||||
| 258 | // S3 acceptance: a failed member never publishes speculative parser/screen or | 303 | // S3 acceptance: a failed member never publishes speculative parser/screen or | ||