Description

Problem

Two subtests in TestPrivateTxSubmissionRetry failed intermittently on CI:

--- FAIL: TestPrivateTxSubmissionRetry/retry_succeeds_after_N_attempts (4.10s)
    multiclient_test.go:925: expected server 0 to be called 3 times
                             expected: 3, actual: 2

--- FAIL: TestPrivateTxSubmissionRetry/retry_stops_when_tx_found_in_local_database (2.10s)
    multiclient_test.go:968: expected server 0 to be called only once, no retries after tx found
                             expected: 1, actual: 2

The tests passed consistently on a developer laptop but failed on CI runners, making the root cause non-obvious.

Root Cause

There were two interrelated timing races, both stemming from the same pattern: the tests used time.Sleep(N × retryInterval + 100ms) to wait for background retry goroutines to complete, then immediately asserted on call counts.

Race 1 — insufficient buffer (retry_succeeds_after_N_attempts):

retryPrivateTxSubmission sleeps privateTxRetryInterval (2s) before each attempt. After its second sleep the goroutine wakes at t ≈ 4s and spawns concurrent HTTP call goroutines. The test asserts at t = 4.1s — only 100ms later. On a loaded CI runner, those HTTP calls took longer than 100ms to complete, so the assertion ran before the third call was recorded:

t=0s    initial submission → counts[0]=1, retry goroutine spawned
t=2s    retry 1 → counts[0]=2, goroutine starts second 2s sleep
t=4s    goroutine wakes, spawns HTTP goroutines for retry 2
t=4.1s  test assertion → sees counts[0]=2  ← RACE: retry 2 not done yet
t=4s+ε  retry 2 goroutines complete → counts[0]=3   (too late)

Race 2 — cross-subtest goroutine leakage (retry_stops_when_tx_found_in_local_database):

All subtests share the same rpcServers array. Subtest 1's retry goroutine outlived subtest 1: when the test assertion at t=4.1s raced subtest 1's second retry (still in progress), the test ended and subtest 2 immediately installed new handlers on the shared servers. Subtest 1's still-running HTTP goroutines then landed on subtest 2's server 0 handler, spuriously incrementing subtest 2's callCounts[0] from 1 to 2.

Reproducing Consistently

The failure only appeared on loaded machines because the 100ms buffer was tight. To reproduce it locally we temporarily injected a 150ms sleep inside the retry goroutine's HTTP call to simulate CI scheduler overhead:

go func(client *rpc.Client, idx int) {
    defer retryWg.Done()
    time.Sleep(150 * time.Millisecond) // simulate CI latency
    err := client.CallContext(...)

With this, both subtests failed 3/3 runs with the exact same error messages as CI. After removing the sleep and applying the fix, both subtests passed 3/3 under the same simulated conditions.

Fix

multiclient.go — two changes:

1. Added retryInterval time.Duration field to multiClient (zero value falls back to the 2s production constant). This lets tests set a short interval (10ms) so the retry goroutine completes near-instantly, eliminating the slow sleeping from the test timeline entirely.

2. submitPrivateTx now returns (error, <-chan struct{}). When a retry goroutine is started, the channel is closed when the goroutine finishes. This gives callers a deterministic signal instead of forcing them to guess how long to sleep.

service.go — updated the single production call site to discard the done channel (err, _ = ...), as the service has no use for it.

multiclient_test.go — all retry subtests updated to:

• Set mc.retryInterval = 10 * time.Millisecond after construction
• Replace every time.Sleep(N × retryInterval + 100ms) with select { case <-done: case <-time.After(5s): t.Fatal(...) }

This eliminates both races: <-done guarantees the goroutine has fully completed before any assertion runs, and since subtest N's goroutine is provably done before subtest N+1 installs new handlers, there is no cross-subtest leakage.

Result

• TestPrivateTxSubmissionRetry was previously taking ~55s per run (dominated by 2s sleep intervals) and failing intermittently. It now runs in ~2.3s total and passes 10/10 with -race.
• No behavioural change to production retry logic — only the retry interval is now injectable for testing.

Changes

• Bugfix (non-breaking change that solves an issue)
• Hotfix (change that solves an urgent issue, and requires immediate attention)
• New feature (non-breaking change that adds functionality)
• Breaking change (change that is not backwards-compatible and/or changes current functionality)
• Changes only for a subset of nodes