feat: replace task scheduler with hydra - #1369
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe task scheduler is migrated from internal crank execution to Hydra-backed crank creation and cancellation. Scheduler configuration now contains a faucet keypair, persistence is limited to legacy-task migration, and validator startup delegates the faucet on-chain. Internal crank instruction and builtin paths are removed. Dependencies and integration tests are updated for Hydra crank lifecycle validation. Assessment against linked issues:
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 |
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 (1)
magicblock-task-scheduler/src/db.rs (1)
117-143: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t let one malformed row abort all migration.
A single bad
instructionsblob orauthoritystring makesget_tasks()returnErr, so valid tasks are not migrated and the DB is not drained. Parse rows independently and remove/quarantine only the malformed task id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/db.rs` around lines 117 - 143, In the task-loading path around the `stmt.query_map`/`rows.collect` logic in `get_tasks`, a single malformed `instructions` blob or `authority` value currently turns the whole read into `Err` and blocks draining valid tasks. Change the row handling so each record is parsed independently, and on deserialization/parse failure, skip or quarantine only that task’s `id` instead of propagating the error. Keep the successful `DbTask` construction path unchanged, but ensure bad rows are removed or marked separately so migration can continue for the rest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-task-scheduler/src/db.rs`:
- Around line 19-28: Preserve the legacy scheduling timing when migrating tasks:
DbTask and get_tasks currently drop the previous last_execution_millis/timestamp
data, so recreated Hydra cranks can drift from their original cadence. Update
DbTask to retain enough legacy timing metadata and have get_tasks use it to
compute the correct Hydra start_slot, or otherwise make the migration explicitly
reset timing with documented behavior. Use the existing DbTask and get_tasks
flow as the place to carry this state through the migration path.
In `@magicblock-task-scheduler/src/hydra.rs`:
- Around line 67-94: The Hydra create serialization path in
CreateArgs::serialize and build_create_ix currently assumes scheduled is
non-empty and blindly casts meta/data lengths with as, which can generate
invalid wire data. Update the builder/serializer to return a Result and validate
the Hydra invariants up front: reject empty scheduled slices, and use checked
conversions for metas.len() and data.len() so oversized values fail
deterministically instead of truncating.
In `@magicblock-task-scheduler/src/service.rs`:
- Around line 143-158: Keep valid tasks in SQLite until Hydra creation succeeds;
the migration path in the task scheduler currently removes tasks even when
`wait_for_block_ready()` times out or `schedule_crank()` fails. Update the
migration logic around `schedule_crank`, `wait_for_block_ready`, and
`db.remove_task` so each task is only deleted after a successful Hydra creation,
and leave the SQLite row intact on any error so it can be retried later.
- Around line 201-213: Reject non-positive iteration counts before calling
schedule_crank so we never create a Hydra crank with remaining = 0; update the
task scheduling path in service.rs to validate task.iterations up front and
return early for zero or negative values, mirroring the migration behavior that
skips exhausted tasks. Apply the same guard in the other task creation/update
paths that feed schedule_crank, using the existing schedule_crank and
task.iterations symbols to locate the affected flows.
In `@programs/magicblock/src/schedule_task/mod.rs`:
- Around line 22-28: The signer rejection in the schedule_task logic is now
broader than the current log message implies, so update the stale error text in
the `if account.is_signer` branch to match the new rule. Keep the change
localized to the `ic_msg!` call in `schedule_task` so it clearly states that no
signer is allowed, rather than saying the crank signer PDA is permitted.
---
Outside diff comments:
In `@magicblock-task-scheduler/src/db.rs`:
- Around line 117-143: In the task-loading path around the
`stmt.query_map`/`rows.collect` logic in `get_tasks`, a single malformed
`instructions` blob or `authority` value currently turns the whole read into
`Err` and blocks draining valid tasks. Change the row handling so each record is
parsed independently, and on deserialization/parse failure, skip or quarantine
only that task’s `id` instead of propagating the error. Keep the successful
`DbTask` construction path unchanged, but ensure bad rows are removed or marked
separately so migration can continue for the rest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70d07510-06dd-49d4-a882-8e1ad7b04ea5
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/programs/hydra/hydra.sois excluded by!**/*.so
📒 Files selected for processing (32)
config.example.tomlmagicblock-api/src/magic_validator.rsmagicblock-config/src/config/mod.rsmagicblock-config/src/config/scheduler.rsmagicblock-config/src/consts.rsmagicblock-config/src/lib.rsmagicblock-config/src/tests.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-magic-program-api/src/pda.rsmagicblock-processor/src/builtins.rsmagicblock-task-scheduler/Cargo.tomlmagicblock-task-scheduler/src/db.rsmagicblock-task-scheduler/src/hydra.rsmagicblock-task-scheduler/src/lib.rsmagicblock-task-scheduler/src/service.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_task/mod.rsprograms/magicblock/src/schedule_task/process_execute_task.rsprograms/magicblock/src/schedule_task/process_schedule_task.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/configs/schedule-task.devnet.tomltest-integration/test-ledger-restore/src/lib.rstest-integration/test-task-scheduler/src/lib.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rstest-integration/test-task-scheduler/tests/test_independent_authority.rstest-integration/test-task-scheduler/tests/test_migration.rstest-integration/test-task-scheduler/tests/test_reschedule_task.rstest-integration/test-task-scheduler/tests/test_schedule_error.rstest-integration/test-task-scheduler/tests/test_schedule_magic_cpi_crank.rstest-integration/test-task-scheduler/tests/test_schedule_task.rstest-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rstest-integration/test-task-scheduler/tests/test_use_crank_signer.rs
💤 Files with no reviewable changes (14)
- test-integration/test-task-scheduler/tests/test_use_crank_signer.rs
- test-integration/test-task-scheduler/tests/test_schedule_magic_cpi_crank.rs
- test-integration/test-task-scheduler/tests/test_schedule_error.rs
- magicblock-config/src/config/scheduler.rs
- magicblock-magic-program-api/src/pda.rs
- magicblock-processor/src/builtins.rs
- magicblock-config/src/consts.rs
- magicblock-magic-program-api/src/instruction.rs
- magicblock-api/src/magic_validator.rs
- test-integration/test-task-scheduler/tests/test_unauthorized_reschedule.rs
- config.example.toml
- magicblock-config/src/config/mod.rs
- programs/magicblock/src/schedule_task/process_execute_task.rs
- magicblock-config/src/tests.rs
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
magicblock-task-scheduler/src/service.rs (1)
110-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWait for a usable blockhash before serving runtime requests.
send_create()andsend_cancel()pullself.block.load().blockhashdirectly, but the main loop only gates on faucet readiness. Inmagicblock-api/src/magic_validator.rs, the scheduler is spawned beforeinit_slot_ticker(...), so the first schedule/cancel requests can still hit the default blockhash and get logged+dropped instead of retried. Gate the runtime loop on block readiness too, or retry requests until a real blockhash exists.Also applies to: 217-230, 333-335, 391-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/service.rs` around lines 110 - 125, The scheduler loop currently waits only for faucet readiness, but runtime requests in send_create and send_cancel can still see the default blockhash and get dropped. Update TaskSchedulerService::run (or the request processing path used by process_request) to also wait until self.block.load().blockhash is usable before receiving scheduled_tasks, or add retry/requeue handling when the blockhash is not yet initialized. Make sure the same readiness guard covers the initial spawn path from magic_validator so requests are not processed until both faucet and block readiness are satisfied.magicblock-api/src/magic_validator.rs (1)
439-442: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReplace this
expectwith proper error propagation.A missing
tasks_servicechannel now panics during startup instead of returning a structuredApiError. Handle it like the replication-channel branch above and fail initialization cleanly.Suggested change
let task_scheduler = TaskSchedulerService::new( &task_scheduler_db_path, config.aperture.listen.http(), faucet_keypair.insecure_clone(), dispatch .tasks_service .take() - .expect("tasks_service should be initialized"), + .ok_or_else(|| { + ApiError::FailedToSendModeSwitch( + "tasks_service channel missing after init".to_owned(), + ) + })?, ledger.latest_block().clone(), Duration::from_millis(config.ledger.block_time_ms()), token.clone(),As per path instructions,
{magicblock-*,programs,storage-proto}/**: Treat any usage of.unwrap()or.expect()in production Rust code as a MAJOR issue. These should not be categorized as trivial or nit-level concerns. Request proper error handling or explicit justification with invariants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-api/src/magic_validator.rs` around lines 439 - 442, The `tasks_service` initialization in `magic_validator.rs` still uses `expect`, which can panic during startup instead of returning a structured `ApiError`. Update the `tasks_service` branch in the same way as the replication-channel handling above by checking the `Option`, converting the missing-channel case into an error, and propagating it through the existing initialization flow. Use the surrounding `dispatch.tasks_service.take()` logic and related startup error path to keep failure handling consistent and non-panicking.Source: Path instructions
test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs (1)
72-81: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFetch a fresh blockhash before the cancel transaction.
wait_for_hydra_crank()can spend up to 10s here, but the cancel send below still reuses the blockhash fetched before scheduling. With the 50ms test block time, that can age out of the recent-blockhash window and make this test intermittently fail.Proposed fix
wait_for_hydra_crank( &ctx, &crank_pda, Duration::from_secs(10), &mut validator, ); + let ephem_blockhash = + expect!(ctx.try_get_latest_blockhash_ephem(), validator); + // Cancel the task while it is still ongoing. let sig = expect!( ctx.send_transaction_ephem_with_preflight(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs` around lines 72 - 81, The cancel flow in test_cancel_ongoing_task is reusing an old blockhash after wait_for_hydra_crank() can block for several seconds, which may let the transaction fall out of the recent-blockhash window. Before constructing and sending the cancel transaction, fetch a fresh blockhash in the same place you build the cancel request (near the cancel task step) so the transaction uses up-to-date cluster state. Refer to the cancel path around wait_for_hydra_crank(), the cancel send logic, and the task cancellation transaction setup in test_cancel_ongoing_task.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-api/Cargo.toml`:
- Line 56: Enable the missing bincode feature for the solana-system-interface
dependency in the Cargo.toml entry used by magicblock-api. The fund_account path
relies on solana_system_interface::instruction::assign, so update the existing
workspace dependency declaration to include the bincode feature and keep the
dependency location intact. Refer to the solana-system-interface dependency line
and the fund_account.rs usage when making the change.
In `@magicblock-api/src/errors.rs`:
- Around line 39-40: The `FailedToDelegateFaucet` error text in `errors.rs` is
misleading because `ensure_faucet_delegated_on_chain` only checks/delegates a
pre-funded faucet and never funds it. Update the `#[error(...)]` message for
`FailedToDelegateFaucet(Pubkey, String)` to describe the correct remediation:
the faucet must already be funded before delegation, while keeping the faucet
pubkey and underlying error details in the message.
In `@magicblock-api/src/fund_account.rs`:
- Around line 94-97: The early delegation check in fund_account should not rely
only on account.owner == dlp_api::id(), because it can miss cases where the
faucet is delegated to a different validator. Update the already_delegated logic
to verify the delegation matches the target validator binding used by the later
delegation setup in this function, or return an error when the existing
delegation points elsewhere. Apply the same validator-specific validation in the
related delegation path around the later delegation checks so startup does not
skip re-delegation on a mismatched faucet.
In `@magicblock-config/src/config/scheduler.rs`:
- Around line 20-27: The default implementation of TaskSchedulerConfig should
not fall back to the repository-known faucet private key. Update
TaskSchedulerConfig::default and any config-loading path used by
MagicValidator::try_from_config so task-scheduler.faucet-keypair must be
explicitly provided for real deployments, or is only populated by an explicit
dev/test-only mode. Replace consts::DEFAULT_TASK_SCHEDULER_FAUCET_KEYPAIR with a
non-reusable placeholder/example value and ensure
SerdeKeypair/Keypair::from_base58_string are not used to silently enable the
published secret by default.
In `@test-integration/test-task-scheduler/src/lib.rs`:
- Around line 71-89: The test harness is pre-delegating the faucet in a way that
hides startup delegation behavior. Update airdrop_faucet to only fund the faucet
and remove the delegate_account_to_validator call there, so setup_validator*
starts with an undelegated faucet and exercises
ensure_faucet_delegated_on_chain. If needed, add a separate startup-focused test
that explicitly begins with an undelegated faucet and verifies the delegation
path.
In `@test-integration/test-task-scheduler/tests/test_undrained_validator.rs`:
- Around line 33-35: The test is using a hard-coded default validator keypair
literal instead of the validator identity from the harness, which can make the
balance check target the wrong account. Update the setup in
test_undrained_validator to source the validator pubkey from setup_validator()
or reuse the shared consts::DEFAULT_VALIDATOR_KEYPAIR, and then use that value
wherever validator_identity is checked.
---
Outside diff comments:
In `@magicblock-api/src/magic_validator.rs`:
- Around line 439-442: The `tasks_service` initialization in
`magic_validator.rs` still uses `expect`, which can panic during startup instead
of returning a structured `ApiError`. Update the `tasks_service` branch in the
same way as the replication-channel handling above by checking the `Option`,
converting the missing-channel case into an error, and propagating it through
the existing initialization flow. Use the surrounding
`dispatch.tasks_service.take()` logic and related startup error path to keep
failure handling consistent and non-panicking.
In `@magicblock-task-scheduler/src/service.rs`:
- Around line 110-125: The scheduler loop currently waits only for faucet
readiness, but runtime requests in send_create and send_cancel can still see the
default blockhash and get dropped. Update TaskSchedulerService::run (or the
request processing path used by process_request) to also wait until
self.block.load().blockhash is usable before receiving scheduled_tasks, or add
retry/requeue handling when the blockhash is not yet initialized. Make sure the
same readiness guard covers the initial spawn path from magic_validator so
requests are not processed until both faucet and block readiness are satisfied.
In `@test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs`:
- Around line 72-81: The cancel flow in test_cancel_ongoing_task is reusing an
old blockhash after wait_for_hydra_crank() can block for several seconds, which
may let the transaction fall out of the recent-blockhash window. Before
constructing and sending the cancel transaction, fetch a fresh blockhash in the
same place you build the cancel request (near the cancel task step) so the
transaction uses up-to-date cluster state. Refer to the cancel path around
wait_for_hydra_crank(), the cancel send logic, and the task cancellation
transaction setup in test_cancel_ongoing_task.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6831594-1813-492f-998c-c519f321926d
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.locktest-integration/programs/hydra/hydra.sois excluded by!**/*.so
📒 Files selected for processing (23)
Cargo.tomlconfig.example.tomlmagicblock-api/Cargo.tomlmagicblock-api/src/errors.rsmagicblock-api/src/fund_account.rsmagicblock-api/src/magic_validator.rsmagicblock-config/src/config/scheduler.rsmagicblock-config/src/consts.rsmagicblock-config/src/lib.rsmagicblock-config/src/types/crypto.rsmagicblock-task-scheduler/Cargo.tomlmagicblock-task-scheduler/src/errors.rsmagicblock-task-scheduler/src/lib.rsmagicblock-task-scheduler/src/service.rstest-integration/Cargo.tomltest-integration/test-task-scheduler/Cargo.tomltest-integration/test-task-scheduler/src/lib.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rstest-integration/test-task-scheduler/tests/test_independent_authority.rstest-integration/test-task-scheduler/tests/test_migration.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_undrained_validator.rs
💤 Files with no reviewable changes (1)
- magicblock-task-scheduler/src/lib.rs
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-config/src/tests.rs (1)
581-590: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd environment parsing coverage for the faucet keypair.
The replacement
MBV_TASK_SCHEDULER__FAUCET_KEYPAIRcontract is untested. Add a guard and assertions for explicit configuration and omission behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-config/src/tests.rs` around lines 581 - 590, Extend the environment parsing tests around the existing EnvVarGuard setup to cover MBV_TASK_SCHEDULER__FAUCET_KEYPAIR: add a guard with an explicit keypair value and assert that configuration parses to that value, then verify the omitted-variable case produces the expected absence/default behavior. Ensure the guard is scoped so both explicit and omission assertions are isolated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@magicblock-api/src/magic_validator.rs`:
- Around line 452-467: Update the validator initialization flow around
TaskSchedulerService::new to propagate initialization failures as an ApiError
instead of converting them with .ok(). Replace the changed
dispatch.tasks_service.expect(...) with explicit error propagation, preserving
the existing error context where appropriate, so start() never receives a
missing task scheduler and does not rely on an unconditional expect.
- Around line 971-989: Ensure scheduler startup cannot proceed until
ensure_faucet_delegated_on_chain completes successfully, rather than performing
the check only in the detached startup task. Add an explicit readiness barrier
or await this setup before the scheduler launch flow in start(), while
preserving the existing failure logging and process exit behavior.
In `@magicblock-task-scheduler/src/service.rs`:
- Around line 238-251: Update process_request and the schedule/cancel handling
so failed operations are not permanently dropped after a single Hydra/RPC error.
Persist pending operations or implement bounded retries that reconcile current
task state before retrying, especially for ambiguous create and cancel
transactions; preserve clear failure reporting after recovery attempts are
exhausted.
- Around line 134-137: Update the migration documentation to state that invalid
rows and successfully migrated rows are removed, while valid rows whose Hydra
crank creation fails remain in SQLite for a later startup. Apply this correction
in magicblock-task-scheduler/src/service.rs at lines 134-137 and align the
migration narrative in .agents/context/crates/magicblock-task-scheduler.md at
line 119; no code behavior changes are required.
- Around line 321-327: Update crank_exists to return TaskSchedulerResult<bool>,
preserving true only when the account owner is EPHEMERAL_PROGRAM_ID and
returning false only for a confirmed account-not-found response. Propagate all
other RPC failures instead of treating them as absent, and update callers to
handle the propagated result so reschedule cancellation and crank creation
remain correct.
In `@test-integration/test-task-scheduler/tests/test_reschedule_task.rs`:
- Around line 106-113: Strengthen the rescheduling assertion around the second
wait_for_hydra_crank call by capturing the crank account’s original lamports and
data before rescheduling, then polling until the account reflects the expected
larger funding or replacement state. Ensure the test fails when rescheduling is
ignored, rather than only confirming that the same crank PDA still exists.
In `@test-integration/test-task-scheduler/tests/test_schedule_task.rs`:
- Around line 100-106: Update wait_for_hydra_crank_closed and its callers so
RPC/transport failures propagate instead of being interpreted as closure;
explicitly treat account-not-found as closed. In
test-integration/test-task-scheduler/tests/test_schedule_task.rs:100-106, use
the corrected closure helper; in
test-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rs:108-114,
verify account removal or zero lamports; in
test-integration/test-task-scheduler/tests/test_independent_authority.rs:117-122
and test-integration/test-task-scheduler/tests/test_reschedule_task.rs:137-142,
ensure transport errors cannot satisfy cancellation.
---
Outside diff comments:
In `@magicblock-config/src/tests.rs`:
- Around line 581-590: Extend the environment parsing tests around the existing
EnvVarGuard setup to cover MBV_TASK_SCHEDULER__FAUCET_KEYPAIR: add a guard with
an explicit keypair value and assert that configuration parses to that value,
then verify the omitted-variable case produces the expected absence/default
behavior. Ensure the guard is scoped so both explicit and omission assertions
are isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 042fcb77-e76b-41aa-a013-e6851291fb12
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locktest-integration/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.agents/context/crates/magicblock-task-scheduler.mdCargo.tomlconfig.example.tomlmagicblock-api/Cargo.tomlmagicblock-api/src/crank_faucet.rsmagicblock-api/src/errors.rsmagicblock-api/src/lib.rsmagicblock-api/src/magic_validator.rsmagicblock-config/src/config/scheduler.rsmagicblock-config/src/consts.rsmagicblock-config/src/tests.rsmagicblock-magic-program-api/src/instruction.rsmagicblock-processor/src/builtins.rsmagicblock-task-scheduler/src/db.rsmagicblock-task-scheduler/src/service.rsprograms/magicblock/src/magicblock_processor.rsprograms/magicblock/src/schedule_task/mod.rsprograms/magicblock/src/utils/instruction_utils.rstest-integration/Cargo.tomltest-integration/configs/schedule-task.devnet.tomltest-integration/test-task-scheduler/src/lib.rstest-integration/test-task-scheduler/tests/test_cancel_ongoing_task.rstest-integration/test-task-scheduler/tests/test_independent_authority.rstest-integration/test-task-scheduler/tests/test_migration.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_schedule_task_signed.rstest-integration/test-task-scheduler/tests/test_scheduled_commits.rstest-integration/test-task-scheduler/tests/test_undrained_faucet.rs
💤 Files with no reviewable changes (3)
- test-integration/test-task-scheduler/tests/test_scheduled_commits.rs
- magicblock-magic-program-api/src/instruction.rs
- magicblock-processor/src/builtins.rs
There was a problem hiding this comment.
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)
120-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBreak the loop if the task channel is closed.
Currently, if the
scheduled_taskschannel is closed (recv()returnsNone), theselect!macro will disable the branch and block indefinitely onself.token.cancelled(). It is more idiomatic to exit the loop when the channel is closed to prevent the task from hanging unnecessarily.♻️ Proposed refactor
select! { - Some(request) = self.scheduled_tasks.recv() => { - self.process_request(request).await; - } + request = self.scheduled_tasks.recv() => { + match request { + Some(req) => self.process_request(req).await, + None => break, + } + } _ = self.token.cancelled() => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/service.rs` around lines 120 - 127, Update the service loop’s select! branch receiving from self.scheduled_tasks to handle both Some(request) and None, processing requests as before and breaking the loop when the channel is closed; retain the existing self.token.cancelled() shutdown behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@magicblock-task-scheduler/src/service.rs`:
- Around line 120-127: Update the service loop’s select! branch receiving from
self.scheduled_tasks to handle both Some(request) and None, processing requests
as before and breaking the loop when the channel is closed; retain the existing
self.token.cancelled() shutdown behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93c27f68-337c-4267-8758-8f43d259952e
📒 Files selected for processing (1)
magicblock-task-scheduler/src/service.rs
There was a problem hiding this comment.
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)
120-128: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent panic on closed channel in
tokio::select!.If
self.scheduled_tasks.recv()returnsNone(which happens if the channel is closed), the patternSome(request)will fail to match. Because thistokio::select!macro lacks anelsebranch, a pattern mismatch will cause it to panic and crash the task scheduler.Please handle the
Nonecase gracefully (e.g., by breaking out of the loop) to ensure stability.💻 Proposed fix
loop { select! { - Some(request) = self.scheduled_tasks.recv() => { - self.process_request(request).await; - } + request_opt = self.scheduled_tasks.recv() => { + match request_opt { + Some(request) => self.process_request(request).await, + None => { + debug!("Scheduled tasks channel closed"); + break; + } + } + } _ = self.token.cancelled() => { break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@magicblock-task-scheduler/src/service.rs` around lines 120 - 128, Update the tokio::select! loop around self.scheduled_tasks.recv() to handle a closed channel when recv returns None, breaking out of the scheduler loop instead of allowing the unmatched pattern to panic; preserve the existing request processing and cancellation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@magicblock-task-scheduler/src/service.rs`:
- Around line 120-128: Update the tokio::select! loop around
self.scheduled_tasks.recv() to handle a closed channel when recv returns None,
breaking out of the scheduler loop instead of allowing the unmatched pattern to
panic; preserve the existing request processing and cancellation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ac21ada-ec5c-45ce-8591-652290664e70
📒 Files selected for processing (5)
.agents/context/crates/magicblock-task-scheduler.mdmagicblock-api/src/magic_validator.rsmagicblock-task-scheduler/src/service.rstest-integration/test-task-scheduler/src/lib.rstest-integration/test-task-scheduler/tests/test_reschedule_task.rs
💤 Files with no reviewable changes (1)
- test-integration/test-task-scheduler/tests/test_reschedule_task.rs
283e0e3 to
768aa31
Compare
Closes #1375
Summary
First step toward moving toward an external crank service based on Hydra. This PR preserves backward compatibility by forward legacy crank requests to the Hydra program, with the validator being the sponsor of those cranks. Any cranks currently in the SQLite DB will be migrated
Breaking Changes
The task scheduler config is now removed, so validators that were specifying values need to remove them.
Important
This shouldn't be merged until the ephemeral hydra program is deployed and validators are provided with a faucet keypair (will be delegated, so one per validator) funded with enough SOL to prepay for current and future cranks
Also, it requires running the following script to convert the existing DB to hydra crank
Tested using the following script:
python migration-script.py --storage-path path --rpc-url url --identity-keypair keypair --faucet-keypair keypairSummary by CodeRabbit