accounts/keystore: serialize scans with deletion

[pratikspatil024]

Summary

The nightly race run 30603502369 failed in accounts/keystore.TestWalletNotifications because a filesystem scan could race with KeyStore.Delete: the scan could read an account before deletion, then reinsert it into the cache after the file and cached entry were removed. This left a stale wallet and emitted an out-of-order WalletArrived event.

This behavior is inherited from upstream go-ethereum and is tracked in ethereum/go-ethereum#29830 and the open fix attempt ethereum/go-ethereum#30053. The fix serializes complete account scans with file deletion inside accountCache, so either the scan completes before deletion or it observes the post-deletion filesystem state. The strict event-order assertion remains unchanged. A regression test covers the error path where a key file disappears after successful key lookup.

Executed tests

• Before the fix: go test -race ./accounts/keystore -run '^TestWalletNotifications$' -count=200 reproduced two failures.
• After the fix: the same command passed 200/200 iterations.
• go test -race ./accounts/keystore -shuffle=1785471185402047521 -count=10
• go test -race ./accounts/keystore -run '^TestDeleteFileDisappearsAfterKeyLookup$' -count=20
• go test ./accounts/keystore
• go vet ./accounts/keystore
• make lint (0 issues)
• diffguard --base origin/develop . (100% mutation score, 10/10 killed)

gosec ./accounts/keystore/... could not build a complete SSA representation with the repository's Go 1.26 toolchain. Its reported findings were pre-existing and outside the changed lines.

Rollout notes

This is a backward-compatible keystore synchronization fix. It does not affect consensus, configuration, key formats, or persisted chain data. A deletion may briefly wait for an in-progress keystore scan; no coordinated rollout is required.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.07%. Comparing base (f50ee11) to head (bc9a507).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2336      +/-   ##
===========================================
- Coverage    54.08%   54.07%   -0.02%     
===========================================
  Files          907      907              
  Lines       162090   162101      +11     
===========================================
- Hits         87674    87654      -20     
- Misses       68990    69018      +28     
- Partials      5426     5429       +3     

Files with missing lines	Coverage Δ
accounts/keystore/account_cache.go	89.76% <100.00%> (+0.60%)	⬆️
accounts/keystore/keystore.go	76.80% <100.00%> (-0.08%)	⬇️

... and 27 files with indirect coverage changes

Files with missing lines	Coverage Δ
accounts/keystore/account_cache.go	89.76% <100.00%> (+0.60%)	⬆️
accounts/keystore/keystore.go	76.80% <100.00%> (-0.08%)	⬇️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pratikspatil024]

codegenie review

[github-actions]

codegenie review

No credible findings.

Coverage

Reviewed 6/6 hunks.
Coverage levels: deep 3, normal 2, light 1, skip 0.

Needs Human Attention

• Whether TestWalletNotifications' strict event-order assertions were left unchanged by this PR (no hunk for it appeared in the packet).
  Files: accounts/keystore/keystore_test.go
  Symbols: TestWalletNotifications, checkEvents
  Reason: Packet reviewer could not resolve this question from the reviewed context.

Stats

Model: anthropic claude-opus-5 high
Elapsed time: 2m 15s
Git: 0xPolygon/bor from develop to ppatil/nightly-race-wallet-notifications (bc9a507)
Review completeness: complete.
Usage: model calls 29, tokens 528616, cost $2.1827.
Effective caps: tokens 8000000.
Local context pressure: 2 tool-budget rejections, 3 degraded tool results.

No Findings

No credible findings were found.

— workflow run

[Copilot]

🟢 Ready to approve

The concurrency fix is narrowly scoped, uses consistent lock ordering to avoid deadlocks, and is backed by targeted race/regression tests matching the reported failure mode.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Fixes a keystore cache/event race where a concurrent filesystem scan could re-add an account after KeyStore.Delete removed the key file and cache entry, resulting in stale wallets and out-of-order wallet events. The change serializes full keystore scans with key-file deletion inside accountCache, and adds a regression test for the “file disappears after key lookup” delete path.

Changes:

• Serialize accountCache.scanAccounts() with key-file deletion via a new scanMu.
• Route KeyStore.Delete through accountCache.deleteFile to ensure deletion is scan-serialized.
• Add TestDeleteFileDisappearsAfterKeyLookup to cover deletion behavior when the key file vanishes after successful lookup.

File summaries

File	Description
accounts/keystore/keystore.go	Switches Delete to a scan-serialized deletion path (cache.deleteFile) and keeps wallet refresh behavior.
accounts/keystore/account_cache.go	Adds scanMu and uses it to serialize full scans with file deletion to prevent stale cache re-insertion.
accounts/keystore/keystore_test.go	Adds a regression test that simulates key-file disappearance after GetKey and asserts Delete returns os.IsNotExist.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.