Skip to content

test(session): add concurrent regression test for State.All()#725

Open
nuthalapativarun wants to merge 4 commits intogoogle:mainfrom
nuthalapativarun:fix/561-state-all-concurrent-race-test
Open

test(session): add concurrent regression test for State.All()#725
nuthalapativarun wants to merge 4 commits intogoogle:mainfrom
nuthalapativarun:fix/561-state-all-concurrent-race-test

Conversation

@nuthalapativarun
Copy link
Copy Markdown

Link to Issue

Fixes #561

Description of Change

Problem:
state.All() previously released the read lock between each map iteration step (s.mu.RUnlock() inside the range s.state loop), allowing concurrent Set() calls to mutate the map mid-iteration and trigger a fatal "concurrent map iteration and map write" panic.

Solution:
The fix (snapshotting the map under lock via maps.Clone before iterating) is already in session/inmemory.go. However, no concurrent test existed to verify this and guard against future regressions. This PR adds TestInMemoryState_All_ConcurrentSet which:

  • Creates a session with initial state
  • Races State.All() (200 iterations) against 8 goroutines each appending events with state deltas concurrently
  • Passes cleanly under go test -race

Testing Plan

Unit Tests:

  • TestInMemoryState_All_ConcurrentSet added to session/inmemory_test.go
  • All tests pass with the race detector enabled
$ go test -race ./session/...
ok  	google.golang.org/adk/session	1.357s
ok  	google.golang.org/adk/session/database	1.619s
ok  	google.golang.org/adk/session/vertexai	35.586s

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

state.All() previously released the read lock between each map
iteration step, allowing concurrent Set() calls to trigger a fatal
"concurrent map iteration and map write" panic (issue google#561).

The fix (maps.Clone snapshot under lock) is already in place.
This commit adds TestInMemoryState_All_ConcurrentSet to verify
the fix holds under -race and prevent future regressions: it races
State.All() against 8 concurrent goroutines each appending events
with state deltas.

Fixes google#561
@nuthalapativarun
Copy link
Copy Markdown
Author

Hi, just checking in on this one — happy to make any adjustments if something needs to change. Let me know if there's anything blocking review. Thanks!

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.

inmemory State.All unlocks during map iteration -> concurrent map iteration panic

1 participant