Skip to content

fix(storev2): hold read lock in GetCommitKVStore during concurrent map access#3517

Open
amir-deris wants to merge 6 commits into
mainfrom
amir/plt-416-fix-concurrent-map-access
Open

fix(storev2): hold read lock in GetCommitKVStore during concurrent map access#3517
amir-deris wants to merge 6 commits into
mainfrom
amir/plt-416-fix-concurrent-map-access

Conversation

@amir-deris

@amir-deris amir-deris commented May 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds RLock/RUnlock to GetCommitKVStore in rootmulti/store.go so reads of the ckvStores map are properly synchronized against concurrent writes.
  • Adds TestGetCommitKVStoreNoDataRace to exercise the concurrent read/write path; run with go test -race ./sei-cosmos/storev2/rootmulti/....

Test plan

  • go test -race ./sei-cosmos/storev2/rootmulti/... passes with no data race reports

@cursor

cursor Bot commented May 28, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches core multistore concurrency on the commit path; the change is small (read locks only) but incorrect locking here could cause races or subtle staleness under load.

Overview
Fixes unsynchronized reads of the ckvStores map in storev2 rootmulti.Store by taking RLock/RUnlock in GetStore, GetKVStore, and GetCommitKVStore, matching accessors like CacheMultiStore that already guard the same map.

That keeps concurrent Commit / LoadVersionAndUpgrade paths (which replace or reload entries under a write lock) from racing map lookups during block execution, flush, and queries.

Adds TestGetCommitKVStore_ReaderRespectsWriteLock, which holds a write lock, starts a reader, and asserts the read blocks until unlock and then sees the updated store pointer.

Reviewed by Cursor Bugbot for commit 42f5662. Bugbot is set up for automated code reviews on this repo. Configure here.

@amir-deris amir-deris changed the title Added concurrent safe read for getComitKVStore and added test fix(storev2): hold read lock in GetCommitKVStore during concurrent map access May 28, 2026
@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJun 11, 2026, 8:36 AM

@amir-deris amir-deris requested review from bdchatham and masih May 28, 2026 19:03
@codecov

codecov Bot commented May 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.10%. Comparing base (09c70ba) to head (42f5662).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3517      +/-   ##
==========================================
- Coverage   59.17%   58.10%   -1.07%     
==========================================
  Files        2215     2131      -84     
  Lines      183370   173058   -10312     
==========================================
- Hits       108512   100563    -7949     
+ Misses      65047    63522    -1525     
+ Partials     9811     8973     -838     
Flag Coverage Δ
sei-chain-pr 65.48% <100.00%> (?)
sei-db 70.41% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sei-cosmos/storev2/rootmulti/store.go 67.36% <100.00%> (+0.31%) ⬆️

... and 275 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bdchatham

Copy link
Copy Markdown
Contributor

Could we tighten the test? Something like:

func TestGetCommitKVStore_ReaderRespectsWriteLock(t *testing.T) {
    store := &Store{
        storeKeys: map[string]types.StoreKey{},
        ckvStores: map[types.StoreKey]types.CommitKVStore{},
    }
    key := types.NewKVStoreKey("bank")
    store.storeKeys[key.Name()] = key
    store.ckvStores[key] = mem.NewStore()

    store.mtx.Lock()

    readDone := make(chan types.CommitKVStore, 1)
    go func() {
        readDone <- store.GetCommitKVStore(key)
    }()

    select {
    case <-readDone:
        t.Fatal("GetCommitKVStore returned while write lock held — RLock missing")
    case <-time.After(50 * time.Millisecond):
    }

    newVal := mem.NewStore()
    store.ckvStores = map[types.StoreKey]types.CommitKVStore{key: newVal}
    store.mtx.Unlock()

    require.Same(t, newVal, <-readDone)
}

This deterministically verifies the RLock is doing its job — the read blocks while the write lock is held, then observes the post-write value — instead of relying on the scheduler to overlap two unsynchronized goroutines and the race detector to notice.

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e5a3ced. Configure here.

Comment thread sei-cosmos/storev2/rootmulti/store.go
@masih

masih commented May 29, 2026

Copy link
Copy Markdown
Collaborator

@codex review this pr

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants