fix: Fix race condition for EventNotification#12738
Conversation
WalkthroughA new GenServer module for event notification cleanup was introduced, which periodically deletes old Changes
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/indexer/lib/indexer/utils/notifications_cleaner.ex (1)
1-5: Add missing @doc for the module.The module documentation should follow the established format for better clarity.
defmodule Indexer.Utils.NotificationsCleaner do @moduledoc """ - Module is responsible for cleaning up notifications from the database. + GenServer responsible for periodically cleaning up old event notifications from the database to prevent accumulation and potential race conditions. + + This module performs two types of cleanup: + - Legacy cleanup: Removes notifications with nil inserted_at timestamps + - Age-based cleanup: Removes notifications older than the configured max_age + + The cleanup process runs at configurable intervals and is controlled by the + enabled flag in the application configuration. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/explorer/lib/explorer/chain/events/listener.ex(2 hunks)apps/explorer/lib/explorer/utility/event_notification.ex(1 hunks)apps/explorer/priv/repo/migrations/20250704124014_add_timestamp_to_event_notifications.exs(1 hunks)apps/explorer/test/support/factory.ex(2 hunks)apps/indexer/lib/indexer/supervisor.ex(2 hunks)apps/indexer/lib/indexer/utils/notifications_cleaner.ex(1 hunks)apps/indexer/test/indexer/utils/notifications_cleaner_test.exs(1 hunks)config/runtime.exs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ex`: This is a module written in Elixir. For changed functions here are ge...
**/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """ This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars. The next lines in the documentation comment of any function must be wrapped after 80 chars (soft limit). It will allow to read documentation comments well in the code editors. ## Parameters - `param1`: Description of param1 - `param2`: Description of param2 ## Returns - what the function returns in one case - what the function returns if there is more than one possible outcome """
- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """ Provides an example function. ## Parameters - `integer`: An integer input. ## Returns - A string result. """ @spec example_function(integer()) :: String.t() def example_function(1), do: "One" def example_function(2), do: "Two"
- Behavior-Implementing Function:
# Handles an example call. @impl GenServer def handle_call(:example, _from, state) do {:reply, :ok, state} end
- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok
- Do not raise a comment for a missing @SPEC.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
apps/indexer/lib/indexer/supervisor.exapps/explorer/lib/explorer/utility/event_notification.exapps/explorer/test/support/factory.exapps/explorer/lib/explorer/chain/events/listener.exapps/indexer/lib/indexer/utils/notifications_cleaner.ex
🧠 Learnings (3)
config/runtime.exs (4)
Learnt from: Qwerty5Uiop
PR: blockscout/blockscout#11685
File: apps/explorer/lib/explorer/chain/token/instance.ex:929-950
Timestamp: 2025-01-20T08:06:21.116Z
Learning: The token instance retry configuration in `config/runtime.exs` has robust defaults and validation through ConfigHelper:
- max_refetch_interval: "168h" (1 week), parsed through parse_time_env_var
- exp_timeout_base: 2, parsed through parse_integer_env_var
- exp_timeout_coeff: 100, parsed through parse_integer_env_var
These values are used in exponential backoff calculations and are guaranteed to be valid through the ConfigHelper parsing.
Learnt from: Qwerty5Uiop
PR: blockscout/blockscout#11685
File: apps/explorer/lib/explorer/chain/token/instance.ex:929-950
Timestamp: 2025-01-20T08:06:21.116Z
Learning: The token instance retry configuration in `config/runtime.exs` has robust defaults:
- max_refetch_interval: "168h" (1 week)
- exp_timeout_base: 2
- exp_timeout_coeff: 100
These values are parsed through ConfigHelper functions and have proven reliable in production.
Learnt from: Qwerty5Uiop
PR: blockscout/blockscout#11685
File: apps/explorer/lib/explorer/chain/token/instance.ex:929-950
Timestamp: 2025-01-20T08:06:21.116Z
Learning: The `max_refetch_interval` configuration value for `Indexer.Fetcher.TokenInstance.Retry` has a default value in config and cannot be nil.
Learnt from: Qwerty5Uiop
PR: blockscout/blockscout#12559
File: apps/explorer/test/explorer/migrator/reindex_blocks_with_missing_transactions_test.exs:14-20
Timestamp: 2025-06-16T14:39:16.305Z
Learning: In the Blockscout codebase, test files for migrators like ReindexBlocksWithMissingTransactions should not provide default values when calling Application.get_env/2 for configuration, because the configuration is guaranteed to be set in runtime config. It's preferred for tests to fail fast if configuration is missing rather than silently using defaults, as this indicates a real configuration problem.
apps/explorer/priv/repo/migrations/20250704124014_add_timestamp_to_event_notifications.exs (1)
Learnt from: akolotov
PR: blockscout/blockscout#11485
File: apps/explorer/lib/explorer/migrator/filling_migration.ex:266-267
Timestamp: 2024-12-25T21:09:01.545Z
Learning: In `apps/explorer/lib/explorer/migrator/filling_migration.ex`, it is the user's preference to avoid making changes to the implementation of `run_task/1` (e.g., adding error handling logic) to prevent affecting existing functionality in use.
apps/indexer/test/indexer/utils/notifications_cleaner_test.exs (1)
Learnt from: Qwerty5Uiop
PR: blockscout/blockscout#12559
File: apps/explorer/test/explorer/migrator/reindex_blocks_with_missing_transactions_test.exs:14-20
Timestamp: 2025-06-16T14:39:16.305Z
Learning: In the Blockscout codebase, test files for migrators like ReindexBlocksWithMissingTransactions should not provide default values when calling Application.get_env/2 for configuration, because the configuration is guaranteed to be set in runtime config. It's preferred for tests to fail fast if configuration is missing rather than silently using defaults, as this indicates a real configuration problem.
🧬 Code Graph Analysis (3)
apps/indexer/lib/indexer/supervisor.ex (1)
apps/explorer/lib/explorer/application.ex (1)
configure(360-366)
config/runtime.exs (2)
apps/indexer/test/indexer/fetcher/on_demand/contract_code_test.exs (1)
parse_time_env_var(277-285)config/config_helper.exs (1)
parse_time_env_var(102-116)
apps/indexer/test/indexer/utils/notifications_cleaner_test.exs (1)
apps/indexer/lib/indexer/utils/notifications_cleaner.ex (1)
start_link(15-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: EthereumJSONRPC Tests (default)
- GitHub Check: Blockscout Web Tests (default)
- GitHub Check: Indexer Tests (default)
- GitHub Check: Dialyzer static analysis (default)
- GitHub Check: Explorer Tests (default)
🔇 Additional comments (16)
apps/explorer/lib/explorer/utility/event_notification.ex (1)
11-11: LGTM: Timestamps added to support cleanup functionality.The addition of
timestamps()is correctly implemented and aligns with the migration. This enables the NotificationsCleaner to identify old notifications for periodic cleanup.apps/indexer/lib/indexer/supervisor.ex (2)
67-67: LGTM: Alias added for NotificationsCleaner.The alias is correctly added to support the new cleanup functionality.
261-262: LGTM: Appropriate section comment added.The comment clearly identifies the notifications cleaner section in the supervision tree.
apps/explorer/test/support/factory.ex (2)
65-65: LGTM: EventNotification alias added appropriately.The alias is correctly added to support the new factory function.
1403-1408: LGTM: Well-implemented factory function.The factory follows standard ExMachina patterns and correctly provides test data with timestamp, supporting testing of the cleanup functionality.
apps/explorer/priv/repo/migrations/20250704124014_add_timestamp_to_event_notifications.exs (1)
1-9: LGTM: Well-structured migration for adding timestamps.The migration correctly adds nullable timestamp columns to support the new cleanup functionality. The
null: trueoption is appropriate for existing records, and the implementation follows standard Ecto patterns.config/runtime.exs (1)
1414-1417: LGTM! Configuration follows established patterns.The configuration block properly uses ConfigHelper functions and sets reasonable defaults. The environment variable naming follows the established
INDEXER_prefix pattern.apps/explorer/lib/explorer/chain/events/listener.ex (2)
43-43: LGTM! Function call updated to match renamed function.The call correctly references the renamed
fetch_event_notification/1function.
71-79: LGTM! Function properly renamed and simplified.The function correctly:
- Removes the deletion logic to prevent race conditions
- Maintains the same return behavior (returns data or nil)
- Has a clearer name that reflects its actual behavior
This change aligns with the PR objective to decouple fetching from deletion, allowing the new NotificationsCleaner to handle cleanup asynchronously.
apps/indexer/lib/indexer/utils/notifications_cleaner.ex (4)
15-23: LGTM! Proper GenServer implementation.The
start_link/1andinit/1functions follow GenServer best practices. The immediate scheduling of both cleanup messages on startup is appropriate for ensuring immediate cleanup of any existing legacy data.
25-34: LGTM! Well-structured message handling.The message handlers properly:
- Handle legacy cleanup as a one-time operation
- Schedule periodic cleanup with
Process.send_after- Return proper GenServer responses
36-53: LGTM! Cleanup functions are well-implemented.Both cleanup functions:
- Use proper Ecto query patterns with
ago/2andis_nil/1- Log the deletion counts for observability
- Handle the database operations correctly
The separation of legacy cleanup (nil timestamps) from age-based cleanup is a good design choice.
54-65: LGTM! Clean configuration access pattern.The private helper functions properly abstract configuration access and follow established patterns in the codebase.
apps/indexer/test/indexer/utils/notifications_cleaner_test.exs (3)
10-21: LGTM! Proper test configuration management.The setup correctly:
- Stores the original configuration before each test
- Restores it in the
on_exitcallback to prevent test pollution- Follows the pattern of not providing defaults, which aligns with the codebase learning that tests should fail fast if configuration is missing
22-28: LGTM! Appropriate GenServer startup test.The test properly verifies that the GenServer starts and the process is alive, then cleans up by terminating the process.
30-97: LGTM! Comprehensive cleanup behavior testing.The test suite excellently covers:
- Deletion of old notifications while preserving new ones
- Multiple old notifications deletion
- No deletions when notifications are within the age threshold
- Proper use of factory functions with timestamps
- Appropriate assertions for expected behavior
The use of
Process.sleep(500)is necessary to allow the asynchronous GenServer cleanup to complete.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/indexer/lib/indexer/utils/notifications_cleaner.ex (1)
2-4: Enhance module documentation for better clarity.The module documentation is brief. Consider expanding it to describe the cleanup process, configuration options, and how it integrates with the supervision tree.
- @moduledoc """ - Module is responsible for cleaning up notifications from the database. - """ + @moduledoc """ + GenServer that periodically cleans up old event notifications from the database. + + The cleaner runs immediately upon initialization and then schedules periodic + cleanups based on the configured interval. It deletes notifications older than + the configured maximum age using the `inserted_at` timestamp. + + ## Configuration + + The module expects the following configuration in the application environment: + - `:max_age` - Maximum age of notifications in milliseconds + - `:interval` - Cleanup interval in milliseconds + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/explorer/priv/repo/migrations/20250704124014_add_timestamp_to_event_notifications.exs(1 hunks)apps/indexer/lib/indexer/utils/notifications_cleaner.ex(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ex`: This is a module written in Elixir. For changed functions here are ge...
**/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """ This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars. The next lines in the documentation comment of any function must be wrapped after 80 chars (soft limit). It will allow to read documentation comments well in the code editors. ## Parameters - `param1`: Description of param1 - `param2`: Description of param2 ## Returns - what the function returns in one case - what the function returns if there is more than one possible outcome """
- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """ Provides an example function. ## Parameters - `integer`: An integer input. ## Returns - A string result. """ @spec example_function(integer()) :: String.t() def example_function(1), do: "One" def example_function(2), do: "Two"
- Behavior-Implementing Function:
# Handles an example call. @impl GenServer def handle_call(:example, _from, state) do {:reply, :ok, state} end
- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok
- Do not raise a comment for a missing @SPEC.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
apps/indexer/lib/indexer/utils/notifications_cleaner.ex
🧠 Learnings (1)
apps/explorer/priv/repo/migrations/20250704124014_add_timestamp_to_event_notifications.exs (1)
Learnt from: akolotov
PR: blockscout/blockscout#11485
File: apps/explorer/lib/explorer/migrator/filling_migration.ex:266-267
Timestamp: 2024-12-25T21:09:01.545Z
Learning: In `apps/explorer/lib/explorer/migrator/filling_migration.ex`, it is the user's preference to avoid making changes to the implementation of `run_task/1` (e.g., adding error handling logic) to prevent affecting existing functionality in use.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/explorer/priv/repo/migrations/20250704124014_add_timestamp_to_event_notifications.exs (2)
5-5: Consider the implications of truncating existing data.The truncation removes all existing event notifications. While this aligns with the race condition fix and the need for timestamped records, ensure that losing existing notifications is acceptable in your use case.
Consider documenting this data loss in the migration or PR description to make the impact clear to other developers.
7-9: LGTM! Clean migration structure.The migration correctly adds timestamp fields using the
timestamps()macro, which will create bothinserted_atandupdated_atcolumns with appropriate defaults and constraints.
| defp clean_up_notifications do | ||
| {count, _} = | ||
| EventNotification | ||
| |> where([en], en.inserted_at < ago(^max_age(), "millisecond")) | ||
| |> Repo.delete_all() | ||
|
|
||
| Logger.info("Deleted #{count} event notifications") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and configuration validation.
The cleanup function doesn't validate configuration values or handle database errors gracefully.
defp clean_up_notifications do
+ max_age_val = max_age()
+
+ if is_nil(max_age_val) or max_age_val <= 0 do
+ Logger.warning("Invalid max_age configuration: #{inspect(max_age_val)}")
+ return
+ end
+
{count, _} =
EventNotification
- |> where([en], en.inserted_at < ago(^max_age(), "millisecond"))
+ |> where([en], en.inserted_at < ago(^max_age_val, "millisecond"))
|> Repo.delete_all()
Logger.info("Deleted #{count} event notifications")
endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/indexer/lib/indexer/utils/notifications_cleaner.ex around lines 30 to
37, the clean_up_notifications function lacks validation for the max_age
configuration and does not handle potential database errors from
Repo.delete_all. Add checks to ensure max_age returns a valid value before using
it, and wrap the Repo.delete_all call in a try-rescue block or use with to catch
and log any errors gracefully without crashing the process.
| def handle_info(:clean_up_notifications, state) do | ||
| clean_up_notifications() | ||
| Process.send_after(self(), :clean_up_notifications, interval()) | ||
| {:noreply, state} | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the cleanup process.
The handle_info/2 callback doesn't handle potential database errors during cleanup, which could crash the GenServer.
def handle_info(:clean_up_notifications, state) do
- clean_up_notifications()
- Process.send_after(self(), :clean_up_notifications, interval())
+ try do
+ clean_up_notifications()
+ rescue
+ error ->
+ Logger.error("Failed to clean up notifications: #{inspect(error)}")
+ end
+
+ Process.send_after(self(), :clean_up_notifications, interval())
{:noreply, state}
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def handle_info(:clean_up_notifications, state) do | |
| clean_up_notifications() | |
| Process.send_after(self(), :clean_up_notifications, interval()) | |
| {:noreply, state} | |
| end | |
| def handle_info(:clean_up_notifications, state) do | |
| try do | |
| clean_up_notifications() | |
| rescue | |
| error -> | |
| Logger.error("Failed to clean up notifications: #{inspect(error)}") | |
| end | |
| Process.send_after(self(), :clean_up_notifications, interval()) | |
| {:noreply, state} | |
| end |
🤖 Prompt for AI Agents
In apps/indexer/lib/indexer/utils/notifications_cleaner.ex around lines 24 to
28, the handle_info/2 function calls clean_up_notifications without handling
potential errors, risking a GenServer crash. Wrap the call to
clean_up_notifications in a try-rescue block or use pattern matching to catch
and handle any errors or exceptions from the cleanup process, ensuring the
GenServer remains stable and logs or manages errors appropriately.
| def init(args) do | ||
| Process.send(self(), :clean_up_notifications, []) | ||
| {:ok, args} | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider potential timing issues with immediate cleanup.
The immediate cleanup in init/1 might run before the database migration is complete or before the system is fully initialized, potentially causing errors.
Consider adding a small delay or checking system readiness:
def init(args) do
- Process.send(self(), :clean_up_notifications, [])
+ Process.send_after(self(), :clean_up_notifications, 1000)
{:ok, args}
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def init(args) do | |
| Process.send(self(), :clean_up_notifications, []) | |
| {:ok, args} | |
| end | |
| def init(args) do | |
| Process.send_after(self(), :clean_up_notifications, 1000) | |
| {:ok, args} | |
| end |
🤖 Prompt for AI Agents
In apps/indexer/lib/indexer/utils/notifications_cleaner.ex around lines 19 to
22, the immediate call to clean up notifications in the init/1 function may
execute before the system or database is fully ready, causing errors. Modify the
code to introduce a small delay before sending the :clean_up_notifications
message, such as using Process.send_after/3 with a short timeout, or implement a
readiness check to ensure the system is fully initialized before triggering
cleanup.
| defp config do | ||
| Application.get_env(:indexer, __MODULE__) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add configuration validation.
The configuration helper doesn't validate that the required configuration exists, which could cause runtime errors.
defp config do
- Application.get_env(:indexer, __MODULE__)
+ case Application.get_env(:indexer, __MODULE__) do
+ nil ->
+ Logger.error("Missing configuration for #{__MODULE__}")
+ %{max_age: 86_400_000, interval: 3_600_000} # Default values
+ config -> config
+ end
endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/indexer/lib/indexer/utils/notifications_cleaner.ex around lines 47 to
49, the config function retrieves configuration without validating its presence.
Modify the function to check if the required configuration exists and raise a
clear error or handle the missing config case appropriately to prevent runtime
errors.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/indexer/lib/indexer/utils/event_notifications_cleaner.ex (1)
24-28: Add @impl annotation for GenServer callback.The handle_info/2 function should include the @impl annotation for clarity and maintainability.
+@impl GenServer def handle_info(:clean_up_event_notifications, state) doapps/indexer/test/indexer/utils/event_notifications_cleaner_test.exs (1)
30-97: Add test coverage for error scenarios.The tests only cover happy path scenarios. Consider adding tests for error conditions such as database failures or invalid configurations.
+test "handles database errors gracefully" do + # Mock Repo.delete_all to raise an error + # Test that the GenServer continues running after an error +end + +test "validates configuration on startup" do + # Test behavior with missing or invalid configuration +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/indexer/lib/indexer/supervisor.ex(2 hunks)apps/indexer/lib/indexer/utils/event_notifications_cleaner.ex(1 hunks)apps/indexer/test/indexer/utils/event_notifications_cleaner_test.exs(1 hunks)config/runtime.exs(1 hunks)docker-compose/envs/common-blockscout.env(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/indexer/lib/indexer/supervisor.ex
- config/runtime.exs
- docker-compose/envs/common-blockscout.env
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ex`: This is a module written in Elixir. For changed functions here are ge...
**/*.ex: This is a module written in Elixir. For changed functions here are general guidelines for reviewing:
- Context Awareness:
- Before raising a comment about missing specifications (@SPEC) or documentation (@doc), verify whether they exist in the corresponding code.
- Check if a single @SPEC or @doc covers multiple clauses of a function (e.g., functions with different argument patterns). Do not raise a comment if one shared @SPEC or @doc exists.
- Behavior-Implementing Functions:
- If a function is preceded by @impl, confirm that it implements a behavior. Do not raise comments for missing @SPEC or @doc if:
- @SPEC is omitted (this is acceptable).
- Documentation is omitted or provided in #-style comments.
- Predefined Functions:
- For GenServer functions like child_spec/1 and start_link/1 do not raise comments for missing @SPEC or @doc.
- Documentation Format:
- When reviewing @doc comments, ensure that it follows the following format:
@doc """ This function does X, Y, and Z and this very first sentence of the comment must not be wrapped if the line exceeds 80 chars. The next lines in the documentation comment of any function must be wrapped after 80 chars (soft limit). It will allow to read documentation comments well in the code editors. ## Parameters - `param1`: Description of param1 - `param2`: Description of param2 ## Returns - what the function returns in one case - what the function returns if there is more than one possible outcome """
- Exceptions for Private Functions:
- For private functions:
Example Scenarios to Reduce False Positives
- Single Specification and Documentation for Multiple Clauses (i.e., different implementations based on pattern matching of arguments):
@doc """ Provides an example function. ## Parameters - `integer`: An integer input. ## Returns - A string result. """ @spec example_function(integer()) :: String.t() def example_function(1), do: "One" def example_function(2), do: "Two"
- Behavior-Implementing Function:
# Handles an example call. @impl GenServer def handle_call(:example, _from, state) do {:reply, :ok, state} end
- Do not raise a comment for missing @SPEC.
- Ensure the documentation is valid as a # comment.
- Function with No Arguments:
defp no_arg_function, do: :ok
- Do not raise a comment for a missing @SPEC.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
apps/indexer/lib/indexer/utils/event_notifications_cleaner.ex
🧠 Learnings (1)
apps/indexer/test/indexer/utils/event_notifications_cleaner_test.exs (1)
Learnt from: Qwerty5Uiop
PR: blockscout/blockscout#12559
File: apps/explorer/test/explorer/migrator/reindex_blocks_with_missing_transactions_test.exs:14-20
Timestamp: 2025-06-16T14:39:16.305Z
Learning: In the Blockscout codebase, test files for migrators like ReindexBlocksWithMissingTransactions should not provide default values when calling Application.get_env/2 for configuration, because the configuration is guaranteed to be set in runtime config. It's preferred for tests to fail fast if configuration is missing rather than silently using defaults, as this indicates a real configuration problem.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: EthereumJSONRPC Tests (default)
- GitHub Check: Explorer Tests (default)
- GitHub Check: Blockscout Web Tests (default)
- GitHub Check: Indexer Tests (default)
- GitHub Check: Dialyzer static analysis (default)
🔇 Additional comments (5)
apps/indexer/lib/indexer/utils/event_notifications_cleaner.ex (3)
1-14: LGTM - Well-structured module setup.The module declaration, imports, and dependencies are properly organized. The moduledoc follows the expected format.
15-17: LGTM - Standard GenServer start_link function.The implementation follows GenServer conventions correctly.
19-22: LGTM - Proper init callback implementation.The immediate scheduling of cleanup on startup is appropriate for this use case.
apps/indexer/test/indexer/utils/event_notifications_cleaner_test.exs (2)
1-20: LGTM - Proper test setup with configuration isolation.The setup properly preserves and restores configuration between tests, which is essential for isolated test runs.
22-28: LGTM - Good GenServer startup test.The test properly verifies GenServer startup and includes cleanup of the process.
| defp max_age do | ||
| config()[:max_age] | ||
| end | ||
|
|
||
| defp interval do | ||
| config()[:interval] | ||
| end | ||
|
|
||
| defp config do | ||
| Application.get_env(:indexer, __MODULE__) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for configuration values.
The configuration helper functions should validate that required values are present and valid.
defp max_age do
- config()[:max_age]
+ case config()[:max_age] do
+ nil -> raise "EventNotificationsCleaner max_age configuration is missing"
+ value when is_integer(value) and value > 0 -> value
+ value -> raise "EventNotificationsCleaner max_age must be a positive integer, got: #{inspect(value)}"
+ end
end
defp interval do
- config()[:interval]
+ case config()[:interval] do
+ nil -> raise "EventNotificationsCleaner interval configuration is missing"
+ value when is_integer(value) and value > 0 -> value
+ value -> raise "EventNotificationsCleaner interval must be a positive integer, got: #{inspect(value)}"
+ end
end
defp config do
- Application.get_env(:indexer, __MODULE__)
+ case Application.get_env(:indexer, __MODULE__) do
+ nil -> raise "EventNotificationsCleaner configuration is missing"
+ config -> config
+ end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defp max_age do | |
| config()[:max_age] | |
| end | |
| defp interval do | |
| config()[:interval] | |
| end | |
| defp config do | |
| Application.get_env(:indexer, __MODULE__) | |
| end | |
| defp max_age do | |
| case config()[:max_age] do | |
| nil -> | |
| raise "EventNotificationsCleaner max_age configuration is missing" | |
| value when is_integer(value) and value > 0 -> | |
| value | |
| value -> | |
| raise "EventNotificationsCleaner max_age must be a positive integer, got: #{inspect(value)}" | |
| end | |
| end | |
| defp interval do | |
| case config()[:interval] do | |
| nil -> | |
| raise "EventNotificationsCleaner interval configuration is missing" | |
| value when is_integer(value) and value > 0 -> | |
| value | |
| value -> | |
| raise "EventNotificationsCleaner interval must be a positive integer, got: #{inspect(value)}" | |
| end | |
| end | |
| defp config do | |
| case Application.get_env(:indexer, __MODULE__) do | |
| nil -> | |
| raise "EventNotificationsCleaner configuration is missing" | |
| config -> | |
| config | |
| end | |
| end |
🤖 Prompt for AI Agents
In apps/indexer/lib/indexer/utils/event_notifications_cleaner.ex around lines 39
to 49, the config helper functions do not validate the presence or validity of
required configuration values. Add validation logic in the config function or in
max_age and interval functions to check that the retrieved values are not nil
and meet expected criteria, raising an appropriate error or fallback if
validation fails.
| defp clean_up_event_notifications do | ||
| {count, _} = | ||
| EventNotification | ||
| |> where([en], en.inserted_at < ago(^max_age(), "millisecond")) | ||
| |> Repo.delete_all() | ||
|
|
||
| Logger.info("Deleted #{count} event notifications") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
The cleanup function should handle potential database errors gracefully to prevent GenServer crashes.
defp clean_up_event_notifications do
- {count, _} =
- EventNotification
- |> where([en], en.inserted_at < ago(^max_age(), "millisecond"))
- |> Repo.delete_all()
+ try do
+ {count, _} =
+ EventNotification
+ |> where([en], en.inserted_at < ago(^max_age(), "millisecond"))
+ |> Repo.delete_all()
+
+ Logger.info("Deleted #{count} event notifications")
+ rescue
+ error ->
+ Logger.error("Failed to clean up event notifications: #{inspect(error)}")
+ end
- Logger.info("Deleted #{count} event notifications")
end🤖 Prompt for AI Agents
In apps/indexer/lib/indexer/utils/event_notifications_cleaner.ex around lines 30
to 37, the clean_up_event_notifications function lacks error handling for the
database delete operation, which can cause GenServer crashes if an error occurs.
Modify the function to wrap the Repo.delete_all call in a try-rescue block or
use pattern matching on the result to handle errors gracefully, logging any
failures without crashing the process.
| test "deletes multiple old notifications" do | ||
| old_time = DateTime.utc_now() |> DateTime.add(-2000, :millisecond) | ||
|
|
||
| # Insert multiple old notifications | ||
| insert_list(3, :event_notification, inserted_at: old_time) | ||
|
|
||
| # Insert one new notification | ||
| insert(:event_notification, data: "new_data") | ||
|
|
||
| # Verify all notifications exist | ||
| assert Repo.aggregate(EventNotification, :count) == 4 | ||
|
|
||
| # Set configuration | ||
| config = Application.get_env(:indexer, EventNotificationsCleaner) | ||
| Application.put_env(:indexer, EventNotificationsCleaner, Keyword.put(config, :max_age, 1000)) | ||
|
|
||
| assert {:ok, _pid} = EventNotificationsCleaner.start_link([]) | ||
| Process.sleep(500) | ||
|
|
||
| # Verify only the new notification remains | ||
| assert Repo.aggregate(EventNotification, :count) == 1 | ||
| remaining = Repo.one(from(n in EventNotification, select: n.data)) | ||
| assert remaining == "new_data" | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same timing issue applies here.
This test has the same Process.sleep(500) timing dependency that should be addressed.
Apply the same synchronization improvement as suggested for the previous test.
🤖 Prompt for AI Agents
In apps/indexer/test/indexer/utils/event_notifications_cleaner_test.exs around
lines 55 to 78, the test relies on Process.sleep(500) to wait for the
EventNotificationsCleaner process to complete, which is a timing-dependent and
unreliable approach. Replace the Process.sleep call with a synchronization
mechanism such as monitoring the cleaner process or using a message/assertion
that confirms the cleanup has finished before proceeding with assertions. This
ensures the test only continues once the cleanup is complete, making it more
stable and deterministic.
| test "does not delete notifications when none are old enough" do | ||
| # Insert only new notifications | ||
| insert_list(3, :event_notification) | ||
|
|
||
| # Verify notifications exist | ||
| assert Repo.aggregate(EventNotification, :count) == 3 | ||
|
|
||
| # Set configuration | ||
| config = Application.get_env(:indexer, EventNotificationsCleaner) | ||
| Application.put_env(:indexer, EventNotificationsCleaner, Keyword.put(config, :max_age, 1000)) | ||
|
|
||
| assert {:ok, _pid} = EventNotificationsCleaner.start_link([]) | ||
| Process.sleep(500) | ||
|
|
||
| # Verify all notifications still exist | ||
| assert Repo.aggregate(EventNotification, :count) == 3 | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same timing issue applies here.
This test also uses Process.sleep(500) which should be replaced with more reliable synchronization.
Apply the same synchronization improvement as suggested for the previous tests.
🤖 Prompt for AI Agents
In apps/indexer/test/indexer/utils/event_notifications_cleaner_test.exs around
lines 80 to 96, replace the unreliable Process.sleep(500) call with a more
reliable synchronization method, such as waiting for a specific message or event
that confirms the EventNotificationsCleaner process has completed its work. This
ensures the test only proceeds after the cleanup attempt finishes, avoiding
flaky timing issues.
| test "deletes notifications older than max_age" do | ||
| # Create notifications with different timestamps | ||
| old_time = DateTime.utc_now() |> DateTime.add(-2000, :millisecond) | ||
| new_time = DateTime.utc_now() |> DateTime.add(-500, :millisecond) | ||
|
|
||
| insert(:event_notification, data: "old_data", inserted_at: old_time) | ||
| insert(:event_notification, data: "new_data", inserted_at: new_time) | ||
|
|
||
| # Verify both notifications exist | ||
| assert Repo.aggregate(EventNotification, :count) == 2 | ||
|
|
||
| # Set configuration for max_age of 1000ms | ||
| config = Application.get_env(:indexer, EventNotificationsCleaner) | ||
| Application.put_env(:indexer, EventNotificationsCleaner, Keyword.put(config, :max_age, 1000)) | ||
|
|
||
| assert {:ok, _pid} = EventNotificationsCleaner.start_link([]) | ||
| Process.sleep(500) | ||
|
|
||
| # Verify only the old notification was deleted | ||
| assert Repo.aggregate(EventNotification, :count) == 1 | ||
| remaining = Repo.one(from(n in EventNotification, select: n.data)) | ||
| assert remaining == "new_data" | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Process.sleep with more reliable synchronization.
Using Process.sleep(500) makes tests fragile and can lead to flaky test results. Consider using a more deterministic approach.
assert {:ok, _pid} = EventNotificationsCleaner.start_link([])
-Process.sleep(500)
+
+# Wait for cleanup to complete by polling the database
+:timer.sleep(100)
+Process.send(pid, :clean_up_event_notifications, [])
+:timer.sleep(100)Alternatively, consider exposing the cleanup function for direct testing:
+# In the cleaner module, add a public function for testing
+def clean_up_event_notifications_sync do
+ clean_up_event_notifications()
+end
+# In the test
+EventNotificationsCleaner.clean_up_event_notifications_sync()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/indexer/test/indexer/utils/event_notifications_cleaner_test.exs around
lines 31 to 53, replace the use of Process.sleep(500) with a more reliable
synchronization method to avoid flaky tests. Instead of sleeping, either wait
for a message or use a callback that confirms the cleanup process has completed.
Alternatively, refactor the EventNotificationsCleaner module to expose the
cleanup function directly and call it in the test to deterministically verify
the deletion of old notifications.
3 participants
Closes #12724
Motivation
Race condition in case multiple API pods setup:
(
Explorer.Chain.Events.Listenermodule)Changelog
fetch_and_delete_event_notification->fetch_event_notification(Explorer.Chain.Events.Listener)Indexer.Utils.NotificationsCleanerwhich is now responsible for clean up notificationsINDEXER_NOTIFICATIONS_CLEANUP_INTERVALINDEXER_NOTIFICATIONS_CLEANUP_ENABLEDINDEXER_NOTIFICATIONS_CLEANUP_MAX_AGEChecklist for your Pull Request (PR)
masterin the Version column. If I removed variable, I added it to Deprecated ENV Variables page. After merging docs PR, changes will be reflected in these pages.Summary by CodeRabbit
New Features
Bug Fixes
Chores