Skip to content

Synchronize the in-memory checkpoint manager#522

Open
PratikDhanave wants to merge 4 commits into
microsoft:mainfrom
PratikDhanave:fix-inmemory-checkpoint-manager-race
Open

Synchronize the in-memory checkpoint manager#522
PratikDhanave wants to merge 4 commits into
microsoft:mainfrom
PratikDhanave:fix-inmemory-checkpoint-manager-race

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

Summary

inMemoryManager (workflow/checkpoint/manager.go) has an unsynchronized Store map[string]*SessionCache and per-session caches. The manager is keyed by session ID, and NewInMemoryManager documents no single-use restriction — so sharing one manager across concurrent workflow runs (distinct sessions committing at the same time) is a natural pattern. Under that use, concurrent Commit calls race on the map and can crash the process with fatal: concurrent map writes.

Under -race, a handful of concurrent commits reliably report data races on the Store access in sessionStore.

Fix

Guard Store with a sync.Mutex in Commit, Lookup, and RetrieveIndex. The critical sections are pure in-memory map/cache operations (no I/O), so holding the lock across each method is cheap.

Design note: I fixed this by making the in-memory manager safe for concurrent use, since its session-keyed design invites sharing. The alternative is to document it as not concurrency-safe, mirroring FileSystemJSONStore's existing caveat, and leave callers to serialize. Happy to switch to the doc-only approach if you'd prefer the stores be consistent in that direction instead.

Public API

No exported symbols change.

Tests

Adds TestInMemoryManager_ConcurrentSessions_NoRace (internal test): 64 goroutines Commit and RetrieveIndex distinct sessions concurrently. It reports data races under -race before this change and passes after.

go test -race ./workflow/checkpoint passes; go build ./... and go vet clean; gofumpt clean.

inMemoryManager.Store (and its per-session caches) had no synchronization,
but the manager is keyed by session ID and NewInMemoryManager carries no
single-use caveat -- so sharing one manager across concurrent workflow runs
(distinct sessions) is a natural pattern. Concurrent Commit calls then race
on the map and can crash with 'fatal: concurrent map writes'.

Guard Store with a mutex in Commit, Lookup, and RetrieveIndex. The critical
sections are pure in-memory map/cache operations (no I/O), so holding the
lock across each method is cheap.

Adds TestInMemoryManager_ConcurrentSessions_NoRace: 64 goroutines commit and
read distinct sessions concurrently; it reports data races under -race before
this change and passes after.
Copilot AI review requested due to automatic review settings July 17, 2026 05:09
@PratikDhanave
PratikDhanave requested a review from a team as a code owner July 17, 2026 05:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Synchronizes the in-memory checkpoint Manager implementation to prevent concurrent access races when a single inMemoryManager is shared across multiple concurrent workflow sessions.

Changes:

  • Added a mutex to inMemoryManager and guarded Commit, Lookup, and RetrieveIndex to protect the session Store map and per-session caches.
  • Added an internal concurrency test intended to reproduce the prior race under -race and validate the fix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
workflow/checkpoint/manager.go Adds a sync.Mutex to inMemoryManager and locks around Store/cache access in key methods.
workflow/checkpoint/manager_race_internal_test.go Adds a concurrent multi-session test to validate race-free behavior under -race.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants