Merge pull request #43 from bnema/improve/003-session-steal-render-guard
fix(daemon): skip stale session paint
2 files · +112 -0
M internal/usecase/daemon/picker_test.go +100 -0
| @@ -184,6 +184,106 @@ func TestPickerCrossSessionSwitchDetachesExistingClient(t *testing.T) { | |||
| 184 | 184 | awaitFrame(t, sends1, ports.MsgOutput) | |
| 185 | 185 | } | |
| 186 | 186 | ||
| 187 | + | func TestPickerStalePaintAfterSessionSwitchSendsNoFrame(t *testing.T) { | |
| 188 | + | p1, releasePTY1 := newBlockingPTY(t) | |
| 189 | + | p2, releasePTY2 := newBlockingPTY(t) | |
| 190 | + | defer releasePTY1() | |
| 191 | + | defer releasePTY2() | |
| 192 | + | d := newTestDaemon(t, nil, stubClock{}) | |
| 193 | + | tr1, sends1 := newCapturingTransport(t) | |
| 194 | + | tr2, _ := newCapturingTransport(t) | |
| 195 | + | ac1 := &attachedClient{tr: tr1, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}} | |
| 196 | + | ac1.initOverlays() | |
| 197 | + | ac2 := &attachedClient{tr: tr2, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}} | |
| 198 | + | ac2.initOverlays() | |
| 199 | + | sctx1, cancel1 := context.WithCancel(d.serveCtx) | |
| 200 | + | sctx2, cancel2 := context.WithCancel(d.serveCtx) | |
| 201 | + | defer cancel1() | |
| 202 | + | defer cancel2() | |
| 203 | + | sess1 := &session{id: "s1", name: "alpha", ephemeral: true, ctx: sctx1, cancel: cancel1, tabs: []*tab{newTestTabWithContext(p1, sctx1, cancel1)}, client: ac1} | |
| 204 | + | sess2 := &session{id: "s2", name: "beta", ctx: sctx2, cancel: cancel2, tabs: []*tab{newTestTabWithContext(p2, sctx2, cancel2)}, client: ac2} | |
| 205 | + | ac1.setSession(sess1) | |
| 206 | + | ac2.setSession(sess2) | |
| 207 | + | d.sessions[sess1.id] = sess1 | |
| 208 | + | d.sessions[sess2.id] = sess2 | |
| 209 | + | ||
| 210 | + | d.firstPaint(sess1, ac1, ac1.size) | |
| 211 | + | awaitFrame(t, sends1, ports.MsgOutput) | |
| 212 | + | d.stealClientForTarget(sess1, ac1, sess2, picker.Target{Session: sess2.id}) | |
| 213 | + | d.firstPaint(sess2, ac1, ac1.size) | |
| 214 | + | awaitFrame(t, sends1, ports.MsgOutput) | |
| 215 | + | require.Same(t, sess2, ac1.currentSession()) | |
| 216 | + | for len(sends1) > 0 { | |
| 217 | + | <-sends1 | |
| 218 | + | } | |
| 219 | + | oldPane := sess1.tabs[0].focusedPane() | |
| 220 | + | oldPane.screen.Write([]byte("stale-damage")) | |
| 221 | + | require.NotEmpty(t, oldPane.screen.Damage()) | |
| 222 | + | ||
| 223 | + | d.paint(sess1, ac1, false) | |
| 224 | + | ||
| 225 | + | require.Zero(t, len(sends1), "stale paint from old session sent a frame") | |
| 226 | + | require.NotEmpty(t, oldPane.screen.Damage(), "stale paint from old session consumed damage") | |
| 227 | + | } | |
| 228 | + | ||
| 229 | + | func TestPickerSessionSwitchWaitsForInFlightPaintSend(t *testing.T) { | |
| 230 | + | p1, releasePTY1 := newBlockingPTY(t) | |
| 231 | + | p2, releasePTY2 := newBlockingPTY(t) | |
| 232 | + | defer releasePTY1() | |
| 233 | + | defer releasePTY2() | |
| 234 | + | d := newTestDaemon(t, nil, stubClock{}) | |
| 235 | + | enteredSend := make(chan struct{}) | |
| 236 | + | releaseSend := make(chan struct{}) | |
| 237 | + | tr := portsmocks.NewMockTransport(t) | |
| 238 | + | tr.EXPECT().Send(mock.Anything).RunAndReturn(func(ports.Frame) error { | |
| 239 | + | close(enteredSend) | |
| 240 | + | <-releaseSend | |
| 241 | + | return nil | |
| 242 | + | }).Once() | |
| 243 | + | tr.EXPECT().Close().Return(nil).Maybe() | |
| 244 | + | ac := &attachedClient{tr: tr, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}} | |
| 245 | + | ac.initOverlays() | |
| 246 | + | sctx1, cancel1 := context.WithCancel(d.serveCtx) | |
| 247 | + | sctx2, cancel2 := context.WithCancel(d.serveCtx) | |
| 248 | + | defer cancel1() | |
| 249 | + | defer cancel2() | |
| 250 | + | sess1 := &session{id: "s1", name: "alpha", ctx: sctx1, cancel: cancel1, tabs: []*tab{newTestTabWithContext(p1, sctx1, cancel1)}, client: ac} | |
| 251 | + | sess2 := &session{id: "s2", name: "beta", ctx: sctx2, cancel: cancel2, tabs: []*tab{newTestTabWithContext(p2, sctx2, cancel2)}} | |
| 252 | + | ac.setSession(sess1) | |
| 253 | + | ||
| 254 | + | sess1.tabs[0].focusedPane().screen.Write([]byte("paint while switching")) | |
| 255 | + | paintDone := make(chan struct{}) | |
| 256 | + | go func() { | |
| 257 | + | d.paint(sess1, ac, false) | |
| 258 | + | close(paintDone) | |
| 259 | + | }() | |
| 260 | + | <-enteredSend | |
| 261 | + | ||
| 262 | + | switchDone := make(chan struct{}) | |
| 263 | + | go func() { | |
| 264 | + | ac.setSession(sess2) | |
| 265 | + | close(switchDone) | |
| 266 | + | }() | |
| 267 | + | select { | |
| 268 | + | case <-switchDone: | |
| 269 | + | t.Fatal("session switch completed while paint Send was still in flight") | |
| 270 | + | case <-time.After(50 * time.Millisecond): | |
| 271 | + | } | |
| 272 | + | ||
| 273 | + | close(releaseSend) | |
| 274 | + | select { | |
| 275 | + | case <-paintDone: | |
| 276 | + | case <-time.After(time.Second): | |
| 277 | + | t.Fatal("paint did not finish after Send was released") | |
| 278 | + | } | |
| 279 | + | select { | |
| 280 | + | case <-switchDone: | |
| 281 | + | case <-time.After(time.Second): | |
| 282 | + | t.Fatal("session switch did not complete after paint finished") | |
| 283 | + | } | |
| 284 | + | require.Same(t, sess2, ac.currentSession()) | |
| 285 | + | } | |
| 286 | + | ||
| 187 | 287 | func TestPickerCrossSessionSwitchCopiesTerminalEnvForFutureTabs(t *testing.T) { | |
| 188 | 288 | p1, releasePTY1 := newBlockingPTY(t) | |
| 189 | 289 | p2, releasePTY2 := newBlockingPTY(t) | |
| @@ -184,6 +184,106 @@ func TestPickerCrossSessionSwitchDetachesExistingClient(t *testing.T) { | |||||
| 184 | awaitFrame(t, sends1, ports.MsgOutput) | 184 | awaitFrame(t, sends1, ports.MsgOutput) | ||
| 185 | } | 185 | } | ||
| 186 | 186 | ||||
| 187 | + | func TestPickerStalePaintAfterSessionSwitchSendsNoFrame(t *testing.T) { | |||
| 188 | + | p1, releasePTY1 := newBlockingPTY(t) | |||
| 189 | + | p2, releasePTY2 := newBlockingPTY(t) | |||
| 190 | + | defer releasePTY1() | |||
| 191 | + | defer releasePTY2() | |||
| 192 | + | d := newTestDaemon(t, nil, stubClock{}) | |||
| 193 | + | tr1, sends1 := newCapturingTransport(t) | |||
| 194 | + | tr2, _ := newCapturingTransport(t) | |||
| 195 | + | ac1 := &attachedClient{tr: tr1, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}} | |||
| 196 | + | ac1.initOverlays() | |||
| 197 | + | ac2 := &attachedClient{tr: tr2, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}} | |||
| 198 | + | ac2.initOverlays() | |||
| 199 | + | sctx1, cancel1 := context.WithCancel(d.serveCtx) | |||
| 200 | + | sctx2, cancel2 := context.WithCancel(d.serveCtx) | |||
| 201 | + | defer cancel1() | |||
| 202 | + | defer cancel2() | |||
| 203 | + | sess1 := &session{id: "s1", name: "alpha", ephemeral: true, ctx: sctx1, cancel: cancel1, tabs: []*tab{newTestTabWithContext(p1, sctx1, cancel1)}, client: ac1} | |||
| 204 | + | sess2 := &session{id: "s2", name: "beta", ctx: sctx2, cancel: cancel2, tabs: []*tab{newTestTabWithContext(p2, sctx2, cancel2)}, client: ac2} | |||
| 205 | + | ac1.setSession(sess1) | |||
| 206 | + | ac2.setSession(sess2) | |||
| 207 | + | d.sessions[sess1.id] = sess1 | |||
| 208 | + | d.sessions[sess2.id] = sess2 | |||
| 209 | + | ||||
| 210 | + | d.firstPaint(sess1, ac1, ac1.size) | |||
| 211 | + | awaitFrame(t, sends1, ports.MsgOutput) | |||
| 212 | + | d.stealClientForTarget(sess1, ac1, sess2, picker.Target{Session: sess2.id}) | |||
| 213 | + | d.firstPaint(sess2, ac1, ac1.size) | |||
| 214 | + | awaitFrame(t, sends1, ports.MsgOutput) | |||
| 215 | + | require.Same(t, sess2, ac1.currentSession()) | |||
| 216 | + | for len(sends1) > 0 { | |||
| 217 | + | <-sends1 | |||
| 218 | + | } | |||
| 219 | + | oldPane := sess1.tabs[0].focusedPane() | |||
| 220 | + | oldPane.screen.Write([]byte("stale-damage")) | |||
| 221 | + | require.NotEmpty(t, oldPane.screen.Damage()) | |||
| 222 | + | ||||
| 223 | + | d.paint(sess1, ac1, false) | |||
| 224 | + | ||||
| 225 | + | require.Zero(t, len(sends1), "stale paint from old session sent a frame") | |||
| 226 | + | require.NotEmpty(t, oldPane.screen.Damage(), "stale paint from old session consumed damage") | |||
| 227 | + | } | |||
| 228 | + | ||||
| 229 | + | func TestPickerSessionSwitchWaitsForInFlightPaintSend(t *testing.T) { | |||
| 230 | + | p1, releasePTY1 := newBlockingPTY(t) | |||
| 231 | + | p2, releasePTY2 := newBlockingPTY(t) | |||
| 232 | + | defer releasePTY1() | |||
| 233 | + | defer releasePTY2() | |||
| 234 | + | d := newTestDaemon(t, nil, stubClock{}) | |||
| 235 | + | enteredSend := make(chan struct{}) | |||
| 236 | + | releaseSend := make(chan struct{}) | |||
| 237 | + | tr := portsmocks.NewMockTransport(t) | |||
| 238 | + | tr.EXPECT().Send(mock.Anything).RunAndReturn(func(ports.Frame) error { | |||
| 239 | + | close(enteredSend) | |||
| 240 | + | <-releaseSend | |||
| 241 | + | return nil | |||
| 242 | + | }).Once() | |||
| 243 | + | tr.EXPECT().Close().Return(nil).Maybe() | |||
| 244 | + | ac := &attachedClient{tr: tr, rend: renderer.New(renderer.Capabilities{}), size: domain.Size{Cols: 80, Rows: 24}} | |||
| 245 | + | ac.initOverlays() | |||
| 246 | + | sctx1, cancel1 := context.WithCancel(d.serveCtx) | |||
| 247 | + | sctx2, cancel2 := context.WithCancel(d.serveCtx) | |||
| 248 | + | defer cancel1() | |||
| 249 | + | defer cancel2() | |||
| 250 | + | sess1 := &session{id: "s1", name: "alpha", ctx: sctx1, cancel: cancel1, tabs: []*tab{newTestTabWithContext(p1, sctx1, cancel1)}, client: ac} | |||
| 251 | + | sess2 := &session{id: "s2", name: "beta", ctx: sctx2, cancel: cancel2, tabs: []*tab{newTestTabWithContext(p2, sctx2, cancel2)}} | |||
| 252 | + | ac.setSession(sess1) | |||
| 253 | + | ||||
| 254 | + | sess1.tabs[0].focusedPane().screen.Write([]byte("paint while switching")) | |||
| 255 | + | paintDone := make(chan struct{}) | |||
| 256 | + | go func() { | |||
| 257 | + | d.paint(sess1, ac, false) | |||
| 258 | + | close(paintDone) | |||
| 259 | + | }() | |||
| 260 | + | <-enteredSend | |||
| 261 | + | ||||
| 262 | + | switchDone := make(chan struct{}) | |||
| 263 | + | go func() { | |||
| 264 | + | ac.setSession(sess2) | |||
| 265 | + | close(switchDone) | |||
| 266 | + | }() | |||
| 267 | + | select { | |||
| 268 | + | case <-switchDone: | |||
| 269 | + | t.Fatal("session switch completed while paint Send was still in flight") | |||
| 270 | + | case <-time.After(50 * time.Millisecond): | |||
| 271 | + | } | |||
| 272 | + | ||||
| 273 | + | close(releaseSend) | |||
| 274 | + | select { | |||
| 275 | + | case <-paintDone: | |||
| 276 | + | case <-time.After(time.Second): | |||
| 277 | + | t.Fatal("paint did not finish after Send was released") | |||
| 278 | + | } | |||
| 279 | + | select { | |||
| 280 | + | case <-switchDone: | |||
| 281 | + | case <-time.After(time.Second): | |||
| 282 | + | t.Fatal("session switch did not complete after paint finished") | |||
| 283 | + | } | |||
| 284 | + | require.Same(t, sess2, ac.currentSession()) | |||
| 285 | + | } | |||
| 286 | + | ||||
| 187 | func TestPickerCrossSessionSwitchCopiesTerminalEnvForFutureTabs(t *testing.T) { | 287 | func TestPickerCrossSessionSwitchCopiesTerminalEnvForFutureTabs(t *testing.T) { | ||
| 188 | p1, releasePTY1 := newBlockingPTY(t) | 288 | p1, releasePTY1 := newBlockingPTY(t) | ||
| 189 | p2, releasePTY2 := newBlockingPTY(t) | 289 | p2, releasePTY2 := newBlockingPTY(t) | ||
M internal/usecase/daemon/render.go +12 -0
| @@ -280,6 +280,10 @@ func (d *Daemon) paint(sess *session, ac *attachedClient, reset bool) { | |||
| 280 | 280 | ||
| 281 | 281 | ac.initOverlays() | |
| 282 | 282 | ac.sendMu.Lock() | |
| 283 | + | if ac.currentSession() != sess { | |
| 284 | + | ac.sendMu.Unlock() | |
| 285 | + | return | |
| 286 | + | } | |
| 283 | 287 | // Ack-gated coalescing: while the client is at least maxUnackedOutputStates | |
| 284 | 288 | // behind, skip composing another diff and mark the paint deferred. The | |
| 285 | 289 | // MsgOutputAck handler flushes one cumulative paint once acks catch up. | |
| @@ -364,6 +368,13 @@ func (d *Daemon) paint(sess *session, ac *attachedClient, reset bool) { | |||
| 364 | 368 | p.mu.Lock() | |
| 365 | 369 | desiredCursor := desiredCursorOut(p.screen, cursorContent, !cursorVisible || overlays.copyActive || overlays.copySearchModel != nil || overlays.pickerActive || overlays.paletteActive || overlays.promptActive) | |
| 366 | 370 | p.mu.Unlock() | |
| 371 | + | ac.sess.mu.Lock() | |
| 372 | + | if ac.sess.v != sess { | |
| 373 | + | ac.sess.mu.Unlock() | |
| 374 | + | tb.mu.Unlock() | |
| 375 | + | ac.sendMu.Unlock() | |
| 376 | + | return | |
| 377 | + | } | |
| 367 | 378 | data, err := ac.rend.Draw(frame, damage) | |
| 368 | 379 | var cursorTail []byte | |
| 369 | 380 | if err == nil { | |
| @@ -389,6 +400,7 @@ func (d *Daemon) paint(sess *session, ac *attachedClient, reset bool) { | |||
| 389 | 400 | } | |
| 390 | 401 | } | |
| 391 | 402 | } | |
| 403 | + | ac.sess.mu.Unlock() | |
| 392 | 404 | ac.sendMu.Unlock() | |
| 393 | 405 | ||
| 394 | 406 | if err != nil { | |
| @@ -280,6 +280,10 @@ func (d *Daemon) paint(sess *session, ac *attachedClient, reset bool) { | |||||
| 280 | 280 | ||||
| 281 | ac.initOverlays() | 281 | ac.initOverlays() | ||
| 282 | ac.sendMu.Lock() | 282 | ac.sendMu.Lock() | ||
| 283 | + | if ac.currentSession() != sess { | |||
| 284 | + | ac.sendMu.Unlock() | |||
| 285 | + | return | |||
| 286 | + | } | |||
| 283 | // Ack-gated coalescing: while the client is at least maxUnackedOutputStates | 287 | // Ack-gated coalescing: while the client is at least maxUnackedOutputStates | ||
| 284 | // behind, skip composing another diff and mark the paint deferred. The | 288 | // behind, skip composing another diff and mark the paint deferred. The | ||
| 285 | // MsgOutputAck handler flushes one cumulative paint once acks catch up. | 289 | // MsgOutputAck handler flushes one cumulative paint once acks catch up. | ||
| @@ -364,6 +368,13 @@ func (d *Daemon) paint(sess *session, ac *attachedClient, reset bool) { | |||||
| 364 | p.mu.Lock() | 368 | p.mu.Lock() | ||
| 365 | desiredCursor := desiredCursorOut(p.screen, cursorContent, !cursorVisible || overlays.copyActive || overlays.copySearchModel != nil || overlays.pickerActive || overlays.paletteActive || overlays.promptActive) | 369 | desiredCursor := desiredCursorOut(p.screen, cursorContent, !cursorVisible || overlays.copyActive || overlays.copySearchModel != nil || overlays.pickerActive || overlays.paletteActive || overlays.promptActive) | ||
| 366 | p.mu.Unlock() | 370 | p.mu.Unlock() | ||
| 371 | + | ac.sess.mu.Lock() | |||
| 372 | + | if ac.sess.v != sess { | |||
| 373 | + | ac.sess.mu.Unlock() | |||
| 374 | + | tb.mu.Unlock() | |||
| 375 | + | ac.sendMu.Unlock() | |||
| 376 | + | return | |||
| 377 | + | } | |||
| 367 | data, err := ac.rend.Draw(frame, damage) | 378 | data, err := ac.rend.Draw(frame, damage) | ||
| 368 | var cursorTail []byte | 379 | var cursorTail []byte | ||
| 369 | if err == nil { | 380 | if err == nil { | ||
| @@ -389,6 +400,7 @@ func (d *Daemon) paint(sess *session, ac *attachedClient, reset bool) { | |||||
| 389 | } | 400 | } | ||
| 390 | } | 401 | } | ||
| 391 | } | 402 | } | ||
| 403 | + | ac.sess.mu.Unlock() | |||
| 392 | ac.sendMu.Unlock() | 404 | ac.sendMu.Unlock() | ||
| 393 | 405 | ||||
| 394 | if err != nil { | 406 | if err != nil { | ||