diff --git a/server/services/mocks.go b/server/services/mocks.go index 57779cf..921ca51 100644 --- a/server/services/mocks.go +++ b/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() } diff --git a/server/services/mocks_test.go b/server/services/mocks_test.go index 828373e..a7ed1d7 100644 --- a/server/services/mocks_test.go +++ b/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