Skip to content

fix(memory): hold read lock while scanning sessions in SearchMemory#1139

Open
winklemad wants to merge 1 commit into
google:mainfrom
winklemad:fix-memory-search-race
Open

fix(memory): hold read lock while scanning sessions in SearchMemory#1139
winklemad wants to merge 1 commit into
google:mainfrom
winklemad:fix-memory-search-race

Conversation

@winklemad

Copy link
Copy Markdown

Summary

InMemoryService.SearchMemory has a data race: it reads the per-user session map under the read lock, but releases the lock before iterating. AddSessionToMemory writes into that same map under the write lock, so a concurrent Search and Add on the same app/user race — and can panic with fatal error: concurrent map iteration and map write. The service is documented as thread-safe.

No linked issue — found while reviewing the in-memory services under the race detector.

Root cause

s.mu.RLock()
values, ok := s.store[k]
s.mu.RUnlock()          // lock released here
...
for _, events := range values {   // shared map iterated with no lock held

values is the shared per-user map[sessionID][]value; iterating it after releasing the lock races with AddSessionToMemory, which does v[sid] = values under the write lock.

Fix

Hold the read lock across the whole scan. AddSessionToMemory replaces session slices wholesale rather than mutating them in place, so a read lock over the iteration is sufficient and still lets searches run concurrently. This mirrors the fix already applied to the session package's state.All() (#561).

Tests

  • memory: added Test_inMemoryService_SearchMemory_Concurrent, which runs SearchMemory and AddSessionToMemory on the same app/user in parallel. Under -race (as CI runs it) it reports a DATA RACE at inmemory.go on main and passes with this change.

Verified locally with the CI command go test -race -mod=readonly -count=1 -shuffle=on ./... (all packages pass), plus go vet, gofmt, and golangci-lint run clean.

InMemoryService.SearchMemory read the per-user session map under the read
lock but released it before iterating, while AddSessionToMemory writes into
that same map under the write lock. Concurrent Search and Add on the same
app/user therefore race and can panic with "concurrent map iteration and
map write", even though the service is documented as thread-safe.

Hold the read lock across the whole scan. AddSessionToMemory replaces session
slices wholesale rather than mutating them in place, so a read lock over the
iteration is sufficient and still lets searches run concurrently.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant