[Testing] Fix races and bugs found by flaky tests - #8633
Conversation
📝 WalkthroughWalkthroughThe changes address concurrent state updates, cache lookup and record ownership, gRPC shutdown handling, atomic registry startup state, asynchronous test behavior, and slashing message-type handling. ChangesOptimistic sync pipeline
Shared cache and record safety
Server and registry lifecycle handling
Slashing offense logging
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
janezpodhostnik
changed the title
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@network/alsp/internal/cache.go`:
- Around line 126-129: Replace the panic call in the error handler that logs
"unexpected error while getting spam record from cache" with
irrecoverable.NewExceptionf(...) instead, using the same descriptive error
message. This ensures backend faults in the cache are handled consistently
through Flow's exception mechanism rather than bypassing it with a raw panic.
In `@network/p2p/scoring/registry.go`:
- Around line 192-196: Update the startup guard around silencePeriodStartTime to
use CompareAndSwap(nil, &now) as an atomic one-shot initialization; if the swap
fails, return immediately from the current worker instead of calling
parent.Throw or continuing toward ready(). Preserve the timestamp initialization
and startup behavior for the winning worker.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bbe8b0d-3d8c-4daa-a2a0-b6dd5de7c161
📒 Files selected for processing (13)
module/executiondatasync/optimistic_sync/pipeline/pipeline.gomodule/executiondatasync/optimistic_sync/pipeline/pipeline_functional_test.gomodule/executiondatasync/optimistic_sync/pipeline/pipeline_test.gomodule/executiondatasync/optimistic_sync/pipeline/pipeline_test_utils.gomodule/grpcserver/server.gomodule/mempool/herocache/backdata/cache.gomodule/mempool/herocache/backdata/cache_test.gonetwork/alsp/internal/cache.gonetwork/p2p/inspector/validation/control_message_validation_inspector.gonetwork/p2p/scoring/internal/appSpecificScoreCache.gonetwork/p2p/scoring/internal/subscriptionCache.gonetwork/p2p/scoring/registry.gonetwork/slashing/consumer.go
Kay-Zee
left a comment
Kay-Zee
left a comment
There was a problem hiding this comment.
Approving — the fixes are the right shape, and I checked them against the surrounding code, not just the diff.
The optimistic-sync change is the trickiest and it holds up. Run's CAS (Pending → initial) can't clobber a concurrent newer parent update the way the old unconditional Store could, and unconditional store-then-notify in OnParentStateUpdated closes the lost-update window that could strand the pipeline at Processing forever. Spurious notifications are genuinely free: engine.Notifier is a non-blocking buffered-chan(1), and the loop re-derives everything from atomics, so the handlers are idempotent — onStartProcessing/onProcessing/onPersistChanges all gate on loaded state.
The herocache get change is the only true logic bug in the set, and the fix is exactly right: an ejected slot keeps its 32-bit prefix, so the old early-return could miss a real key living in a later slot of the same bucket. continue fixes it, and the loop-exhausted path still fires OnKeyGetFailure once — no double-counting, misses still counted. The regression test genuinely constructs the collision (same bucket, same 4-byte prefix, LRU ejection) and would fail on master.
The read-after-unlock races in the ALSP and scoring caches are all real: Backend.AdjustWithInit and Backend.Run hold the backend mutex while the adjust closure runs, so capturing penalty / copying records inside the closure is the correct fix, and copy-on-write in the two scoring caches keeps previously returned records immutable — GetSubscribedTopics does read Topics outside the lock, so the in-place append was racing.
grpcserver's ErrServerStopped tolerance is the standard shutdown idiom, and the pubsub slices.Clone is the minimal fix — only the slice header races, the messages are read-only.
Three nits, take or leave: ALSP Get panics on the impossible Run error rather than returning one (fine given the no-error signature); 500ms→5s is a 10x jump on the test timeouts, so masked failures take longer to surface; and silencePeriodStartTime nil→still-silenced is the right conservative read, though the atomic-pointer indirection is more ceremony than atomic.Int64 would have been.
3 participants
Fixes bugs discovered while stress-running the full unit test suite (500+ full-suite runs plus per-test
-racestress runs, continuing the work from #8626).Changes
Cache.getstopped scanning a bucket when it hit a slot whose value was ejected but whose stale 32-bit id prefix collided with the queried key, reporting a present entity as missing. Now continues scanning. Includes a deterministic regression test (verified to fail before the fix).Rununconditionally stored the initial parent state and could overwrite a concurrently deliveredOnParentStateUpdated, losing the update and deadlocking the pipeline (it would never observeStateCompleteand never persist).Runnow initializes with CompareAndSwap fromStatePending, andOnParentStateUpdateduses an unconditional store so updates cannot be silently dropped. Includes the pipeline test harness fixes this change interlocks with.ready()andServe,ServereturnsErrServerStopped, which was thrown as an irrecoverable error during a normal shutdown. Now excluded explicitly (gRPC returns nil when stopped while serving, so the exclusion is precise).SpamRecordCache:Getcopied record fields andAdjustWithInitread the adjusted penalty after the backend lock was released, racing in-place record mutations. Both now read under the lock.logOffensemutated the caller'sViolation(defaultingMsgType), racing concurrent use of the same violation. Uses a local variable now.silencePeriodStartTime(multi-wordtime.Time) was written by the startup worker while the score function read it concurrently, now an atomic pointer.AppSpecificScoreCacheandSubscriptionRecordCachemutated records in place whileGetreads fields outside the lock, both use copy-on-write now.inspectRpcPublishMessagesshuffled the live RPC's publish message slice on worker goroutines while libp2p pubsub still reads the same RPC. Now samples a shallow clone.All fixes verified with 20-100x
-racestress runs per affected package.network/alsp/...,network/p2p/scoring/..., andconsensus/hotstuff/integrationare now fully race-clean.Related: #8626, #8629
Follow-up PRs will add dedicated regression tests for paths that still lack deterministic coverage (grpcserver shutdown race, slashing consumer, pipeline initialization race).
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit