fix(nonce-manager): serialise refresh() under the per-account lock - #228
Merged
N-thnI merged 1 commit intoAug 29, 2026
Merged
Conversation
refresh() deleted the cache entry and issued its own getAccount round-trip without touching this.locks, so it could interleave with an in-flight reserve() for the same account. The suspended reserve() then overwrote the cache with its older read, discarding refresh()'s value and handing back a colliding sequence — on the recovery path an operator invokes after something has already gone wrong. Factor the wait/acquire/release dance out of reserve() into a shared withLock() helper and route both reserve() and refresh() through it. The wait is now a loop rather than a single await so multiple parked callers re-check the slot instead of waking together. Adds regression tests that suspend getAccount mid-reserve() and call refresh() (and the mirror case); both fail on the previous implementation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This Pull Request resolves a high-priority concurrency vulnerability in
NonceManagerwhererefresh()bypassed the per-account serialization lock (this.locks).By introducing a shared
withLockmutex helper and routing bothreserve()andrefresh()through it, this patch guarantees mutual exclusion across network I/O suspensions, preventing sequence collisions and cache corruption during manual recovery and fee-bump flows.Linked Issue
Summary of Changes
1. Per-Account Mutex Helper (
engine-bridge/src/nonce-manager.ts)withLockExecution: Factored per-account lock acquisition, execution, and release logic into a reusableprivate async withLock<T>(accountId: string, fn: () => Promise<T>): Promise<T>helper.reserve()andrefresh()throughwithLock, ensuring that an in-flightgetAccountnetwork round-trip cannot be interleaved with or overwritten by a concurrent refresh operation.2. Concurrency Regression Tests (
engine-bridge/src/__tests__/nonce-manager.test.ts)getAccountduringreserve()and triggersrefresh()mid-flight, verifying that the second operation queues behind the lock and preserves the latest sequence.reserve()during an in-flightrefresh().Verification Logs
withLockimplementation.tsc --noEmitis clean; zero new ESLint warnings or errors introduced (pre-existing warnings in untouched files remain unchanged).Notes for Maintainers
release()still performs cache adjustments outsidewithLock. Becauserelease()does not perform asynchronous network I/O, it was left untouched to maintain minimal diff scope, but it can be wrapped inwithLockin a future cleanup.