feat: replace task context with thread-local storage #614
Conversation
This stash can be used for communication between magic program and the rest of the validator, as a more performant and easier to use alternative to Context accounts.
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
|
Warning Rate limit exceeded@Dodecahedr0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughRemoves on-chain TaskContext and its constants; replaces ticker-based scheduler with a channel-driven TaskSchedulerService. Adds TaskRequest types and a thread-local ExecutionTlsStash for registering tasks during execution. Wires scheduled-tasks channels across core, executor, scheduler, API, programs, tests, and testkit; removes millis_per_tick config and related code. Changes
Sequence Diagram(s)sequenceDiagram
participant EX as Transaction Executor
participant TLS as ExecutionTlsStash (TLS)
participant CH as tasks_tx Channel
participant TS as TaskSchedulerService
participant DB as Task DB
EX->>TLS: register_task(TaskRequest::Schedule/Cancel)
note right of TLS `#dbeafe`: per-thread queue (TLS)
EX->>EX: finish transaction execution
EX->>TLS: next_task() (drain)
TLS-->>EX: TaskRequest
EX->>CH: send(TaskRequest) (best-effort)
CH->>TS: receive TaskRequest
TS->>TS: process_request(...)
alt Schedule
TS->>DB: insert DbTask / schedule
else Cancel
TS->>DB: remove/cancel task
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
42-49: Update the out-of-date error text.
This log still references the old task-context account even though the instruction no longer requires it, which will mislead operators debugging failures.Apply this diff to keep the message accurate:
- "ScheduleTask ERR: not enough accounts to schedule task ({}), need payer, signing program and task context", + "ScheduleTask ERR: not enough accounts to schedule task ({}), need payer and signing program accounts",magicblock-task-scheduler/src/service.rs (1)
234-237: Cancellation currently ignored for freshly scheduled tasksNewly scheduled tasks never record their
DelayQueuekey, soremove_task_from_queuecannot pull them out if a cancel request arrives before the first execution—cancellations silently fail and the task still runs. Please stash the key when inserting (and clear any existing entry) so we can honor cancel requests.self.db.insert_task(&db_task)?; - self.task_queue - .insert(db_task.clone(), Duration::from_millis(0)); + self.remove_task_from_queue(task.id); + let key = self + .task_queue + .insert(db_task.clone(), Duration::from_millis(0)); + self.task_queue_keys.insert(task.id, key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockprograms/elfs/guinea.sois excluded by!**/*.sotest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
magicblock-api/src/fund_account.rs(1 hunks)magicblock-api/src/magic_validator.rs(8 hunks)magicblock-chainlink/src/chainlink/blacklisted_accounts.rs(0 hunks)magicblock-committor-program/Cargo.toml(1 hunks)magicblock-config/src/lib.rs(4 hunks)magicblock-config/src/task_scheduler.rs(1 hunks)magicblock-config/tests/fixtures/11_everything-defined.toml(0 hunks)magicblock-config/tests/parse_config.rs(1 hunks)magicblock-config/tests/read_config.rs(1 hunks)magicblock-core/src/link.rs(6 hunks)magicblock-core/src/link/transactions.rs(3 hunks)magicblock-magic-program-api/src/args.rs(1 hunks)magicblock-magic-program-api/src/instruction.rs(0 hunks)magicblock-magic-program-api/src/lib.rs(1 hunks)magicblock-magic-program-api/src/tls.rs(1 hunks)magicblock-processor/Cargo.toml(1 hunks)magicblock-processor/src/executor/mod.rs(3 hunks)magicblock-processor/src/executor/processing.rs(4 hunks)magicblock-processor/src/scheduler/state.rs(2 hunks)magicblock-task-scheduler/Cargo.toml(1 hunks)magicblock-task-scheduler/src/service.rs(6 hunks)magicblock-task-scheduler/tests/service.rs(1 hunks)programs/guinea/Cargo.toml(1 hunks)programs/guinea/src/lib.rs(5 hunks)programs/magicblock/src/lib.rs(0 hunks)programs/magicblock/src/magicblock_processor.rs(1 hunks)programs/magicblock/src/schedule_task/mod.rs(1 hunks)programs/magicblock/src/schedule_task/process_cancel_task.rs(5 hunks)programs/magicblock/src/schedule_task/process_process_tasks.rs(0 hunks)programs/magicblock/src/schedule_task/process_schedule_task.rs(6 hunks)programs/magicblock/src/schedule_task/utils.rs(0 hunks)programs/magicblock/src/task_context.rs(0 hunks)programs/magicblock/src/utils/instruction_utils.rs(2 hunks)test-integration/configs/schedule-task.ephem.toml(0 hunks)test-integration/programs/flexi-counter/src/instruction.rs(0 hunks)test-integration/programs/flexi-counter/src/processor.rs(2 hunks)test-integration/programs/schedulecommit/Cargo.toml(1 hunks)test-integration/test-ledger-restore/src/lib.rs(1 hunks)test-integration/test-task-scheduler/src/lib.rs(1 hunks)test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs(1 hunks)test-integration/test-task-scheduler/tests/test_reschedule_task.rs(1 hunks)test-integration/test-task-scheduler/tests/test_schedule_error.rs(1 hunks)test-integration/test-task-scheduler/tests/test_schedule_task.rs(1 hunks)test-integration/test-task-scheduler/tests/test_schedule_task_signed.rs(1 hunks)test-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rs(1 hunks)test-kit/src/lib.rs(3 hunks)
💤 Files with no reviewable changes (9)
- programs/magicblock/src/lib.rs
- programs/magicblock/src/schedule_task/utils.rs
- programs/magicblock/src/schedule_task/process_process_tasks.rs
- magicblock-magic-program-api/src/instruction.rs
- magicblock-config/tests/fixtures/11_everything-defined.toml
- test-integration/programs/flexi-counter/src/instruction.rs
- programs/magicblock/src/task_context.rs
- magicblock-chainlink/src/chainlink/blacklisted_accounts.rs
- test-integration/configs/schedule-task.ephem.toml
🧰 Additional context used
🧠 Learnings (5)
📚 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-magic-program-api/src/lib.rsprograms/guinea/src/lib.rsmagicblock-api/src/fund_account.rsprograms/magicblock/src/schedule_task/process_schedule_task.rstest-integration/programs/flexi-counter/src/processor.rsprograms/magicblock/src/schedule_task/process_cancel_task.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-core/src/link/transactions.rsmagicblock-processor/src/executor/mod.rsmagicblock-core/src/link.rsmagicblock-processor/src/scheduler/state.rsprograms/magicblock/src/schedule_task/process_schedule_task.rsmagicblock-task-scheduler/src/service.rstest-kit/src/lib.rsmagicblock-api/src/magic_validator.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-task-scheduler/tests/test_unauthorized_reschedule.rsmagicblock-processor/src/executor/processing.rsmagicblock-task-scheduler/tests/service.rstest-integration/test-ledger-restore/src/lib.rsprograms/magicblock/src/schedule_task/process_schedule_task.rstest-integration/programs/flexi-counter/src/processor.rstest-integration/test-task-scheduler/tests/test_schedule_task_signed.rstest-integration/test-task-scheduler/tests/test_schedule_error.rstest-kit/src/lib.rstest-integration/test-task-scheduler/tests/test_reschedule_task.rstest-integration/test-task-scheduler/tests/test_schedule_task.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rsprograms/magicblock/src/schedule_task/process_cancel_task.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-api/src/fund_account.rsmagicblock-processor/src/scheduler/state.rs
📚 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-task-scheduler/src/service.rs
🧬 Code graph analysis (7)
magicblock-processor/src/executor/processing.rs (1)
magicblock-magic-program-api/src/tls.rs (2)
next_task(22-24)clear(26-31)
programs/magicblock/src/magicblock_processor.rs (4)
programs/magicblock/src/utils/instruction_utils.rs (1)
schedule_task(200-209)programs/magicblock/src/schedule_task/process_cancel_task.rs (1)
process_cancel_task(13-52)test-integration/programs/flexi-counter/src/processor.rs (2)
process_cancel_task(458-485)process_schedule_task(401-456)programs/magicblock/src/schedule_task/process_schedule_task.rs (1)
process_schedule_task(18-123)
magicblock-task-scheduler/tests/service.rs (1)
test-kit/src/lib.rs (3)
new(83-85)default(67-69)get_account(281-291)
programs/magicblock/src/schedule_task/process_schedule_task.rs (2)
programs/magicblock/src/utils/accounts.rs (1)
get_instruction_pubkey_with_idx(94-102)magicblock-magic-program-api/src/tls.rs (1)
register_task(17-20)
magicblock-task-scheduler/src/service.rs (4)
magicblock-core/src/link.rs (1)
link(60-89)magicblock-task-scheduler/src/db.rs (2)
new(50-89)path(46-48)magicblock-api/src/magic_validator.rs (1)
start(635-711)magicblock-magic-program-api/src/args.rs (1)
id(142-147)
programs/magicblock/src/schedule_task/process_cancel_task.rs (2)
programs/magicblock/src/utils/accounts.rs (1)
get_instruction_pubkey_with_idx(94-102)magicblock-magic-program-api/src/tls.rs (1)
register_task(17-20)
magicblock-api/src/magic_validator.rs (4)
magicblock-core/src/link.rs (1)
link(60-89)magicblock-task-scheduler/src/db.rs (1)
path(46-48)magicblock-task-scheduler/src/service.rs (1)
new(58-91)magicblock-processor/src/executor/mod.rs (1)
new(68-110)
⏰ 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: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
🔇 Additional comments (43)
programs/guinea/Cargo.toml (1)
19-19: Dependency is properly used and necessary.The verification confirms that
magicblock-magic-program-apiis actively imported and used in the guinea crate (programs/guinea/src/lib.rs:4with usages at lines 109 and 130). The dependency addition is justified and follows the existing workspace dependency pattern used throughout the Cargo.toml file.magicblock-task-scheduler/Cargo.toml (1)
32-38: All dev-dependencies are valid and properly used in tests.Verification confirms:
- All 6 crates are defined in the workspace root
Cargo.toml- Test file
magicblock-task-scheduler/tests/service.rsuses guinea, test-kit, magicblock-magic-program-api, and solana-account directly; solana-pubkey and solana-signature are transitively integrated through test-kit- Two integration tests exercise task scheduling functionality (schedule and cancel operations)
magicblock-committor-program/Cargo.toml (1)
32-33: Clarify the purpose and necessity of these feature flags for the PR objective.The addition of
custom-heapandcustom-panicfeatures across multiple crates appears unrelated to the stated PR objective of replacing ticker-based task scheduling with thread-local storage. These features should either be:
- Documented with a clear explanation of their necessity, or
- Moved to a separate PR if they're orthogonal to this refactoring.
Provide clarification on:
- Why are
custom-heapandcustom-panicfeatures required for the TLS task scheduling refactoring?- Are these features documented in an RFC or design document?
- Are they gated by conditional compilation logic that ties to the scheduler changes?
If these features are preparatory/future work, consider extracting them to a separate infrastructure improvement PR to keep this PR focused on the task scheduling objective.
magicblock-processor/Cargo.toml (1)
21-21: Remove unused dependency or add integration code.The workspace dependency is properly configured; however,
magicblock-magic-program-apiis not imported or used anywhere in the magicblock-processor codebase. Either remove this dependency frommagicblock-processor/Cargo.tomlline 21, or add the code that integrates it (if this PR is incomplete).⛔ Skipped due to learnings
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.programs/magicblock/src/utils/instruction_utils.rs (2)
240-251: Verification confirmed: No issues found.The
CancelTaskinstruction change has been properly implemented end-to-end:
- The processor (
process_cancel_task.rs) expects only one account (the authority at index 0), with no references toTASK_CONTEXT_PUBKEY.- The instruction builder (
cancel_task_instruction) creates a single-account instruction matching the processor's expectation.- All callsites—including internal tests and external programs (guinea, flexi-counter)—pass only the authority account.
- Tests verify the correct behavior: authority must be a signer, unsigned attempts fail appropriately.
The breaking change has been consistently applied throughout the codebase.
211-226: No verification required—changes are properly coordinated.The removal of
TASK_CONTEXT_PUBKEYis correctly implemented across the codebase:
process_schedule_task()expects accounts:[payer (idx 0), ...instruction_accounts (idx 1+)]✓process_cancel_task()expects accounts:[authority (idx 0)]✓- All instruction builders match these expectations ✓
- All callsites are compatible ✓
Minor note: The error message in
process_schedule_taskstill mentions "task context," which is outdated documentation but doesn't affect functionality. Consider updating to reflect the current channel-based architecture.programs/guinea/src/lib.rs (4)
4-6: LGTM!The imports are properly organized and all are necessary for the cross-program invocation functionality.
Also applies to: 12-12, 14-14
27-31: LGTM!The new instruction variants are well-defined and appropriately typed for the task scheduling functionality.
67-75: LGTM!The increment function follows the existing patterns in the file and includes proper error handling.
160-168: LGTM!The instruction handling in the match statement is properly implemented and follows the existing patterns.
test-integration/test-task-scheduler/tests/test_schedule_error.rs (2)
3-3: LGTM!The import change from TASK_CONTEXT_PUBKEY to MAGIC_PROGRAM_ID aligns with the PR's refactoring objective to remove TaskContext-based scheduling.
43-51: LGTM!The instruction constructor calls are properly updated to use MAGIC_PROGRAM_ID, maintaining consistency with the API refactoring.
Also applies to: 128-132
test-kit/src/lib.rs (3)
40-41: LGTM!The NOOP_PROGRAM_ID constant is properly defined with compile-time validation.
127-127: LGTM!The
tasks_txchannel integration is essential for the new event-driven task scheduling architecture.
138-143: NOOP program binary verified — code is ready.The NOOP program binary exists at the specified path. The code follows the established test program initialization pattern.
test-integration/test-task-scheduler/tests/test_reschedule_task.rs (2)
3-3: LGTM!The import change is consistent with the broader refactoring to remove TaskContext-based scheduling.
42-50: LGTM!All instruction constructor calls are consistently updated to use MAGIC_PROGRAM_ID.
Also applies to: 77-85, 168-172
test-integration/test-task-scheduler/src/lib.rs (1)
46-46: LGTM!The TaskSchedulerConfig simplification is consistent with the shift to event-driven task scheduling.
test-integration/test-ledger-restore/src/lib.rs (1)
155-155: LGTM!The formatting change improves code conciseness without affecting functionality.
programs/magicblock/src/magicblock_processor.rs (2)
8-8: LGTM!The removal of
process_process_tasksimport is consistent with the elimination of the ProcessTasks instruction path.
68-73: ProcessTasks removal verified complete.The MagicBlockInstruction enum and match statement in magicblock_processor.rs have been properly updated. ProcessTasks has been completely removed from the codebase with no remaining references, and the ScheduleTask and CancelTask instruction variants are correctly handled. The event-driven scheduling refactoring is complete.
magicblock-config/tests/parse_config.rs (1)
285-285: Verified: TaskSchedulerConfig correctly refactored for event-driven scheduling.The struct definition confirms that
millis_per_tickhas been removed and only theresetfield remains, exactly as shown in the test at line 285. The initializationTaskSchedulerConfig { reset: true }is correct.magicblock-config/src/lib.rs (1)
271-271: LGTM! Tests updated consistently with the new TaskSchedulerConfig API.All test cases correctly reflect the removal of
millis_per_tickfrom the public TaskSchedulerConfig struct, using only theresetfield.Also applies to: 356-356, 438-438, 513-513
test-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rs (1)
3-3: LGTM! Instruction builder correctly updated to remove TaskContext dependency.The removal of
TASK_CONTEXT_PUBKEYfrom both the import and thecreate_schedule_task_ixcalls aligns with the PR's objective to replace task context with thread-local storage.Also applies to: 47-55, 82-90
magicblock-magic-program-api/src/lib.rs (1)
3-3: LGTM! TLS module addition aligns with the event-driven architecture.The new
tlsmodule introduces thread-local storage for task scheduling, replacing the previous TaskContext-based polling approach.magicblock-config/src/task_scheduler.rs (1)
7-24: LGTM! Simplified configuration aligns with the new event-driven architecture.Removing
millis_per_tickis consistent with eliminating ticker-based polling in favor of direct channel-based task notification. The derivedDefaultimplementation simplifies the code.magicblock-config/tests/read_config.rs (1)
204-204: LGTM! Test correctly updated for the new TaskSchedulerConfig API.The test configuration construction is consistent with the removal of
millis_per_tick.test-integration/programs/flexi-counter/src/processor.rs (2)
451-451: LGTM! Correctly removed task_context_info from schedule task invocation.The
invoke_signedcall now correctly omits thetask_context_infoaccount, aligning with the shift to TLS-based task scheduling.
479-482: LGTM! Correctly removed task_context_info from cancel task flow.Both the
AccountMetavector and theinvokecall have been updated to excludetask_context_info, consistent with the removal of TaskContext-based task management.programs/magicblock/src/schedule_task/mod.rs (1)
1-5: LGTM! Module structure simplified by removing ProcessTasks pathway.The removal of
process_process_tasksandutilsmodules aligns with eliminating ticker-based task polling. Only the essential schedule and cancel operations remain, consistent with the new event-driven architecture.magicblock-processor/src/scheduler/state.rs (1)
6-8: LGTM! New task channel properly initialized across all construction sites.The
tasks_txfield successfully introduces event-driven task scheduling by replacing ticker-based polling. Verification confirms the channel is properly initialized in both known construction sites (test-kit and magicblock-api), consistently sourced fromvalidator_channels.tasks_service.magicblock-processor/src/executor/processing.rs (1)
98-99: LGTM: Proper stash cleanup.The unconditional clearing of ExecutionTlsStash after transaction processing (lines 98-99) and in the simulate path (lines 145-147) correctly prevents task leakage between transactions and ensures simulations don't interfere with actual executions.
Also applies to: 145-147
test-integration/test-task-scheduler/tests/test_schedule_task.rs (1)
3-3: LGTM: Test updated to match new instruction signatures.The test correctly updates to use
MAGIC_PROGRAM_IDinstead ofTASK_CONTEXT_PUBKEY, aligning with the removal of TaskContext-based scheduling. The test logic remains functionally equivalent.Also applies to: 42-50, 133-137
magicblock-api/src/fund_account.rs (1)
6-6: LGTM: Simplified account funding aligns with TaskContext removal.The removal of
fund_task_contextand related TaskContext handling is consistent with the architectural shift to TLS-based task scheduling. The code correctly retains only the MagicContext initialization.Also applies to: 74-87
magicblock-processor/src/executor/mod.rs (1)
8-8: LGTM: Clean channel integration.The addition of the
tasks_txchannel toTransactionExecutoris properly wired through initialization and aligns with the new task scheduling architecture.Also applies to: 53-54, 105-105
magicblock-core/src/link/transactions.rs (1)
34-37: Verify unbounded channel choice for scheduled tasks.The use of
UnboundedSender/UnboundedReceivermeans tasks will accumulate in memory without backpressure if the task scheduler service falls behind or hangs. While the send-side error handling (in processing.rs line 89) logs warnings on failure, an unbounded channel can still lead to memory growth.Consider whether a bounded channel with appropriate capacity would provide better resource management, or confirm that unbounded semantics are intentional for the "fire-and-forget" task scheduling model.
programs/magicblock/src/schedule_task/process_cancel_task.rs (2)
3-6: LGTM: Clean migration to TLS-based task cancellation.The refactor from TaskContext manipulation to ExecutionTlsStash registration is clean and correct. Signer validation is properly maintained, and the cancel request is correctly queued for processing.
Also applies to: 36-43
76-79: LGTM: Test updates aligned with TaskContext removal.The tests correctly simplify to only require the payer account, removing the now-unnecessary TaskContext account handling. Test expectations remain functionally equivalent.
Also applies to: 101-104
test-integration/test-task-scheduler/tests/test_schedule_task_signed.rs (1)
2-2: LGTM: Test updated consistently with instruction signature changes.The test correctly updates to use
MAGIC_PROGRAM_IDinstead ofTASK_CONTEXT_PUBKEY, maintaining consistency with other test updates in this PR.Also applies to: 38-46
magicblock-magic-program-api/src/tls.rs (1)
1-32: LGTM: Well-designed thread-local task stash.The implementation correctly uses thread-local storage with interior mutability for per-executor-thread task queuing. Key design points:
- FIFO ordering (push_back/pop_front) is appropriate for task queue semantics
- No size limits on the queues is acceptable because the caller (processing.rs) clears unconditionally after each transaction (line 99) and in simulate paths (line 147), preventing unbounded accumulation
- Thread-local isolation ensures tasks don't leak between executor threads
magicblock-core/src/link.rs (1)
65-86: Task channel wiring LGTM.
Channel creation and hand-off mirror the existing pattern for other endpoints, and wrapping the dispatch receiver inOptionlets downstream consumerstake()ownership safely.magicblock-magic-program-api/src/args.rs (1)
119-148: TaskRequest shape is clean.
The enum plusid()helper keep schedule/cancel aligned and make downstream code straightforward to match on identifiers.magicblock-api/src/magic_validator.rs (1)
332-343: Task scheduler initialization looks solid.
Passing ownership of the dispatchtasks_servicereceiver intoTaskSchedulerService::newhere completes the new TLS-driven flow without leaving dangling consumers.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-processor/src/executor/processing.rs (1)
54-60: Critical: ExecutionTlsStash not cleared on early return.When a transaction fails to load, the function returns early without clearing the ExecutionTlsStash. Any tasks added during the failed execution will leak into the next transaction, potentially causing incorrect task scheduling.
Apply this diff to clear the stash before the early return:
if let Err(err) = result { let status = Err(err); self.commit_failed_transaction(txn, status.clone()); FAILED_TRANSACTIONS_COUNT.inc(); + ExecutionTlsStash::clear(); tx.map(|tx| tx.send(status)); return; }
♻️ Duplicate comments (2)
magicblock-task-scheduler/tests/service.rs (1)
74-87: Remove redundant sleep before polling loop.The fixed 10ms sleep at line 75 is unnecessary because the polling loop (lines 78-84) immediately follows and will efficiently wait for the account mutation. The polling loop already handles the timing correctly with its 20ms interval and 1-second timeout.
Apply this diff to remove the redundant sleep:
- // Wait the task scheduler to receive the task - tokio::time::sleep(Duration::from_millis(10)).await; - // Wait until the task scheduler actually mutates the account (with an upper bound to avoid hangs) tokio::time::timeout(Duration::from_secs(1), async {magicblock-processor/src/executor/processing.rs (1)
89-91: Already flagged: Typo in warning message + optional improvement.The typo "and longer running" → "is no longer running" was already flagged in a previous review.
Additionally, the error is not captured in the closure, so the actual error details aren't logged. Consider capturing and logging it for better diagnostics.
Apply this diff to fix both issues:
- let _ = self.tasks_tx.send(task).inspect_err(|_| - warn!("Scheduled tasks service has hung up and is longer running") + let _ = self.tasks_tx.send(task).inspect_err(|e| + warn!("Scheduled tasks service has hung up and is no longer running: {e}") );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
magicblock-processor/src/executor/processing.rs(4 hunks)magicblock-task-scheduler/tests/service.rs(1 hunks)programs/guinea/src/lib.rs(5 hunks)programs/magicblock/src/validator.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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-processor/src/executor/processing.rsmagicblock-task-scheduler/tests/service.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-processor/src/executor/processing.rsprograms/guinea/src/lib.rsmagicblock-task-scheduler/tests/service.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:
programs/guinea/src/lib.rs
📚 Learning: 2025-11-07T14:20:31.425Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.425Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
programs/guinea/src/lib.rsmagicblock-task-scheduler/tests/service.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-task-scheduler/tests/service.rs
🧬 Code graph analysis (2)
magicblock-processor/src/executor/processing.rs (1)
magicblock-magic-program-api/src/tls.rs (2)
next_task(22-24)clear(26-31)
magicblock-task-scheduler/tests/service.rs (2)
programs/magicblock/src/validator.rs (1)
init_validator_authority_if_needed(54-62)test-kit/src/lib.rs (1)
new(83-85)
⏰ 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: run_make_ci_lint
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (15)
magicblock-task-scheduler/tests/service.rs (9)
1-18: LGTM!The imports are well-organized and all appear necessary for the integration tests.
19-45: LGTM! Test setup is thorough.The test properly initializes the execution environment, validator authority, and task scheduler service. The use of
expecton the ledger parent path and tasks service is acceptable in test code.
47-72: LGTM! Task scheduling logic is correct.The test correctly constructs a ScheduleTask instruction with a single Increment instruction and properly asserts the transaction execution succeeds.
77-87: LGTM! Polling loop properly addresses past flakiness concerns.The bounded retry with timeout and polling correctly replaces the previous fixed sleep, making the test resilient to scheduler timing variations. The 20ms poll interval and 1-second timeout are reasonable values.
89-92: LGTM! Cleanup is correct.Canceling the token and awaiting the handle ensures proper shutdown of the task scheduler service.
Based on learnings
151-166: LGTM! Polling loop properly addresses past flakiness concerns.The bounded retry correctly waits until at least 5 executions are observed before proceeding with cancellation. The 20ms poll interval and 2-second timeout are appropriate for this test.
168-183: LGTM! Task cancellation logic is correct.The test properly constructs and executes the CancelTask instruction.
196-209: LGTM! Cancellation verification is thorough.The test correctly waits for multiple intervals after cancellation and verifies that the account value remains unchanged, confirming that the scheduler stopped executing the task.
211-214: LGTM! Cleanup is correct.Canceling the token and awaiting the handle ensures proper shutdown of the task scheduler service.
Based on learnings
magicblock-processor/src/executor/processing.rs (2)
98-99: LGTM: Unconditional stash clearing.The stash is correctly cleared after transaction processing, ensuring no tasks leak between transactions (once the early-return issue at line 60 is fixed).
145-147: LGTM: Simulation stash clearing.Correctly clears the stash after simulation without draining tasks, preventing simulations from scheduling actual tasks or interfering with real execution.
programs/guinea/src/lib.rs (4)
4-14: LGTM! Imports are appropriate for CPI.All new imports (
ScheduleTaskArgs,MagicBlockInstruction,AccountMeta,Instruction,invoke) are properly used in the new task scheduling functions.
23-32: LGTM! New instruction variants are well-defined.The
Increment,ScheduleTask, andCancelTaskvariants are properly typed and consistent with the existing enum style.
104-114: Good! Past review feedback has been addressed.The previous review flagged that
_magic_program_infowas extracted but never validated. The current implementation correctly:
- Removes the underscore prefix (line 104)
- Validates the program ID matches
magicblock_magic_program_api::ID(lines 108-110)- Validates the payer is a signer (lines 112-114)
176-184: LGTM! Instruction routing is correct.The new instruction variants are properly routed to their respective handler functions with appropriate error propagation.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-processor/src/executor/processing.rs (1)
54-60: Add stash clearing before early return for consistency.The early return at line 59 doesn't clear
ExecutionTlsStash, which contradicts the comment at line 99 stating "no matter what happened to the transaction we clear the stash."While the stash should be empty when load fails (no execution occurs), defensive programming suggests clearing TLS state on all exit paths to prevent potential stale data leakage between transactions on the same thread.
Apply this diff:
if let Err(err) = result { let status = Err(err); self.commit_failed_transaction(txn, status.clone()); FAILED_TRANSACTIONS_COUNT.inc(); + ExecutionTlsStash::clear(); tx.map(|tx| tx.send(status)); return; }
♻️ Duplicate comments (1)
magicblock-processor/src/executor/processing.rs (1)
84-94: Fix remaining typo in warning message.The warning message at line 91 still contains a grammatical error. While the previous review's fix changed "and longer" to "is longer," it should be "is no longer running."
Apply this diff:
let _ = self.tasks_tx.send(task).inspect_err(|_| - warn!("Scheduled tasks service has hung up and is longer running") + warn!("Scheduled tasks service has hung up and is no longer running") );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
programs/elfs/guinea.sois excluded by!**/*.so
📒 Files selected for processing (3)
magicblock-processor/src/executor/processing.rs(4 hunks)magicblock-task-scheduler/tests/service.rs(1 hunks)programs/guinea/src/lib.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 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-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.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-task-scheduler/tests/service.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
magicblock-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.rsprograms/guinea/src/lib.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.rsprograms/guinea/src/lib.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-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.rsprograms/guinea/src/lib.rs
📚 Learning: 2025-11-04T13:22:38.811Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-aperture/src/requests/http/get_fee_for_message.rs:25-31
Timestamp: 2025-11-04T13:22:38.811Z
Learning: In magicblock-aperture, request size validation (including limits on message size) is enforced at the request ingestion layer, before handlers like get_fee_for_message are invoked. Therefore, handlers do not need additional size guards on decoded data.
Applied to files:
magicblock-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.rsprograms/guinea/src/lib.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
magicblock-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.rsprograms/guinea/src/lib.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.rsprograms/guinea/src/lib.rs
📚 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-task-scheduler/tests/service.rsmagicblock-processor/src/executor/processing.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-processor/src/executor/processing.rsprograms/guinea/src/lib.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
programs/guinea/src/lib.rs
🧬 Code graph analysis (2)
magicblock-task-scheduler/tests/service.rs (2)
programs/magicblock/src/validator.rs (2)
init_validator_authority_if_needed(54-62)validator_authority_id(35-42)test-kit/src/lib.rs (1)
new(83-85)
magicblock-processor/src/executor/processing.rs (1)
magicblock-magic-program-api/src/tls.rs (2)
next_task(22-24)clear(26-31)
⏰ 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: run_make_ci_lint
- GitHub Check: Build Project
- GitHub Check: run_make_ci_test
🔇 Additional comments (14)
programs/guinea/src/lib.rs (6)
4-17: LGTM! Imports are complete and appropriate.All necessary imports for CPI functionality are present:
ScheduleTaskArgsandMagicBlockInstructionfrom the magic program API, plusAccountMeta,Instruction, andinvokefrom Solana program crate.
27-32: LGTM! Enum variants follow existing patterns.The new instruction variants (
Increment,ScheduleTask,CancelTask) are well-defined with appropriate parameter types.
67-77: LGTM! Overflow protection correctly implemented.The
incrementfunction now properly useschecked_addto prevent arithmetic overflow, returningProgramError::ArithmeticOverflowwhen the byte value would exceed 255. This addresses the previous review concern about wrapping behavior.
102-130: LGTM! CPI setup and validation are correct.The
schedule_taskfunction properly:
- Validates the magic program account ID (lines 110-112)
- Verifies the payer is a signer (lines 114-116)
- Constructs the instruction with correct account metadata
- Invokes the CPI with the appropriate account slice
The implementation follows Solana CPI best practices. The magic program account validation provides an additional security check even though it's not required in the
invokecall itself.
132-156: LGTM! CPI implementation mirrors schedule_task correctly.The
cancel_taskfunction follows the same validation pattern asschedule_task:
- Magic program ID validation (lines 139-141)
- Signer verification (lines 143-145)
- Correct instruction construction and invocation
The implementation is consistent and correct.
178-186: LGTM! Instruction routing is properly implemented.The new match arms correctly route the instruction variants to their respective handler functions with proper error propagation.
magicblock-processor/src/executor/processing.rs (3)
1-1: LGTM!The new imports for
warnlogging andExecutionTlsStashare appropriate for the thread-local task scheduling integration.Also applies to: 10-10
99-100: LGTM!Unconditional clearing of the TLS stash ensures no task leakage between transactions in the normal execution flow.
146-148: LGTM!Clearing the stash after simulation correctly prevents any tasks scheduled during simulation from being processed, since simulations don't persist state changes.
magicblock-task-scheduler/tests/service.rs (5)
22-55: Well-structured setup function.The setup function properly extracts common initialization logic and returns all necessary components for the tests. The database path is correctly derived from the ledger path, ensuring test isolation.
Note: The validator authority pattern (lines 29-32) creates shared state across tests since
init_validator_authority_if_neededonly initializes once. While this is likely intentional for the test environment, be aware that the authority is tied to the first test's payer that executes.
91-101: Excellent use of bounded timeout with polling.The test correctly waits for the scheduler to execute the task using a polling loop with an upper-bound timeout. This approach prevents flakiness that would occur with fixed sleep durations and provides clear failure messages if the task never completes.
145-160: Robust cancellation test setup.The test correctly waits until at least 5 executions are observed before canceling, ensuring the test validates actual cancellation behavior rather than racing with initial task startup. The timeout provides clear failure indication if the scheduler stalls.
185-190: Clear and correct assertion logic.The assertion properly handles the race condition where additional executions might occur between detecting 5 executions and the cancel taking effect. The descriptive message with actual values will help diagnose failures.
192-205: Appropriate post-cancellation verification.Using a fixed sleep of
2 * intervalafter cancellation is reasonable here—it gives the scheduler enough time to potentially execute one more iteration (which shouldn't happen), validating that cancellation took effect. The final assertion with a clear message ensures no further executions occurred.
bmuddha
left a comment
bmuddha
left a comment
There was a problem hiding this comment.
A few minor nits, LGTM otherwise
taco-paco
left a comment
taco-paco
left a comment
There was a problem hiding this comment.
Some comments & questions
| // Commit transaction to the ledger and schedule tasks | ||
| // TODO: send intents here as well once implemented | ||
| if !is_replay { | ||
| self.commit_transaction(txn, processed, balances); |
There was a problem hiding this comment.
@bmuddha @thlorenz
we have code from above where we commit if tx.is_some()
if result.is_err() && tx.is_some() {
// But we always commit transaction to the ledger (mostly for user convenience)
if !is_replay {
self.commit_transaction(txn, processed, balances);
}
return result;
}and then code here that commits even if tx.is_none(). Is it correct way of doing it?
There was a problem hiding this comment.
What I mean is that in this code it doesn't matter if tx.is_some() or not. It will be committed anyway. assuming is_replay = false
| /// Interval at which the task scheduler will check for requests in the context | ||
| tick_interval: Duration, | ||
| /// Queue of tasks to execute | ||
| task_queue: DelayQueue<DbTask>, |
There was a problem hiding this comment.
Imo we shall work on native tasks TaskRequest rather than some DB related task. Right now we have a structure where we convert TaskRequest and native tasks into some DbTask, rather than other way around. We need to get tasks from DB on restart/retries, convert to normal tasks and execute those. Otherwise our DB models leak into this service, while they only DB related and refer to how we store them in DB
There was a problem hiding this comment.
Request don't contain execution specific information (executions_left, last_execution_millis). Creating requests from actual tasks would remove those, resulting in different behaviors (e.g. a task that runs once a day will run again if we restart the validator immediatly after the first execution). The idea was to reduce the amount of data passed in the schedule instruction
| intents: VecDeque<()>, | ||
| } | ||
|
|
||
| thread_local! { |
There was a problem hiding this comment.
magicblock-magic-program-api just exposes some core types for SDKs and such, while this here is validator implementation details. It should be moved somewhere else.
SDK and dlp shalln't be aware of such details
There was a problem hiding this comment.
Well, I'm open to recommendations, creating a separate crate just for this seems a bit of an overkill.
There was a problem hiding this comment.
It already was moved here
or it could be in magic-program as TransactionScheduler currently is
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-processor/src/executor/processing.rs (1)
109-147: Consider clearing TLS stash at the beginning of simulate.Currently, the stash is cleared only at the end (line 145). If a previous transaction left tasks in the stash due to a bug or race condition, this simulation will see those stray tasks. For defensive programming, clear the stash at the beginning to ensure a clean state.
Apply this diff:
pub(super) fn simulate( &self, transaction: [SanitizedTransaction; 1], tx: TxnSimulationResultTx, ) { + // Clear stash at start to ensure simulations start with clean state + ExecutionTlsStash::clear(); + let (result, _) = self.process(&transaction);magicblock-task-scheduler/src/service.rs (1)
212-235: Critical: Missing task queue key storage.Line 231 inserts the task into
task_queuebut doesn't capture and store the returnedKeyintask_queue_keys. This prevents the task from being properly removed from the queue later (seeremove_task_from_queueon line 280). Compare with the correct pattern instart()at lines 111-112.Apply this diff to fix:
self.db.insert_task(&task)?; - self.task_queue - .insert(task.clone(), Duration::from_millis(0)); + let key = self.task_queue + .insert(task.clone(), Duration::from_millis(0)); + self.task_queue_keys.insert(task.id, key); debug!("Registered task {} from context", task.id);
♻️ Duplicate comments (1)
magicblock-processor/src/executor/processing.rs (1)
52-63: Critical: TLS stash not cleared on load failure.When a transaction fails to load, the function returns early at line 62 without clearing
ExecutionTlsStash. Any tasks or intents left in the stash from previous transactions will leak into the next transaction processed on this thread, causing incorrect task scheduling.Apply this diff to clear the stash before returning:
let processed = match result { Ok(processed) => processed, Err(err) => { + // Clear stash to prevent task leakage to next transaction + ExecutionTlsStash::clear(); + // Transaction failed to load, we persist it to the // ledger, only for the convenience of the user let status = Err(err); self.commit_failed_transaction(txn, status.clone()); FAILED_TRANSACTIONS_COUNT.inc(); tx.map(|tx| tx.send(status)); return; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
magicblock-processor/src/executor/processing.rs(3 hunks)magicblock-task-scheduler/src/db.rs(2 hunks)magicblock-task-scheduler/src/service.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 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-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.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-processor/src/executor/processing.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.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-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-11-04T13:22:38.811Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-aperture/src/requests/http/get_fee_for_message.rs:25-31
Timestamp: 2025-11-04T13:22:38.811Z
Learning: In magicblock-aperture, request size validation (including limits on message size) is enforced at the request ingestion layer, before handlers like get_fee_for_message are invoked. Therefore, handlers do not need additional size guards on decoded data.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.rs
📚 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-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/executor/processing.rs
📚 Learning: 2025-10-27T09:55:17.808Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:296-298
Timestamp: 2025-10-27T09:55:17.808Z
Learning: In the magicblock-validator codebase, it is acceptable to log API keys in debug/info statements because logs are private and the team considers API keys not that sensitive in their security model.
Applied to files:
magicblock-processor/src/executor/processing.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-processor/src/executor/processing.rsmagicblock-task-scheduler/src/service.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-task-scheduler/src/service.rs
🧬 Code graph analysis (2)
magicblock-processor/src/executor/processing.rs (1)
magicblock-magic-program-api/src/tls.rs (2)
next_task(22-24)clear(26-31)
magicblock-task-scheduler/src/service.rs (3)
magicblock-core/src/link.rs (1)
link(60-89)magicblock-api/src/magic_validator.rs (1)
start(635-711)magicblock-magic-program-api/src/args.rs (1)
id(142-147)
⏰ 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: run_make_ci_test
- GitHub Check: run_make_ci_lint
- GitHub Check: Build Project
🔇 Additional comments (9)
magicblock-processor/src/executor/processing.rs (2)
65-67: LGTM: Account commitment correctly handles both success and failure.The
commit_accountscall is appropriately placed after processing. The method internally checks transaction status and commits only the fee payer account for failed transactions (to charge fees), while committing all modified accounts for successful transactions.
94-97: LGTM: Ledger commitment logic is correct.The unconditional ledger commit (when not replaying) allows users to query the status of both successful and failed transactions. The
is_replayguard correctly prevents duplicate writes for replayed transactions.magicblock-task-scheduler/src/service.rs (7)
36-53: LGTM!The structural changes align well with the PR objectives—replacing bank-based polling with channel-driven task intake and adding cancellation token support.
60-93: LGTM!The constructor properly handles database reset and initialization. Deferring task loading to
start()is the correct design.
95-116: LGTM!The initialization properly loads persisted tasks and schedules them with correct timeouts. The use of
saturating_subprevents underflow for overdue tasks.
183-210: LGTM with a note on the TODO.The task execution and rescheduling logic is correct. The TODO comment (lines 186-190) notes that transaction success/failure isn't immediately validated here. Ensure this is tracked for future implementation.
244-277: LGTM!The event loop correctly handles task execution, incoming requests, and cancellation. The nested Result handling (lines 259-268) properly distinguishes between recoverable warnings and fatal errors, though this could be clearer if the
process_requestreturn type is refactored as suggested earlier.
156-181: LGTM!The cancel request processing correctly validates authority and cleans up both queue and database state.
285-316: LGTM!The transaction processing correctly uses a noop instruction with an atomic counter to ensure transaction uniqueness. This is a clean solution for making otherwise identical transactions distinguishable.
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-task-scheduler/src/service.rs (1)
212-235: Missing queue key storage prevents immediate task cancellation.On line 230-231,
insert()returns aKeythat should be stored intask_queue_keysto enable task removal before first execution. Currently, newly registered tasks cannot be cancelled until they execute at least once and are re-inserted with key tracking (line 203).Apply this fix:
- self.task_queue - .insert(task.clone(), Duration::from_millis(0)); + let key = self.task_queue.insert(task.clone(), Duration::from_millis(0)); + self.task_queue_keys.insert(task.id, key); debug!("Registered task {} from context", task.id);This ensures that
remove_task_from_queue()can properly cancel newly registered tasks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
magicblock-task-scheduler/src/service.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
magicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-task-scheduler/src/service.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-task-scheduler/src/service.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
magicblock-task-scheduler/src/service.rs
📚 Learning: 2025-11-04T13:22:38.811Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-aperture/src/requests/http/get_fee_for_message.rs:25-31
Timestamp: 2025-11-04T13:22:38.811Z
Learning: In magicblock-aperture, request size validation (including limits on message size) is enforced at the request ingestion layer, before handlers like get_fee_for_message are invoked. Therefore, handlers do not need additional size guards on decoded data.
Applied to files:
magicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-task-scheduler/src/service.rs
📚 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-task-scheduler/src/service.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-task-scheduler/src/service.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-task-scheduler/src/service.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-task-scheduler/src/service.rs
🧬 Code graph analysis (1)
magicblock-task-scheduler/src/service.rs (5)
magicblock-core/src/link.rs (1)
link(60-89)magicblock-task-scheduler/src/db.rs (2)
new(64-103)path(60-62)magicblock-processor/src/executor/mod.rs (3)
new(68-110)spawn(134-145)run(159-198)magicblock-api/src/magic_validator.rs (1)
start(635-711)magicblock-magic-program-api/src/args.rs (1)
id(142-147)
⏰ 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). (2)
- GitHub Check: run_make_ci_test
- GitHub Check: Build Project
🔇 Additional comments (4)
magicblock-task-scheduler/src/service.rs (4)
36-53: LGTM! Clean refactoring to channel-based architecture.The struct definition has been successfully refactored from a generic bank-based model to a channel-driven architecture. The addition of
scheduled_tasksandtokenfields properly supports the new event-driven task intake and graceful shutdown.
60-93: LGTM! Constructor cleanly separates initialization from execution.The constructor properly initializes the service with empty queues and defers task loading to the
start()method. This separation of concerns improves testability and allows for proper error handling before spawning the task.
95-116: LGTM! Proper lifecycle initialization.The
start()method correctly:
- Consumes
selfto ensure single-use semantics- Loads persisted tasks from the database
- Calculates appropriate delays using
saturating_subto prevent underflow- Returns a
JoinHandlethat allows callers to await completionThe pattern aligns well with the usage in
magicblock-api/src/magic_validator.rs.
258-269: Clarify behavior when task channel is closed.The
scheduled_tasks.recv()call returnsNonewhen all senders are dropped, but theSome(task)pattern meansNoneis silently ignored. If the channel closes (e.g., validator shutdown), the scheduler will continue processing queued tasks but stop accepting new ones.Is this drain-then-exit behavior intentional, or should the loop explicitly handle channel closure?
Consider adding explicit handling:
Some(task) = self.scheduled_tasks.recv() => { match self.process_request(&task) { // ... existing handling } } + else => { + info!("Task channel closed, draining remaining tasks"); + // Could break here or continue draining + }Alternatively, if draining is desired, add a comment explaining the behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
magicblock-task-scheduler/src/service.rs (1)
60-65: SAFETY comment still incorrectly claims ScheduledTasksRx is Sync.The comment states "All fields... are Send+Sync" but
ScheduledTasksRx(UnboundedReceiver<TaskRequest>) is!Sync. The architectural safety guarantee is correct (single-task ownership viatokio::spawninstart()+ exclusive&mut selfaccess), but the field-level claim remains inaccurate.Apply this diff to accurately reflect the safety rationale:
-// SAFETY: TaskSchedulerService is moved into a single Tokio task in `start()` and never cloned. -// It runs exclusively on that task's thread. All fields (SchedulerDatabase, TransactionSchedulerHandle, -// ScheduledTasksRx, LatestBlock, DelayQueue, HashMap, AtomicU64, CancellationToken) are Send+Sync, -// and the service maintains exclusive ownership throughout its lifetime. +// SAFETY: TaskSchedulerService is moved (not cloned) into a single Tokio task in `start()` and +// only ever accessed via &mut self, ensuring the UnboundedReceiver (scheduled_tasks) single-consumer +// guarantee is maintained. The architectural confinement to a single task makes Sync safe despite +// scheduled_tasks being !Sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-processor/src/executor/processing.rs(3 hunks)magicblock-task-scheduler/src/service.rs(7 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 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.533Z
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.
📚 Learning: 2025-11-12T09:46:27.533Z
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.533Z
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.
Applied to files:
magicblock-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 Learning: 2025-11-04T10:53:50.922Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-processor/src/scheduler/locks.rs:110-122
Timestamp: 2025-11-04T10:53:50.922Z
Learning: In magicblock-processor, the TransactionScheduler runs in a single, dedicated thread and will always remain single-threaded. The `next_transaction_id()` function in scheduler/locks.rs uses `unsafe static mut` which is safe given this architectural guarantee.
Applied to files:
magicblock-task-scheduler/src/service.rs
📚 Learning: 2025-10-21T13:06:38.900Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/http/get_program_accounts.rs:17-25
Timestamp: 2025-10-21T13:06:38.900Z
Learning: The magicblock validator does not support ledger forking, so commitment-based state queries (processed/confirmed/finalized) are not applicable. RPC methods can safely ignore commitment and minContextSlot parameters from Solana RPC config objects.
Applied to files:
magicblock-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 Learning: 2025-10-28T13:15:42.706Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 596
File: magicblock-processor/src/scheduler.rs:1-1
Timestamp: 2025-10-28T13:15:42.706Z
Learning: In magicblock-processor, transaction indexes were always set to 0 even before the changes in PR #596. The proper transaction indexing within slots will be addressed during the planned ledger rewrite.
Applied to files:
magicblock-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.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-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 Learning: 2025-10-26T16:54:39.084Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: test-manual/Cargo.toml:0-0
Timestamp: 2025-10-26T16:54:39.084Z
Learning: In the magicblock-validator repository, use git branch references (not commit hashes or tags) for the helius-laserstream dependency to allow automatic updates when the branch is pushed to.
Applied to files:
magicblock-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 Learning: 2025-11-04T13:22:38.811Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 589
File: magicblock-aperture/src/requests/http/get_fee_for_message.rs:25-31
Timestamp: 2025-11-04T13:22:38.811Z
Learning: In magicblock-aperture, request size validation (including limits on message size) is enforced at the request ingestion layer, before handlers like get_fee_for_message are invoked. Therefore, handlers do not need additional size guards on decoded data.
Applied to files:
magicblock-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 Learning: 2025-10-21T11:00:18.396Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/encoder.rs:176-187
Timestamp: 2025-10-21T11:00:18.396Z
Learning: In the magicblock validator, the current slot is always the root slot. The SlotEncoder in magicblock-aperture/src/encoder.rs correctly sets `root: slot` because there is no lag between current and root slots in this architecture.
Applied to files:
magicblock-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 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-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 Learning: 2025-11-07T14:20:31.457Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 621
File: magicblock-chainlink/src/remote_account_provider/chain_pubsub_actor.rs:457-495
Timestamp: 2025-11-07T14:20:31.457Z
Learning: In magicblock-chainlink/src/remote_account_provider/chain_pubsub_client.rs, the unsubscribe closure returned by PubSubConnection::account_subscribe(...) resolves to () (unit), not a Result. Downstream code should not attempt to inspect an unsubscribe result and can optionally wrap it in a timeout to guard against hangs.
Applied to files:
magicblock-task-scheduler/src/service.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-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.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-task-scheduler/src/service.rsmagicblock-processor/src/executor/processing.rs
📚 Learning: 2025-10-21T10:34:59.140Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-accounts-db/src/lib.rs:63-72
Timestamp: 2025-10-21T10:34:59.140Z
Learning: In magicblock-validator, the AccountsDb "stop-the-world" synchronizer is managed at the processor/executor level, not at the AccountsDb API level. Transaction executors in magicblock-processor hold a read lock (sync.read()) for the duration of each slot and release it only at slot boundaries, ensuring all account writes happen under the read lock. Snapshot operations acquire a write lock, blocking until all executors release their read locks. This pattern ensures mutual exclusion between writes and snapshots without requiring read guards in AccountsDb write APIs.
Applied to files:
magicblock-processor/src/executor/processing.rs
📚 Learning: 2025-10-27T09:55:17.808Z
Learnt from: thlorenz
Repo: magicblock-labs/magicblock-validator PR: 587
File: magicblock-chainlink/src/remote_account_provider/mod.rs:296-298
Timestamp: 2025-10-27T09:55:17.808Z
Learning: In the magicblock-validator codebase, it is acceptable to log API keys in debug/info statements because logs are private and the team considers API keys not that sensitive in their security model.
Applied to files:
magicblock-processor/src/executor/processing.rs
🧬 Code graph analysis (2)
magicblock-task-scheduler/src/service.rs (4)
magicblock-core/src/link.rs (1)
link(60-89)magicblock-task-scheduler/src/db.rs (2)
new(64-103)path(60-62)magicblock-processor/src/executor/mod.rs (1)
new(68-110)magicblock-magic-program-api/src/args.rs (1)
id(142-147)
magicblock-processor/src/executor/processing.rs (1)
magicblock-magic-program-api/src/tls.rs (2)
next_task(22-24)clear(26-31)
⏰ 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 (4)
magicblock-task-scheduler/src/service.rs (4)
67-123: Well-structured lifecycle split between construction and startup.The separation of
new()(synchronous construction) andstart()(async initialization + spawn) provides clear initialization stages and prevents accidental reuse by consumingself. The persisted task recovery logic correctly calculates timeouts relative to current time.
55-58: Excellent improvement: ProcessingOutcome eliminates confusing nested Results.The
ProcessingOutcomeenum cleanly distinguishes fatal errors (outerErr) from recoverable processing failures (Ok(Recoverable(e))), addressing the previous review feedback. Error handling properly logs, persists failures viainsert_failed_scheduling, and allows the scheduler to continue processing.Also applies to: 125-161
219-242: Consider capturing task_queue key for newly registered tasks.The
impl Into<DbTask>parameter elegantly accepts bothDbTaskandScheduleTaskRequest, simplifying callers. However, Line 237-238 insert the task intotask_queuebut don't capture the returnedKeyintask_queue_keys. This preventsremove_task_from_queue()from finding the task if a cancel request arrives before the first execution completes.Compare with
start()(Lines 118-119) which does capture the key:let key = self.task_queue.insert(task, timeout); self.task_queue_keys.insert(task_id, key);Apply this diff if cancellation of newly scheduled tasks should be supported:
- self.db.insert_task(&task)?; - self.task_queue - .insert(task.clone(), Duration::from_millis(0)); + self.db.insert_task(&task)?; + let key = self.task_queue.insert(task.clone(), Duration::from_millis(0)); + self.task_queue_keys.insert(task.id, key);
251-284: The review comment contains an incorrect analysis of Tokio'sselect!behavior and does not reflect an actual issue.When a pattern does not match the result of an async computation, the remaining async expressions continue to execute concurrently, which means the scheduler's loop will not busy-loop when the channel closes. By using pattern matching, the
select!macro continues waiting on the remaining branches. In this case, the loop will continue to check the cancellation token, which ensures proper shutdown.The executor workers hold the sender and will stop sending tasks once they are shut down or when the cancellation token is signalled during shutdown (as seen in the codebase's shutdown sequence). The receiver does not need explicit None-handling in the select! pattern—the existing logic correctly handles this scenario.
Fix #523
It reduces overhead by receiving new tasks directly from the executor, rather than periodically deserializing the task context. It also introduces new task scheduler tests using
test-kitSummary by CodeRabbit
New Features
Refactoring
Tests
Chores