fix(accounts/keystore): fix flaky TestUpdatedKeyfileContents notification race#2231
fix(accounts/keystore): fix flaky TestUpdatedKeyfileContents notification race#2231gzliudan wants to merge 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
Copilot,
liam-lai and
wanwiset25
There was a problem hiding this comment.
Pull request overview
This PR addresses flakiness in accounts/keystore’s TestUpdatedKeyfileContents by changing the waitForAccounts helper to decouple “accounts list matches expected” from “keystore change notification received”, removing a timing dependency between cache visibility and channel delivery.
Changes:
- Updates
waitForAccountsto wait (within the same timeout window) until both the expected account list is observed and aks.changesnotification is observed. - Defers the “wasn't notified of new accounts” failure until the timeout elapses (instead of failing immediately on a single check).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !haveAccounts { | ||
| list = ks.Accounts() | ||
| haveAccounts = reflect.DeepEqual(list, wantAccounts) | ||
| } | ||
| if !haveChange { | ||
| select { | ||
| case <-ks.changes: | ||
| haveChange = true | ||
| default: | ||
| return errors.New("wasn't notified of new accounts") | ||
| } | ||
| } | ||
| if haveAccounts && haveChange { | ||
| return nil |
There was a problem hiding this comment.
haveAccounts is latched true after the first time ks.Accounts() matches wantAccounts, and the helper stops re-reading the account list. Since the cache update path can expose intermediate states (e.g., scanAccounts processes updates as deleteByFile then add with separate locks), a transient match can be observed and then later diverge; this helper would still return success once a change is seen, or return the "wasn't notified" error even if accounts no longer match. Recompute list/equality each loop (or at least re-check immediately before returning and before deciding the final error) so success requires the current account list equals wantAccounts while still allowing the change and match to happen in either order.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…tion race TestUpdatedKeyfileContents was intermittently failing with: - Emptying account file failed - wasn't notified of new accounts Root cause: waitForAccounts required the account list match and an immediately readable ks.changes notification in the same instant, creating a timing race between cache update visibility and channel delivery. This change keeps the same timeout window but waits until both conditions are observed, which preserves test intent while removing the flaky timing dependency. Validation: - go test ./accounts/keystore -run '^TestUpdatedKeyfileContents$' -count=100
gzliudan
force-pushed
the
fix-xdc-TestUpdatedKeyfileContents
branch
from
c3b0ad3 to
495921f
Compare
Proposed changes
TestUpdatedKeyfileContents was intermittently failing with:
Root cause: waitForAccounts required the account list match and an immediately readable ks.changes notification in the same instant, creating a timing race between cache update visibility and channel delivery.
This change keeps the same timeout window but waits until both conditions are observed, which preserves test intent while removing the flaky timing dependency.
Validation:
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that