[BUG] NonceManager.reserve() has a check-then-act race allowing duplicate sequence reservations

[N-thnI]

Description

NonceManager.reserve() uses a check-then-act lock that isn't atomic, so two concurrent reservations for the same account can both pass the guard and come back with the identical sequence number — the exact collision this class exists to prevent.

Location

engine-bridge/src/nonce-manager.ts:24-39 (reserve), engine-bridge/src/nonce-manager.ts:65-68 (waitForLock)

Current Behavior

async reserve(accountId: string): Promise<bigint> {
    await this.waitForLock(accountId);        // (1) check

    let resolve!: () => void;
    const lock = new Promise<void>(r => { resolve = r; });
    this.locks.set(accountId, lock);           // (2) act

    try {
      const seq = await this.nextSequence(accountId);
      this.cache.set(accountId, seq + 1n);
      return seq;
    } finally {
      this.locks.delete(accountId);
      resolve();
    }
}

private async waitForLock(accountId: string): Promise<void> {
    const existing = this.locks.get(accountId);
    if (existing) await existing;
}

The lock is only written in step (2), inside reserve itself, after waitForLock has already returned. If two calls to reserve(accountId) are made back-to-back (e.g. Promise.all([mgr.reserve(acc), mgr.reserve(acc)])), both invocations read this.locks.get(accountId) as undefined before either one reaches the locks.set(...) line, because await this.waitForLock(...) yields a microtask even when there's nothing to wait for. Both calls then proceed to set their own lock entries (the second overwrites the first in the Map) and both call nextSequence concurrently.

Expected Behavior

Only one in-flight reserve() call per accountId should ever be inside the critical section at a time; a second concurrent call must wait for the first to fully release its lock before reading/incrementing the cached sequence.

Repro / Evidence

const mgr = new NonceManager(rpc);
const [a, b] = await Promise.all([
  mgr.reserve("GABC..."),
  mgr.reserve("GABC..."),
]);
// a === b in the racing case — two transactions built with the same
// sequence number, one of which will be rejected by the network (or worse,
// silently reordered if submitted through different RPC nodes).

This traces directly from the code: the lock map is only mutated after the async gap that both callers pass through together, so the "lock" never actually excludes the second caller in the race window.

Impact

This is the module whose entire purpose is "serialise reservation... to prevent races" (per its own doc comment). A race here produces duplicate Stellar sequence numbers under concurrent load — transaction failures at best, misordered fund-moving transactions at worst. nonce-manager.ts also currently has no test file (engine-bridge/src/__tests__/ has no nonce-manager.test.ts), so this has no regression coverage.

Suggested Fix

Register the lock synchronously before any await, e.g. by building a single promise chain per account (this.locks.set(accountId, (this.locks.get(accountId) ?? Promise.resolve()).then(...))) so the "check" and "act" happen in the same synchronous tick, instead of splitting them across waitForLock + a later locks.set.

Acceptance Criteria

• AC-1: Two concurrent reserve() calls for the same accountId never return the same sequence number.
• AC-2: A new test in engine-bridge/src/__tests__/nonce-manager.test.ts reproduces the race (e.g. fires N concurrent reserve() calls and asserts all N returned sequences are unique) and fails against the current implementation.
• AC-3: Existing release/refresh behavior is unchanged for the single-caller case.

Definition of Done

• Fix merged with all AC items checked.
• Regression test passes in CI.
• No new eslint warnings introduced.

Labels: bug