[Access] Access ingestion error handle#8385
Conversation
📝 WalkthroughWalkthroughAdds Access metrics propagation to the ingestion engine, introduces a new ingestion-finalized-block-height metric and noop/mock implementations, and hardens finalized-block processing with context-aware error handling and updated metric reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant FinBlock as Finalized Block Event
participant Engine as Ingestion Engine
participant Storage as Storage Layer
participant Collections as Collection Lookup
participant Metrics as AccessMetrics
FinBlock->>Engine: OnFinalizedBlock(block)
Engine->>Storage: Index block
alt Indexing Success
Storage-->>Engine: success
Engine->>Collections: Request missing collections
Collections-->>Engine: collection data
Engine->>Metrics: UpdateIngestionFinalizedBlockHeight(height)
Metrics-->>Engine: recorded
Engine-->>FinBlock: processing complete
else ErrAlreadyExists
Storage-->>Engine: ErrAlreadyExists
Engine->>Engine: Log warning (height, blockID)
Engine->>Collections: Request missing collections
Collections-->>Engine: collection data
Engine->>Metrics: UpdateIngestionFinalizedBlockHeight(height)
Metrics-->>Engine: recorded
Engine-->>FinBlock: processing continues
else Other Error
Storage-->>Engine: error
Engine->>Engine: Wrap error with context
Engine-->>FinBlock: return error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
…n for a finalized block
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@engine/access/ingestion/engine.go`:
- Line 412: The call to ingestion.New() should defensively validate
builder.AccessMetrics to avoid nil pointer deref at
e.accessMetrics.UpdateIngestionFinalizedBlockHeight(block.Height); wrap the
argument with notNil(builder.AccessMetrics) in the ingestion.New(...) invocation
(same place other deps like CollectionSyncer/CollectionIndexer use notNil()) so
ingestion receives a non-nil validated AccessMetrics implementation.
| return fmt.Errorf("could not request collections for block: %w", err) | ||
| } | ||
| e.collectionExecutedMetric.BlockFinalized(block) | ||
| e.accessMetrics.UpdateIngestionFinalizedBlockHeight(block.Height) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the ingestion package structure and New() function signature
fd -n "engine.go" engine/access/ingestion/ | head -5Repository: onflow/flow-go
Length of output: 288
🏁 Script executed:
# Get the constructor signature for ingestion.New()
rg -n -A 30 "^func New\(" engine/access/ingestion/ --type=go | head -60Repository: onflow/flow-go
Length of output: 5989
🏁 Script executed:
# Find all callers of ingestion.New()
rg -n "ingestion\.New\(" --type=goRepository: onflow/flow-go
Length of output: 407
🏁 Script executed:
# Also check for pattern 'New(' in the ingestion package to find constructor
rg -n "func.*New.*(" engine/access/ingestion/engine.go --type=goRepository: onflow/flow-go
Length of output: 132
🏁 Script executed:
# Read the ingestion.New() constructor to see if it validates accessMetrics
sed -n '87,200p' engine/access/ingestion/engine.goRepository: onflow/flow-go
Length of output: 4208
🏁 Script executed:
# Check the first caller in access_test.go
sed -n '770,800p' engine/access/access_test.goRepository: onflow/flow-go
Length of output: 800
🏁 Script executed:
# Check the second test caller
sed -n '1035,1065p' engine/access/access_test.goRepository: onflow/flow-go
Length of output: 682
🏁 Script executed:
# Check the third test caller
sed -n '1308,1338p' engine/access/access_test.goRepository: onflow/flow-go
Length of output: 755
🏁 Script executed:
# Check the cmd/consensus/main.go caller
sed -n '550,575p' cmd/consensus/main.goRepository: onflow/flow-go
Length of output: 760
🏁 Script executed:
# Check the cmd/access/node_builder caller
sed -n '2260,2290p' cmd/access/node_builder/access_node_builder.goRepository: onflow/flow-go
Length of output: 928
🏁 Script executed:
# Check what notNil() does in access_node_builder to understand defensive pattern
rg -B 5 -A 5 "func notNil" cmd/access/node_builder/access_node_builder.go | head -20Repository: onflow/flow-go
Length of output: 526
🏁 Script executed:
# Look at module.AccessMetrics definition to see if it's interface or concrete
rg -n "type AccessMetrics" --type=goRepository: onflow/flow-go
Length of output: 171
🏁 Script executed:
# Examine the interface definition
rg -B 2 -A 10 "type AccessMetrics interface" --type=goRepository: onflow/flow-go
Length of output: 712
🏁 Script executed:
# Verify if builder.AccessMetrics could be nil by checking how it's initialized
rg -B 10 "builder.AccessMetrics =" cmd/access/node_builder/access_node_builder.go | head -30Repository: onflow/flow-go
Length of output: 413
🏁 Script executed:
# Check if AccessMetrics is initialized in all code paths
rg -n "AccessMetrics" cmd/access/node_builder/access_node_builder.go | grep -E "(AccessMetrics\s*=|AccessMetrics:)" | head -20Repository: onflow/flow-go
Length of output: 222
🏁 Script executed:
# Check the exact context around the ingestion.New() call to see if AccessMetrics is wrapped with notNil()
sed -n '2265,2290p' cmd/access/node_builder/access_node_builder.goRepository: onflow/flow-go
Length of output: 689
🏁 Script executed:
# Also check line 2132 context to understand which component that's for
sed -n '2120,2145p' cmd/access/node_builder/access_node_builder.goRepository: onflow/flow-go
Length of output: 1432
Add notNil() wrapper for builder.AccessMetrics in ingestion.New() call to ensure defensive validation.
At line 2280 in cmd/access/node_builder/access_node_builder.go, builder.AccessMetrics is passed to ingestion.New() without the notNil() validation, unlike other critical dependencies in the same call (CollectionSyncer, CollectionIndexer, collectionExecutedMetric, TxResultErrorMessagesCore). Since accessMetrics is an interface type that can be nil, and the code at line 412 in engine/access/ingestion/engine.go calls e.accessMetrics.UpdateIngestionFinalizedBlockHeight(block.Height) without a nil check, this creates a potential nil pointer dereference. Wrap builder.AccessMetrics with notNil() to align with the project's defensive pattern used consistently for other critical dependencies.
🤖 Prompt for AI Agents
In `@engine/access/ingestion/engine.go` at line 412, The call to ingestion.New()
should defensively validate builder.AccessMetrics to avoid nil pointer deref at
e.accessMetrics.UpdateIngestionFinalizedBlockHeight(block.Height); wrap the
argument with notNil(builder.AccessMetrics) in the ingestion.New(...) invocation
(same place other deps like CollectionSyncer/CollectionIndexer use notNil()) so
ingestion receives a non-nil validated AccessMetrics implementation.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| cluster := new(protocolmock.Cluster) | ||
| epoch := new(protocolmock.CommittedEpoch) | ||
| epochs := new(protocolmock.EpochQuery) | ||
| snap := new(protocolmock.Snapshot) |
There was a problem hiding this comment.
Is it intentional that these use new(module.mockObject) rather than module.NewMockObject(s.T())? This way the expectations aren't checked by default, which has bitten me once before.
4 participants
Backport #8383 and #8389 to master
Summary by CodeRabbit
New Features
Improvements
Tests