Merge pull request #332 from smocker-dev/fix/session-race

fix(server): create the default session atomically (no duplicate "Session #1")

Author: gwleclerc

server/services/mocks.go

@@ -302,6 +302,16 @@ func (s *mocks) GetHistoryByPath(sessionID, filterPath string) (types.History, e
 }
 
 func (s *mocks) NewSession(name string) *types.Session {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.newSessionLocked(name)
+}
+
+// newSessionLocked appends a new session and returns it. The caller MUST hold s.mu: the default
+// name and the carried-over locked mocks are both derived from s.sessions, so the whole thing has
+// to happen in one critical section — otherwise two concurrent creators can read the same length
+// and both mint e.g. "Session #1" (the bug behind the flaky rename e2e test).
+func (s *mocks) newSessionLocked(name string) *types.Session {
 	if strings.TrimSpace(name) == "" {
 		name = fmt.Sprintf("Session #%d", len(s.sessions)+1)
 	}
@@ -313,21 +323,16 @@ func (s *mocks) NewSession(name string) *types.Session {
 		history = types.History{}
 	}
 
+	// Carry over the locked mocks from the current last session, reset for the new one.
 	mocks := types.Mocks{}
 	if len(s.sessions) > 0 {
-		session := s.GetLastSession()
-		s.mu.Lock()
-		for _, mock := range session.Mocks {
+		for _, mock := range s.sessions[len(s.sessions)-1].Mocks {
 			if mock.State.Locked {
 				mocks = append(mocks, mock.CloneAndReset())
 			}
 		}
-		s.mu.Unlock()
 	}
 
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
 	session := &types.Session{
 		ID:      types.NewID(),
 		Name:    name,
@@ -384,12 +389,10 @@ func (s *mocks) DeleteSession(id string) error {
 
 func (s *mocks) GetLastSession() *types.Session {
 	s.mu.Lock()
+	defer s.mu.Unlock()
 	if len(s.sessions) == 0 {
-		s.mu.Unlock()
-		s.NewSession("")
-		s.mu.Lock()
+		return s.newSessionLocked("").Clone()
 	}
-	defer s.mu.Unlock()
 	return s.sessions[len(s.sessions)-1].Clone()
 }
 

server/services/mocks_test.go

@@ -3,6 +3,7 @@ package services
 import (
 	"os"
 	"path/filepath"
+	"sync"
 	"testing"
 
 	"github.com/smocker-dev/smocker/server/types"
@@ -18,6 +19,31 @@ func newTestMocks(t *testing.T) Mocks {
 	return svc
 }
 
+// TestGetLastSessionConcurrent guards the TOCTOU race that let concurrent callers each create a
+// default session (two "Session #1") when the list was empty — the source of the flaky "rename a
+// session" e2e test. Run under -race.
+func TestGetLastSessionConcurrent(t *testing.T) {
+	svc := newTestMocks(t)
+
+	const n = 50
+	start := make(chan struct{})
+	var wg sync.WaitGroup
+	wg.Add(n)
+	for i := 0; i < n; i++ {
+		go func() {
+			defer wg.Done()
+			<-start
+			_ = svc.GetLastSession()
+		}()
+	}
+	close(start) // release all goroutines at once to maximize contention
+	wg.Wait()
+
+	if got := len(svc.GetSessions()); got != 1 {
+		t.Fatalf("concurrent GetLastSession on an empty service created %d sessions, want 1", got)
+	}
+}
+
 func mockFromYAML(t *testing.T, s string) *types.Mock {
 	t.Helper()
 	var m types.Mock