offchain/scheduler: fail the boot when SOLANA_RPC is unset - #4254
Open
bgm-malbeclabs wants to merge 2 commits into
Open
offchain/scheduler: fail the boot when SOLANA_RPC is unset#4254bgm-malbeclabs wants to merge 2 commits into
bgm-malbeclabs wants to merge 2 commits into
Conversation
The three workers each read the one `:solana_rpc` config key and hand it straight to the NIF, which wants a String. `config/runtime.exs` read the variable with `System.get_env`, so an unset variable reached Rust as nil. `System.fetch_env!` stops the boot instead. The test environment is exempt, because CI runs `mix test` with no such variable and the tests set the key themselves. `DZ_LEDGER_RPC` set a `:ledger_rpc` key that nothing in the application read, so it goes. Two more findings deferred out of the import, which had to keep each tree byte-identical to its source: the scheduler README closes its shell code block, and the revenue-distribution journal test is renamed to match the `lifetime_swapped_2z_amount` field it covers.
| - E2E/QA | ||
| - Remove `TestQA_MulticastSettlement`. It funded a seat through `doublezero-solana shreds pay`, which is going away. The agent seat-pay RPC now returns Unimplemented if something still calls it. Unused settlement helpers go with the test. (#4248) | ||
| - Offchain | ||
| - The scheduler refuses to boot when `SOLANA_RPC` is unset, rather than handing `nil` to the Rust NIF. All three workers read the one config key, so the check sits in `config/runtime.exs` where the single cause was. The test environment is exempt, since the tests set the key themselves. `DZ_LEDGER_RPC` no longer sets a `ledger_rpc` key that nothing in the application read. (#4240 follow-up) |
Contributor
There was a problem hiding this comment.
fetch_env! only fails when the variable is missing. SOLANA_RPC= (empty) still boots and still hands "" to the NIF.
Contributor
Author
There was a problem hiding this comment.
Right, and the empty case lands in exactly the same place: fetch_env! raises only when the variable is absent, so SOLANA_RPC= would hand "" to the NIF instead of nil.
Fixed in 03630ec. The value is trimmed and an empty one raises, naming the variable and why it matters. Checked each case with Config.Reader: absent in the test environment with the variable unset, System.EnvError in prod with it unset, a RuntimeError in prod with it empty or whitespace-only, and the URL in prod with a real value.
martinsander00
approved these changes
Sep 1, 2026
`System.fetch_env!` raises only when the variable is absent, so `SOLANA_RPC=` passed an empty string through to the NIF, which is the failure the guard exists to stop, one step further along. The check now trims and refuses an empty value, naming the variable and why it matters. Checked with Config.Reader against each case: absent in the test environment with the variable unset, System.EnvError in prod with it unset, a RuntimeError in prod with it empty or whitespace, and the URL in prod with a real value.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It has a couple of small but concrete correctness and consistency issues (trimmed validation not applied to the configured URL, and a non-standard changelog reference format).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the offchain scheduler boot path by enforcing a required Solana RPC URL at runtime (outside the test environment), and it includes small follow-up maintenance fixes in docs, tests, and the changelog.
Changes:
- Fail scheduler boot in non-test envs when
SOLANA_RPCis missing or empty, and remove the unusedledger_rpcconfig key. - Fix
offchain/schedulerREADME Markdown rendering by closing an unterminated shell code block. - Rename a revenue-distribution journal test to match the
lifetime_swapped_2z_amountfield it validates, and document these updates inCHANGELOG.md.
File summaries
| File | Description |
|---|---|
| offchain/scheduler/config/runtime.exs | Enforces SOLANA_RPC presence and non-empty value outside :test before configuring workers. |
| offchain/scheduler/README.md | Closes the shell code block so subsequent headings render correctly. |
| solana/programs/revenue-distribution/src/state/journal.rs | Renames a unit test for consistency with the field/method under test. |
| CHANGELOG.md | Records the behavioral change and small follow-ups under Unreleased. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+14
to
+20
| solana_rpc = System.fetch_env!("SOLANA_RPC") | ||
|
|
||
| if String.trim(solana_rpc) == "" do | ||
| raise "SOLANA_RPC is set but empty. The workers pass it straight to the NIF." | ||
| end | ||
|
|
||
| config :scheduler, solana_rpc: solana_rpc |
| - E2E/QA | ||
| - Remove `TestQA_MulticastSettlement`. It funded a seat through `doublezero-solana shreds pay`, which is going away. The agent seat-pay RPC now returns Unimplemented if something still calls it. Unused settlement helpers go with the test. (#4248) | ||
| - Offchain | ||
| - The scheduler refuses to boot when `SOLANA_RPC` is unset or empty, rather than handing `nil` or an empty string to the Rust NIF. All three workers read the one config key, so the check sits in `config/runtime.exs` where the single cause was. The test environment is exempt, since the tests set the key themselves. `DZ_LEDGER_RPC` no longer sets a `ledger_rpc` key that nothing in the application read. (#4240 follow-up) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three findings raised on #4240 that could not be fixed there. That pull request gates on each imported tree staying byte-identical to its filtered source, so any edit to
offchain/orsolana/had to wait for a follow-up. Stacked on #4240 and based on that branch, so the diff shown here is these fixes alone. It retargets tomainonce #4240 merges.Summary
SOLANA_RPCis unset. All three workers read the one:solana_rpcconfig key and hand it straight to the NIF, which wants aString.config/runtime.exsread the variable withSystem.get_env, so an unset variable reached Rust asnil.System.fetch_env!stops the boot instead. One line in the config covers all three workers, which is where the single cause was, rather than a guard in each of them.mix testwith noSOLANA_RPC, and the tests set the key themselves throughApplication.put_env. An unguardedfetch_env!would break the release workflow's test step.DZ_LEDGER_RPCno longer sets a key nothing reads. It set:ledger_rpc, and no worker, module or NIF callback reads that key. It has the same unset-variable gap, so leaving it in place would invite a fix to dead config.test_lifetime_swept_2z_amounttotest_lifetime_swapped_2z_amount, matching thelifetime_swapped_2z_amountfield and method it covers, so a search for the swapped-amount tests finds it.Reported in the copilot review on #4240 and tracked as malbeclabs/infra#2395.
Diff Breakdown
One behaviour change, in a config file. The rest is a heading that rendered in the wrong place and a test name.
Testing Verification
Config.Reader.read!("config/runtime.exs", env: ...)run against each case: the key is absent in the test environment with the variable unset, the read raisesSystem.EnvErrorin prod with it unset, and it carries the URL in prod with it set.cargo test -p doublezero-revenue-distribution --lib test_lifetime_swapped_2z_amountpasses from insidesolana/.mix format --check-formatted config/runtime.exsclean.