Merge pull request #116 from bnema/fix/snapshot-kill-test-race

test(daemon): fix macOS CI hang in kill-session snapshot test

commit: 4904a5aa909189993479f97153c0911e1f4a9716
author: bnema · 3mo ago · 2026-07-19 06:56 UTC
committer: GitHub · 3mo ago · 2026-07-19 06:56 UTC
merge of: 092c799 + f1556b7 · diff against the first parent
signature: GPG UnverifiedB5690EEEBB952194 The key is unknown

1 file · +48 -12

M internal/usecase/daemon/snapshot_test.go +48 -12


@@ -738,24 +738,20 @@ func TestSnapshotCaptureSkipsBareShellProcess(t *testing.T) {
738	738		}
739	739
740	740		func TestKillSessionSnapshotsNamedSessionBeforeClosingPanes(t *testing.T) {
741		-	store := portsmocks.NewMockSnapshotStore(t)
	741	+	store := newGatedSnapshotStore()
742	742		d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{})
743	743		WithSnapshotStore(store)(d)
744		-	startSnapshotEncodeWorker(t, d)
	744	+	d.startSnapshotEncodeWorker()
	745	+	t.Cleanup(func() {
	746	+	store.unblock()
	747	+	d.stopSnapshotEncodeWorker()
	748	+	})
745	749		d.procCwd = func(int) (string, error) { return "/live", nil }
746	750
747	751		sess := newSnapshotTestSession(t, "work", false, "/fallback")
748	752		d.sessions[sess.id] = sess
749	753
750	754		var closed atomic.Bool
751		-	store.EXPECT().Write("work", mock.Anything).RunAndReturn(func(_ string, data []byte) error {
752		-	snap, err := snapcodec.Unmarshal(data)
753		-	require.NoError(t, err)
754		-	require.Equal(t, "work", snap.Name)
755		-	require.Equal(t, "/live", snap.Tabs[0].Panes[0].Cwd)
756		-	require.True(t, closed.Load(), "snapshot persistence must not delay pane close")
757		-	return nil
758		-	}).Once()
759	755		mockPTY, ok := sess.tabs[0].panes["pane-1"].pty.(*portsmocks.MockPTY)
760	756		require.True(t, ok)
761	757		mockPTY.EXPECT().Close().RunAndReturn(func() error {

@@ -764,7 +760,22 @@ func TestKillSessionSnapshotsNamedSessionBeforeClosingPanes(t *testing.T) {
764	760		}).Once()
765	761
766	762		require.NoError(t, d.killSession(sess, 0, false))
	763	+	// closeAllPanes runs synchronously inside killSession, which has already
	764	+	// returned here. The store's write gate is still held below, so
	765	+	// persistence cannot have completed yet: this proves pane close did not
	766	+	// wait on it, with zero timing dependence on the encode worker.
	767	+	require.True(t, closed.Load(), "snapshot persistence must not delay pane close")
	768	+
	769	+	store.unblock()
767	770		awaitSnapshotClean(t, sess)
	771	+
	772	+	writes, name, data := store.recorded()
	773	+	require.Equal(t, 1, writes)
	774	+	require.Equal(t, "work", name)
	775	+	snap, err := snapcodec.Unmarshal(data)
	776	+	require.NoError(t, err)
	777	+	require.Equal(t, "work", snap.Name)
	778	+	require.Equal(t, "/live", snap.Tabs[0].Panes[0].Cwd)
768	779		}
769	780
770	781		func TestPTYReaderSynchronizedUpdateFlushMarksSnapshotDirty(t *testing.T) {

@@ -1061,6 +1072,8 @@ func awaitSnapshotIdle(t testing.TB, sess *session) {
1061	1072
1062	1073		func awaitSnapshotClean(t testing.T, sess session) {
1063	1074		t.Helper()
	1075	+	timer := time.NewTimer(testWaitTimeout)
	1076	+	defer timer.Stop()
1064	1077		for {
1065	1078		sess.snapshotMu.Lock()
1066	1079		clean := !sess.snapDirty.Load()

@@ -1069,7 +1082,11 @@ func awaitSnapshotClean(t testing.T, sess session) {
1069	1082		if clean {
1070	1083		return
1071	1084		}
1072		-	<-changed
	1085	+	select {
	1086	+	case <-changed:
	1087	+	case <-timer.C:
	1088	+	t.Fatal("timed out waiting for snapshot to become clean")
	1089	+	}
1073	1090		}
1074	1091		}
1075	1092

@@ -1090,6 +1107,11 @@ type gatedSnapshotStore struct {
1090	1107		release chan struct{}
1091	1108		once sync.Once
1092	1109		releaseOnce sync.Once
	1110	+
	1111	+	mu sync.Mutex
	1112	+	writes int
	1113	+	name string
	1114	+	data []byte
1093	1115		}
1094	1116
1095	1117		func newGatedSnapshotStore() *gatedSnapshotStore {

@@ -1099,14 +1121,28 @@ func newGatedSnapshotStore() *gatedSnapshotStore {
1099	1121		func (s *gatedSnapshotStore) enter() { s.once.Do(func() { close(s.entered) }) }
1100	1122		func (s *gatedSnapshotStore) unblock() { s.releaseOnce.Do(func() { close(s.release) }) }
1101	1123
1102		-	func (s *gatedSnapshotStore) Write(_ string, _ []byte) error {
	1124	+	func (s *gatedSnapshotStore) Write(name string, data []byte) error {
1103	1125		s.enter()
1104	1126		<-s.release
	1127	+	s.mu.Lock()
	1128	+	s.writes++
	1129	+	s.name = name
	1130	+	s.data = append([]byte(nil), data...)
	1131	+	s.mu.Unlock()
1105	1132		return nil
1106	1133		}
1107	1134		func (*gatedSnapshotStore) Load() ([]ports.SnapshotBlob, error) { return nil, nil }
1108	1135		func (*gatedSnapshotStore) Delete(string) error { return nil }
1109	1136
	1137	+	// recorded returns the write count and the most recently written name/data,
	1138	+	// captured under the store's own lock so it is safe to call from the test
	1139	+	// goroutine after the write has happened-before via awaitSnapshotClean.
	1140	+	func (s *gatedSnapshotStore) recorded() (int, string, []byte) {
	1141	+	s.mu.Lock()
	1142	+	defer s.mu.Unlock()
	1143	+	return s.writes, s.name, s.data
	1144	+	}
	1145	+
1110	1146		type neverReturningSnapshotStore struct {
1111	1147		entered chan struct{}
1112	1148		once sync.Once

 }
 func TestKillSessionSnapshotsNamedSessionBeforeClosingPanes(t *testing.T) {
-	store := portsmocks.NewMockSnapshotStore(t)
+	store := newGatedSnapshotStore()
 	d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{})
 	WithSnapshotStore(store)(d)
-	startSnapshotEncodeWorker(t, d)
+	d.startSnapshotEncodeWorker()
+	t.Cleanup(func() {
+		store.unblock()
+		d.stopSnapshotEncodeWorker()
+	})
 	d.procCwd = func(int) (string, error) { return "/live", nil }
 	sess := newSnapshotTestSession(t, "work", false, "/fallback")
 	d.sessions[sess.id] = sess
 	var closed atomic.Bool
-	store.EXPECT().Write("work", mock.Anything).RunAndReturn(func(_ string, data []byte) error {
-		snap, err := snapcodec.Unmarshal(data)
-		require.NoError(t, err)
-		require.Equal(t, "work", snap.Name)
-		require.Equal(t, "/live", snap.Tabs[0].Panes[0].Cwd)
-		require.True(t, closed.Load(), "snapshot persistence must not delay pane close")
-		return nil
-	}).Once()
 	mockPTY, ok := sess.tabs[0].panes["pane-1"].pty.(*portsmocks.MockPTY)
 	require.True(t, ok)
 	mockPTY.EXPECT().Close().RunAndReturn(func() error {
 	}).Once()
 	require.NoError(t, d.killSession(sess, 0, false))
+	// closeAllPanes runs synchronously inside killSession, which has already
+	// returned here. The store's write gate is still held below, so
+	// persistence cannot have completed yet: this proves pane close did not
+	// wait on it, with zero timing dependence on the encode worker.
+	require.True(t, closed.Load(), "snapshot persistence must not delay pane close")
+	store.unblock()
 	awaitSnapshotClean(t, sess)
+	writes, name, data := store.recorded()
+	require.Equal(t, 1, writes)
+	require.Equal(t, "work", name)
+	snap, err := snapcodec.Unmarshal(data)
+	require.NoError(t, err)
+	require.Equal(t, "work", snap.Name)
+	require.Equal(t, "/live", snap.Tabs[0].Panes[0].Cwd)
 }
 func TestPTYReaderSynchronizedUpdateFlushMarksSnapshotDirty(t *testing.T) {
 func awaitSnapshotClean(t *testing.T, sess *session) {
 	t.Helper()
+	timer := time.NewTimer(testWaitTimeout)
+	defer timer.Stop()
 	for {
 		sess.snapshotMu.Lock()
 		clean := !sess.snapDirty.Load()
 		if clean {
 			return
 		}
-		<-changed
+		select {
+		case <-changed:
+		case <-timer.C:
+			t.Fatal("timed out waiting for snapshot to become clean")
+		}
 	}
 }
 	release     chan struct{}
 	once        sync.Once
 	releaseOnce sync.Once
+	mu     sync.Mutex
+	writes int
+	name   string
+	data   []byte
 }
 func newGatedSnapshotStore() *gatedSnapshotStore {
 func (s *gatedSnapshotStore) enter()   { s.once.Do(func() { close(s.entered) }) }
 func (s *gatedSnapshotStore) unblock() { s.releaseOnce.Do(func() { close(s.release) }) }
-func (s *gatedSnapshotStore) Write(_ string, _ []byte) error {
+func (s *gatedSnapshotStore) Write(name string, data []byte) error {
 	s.enter()
 	<-s.release
+	s.mu.Lock()
+	s.writes++
+	s.name = name
+	s.data = append([]byte(nil), data...)
+	s.mu.Unlock()
 	return nil
 }
 func (*gatedSnapshotStore) Load() ([]ports.SnapshotBlob, error) { return nil, nil }
 func (*gatedSnapshotStore) Delete(string) error                 { return nil }
+// recorded returns the write count and the most recently written name/data,
+// captured under the store's own lock so it is safe to call from the test
+// goroutine after the write has happened-before via awaitSnapshotClean.
+func (s *gatedSnapshotStore) recorded() (int, string, []byte) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.writes, s.name, s.data
+}
 type neverReturningSnapshotStore struct {
 	entered chan struct{}
 	once    sync.Once