fix(remote): address picker review findings
- commit
- 56985fd6fc197bc31d5b7e507f359e2df701fc7c
- author
- brice · · 2026-08-07 12:27 UTC
- parent
- 17667bd
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
11 files · +140 -68
M internal/usecase/daemon/proxied_handshake_test.go +18 -0
| @@ -108,6 +108,24 @@ func TestProxiedPaintFlushesRuntimeMarks(t *testing.T) { | |||
| 108 | 108 | require.NotEmpty(t, observer.marks, "proxied rendering must flush observer marks after releasing sendMu") | |
| 109 | 109 | } | |
| 110 | 110 | ||
| 111 | + | func TestProxiedOutputStartCommitsOnlyAfterAcceptedFrame(t *testing.T) { | |
| 112 | + | d, sess, ac, _ := newManualSessionWithPTYs(t, newQuietPTY()) | |
| 113 | + | ac.renderMode = ports.RenderModeProxiedContent | |
| 114 | + | state, ok := captureLocalRenderState(sess, ac, renderCaptureRequest{reset: true}) | |
| 115 | + | require.True(t, ok) | |
| 116 | + | composed := composeProxiedContent(*state) | |
| 117 | + | ||
| 118 | + | state.attachment = &attachedClient{} | |
| 119 | + | ac.sendMu.Lock() | |
| 120 | + | require.False(t, d.emitFrame(sess, ac, state, composed)) | |
| 121 | + | require.False(t, ac.proxiedOutputStarted, "rejected proxied output must remain retryable") | |
| 122 | + | ||
| 123 | + | state.attachment = ac | |
| 124 | + | ac.sendMu.Lock() | |
| 125 | + | require.True(t, d.emitFrame(sess, ac, state, composed)) | |
| 126 | + | require.True(t, ac.proxiedOutputStarted, "accepted proxied output starts the output boundary") | |
| 127 | + | } | |
| 128 | + | ||
| 111 | 129 | func TestProxiedMetadataFallsBackFromStaleAttachmentTab(t *testing.T) { | |
| 112 | 130 | _, sess, ac, _ := newManualSessionWithPTYs(t, newQuietPTY()) | |
| 113 | 131 | sess.mu.Lock() | |
| @@ -108,6 +108,24 @@ func TestProxiedPaintFlushesRuntimeMarks(t *testing.T) { | |||||
| 108 | require.NotEmpty(t, observer.marks, "proxied rendering must flush observer marks after releasing sendMu") | 108 | require.NotEmpty(t, observer.marks, "proxied rendering must flush observer marks after releasing sendMu") | ||
| 109 | } | 109 | } | ||
| 110 | 110 | ||||
| 111 | + | func TestProxiedOutputStartCommitsOnlyAfterAcceptedFrame(t *testing.T) { | |||
| 112 | + | d, sess, ac, _ := newManualSessionWithPTYs(t, newQuietPTY()) | |||
| 113 | + | ac.renderMode = ports.RenderModeProxiedContent | |||
| 114 | + | state, ok := captureLocalRenderState(sess, ac, renderCaptureRequest{reset: true}) | |||
| 115 | + | require.True(t, ok) | |||
| 116 | + | composed := composeProxiedContent(*state) | |||
| 117 | + | ||||
| 118 | + | state.attachment = &attachedClient{} | |||
| 119 | + | ac.sendMu.Lock() | |||
| 120 | + | require.False(t, d.emitFrame(sess, ac, state, composed)) | |||
| 121 | + | require.False(t, ac.proxiedOutputStarted, "rejected proxied output must remain retryable") | |||
| 122 | + | ||||
| 123 | + | state.attachment = ac | |||
| 124 | + | ac.sendMu.Lock() | |||
| 125 | + | require.True(t, d.emitFrame(sess, ac, state, composed)) | |||
| 126 | + | require.True(t, ac.proxiedOutputStarted, "accepted proxied output starts the output boundary") | |||
| 127 | + | } | |||
| 128 | + | ||||
| 111 | func TestProxiedMetadataFallsBackFromStaleAttachmentTab(t *testing.T) { | 129 | func TestProxiedMetadataFallsBackFromStaleAttachmentTab(t *testing.T) { | ||
| 112 | _, sess, ac, _ := newManualSessionWithPTYs(t, newQuietPTY()) | 130 | _, sess, ac, _ := newManualSessionWithPTYs(t, newQuietPTY()) | ||
| 113 | sess.mu.Lock() | 131 | sess.mu.Lock() | ||
M internal/usecase/daemon/proxied_render.go +0 -1
| @@ -70,7 +70,6 @@ func (d *Daemon) paintProxiedContent(entry *session, ac *attachedClient, reset b | |||
| 70 | 70 | return paintRejected | |
| 71 | 71 | } | |
| 72 | 72 | composed := composeProxiedContent(*state) | |
| 73 | - | ac.proxiedOutputStarted = true | |
| 74 | 73 | if !d.emitFrame(entry, ac, state, composed, &marks) { | |
| 75 | 74 | return paintRejected | |
| 76 | 75 | } | |
| @@ -70,7 +70,6 @@ func (d *Daemon) paintProxiedContent(entry *session, ac *attachedClient, reset b | |||||
| 70 | return paintRejected | 70 | return paintRejected | ||
| 71 | } | 71 | } | ||
| 72 | composed := composeProxiedContent(*state) | 72 | composed := composeProxiedContent(*state) | ||
| 73 | - | ac.proxiedOutputStarted = true | |||
| 74 | if !d.emitFrame(entry, ac, state, composed, &marks) { | 73 | if !d.emitFrame(entry, ac, state, composed, &marks) { | ||
| 75 | return paintRejected | 74 | return paintRejected | ||
| 76 | } | 75 | } | ||
M internal/usecase/daemon/remote_attachment_transition_back_test.go +30 -0
| @@ -117,3 +117,33 @@ func TestBackSessionForAttachmentRejectsReboundRemoteToken(t *testing.T) { | |||
| 117 | 117 | require.True(t, view.attachmentRegistered(ac)) | |
| 118 | 118 | require.Same(t, local, ac.previousOwner.Get(), "a stale token must not consume valid history") | |
| 119 | 119 | } | |
| 120 | + | ||
| 121 | + | func TestBackSessionForAttachmentClearsRemoteHistoryAfterCurrentTransitionFailure(t *testing.T) { | |
| 122 | + | d, local, ac, view, token := remoteBackFixture(t) | |
| 123 | + | require.NoError(t, d.backSessionForAttachment(token)) | |
| 124 | + | localToken := local.attachmentToken(ac, ac.transport()) | |
| 125 | + | ||
| 126 | + | view.mu.Lock() | |
| 127 | + | view.closed = true | |
| 128 | + | view.mu.Unlock() | |
| 129 | + | err := d.backSessionForAttachment(localToken) | |
| 130 | + | ||
| 131 | + | require.Error(t, err) | |
| 132 | + | require.Same(t, local, ac.currentAttachmentOwner()) | |
| 133 | + | require.Nil(t, ac.previousOwner.Get(), "current transition failure clears consumed remote history") | |
| 134 | + | } | |
| 135 | + | ||
| 136 | + | func TestBackSessionForAttachmentKeepsRemoteHistoryAfterStaleTransitionCancellation(t *testing.T) { | |
| 137 | + | d, local, ac, view, token := remoteBackFixture(t) | |
| 138 | + | require.NoError(t, d.backSessionForAttachment(token)) | |
| 139 | + | localToken := local.attachmentToken(ac, ac.transport()) | |
| 140 | + | ac.connectionGeneration.Add(1) | |
| 141 | + | ||
| 142 | + | view.mu.Lock() | |
| 143 | + | view.closed = true | |
| 144 | + | view.mu.Unlock() | |
| 145 | + | require.NoError(t, d.backSessionForAttachment(localToken)) | |
| 146 | + | ||
| 147 | + | require.Same(t, local, ac.currentAttachmentOwner()) | |
| 148 | + | require.Same(t, view, ac.previousOwner.Get(), "stale transition cancellation preserves remote history") | |
| 149 | + | } | |
| @@ -117,3 +117,33 @@ func TestBackSessionForAttachmentRejectsReboundRemoteToken(t *testing.T) { | |||||
| 117 | require.True(t, view.attachmentRegistered(ac)) | 117 | require.True(t, view.attachmentRegistered(ac)) | ||
| 118 | require.Same(t, local, ac.previousOwner.Get(), "a stale token must not consume valid history") | 118 | require.Same(t, local, ac.previousOwner.Get(), "a stale token must not consume valid history") | ||
| 119 | } | 119 | } | ||
| 120 | + | ||||
| 121 | + | func TestBackSessionForAttachmentClearsRemoteHistoryAfterCurrentTransitionFailure(t *testing.T) { | |||
| 122 | + | d, local, ac, view, token := remoteBackFixture(t) | |||
| 123 | + | require.NoError(t, d.backSessionForAttachment(token)) | |||
| 124 | + | localToken := local.attachmentToken(ac, ac.transport()) | |||
| 125 | + | ||||
| 126 | + | view.mu.Lock() | |||
| 127 | + | view.closed = true | |||
| 128 | + | view.mu.Unlock() | |||
| 129 | + | err := d.backSessionForAttachment(localToken) | |||
| 130 | + | ||||
| 131 | + | require.Error(t, err) | |||
| 132 | + | require.Same(t, local, ac.currentAttachmentOwner()) | |||
| 133 | + | require.Nil(t, ac.previousOwner.Get(), "current transition failure clears consumed remote history") | |||
| 134 | + | } | |||
| 135 | + | ||||
| 136 | + | func TestBackSessionForAttachmentKeepsRemoteHistoryAfterStaleTransitionCancellation(t *testing.T) { | |||
| 137 | + | d, local, ac, view, token := remoteBackFixture(t) | |||
| 138 | + | require.NoError(t, d.backSessionForAttachment(token)) | |||
| 139 | + | localToken := local.attachmentToken(ac, ac.transport()) | |||
| 140 | + | ac.connectionGeneration.Add(1) | |||
| 141 | + | ||||
| 142 | + | view.mu.Lock() | |||
| 143 | + | view.closed = true | |||
| 144 | + | view.mu.Unlock() | |||
| 145 | + | require.NoError(t, d.backSessionForAttachment(localToken)) | |||
| 146 | + | ||||
| 147 | + | require.Same(t, local, ac.currentAttachmentOwner()) | |||
| 148 | + | require.Same(t, view, ac.previousOwner.Get(), "stale transition cancellation preserves remote history") | |||
| 149 | + | } | |||
M internal/usecase/daemon/remote_link.go +11 -28
| @@ -826,10 +826,17 @@ func (link *remoteLink) send(frame ports.Frame) error { | |||
| 826 | 826 | if link == nil || link.view == nil { | |
| 827 | 827 | return errRemoteViewUnavailable | |
| 828 | 828 | } | |
| 829 | - | link.sendMu.Lock() | |
| 830 | 829 | view := link.view | |
| 831 | 830 | view.mu.Lock() | |
| 832 | 831 | current := !view.closed && view.link == link && view.linkGeneration == link.generation && link.active | |
| 832 | + | view.mu.Unlock() | |
| 833 | + | if !current { | |
| 834 | + | return errRemoteViewStale | |
| 835 | + | } | |
| 836 | + | ||
| 837 | + | link.sendMu.Lock() | |
| 838 | + | view.mu.Lock() | |
| 839 | + | current = !view.closed && view.link == link && view.linkGeneration == link.generation && link.active | |
| 833 | 840 | transport := link.transport | |
| 834 | 841 | view.mu.Unlock() | |
| 835 | 842 | if !current || transport == nil { | |
| @@ -936,15 +943,7 @@ func (d *Daemon) markRemoteLinkUnavailable(link *remoteLink) { | |||
| 936 | 943 | if !current { | |
| 937 | 944 | return | |
| 938 | 945 | } | |
| 939 | - | if link.commands != nil { | |
| 940 | - | link.commands.FailGeneration(link.generation, errRemoteViewUnavailable) | |
| 941 | - | } | |
| 942 | - | if link.cancel != nil { | |
| 943 | - | link.cancel() | |
| 944 | - | } | |
| 945 | - | if link.transport != nil { | |
| 946 | - | _ = link.transport.Close() | |
| 947 | - | } | |
| 946 | + | interruptRemoteLink(link) | |
| 948 | 947 | d.repaintRemoteViewAttachments(view, attachments) | |
| 949 | 948 | if startReconnect { | |
| 950 | 949 | go d.reconnectRemoteView(view, link, reconnectGeneration, reconnectTarget, reconnectSize) | |
| @@ -966,15 +965,7 @@ func (d *Daemon) stopUnpublishedRemoteView(view *remoteView) { | |||
| 966 | 965 | signalRemoteViewMetadataChangedLocked(view) | |
| 967 | 966 | view.mu.Unlock() | |
| 968 | 967 | if link != nil { | |
| 969 | - | if link.commands != nil { | |
| 970 | - | link.commands.FailGeneration(link.generation, errRemoteViewUnavailable) | |
| 971 | - | } | |
| 972 | - | if link.cancel != nil { | |
| 973 | - | link.cancel() | |
| 974 | - | } | |
| 975 | - | if link.transport != nil { | |
| 976 | - | _ = link.transport.Close() | |
| 977 | - | } | |
| 968 | + | interruptRemoteLink(link) | |
| 978 | 969 | } | |
| 979 | 970 | } | |
| 980 | 971 | ||
| @@ -996,15 +987,7 @@ func (d *Daemon) stopRemoteViewLink(view *remoteView) *remoteLink { | |||
| 996 | 987 | } | |
| 997 | 988 | view.mu.Unlock() | |
| 998 | 989 | if link != nil { | |
| 999 | - | if link.commands != nil { | |
| 1000 | - | link.commands.FailGeneration(link.generation, errRemoteViewUnavailable) | |
| 1001 | - | } | |
| 1002 | - | if link.cancel != nil { | |
| 1003 | - | link.cancel() | |
| 1004 | - | } | |
| 1005 | - | if link.transport != nil { | |
| 1006 | - | _ = link.transport.Close() | |
| 1007 | - | } | |
| 990 | + | interruptRemoteLink(link) | |
| 1008 | 991 | } | |
| 1009 | 992 | return link | |
| 1010 | 993 | } | |
| @@ -826,10 +826,17 @@ func (link *remoteLink) send(frame ports.Frame) error { | |||||
| 826 | if link == nil || link.view == nil { | 826 | if link == nil || link.view == nil { | ||
| 827 | return errRemoteViewUnavailable | 827 | return errRemoteViewUnavailable | ||
| 828 | } | 828 | } | ||
| 829 | - | link.sendMu.Lock() | |||
| 830 | view := link.view | 829 | view := link.view | ||
| 831 | view.mu.Lock() | 830 | view.mu.Lock() | ||
| 832 | current := !view.closed && view.link == link && view.linkGeneration == link.generation && link.active | 831 | current := !view.closed && view.link == link && view.linkGeneration == link.generation && link.active | ||
| 832 | + | view.mu.Unlock() | |||
| 833 | + | if !current { | |||
| 834 | + | return errRemoteViewStale | |||
| 835 | + | } | |||
| 836 | + | ||||
| 837 | + | link.sendMu.Lock() | |||
| 838 | + | view.mu.Lock() | |||
| 839 | + | current = !view.closed && view.link == link && view.linkGeneration == link.generation && link.active | |||
| 833 | transport := link.transport | 840 | transport := link.transport | ||
| 834 | view.mu.Unlock() | 841 | view.mu.Unlock() | ||
| 835 | if !current || transport == nil { | 842 | if !current || transport == nil { | ||
| @@ -936,15 +943,7 @@ func (d *Daemon) markRemoteLinkUnavailable(link *remoteLink) { | |||||
| 936 | if !current { | 943 | if !current { | ||
| 937 | return | 944 | return | ||
| 938 | } | 945 | } | ||
| 939 | - | if link.commands != nil { | 946 | + | interruptRemoteLink(link) |
| 940 | - | link.commands.FailGeneration(link.generation, errRemoteViewUnavailable) | |||
| 941 | - | } | |||
| 942 | - | if link.cancel != nil { | |||
| 943 | - | link.cancel() | |||
| 944 | - | } | |||
| 945 | - | if link.transport != nil { | |||
| 946 | - | _ = link.transport.Close() | |||
| 947 | - | } | |||
| 948 | d.repaintRemoteViewAttachments(view, attachments) | 947 | d.repaintRemoteViewAttachments(view, attachments) | ||
| 949 | if startReconnect { | 948 | if startReconnect { | ||
| 950 | go d.reconnectRemoteView(view, link, reconnectGeneration, reconnectTarget, reconnectSize) | 949 | go d.reconnectRemoteView(view, link, reconnectGeneration, reconnectTarget, reconnectSize) | ||
| @@ -966,15 +965,7 @@ func (d *Daemon) stopUnpublishedRemoteView(view *remoteView) { | |||||
| 966 | signalRemoteViewMetadataChangedLocked(view) | 965 | signalRemoteViewMetadataChangedLocked(view) | ||
| 967 | view.mu.Unlock() | 966 | view.mu.Unlock() | ||
| 968 | if link != nil { | 967 | if link != nil { | ||
| 969 | - | if link.commands != nil { | 968 | + | interruptRemoteLink(link) |
| 970 | - | link.commands.FailGeneration(link.generation, errRemoteViewUnavailable) | |||
| 971 | - | } | |||
| 972 | - | if link.cancel != nil { | |||
| 973 | - | link.cancel() | |||
| 974 | - | } | |||
| 975 | - | if link.transport != nil { | |||
| 976 | - | _ = link.transport.Close() | |||
| 977 | - | } | |||
| 978 | } | 969 | } | ||
| 979 | } | 970 | } | ||
| 980 | 971 | ||||
| @@ -996,15 +987,7 @@ func (d *Daemon) stopRemoteViewLink(view *remoteView) *remoteLink { | |||||
| 996 | } | 987 | } | ||
| 997 | view.mu.Unlock() | 988 | view.mu.Unlock() | ||
| 998 | if link != nil { | 989 | if link != nil { | ||
| 999 | - | if link.commands != nil { | 990 | + | interruptRemoteLink(link) |
| 1000 | - | link.commands.FailGeneration(link.generation, errRemoteViewUnavailable) | |||
| 1001 | - | } | |||
| 1002 | - | if link.cancel != nil { | |||
| 1003 | - | link.cancel() | |||
| 1004 | - | } | |||
| 1005 | - | if link.transport != nil { | |||
| 1006 | - | _ = link.transport.Close() | |||
| 1007 | - | } | |||
| 1008 | } | 991 | } | ||
| 1009 | return link | 992 | return link | ||
| 1010 | } | 993 | } | ||
M internal/usecase/daemon/remote_link_test.go +17 -0
| @@ -270,7 +270,9 @@ func TestRemoteLinkAcceptedMetadataRepaintsEveryAttachedClientUnderLocalChrome(t | |||
| 270 | 270 | func TestRemoteLinkAppliesSideEffectOutputWithoutAdvancingStateOrAcking(t *testing.T) { | |
| 271 | 271 | _, view, link, _ := newRemoteMetadataLinkFixture(t) | |
| 272 | 272 | attachment, _ := attachRemoteMetadataClient(t, view) | |
| 273 | + | view.mu.Lock() | |
| 273 | 274 | before := view.output | |
| 275 | + | view.mu.Unlock() | |
| 274 | 276 | ack, reset, attachments, accepted := view.applyRemoteOutput(link, ports.Output{ | |
| 275 | 277 | Epoch: 1, Size: domain.Size{Cols: 80, Rows: 22}, Data: []byte("side effect"), | |
| 276 | 278 | }) | |
| @@ -284,6 +286,21 @@ func TestRemoteLinkAppliesSideEffectOutputWithoutAdvancingStateOrAcking(t *testi | |||
| 284 | 286 | view.mu.Unlock() | |
| 285 | 287 | } | |
| 286 | 288 | ||
| 289 | + | func TestRemoteLinkSendRejectsInactiveLinkBeforeTransportUse(t *testing.T) { | |
| 290 | + | _, view, link, transport := newRemoteMetadataLinkFixture(t) | |
| 291 | + | view.mu.Lock() | |
| 292 | + | link.active = false | |
| 293 | + | view.mu.Unlock() | |
| 294 | + | ||
| 295 | + | err := link.send(ports.Frame{Type: ports.MsgPing}) | |
| 296 | + | require.ErrorIs(t, err, errRemoteViewStale) | |
| 297 | + | select { | |
| 298 | + | case <-transport.sent: | |
| 299 | + | t.Fatal("stale remote link sent a frame") | |
| 300 | + | default: | |
| 301 | + | } | |
| 302 | + | } | |
| 303 | + | ||
| 287 | 304 | func TestRemoteLinkIgnoresDuplicateAndOlderMetadataWithoutStopping(t *testing.T) { | |
| 288 | 305 | d, view, link, transport := newRemoteMetadataLinkFixture(t) | |
| 289 | 306 | metadata := ports.SessionMeta{ | |
| @@ -270,7 +270,9 @@ func TestRemoteLinkAcceptedMetadataRepaintsEveryAttachedClientUnderLocalChrome(t | |||||
| 270 | func TestRemoteLinkAppliesSideEffectOutputWithoutAdvancingStateOrAcking(t *testing.T) { | 270 | func TestRemoteLinkAppliesSideEffectOutputWithoutAdvancingStateOrAcking(t *testing.T) { | ||
| 271 | _, view, link, _ := newRemoteMetadataLinkFixture(t) | 271 | _, view, link, _ := newRemoteMetadataLinkFixture(t) | ||
| 272 | attachment, _ := attachRemoteMetadataClient(t, view) | 272 | attachment, _ := attachRemoteMetadataClient(t, view) | ||
| 273 | + | view.mu.Lock() | |||
| 273 | before := view.output | 274 | before := view.output | ||
| 275 | + | view.mu.Unlock() | |||
| 274 | ack, reset, attachments, accepted := view.applyRemoteOutput(link, ports.Output{ | 276 | ack, reset, attachments, accepted := view.applyRemoteOutput(link, ports.Output{ | ||
| 275 | Epoch: 1, Size: domain.Size{Cols: 80, Rows: 22}, Data: []byte("side effect"), | 277 | Epoch: 1, Size: domain.Size{Cols: 80, Rows: 22}, Data: []byte("side effect"), | ||
| 276 | }) | 278 | }) | ||
| @@ -284,6 +286,21 @@ func TestRemoteLinkAppliesSideEffectOutputWithoutAdvancingStateOrAcking(t *testi | |||||
| 284 | view.mu.Unlock() | 286 | view.mu.Unlock() | ||
| 285 | } | 287 | } | ||
| 286 | 288 | ||||
| 289 | + | func TestRemoteLinkSendRejectsInactiveLinkBeforeTransportUse(t *testing.T) { | |||
| 290 | + | _, view, link, transport := newRemoteMetadataLinkFixture(t) | |||
| 291 | + | view.mu.Lock() | |||
| 292 | + | link.active = false | |||
| 293 | + | view.mu.Unlock() | |||
| 294 | + | ||||
| 295 | + | err := link.send(ports.Frame{Type: ports.MsgPing}) | |||
| 296 | + | require.ErrorIs(t, err, errRemoteViewStale) | |||
| 297 | + | select { | |||
| 298 | + | case <-transport.sent: | |||
| 299 | + | t.Fatal("stale remote link sent a frame") | |||
| 300 | + | default: | |||
| 301 | + | } | |||
| 302 | + | } | |||
| 303 | + | ||||
| 287 | func TestRemoteLinkIgnoresDuplicateAndOlderMetadataWithoutStopping(t *testing.T) { | 304 | func TestRemoteLinkIgnoresDuplicateAndOlderMetadataWithoutStopping(t *testing.T) { | ||
| 288 | d, view, link, transport := newRemoteMetadataLinkFixture(t) | 305 | d, view, link, transport := newRemoteMetadataLinkFixture(t) | ||
| 289 | metadata := ports.SessionMeta{ | 306 | metadata := ports.SessionMeta{ | ||
M internal/usecase/daemon/remote_view_tabs.go +2 -2
| @@ -60,7 +60,7 @@ func (d *Daemon) activateRemoteViewTab(ctx context.Context, view *remoteView, ta | |||
| 60 | 60 | return err | |
| 61 | 61 | } | |
| 62 | 62 | if slug == "" { | |
| 63 | - | return nil | |
| 63 | + | continue | |
| 64 | 64 | } | |
| 65 | 65 | if err := d.sendRemoteViewCommand(ctx, link, generation, slug); err != nil { | |
| 66 | 66 | return err | |
| @@ -129,7 +129,7 @@ func remoteViewRelativeTabCommand(view *remoteView, link *remoteLink, generation | |||
| 129 | 129 | } | |
| 130 | 130 | forward := (selectedIndex - activeIndex + len(view.metadata.Tabs)) % len(view.metadata.Tabs) | |
| 131 | 131 | backward := (activeIndex - selectedIndex + len(view.metadata.Tabs)) % len(view.metadata.Tabs) | |
| 132 | - | if forward == 0 || backward == 0 { | |
| 132 | + | if forward == 0 { | |
| 133 | 133 | return "", nil | |
| 134 | 134 | } | |
| 135 | 135 | if forward <= backward { | |
| @@ -60,7 +60,7 @@ func (d *Daemon) activateRemoteViewTab(ctx context.Context, view *remoteView, ta | |||||
| 60 | return err | 60 | return err | ||
| 61 | } | 61 | } | ||
| 62 | if slug == "" { | 62 | if slug == "" { | ||
| 63 | - | return nil | 63 | + | continue |
| 64 | } | 64 | } | ||
| 65 | if err := d.sendRemoteViewCommand(ctx, link, generation, slug); err != nil { | 65 | if err := d.sendRemoteViewCommand(ctx, link, generation, slug); err != nil { | ||
| 66 | return err | 66 | return err | ||
| @@ -129,7 +129,7 @@ func remoteViewRelativeTabCommand(view *remoteView, link *remoteLink, generation | |||||
| 129 | } | 129 | } | ||
| 130 | forward := (selectedIndex - activeIndex + len(view.metadata.Tabs)) % len(view.metadata.Tabs) | 130 | forward := (selectedIndex - activeIndex + len(view.metadata.Tabs)) % len(view.metadata.Tabs) | ||
| 131 | backward := (activeIndex - selectedIndex + len(view.metadata.Tabs)) % len(view.metadata.Tabs) | 131 | backward := (activeIndex - selectedIndex + len(view.metadata.Tabs)) % len(view.metadata.Tabs) | ||
| 132 | - | if forward == 0 || backward == 0 { | 132 | + | if forward == 0 { |
| 133 | return "", nil | 133 | return "", nil | ||
| 134 | } | 134 | } | ||
| 135 | if forward <= backward { | 135 | if forward <= backward { | ||
M internal/usecase/daemon/render_pipeline.go +5 -0
| @@ -566,6 +566,11 @@ func (d *Daemon) emitAttachmentFrame(owner attachmentOwner, ac *attachedClient, | |||
| 566 | 566 | // represents the captured frame. Lock panes only under sendMu and with no | |
| 567 | 567 | // session guard held. | |
| 568 | 568 | commitDamageReceipts(state.receipts) | |
| 569 | + | if ac.renderMode == ports.RenderModeProxiedContent { | |
| 570 | + | // A proxied output boundary is established only after preparation and | |
| 571 | + | // transport emission succeed, while sendMu still serializes the transition. | |
| 572 | + | ac.proxiedOutputStarted = true | |
| 573 | + | } | |
| 569 | 574 | if ac.renderStages.emit != nil { | |
| 570 | 575 | ac.renderStages.emit() | |
| 571 | 576 | } | |
| @@ -566,6 +566,11 @@ func (d *Daemon) emitAttachmentFrame(owner attachmentOwner, ac *attachedClient, | |||||
| 566 | // represents the captured frame. Lock panes only under sendMu and with no | 566 | // represents the captured frame. Lock panes only under sendMu and with no | ||
| 567 | // session guard held. | 567 | // session guard held. | ||
| 568 | commitDamageReceipts(state.receipts) | 568 | commitDamageReceipts(state.receipts) | ||
| 569 | + | if ac.renderMode == ports.RenderModeProxiedContent { | |||
| 570 | + | // A proxied output boundary is established only after preparation and | |||
| 571 | + | // transport emission succeed, while sendMu still serializes the transition. | |||
| 572 | + | ac.proxiedOutputStarted = true | |||
| 573 | + | } | |||
| 569 | if ac.renderStages.emit != nil { | 574 | if ac.renderStages.emit != nil { | ||
| 570 | ac.renderStages.emit() | 575 | ac.renderStages.emit() | ||
| 571 | } | 576 | } | ||
M internal/usecase/daemon/session_back.go +1 -0
| @@ -51,6 +51,7 @@ func (d *Daemon) backSessionForAttachment(token attachmentConnectionToken, after | |||
| 51 | 51 | if !token.current() { | |
| 52 | 52 | return nil | |
| 53 | 53 | } | |
| 54 | + | token.ac.clearPreviousOwnerIf(previous) | |
| 54 | 55 | return err | |
| 55 | 56 | } | |
| 56 | 57 | d.firstPaintForTransition(published) | |
| @@ -51,6 +51,7 @@ func (d *Daemon) backSessionForAttachment(token attachmentConnectionToken, after | |||||
| 51 | if !token.current() { | 51 | if !token.current() { | ||
| 52 | return nil | 52 | return nil | ||
| 53 | } | 53 | } | ||
| 54 | + | token.ac.clearPreviousOwnerIf(previous) | |||
| 54 | return err | 55 | return err | ||
| 55 | } | 56 | } | ||
| 56 | d.firstPaintForTransition(published) | 57 | d.firstPaintForTransition(published) | ||
M internal/usecase/picker/model_test.go +22 -22
| @@ -491,30 +491,30 @@ func TestSelectedIndexReportsRawSelectedRow(t *testing.T) { | |||
| 491 | 491 | require.Equal(t, 1, m.SelectedIndex()) | |
| 492 | 492 | } | |
| 493 | 493 | ||
| 494 | - | func TestStoppedSessionSelectableAndRendered(t *testing.T) { | |
| 495 | - | m := New([]SessionView{{ID: "stopped:work", Name: "work", Tabs: []TabEntry{{Name: ""}}, Stopped: true}}, SelectionConfig{Mode: SelectNavigationTab}) | |
| 496 | - | got, ok := m.Selected() | |
| 497 | - | require.True(t, ok) | |
| 498 | - | require.Equal(t, Target{Session: "stopped:work", Name: "work", TabIndex: 0, Stopped: true}, got) | |
| 499 | - | frame := m.Render(domain.Size{Cols: 24, Rows: 4}, Preview{}) | |
| 500 | - | require.Equal(t, 'w', frame.At(0, 0).Rune) | |
| 501 | - | require.Equal(t, '(', frame.At(5, 0).Rune) | |
| 502 | - | var label strings.Builder | |
| 503 | - | for _, cell := range frame.Row(0) { | |
| 504 | - | label.WriteRune(cell.Rune) | |
| 505 | - | } | |
| 506 | - | require.Contains(t, label.String(), "(down)") | |
| 507 | - | } | |
| 508 | - | ||
| 509 | - | func TestStoppedSessionReservesDownSuffix(t *testing.T) { | |
| 510 | - | m := New([]SessionView{{ID: "stopped:long", Name: "long-session", Tabs: []TabEntry{{}}, Stopped: true}}, SelectionConfig{Mode: SelectNavigationTab}) | |
| 494 | + | func TestStoppedSessionRendersDownSuffix(t *testing.T) { | |
| 495 | + | for _, tt := range []struct { | |
| 496 | + | name string | |
| 497 | + | sessionID domain.SessionID | |
| 498 | + | sessionName string | |
| 499 | + | size domain.Size | |
| 500 | + | }{ | |
| 501 | + | {name: "normal", sessionID: "stopped:work", sessionName: "work", size: domain.Size{Cols: 24, Rows: 4}}, | |
| 502 | + | {name: "constrained", sessionID: "stopped:long", sessionName: "long-session", size: domain.Size{Cols: 8, Rows: 2}}, | |
| 503 | + | } { | |
| 504 | + | t.Run(tt.name, func(t *testing.T) { | |
| 505 | + | m := New([]SessionView{{ID: tt.sessionID, Name: tt.sessionName, Tabs: []TabEntry{{Name: ""}}, Stopped: true}}, SelectionConfig{Mode: SelectNavigationTab}) | |
| 506 | + | got, ok := m.Selected() | |
| 507 | + | require.True(t, ok) | |
| 508 | + | require.Equal(t, Target{Session: tt.sessionID, Name: tt.sessionName, TabIndex: 0, Stopped: true}, got) | |
| 511 | 509 | ||
| 512 | - | frame := m.Render(domain.Size{Cols: 8, Rows: 2}, Preview{}) | |
| 513 | - | var label strings.Builder | |
| 514 | - | for _, cell := range frame.Row(0) { | |
| 515 | - | label.WriteRune(cell.Rune) | |
| 510 | + | frame := m.Render(tt.size, Preview{}) | |
| 511 | + | var label strings.Builder | |
| 512 | + | for _, cell := range frame.Row(0) { | |
| 513 | + | label.WriteRune(cell.Rune) | |
| 514 | + | } | |
| 515 | + | require.Contains(t, label.String(), "(down)") | |
| 516 | + | }) | |
| 516 | 517 | } | |
| 517 | - | require.Contains(t, label.String(), "(down)") | |
| 518 | 518 | } | |
| 519 | 519 | ||
| 520 | 520 | func TestRenderStopsStoppedRowsDimItalic(t *testing.T) { | |
| @@ -491,30 +491,30 @@ func TestSelectedIndexReportsRawSelectedRow(t *testing.T) { | |||||
| 491 | require.Equal(t, 1, m.SelectedIndex()) | 491 | require.Equal(t, 1, m.SelectedIndex()) | ||
| 492 | } | 492 | } | ||
| 493 | 493 | ||||
| 494 | - | func TestStoppedSessionSelectableAndRendered(t *testing.T) { | 494 | + | func TestStoppedSessionRendersDownSuffix(t *testing.T) { |
| 495 | - | m := New([]SessionView{{ID: "stopped:work", Name: "work", Tabs: []TabEntry{{Name: ""}}, Stopped: true}}, SelectionConfig{Mode: SelectNavigationTab}) | 495 | + | for _, tt := range []struct { |
| 496 | - | got, ok := m.Selected() | 496 | + | name string |
| 497 | - | require.True(t, ok) | 497 | + | sessionID domain.SessionID |
| 498 | - | require.Equal(t, Target{Session: "stopped:work", Name: "work", TabIndex: 0, Stopped: true}, got) | 498 | + | sessionName string |
| 499 | - | frame := m.Render(domain.Size{Cols: 24, Rows: 4}, Preview{}) | 499 | + | size domain.Size |
| 500 | - | require.Equal(t, 'w', frame.At(0, 0).Rune) | 500 | + | }{ |
| 501 | - | require.Equal(t, '(', frame.At(5, 0).Rune) | 501 | + | {name: "normal", sessionID: "stopped:work", sessionName: "work", size: domain.Size{Cols: 24, Rows: 4}}, |
| 502 | - | var label strings.Builder | 502 | + | {name: "constrained", sessionID: "stopped:long", sessionName: "long-session", size: domain.Size{Cols: 8, Rows: 2}}, |
| 503 | - | for _, cell := range frame.Row(0) { | 503 | + | } { |
| 504 | - | label.WriteRune(cell.Rune) | 504 | + | t.Run(tt.name, func(t *testing.T) { |
| 505 | - | } | 505 | + | m := New([]SessionView{{ID: tt.sessionID, Name: tt.sessionName, Tabs: []TabEntry{{Name: ""}}, Stopped: true}}, SelectionConfig{Mode: SelectNavigationTab}) |
| 506 | - | require.Contains(t, label.String(), "(down)") | 506 | + | got, ok := m.Selected() |
| 507 | - | } | 507 | + | require.True(t, ok) |
| 508 | - | 508 | + | require.Equal(t, Target{Session: tt.sessionID, Name: tt.sessionName, TabIndex: 0, Stopped: true}, got) | |
| 509 | - | func TestStoppedSessionReservesDownSuffix(t *testing.T) { | |||
| 510 | - | m := New([]SessionView{{ID: "stopped:long", Name: "long-session", Tabs: []TabEntry{{}}, Stopped: true}}, SelectionConfig{Mode: SelectNavigationTab}) | |||
| 511 | 509 | ||||
| 512 | - | frame := m.Render(domain.Size{Cols: 8, Rows: 2}, Preview{}) | 510 | + | frame := m.Render(tt.size, Preview{}) |
| 513 | - | var label strings.Builder | 511 | + | var label strings.Builder |
| 514 | - | for _, cell := range frame.Row(0) { | 512 | + | for _, cell := range frame.Row(0) { |
| 515 | - | label.WriteRune(cell.Rune) | 513 | + | label.WriteRune(cell.Rune) |
| 514 | + | } | |||
| 515 | + | require.Contains(t, label.String(), "(down)") | |||
| 516 | + | }) | |||
| 516 | } | 517 | } | ||
| 517 | - | require.Contains(t, label.String(), "(down)") | |||
| 518 | } | 518 | } | ||
| 519 | 519 | ||||
| 520 | func TestRenderStopsStoppedRowsDimItalic(t *testing.T) { | 520 | func TestRenderStopsStoppedRowsDimItalic(t *testing.T) { | ||
M scripts/remote-picker-harness/main.go +13 -15
| @@ -996,23 +996,21 @@ func assertNoDirectHandoff(tr *harnessTransport) error { | |||
| 996 | 996 | if tr == nil || tr.probe == nil { | |
| 997 | 997 | return errors.New("local picker transport has no visual probe") | |
| 998 | 998 | } | |
| 999 | - | // Inspect frames already queued by the receiver, retaining non-handoff | |
| 1000 | - | // frames for the next receiveWithTimeout call. | |
| 1001 | - | for { | |
| 1002 | - | select { | |
| 1003 | - | case outcome := <-tr.frames: | |
| 1004 | - | if outcome.frame.Type == ports.MsgAttachTarget { | |
| 1005 | - | tr.probe.recordIncoming(outcome.frame) | |
| 1006 | - | } else { | |
| 1007 | - | tr.queued = append(tr.queued, outcome) | |
| 1008 | - | } | |
| 1009 | - | default: | |
| 1010 | - | if tr.probe.unexpectedHandoffs != 0 { | |
| 1011 | - | return fmt.Errorf("picker emitted %d direct MsgAttachTarget handoff frame(s)", tr.probe.unexpectedHandoffs) | |
| 1012 | - | } | |
| 1013 | - | return nil | |
| 999 | + | // Inspect only the frames already queued at assertion start, retaining | |
| 1000 | + | // non-handoff frames for the next receiveWithTimeout call. | |
| 1001 | + | queued := len(tr.frames) | |
| 1002 | + | for range queued { | |
| 1003 | + | outcome := <-tr.frames | |
| 1004 | + | if outcome.frame.Type == ports.MsgAttachTarget { | |
| 1005 | + | tr.probe.recordIncoming(outcome.frame) | |
| 1006 | + | } else { | |
| 1007 | + | tr.queued = append(tr.queued, outcome) | |
| 1014 | 1008 | } | |
| 1015 | 1009 | } | |
| 1010 | + | if tr.probe.unexpectedHandoffs != 0 { | |
| 1011 | + | return fmt.Errorf("picker emitted %d direct MsgAttachTarget handoff frame(s)", tr.probe.unexpectedHandoffs) | |
| 1012 | + | } | |
| 1013 | + | return nil | |
| 1016 | 1014 | } | |
| 1017 | 1015 | ||
| 1018 | 1016 | func receiveWithTimeout(ctx context.Context, tr *harnessTransport) (ports.Frame, error) { | |
| @@ -996,23 +996,21 @@ func assertNoDirectHandoff(tr *harnessTransport) error { | |||||
| 996 | if tr == nil || tr.probe == nil { | 996 | if tr == nil || tr.probe == nil { | ||
| 997 | return errors.New("local picker transport has no visual probe") | 997 | return errors.New("local picker transport has no visual probe") | ||
| 998 | } | 998 | } | ||
| 999 | - | // Inspect frames already queued by the receiver, retaining non-handoff | 999 | + | // Inspect only the frames already queued at assertion start, retaining |
| 1000 | - | // frames for the next receiveWithTimeout call. | 1000 | + | // non-handoff frames for the next receiveWithTimeout call. |
| 1001 | - | for { | 1001 | + | queued := len(tr.frames) |
| 1002 | - | select { | 1002 | + | for range queued { |
| 1003 | - | case outcome := <-tr.frames: | 1003 | + | outcome := <-tr.frames |
| 1004 | - | if outcome.frame.Type == ports.MsgAttachTarget { | 1004 | + | if outcome.frame.Type == ports.MsgAttachTarget { |
| 1005 | - | tr.probe.recordIncoming(outcome.frame) | 1005 | + | tr.probe.recordIncoming(outcome.frame) |
| 1006 | - | } else { | 1006 | + | } else { |
| 1007 | - | tr.queued = append(tr.queued, outcome) | 1007 | + | tr.queued = append(tr.queued, outcome) |
| 1008 | - | } | |||
| 1009 | - | default: | |||
| 1010 | - | if tr.probe.unexpectedHandoffs != 0 { | |||
| 1011 | - | return fmt.Errorf("picker emitted %d direct MsgAttachTarget handoff frame(s)", tr.probe.unexpectedHandoffs) | |||
| 1012 | - | } | |||
| 1013 | - | return nil | |||
| 1014 | } | 1008 | } | ||
| 1015 | } | 1009 | } | ||
| 1010 | + | if tr.probe.unexpectedHandoffs != 0 { | |||
| 1011 | + | return fmt.Errorf("picker emitted %d direct MsgAttachTarget handoff frame(s)", tr.probe.unexpectedHandoffs) | |||
| 1012 | + | } | |||
| 1013 | + | return nil | |||
| 1016 | } | 1014 | } | ||
| 1017 | 1015 | ||||
| 1018 | func receiveWithTimeout(ctx context.Context, tr *harnessTransport) (ports.Frame, error) { | 1016 | func receiveWithTimeout(ctx context.Context, tr *harnessTransport) (ports.Frame, error) { | ||
M scripts/remote-picker-harness/main_test.go +21 -0
| @@ -4,6 +4,8 @@ import ( | |||
| 4 | 4 | "errors" | |
| 5 | 5 | "strings" | |
| 6 | 6 | "testing" | |
| 7 | + | ||
| 8 | + | "github.com/bnema/vev/internal/ports" | |
| 7 | 9 | ) | |
| 8 | 10 | ||
| 9 | 11 | func TestBoundedStderrRetainsAtMostDiagnosticLimit(t *testing.T) { | |
| @@ -42,3 +44,22 @@ func TestRunCapturedCommandIncludesStderr(t *testing.T) { | |||
| 42 | 44 | t.Fatalf("command error = %v", err) | |
| 43 | 45 | } | |
| 44 | 46 | } | |
| 47 | + | ||
| 48 | + | func TestAssertNoDirectHandoffProcessesInitialFramesAndRetainsOutput(t *testing.T) { | |
| 49 | + | tr := &harnessTransport{ | |
| 50 | + | probe: &visualProbe{}, | |
| 51 | + | frames: make(chan harnessFrame, 2), | |
| 52 | + | } | |
| 53 | + | tr.frames <- harnessFrame{frame: ports.Frame{Type: ports.MsgOutput}} | |
| 54 | + | tr.frames <- harnessFrame{frame: ports.Frame{Type: ports.MsgAttachTarget}} | |
| 55 | + | ||
| 56 | + | if err := assertNoDirectHandoff(tr); err == nil { | |
| 57 | + | t.Fatal("direct handoff was not rejected") | |
| 58 | + | } | |
| 59 | + | if len(tr.queued) != 1 || tr.queued[0].frame.Type != ports.MsgOutput { | |
| 60 | + | t.Fatalf("queued frames = %#v, want one retained output", tr.queued) | |
| 61 | + | } | |
| 62 | + | if tr.probe.unexpectedHandoffs != 1 { | |
| 63 | + | t.Fatalf("unexpected handoffs = %d, want 1", tr.probe.unexpectedHandoffs) | |
| 64 | + | } | |
| 65 | + | } | |
| @@ -4,6 +4,8 @@ import ( | |||||
| 4 | "errors" | 4 | "errors" | ||
| 5 | "strings" | 5 | "strings" | ||
| 6 | "testing" | 6 | "testing" | ||
| 7 | + | ||||
| 8 | + | "github.com/bnema/vev/internal/ports" | |||
| 7 | ) | 9 | ) | ||
| 8 | 10 | ||||
| 9 | func TestBoundedStderrRetainsAtMostDiagnosticLimit(t *testing.T) { | 11 | func TestBoundedStderrRetainsAtMostDiagnosticLimit(t *testing.T) { | ||
| @@ -42,3 +44,22 @@ func TestRunCapturedCommandIncludesStderr(t *testing.T) { | |||||
| 42 | t.Fatalf("command error = %v", err) | 44 | t.Fatalf("command error = %v", err) | ||
| 43 | } | 45 | } | ||
| 44 | } | 46 | } | ||
| 47 | + | ||||
| 48 | + | func TestAssertNoDirectHandoffProcessesInitialFramesAndRetainsOutput(t *testing.T) { | |||
| 49 | + | tr := &harnessTransport{ | |||
| 50 | + | probe: &visualProbe{}, | |||
| 51 | + | frames: make(chan harnessFrame, 2), | |||
| 52 | + | } | |||
| 53 | + | tr.frames <- harnessFrame{frame: ports.Frame{Type: ports.MsgOutput}} | |||
| 54 | + | tr.frames <- harnessFrame{frame: ports.Frame{Type: ports.MsgAttachTarget}} | |||
| 55 | + | ||||
| 56 | + | if err := assertNoDirectHandoff(tr); err == nil { | |||
| 57 | + | t.Fatal("direct handoff was not rejected") | |||
| 58 | + | } | |||
| 59 | + | if len(tr.queued) != 1 || tr.queued[0].frame.Type != ports.MsgOutput { | |||
| 60 | + | t.Fatalf("queued frames = %#v, want one retained output", tr.queued) | |||
| 61 | + | } | |||
| 62 | + | if tr.probe.unexpectedHandoffs != 1 { | |||
| 63 | + | t.Fatalf("unexpected handoffs = %d, want 1", tr.probe.unexpectedHandoffs) | |||
| 64 | + | } | |||
| 65 | + | } | |||