feat: add time-based gRPC stream optimization#1000
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new gRPC configuration Suggested reviewers
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-config/src/tests.rs (1)
479-553: 🧹 Nitpick | 🔵 TrivialAdd a boundary test for
MBV_GRPC__MAX_TIME_WITHOUT_OPTIMIZATION_SECS=0.Great coverage for defaults and overrides; please also lock in behavior for
0(reject or clamp) to prevent regressions around interval construction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-config/src/tests.rs` around lines 479 - 553, Add a new unit test (e.g., test_env_vars_grpc_zero_timeout) that sets MBV_GRPC__MAX_TIME_WITHOUT_OPTIMIZATION_SECS to "0" using EnvVarGuard (same pattern as test_env_vars_full_coverage), calls run_cli(vec![]) and then asserts the resulting config.grpc.max_time_without_optimization_secs equals 0 to lock in the zero-boundary behavior for the interval parsing in the gRPC config; place the new test near test_env_vars_full_coverage and reference the MBV_GRPC__MAX_TIME_WITHOUT_OPTIMIZATION_SECS env var and the config.grpc.max_time_without_optimization_secs field in the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs`:
- Around line 231-233: The code constructs optimization_interval_duration from
grpc_config.max_time_without_optimization_secs and later uses it to create a
tokio::time::interval which panics on a zero Duration; update the construction
of optimization_interval_duration (and/or validate
grpc_config.max_time_without_optimization_secs earlier) to enforce a minimum of
1 second (e.g., clamp the value to at least 1 when computing
optimization_interval_duration used by Self and by the tokio::time::interval
call), or return an error if the config is invalid; reference the symbols
optimization_interval_duration, grpc_config.max_time_without_optimization_secs,
and the tokio::time::interval usage in actor.rs when applying the fix.
---
Outside diff comments:
In `@magicblock-config/src/tests.rs`:
- Around line 479-553: Add a new unit test (e.g.,
test_env_vars_grpc_zero_timeout) that sets
MBV_GRPC__MAX_TIME_WITHOUT_OPTIMIZATION_SECS to "0" using EnvVarGuard (same
pattern as test_env_vars_full_coverage), calls run_cli(vec![]) and then asserts
the resulting config.grpc.max_time_without_optimization_secs equals 0 to lock in
the zero-boundary behavior for the interval parsing in the gRPC config; place
the new test near test_env_vars_full_coverage and reference the
MBV_GRPC__MAX_TIME_WITHOUT_OPTIMIZATION_SECS env var and the
config.grpc.max_time_without_optimization_secs field in the assertion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
config.example.tomlmagicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rsmagicblock-chainlink/src/remote_account_provider/chain_laser_actor/stream_manager.rsmagicblock-config/src/config/grpc.rsmagicblock-config/src/tests.rs
magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs (1)
231-233:⚠️ Potential issue | 🔴 CriticalPrevent runtime panic when the optimization interval is configured as
0.Line 232 accepts
max_time_without_optimization_secsas-is, and Line 263 passes it totokio::time::interval. If config/env sets it to0, actor startup will panic.Proposed fix
- let optimization_interval_duration = - Duration::from_secs(grpc_config.max_time_without_optimization_secs); + let configured_secs = grpc_config.max_time_without_optimization_secs; + let optimization_interval_duration = + Duration::from_secs(configured_secs.max(1)); + if configured_secs == 0 { + warn!( + client_id = %client_id, + "MBV_GRPC__MAX_TIME_WITHOUT_OPTIMIZATION_SECS=0 is invalid; clamping to 1s" + ); + }Use this read-only script to verify whether a zero guard exists at actor/config boundaries:
#!/bin/bash set -euo pipefail echo "=== Interval construction and source value ===" rg -n "max_time_without_optimization_secs|optimization_interval_duration|tokio::time::interval|Duration::from_secs" \ magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs \ magicblock-config/src/config/grpc.rs \ magicblock-config/src/tests.rs echo echo "=== Zero-value guards/validation checks ===" rg -n "max_time_without_optimization_secs.*==\\s*0|==\\s*0.*max_time_without_optimization_secs|max\\(1\\)|NonZeroU64|assert!\\(.*max_time_without_optimization_secs" \ magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs \ magicblock-config/src/config/grpc.rs \ magicblock-config/src/tests.rs || trueExpected result: if no guard/validation hits appear, the panic path is still possible.
Also applies to: 262-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs` around lines 231 - 233, The actor currently constructs optimization_interval_duration directly from grpc_config.max_time_without_optimization_secs and later passes it into tokio::time::interval, which will panic if the value is 0; update the code that creates optimization_interval_duration (the Duration::from_secs call that uses max_time_without_optimization_secs) to guard or clamp zero values (e.g. treat 0 as 1) before constructing the Duration or validate and return an error, ensuring the same check is applied at the config boundary or immediately inside the actor initialization so tokio::time::interval never receives a zero duration; refer to the symbols grpc_config.max_time_without_optimization_secs, optimization_interval_duration and tokio::time::interval when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs`:
- Around line 231-233: The actor currently constructs
optimization_interval_duration directly from
grpc_config.max_time_without_optimization_secs and later passes it into
tokio::time::interval, which will panic if the value is 0; update the code that
creates optimization_interval_duration (the Duration::from_secs call that uses
max_time_without_optimization_secs) to guard or clamp zero values (e.g. treat 0
as 1) before constructing the Duration or validate and return an error, ensuring
the same check is applied at the config boundary or immediately inside the actor
initialization so tokio::time::interval never receives a zero duration; refer to
the symbols grpc_config.max_time_without_optimization_secs,
optimization_interval_duration and tokio::time::interval when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs
Amp-Thread-ID: https://ampcode.com/threads/T-019c9880-7326-71ac-98e5-f126d67a807d Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs`:
- Around line 263-267: The tokio interval created in actor.rs with let mut
optimization_interval =
tokio::time::interval(self.optimization_interval_duration) uses the default
Burst behavior which can emit many missed ticks after a stall; change it to
explicitly set the missed-tick behavior (for example call
optimization_interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay)
right after creating the interval) so optimize() is not invoked in a rapid burst
after delays, and keep the existing optimization_interval.tick().await
consumption of the immediate first tick.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs
magicblock-chainlink/src/remote_account_provider/chain_laser_actor/actor.rs
Show resolved
Hide resolved
Amp-Thread-ID: https://ampcode.com/threads/T-019c98bd-1db8-715b-b183-ee8de3322c45 Co-authored-by: Amp <amp@ampcode.com>
1 participant
Summary
Add time-based optimization interval to guarantee per-second optimizations in the gRPC chain laser actor, preventing optimization starvation when subscription count thresholds aren't reached.
Details
The chain laser actor previously relied solely on subscription count thresholds to trigger stream optimizations. This could lead to unoptimized streams persisting indefinitely if subscriptions didn't exceed the limits. The new time-based interval ensures streams are optimized at least once per configured interval (defaults to 60 seconds), providing a safety mechanism for consistent performance.
Configuration
Added
max_time_without_optimization_secsconfiguration parameter to the gRPC config with a default value of 60 seconds. This can be overridden via theMBV_GRPC__MAX_TIME_WITHOUT_OPTIMIZATION_SECSenvironment variable.Chain Laser Actor
Integrated a tokio interval timer into the actor's main event loop that periodically triggers optimization if unoptimized streams exist. The interval is reset whenever an optimization occurs (either event-based or time-based), preventing redundant optimizations. This uses a simple flag-based approach to communicate optimization completion between the stream manager and actor.
Stream Manager
Added helper methods to support the time-based optimization mechanism:
has_unoptimized_streams()- checks if old unoptimized handles existtake_optimized_flag()- retrieves and resets the optimization flagSummary by CodeRabbit
New Features
Configuration
Tests
Documentation