fix(daemon): clear floating copy state on teardown
- commit
- 4e7c62c2d03ea7441e3f6dbb667715230ff72be0
- author
- brice · · 2026-07-10 16:15 UTC
- parent
- 23cc4d7
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
3 files · +78 -67
M internal/usecase/daemon/floating.go +4 -1
| @@ -373,13 +373,16 @@ func (d *Daemon) reapFloating(sess *session, tb *tab, p *pane, generation uint64 | |||
| 373 | 373 | } | |
| 374 | 374 | ||
| 375 | 375 | // teardownFloating invalidates first, then releases resources outside tb.mu. | |
| 376 | - | func (d *Daemon) teardownFloating(tb *tab) { | |
| 376 | + | func (d *Daemon) teardownFloating(tb *tab, ac *attachedClient) { | |
| 377 | 377 | if tb == nil { | |
| 378 | 378 | return | |
| 379 | 379 | } | |
| 380 | 380 | tb.mu.Lock() | |
| 381 | 381 | p := tb.takeFloatingLocked() | |
| 382 | 382 | tb.mu.Unlock() | |
| 383 | + | if ac != nil { | |
| 384 | + | ac.overlays.clearCopyModeForPane(p) | |
| 385 | + | } | |
| 383 | 386 | closeFloatingPane(p) | |
| 384 | 387 | } | |
| 385 | 388 | ||
| @@ -373,13 +373,16 @@ func (d *Daemon) reapFloating(sess *session, tb *tab, p *pane, generation uint64 | |||||
| 373 | } | 373 | } | ||
| 374 | 374 | ||||
| 375 | // teardownFloating invalidates first, then releases resources outside tb.mu. | 375 | // teardownFloating invalidates first, then releases resources outside tb.mu. | ||
| 376 | - | func (d *Daemon) teardownFloating(tb *tab) { | 376 | + | func (d *Daemon) teardownFloating(tb *tab, ac *attachedClient) { |
| 377 | if tb == nil { | 377 | if tb == nil { | ||
| 378 | return | 378 | return | ||
| 379 | } | 379 | } | ||
| 380 | tb.mu.Lock() | 380 | tb.mu.Lock() | ||
| 381 | p := tb.takeFloatingLocked() | 381 | p := tb.takeFloatingLocked() | ||
| 382 | tb.mu.Unlock() | 382 | tb.mu.Unlock() | ||
| 383 | + | if ac != nil { | |||
| 384 | + | ac.overlays.clearCopyModeForPane(p) | |||
| 385 | + | } | |||
| 383 | closeFloatingPane(p) | 386 | closeFloatingPane(p) | ||
| 384 | } | 387 | } | ||
| 385 | 388 | ||||
M internal/usecase/daemon/floating_test.go +71 -65
| @@ -16,10 +16,35 @@ import ( | |||
| 16 | 16 | "github.com/bnema/vev/internal/domain" | |
| 17 | 17 | "github.com/bnema/vev/internal/ports" | |
| 18 | 18 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | |
| 19 | + | scopy "github.com/bnema/vev/internal/usecase/copy" | |
| 19 | 20 | "github.com/bnema/vev/internal/usecase/layout" | |
| 20 | 21 | themeui "github.com/bnema/vev/internal/usecase/theme" | |
| 21 | 22 | ) | |
| 22 | 23 | ||
| 24 | + | type gatedFloatingOpen struct { | |
| 25 | + | command string | |
| 26 | + | args []string | |
| 27 | + | dir string | |
| 28 | + | size domain.Size | |
| 29 | + | } | |
| 30 | + | ||
| 31 | + | func newGatedOpenFactory(t *testing.T, result ports.PTY, openErr error) (*portsmocks.MockPTYFactory, <-chan gatedFloatingOpen, func()) { | |
| 32 | + | t.Helper() | |
| 33 | + | factory := portsmocks.NewMockPTYFactory(t) | |
| 34 | + | opened := make(chan gatedFloatingOpen, 1) | |
| 35 | + | release := make(chan struct{}) | |
| 36 | + | var releaseOnce sync.Once | |
| 37 | + | releaseOpen := func() { releaseOnce.Do(func() { close(release) }) } | |
| 38 | + | t.Cleanup(releaseOpen) | |
| 39 | + | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |
| 40 | + | func(command string, args, _ []string, dir string, size domain.Size) (ports.PTY, error) { | |
| 41 | + | opened <- gatedFloatingOpen{command: command, args: append([]string(nil), args...), dir: dir, size: size} | |
| 42 | + | <-release | |
| 43 | + | return result, openErr | |
| 44 | + | }).Once() | |
| 45 | + | return factory, opened, releaseOpen | |
| 46 | + | } | |
| 47 | + | ||
| 23 | 48 | func TestFloatingSlotTransitions(t *testing.T) { | |
| 24 | 49 | first := &pane{} | |
| 25 | 50 | second := &pane{} | |
| @@ -116,19 +141,7 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi | |||
| 116 | 141 | return 0, context.Canceled | |
| 117 | 142 | }).Once() | |
| 118 | 143 | pty.EXPECT().Close().RunAndReturn(func() error { unblock(); return nil }).Once() | |
| 119 | - | factory := portsmocks.NewMockPTYFactory(t) | |
| 120 | - | opened := make(chan struct{}) | |
| 121 | - | allowOpen := make(chan struct{}) | |
| 122 | - | var gotCommand, gotDir string | |
| 123 | - | var gotArgs []string | |
| 124 | - | var gotSize domain.Size | |
| 125 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |
| 126 | - | func(command string, args, _ []string, dir string, size domain.Size) (ports.PTY, error) { | |
| 127 | - | gotCommand, gotArgs, gotDir, gotSize = command, append([]string(nil), args...), dir, size | |
| 128 | - | close(opened) | |
| 129 | - | <-allowOpen | |
| 130 | - | return pty, nil | |
| 131 | - | }).Once() | |
| 144 | + | factory, opened, allowOpen := newGatedOpenFactory(t, pty, nil) | |
| 132 | 145 | d := newTestDaemon(t, factory, stubClock{}) | |
| 133 | 146 | d.shell = "/bin/custom-shell" | |
| 134 | 147 | cwd := t.TempDir() | |
| @@ -140,14 +153,15 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi | |||
| 140 | 153 | // Open has started while this goroutine owns tab.mu: an external factory | |
| 141 | 154 | // call under that lock would deadlock this channel-controlled test. | |
| 142 | 155 | tb.mu.Lock() | |
| 156 | + | var openCall gatedFloatingOpen | |
| 143 | 157 | select { | |
| 144 | - | case <-opened: | |
| 158 | + | case openCall = <-opened: | |
| 145 | 159 | case <-time.After(time.Second): | |
| 146 | 160 | t.Fatal("floating Open waited for tab.mu") | |
| 147 | 161 | } | |
| 148 | 162 | tb.mu.Unlock() | |
| 149 | 163 | d.ApplyConfig(domain.Config{Theme: domain.ThemeDark, Floating: domain.FloatingConfig{Command: "later", Width: 90, Height: 90}}) | |
| 150 | - | close(allowOpen) | |
| 164 | + | allowOpen() | |
| 151 | 165 | select { | |
| 152 | 166 | case <-readerStarted: // install started exactly one reader after the async Open | |
| 153 | 167 | case <-time.After(time.Second): | |
| @@ -159,11 +173,11 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi | |||
| 159 | 173 | require.NotNil(t, floating) | |
| 160 | 174 | assertPaneDefaultColors(t, floating, themeui.BuiltinDark.Foreground, themeui.BuiltinDark.Background) | |
| 161 | 175 | assertPaneColorScheme(t, floating, false) | |
| 162 | - | require.Equal(t, "/bin/custom-shell", gotCommand) | |
| 163 | - | require.Equal(t, []string{"-lc", "btop --utf"}, gotArgs) | |
| 164 | - | require.Equal(t, cwd, gotDir) | |
| 165 | - | require.Equal(t, domain.Size{Cols: 48, Rows: 18}, gotSize) | |
| 166 | - | d.teardownFloating(tb) | |
| 176 | + | require.Equal(t, "/bin/custom-shell", openCall.command) | |
| 177 | + | require.Equal(t, []string{"-lc", "btop --utf"}, openCall.args) | |
| 178 | + | require.Equal(t, cwd, openCall.dir) | |
| 179 | + | require.Equal(t, domain.Size{Cols: 48, Rows: 18}, openCall.size) | |
| 180 | + | d.teardownFloating(tb, nil) | |
| 167 | 181 | unblock() | |
| 168 | 182 | d.sessWg.Wait() | |
| 169 | 183 | } | |
| @@ -185,15 +199,7 @@ func TestFloatingLaunchOwnershipJoinsShutdown(t *testing.T) { | |||
| 185 | 199 | releaseReader() | |
| 186 | 200 | return nil | |
| 187 | 201 | }).Once() | |
| 188 | - | factory := portsmocks.NewMockPTYFactory(t) | |
| 189 | - | opened := make(chan struct{}) | |
| 190 | - | releaseOpen := make(chan struct{}) | |
| 191 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |
| 192 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |
| 193 | - | close(opened) | |
| 194 | - | <-releaseOpen | |
| 195 | - | return floatingPTY, nil | |
| 196 | - | }).Once() | |
| 202 | + | factory, opened, releaseOpen := newGatedOpenFactory(t, floatingPTY, nil) | |
| 197 | 203 | d := newTestDaemon(t, factory, stubClock{}) | |
| 198 | 204 | tb := newFloatingTestTab(t) | |
| 199 | 205 | sessCtx, cancel := context.WithCancel(t.Context()) | |
| @@ -221,7 +227,7 @@ func TestFloatingLaunchOwnershipJoinsShutdown(t *testing.T) { | |||
| 221 | 227 | launchUnaccounted = true | |
| 222 | 228 | case <-time.After(time.Second): | |
| 223 | 229 | } | |
| 224 | - | close(releaseOpen) | |
| 230 | + | releaseOpen() | |
| 225 | 231 | select { | |
| 226 | 232 | case <-readerStarted: | |
| 227 | 233 | case <-time.After(time.Second): | |
| @@ -245,22 +251,14 @@ func TestFloatingLifecycleStaleSuccessAndOldExitCannotReplaceCurrentSlot(t *test | |||
| 245 | 251 | pty := portsmocks.NewMockPTY(t) | |
| 246 | 252 | closed := make(chan struct{}) | |
| 247 | 253 | pty.EXPECT().Close().RunAndReturn(func() error { close(closed); return nil }).Once() | |
| 248 | - | factory := portsmocks.NewMockPTYFactory(t) | |
| 249 | - | opened := make(chan struct{}) | |
| 250 | - | allowOpen := make(chan struct{}) | |
| 251 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |
| 252 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |
| 253 | - | close(opened) | |
| 254 | - | <-allowOpen | |
| 255 | - | return pty, nil | |
| 256 | - | }).Once() | |
| 254 | + | factory, opened, allowOpen := newGatedOpenFactory(t, pty, nil) | |
| 257 | 255 | d := newTestDaemon(t, factory, stubClock{}) | |
| 258 | 256 | tb := newFloatingTestTab(t) | |
| 259 | 257 | sess := &session{name: "work", tabs: []*tab{tb}, ctx: t.Context()} | |
| 260 | 258 | d.ensureFloatingWarm(sess, tb) | |
| 261 | 259 | <-opened | |
| 262 | - | d.teardownFloating(tb) // invalidate before the delayed Open returns | |
| 263 | - | close(allowOpen) | |
| 260 | + | d.teardownFloating(tb, nil) // invalidate before the delayed Open returns | |
| 261 | + | allowOpen() | |
| 264 | 262 | select { | |
| 265 | 263 | case <-closed: | |
| 266 | 264 | case <-time.After(time.Second): | |
| @@ -294,7 +292,7 @@ func TestFloatingReapAndTeardownCloseMatchingPaneOnce(t *testing.T) { | |||
| 294 | 292 | tb.mu.Unlock() | |
| 295 | 293 | d := newTestDaemon(t, nil, stubClock{}) | |
| 296 | 294 | d.reapFloating(&session{}, tb, p, g) | |
| 297 | - | d.teardownFloating(tb) | |
| 295 | + | d.teardownFloating(tb, nil) | |
| 298 | 296 | pty.AssertNumberOfCalls(t, "Close", 1) | |
| 299 | 297 | } | |
| 300 | 298 | ||
| @@ -307,11 +305,35 @@ func TestFloatingTeardownClosesInstalledPaneOnce(t *testing.T) { | |||
| 307 | 305 | require.True(t, tb.installFloatingLocked(p, g)) | |
| 308 | 306 | tb.mu.Unlock() | |
| 309 | 307 | d := newTestDaemon(t, nil, stubClock{}) | |
| 310 | - | d.teardownFloating(tb) | |
| 311 | - | d.teardownFloating(tb) | |
| 308 | + | d.teardownFloating(tb, nil) | |
| 309 | + | d.teardownFloating(tb, nil) | |
| 312 | 310 | pty.AssertNumberOfCalls(t, "Close", 1) | |
| 313 | 311 | } | |
| 314 | 312 | ||
| 313 | + | func TestFloatingTeardownClearsCapturedCopyMode(t *testing.T) { | |
| 314 | + | pty, _ := newBlockingPTY(t) | |
| 315 | + | tb := newFloatingTestTab(t) | |
| 316 | + | p := newPane(layout.PaneID("floating"), pty, domain.Size{Cols: 10, Rows: 10}) | |
| 317 | + | tb.mu.Lock() | |
| 318 | + | generation := tb.beginFloatingWarmLocked(true) | |
| 319 | + | require.True(t, tb.installFloatingLocked(p, generation)) | |
| 320 | + | tb.mu.Unlock() | |
| 321 | + | ac := &attachedClient{} | |
| 322 | + | ac.initOverlays() | |
| 323 | + | ac.overlays.copyMu.Lock() | |
| 324 | + | ac.overlays.copyMode = &scopy.Mode{} | |
| 325 | + | ac.overlays.copyPane = p | |
| 326 | + | ac.overlays.copyMu.Unlock() | |
| 327 | + | d := newTestDaemon(t, nil, stubClock{}) | |
| 328 | + | ||
| 329 | + | d.teardownFloating(tb, ac) | |
| 330 | + | ||
| 331 | + | require.False(t, ac.overlays.copyActive()) | |
| 332 | + | ac.overlays.copyMu.Lock() | |
| 333 | + | require.Nil(t, ac.overlays.copyPane) | |
| 334 | + | ac.overlays.copyMu.Unlock() | |
| 335 | + | } | |
| 336 | + | ||
| 315 | 337 | func TestFloatingEOFRepaintsVisibleSlotOnly(t *testing.T) { | |
| 316 | 338 | newCase := func(t *testing.T, state floatingState) (*Daemon, *session, *attachedClient, chan ports.Frame, *tab, *pane, func()) { | |
| 317 | 339 | t.Helper() | |
| @@ -486,15 +508,7 @@ func (h floatingLogHandler) Handle(ctx context.Context, r slog.Record) error { | |||
| 486 | 508 | } | |
| 487 | 509 | ||
| 488 | 510 | func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testing.T) { | |
| 489 | - | factory := portsmocks.NewMockPTYFactory(t) | |
| 490 | - | opened := make(chan struct{}) | |
| 491 | - | release := make(chan struct{}) | |
| 492 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |
| 493 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |
| 494 | - | close(opened) | |
| 495 | - | <-release | |
| 496 | - | return nil, io.ErrUnexpectedEOF | |
| 497 | - | }).Once() | |
| 511 | + | factory, opened, release := newGatedOpenFactory(t, nil, io.ErrUnexpectedEOF) | |
| 498 | 512 | var logs bytes.Buffer | |
| 499 | 513 | handled := make(chan struct{}) | |
| 500 | 514 | d := newTestDaemon(t, factory, stubClock{}) | |
| @@ -518,7 +532,7 @@ func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testi | |||
| 518 | 532 | sess.mu.Unlock() | |
| 519 | 533 | } | |
| 520 | 534 | }() | |
| 521 | - | close(release) | |
| 535 | + | release() | |
| 522 | 536 | select { | |
| 523 | 537 | case <-handled: | |
| 524 | 538 | case <-time.After(time.Second): | |
| @@ -532,15 +546,7 @@ func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testi | |||
| 532 | 546 | ||
| 533 | 547 | func TestFloatingInstallLogsSessionNameSafelyDuringRename(t *testing.T) { | |
| 534 | 548 | floatingPTY, _ := newBlockingPTY(t) | |
| 535 | - | factory := portsmocks.NewMockPTYFactory(t) | |
| 536 | - | opened := make(chan struct{}) | |
| 537 | - | allowOpen := make(chan struct{}) | |
| 538 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |
| 539 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |
| 540 | - | close(opened) | |
| 541 | - | <-allowOpen | |
| 542 | - | return floatingPTY, nil | |
| 543 | - | }).Once() | |
| 549 | + | factory, opened, allowOpen := newGatedOpenFactory(t, floatingPTY, nil) | |
| 544 | 550 | var logs bytes.Buffer | |
| 545 | 551 | handled := make(chan struct{}) | |
| 546 | 552 | d := newTestDaemon(t, factory, stubClock{}) | |
| @@ -571,7 +577,7 @@ func TestFloatingInstallLogsSessionNameSafelyDuringRename(t *testing.T) { | |||
| 571 | 577 | } | |
| 572 | 578 | }() | |
| 573 | 579 | <-renamed | |
| 574 | - | close(allowOpen) | |
| 580 | + | allowOpen() | |
| 575 | 581 | select { | |
| 576 | 582 | case <-handled: | |
| 577 | 583 | case <-time.After(time.Second): | |
| @@ -581,7 +587,7 @@ func TestFloatingInstallLogsSessionNameSafelyDuringRename(t *testing.T) { | |||
| 581 | 587 | <-renameDone | |
| 582 | 588 | require.Contains(t, logs.String(), "session=renamed", logs.String()) | |
| 583 | 589 | ||
| 584 | - | d.teardownFloating(tb) | |
| 590 | + | d.teardownFloating(tb, nil) | |
| 585 | 591 | d.sessWg.Wait() | |
| 586 | 592 | } | |
| 587 | 593 | ||
| @@ -16,10 +16,35 @@ import ( | |||||
| 16 | "github.com/bnema/vev/internal/domain" | 16 | "github.com/bnema/vev/internal/domain" | ||
| 17 | "github.com/bnema/vev/internal/ports" | 17 | "github.com/bnema/vev/internal/ports" | ||
| 18 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | 18 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | ||
| 19 | + | scopy "github.com/bnema/vev/internal/usecase/copy" | |||
| 19 | "github.com/bnema/vev/internal/usecase/layout" | 20 | "github.com/bnema/vev/internal/usecase/layout" | ||
| 20 | themeui "github.com/bnema/vev/internal/usecase/theme" | 21 | themeui "github.com/bnema/vev/internal/usecase/theme" | ||
| 21 | ) | 22 | ) | ||
| 22 | 23 | ||||
| 24 | + | type gatedFloatingOpen struct { | |||
| 25 | + | command string | |||
| 26 | + | args []string | |||
| 27 | + | dir string | |||
| 28 | + | size domain.Size | |||
| 29 | + | } | |||
| 30 | + | ||||
| 31 | + | func newGatedOpenFactory(t *testing.T, result ports.PTY, openErr error) (*portsmocks.MockPTYFactory, <-chan gatedFloatingOpen, func()) { | |||
| 32 | + | t.Helper() | |||
| 33 | + | factory := portsmocks.NewMockPTYFactory(t) | |||
| 34 | + | opened := make(chan gatedFloatingOpen, 1) | |||
| 35 | + | release := make(chan struct{}) | |||
| 36 | + | var releaseOnce sync.Once | |||
| 37 | + | releaseOpen := func() { releaseOnce.Do(func() { close(release) }) } | |||
| 38 | + | t.Cleanup(releaseOpen) | |||
| 39 | + | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |||
| 40 | + | func(command string, args, _ []string, dir string, size domain.Size) (ports.PTY, error) { | |||
| 41 | + | opened <- gatedFloatingOpen{command: command, args: append([]string(nil), args...), dir: dir, size: size} | |||
| 42 | + | <-release | |||
| 43 | + | return result, openErr | |||
| 44 | + | }).Once() | |||
| 45 | + | return factory, opened, releaseOpen | |||
| 46 | + | } | |||
| 47 | + | ||||
| 23 | func TestFloatingSlotTransitions(t *testing.T) { | 48 | func TestFloatingSlotTransitions(t *testing.T) { | ||
| 24 | first := &pane{} | 49 | first := &pane{} | ||
| 25 | second := &pane{} | 50 | second := &pane{} | ||
| @@ -116,19 +141,7 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi | |||||
| 116 | return 0, context.Canceled | 141 | return 0, context.Canceled | ||
| 117 | }).Once() | 142 | }).Once() | ||
| 118 | pty.EXPECT().Close().RunAndReturn(func() error { unblock(); return nil }).Once() | 143 | pty.EXPECT().Close().RunAndReturn(func() error { unblock(); return nil }).Once() | ||
| 119 | - | factory := portsmocks.NewMockPTYFactory(t) | 144 | + | factory, opened, allowOpen := newGatedOpenFactory(t, pty, nil) |
| 120 | - | opened := make(chan struct{}) | |||
| 121 | - | allowOpen := make(chan struct{}) | |||
| 122 | - | var gotCommand, gotDir string | |||
| 123 | - | var gotArgs []string | |||
| 124 | - | var gotSize domain.Size | |||
| 125 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |||
| 126 | - | func(command string, args, _ []string, dir string, size domain.Size) (ports.PTY, error) { | |||
| 127 | - | gotCommand, gotArgs, gotDir, gotSize = command, append([]string(nil), args...), dir, size | |||
| 128 | - | close(opened) | |||
| 129 | - | <-allowOpen | |||
| 130 | - | return pty, nil | |||
| 131 | - | }).Once() | |||
| 132 | d := newTestDaemon(t, factory, stubClock{}) | 145 | d := newTestDaemon(t, factory, stubClock{}) | ||
| 133 | d.shell = "/bin/custom-shell" | 146 | d.shell = "/bin/custom-shell" | ||
| 134 | cwd := t.TempDir() | 147 | cwd := t.TempDir() | ||
| @@ -140,14 +153,15 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi | |||||
| 140 | // Open has started while this goroutine owns tab.mu: an external factory | 153 | // Open has started while this goroutine owns tab.mu: an external factory | ||
| 141 | // call under that lock would deadlock this channel-controlled test. | 154 | // call under that lock would deadlock this channel-controlled test. | ||
| 142 | tb.mu.Lock() | 155 | tb.mu.Lock() | ||
| 156 | + | var openCall gatedFloatingOpen | |||
| 143 | select { | 157 | select { | ||
| 144 | - | case <-opened: | 158 | + | case openCall = <-opened: |
| 145 | case <-time.After(time.Second): | 159 | case <-time.After(time.Second): | ||
| 146 | t.Fatal("floating Open waited for tab.mu") | 160 | t.Fatal("floating Open waited for tab.mu") | ||
| 147 | } | 161 | } | ||
| 148 | tb.mu.Unlock() | 162 | tb.mu.Unlock() | ||
| 149 | d.ApplyConfig(domain.Config{Theme: domain.ThemeDark, Floating: domain.FloatingConfig{Command: "later", Width: 90, Height: 90}}) | 163 | d.ApplyConfig(domain.Config{Theme: domain.ThemeDark, Floating: domain.FloatingConfig{Command: "later", Width: 90, Height: 90}}) | ||
| 150 | - | close(allowOpen) | 164 | + | allowOpen() |
| 151 | select { | 165 | select { | ||
| 152 | case <-readerStarted: // install started exactly one reader after the async Open | 166 | case <-readerStarted: // install started exactly one reader after the async Open | ||
| 153 | case <-time.After(time.Second): | 167 | case <-time.After(time.Second): | ||
| @@ -159,11 +173,11 @@ func TestFloatingLifecycleCapturesLaunchBeforeOpenAndDoesNotHoldTabLock(t *testi | |||||
| 159 | require.NotNil(t, floating) | 173 | require.NotNil(t, floating) | ||
| 160 | assertPaneDefaultColors(t, floating, themeui.BuiltinDark.Foreground, themeui.BuiltinDark.Background) | 174 | assertPaneDefaultColors(t, floating, themeui.BuiltinDark.Foreground, themeui.BuiltinDark.Background) | ||
| 161 | assertPaneColorScheme(t, floating, false) | 175 | assertPaneColorScheme(t, floating, false) | ||
| 162 | - | require.Equal(t, "/bin/custom-shell", gotCommand) | 176 | + | require.Equal(t, "/bin/custom-shell", openCall.command) |
| 163 | - | require.Equal(t, []string{"-lc", "btop --utf"}, gotArgs) | 177 | + | require.Equal(t, []string{"-lc", "btop --utf"}, openCall.args) |
| 164 | - | require.Equal(t, cwd, gotDir) | 178 | + | require.Equal(t, cwd, openCall.dir) |
| 165 | - | require.Equal(t, domain.Size{Cols: 48, Rows: 18}, gotSize) | 179 | + | require.Equal(t, domain.Size{Cols: 48, Rows: 18}, openCall.size) |
| 166 | - | d.teardownFloating(tb) | 180 | + | d.teardownFloating(tb, nil) |
| 167 | unblock() | 181 | unblock() | ||
| 168 | d.sessWg.Wait() | 182 | d.sessWg.Wait() | ||
| 169 | } | 183 | } | ||
| @@ -185,15 +199,7 @@ func TestFloatingLaunchOwnershipJoinsShutdown(t *testing.T) { | |||||
| 185 | releaseReader() | 199 | releaseReader() | ||
| 186 | return nil | 200 | return nil | ||
| 187 | }).Once() | 201 | }).Once() | ||
| 188 | - | factory := portsmocks.NewMockPTYFactory(t) | 202 | + | factory, opened, releaseOpen := newGatedOpenFactory(t, floatingPTY, nil) |
| 189 | - | opened := make(chan struct{}) | |||
| 190 | - | releaseOpen := make(chan struct{}) | |||
| 191 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |||
| 192 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |||
| 193 | - | close(opened) | |||
| 194 | - | <-releaseOpen | |||
| 195 | - | return floatingPTY, nil | |||
| 196 | - | }).Once() | |||
| 197 | d := newTestDaemon(t, factory, stubClock{}) | 203 | d := newTestDaemon(t, factory, stubClock{}) | ||
| 198 | tb := newFloatingTestTab(t) | 204 | tb := newFloatingTestTab(t) | ||
| 199 | sessCtx, cancel := context.WithCancel(t.Context()) | 205 | sessCtx, cancel := context.WithCancel(t.Context()) | ||
| @@ -221,7 +227,7 @@ func TestFloatingLaunchOwnershipJoinsShutdown(t *testing.T) { | |||||
| 221 | launchUnaccounted = true | 227 | launchUnaccounted = true | ||
| 222 | case <-time.After(time.Second): | 228 | case <-time.After(time.Second): | ||
| 223 | } | 229 | } | ||
| 224 | - | close(releaseOpen) | 230 | + | releaseOpen() |
| 225 | select { | 231 | select { | ||
| 226 | case <-readerStarted: | 232 | case <-readerStarted: | ||
| 227 | case <-time.After(time.Second): | 233 | case <-time.After(time.Second): | ||
| @@ -245,22 +251,14 @@ func TestFloatingLifecycleStaleSuccessAndOldExitCannotReplaceCurrentSlot(t *test | |||||
| 245 | pty := portsmocks.NewMockPTY(t) | 251 | pty := portsmocks.NewMockPTY(t) | ||
| 246 | closed := make(chan struct{}) | 252 | closed := make(chan struct{}) | ||
| 247 | pty.EXPECT().Close().RunAndReturn(func() error { close(closed); return nil }).Once() | 253 | pty.EXPECT().Close().RunAndReturn(func() error { close(closed); return nil }).Once() | ||
| 248 | - | factory := portsmocks.NewMockPTYFactory(t) | 254 | + | factory, opened, allowOpen := newGatedOpenFactory(t, pty, nil) |
| 249 | - | opened := make(chan struct{}) | |||
| 250 | - | allowOpen := make(chan struct{}) | |||
| 251 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |||
| 252 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |||
| 253 | - | close(opened) | |||
| 254 | - | <-allowOpen | |||
| 255 | - | return pty, nil | |||
| 256 | - | }).Once() | |||
| 257 | d := newTestDaemon(t, factory, stubClock{}) | 255 | d := newTestDaemon(t, factory, stubClock{}) | ||
| 258 | tb := newFloatingTestTab(t) | 256 | tb := newFloatingTestTab(t) | ||
| 259 | sess := &session{name: "work", tabs: []*tab{tb}, ctx: t.Context()} | 257 | sess := &session{name: "work", tabs: []*tab{tb}, ctx: t.Context()} | ||
| 260 | d.ensureFloatingWarm(sess, tb) | 258 | d.ensureFloatingWarm(sess, tb) | ||
| 261 | <-opened | 259 | <-opened | ||
| 262 | - | d.teardownFloating(tb) // invalidate before the delayed Open returns | 260 | + | d.teardownFloating(tb, nil) // invalidate before the delayed Open returns |
| 263 | - | close(allowOpen) | 261 | + | allowOpen() |
| 264 | select { | 262 | select { | ||
| 265 | case <-closed: | 263 | case <-closed: | ||
| 266 | case <-time.After(time.Second): | 264 | case <-time.After(time.Second): | ||
| @@ -294,7 +292,7 @@ func TestFloatingReapAndTeardownCloseMatchingPaneOnce(t *testing.T) { | |||||
| 294 | tb.mu.Unlock() | 292 | tb.mu.Unlock() | ||
| 295 | d := newTestDaemon(t, nil, stubClock{}) | 293 | d := newTestDaemon(t, nil, stubClock{}) | ||
| 296 | d.reapFloating(&session{}, tb, p, g) | 294 | d.reapFloating(&session{}, tb, p, g) | ||
| 297 | - | d.teardownFloating(tb) | 295 | + | d.teardownFloating(tb, nil) |
| 298 | pty.AssertNumberOfCalls(t, "Close", 1) | 296 | pty.AssertNumberOfCalls(t, "Close", 1) | ||
| 299 | } | 297 | } | ||
| 300 | 298 | ||||
| @@ -307,11 +305,35 @@ func TestFloatingTeardownClosesInstalledPaneOnce(t *testing.T) { | |||||
| 307 | require.True(t, tb.installFloatingLocked(p, g)) | 305 | require.True(t, tb.installFloatingLocked(p, g)) | ||
| 308 | tb.mu.Unlock() | 306 | tb.mu.Unlock() | ||
| 309 | d := newTestDaemon(t, nil, stubClock{}) | 307 | d := newTestDaemon(t, nil, stubClock{}) | ||
| 310 | - | d.teardownFloating(tb) | 308 | + | d.teardownFloating(tb, nil) |
| 311 | - | d.teardownFloating(tb) | 309 | + | d.teardownFloating(tb, nil) |
| 312 | pty.AssertNumberOfCalls(t, "Close", 1) | 310 | pty.AssertNumberOfCalls(t, "Close", 1) | ||
| 313 | } | 311 | } | ||
| 314 | 312 | ||||
| 313 | + | func TestFloatingTeardownClearsCapturedCopyMode(t *testing.T) { | |||
| 314 | + | pty, _ := newBlockingPTY(t) | |||
| 315 | + | tb := newFloatingTestTab(t) | |||
| 316 | + | p := newPane(layout.PaneID("floating"), pty, domain.Size{Cols: 10, Rows: 10}) | |||
| 317 | + | tb.mu.Lock() | |||
| 318 | + | generation := tb.beginFloatingWarmLocked(true) | |||
| 319 | + | require.True(t, tb.installFloatingLocked(p, generation)) | |||
| 320 | + | tb.mu.Unlock() | |||
| 321 | + | ac := &attachedClient{} | |||
| 322 | + | ac.initOverlays() | |||
| 323 | + | ac.overlays.copyMu.Lock() | |||
| 324 | + | ac.overlays.copyMode = &scopy.Mode{} | |||
| 325 | + | ac.overlays.copyPane = p | |||
| 326 | + | ac.overlays.copyMu.Unlock() | |||
| 327 | + | d := newTestDaemon(t, nil, stubClock{}) | |||
| 328 | + | ||||
| 329 | + | d.teardownFloating(tb, ac) | |||
| 330 | + | ||||
| 331 | + | require.False(t, ac.overlays.copyActive()) | |||
| 332 | + | ac.overlays.copyMu.Lock() | |||
| 333 | + | require.Nil(t, ac.overlays.copyPane) | |||
| 334 | + | ac.overlays.copyMu.Unlock() | |||
| 335 | + | } | |||
| 336 | + | ||||
| 315 | func TestFloatingEOFRepaintsVisibleSlotOnly(t *testing.T) { | 337 | func TestFloatingEOFRepaintsVisibleSlotOnly(t *testing.T) { | ||
| 316 | newCase := func(t *testing.T, state floatingState) (*Daemon, *session, *attachedClient, chan ports.Frame, *tab, *pane, func()) { | 338 | newCase := func(t *testing.T, state floatingState) (*Daemon, *session, *attachedClient, chan ports.Frame, *tab, *pane, func()) { | ||
| 317 | t.Helper() | 339 | t.Helper() | ||
| @@ -486,15 +508,7 @@ func (h floatingLogHandler) Handle(ctx context.Context, r slog.Record) error { | |||||
| 486 | } | 508 | } | ||
| 487 | 509 | ||||
| 488 | func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testing.T) { | 510 | func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testing.T) { | ||
| 489 | - | factory := portsmocks.NewMockPTYFactory(t) | 511 | + | factory, opened, release := newGatedOpenFactory(t, nil, io.ErrUnexpectedEOF) |
| 490 | - | opened := make(chan struct{}) | |||
| 491 | - | release := make(chan struct{}) | |||
| 492 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |||
| 493 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |||
| 494 | - | close(opened) | |||
| 495 | - | <-release | |||
| 496 | - | return nil, io.ErrUnexpectedEOF | |||
| 497 | - | }).Once() | |||
| 498 | var logs bytes.Buffer | 512 | var logs bytes.Buffer | ||
| 499 | handled := make(chan struct{}) | 513 | handled := make(chan struct{}) | ||
| 500 | d := newTestDaemon(t, factory, stubClock{}) | 514 | d := newTestDaemon(t, factory, stubClock{}) | ||
| @@ -518,7 +532,7 @@ func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testi | |||||
| 518 | sess.mu.Unlock() | 532 | sess.mu.Unlock() | ||
| 519 | } | 533 | } | ||
| 520 | }() | 534 | }() | ||
| 521 | - | close(release) | 535 | + | release() |
| 522 | select { | 536 | select { | ||
| 523 | case <-handled: | 537 | case <-handled: | ||
| 524 | case <-time.After(time.Second): | 538 | case <-time.After(time.Second): | ||
| @@ -532,15 +546,7 @@ func TestFloatingLaunchFailureCapturesSessionNameBeforeConcurrentRename(t *testi | |||||
| 532 | 546 | ||||
| 533 | func TestFloatingInstallLogsSessionNameSafelyDuringRename(t *testing.T) { | 547 | func TestFloatingInstallLogsSessionNameSafelyDuringRename(t *testing.T) { | ||
| 534 | floatingPTY, _ := newBlockingPTY(t) | 548 | floatingPTY, _ := newBlockingPTY(t) | ||
| 535 | - | factory := portsmocks.NewMockPTYFactory(t) | 549 | + | factory, opened, allowOpen := newGatedOpenFactory(t, floatingPTY, nil) |
| 536 | - | opened := make(chan struct{}) | |||
| 537 | - | allowOpen := make(chan struct{}) | |||
| 538 | - | factory.EXPECT().Open(mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).RunAndReturn( | |||
| 539 | - | func(string, []string, []string, string, domain.Size) (ports.PTY, error) { | |||
| 540 | - | close(opened) | |||
| 541 | - | <-allowOpen | |||
| 542 | - | return floatingPTY, nil | |||
| 543 | - | }).Once() | |||
| 544 | var logs bytes.Buffer | 550 | var logs bytes.Buffer | ||
| 545 | handled := make(chan struct{}) | 551 | handled := make(chan struct{}) | ||
| 546 | d := newTestDaemon(t, factory, stubClock{}) | 552 | d := newTestDaemon(t, factory, stubClock{}) | ||
| @@ -571,7 +577,7 @@ func TestFloatingInstallLogsSessionNameSafelyDuringRename(t *testing.T) { | |||||
| 571 | } | 577 | } | ||
| 572 | }() | 578 | }() | ||
| 573 | <-renamed | 579 | <-renamed | ||
| 574 | - | close(allowOpen) | 580 | + | allowOpen() |
| 575 | select { | 581 | select { | ||
| 576 | case <-handled: | 582 | case <-handled: | ||
| 577 | case <-time.After(time.Second): | 583 | case <-time.After(time.Second): | ||
| @@ -581,7 +587,7 @@ func TestFloatingInstallLogsSessionNameSafelyDuringRename(t *testing.T) { | |||||
| 581 | <-renameDone | 587 | <-renameDone | ||
| 582 | require.Contains(t, logs.String(), "session=renamed", logs.String()) | 588 | require.Contains(t, logs.String(), "session=renamed", logs.String()) | ||
| 583 | 589 | ||||
| 584 | - | d.teardownFloating(tb) | 590 | + | d.teardownFloating(tb, nil) |
| 585 | d.sessWg.Wait() | 591 | d.sessWg.Wait() | ||
| 586 | } | 592 | } | ||
| 587 | 593 | ||||
M internal/usecase/daemon/session.go +3 -1
| @@ -75,7 +75,7 @@ type session struct { | |||
| 75 | 75 | ||
| 76 | 76 | // tab is a pane layout container; pane owns PTY/screen/scrollback/render scheduling state. | |
| 77 | 77 | type tab struct { | |
| 78 | - | mu sync.Mutex // guards tree, panes, nextPaneID, size, previewClient, and pane map membership | |
| 78 | + | mu sync.Mutex // guards tree, panes, floating, nextPaneID, size, previewClient, and pane map membership | |
| 79 | 79 | ||
| 80 | 80 | stableID string | |
| 81 | 81 | name string | |
| @@ -638,6 +638,7 @@ func (d *Daemon) closeTab(sess *session, tb *tab, repaint bool) { | |||
| 638 | 638 | if tb.cancel != nil { | |
| 639 | 639 | tb.cancel() | |
| 640 | 640 | } | |
| 641 | + | d.teardownFloating(tb, ac) | |
| 641 | 642 | tb.closeAllPanes() | |
| 642 | 643 | if wasActive { | |
| 643 | 644 | d.activateTab(sess, destination) | |
| @@ -741,6 +742,7 @@ func (d *Daemon) killSession(sess *session, reason uint8, purge bool) error { | |||
| 741 | 742 | sess.mu.Unlock() | |
| 742 | 743 | for _, tb := range tabs { | |
| 743 | 744 | d.clearDestroyedTabPreview(tb) | |
| 745 | + | d.teardownFloating(tb, ac) | |
| 744 | 746 | tb.closeAllPanes() | |
| 745 | 747 | } | |
| 746 | 748 | for _, path := range clipFiles { | |
| @@ -75,7 +75,7 @@ type session struct { | |||||
| 75 | 75 | ||||
| 76 | // tab is a pane layout container; pane owns PTY/screen/scrollback/render scheduling state. | 76 | // tab is a pane layout container; pane owns PTY/screen/scrollback/render scheduling state. | ||
| 77 | type tab struct { | 77 | type tab struct { | ||
| 78 | - | mu sync.Mutex // guards tree, panes, nextPaneID, size, previewClient, and pane map membership | 78 | + | mu sync.Mutex // guards tree, panes, floating, nextPaneID, size, previewClient, and pane map membership |
| 79 | 79 | ||||
| 80 | stableID string | 80 | stableID string | ||
| 81 | name string | 81 | name string | ||
| @@ -638,6 +638,7 @@ func (d *Daemon) closeTab(sess *session, tb *tab, repaint bool) { | |||||
| 638 | if tb.cancel != nil { | 638 | if tb.cancel != nil { | ||
| 639 | tb.cancel() | 639 | tb.cancel() | ||
| 640 | } | 640 | } | ||
| 641 | + | d.teardownFloating(tb, ac) | |||
| 641 | tb.closeAllPanes() | 642 | tb.closeAllPanes() | ||
| 642 | if wasActive { | 643 | if wasActive { | ||
| 643 | d.activateTab(sess, destination) | 644 | d.activateTab(sess, destination) | ||
| @@ -741,6 +742,7 @@ func (d *Daemon) killSession(sess *session, reason uint8, purge bool) error { | |||||
| 741 | sess.mu.Unlock() | 742 | sess.mu.Unlock() | ||
| 742 | for _, tb := range tabs { | 743 | for _, tb := range tabs { | ||
| 743 | d.clearDestroyedTabPreview(tb) | 744 | d.clearDestroyedTabPreview(tb) | ||
| 745 | + | d.teardownFloating(tb, ac) | |||
| 744 | tb.closeAllPanes() | 746 | tb.closeAllPanes() | ||
| 745 | } | 747 | } | ||
| 746 | for _, path := range clipFiles { | 748 | for _, path := range clipFiles { | ||