coderabbitai[bot]
commented
1 week ago Walkthrough
The pull request introduces enhancements to error handling across multiple files, including the addition of new error variables for missing blocks and transactions. It updates the decodeCadenceEvents function to utilize custom error types, improves the RPCSubscriber struct with recovery mechanisms, and renames functions for clarity. These changes collectively aim to strengthen the robustness of event subscription and error reporting within the application.
Changes
| File | Change Summary |
|---|---|
models/errors/errors.go |
Added ErrMissingBlock and ErrMissingTransactions for improved error handling. |
models/events.go |
Updated error handling in decodeCadenceEvents to use custom error types and improved messages. |
services/ingestion/subscriber.go |
Enhanced RPCSubscriber with recovery mechanisms, added fields for recovery state, and renamed functions for clarity. |
services/ingestion/subscriber_test.go |
Added Test_MissingBlockEvent to test behavior when a block event is missing. |
services/testutils/mock_client.go |
Introduced SetupClient to streamline mock client setup for block events. |
Possibly related PRs
- #366: Improve error handling in the API, relevant to the main PR's focus on enhancing error reporting.
- #370: Enhances error handling across the API, aligning with the new error variables added.
- #529: Modifies validation of EVM block events, directly related to the changes in error handling for missing blocks or transactions.
- #533: Addresses inconsistencies in data representation related to block receipts, which ties into the main PR's focus on error handling.
Suggested labels
Improvement, Bugfix
Poem
š In the code where errors play,
New flags arise to light the way.
Blocks and transactions, now we see,
With clearer paths, we hop with glee!
Recovery's here, a joyful cheer,
For every missing piece, we steer! š
Recent review details
**Configuration used: CodeRabbit UI** **Review profile: CHILL**Commits
Files that changed from the base of the PR and between 5f3f78995c68b2e0d8a6b33dddfdeb48a25f0566 and 6dd8b6d3e959e666bbe87a099d65afa3e2973579.Files selected for processing (2)
* services/ingestion/subscriber.go (6 hunks) * services/ingestion/subscriber_test.go (2 hunks)Additional comments not posted (10)
services/ingestion/subscriber.go (9)
`37-39`: **LGTM!** The added fields `recovery` and `recoveredEvents` in the `RPCSubscriber` struct are appropriate for managing recovery states and storing potentially missing events during the recovery process. --- `114-114`: **Looks good!** Renaming `events` to `eventsChan` improves code clarity by explicitly indicating it's a channel. --- `119-120`: **Error handling looks good!** The updated error handling logic to send the error on the `eventsChan` and return immediately is correct. This allows the consumer to handle the error gracefully and avoids blocking the goroutine unnecessarily. Also applies to: 125-128 --- `153-160`: **Recovery logic looks good!** The added recovery logic in the event processing code is correct: - It checks if the subscriber is in recovery mode or if the `evmEvents` contain an error. - If so, it calls the `recover` method to attempt recovery. - If still in recovery mode after calling `recover`, it continues to the next event, preventing the subscriber from getting stuck. This helps handle scenarios where the subscriber encounters errors while processing events and attempts to recover gracefully. --- `172-174`: **Error handling for `errChan` looks good!** The updated error handling logic for `errChan` is correct: - When an error is received on `errChan`, it sends an `ErrDisconnected` error on the `eventsChan` to notify the consumer about the disconnection. - It then returns to stop further event processing. This allows the consumer to handle the disconnection error appropriately. Also applies to: 176-177 --- Line range hint `270-307`: **Method renaming and implementation look good!** The renaming of `fetchBlockEvents` to `fetchMissingData` accurately reflects the method's purpose of fetching missing data when the event streaming API returns an inconsistent response. The implementation is correct: - It removes existing events from `blockEvents` to ensure only the recovered events are returned. - It fetches the missing events using `GetEventsForHeightRange` as a backup mechanism. - It appends the recovered events to `blockEvents.Events` and returns the updated `blockEvents`. This provides a reliable way to handle inconsistent responses and recover missing data. --- `309-324`: **New method implementation looks good!** The new method `accumulateEventsMissingBlock` is implemented correctly: - It accumulates transaction events until it can produce a valid EVM block event containing a block and transactions, helping recover from missing block scenarios. - It appends the received events to `r.recoveredEvents` and updates `events.Events` with the accumulated events to ensure all the accumulated events are included. - It sets `r.recovery` to `true` if the recovered block events still have an error, otherwise it resets `r.recovery` and clears `r.recoveredEvents`, which is appropriate. This method provides a mechanism to handle missing block scenarios and recover a valid block event. --- `326-350`: **New method implementation looks good!** The new method `recover` is implemented correctly to handle invalid data sent over the event stream: - It logs a warning message with the error details when entering recovery mode, which helps with debugging and monitoring. - It calls `accumulateEventsMissingBlock` when the error is `ErrMissingBlock` or already in recovery mode, which is appropriate to handle missing block scenarios. - It calls `fetchMissingData` when the error is `ErrMissingTransactions`, which is correct to fetch missing transaction data. - For any other error, it returns a `BlockEventsError` with the original error, which is a reasonable fallback. This method provides a centralized recovery mechanism to handle different error scenarios and attempt to recover valid data. --- `337-340`: **Logger usage looks good!** The usage of the logger in the `recover` method is appropriate: - Logging a warning message when entering recovery mode is useful for debugging and monitoring purposes. - Including the error details in the log message provides valuable context for understanding the issue. - Logging the Flow block height helps identify the specific block where the recovery mode was triggered. This logging statement helps with troubleshooting and monitoring the recovery process.services/ingestion/subscriber_test.go (1)
`70-155`: **Excellent test for missing block scenario!** This test function is well-structured, thoroughly verifies the expected behavior of the `RPCSubscriber` when encountering a missing block in the event stream, and demonstrates effective use of goroutines and assertions. The test simulates the scenario accurately by selectively removing the block event based on the block height and validates that the subscriber identifies the missing block and includes all the missing transactions in the subsequent found block. The test enhances the overall test coverage and helps ensure the reliability and correctness of the subscriber in handling event stream inconsistencies gracefully. It serves as a valuable regression test to prevent future bugs related to missing block handling. Great job on adding this comprehensive test!
Description
Recover from a missing block in the event stream. This can happen due to issues with system transactions.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
ErrMissingBlockandErrMissingTransactions.RPCSubscriberfunctionality with recovery mechanisms for event subscription.Bug Fixes
Refactor
fetchBlockEventstofetchMissingDatafor better clarity.