fix: clean up failed player restart activation
- commit
- 5ee601d7efe3f3dca6c93ec55d7ff751ab41a340
- author
- brice · · 2026-04-10 14:46 UTC
- parent
- f248e04
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +112 -2
M internal/core/player.go +7 -1
| @@ -16,8 +16,11 @@ var ( | |||
| 16 | 16 | ErrPlayerClosed = errors.New("player is closed") | |
| 17 | 17 | ) | |
| 18 | 18 | ||
| 19 | - | // player is the internal player shell with state machine | |
| 19 | + | // player is the internal player shell with state machine. | |
| 20 | 20 | type player struct { | |
| 21 | + | // config is immutable after construction. It is safe to read from | |
| 22 | + | // any goroutine — including the PipeWire process callback — without | |
| 23 | + | // holding mu. | |
| 21 | 24 | config playerConfig | |
| 22 | 25 | callbacks playerCallbacks | |
| 23 | 26 | state atomic.Int32 | |
| @@ -141,6 +144,9 @@ func (p *player) Start() error { | |||
| 141 | 144 | p.mu.Unlock() | |
| 142 | 145 | ||
| 143 | 146 | if err := p.streamOps.SetStreamActive(streamPtr, true); err != nil { | |
| 147 | + | // Activation failed — clean up owned resources so stale pointers | |
| 148 | + | // are not left behind, then transition to Error. | |
| 149 | + | p.teardown() | |
| 144 | 150 | p.transition(PlayerStateError) | |
| 145 | 151 | return err | |
| 146 | 152 | } | |
| @@ -16,8 +16,11 @@ var ( | |||||
| 16 | ErrPlayerClosed = errors.New("player is closed") | 16 | ErrPlayerClosed = errors.New("player is closed") | ||
| 17 | ) | 17 | ) | ||
| 18 | 18 | ||||
| 19 | - | // player is the internal player shell with state machine | 19 | + | // player is the internal player shell with state machine. |
| 20 | type player struct { | 20 | type player struct { | ||
| 21 | + | // config is immutable after construction. It is safe to read from | |||
| 22 | + | // any goroutine — including the PipeWire process callback — without | |||
| 23 | + | // holding mu. | |||
| 21 | config playerConfig | 24 | config playerConfig | ||
| 22 | callbacks playerCallbacks | 25 | callbacks playerCallbacks | ||
| 23 | state atomic.Int32 | 26 | state atomic.Int32 | ||
| @@ -141,6 +144,9 @@ func (p *player) Start() error { | |||||
| 141 | p.mu.Unlock() | 144 | p.mu.Unlock() | ||
| 142 | 145 | ||||
| 143 | if err := p.streamOps.SetStreamActive(streamPtr, true); err != nil { | 146 | if err := p.streamOps.SetStreamActive(streamPtr, true); err != nil { | ||
| 147 | + | // Activation failed — clean up owned resources so stale pointers | |||
| 148 | + | // are not left behind, then transition to Error. | |||
| 149 | + | p.teardown() | |||
| 144 | p.transition(PlayerStateError) | 150 | p.transition(PlayerStateError) | ||
| 145 | return err | 151 | return err | ||
| 146 | } | 152 | } | ||
M internal/core/player_test.go +105 -1
| @@ -36,7 +36,7 @@ func expectStartWithSync(mockOps *mocks.MockStreamOps, fakeLoop, fakeStream unsa | |||
| 36 | 36 | Channels: cfg.Channels, | |
| 37 | 37 | FramesPerBuffer: cfg.FramesPerBuffer, | |
| 38 | 38 | }).Return(nil) | |
| 39 | - | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(nil) | |
| 39 | + | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(nil).Once() | |
| 40 | 40 | mockOps.EXPECT().RunMainLoop(fakeLoop).RunAndReturn(func(unsafe.Pointer) error { | |
| 41 | 41 | wg.Done() | |
| 42 | 42 | return nil | |
| @@ -544,3 +544,107 @@ func TestPlayerTeardownContinuesOnDisconnectError(t *testing.T) { | |||
| 544 | 544 | t.Fatalf("Close should succeed despite disconnect error: %v", err) | |
| 545 | 545 | } | |
| 546 | 546 | } | |
| 547 | + | ||
| 548 | + | // TestPlayerRestartActivationFailureCleansUpResources verifies that when | |
| 549 | + | // SetStreamActive(true) fails during a restart (after Stop), owned stream | |
| 550 | + | // and loop resources are cleaned up and no stale pointers remain. | |
| 551 | + | func TestPlayerRestartActivationFailureCleansUpResources(t *testing.T) { | |
| 552 | + | mockOps := mocks.NewMockStreamOps(t) | |
| 553 | + | fakeLoop := unsafe.Pointer(uintptr(0xCAFE)) | |
| 554 | + | fakeStream := unsafe.Pointer(uintptr(0xBEEF)) | |
| 555 | + | cfg := defaultTestConfig() | |
| 556 | + | activateErr := errors.New("activate failed on restart") | |
| 557 | + | ||
| 558 | + | // First start: create resources | |
| 559 | + | wg := expectStartWithSync(mockOps, fakeLoop, fakeStream, cfg) | |
| 560 | + | // Stop: deactivate | |
| 561 | + | mockOps.EXPECT().SetStreamActive(fakeStream, false).Return(nil) | |
| 562 | + | // Restart: activation fails → teardown cleans up | |
| 563 | + | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(activateErr) | |
| 564 | + | // teardown should call DisconnectStream, DestroyStream, QuitMainLoop, DestroyMainLoop | |
| 565 | + | mockOps.EXPECT().DisconnectStream(fakeStream).Return(nil) | |
| 566 | + | mockOps.EXPECT().DestroyStream(fakeStream).Return() | |
| 567 | + | mockOps.EXPECT().QuitMainLoop(fakeLoop).Return() | |
| 568 | + | mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return() | |
| 569 | + | ||
| 570 | + | p := newPlayer(cfg, PlayerCallbacks{}) | |
| 571 | + | p.streamOps = mockOps | |
| 572 | + | ||
| 573 | + | // Start | |
| 574 | + | if err := p.Start(); err != nil { | |
| 575 | + | t.Fatalf("Start failed: %v", err) | |
| 576 | + | } | |
| 577 | + | waitOnMainLoop(t, wg) | |
| 578 | + | ||
| 579 | + | // Stop | |
| 580 | + | if err := p.Stop(); err != nil { | |
| 581 | + | t.Fatalf("Stop failed: %v", err) | |
| 582 | + | } | |
| 583 | + | ||
| 584 | + | // Restart should fail because SetStreamActive fails | |
| 585 | + | err := p.Start() | |
| 586 | + | if err == nil { | |
| 587 | + | t.Fatal("expected Start to fail on restart activation failure") | |
| 588 | + | } | |
| 589 | + | if !errors.Is(err, activateErr) { | |
| 590 | + | t.Fatalf("expected activateErr, got %v", err) | |
| 591 | + | } | |
| 592 | + | ||
| 593 | + | // Player should be in Error state | |
| 594 | + | if p.State() != PlayerStateError { | |
| 595 | + | t.Errorf("expected Error state after failed restart activation, got %v", p.State()) | |
| 596 | + | } | |
| 597 | + | ||
| 598 | + | // Stale pointers must be cleared | |
| 599 | + | p.mu.Lock() | |
| 600 | + | loopPtr := p.loopPtr | |
| 601 | + | streamPtr := p.streamPtr | |
| 602 | + | p.mu.Unlock() | |
| 603 | + | ||
| 604 | + | if loopPtr != nil { | |
| 605 | + | t.Error("expected loopPtr to be nil after failed restart activation") | |
| 606 | + | } | |
| 607 | + | if streamPtr != nil { | |
| 608 | + | t.Error("expected streamPtr to be nil after failed restart activation") | |
| 609 | + | } | |
| 610 | + | } | |
| 611 | + | ||
| 612 | + | // TestPlayerRestartDoesNotStartSecondMainLoop verifies that when restarting | |
| 613 | + | // after Stop (reusing existing resources), no second RunMainLoop call is | |
| 614 | + | // made — only SetStreamActive(true) is called. | |
| 615 | + | func TestPlayerRestartDoesNotStartSecondMainLoop(t *testing.T) { | |
| 616 | + | mockOps := mocks.NewMockStreamOps(t) | |
| 617 | + | fakeLoop := unsafe.Pointer(uintptr(0xCAFE)) | |
| 618 | + | fakeStream := unsafe.Pointer(uintptr(0xBEEF)) | |
| 619 | + | cfg := defaultTestConfig() | |
| 620 | + | ||
| 621 | + | // First start: full resource creation + RunMainLoop | |
| 622 | + | wg := expectStartWithSync(mockOps, fakeLoop, fakeStream, cfg) | |
| 623 | + | // Stop: deactivate | |
| 624 | + | mockOps.EXPECT().SetStreamActive(fakeStream, false).Return(nil) | |
| 625 | + | // Restart: only SetStreamActive(true) — NO CreateMainLoop, NO RunMainLoop | |
| 626 | + | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(nil) | |
| 627 | + | ||
| 628 | + | p := newPlayer(cfg, PlayerCallbacks{}) | |
| 629 | + | p.streamOps = mockOps | |
| 630 | + | ||
| 631 | + | // Start | |
| 632 | + | if err := p.Start(); err != nil { | |
| 633 | + | t.Fatalf("Start failed: %v", err) | |
| 634 | + | } | |
| 635 | + | waitOnMainLoop(t, wg) | |
| 636 | + | ||
| 637 | + | // Stop | |
| 638 | + | if err := p.Stop(); err != nil { | |
| 639 | + | t.Fatalf("Stop failed: %v", err) | |
| 640 | + | } | |
| 641 | + | ||
| 642 | + | // Restart — mock will fail if any extra calls are made | |
| 643 | + | if err := p.Start(); err != nil { | |
| 644 | + | t.Fatalf("Restart failed: %v", err) | |
| 645 | + | } | |
| 646 | + | ||
| 647 | + | if p.State() != PlayerStatePlaying { | |
| 648 | + | t.Fatalf("expected Playing state after restart, got %v", p.State()) | |
| 649 | + | } | |
| 650 | + | } | |
| @@ -36,7 +36,7 @@ func expectStartWithSync(mockOps *mocks.MockStreamOps, fakeLoop, fakeStream unsa | |||||
| 36 | Channels: cfg.Channels, | 36 | Channels: cfg.Channels, | ||
| 37 | FramesPerBuffer: cfg.FramesPerBuffer, | 37 | FramesPerBuffer: cfg.FramesPerBuffer, | ||
| 38 | }).Return(nil) | 38 | }).Return(nil) | ||
| 39 | - | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(nil) | 39 | + | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(nil).Once() |
| 40 | mockOps.EXPECT().RunMainLoop(fakeLoop).RunAndReturn(func(unsafe.Pointer) error { | 40 | mockOps.EXPECT().RunMainLoop(fakeLoop).RunAndReturn(func(unsafe.Pointer) error { | ||
| 41 | wg.Done() | 41 | wg.Done() | ||
| 42 | return nil | 42 | return nil | ||
| @@ -544,3 +544,107 @@ func TestPlayerTeardownContinuesOnDisconnectError(t *testing.T) { | |||||
| 544 | t.Fatalf("Close should succeed despite disconnect error: %v", err) | 544 | t.Fatalf("Close should succeed despite disconnect error: %v", err) | ||
| 545 | } | 545 | } | ||
| 546 | } | 546 | } | ||
| 547 | + | ||||
| 548 | + | // TestPlayerRestartActivationFailureCleansUpResources verifies that when | |||
| 549 | + | // SetStreamActive(true) fails during a restart (after Stop), owned stream | |||
| 550 | + | // and loop resources are cleaned up and no stale pointers remain. | |||
| 551 | + | func TestPlayerRestartActivationFailureCleansUpResources(t *testing.T) { | |||
| 552 | + | mockOps := mocks.NewMockStreamOps(t) | |||
| 553 | + | fakeLoop := unsafe.Pointer(uintptr(0xCAFE)) | |||
| 554 | + | fakeStream := unsafe.Pointer(uintptr(0xBEEF)) | |||
| 555 | + | cfg := defaultTestConfig() | |||
| 556 | + | activateErr := errors.New("activate failed on restart") | |||
| 557 | + | ||||
| 558 | + | // First start: create resources | |||
| 559 | + | wg := expectStartWithSync(mockOps, fakeLoop, fakeStream, cfg) | |||
| 560 | + | // Stop: deactivate | |||
| 561 | + | mockOps.EXPECT().SetStreamActive(fakeStream, false).Return(nil) | |||
| 562 | + | // Restart: activation fails → teardown cleans up | |||
| 563 | + | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(activateErr) | |||
| 564 | + | // teardown should call DisconnectStream, DestroyStream, QuitMainLoop, DestroyMainLoop | |||
| 565 | + | mockOps.EXPECT().DisconnectStream(fakeStream).Return(nil) | |||
| 566 | + | mockOps.EXPECT().DestroyStream(fakeStream).Return() | |||
| 567 | + | mockOps.EXPECT().QuitMainLoop(fakeLoop).Return() | |||
| 568 | + | mockOps.EXPECT().DestroyMainLoop(fakeLoop).Return() | |||
| 569 | + | ||||
| 570 | + | p := newPlayer(cfg, PlayerCallbacks{}) | |||
| 571 | + | p.streamOps = mockOps | |||
| 572 | + | ||||
| 573 | + | // Start | |||
| 574 | + | if err := p.Start(); err != nil { | |||
| 575 | + | t.Fatalf("Start failed: %v", err) | |||
| 576 | + | } | |||
| 577 | + | waitOnMainLoop(t, wg) | |||
| 578 | + | ||||
| 579 | + | // Stop | |||
| 580 | + | if err := p.Stop(); err != nil { | |||
| 581 | + | t.Fatalf("Stop failed: %v", err) | |||
| 582 | + | } | |||
| 583 | + | ||||
| 584 | + | // Restart should fail because SetStreamActive fails | |||
| 585 | + | err := p.Start() | |||
| 586 | + | if err == nil { | |||
| 587 | + | t.Fatal("expected Start to fail on restart activation failure") | |||
| 588 | + | } | |||
| 589 | + | if !errors.Is(err, activateErr) { | |||
| 590 | + | t.Fatalf("expected activateErr, got %v", err) | |||
| 591 | + | } | |||
| 592 | + | ||||
| 593 | + | // Player should be in Error state | |||
| 594 | + | if p.State() != PlayerStateError { | |||
| 595 | + | t.Errorf("expected Error state after failed restart activation, got %v", p.State()) | |||
| 596 | + | } | |||
| 597 | + | ||||
| 598 | + | // Stale pointers must be cleared | |||
| 599 | + | p.mu.Lock() | |||
| 600 | + | loopPtr := p.loopPtr | |||
| 601 | + | streamPtr := p.streamPtr | |||
| 602 | + | p.mu.Unlock() | |||
| 603 | + | ||||
| 604 | + | if loopPtr != nil { | |||
| 605 | + | t.Error("expected loopPtr to be nil after failed restart activation") | |||
| 606 | + | } | |||
| 607 | + | if streamPtr != nil { | |||
| 608 | + | t.Error("expected streamPtr to be nil after failed restart activation") | |||
| 609 | + | } | |||
| 610 | + | } | |||
| 611 | + | ||||
| 612 | + | // TestPlayerRestartDoesNotStartSecondMainLoop verifies that when restarting | |||
| 613 | + | // after Stop (reusing existing resources), no second RunMainLoop call is | |||
| 614 | + | // made — only SetStreamActive(true) is called. | |||
| 615 | + | func TestPlayerRestartDoesNotStartSecondMainLoop(t *testing.T) { | |||
| 616 | + | mockOps := mocks.NewMockStreamOps(t) | |||
| 617 | + | fakeLoop := unsafe.Pointer(uintptr(0xCAFE)) | |||
| 618 | + | fakeStream := unsafe.Pointer(uintptr(0xBEEF)) | |||
| 619 | + | cfg := defaultTestConfig() | |||
| 620 | + | ||||
| 621 | + | // First start: full resource creation + RunMainLoop | |||
| 622 | + | wg := expectStartWithSync(mockOps, fakeLoop, fakeStream, cfg) | |||
| 623 | + | // Stop: deactivate | |||
| 624 | + | mockOps.EXPECT().SetStreamActive(fakeStream, false).Return(nil) | |||
| 625 | + | // Restart: only SetStreamActive(true) — NO CreateMainLoop, NO RunMainLoop | |||
| 626 | + | mockOps.EXPECT().SetStreamActive(fakeStream, true).Return(nil) | |||
| 627 | + | ||||
| 628 | + | p := newPlayer(cfg, PlayerCallbacks{}) | |||
| 629 | + | p.streamOps = mockOps | |||
| 630 | + | ||||
| 631 | + | // Start | |||
| 632 | + | if err := p.Start(); err != nil { | |||
| 633 | + | t.Fatalf("Start failed: %v", err) | |||
| 634 | + | } | |||
| 635 | + | waitOnMainLoop(t, wg) | |||
| 636 | + | ||||
| 637 | + | // Stop | |||
| 638 | + | if err := p.Stop(); err != nil { | |||
| 639 | + | t.Fatalf("Stop failed: %v", err) | |||
| 640 | + | } | |||
| 641 | + | ||||
| 642 | + | // Restart — mock will fail if any extra calls are made | |||
| 643 | + | if err := p.Start(); err != nil { | |||
| 644 | + | t.Fatalf("Restart failed: %v", err) | |||
| 645 | + | } | |||
| 646 | + | ||||
| 647 | + | if p.State() != PlayerStatePlaying { | |||
| 648 | + | t.Fatalf("expected Playing state after restart, got %v", p.State()) | |||
| 649 | + | } | |||
| 650 | + | } | |||