Repository navigation
fix: use isolated runtimes for critical components - #682
Conversation
Separate async runtimes with manual thread allocation, which allows to isolate different components of the system in such a way that no blocking operations can interfere with the whole system. Fixes shutdown sequence of RPC server by avoiding infinite cancel branch in the shutdown loop.
📝 WalkthroughWalkthroughThis pull request converts multiple async methods to synchronous implementations while introducing explicit thread-based runtime management. Key changes include: moving the RPC server from Tokio task spawning to a dedicated std::thread with its own runtime in the MagicValidator; converting asynchronous ledger truncation methods to synchronous equivalents executed on a dedicated worker thread; replacing the Tokio main macro with manual runtime construction in the validator; updating the task scheduler service to take ownership instead of borrowing; and adding connection-level termination guards. The refactoring also adds logging capabilities and increases the EventProcessor instance count from 1 to 2. Possibly related PRs
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 |
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
magicblock-ledger/src/ledger_truncator.rs (2)
126-135: Potential integer underflow whennum_slots_to_truncateis 0.If
excess < slot_size, integer division yieldsnum_slots_to_truncate = 0. Line 129 then computeslowest_slot + 0 - 1, which underflows whenlowest_slotis 0, resulting inu64::MAX.Consider adding an early return when there are no slots to truncate:
let num_slots_to_truncate = excess / slot_size; + if num_slots_to_truncate == 0 { + info!("Excess size smaller than single slot, skipping truncation"); + return Ok(()); + } // Calculating up to which slot we're truncating let truncate_to_slot = lowest_slot + num_slots_to_truncate - 1;
235-276: Stale comment and minor documentation issues.
Lines 247-250: The comment mentions offloading to a "separate thread," but compaction now runs inline within
compact_slot_range. Since the entire worker already runs on a dedicated thread (spawned at line 322), this comment is misleading.Line 236: Typo - "inclusing ramge" → "inclusive range".
Lines 269-272: These columns use
(None, None)for full-table compaction, which contradicts the docstring claiming slot-range compaction. Consider clarifying that some columns require full compaction.- /// Synchronous utility function that triggers and awaits compaction on all the columns - /// Compacts [from_slot; to_slot] inclusing ramge + /// Synchronous utility function that triggers compaction on all columns. + /// Compacts [from_slot; to_slot] inclusive range for slot-keyed columns; + /// performs full compaction for transaction-keyed columns. pub fn compact_slot_range( ledger: &Arc<Ledger>, from_slot: u64, to_slot: u64, ) { if to_slot < from_slot { warn!("LedgerTruncator: Nani2?"); return; } - // Compaction can be run concurrently for different cf - // but it utilizes rocksdb threads, in order not to drain - // our tokio rt threads, we split offload the effort to a - // separate thread + // Compaction runs sequentially on this worker's dedicated thread + // to avoid blocking the main async runtime. let mut measure = Measure::start("Manual compaction");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
magicblock-aperture/src/lib.rs(3 hunks)magicblock-aperture/src/server/http/mod.rs(2 hunks)magicblock-api/src/magic_validator.rs(5 hunks)magicblock-ledger/src/ledger_truncator.rs(9 hunks)magicblock-ledger/tests/test_ledger_truncator.rs(4 hunks)magicblock-task-scheduler/src/service.rs(2 hunks)magicblock-validator/Cargo.toml(1 hunks)magicblock-validator/src/main.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 614
File: magicblock-task-scheduler/src/db.rs:26-0
Timestamp: 2025-11-12T09:46:27.553Z
Learning: In magicblock-task-scheduler, task parameter validation (including ensuring iterations > 0 and enforcing minimum execution intervals) is performed in the Magic program (on-chain) before ScheduleTaskRequest instances reach the scheduler service. The From<&ScheduleTaskRequest> conversion in db.rs does not need additional validation because inputs are already validated at the program level.
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
📚 Learning: 2025-11-04T10:48:00.070Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/mod.rs:217-219
Timestamp: 2025-11-04T10:48:00.070Z
Learning: In magicblock-validator, the codebase uses a pattern where types containing non-Send/non-Sync fields (like Rc<RefCell<...>>) are marked with unsafe impl Send when they are guaranteed to be confined to a single thread through careful API design and thread spawning patterns.
Applied to files:
magicblock-api/src/magic_validator.rs
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-api/src/magic_validator.rs
📚 Learning: 2025-11-24T14:21:00.948Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: Cargo.toml:58-58
Timestamp: 2025-11-24T14:21:00.948Z
Learning: In the magicblock-validator codebase, magicblock-api/Cargo.toml intentionally uses borsh = "1.5.3" (instead of the workspace version 0.10.4) because it needs to deserialize types from the magic-domain-program external dependency, which requires borsh 1.5.x compatibility. This is an intentional exception for interoperability with the magic domain program.
Applied to files:
magicblock-api/src/magic_validator.rsmagicblock-validator/src/main.rsmagicblock-validator/Cargo.toml
📚 Learning: 2025-11-19T12:55:48.931Z
Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-tools/src/validator.rs:193-197
Timestamp: 2025-11-19T12:55:48.931Z
Learning: In the magicblock-validator codebase, when constructing arguments for the light CLI test validator in start_light_validator_with_config (test-integration/test-tools/src/validator.rs), the shlex::split approach is necessary because the light CLI does not handle whitespaces properly in validator arguments. The string concatenation + shlex parsing pattern should not be refactored to direct argument construction in this specific case.
Applied to files:
magicblock-api/src/magic_validator.rs
📚 Learning: 2025-11-21T10:22:07.520Z
Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.
Applied to files:
magicblock-api/src/magic_validator.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-ledger/tests/test_ledger_truncator.rs
📚 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-ledger/src/ledger_truncator.rs
🧬 Code graph analysis (3)
magicblock-api/src/magic_validator.rs (1)
magicblock-processor/src/executor/mod.rs (1)
spawn(140-151)
magicblock-ledger/tests/test_ledger_truncator.rs (1)
magicblock-ledger/src/ledger_truncator.rs (1)
join(350-361)
magicblock-task-scheduler/src/service.rs (3)
magicblock-processor/src/executor/mod.rs (1)
run(165-204)magicblock-aperture/src/lib.rs (1)
run(50-57)magicblock-ledger/src/ledger_truncator.rs (1)
run(49-83)
🔇 Additional comments (16)
magicblock-aperture/src/server/http/mod.rs (1)
107-122: LGTM! Correctly prevents repeated graceful_shutdown calls.The termination guard elegantly solves the infinite cancel branch issue mentioned in the PR objectives. Once
cancel.cancelled()completes, it would remain ready in every subsequent iteration, potentially callinggraceful_shutdown()multiple times. Theterminatingflag ensures it's invoked exactly once per connection.magicblock-validator/Cargo.toml (1)
19-19: LGTM!The
num_cpusdependency is appropriate for determining worker thread counts for the isolated runtimes introduced in this PR.magicblock-task-scheduler/src/service.rs (3)
60-65: Safety comment is accurate for the ownership-based spawning pattern.The
unsafe impl Sendandunsafe impl Syncare justified given thatTaskSchedulerServiceis moved into a single Tokio task instart()and maintains exclusive ownership throughout its lifetime. Based on learnings, this pattern is consistent with the codebase's approach for types confined to a single thread.
102-123: LGTM!The ownership transfer pattern (
mut selfinstead of&mut self) cleanly moves the service into the spawned Tokio task, eliminating borrowing issues across await points. The simplifiedtokio::spawn(self.run())is idiomatic.
251-284: LGTM!The
run(mut self)signature is consistent with the ownership transfer instart(). The select loop correctly handles cancellation, task execution, and scheduled task processing.magicblock-api/src/magic_validator.rs (2)
136-136: LGTM!Changing
rpc_handlefromtokio::task::JoinHandle<()>tothread::JoinHandle<()>correctly reflects the new architecture where the RPC server runs on a dedicated OS thread with its own Tokio runtime, isolating it from the main async runtime.
760-763: No issues found — the shutdown parameter change is correct.The
shutdown(true)call properly invokes the ledger's graceful shutdown method, which flushes pending data and waits for background work cancellation. This is the safer, more correct approach for shutdown sequences where data integrity matters. The parameter aligns with the surrounding flush operations and the comment indicating data should be written to disk before exit.magicblock-ledger/tests/test_ledger_truncator.rs (2)
69-69: LGTM! Synchronous join correctly implemented.The removal of
.awaitcorrectly reflects the API change to a synchronousjoin()method. Calling the blockingthread::join()at test cleanup time (after all async work completes) is acceptable in this test context.Based on learnings, blocking operations in tests are acceptable in this codebase.
Also applies to: 96-96, 156-156
69-69: Alljoin()call sites have been correctly updated to remove.await.Verification confirms that the
LedgerTruncator::join()method is synchronous (returnsthread::Result<()>at line 350), and all four test call sites in this file (lines 69, 96, 156, 195) are properly updated without.await.magicblock-validator/src/main.rs (2)
69-81: LGTM! Runtime construction looks correct.The manual runtime construction with dedicated worker threads for the async runtime is appropriate for isolating critical components. Using
num_cpus::get() / 4with.max(1)ensures at least one worker thread even on systems with fewer than 4 CPUs.
83-143: LGTM! Clean separation of runtime setup and application logic.Moving the startup logic to
run()keeps the syncmain()focused on runtime construction while maintaining the existing async flow.magicblock-ledger/src/ledger_truncator.rs (5)
1-11: LGTM! Imports updated appropriately for the thread-based model.
77-79: LGTM! Correctly updated to call synchronoustruncate.
190-233: LGTM! Clean synchronous conversion.The sequential slot range deletion followed by flush and compaction is appropriate for the isolated thread model.
322-328: LGTM! Proper thread isolation with dedicated runtime.Spawning the worker on a dedicated
std::threadwith its owncurrent_threadruntime achieves the isolation goal, ensuring blocking operations in the truncator cannot affect other system components.
350-361: LGTM! Correctly adapted for std::thread lifecycle.The
joinmethod properly handles the thread-based worker and returnsthread::Result<()>.
| let builder = conn::auto::Builder::new(TokioExecutor::new()); | ||
| let connection = builder.serve_connection(io, handler); | ||
| tokio::pin!(connection); | ||
| let mut terminating = false; |
There was a problem hiding this comment.
Nit: I wonder if there is a better way than using this flag here. Maybe putting the connection inside an Option and then doing something like if let Some(..)?
There was a problem hiding this comment.
it will be ugly to pinning requirements of the connection future, plain flag is the most straightforward solution
* master: fix: use isolated runtimes for critical components (#682)
3 participants
Separate async runtimes with manual thread allocation, which allows to isolate different components of the system in such a way that no blocking operations can interfere with the whole system.
Fixes shutdown sequence of RPC server by avoiding
infinite cancel branch in the shutdown loop.
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.