Skip to content

fix(rpc): guard RCManager subscriptions map and Info reads with the existing mutex#456

Open
amathxbt wants to merge 1 commit into
canopy-network:mainfrom
amathxbt:fix/rcmanager-subscription-race
Open

fix(rpc): guard RCManager subscriptions map and Info reads with the existing mutex#456
amathxbt wants to merge 1 commit into
canopy-network:mainfrom
amathxbt:fix/rcmanager-subscription-race

Conversation

@amathxbt

@amathxbt amathxbt commented Jul 4, 2026

Copy link
Copy Markdown

Bug

RCManager (in cmd/rpc/sock.go) protects its subscriptions map[uint64]*RCSubscription on the write side — AddSubscription() and RemoveSubscription() both correctly take r.l.Lock() before mutating the map — but nearly every read site accessed the map, and each subscription's mutable Info *lib.RootChainInfo field, without holding the lock:

func (r *RCManager) GetHeight(rootChainId uint64) uint64 {
	if sub, found := r.subscriptions[rootChainId]; found {
		return sub.Info.Height
	}
	return 0
}

The same unguarded pattern appeared in GetValidatorSet, GetOrders, GetOrder, IsValidDoubleSigner, GetMinimumEvidenceHeight, GetCheckpoint, GetLotteryWinner, Transaction, and GetDexBatch. sub.Info is itself reassigned under lock elsewhere (GetRootChainInfo does sub.Info = info while holding r.l), and the websocket read-loop can add/remove subscriptions concurrently with these RPC-handler reads — this is a genuine, reachable Go data race since RPC handlers run concurrently per-request.

Fix

Every read of r.subscriptions now takes r.l.Lock()/Unlock() around the map lookup. For functions that also read fields off sub.Info (GetHeight, GetValidatorSet, GetOrders, GetLotteryWinner), the relevant Info fields are snapshotted into local variables while still holding the lock, then the lock is released before any remote RPC call (e.g. sub.ValidatorSet(...), sub.Orders(...)) is made — this closes the race without holding the mutex across network I/O, preserving the existing performance/concurrency characteristics for remote calls.

Testing

Reviewed every call site of r.subscriptions in the file after the change to confirm all reads and writes are now consistently lock-protected, and that no lock is held during any blocking remote call (sub.RootChainInfo, sub.ValidatorSet, sub.Orders, sub.Order, sub.IsValidDoubleSigner, sub.MinimumEvidenceHeight, sub.Checkpoint, sub.Lottery, sub.Transaction, sub.DexBatch) to avoid introducing new lock-contention or deadlock risk.

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