feat: parallelize task cleanup #635
Conversation
feat: add metrics for task preparation & alt preparation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughThis pull request updates the committor service with performance metrics and architectural improvements. It increases the execution time threshold to 5.0 seconds, changes debug logging, and adds Clone trait bounds to the TransactionPreparator generic parameter. Background cleanup task spawning replaces synchronous cleanup in execution flows. The BaseTask trait now requires LabelValue for metric labeling. New LabelValue implementations are added for ArgsTask and BufferTask. Task preparation and ALT preparation are wrapped with performance timing instrumentation, and initialization with parallelized reallocation patterns are introduced. New Committor metrics including counters and histograms for intent tracking and preparation timing are registered. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-11-21T10:22:07.520ZApplied to files:
📚 Learning: 2025-11-07T13:09:52.253ZApplied to files:
🧬 Code graph analysis (1)magicblock-committor-service/src/intent_executor/mod.rs (1)
🔇 Additional comments (5)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs(2 hunks)magicblock-committor-service/src/intent_executor/mod.rs(5 hunks)magicblock-committor-service/src/intent_executor/single_stage_executor.rs(1 hunks)magicblock-committor-service/src/intent_executor/two_stage_executor.rs(1 hunks)magicblock-committor-service/src/tasks/args_task.rs(2 hunks)magicblock-committor-service/src/tasks/buffer_task.rs(2 hunks)magicblock-committor-service/src/tasks/mod.rs(2 hunks)magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs(3 hunks)magicblock-metrics/src/metrics/mod.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-07T13:20:13.793Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/coordinator.rs:227-238
Timestamp: 2025-11-07T13:20:13.793Z
Learning: In magicblock-processor's ExecutionCoordinator (scheduler/coordinator.rs), the `account_contention` HashMap intentionally does not call `shrink_to_fit()`. Maintaining slack capacity is beneficial for performance by avoiding frequent reallocations during high transaction throughput. As long as empty entries are removed from the map (which `clear_account_contention` does), the capacity overhead is acceptable.
Applied to files:
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
magicblock-committor-service/src/tasks/args_task.rs
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
magicblock-committor-service/src/intent_executor/mod.rs
🧬 Code graph analysis (5)
magicblock-committor-service/src/tasks/buffer_task.rs (3)
magicblock-committor-service/src/intent_executor/mod.rs (1)
value(74-82)magicblock-committor-service/src/tasks/args_task.rs (1)
value(171-178)magicblock-committor-service/src/intent_executor/error.rs (1)
value(105-112)
magicblock-metrics/src/metrics/mod.rs (3)
magicblock-committor-service/src/tasks/args_task.rs (1)
task_type(147-154)magicblock-committor-service/src/tasks/buffer_task.rs (1)
task_type(123-127)magicblock-committor-service/src/tasks/mod.rs (1)
task_type(90-90)
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (1)
magicblock-metrics/src/metrics/mod.rs (2)
observe_committor_intent_task_preparation_time(434-442)observe_committor_intent_alt_preparation_time(444-446)
magicblock-committor-service/src/tasks/args_task.rs (3)
magicblock-committor-service/src/intent_executor/mod.rs (1)
value(74-82)magicblock-committor-service/src/tasks/buffer_task.rs (1)
value(146-150)magicblock-committor-service/src/intent_executor/error.rs (1)
value(105-112)
magicblock-committor-service/src/intent_executor/mod.rs (1)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (1)
spawn(98-103)
⏰ 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). (3)
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
- GitHub Check: run_make_ci_lint
🔇 Additional comments (15)
magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs (2)
297-297: LGTM: Execution time threshold increased appropriately.The threshold increase from 2.0s to 5.0s will reduce log noise while still flagging genuinely slow executions.
312-313: LGTM: Enhanced logging with Debug representation.Using
{:?}provides more detailed execution information compared to the previous success/error string conversion.magicblock-committor-service/src/tasks/buffer_task.rs (1)
145-151: LGTM: Clean LabelValue implementation for metrics.The implementation correctly labels buffer tasks for metric collection, consistent with the pattern used in ArgsTask and ExecutionOutput.
magicblock-committor-service/src/tasks/args_task.rs (1)
170-179: LGTM: Comprehensive LabelValue implementation for all task types.The implementation provides distinct, well-named labels for each ArgsTaskType variant, enabling fine-grained metrics collection.
magicblock-committor-service/src/intent_executor/two_stage_executor.rs (1)
26-26: LGTM: Clone bound addition supports background cleanup.The Clone requirement enables the new parallel cleanup pattern introduced in the executor module.
magicblock-committor-service/src/intent_executor/single_stage_executor.rs (1)
27-27: LGTM: Clone bound addition aligns with executor pattern.Consistent with the Clone requirement in TwoStageExecutor, enabling background cleanup tasks.
magicblock-committor-service/src/tasks/mod.rs (1)
13-13: LGTM: LabelValue integration enables task-level metrics.The trait bound addition to BaseTask provides a clean abstraction for metric labeling, with all implementors (ArgsTask, BufferTask) already providing implementations.
Also applies to: 54-54
magicblock-committor-service/src/transaction_preparator/delivery_preparator.rs (3)
70-81: LGTM: Task preparation timing instrumentation added.The metrics integration correctly measures per-task preparation time using the LabelValue trait for task-type labeling.
84-93: LGTM: ALT preparation timing instrumentation added.The metrics correctly capture address lookup table preparation duration.
236-245: LGTM: Initialization parallelization improves performance.The refactored logic correctly sequences initialization first, then parallelizes reallocation operations using
try_join_all, improving buffer preparation throughput.magicblock-committor-service/src/intent_executor/mod.rs (3)
105-105: LGTM: Clone bound supports asynchronous cleanup pattern.The Clone requirement on TransactionPreparator enables the new background cleanup implementation.
740-740: LGTM: Clone bound in trait implementation.Consistent with the updated type bounds in IntentExecutorImpl.
652-675: The review comment is incorrect—the code already has synchronization for this scenario.The race condition mentioned in the code comment (lines 652-656) is intentionally handled via error handling that catches
AccountAlreadyInitializedErrorand logs it as informational. When a buffer is already initialized, the task's preparation state is switched to Cleanup, which ensures the stale buffer is removed before the next operation.This error-catch-and-cleanup pattern serves as the synchronization mechanism. The scenario is not a silent data corruption risk—it's an expected condition that the code explicitly handles.
Likely an incorrect or invalid review comment.
magicblock-metrics/src/metrics/mod.rs (2)
281-288: New committor metrics registration looks correct.All newly added committor metrics are properly wired into the
register!macro, so they’ll be exposed onceregister()is called. No issues here.
395-402: Helpers forCOMMITTOR_INTENTS_COUNTare straightforward and consistent.The increment helpers are thin, type-safe wrappers over the counter and match the style of the rest of the module. No issues.
# Conflicts: # test-integration/test-committor-service/tests/test_intent_executor.rs
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test-integration/test-committor-service/tests/test_intent_executor.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-07T13:09:52.253Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: test-kit/src/lib.rs:275-0
Timestamp: 2025-11-07T13:09:52.253Z
Learning: In test-kit, the transaction scheduler in ExecutionTestEnv is not expected to shut down during tests. Therefore, using `.unwrap()` in test helper methods like `schedule_transaction` is acceptable and will not cause issues in the test environment.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.rs
📚 Learning: 2025-10-14T09:56:14.047Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 564
File: test-integration/programs/flexi-counter/src/processor/call_handler.rs:122-125
Timestamp: 2025-10-14T09:56:14.047Z
Learning: The file test-integration/programs/flexi-counter/src/processor/call_handler.rs contains a test smart contract used for integration testing, not production code.
Applied to files:
test-integration/test-committor-service/tests/test_intent_executor.rs
# Conflicts: # magicblock-metrics/src/metrics/mod.rs
feat: parallelize task cleanup
feat: add metrics for task preparation & alt preparation
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.