[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
| if err != nil { | ||
| // the Run closure above never returns an error; an error here indicates a bug in the | ||
| // backend implementation. | ||
| panic(fmt.Errorf("unexpected error while getting spam record from cache: %w", err)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)cache\.go$|module/component/component\.go|irrecoverable|docs/agents/(CodingConventions|GoDocs)\.md' || true
echo "== target snippet =="
if [ -f network/alsp/internal/cache.go ]; then
sed -n '1,220p' network/alsp/internal/cache.go | nl -ba | sed -n '1,220p'
fi
echo "== irrecoverable usage examples =="
rg -n "irrecoverable|panic\(fmt\.Errorf|panic\(" -S --glob '*.go' . | head -200Repository: onflow/flow-go
Length of output: 1148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read docs guidelines =="
for f in docs/agents/CodingConventions.md docs/agents/GoDocs.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
else
echo "missing $f"
fi
doneRepository: onflow/flow-go
Length of output: 28678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file =="
awk '{printf "%6d\t%s\n", NR, $0}' network/alsp/internal/cache.go | sed -n '1,240p'
echo "== irrecoverable package =="
awk '{printf "%6d\t%s\n", NR, $0}' module/irrecoverable/exception.go | sed -n '1,240p'
awk '{printf "%6d\t%s\n", NR, $0}' module/irrecoverable/irrecoverable.go | sed -n '1,240p'
echo "== irrecoverable import/usages in network/alsp/internal/cache.go and neighboring alsp cache files =="
rg -n 'irrecoverable|panic\(fmt\.Errorf|fmt\.Errorf|No error returns are expected|Unexpected error' network/alsp -S network/alsp -S || true
echo "== component interface =="
awk '{printf "%6d\t%s\n", NR, $0}' module/component/component.go | sed -n '1,240p'Repository: onflow/flow-go
Length of output: 34983
Use irrecoverable exceptions for backend faults.
This cache documents returned errors as irrecoverable bugs, but panic(fmt.Errorf(...)) bypasses Flow’s exception mechanism. Use irrecoverable.NewExceptionf(...) here or refactor the cache methods to return exceptions consistently.
🤖 Prompt for 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.
In `@network/alsp/internal/cache.go` around lines 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.
Source: Coding guidelines
| if reg.silencePeriodStartTime.Load() != nil { | ||
| parent.Throw(fmt.Errorf("gossipsub scoring registry started more than once")) | ||
| } | ||
| reg.silencePeriodStartTime = time.Now() | ||
| now := time.Now() | ||
| reg.silencePeriodStartTime.Store(&now) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n 'go\.uber\.org/atomic v1\.11\.0' go.mod go.sum
rg -n -C 5 'type SignalerContext|Throw\(' module/irrecoverable --glob '*.go'
rg -n -C 3 'CompareAndSwap\(' --glob '*.go'Repository: onflow/flow-go
Length of output: 5471
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Registry outline:\n'
ast-grep outline network/p2p/scoring/registry.go --match Start --view expanded || true
printf '\nRelevant registry lines:\n'
sed -n '160,220p' network/p2p/scoring/registry.go
printf '\nStart call sites/usages:\n'
rg -n -C 3 'gossipsub scoring registry started more than once|scoring.*registry|SilencePeriodStartTime|silencePeriodStartTime' network/p2p/scoring module/irrecoverable --glob '*.go'
printf '\nIrrecoverable implementation:\n'
sed -n '1,80p' module/irrecoverable/irrecoverable.go
printf '\nAtomic API availability snippet from Go stdlib if vendored locally not applicable; run simple Go program unless go unavailable:\n'
if command -v go >/dev/null 2>&1; then
tmp="$(mktemp -d)"
cat > "$tmp/cas.go" <<'EOF'
package main
import (
"fmt"
"go/ast"
"go/parser"
"go/token"
)
func main() {
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, "src/go.uber.org/atomic/atomic", `package atomic
type Int64 struct { v int64 }
func (p *Int64) CompareAndSwap(old int64, new int64) { _ = 1 }
`, parser.ParseComments)
if err != nil {
fmt.Println("parse")
return
}
for _, s := range f.Pkg.Scope.Objects {
fmt.Println("obj", s.Name)
}
}
EOF
true
else
echo "go unavailable"
fiRepository: onflow/flow-go
Length of output: 13612
🌐 Web query:
go.uber.org/atomic Pointer CompareAndSwap nil pointer documentation
💡 Result:
In the go.uber.org/atomic library, the behavior regarding nil pointers when using CompareAndSwap depends on the specific atomic type being used. For atomic.Pointer[T] The atomic.Pointer[T] type is a generic wrapper introduced to provide type-safe atomic operations on pointers [1]. It does not impose a restriction against nil pointers. You can use nil as either the 'old' or 'new' argument in a CompareAndSwap operation [2]. This behavior aligns with standard Go pointer semantics, where nil is a valid value for any pointer type (*T). Contrast with atomic.Value It is important to distinguish this from the atomic.Value type (also provided by go.uber.org/atomic), which shadows the standard library's sync/atomic.Value [3]. The atomic.Value.CompareAndSwap and atomic.Value.Store methods will panic if provided with nil [4][3]. Summary If you are using the modern generic atomic.Pointer[T], nil pointers are permitted [2]. If you are using atomic.Value, passing nil will cause a panic [3].
Citations:
- 1: https://github.com/uber-go/atomic/blob/master/CHANGELOG.md
- 2: https://github.com/uber-go/atomic/blob/master/pointer_go119.go
- 3: https://ref.gotd.dev/pkg/go.uber.org/atomic.html
- 4: https://pkg.go.dev/sync/atomic
🌐 Web query:
go runtime Goexit inside goroutine continues scheduling
💡 Result:
When runtime.Goexit is called, it terminates the specific goroutine that invoked it, but it does not stop the Go scheduler or affect other running goroutines [1][2]. The Go scheduler continues to manage and execute other goroutines as normal [3][2]. The behavior of Goexit can be understood through the following points: 1. Termination of the Goroutine: Goexit causes the calling goroutine to terminate. It executes all deferred functions registered in that goroutine before it finally exits [1][2]. 2. Scheduler Continuity: The runtime scheduler remains active. If other goroutines are runnable, the scheduler will continue to pick them up and execute them [3][2]. 3. Main Goroutine Special Case: If runtime.Goexit is called from the main goroutine, the main goroutine terminates, but the program itself does not exit immediately [2]. Because the main function has not officially returned, the program continues to run as long as other goroutines are active [2]. If all other goroutines finish or exit, the program will crash with a deadlock error because it cannot complete the main function [2]. 4. Internals: When a goroutine finishes or calls Goexit, the runtime executes a cleanup process (goexit0) that transitions the goroutine state to _Gdead, unbinds it from its M (machine thread), puts the goroutine back into a free list for reuse, and calls the scheduler to pick the next runnable goroutine [4][5]. In summary, Goexit is a targeted termination mechanism for a single goroutine and does not halt the overall scheduling of other tasks in the Go runtime [1][2].
Citations:
- 1: https://go.googlesource.com/go/+/85cae877f57e645a13e2c1799ffb1f6592c7474b/src/pkg/runtime/extern.go
- 2: https://pkg.go.dev/runtime
- 3: runtime: program can exit 0 after runtime.Goexit from main golang/go#7711
- 4: https://golang.design/under-the-hood/en/part3concurrency/ch09sched/schedule/
- 5: https://github.com/golang/go/blob/a23a341e10132532b3922ad7a77508df24db75f2/src/runtime/proc1.go
Make startup initialization an atomic one-shot operation.
Load followed by Store and runtime.Goexit is not safe for the one-shot startup guard. Concurrent workers can both observe nil, and multiple workers may store different timestamps or call ready(). Use silencePeriodStartTime.CompareAndSwap(nil, &now) and return on the losing worker instead of continuing.
🤖 Prompt for 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.
In `@network/p2p/scoring/registry.go` around lines 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.
Source: Coding guidelines
2 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