refactor: replace grpc queue info with polling#2068
Conversation
sergeytimoshin
requested review from
SwenSchaeferjohann and
ananas-block
as code owners
WalkthroughReplaces Prost/Tonic gRPC-based Photon queue streaming with a kameo actor-based QueueInfoPoller backed by an indexer RPC; removes photon.proto, gRPC router/work_coordinator, build-time proto compilation, and grpc_port config; adds indexer get_queue_info APIs and models; updates EpochManager to use the actor. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant EM as EpochManager
participant QP as QueueInfoPoller (Actor)
participant PI as PhotonIndexer
Note over EM,QP: startup when indexer_url configured
App->>EM: new(config with indexer_url)
EM->>QP: Spawn QueueInfoPoller::new(indexer_url, api_key)
Note over QP,PI: periodic poll loop
QP->>PI: get_queue_info(config)
PI-->>QP: QueueInfoResult (queues + slot)
QP->>QP: distribute_updates() -> mpsc channels to registered trees
Note over EM,QP: registration flow
App->>EM: process_light_slot_v2_event(tree_pubkey)
EM->>QP: ask(RegisterTree{tree_pubkey}).send()
QP-->>EM: Receiver<QueueUpdateMessage>
Note over EM,QP: unregister
App->>EM: unregister_tree(tree_pubkey)
EM->>QP: ask(UnregisterTree{tree_pubkey}).send()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50–70 minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
forester/src/epoch_manager.rs (1)
1381-1422: Channel closure creates a metrics-spamming tight loop—implement the suggested fixThe analysis is correct. When all senders to
queue_update_rxare dropped (e.g., theQueueInfoPollershuts down), theif let Some(update)block at line 1381 becomes a no-op on every iteration. The loop then repeatedly executes lines 1419–1420 without backoff until the time-based exit condition, hammering your metrics endpoint and wasting CPU.The suggested refactor to match on the
Nonecase and break from the labeled loop is the right approach. Your loop label'inner_processing_loopexists at line 1352, so the syntax is valid. The fix preserves event-driven processing during healthy operation while preventing the runaway loop once the channel closes.Implement the suggested patch at lines 1381–1422 to handle channel closure gracefully.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockand included by nonecli/src/commands/test-validator/index.tsis excluded by none and included by nonecli/src/utils/initTestEnv.tsis excluded by none and included by nonecli/src/utils/processPhotonIndexer.tsis excluded by none and included by noneprogram-tests/compressed-token-test/tests/v1.rsis excluded by none and included by noneprogram-tests/system-cpi-v2-test/tests/event.rsis excluded by none and included by nonescripts/devenv/versions.shis excluded by none and included by nonesdk-tests/client-test/tests/light_client.rsis excluded by none and included by none
📒 Files selected for processing (33)
forester/Cargo.toml(1 hunks)forester/build.rs(0 hunks)forester/proto/photon.proto(0 hunks)forester/src/epoch_manager.rs(11 hunks)forester/src/grpc/mod.rs(0 hunks)forester/src/grpc/router.rs(0 hunks)forester/src/lib.rs(1 hunks)forester/src/polling/mod.rs(1 hunks)forester/src/polling/queue_poller.rs(1 hunks)forester/src/work_coordinator.rs(0 hunks)forester/tests/e2e_test.rs(0 hunks)forester/tests/legacy/batched_address_test.rs(0 hunks)forester/tests/legacy/batched_state_async_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_test.rs(0 hunks)forester/tests/legacy/e2e_test.rs(0 hunks)forester/tests/legacy/e2e_v1_test.rs(0 hunks)forester/tests/test_batch_append_spent.rs(0 hunks)forester/tests/test_compressible_ctoken.rs(0 hunks)sdk-libs/client/src/indexer/indexer_trait.rs(2 hunks)sdk-libs/client/src/indexer/mod.rs(1 hunks)sdk-libs/client/src/indexer/photon_indexer.rs(1 hunks)sdk-libs/client/src/indexer/types.rs(1 hunks)sdk-libs/client/src/lib.rs(0 hunks)sdk-libs/client/src/local_test_validator.rs(0 hunks)sdk-libs/client/src/rpc/indexer.rs(3 hunks)sdk-libs/photon-api/src/apis/default_api.rs(2 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs(1 hunks)sdk-libs/photon-api/src/models/mod.rs(1 hunks)sdk-libs/program-test/src/indexer/test_indexer.rs(1 hunks)sdk-libs/program-test/src/program_test/indexer.rs(1 hunks)
💤 Files with no reviewable changes (16)
- forester/tests/legacy/batched_address_test.rs
- forester/tests/e2e_test.rs
- forester/tests/test_compressible_ctoken.rs
- forester/tests/legacy/batched_state_async_indexer_test.rs
- sdk-libs/client/src/lib.rs
- forester/build.rs
- forester/tests/test_batch_append_spent.rs
- forester/tests/legacy/e2e_v1_test.rs
- forester/src/grpc/mod.rs
- forester/proto/photon.proto
- forester/tests/legacy/batched_state_indexer_test.rs
- sdk-libs/client/src/local_test_validator.rs
- forester/tests/legacy/e2e_test.rs
- forester/src/grpc/router.rs
- forester/src/work_coordinator.rs
- forester/tests/legacy/batched_state_test.rs
🧰 Additional context used
🧬 Code graph analysis (10)
sdk-libs/program-test/src/indexer/test_indexer.rs (8)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1791)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
indexer(382-384)sdk-libs/client/src/rpc/rpc_trait.rs (1)
indexer(199-199)sdk-libs/program-test/src/program_test/rpc.rs (1)
indexer(238-240)sdk-libs/client/src/rpc/client.rs (1)
indexer(684-686)
sdk-libs/client/src/indexer/indexer_trait.rs (4)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1791)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/program-test/src/program_test/indexer.rs (7)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1791)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
indexer(382-384)sdk-libs/program-test/src/program_test/rpc.rs (1)
indexer(238-240)sdk-libs/client/src/rpc/client.rs (1)
indexer(684-686)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (2)
sdk-libs/client/src/indexer/photon_indexer.rs (2)
new(115-126)result(1760-1774)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (5)
forester/src/epoch_manager.rs (1)
new(138-184)forester/src/polling/queue_poller.rs (1)
new(66-74)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(24-34)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (3)
forester/src/polling/queue_poller.rs (1)
new(66-74)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(24-34)
sdk-libs/client/src/rpc/indexer.rs (4)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1791)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/client/src/indexer/photon_indexer.rs (7)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (2)
get_queue_info(868-873)new(1355-1416)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)sdk-libs/photon-api/src/apis/default_api.rs (1)
get_queue_info_post(1752-1789)sdk-libs/client/src/indexer/base58.rs (1)
decode_base58_to_fixed_array(37-48)program-libs/compressed-account/src/pubkey.rs (1)
new_from_array(79-81)
forester/src/epoch_manager.rs (2)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/client/src/rpc/rpc_trait.rs (2)
new(34-42)new(76-78)
forester/src/polling/queue_poller.rs (2)
forester/src/epoch_manager.rs (1)
new(138-184)sdk-libs/client/src/indexer/photon_indexer.rs (2)
new(115-126)result(1760-1774)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: system-programs (sdk-libs, light-sdk-macros light-sdk light-program-test light-client light-compr...
- GitHub Check: system-programs (anchor & pinocchio, ["cargo-test-sbf -p sdk-anchor-test", "cargo-test-sbf -p sdk...
- GitHub Check: system-programs (token test, ["cargo-test-sbf -p sdk-token-test"])
- GitHub Check: system-programs (native, ["cargo-test-sbf -p sdk-native-test", "cargo-test-sbf -p sdk-v1-native-t...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: lint
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: programs (light-system-program-address, ["cargo-test-sbf -p system-test -- test_with_address", "c...
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: cli-v1
- GitHub Check: cli-v2
- GitHub Check: stateless-js-v2
- GitHub Check: stateless-js-v1
- GitHub Check: Forester e2e test
🔇 Additional comments (13)
forester/src/lib.rs (1)
13-13: Polling module export is wired appropriatelyExposing
pub mod polling;at the crate root keeps the new queue‑poller functionality discoverable in the same way as the old gRPC/work coordinator modules. No issues with this wiring from an API or layering perspective.sdk-libs/client/src/indexer/mod.rs (1)
20-22: QueueInfo / QueueInfoResult re-exports are correct and completeRe‑exporting
QueueInfoandQueueInfoResultfromtypeshere keeps the indexer public surface coherent and lets downstream code use the new API without reaching into submodules. This matches how other indexer types are exposed.forester/src/polling/mod.rs (1)
1-5: Nice façade over queue_poller internalsUsing
pub mod queue_poller;plus a focusedpub usekeeps the external API (QueueInfoPoller, messages, and counters) clean while allowing you to reorganize internals later without breaking callers. No issues here.sdk-libs/client/src/indexer/indexer_trait.rs (1)
4-14: New get_queue_info API is well-shaped and consistent with existing Indexer methodsPulling
QueueInfoResultinto the type imports and addingget_queue_info(&self, config: Option<IndexerRpcConfig>) -> Result<Response<QueueInfoResult>, IndexerError>fits neatly with the rest of the trait:
- Read‑only call on
&selfwith the standardIndexerRpcConfigenvelope.- Return type matches the Photon indexer implementation and the client/RPC wrappers that proxy this method.
- Doc comment clearly states what’s returned, so consumers have a good mental model.
No issues from an API‑design or correctness perspective.
Also applies to: 204-209
sdk-libs/program-test/src/program_test/indexer.rs (1)
224-235: Delegation for get_queue_info matches existing wrapper patternsThis implementation follows the same guard‑and‑delegate pattern as the rest of the
Indexer for LightProgramTestimpl: it checks for initialization and then forwards to the inner indexer. The signature and return type are consistent with the trait and the client implementation, so this should “just work” once the underlying indexer supports queue info.forester/src/polling/queue_poller.rs (1)
76-99: Conversion is correct and intentional—original concern was unfoundedVerification confirms
QueueType::from(queue.queue_type as u64)is the intended and safe conversion:
- Indexer source:
queue_typeisu8(fromlight_client::indexer::QueueInfo)- QueueType enum: 5 variants with explicit discriminants 1–5
- From impl: Maps 1→NullifierV1, 2→AddressV1, 3→InputStateV2, 4→AddressV2, 5→OutputStateV2, and panics on any other value
- Result: The cast is lossless and 1:1 within the valid range. Rather than silently misclassifying unexpected values, the code intentionally fails fast with a panic, which is the safer behavior.
No changes needed.
sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
11-20: GetQueueInfoPostRequest model is minimal and appropriate for current usageThe struct and
new()constructor align with howget_queue_info_postis invoked (no params yet) and match other OpenAPI‑generated request types in style. No issues from a modeling perspective.sdk-libs/photon-api/src/models/mod.rs (1)
325-332: Queue info models correctly exposed viamodelsmoduleThe new
pub modandpub useentries forGetQueueInfoPostRequest,GetQueueInfoPost200Response,GetQueueInfoPost200ResponseResult, andQueueInfoare consistent with the rest of the file and give downstream code a clean import surface.sdk-libs/client/src/indexer/photon_indexer.rs (1)
1740-1791:get_queue_infoimplementation is consistent with other Photon indexer RPCsThe new method cleanly follows the existing pattern: wraps the call in
retry, enforcesresult.slot >= config.slot, and converts APIQueueInfoentries into internalQueueInfousing the shared base58 helpers andPubkey::new_from_array. Error mapping intoIndexerError::PhotonErroris also in line with the rest of the file.If the Photon API ever adds an
errorfield to the 200‑response envelope, consider reusing the existingextract_result_with_error_checkhelper for uniform error handling, but the current implementation is correct for the present model.sdk-libs/client/src/rpc/indexer.rs (1)
10-12: RPC facade correctly wires queue APIs throughLightClientImporting
QueueElementsResultandQueueInfoResultinto the local prelude and updatingget_queue_elements’s return type keeps the signatures consistent and avoids overly verbose paths. The newget_queue_infomethod follows the same delegation +NotInitializedpattern as the other RPCs, so the public surface ofLightClientstays uniform.Also applies to: 204-238
sdk-libs/photon-api/src/apis/default_api.rs (1)
298-305:get_queue_info_postwiring matches the existing generated HTTP helpersThe new
GetQueueInfoPostErrorenum andget_queue_info_postfunction follow the same pattern as every other endpoint in this file: same status handling, sameResponseContentconstruction on error, and the usual API‑key + User‑Agent handling. ReusingGetBatchAddressUpdateInfoPost429Responsefor 429/500 is consistent with the surrounding error enums.Given the 200‑response model currently has only a
resultfield (noerror), this helper will treat any non‑2xx HTTP status as an error and any 2xx body as a successful queue info result. That’s fine as long as the Photon indexer doesn’t send logical JSON‑RPC errors inside 2xx responses for/getQueueInfo.Also applies to: 1752-1789
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (1)
11-46: QueueInfo result model aligns with downstream consumer expectations
GetQueueInfoPost200ResponseResultandQueueInfoare shaped as expected for the indexer client:queuesis a flat list,slotis a simple u64, andqueueType/queueSizematch the fields the client maps intolight_client::indexer::QueueInfo. As long as the backend’squeueTypenumeric values stay in sync with yourQueueTypeenum, this model should deserialize cleanly.forester/src/epoch_manager.rs (1)
17-20: QueueInfoPoller integration and V2 tree registration look solidThe new
queue_poller: Option<ActorRef<QueueInfoPoller>>field, initialization inEpochManager::new, and per‑treeRegisterTreeflow inperform_active_work/process_queue_v2are all wired in a way that:
- Only activates polling when
indexer_urlis configured.- Fails soft: if spawning/registering with the poller fails, V2 trees fall back to the existing polling path.
- Keeps V1 and “compression tree” behavior unchanged.
The additional
skip(.., queue_update_rx)annotations on the tracing spans are also a good touch to keep logs readable.Also applies to: 50-51, 100-114, 136-183, 953-1001
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 (2)
forester/src/epoch_manager.rs (2)
972-994: V2 tree tasks spawn even when registration fails, leading to idle workersWhen
RegisterTreefails (lines 982-986), the code logs a warning but still spawns a processing task for that tree. Later,process_queue_v2detects the missing channel (lines 1162-1166) and logs repeatedly that it will be "inactive," but the task continues running and consuming the slot schedule.This wastes resources and produces confusing logs. Consider:
- Skipping the task spawn entirely when registration fails (continue the loop at line 994), OR
- Propagating the error and failing the epoch if V2 queue updates are critical to your operation.
If trees can legitimately run without the indexer, document that behavior clearly so operators understand why tasks show as "inactive."
1376-1418: Critical:recv().awaitblocks indefinitely and can prevent slot-end detectionAt line 1377, the code blocks on
queue_update_rx.recv().awaitwaiting for a queue update. If no update arrives (e.g., the queue is empty or the poller is slow), the task will hang and never check whetherestimated_slot >= forester_slot_details.end_solana_slot(line 1348).This can cause the forester to miss its slot deadline, fail to exit the loop, and potentially stall the entire tree's processing schedule.
Fix: Use
tokio::select!to wait for either:
- An update from
queue_update_rx.recv(), OR- A timeout/interval that periodically checks
estimated_slotand breaks when the slot has ended.Example refactor:
'inner_processing_loop: loop { + // Re-check slot end before blocking on recv if estimated_slot >= forester_slot_details.end_solana_slot { trace!(/* ... */); break 'inner_processing_loop; } - // Wait for queue update from poller - if let Some(update) = queue_update_rx.recv().await { + // Wait for queue update OR timeout to check slot end + let update = tokio::select! { + maybe_update = queue_update_rx.recv() => { + match maybe_update { + Some(u) => u, + None => { + warn!("Queue update channel closed for tree {}", tree_pubkey); + break 'inner_processing_loop; + } + } + } + _ = tokio::time::sleep(Duration::from_millis(500)) => { + // Re-check estimated_slot and continue + estimated_slot = self.slot_tracker.estimated_current_slot(); + continue 'inner_processing_loop; + } + }; + + if update.queue_size > 0 { // existing processing logic... }This ensures the task can always exit when the slot ends, even if no updates arrive.
♻️ Duplicate comments (7)
forester/src/polling/queue_poller.rs (7)
37-37:polling_activefield is never read—either use it or remove itThe
polling_activeflag is set totrueon construction (line 72) andfalseon stop (line 60), but it's never checked anywhere. This makes it dead state that suggests an incomplete shutdown mechanism.Two options:
- Use it for graceful shutdown: Clone the
Arc<AtomicBool>intopolling_loop, checkpolling_active.load(Ordering::Acquire)each iteration, and break cleanly when false.- Remove it: Delete the field and rely entirely on actor shutdown semantics (when
tell().send()fails, the loop exits).Right now it just adds maintenance overhead without providing any functionality.
228-238: Error log on normal shutdown produces false alarmsWhen the actor stops gracefully,
tell().send()will fail and trigger theerror!log at line 234: "Failed to send poll message to actor." This is expected behavior during shutdown, not an error condition, so the log is misleading.Fix: Change line 234 to:
- error!("Failed to send poll message to actor: {:?}", e); + debug!("Polling loop ending (actor stopped): {:?}", e);Alternatively, if you implement the
polling_activeflag check mentioned in the previous comment, you can distinguish intentional shutdown (debug log) from unexpected failure (error log).
103-135: Avoid cloning message and remove closed channels to reduce log spamAt line 114,
message.clone()is unnecessary—you can movemessageintotry_sendand useinfo.*fields for logging. Additionally, closed receivers will triggerClosederrors on every poll until unregistered, filling logs with repeated warnings.Refactor:
fn distribute_updates(&mut self, queue_infos: Vec<QueueInfo>) { for info in queue_infos { if let Some(tx) = self.tree_notifiers.get(&info.tree) { let message = QueueUpdateMessage { tree: info.tree, queue: info.queue, queue_type: info.queue_type, queue_size: info.queue_size, slot: info.slot, }; - match tx.try_send(message.clone()) { + match tx.try_send(message) { Ok(()) => { trace!( "Routed update to tree {}: {} items (type: {:?})", - info.tree, message.queue_size, info.queue_type + info.tree, info.queue_size, info.queue_type ); } Err(mpsc::error::TrySendError::Full(_)) => { warn!(/* ... */); } Err(mpsc::error::TrySendError::Closed(_)) => { - debug!("Tree {} channel closed (task likely finished)", info.tree); + debug!("Removing closed channel for tree {}", info.tree); + self.tree_notifiers.remove(&info.tree); } } } } }This requires changing the signature to
&mut selfto allow removal.
155-164: Double registration silently replaces existing subscriptionIf
RegisterTreeis called twice for the same tree, theinsertat line 161 overwrites the previous sender, and the original receiver suddenly stops receiving updates. This could be confusing for callers who don't expect their subscription to be replaced.Consider:
async fn handle(/* ... */) -> Self::Reply { let (tx, rx) = mpsc::channel(100); - self.tree_notifiers.insert(msg.tree_pubkey, tx); + if let Some(old_tx) = self.tree_notifiers.insert(msg.tree_pubkey, tx) { + warn!("Tree {} was already registered; replacing previous subscription", msg.tree_pubkey); + } debug!("Registered tree {} for queue updates", msg.tree_pubkey); rx }Alternatively, return the existing receiver if one exists, or return a
Resultindicating double registration.
175-182: Log when unregistering a non-existent tree
UnregisterTreesilently does nothing if the tree isn't registered. Logging whenremovereturnsNonehelps detect mismatches (e.g., unregistering before registering, or after the receiver was already dropped).async fn handle(/* ... */) -> Self::Reply { - self.tree_notifiers.remove(&msg.tree_pubkey); + if self.tree_notifiers.remove(&msg.tree_pubkey).is_none() { + debug!("Attempted to unregister tree {} but it was not registered", msg.tree_pubkey); + } else { + debug!("Unregistered tree {}", msg.tree_pubkey); + } - debug!("Unregistered tree {}", msg.tree_pubkey); }
206-219: Short-circuitPollNowwhen no trees are registered to reduce indexer loadThe handler calls
poll_queue_infoeven whentree_notifiersis empty, wasting indexer bandwidth since all results will be discarded indistribute_updates.Add early exit:
async fn handle(/* ... */) -> Self::Reply { + if self.tree_notifiers.is_empty() { + trace!("No trees registered; skipping queue info poll"); + return; + } + match self.poll_queue_info().await { Ok(queue_infos) => { self.distribute_updates(queue_infos); } Err(e) => { error!("Failed to poll queue info: {:?}", e); } } }This eliminates unnecessary indexer calls when idle. For further optimization, consider adaptive backoff (increase interval when queues are empty) to reduce steady-state polling load.
23-23: Consider making polling interval configurable
POLLING_INTERVAL_SECSis hard-coded to 1 second. For operational tuning (e.g., adjusting for indexer rate limits, reducing load when queues are empty, or speeding up during high activity), a configurable interval would be helpful.Suggestion: Add a
polling_intervalfield toForesterConfigand pass it through toQueueInfoPoller::new. This allows runtime/config-based tuning without code changes.For now, 1 second is reasonable, but consider this for future operational flexibility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (7)
forester/src/epoch_manager.rs(13 hunks)forester/src/polling/queue_poller.rs(1 hunks)forester/tests/e2e_test.rs(0 hunks)sdk-libs/client/Cargo.toml(1 hunks)sdk-libs/client/src/indexer/photon_indexer.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs(1 hunks)
💤 Files with no reviewable changes (1)
- forester/tests/e2e_test.rs
🧰 Additional context used
🧬 Code graph analysis (5)
forester/src/epoch_manager.rs (2)
forester/src/polling/queue_poller.rs (1)
new(66-74)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)
sdk-libs/client/src/indexer/photon_indexer.rs (6)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)sdk-libs/photon-api/src/apis/default_api.rs (1)
get_queue_info_post(1752-1789)sdk-libs/client/src/indexer/base58.rs (1)
decode_base58_to_fixed_array(37-48)program-libs/compressed-account/src/pubkey.rs (1)
new_from_array(79-81)
sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (3)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
forester/src/polling/queue_poller.rs (5)
forester/src/epoch_manager.rs (1)
new(138-184)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
new(27-34)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Test program-libs-fast
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-slow
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: Forester e2e test
🔇 Additional comments (6)
sdk-libs/client/src/indexer/photon_indexer.rs (2)
1775-1776: LGTM: Base58 decoding with appropriate error handlingThe use of
decode_base58_to_fixed_arrayfor bothtreeandqueueproperly handles decoding errors via the?operator. The error will propagate asIndexerError::InvalidResponseDataif the base58 string is malformed or the wrong length.
1767-1769: Verify slot staleness check is sufficient for your use caseThe slot check
if api_response.slot < config.slotonly triggers when an explicitconfig.slotis provided. If callers omit the config or don't set a minimum slot, stale data could be returned without warning.If queue info staleness is critical for correctness (e.g., for epoch scheduling), consider:
- Logging when the returned slot is significantly behind the current slot, or
- Documenting that callers must provide a slot threshold if they need freshness guarantees.
This is consistent with other methods in the file, so it's more of a usage note than a defect.
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
11-47: LGTM: JSON-RPC 2.0 response structure is well-definedThe response model now includes the
errorfield (addressing the earlier review concern), and bothresultanderrorare properly optional. TheJsonRpcErrorstructure follows JSON-RPC 2.0 conventions withcode,message, and optionaldata.The constructor defaults
errortoNone, which is appropriate for success responses. Deserialization will handle cases where the server returns an error instead of a result.sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
11-64: LGTM: Request model is correctly structured for parameterless JSON-RPC callThe request model properly represents a JSON-RPC 2.0 call to
getQueueInfo:
- Empty
GetQueueInfoParamsstruct correctly indicates the method takes no parameters- Type-safe enums for
jsonrpc(version) andmethod(name) prevent typos- Constructor properly defaults
paramsto an empty structThis follows standard JSON-RPC patterns and matches the usage in the PhotonIndexer implementation.
forester/src/epoch_manager.rs (2)
150-167: LGTM: QueueInfoPoller initialization with appropriate fallbackThe initialization properly:
- Spawns the actor only when
indexer_urlis configured- Logs the startup and fallback cases clearly
- Uses
Option<ActorRef<QueueInfoPoller>>to handle the optional indexer integrationThe
Nonefallback when indexer_url is not set allows the forester to run without V2 queue updates, which is appropriate for backward compatibility or testing scenarios.
1152-1166: Appropriate fallback when queue update channel is unavailableThe code correctly handles missing
queue_update_rxby logging a warning and returning early. This prevents V2 trees from attempting to process without queue updates, which would be incorrect.The warning message clearly explains the situation: "No queue update channel available for V2 tree ... Processing will be inactive."
This is consistent with the registration failure handling elsewhere in the file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
forester/src/polling/queue_poller.rs (1)
104-136: Remove unnecessary clone and clean up closed senders.Two related issues:
Line 115:
message.clone()is unnecessary—you can movemessageintotry_sendand still log using the fields frominfo(which are already copied intomessage).Lines 130–132: When a receiver is closed, you log at
trace!but never remove the sender fromtree_notifiers. This means every subsequent poll will attempt to send to a closed channel and log again, creating noise and wasting cycles.Apply this refactor to eliminate the clone and clean up closed channels:
- fn distribute_updates(&self, queue_infos: Vec<QueueInfo>) { + fn distribute_updates(&mut self, queue_infos: Vec<QueueInfo>) { + let mut closed_trees = Vec::new(); + for info in queue_infos { if let Some(tx) = self.tree_notifiers.get(&info.tree) { let message = QueueUpdateMessage { tree: info.tree, queue: info.queue, queue_type: info.queue_type, queue_size: info.queue_size, slot: info.slot, }; - match tx.try_send(message.clone()) { + match tx.try_send(message) { Ok(()) => { trace!( "Routed update to tree {}: {} items (type: {:?})", info.tree, - message.queue_size, + info.queue_size, info.queue_type ); } Err(mpsc::error::TrySendError::Full(_)) => { warn!( "Tree {} channel full, dropping update (tree processing slower than updates)", info.tree ); } Err(mpsc::error::TrySendError::Closed(_)) => { - trace!("Tree {} channel closed (task likely finished)", info.tree); + debug!("Tree {} channel closed (task likely finished), will unregister", info.tree); + closed_trees.push(info.tree); } } } } + + // Remove closed senders + for tree in closed_trees { + self.tree_notifiers.remove(&tree); + } }This also requires changing the signature to
&mut selfsince we're now mutatingtree_notifiers. ThePollNowhandler already has&mut self, so this is safe.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (2)
forester/src/polling/queue_poller.rs(1 hunks)sdk-libs/client/Cargo.toml(0 hunks)
💤 Files with no reviewable changes (1)
- sdk-libs/client/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
forester/src/polling/queue_poller.rs (3)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)forester/src/epoch_manager.rs (1)
new(138-184)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
- GitHub Check: Test program-libs-slow
- GitHub Check: Forester e2e test
- GitHub Check: programs (system-cpi-test-v2-functional-read-only, ["cargo-test-sbf -p system-cpi-v2-test -- func...
- GitHub Check: programs (system-cpi-test-v2-functional-account-infos, ["cargo-test-sbf -p system-cpi-v2-test -- ...
- GitHub Check: programs (account-compression-and-registry, ["cargo-test-sbf -p account-compression-test", "cargo...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: programs (light-system-program-compression, ["cargo-test-sbf -p system-test -- test_with_compress...
- GitHub Check: programs (system-cpi-test, ["cargo-test-sbf -p system-cpi-test", "cargo test -p light-system-prog...
🔇 Additional comments (5)
forester/src/polling/queue_poller.rs (5)
163-173: Good handling of double registration.The warning log and explicit
drop(old_sender)clearly communicate the behavior when a tree is registered multiple times. This addresses the concern from past reviews and makes the subscription lifecycle more explicit.Optional enhancement: Consider making the channel buffer size (line 161) configurable alongside
POLLING_INTERVAL_SECSif you expect different workloads or tree update rates.
192-202: Good handling of unregistration edge cases.Checking the
removereturn value and logging when attempting to unregister a non-existent tree makes the subscription lifecycle more observable and easier to debug. This addresses the past review concern.
232-235: Good optimization: early-exit when no subscribers.Skipping the indexer poll when
tree_notifiersis empty avoids unnecessary load. This addresses the past review concern about wasted indexer traffic when idle.
248-279: Polling loop implementation looks solid.Several good practices:
- Line 252:
MissedTickBehavior::Skipprevents tick buildup if processing is slow, which is correct for this use case.- Lines 256, 264: The double check of
polling_active(before and after the tick) ensures responsive shutdown while being defensive about the timing of the stop signal.- Lines 269–274: The error log is appropriate here—since
polling_activeis checked before attempting to send, reaching this error means the actor has stopped unexpectedly (not a clean shutdown), so logging aterror!level is justified.The past review concern about noisy shutdown logs has been addressed by the
polling_activechecks that break the loop cleanly before attempting to send to a stopped actor.
89-89: Theu8→u64conversion is safe but indirect; aFrom<u8>implementation would be more idiomatic.The API returns
queue_typeasu8(confirmed ingit/sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs), which is cast tou64before callingQueueType::from. TheFrom<u64>implementation exists (inprogram-libs/compressed-account/src/lib.rs:152), but noFrom<u8>implementation was found.The cast itself is safe—
u8tou64is lossless—so the code works correctly. However, it's unnecessarily indirect. Since the source is alwaysu8, implementingFrom<u8> for QueueTypewould eliminate the intermediate cast and make the intent clearer. Consider adding that trait implementation to theQueueTypedefinition if it's a frequent conversion pattern across the codebase.
sergeytimoshin
force-pushed
the
sergey/get_queue_info_poll
branch
from
6513182 to
f2549c3
Compare
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)
forester/src/epoch_manager.rs (1)
953-994: V2 tree registration logic is correct, but failed registrations may be silent.The code properly uses
poller.ask(RegisterTree).send().awaitto obtain the queue update receiver. However, if registration fails (lines 981-986), processing continues withqueue_update_rx = None, which means that tree will silently not process any V2 updates.Consider whether failed registration warrants more visibility:
Err(e) => { - warn!( + error!( "Failed to register tree {} with poller: {:?}", tree.tree_accounts.merkle_tree, e ); None }This makes registration failures more prominent in logs since they represent a degraded operational state for that tree.
♻️ Duplicate comments (4)
sdk-libs/client/src/indexer/types.rs (1)
32-44:QueueInfo/QueueInfoResultmatch the Photon queue-info payloadsThe field set (tree, queue, type, size, plus top-level queues + slot) and derives are appropriate for client use and align with how
PhotonIndexer::get_queue_infopopulates them. The earlier comment aboutslotbeing duplicated betweenResponse.contextandQueueInfoResult::slotstill applies but doesn’t block correctness.sdk-libs/program-test/src/indexer/test_indexer.rs (1)
868-873: Avoidunimplemented!inTestIndexer::get_queue_infoto prevent panics in testsRight now this method will panic whenever a test (directly or via LightProgramTest/LightClient) calls
get_queue_info, which makes failures noisy and harder to debug compared to a structuredIndexerError.You can keep the trait satisfied while returning a clear, non-retryable error, e.g.:
- async fn get_queue_info( - &self, - _config: Option<IndexerRpcConfig>, - ) -> Result<Response<light_client::indexer::QueueInfoResult>, IndexerError> { - unimplemented!("get_queue_info") - } + async fn get_queue_info( + &self, + _config: Option<IndexerRpcConfig>, + ) -> Result<Response<light_client::indexer::QueueInfoResult>, IndexerError> { + Err(IndexerError::NotImplemented( + "get_queue_info is not implemented for TestIndexer".into(), + )) + }This keeps the contract explicit and avoids crashing test processes when the API starts being used.
forester/Cargo.toml (1)
62-62: kameo version 0.19 does not exist—use 0.17.x or pin a git revision.As flagged in the previous review, kameo 0.19 is not published on crates.io. The latest stable release is 0.17.2. This will cause
cargo buildto fail.Apply one of these fixes:
Option 1: Use the latest stable release
-kameo = "0.19" +kameo = "0.17"Option 2: Pin to a specific git commit if you need unreleased features
-kameo = "0.19" +kameo = { git = "https://github.com/tqwewe/kameo", rev = "COMMIT_HASH" }After updating, run
cargo updateandcargo checkto verify compatibility with your Tokio version (kameo 0.17.x requires Tokio ^1.44) and MSRV (1.85.1).Based on learnings (previous review).
forester/src/polling/queue_poller.rs (1)
105-137: Consider removing closed senders to avoid repeated logs.The method correctly routes updates to registered trees. However, when a receiver is closed (line 131), the sender remains in
tree_notifiers, causing a trace log on every subsequent update until explicit unregistration.For cleaner behavior, consider removing closed senders immediately:
fn distribute_updates(&self, queue_infos: Vec<QueueInfo>) { + let mut closed_trees = Vec::new(); for info in queue_infos { if let Some(tx) = self.tree_notifiers.get(&info.tree) { // ... existing code ... match tx.try_send(message.clone()) { // ... existing cases ... Err(mpsc::error::TrySendError::Closed(_)) => { trace!("Tree {} channel closed (task likely finished)", info.tree); + closed_trees.push(info.tree); } } } } + // Note: This requires &mut self, so you'd need to change the signature or use interior mutability }Alternatively, change the signature to
fn distribute_updates(&mut self, ...)and remove closed senders inline. This is a minor optimization; current behavior is functionally correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockand included by nonecli/src/commands/test-validator/index.tsis excluded by none and included by nonecli/src/utils/initTestEnv.tsis excluded by none and included by nonecli/src/utils/processPhotonIndexer.tsis excluded by none and included by noneprogram-tests/compressed-token-test/tests/v1.rsis excluded by none and included by noneprogram-tests/system-cpi-v2-test/tests/event.rsis excluded by none and included by nonescripts/devenv/versions.shis excluded by none and included by nonesdk-tests/client-test/tests/light_client.rsis excluded by none and included by none
📒 Files selected for processing (34)
forester/Cargo.toml(1 hunks)forester/build.rs(0 hunks)forester/proto/photon.proto(0 hunks)forester/src/epoch_manager.rs(13 hunks)forester/src/grpc/mod.rs(0 hunks)forester/src/grpc/router.rs(0 hunks)forester/src/lib.rs(1 hunks)forester/src/polling/mod.rs(1 hunks)forester/src/polling/queue_poller.rs(1 hunks)forester/src/work_coordinator.rs(0 hunks)forester/tests/e2e_test.rs(0 hunks)forester/tests/legacy/batched_address_test.rs(0 hunks)forester/tests/legacy/batched_state_async_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_test.rs(0 hunks)forester/tests/legacy/e2e_test.rs(0 hunks)forester/tests/legacy/e2e_v1_test.rs(0 hunks)forester/tests/test_batch_append_spent.rs(0 hunks)forester/tests/test_compressible_ctoken.rs(0 hunks)sdk-libs/client/Cargo.toml(0 hunks)sdk-libs/client/src/indexer/indexer_trait.rs(2 hunks)sdk-libs/client/src/indexer/mod.rs(1 hunks)sdk-libs/client/src/indexer/photon_indexer.rs(1 hunks)sdk-libs/client/src/indexer/types.rs(1 hunks)sdk-libs/client/src/lib.rs(0 hunks)sdk-libs/client/src/local_test_validator.rs(0 hunks)sdk-libs/client/src/rpc/indexer.rs(3 hunks)sdk-libs/photon-api/src/apis/default_api.rs(2 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs(1 hunks)sdk-libs/photon-api/src/models/mod.rs(1 hunks)sdk-libs/program-test/src/indexer/test_indexer.rs(1 hunks)sdk-libs/program-test/src/program_test/indexer.rs(1 hunks)
💤 Files with no reviewable changes (17)
- sdk-libs/client/src/lib.rs
- forester/tests/legacy/batched_state_indexer_test.rs
- forester/tests/test_batch_append_spent.rs
- forester/tests/e2e_test.rs
- forester/tests/legacy/e2e_v1_test.rs
- forester/tests/legacy/batched_state_async_indexer_test.rs
- forester/tests/legacy/batched_state_test.rs
- forester/build.rs
- forester/src/grpc/mod.rs
- sdk-libs/client/src/local_test_validator.rs
- forester/tests/legacy/e2e_test.rs
- forester/tests/test_compressible_ctoken.rs
- forester/src/work_coordinator.rs
- forester/tests/legacy/batched_address_test.rs
- forester/src/grpc/router.rs
- forester/proto/photon.proto
- sdk-libs/client/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (10)
sdk-libs/client/src/indexer/indexer_trait.rs (4)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/program-test/src/indexer/test_indexer.rs (8)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
indexer(382-384)sdk-libs/client/src/rpc/rpc_trait.rs (1)
indexer(199-199)sdk-libs/client/src/rpc/client.rs (1)
indexer(684-686)sdk-libs/program-test/src/program_test/rpc.rs (1)
indexer(238-240)
sdk-libs/client/src/indexer/photon_indexer.rs (4)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/client/src/indexer/base58.rs (1)
decode_base58_to_fixed_array(37-48)program-libs/compressed-account/src/pubkey.rs (1)
new_from_array(79-81)
sdk-libs/client/src/rpc/indexer.rs (4)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (1)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (2)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
new(27-34)
sdk-libs/program-test/src/program_test/indexer.rs (5)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
indexer(382-384)sdk-libs/program-test/src/program_test/rpc.rs (1)
indexer(238-240)
forester/src/epoch_manager.rs (1)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)
sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (2)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
forester/src/polling/queue_poller.rs (4)
forester/src/epoch_manager.rs (1)
new(138-184)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: Test batched-merkle-tree-simulate
🔇 Additional comments (30)
sdk-libs/client/src/indexer/mod.rs (1)
16-23: Re-export wiring forQueueInfo/QueueInfoResultlooks correctThe new queue-related types are exported alongside the rest of the indexer surface, so downstream crates can use them without extra imports; no issues here.
sdk-libs/program-test/src/program_test/indexer.rs (1)
224-235:LightProgramTest::get_queue_infodelegation is consistent with existing indexer methodsThe method correctly guards against
NotInitializedand forwardsconfigand the response from the inner indexer, matching the pattern of other delegated calls.sdk-libs/client/src/indexer/indexer_trait.rs (2)
4-14: Import ofQueueInfoResultinto the trait module is correctThe new type is pulled in alongside other indexer value types, allowing the trait method to use it without fully-qualified paths.
204-209: Newget_queue_infotrait method is well-shapedThe async signature and documentation match the Photon/indexer implementations and queue-info types (tree, queue, type, size), so implementors have a clear contract to follow.
sdk-libs/photon-api/src/models/mod.rs (1)
325-332: Queue-info Photon models are wired intomodels::modconsistentlyThe new
GetQueueInfoPost*modules andQueueInfore-export follow the same pattern as other API endpoints, so downstream uses inphoton_indexerwill resolve cleanly.sdk-libs/client/src/indexer/photon_indexer.rs (1)
1740-1798:PhotonIndexer::get_queue_infocorrectly adapts Photon API to client typesThe method cleanly follows the existing retry pattern, enforces
config.slotfreshness, decodes base58tree/queuefields intoPubkeys, and wraps everything inQueueInfoResultinside aResponse; this is consistent with the rest of the indexer surface.sdk-libs/client/src/rpc/indexer.rs (3)
5-12: Indexer RPC import list correctly includes queue-related typesPulling
QueueElementsResultandQueueInfoResultinto scope simplifies signatures and keeps all indexer value types centralized here.
204-213:get_queue_elementsreturn type refactor is purely cosmeticSwitching from
Response<crate::indexer::QueueElementsResult>toResponse<QueueElementsResult>just leverages the new import; it doesn’t alter behavior or API shape.
228-238: LightClient RPC now exposesget_queue_infoconsistentlyThe new method mirrors other RPC bridge methods: it guards against
NotInitializedand forwardsconfigand the inner indexer’sResponse<QueueInfoResult>unchanged, which is exactly what callers would expect.sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
13-47: LGTM - Previous concern about error field has been addressedThe response model now properly includes both
resultanderrorfields (line 27, 29), matching the JSON-RPC 2.0 pattern used by other endpoints. TheJsonRpcErrorstruct is correctly defined with standard JSON-RPC error fields. The constructor provides a convenient happy-path builder while still allowing direct field access for error cases.sdk-libs/photon-api/src/apis/default_api.rs (2)
298-305: LGTM - Error enum follows established patternThe
GetQueueInfoPostErrorenum is consistent with all other endpoint error enums in this file, using the same variants and structure.
1752-1790: LGTM - Implementation follows established patternsThe
get_queue_info_postfunction correctly implements the standard pattern used by all other POST endpoints in this file: proper configuration handling, User-Agent header attachment, request body serialization, and consistent error response handling.sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (1)
11-46: LGTM - Queue info models are well-structuredThe data models appropriately represent queue information:
GetQueueInfoPost200ResponseResultcorrectly bundles queue data with slot numberQueueInfofields have appropriate types (u8 for queue_type, u64 for sizes)- Serde rename attributes properly map to camelCase JSON field names
- Constructors provide convenient initialization
sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
11-64: LGTM - JSON-RPC request model is correctly structuredThe request model properly implements JSON-RPC 2.0 structure:
- Empty
GetQueueInfoParams(line 37-38) is a valid pattern when the endpoint requires no parameters- Constructor appropriately defaults
params(line 32) since it's emptyJsonrpcandMethodenums enforce valid values at the type level- Default implementations provide sensible values ("2.0" for jsonrpc, GetQueueInfo for method)
forester/src/lib.rs (1)
13-13: LGTM! Clean architectural boundary for the new polling subsystem.The new
pollingmodule properly replaces the removedgrpcandwork_coordinatormodules, providing a clear API boundary for the actor-based queue polling mechanism.forester/src/polling/mod.rs (1)
1-5: LGTM! Clean module structure with well-defined public API.The re-exports provide a clear interface for the polling subsystem: the actor (
QueueInfoPoller), the update message (QueueUpdateMessage), and the three command messages (RegisterTree,UnregisterTree,RegisteredTreeCount).forester/src/epoch_manager.rs (6)
17-17: LGTM! Imports correctly reflect the architectural migration.The new imports properly replace the gRPC-based coordination with actor-based polling:
kameo::actor::{ActorRef, Spawn}for actor lifecycle managementpolling::{QueueInfoPoller, QueueUpdateMessage, RegisterTree}for the new polling APIAlso applies to: 50-50
112-112: LGTM! Field type and Clone implementation are correct.The
queue_pollerfield properly replaces the coordinator, andActorRef<QueueInfoPoller>is the correct handle type for the kameo actor. TheOptionwrapper appropriately handles cases whereindexer_urlis not configured.Also applies to: 130-130
1107-1107: LGTM! Unified processing function with optional event-driven updates.The
queue_update_rxparameter correctly enables V2 trees to receive event-driven queue updates while maintaining compatibility with V1 trees (which passNone).Also applies to: 1111-1117
1152-1166: Clarify expected behavior when V2 trees lack queue update channels.When
queue_update_rxisNone(lines 1162-1166), the code logs a warning and returnsOk(()), meaning the tree won't process any queue items during its assigned slots. This occurs when:
- No indexer URL is configured, or
- Tree registration with the poller failed
Is this the intended behavior, or should these trees fall back to periodic polling of the queue? Currently, a configuration issue or transient registration failure causes permanent inactivity for that tree in that epoch.
Consider documenting this behavior or implementing a fallback:
} else { warn!("No queue update channel for V2 tree {}. Falling back to periodic polling.", tree_schedule.tree_accounts.merkle_tree ); // Optionally: implement fallback polling logic here Ok(()) }
1318-1318: LGTM! V2 event-driven processing properly integrated.The signature correctly takes
queue_update_rx: &mut mpsc::Receiver<QueueUpdateMessage>, and the initialization properly waits for the slot boundary before beginning processing.Also applies to: 1321-1328
150-167: The original review comment is based on an incorrect assumption about kameo's API.After verification, kameo's
spawn()method returns anActorRefdirectly—not aResult. The actor spawn function returns an actor reference (an ActorRef), not a Result. The code at lines 150–167 is correct as written and requires no error handling.The
QueueInfoPollerstruct implements kameo'sActortrait, and when you callspawn()on an actor, it either succeeds and returns anActorRef, or the function itself would panic (not return an error). The suggested match-based error handling is unnecessary and doesn't align with kameo's design.Likely an incorrect or invalid review comment.
forester/src/polling/queue_poller.rs (8)
25-32: LGTM! QueueUpdateMessage is well-structured for queue updates.The message type includes all necessary fields: tree and queue pubkeys, queue type, size, and slot. This provides complete context for processing decisions.
34-38: LGTM! Actor state properly encapsulates dependencies.The structure holds the indexer client, registered tree subscriptions, and a shutdown signal. The
Arc<AtomicBool>forpolling_activeis now properly used for coordinated shutdown (as verified in the polling loop).
40-65: LGTM! Actor lifecycle properly implements coordinated shutdown.The
on_startspawns the polling loop with the sharedpolling_activeflag, andon_stopcorrectly usesOrdering::Release(line 62) to synchronize with theOrdering::Acquireloads in the polling loop. This ensures the shutdown signal is visible to the spawned task.
67-76: LGTM! Constructor properly initializes the actor state.The indexer URL is correctly formatted with the
/v1suffix for the Photon API, and the initial state is properly set up.
78-103: LGTM! Queue info polling with appropriate error logging.The method correctly fetches queue info from the indexer and transforms it into internal types. Error logging is at
error!level (line 99), which is appropriate for indexer communication failures.
149-178: LGTM! Double registration properly handled with clear warnings.The handler correctly detects when a tree is already registered (lines 165-171), logs a warning, and explicitly drops the old sender to close the previous receiver. This makes the behavior predictable and debuggable.
180-205: LGTM! Unregistration includes proper validation.The handler checks whether the tree was actually registered (lines 194-203) and logs a warning for mismatches, helping detect potential bugs in registration/unregistration coordination.
222-247: LGTM! PollNow handler includes optimization for empty subscriptions.The early exit at lines 233-236 avoids unnecessary indexer calls when no trees are registered, reducing load on both the poller and the indexer service.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
forester/Cargo.toml (1)
62-62: kameo 0.19 does not exist on crates.ioAs flagged in the previous review, kameo version 0.19 is not published on crates.io. The latest available version is 0.17.2. You'll need to either:
- Use
kameo = "0.17"orkameo = "0.17.2"(latest stable), or- Pin to a Git commit if you require unreleased features:
kameo = { git = "https://github.com/tqwewe/kameo", rev = "..." }sdk-libs/program-test/src/indexer/test_indexer.rs (1)
868-873: Replaceunimplemented!()with a structured errorThe
unimplemented!()macro will panic at runtime, which creates noisy test failures. In a test harness, prefer returning a clear error:async fn get_queue_info( &self, _config: Option<IndexerRpcConfig>, ) -> Result<Response<light_client::indexer::QueueInfoResult>, IndexerError> { Err(IndexerError::Unknown( "get_queue_info is not implemented for TestIndexer".into(), )) }This gives callers an actionable, non-panicking signal.
forester/src/polling/queue_poller.rs (2)
23-76: Consider making the polling interval configurable rather than hard‑coded
POLLING_INTERVAL_SECS: u64 = 1is baked into the actor andpolling_loop. Depending on indexer latency, rate limits, and deployment characteristics, a fixed 1s poll may be either too aggressive or too slow. Threading aDuration(or rawu64) from configuration intoQueueInfoPoller::newand using it ininterval(Duration::from_secs(...))would make this tunable without code changes.Also applies to: 249-280
105-137: Avoid cloning QueueUpdateMessage and consider pruning closed senders
distribute_updatescurrently clones eachQueueUpdateMessagebeforetry_send, and onTrySendError::Closed(_)it logs but leaves the closed sender intree_notifiers. That means:
- Extra per-update allocation/copy via
message.clone().- Future updates for that tree keep hitting a closed channel and re-logging until something explicitly unregisters it.
You can tighten this by (a) moving the message into
try_sendand logging viainfo’s fields, and (b) removing closed senders so you don’t keep trying them:- fn distribute_updates(&self, queue_infos: Vec<QueueInfo>) { - for info in queue_infos { - if let Some(tx) = self.tree_notifiers.get(&info.tree) { + fn distribute_updates(&mut self, queue_infos: Vec<QueueInfo>) { + for info in queue_infos { + if let Some(tx) = self.tree_notifiers.get(&info.tree) { let message = QueueUpdateMessage { tree: info.tree, queue: info.queue, queue_type: info.queue_type, queue_size: info.queue_size, slot: info.slot, }; - match tx.try_send(message.clone()) { + match tx.try_send(message) { Ok(()) => { trace!( "Routed update to tree {}: {} items (type: {:?})", info.tree, - message.queue_size, + info.queue_size, info.queue_type ); } Err(mpsc::error::TrySendError::Full(_)) => { warn!( "Tree {} channel full, dropping update (tree processing slower than updates)", info.tree ); } Err(mpsc::error::TrySendError::Closed(_)) => { - trace!("Tree {} channel closed (task likely finished)", info.tree); + trace!("Tree {} channel closed; removing notifier", info.tree); + self.tree_notifiers.remove(&info.tree); } } } } }This keeps logs meaningful and avoids repeated work for trees whose receivers are gone.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockand included by nonecli/src/commands/test-validator/index.tsis excluded by none and included by nonecli/src/utils/initTestEnv.tsis excluded by none and included by nonecli/src/utils/processPhotonIndexer.tsis excluded by none and included by noneprogram-tests/compressed-token-test/tests/v1.rsis excluded by none and included by noneprogram-tests/system-cpi-v2-test/tests/event.rsis excluded by none and included by nonescripts/devenv/versions.shis excluded by none and included by nonesdk-tests/client-test/tests/light_client.rsis excluded by none and included by none
📒 Files selected for processing (34)
forester/Cargo.toml(1 hunks)forester/build.rs(0 hunks)forester/proto/photon.proto(0 hunks)forester/src/epoch_manager.rs(13 hunks)forester/src/grpc/mod.rs(0 hunks)forester/src/grpc/router.rs(0 hunks)forester/src/lib.rs(1 hunks)forester/src/polling/mod.rs(1 hunks)forester/src/polling/queue_poller.rs(1 hunks)forester/src/work_coordinator.rs(0 hunks)forester/tests/e2e_test.rs(0 hunks)forester/tests/legacy/batched_address_test.rs(0 hunks)forester/tests/legacy/batched_state_async_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_test.rs(0 hunks)forester/tests/legacy/e2e_test.rs(0 hunks)forester/tests/legacy/e2e_v1_test.rs(0 hunks)forester/tests/test_batch_append_spent.rs(0 hunks)forester/tests/test_compressible_ctoken.rs(0 hunks)sdk-libs/client/Cargo.toml(0 hunks)sdk-libs/client/src/indexer/indexer_trait.rs(2 hunks)sdk-libs/client/src/indexer/mod.rs(1 hunks)sdk-libs/client/src/indexer/photon_indexer.rs(1 hunks)sdk-libs/client/src/indexer/types.rs(1 hunks)sdk-libs/client/src/lib.rs(0 hunks)sdk-libs/client/src/local_test_validator.rs(0 hunks)sdk-libs/client/src/rpc/indexer.rs(3 hunks)sdk-libs/photon-api/src/apis/default_api.rs(2 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs(1 hunks)sdk-libs/photon-api/src/models/mod.rs(1 hunks)sdk-libs/program-test/src/indexer/test_indexer.rs(1 hunks)sdk-libs/program-test/src/program_test/indexer.rs(1 hunks)
💤 Files with no reviewable changes (17)
- forester/build.rs
- forester/tests/legacy/e2e_test.rs
- forester/tests/test_compressible_ctoken.rs
- sdk-libs/client/src/local_test_validator.rs
- forester/tests/legacy/batched_state_test.rs
- sdk-libs/client/src/lib.rs
- forester/tests/legacy/e2e_v1_test.rs
- forester/tests/e2e_test.rs
- forester/tests/legacy/batched_state_async_indexer_test.rs
- forester/src/grpc/mod.rs
- forester/tests/legacy/batched_address_test.rs
- sdk-libs/client/Cargo.toml
- forester/tests/legacy/batched_state_indexer_test.rs
- forester/proto/photon.proto
- forester/src/work_coordinator.rs
- forester/src/grpc/router.rs
- forester/tests/test_batch_append_spent.rs
🧰 Additional context used
🧬 Code graph analysis (9)
sdk-libs/client/src/indexer/photon_indexer.rs (5)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/photon-api/src/apis/default_api.rs (1)
get_queue_info_post(1752-1789)sdk-libs/client/src/indexer/base58.rs (1)
decode_base58_to_fixed_array(37-48)program-libs/compressed-account/src/pubkey.rs (1)
new_from_array(79-81)
sdk-libs/client/src/indexer/indexer_trait.rs (4)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/client/src/rpc/indexer.rs (4)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/program-test/src/program_test/indexer.rs (7)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
indexer(382-384)sdk-libs/program-test/src/program_test/rpc.rs (1)
indexer(238-240)sdk-libs/client/src/rpc/client.rs (1)
indexer(684-686)
sdk-libs/program-test/src/indexer/test_indexer.rs (4)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (3)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (1)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)
forester/src/polling/queue_poller.rs (4)
forester/src/epoch_manager.rs (1)
new(138-184)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (2)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
new(27-34)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
🔇 Additional comments (18)
forester/src/lib.rs (1)
13-13: LGTM! Clean module migrationThe shift from
grpc/work_coordinatortopollingaligns well with the PR's architectural refactor toward actor-based queue polling. The change is minimal and focused.forester/src/polling/mod.rs (1)
1-5: LGTM! Clean module structureThis module provides a clean facade for the polling subsystem. The re-exports are well-named and align with the actor-based architecture described in the PR.
sdk-libs/client/src/indexer/mod.rs (1)
20-22: LGTM! Clean public API extensionThe new
QueueInfoandQueueInfoResulttypes are correctly added to the public API surface, maintaining alphabetical ordering and consistency with the existing export pattern.sdk-libs/program-test/src/program_test/indexer.rs (1)
224-234: LGTM! Consistent delegation patternThe implementation correctly follows the established pattern used by all other trait methods in this file, with proper
NotInitializederror handling.sdk-libs/client/src/indexer/indexer_trait.rs (2)
8-8: LGTM! Clean type importThe
QueueInfoResultimport is correctly placed alphabetically among the other type imports.
204-209: LGTM! Well-documented trait methodThe new
get_queue_infomethod is clearly documented and follows the trait's established patterns. The signature is consistent with other indexer methods (async, takes optional config, returns Response wrapper).sdk-libs/photon-api/src/models/mod.rs (1)
325-332: LGTM! Standard API model exportsThe new queue info request/response models follow the established pattern used throughout this file. The module declarations and re-exports are consistent with the existing API surface.
sdk-libs/client/src/indexer/photon_indexer.rs (1)
1740-1798: get_queue_info wiring matches existing indexer patternsThe new method cleanly reuses
retry, respectsIndexerRpcConfig.slot, and correctly decodes tree/queue from base58 intoPubkeybefore constructingQueueInfoResult. The control flow and error handling are consistent with the other Photon indexer methods.sdk-libs/client/src/indexer/types.rs (1)
32-44: QueueInfo and QueueInfoResult shapes look correctStructs mirror the Photon API queue-info payload (tree/queue as
Pubkey, rawqueue_typeandqueue_size, plus aslotwrapper) and fit cleanly into the existingResponse<Context>pattern.sdk-libs/client/src/rpc/indexer.rs (1)
10-12: RPC facade correctly exposes queue elements and queue infoImporting
QueueElementsResult/QueueInfoResultand delegatingget_queue_elementsandget_queue_infothrough the inner indexer with the existingNotInitializedguard keeps the LightClient surface consistent with other RPC methods.Also applies to: 204-226, 228-238
sdk-libs/photon-api/src/apis/default_api.rs (1)
298-305: get_queue_info_post API wrapper is consistent with existing photon-api endpointsThe new
GetQueueInfoPostErrorenum andget_queue_info_postfunction follow the established pattern: same error typing, API-key handling, JSON body serialization, and 2xx vs error-HTTP branching. This should integrate smoothly with callers likePhotonIndexer::get_queue_info.Also applies to: 1752-1789
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
11-47: JSON-RPC 200-response envelope is well-shaped
JsonRpcErrorplusGetQueueInfoPost200Response(with optionalresultanderror) matches the JSON-RPC 2.0 pattern and gives callers access to both success payloads and logical errors without losing thedatafield.sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (1)
11-46: QueueInfo result model aligns with client and poller usage
GetQueueInfoPost200ResponseResultand innerQueueInfocorrectly model the wire format (includingqueueType/queueSizerenames) and provide simple constructors, making the mapping into clientQueueInfostraightforward.sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
11-64: GetQueueInfoPostRequest JSON-RPC model looks correct and ergonomicHaving
JsonrpcandMethoddefault to"2.0"and"getQueueInfo", plusGetQueueInfoParamsas an empty struct, lets callers useDefault::default()for simple requests while still supporting explicit construction vianew.forester/src/epoch_manager.rs (4)
17-17: LGTM: Import changes align with the polling refactor.The imports correctly replace gRPC dependencies with kameo actor types and the new polling module.
Also applies to: 50-50
112-112: LGTM: Struct field updated correctly.The field type change from coordinator to queue_poller with ActorRef is appropriate for the actor-based approach.
130-130: LGTM: Clone implementation updated.The clone correctly handles the new queue_poller field.
1107-1107: LGTM: Function signature updated consistently.The removal of coordinator from instrumentation and addition of
queue_update_rxparameter align with the polling-based approach.Also applies to: 1111-1117
sergeytimoshin
force-pushed
the
sergey/get_queue_info_poll
branch
from
f2549c3 to
2645798
Compare
sergeytimoshin
force-pushed
the
sergey/get_queue_info_poll
branch
from
2645798 to
14c1be7
Compare
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 (2)
forester/src/epoch_manager.rs (2)
953-1044: V2 polling integration inperform_active_workis correct; minor allocation nitThe new logic that:
- computes the number of V2 trees,
- conditionally logs when a poller is available, and
- registers V2 trees with the poller (failing fast when the poller is unavailable or registration fails)
is a big improvement in terms of correctness and observability.
You can avoid an extra allocation by not materializing
v2_treesjust to get its length:- let (_, v2_trees): (Vec<_>, Vec<_>) = epoch_info - .trees - .iter() - .filter(|tree| !should_skip_tree(&self.config, &tree.tree_accounts.tree_type)) - .partition(|tree| { - matches!( - tree.tree_accounts.tree_type, - TreeType::StateV1 | TreeType::AddressV1 - ) - }); - - let queue_poller = self.queue_poller.clone(); - - if queue_poller.is_some() { - info!("Using QueueInfoPoller for {} V2 trees", v2_trees.len()); - } + let v2_tree_count = epoch_info + .trees + .iter() + .filter(|tree| !should_skip_tree(&self.config, &tree.tree_accounts.tree_type)) + .filter(|tree| matches!(tree.tree_accounts.tree_type, TreeType::StateV2 | TreeType::AddressV2)) + .count(); + + let queue_poller = self.queue_poller.clone(); + + if queue_poller.is_some() { + info!("Using QueueInfoPoller for {} V2 trees", v2_tree_count); + }Purely a micro-optimization/readability tweak.
1331-1469: V2 slot processing loop now has bounded wait and channel‑closure handlingThe new
process_light_slot_v2logic addresses the earlier blocking/closure concerns:
- wraps
queue_update_rx.recv()intokio::time::timeout(QUEUE_UPDATE_TIMEOUT),- tracks consecutive timeouts with
MAX_TIMEOUTSand converts prolonged lack of updates into a clear error, and- handles
Ok(None)by logging and breaking out of the inner loop instead of spinning forever.This prevents a hung poller or closed channel from stalling the tree for the whole slot window without feedback. The specific constants (
QUEUE_UPDATE_TIMEOUT = 150ms,MAX_TIMEOUTS = 100) might need tuning in production, but the structure is solid.
♻️ Duplicate comments (3)
sdk-libs/program-test/src/indexer/test_indexer.rs (1)
868-873: Avoidunimplemented!()in TestIndexer::get_queue_infoThis method will currently panic at runtime if hit, which is brittle for a shared program-test harness.
Prefer returning a structured error so callers get a predictable signal, e.g.:
- async fn get_queue_info( - &self, - _config: Option<IndexerRpcConfig>, - ) -> Result<Response<light_client::indexer::QueueInfoResult>, IndexerError> { - unimplemented!("get_queue_info") - } + async fn get_queue_info( + &self, + _config: Option<IndexerRpcConfig>, + ) -> Result<Response<light_client::indexer::QueueInfoResult>, IndexerError> { + Err(IndexerError::NotImplemented( + "get_queue_info is not implemented for TestIndexer".into(), + )) + }This keeps the trait contract while making failures clearer and non-panicking.
sdk-libs/client/src/indexer/photon_indexer.rs (1)
1740-1798: Queue-info RPC integration looks correct; error mapping nuance remainsThe
get_queue_infoimplementation lines up with the rest of PhotonIndexer (retry semantics, slot freshness check, base58→Pubkey decoding, and returningQueueInfoResultwrapped inResponse). Functionally this should behave as intended.One remaining nuance is that the error mapping into
GetBatchAddressUpdateInfoPost200ResponseErrorstill discards any extra fields (likedata) from the original error; that’s acceptable if you don’t rely on them, but worth revisiting if richer diagnostics are needed.forester/src/polling/queue_poller.rs (1)
105-137: Tightendistribute_updatesto avoid clones and prune closed channelsRight now
distribute_updatesdoes an unnecessarymessage.clone()and keepstree_notifiersentries around after receivers have closed, which can lead to minor overhead and repeated “channel closed” logs.You can:
- move the message into
try_sendand log using fields frominfo, and- remove closed senders from
tree_notifiersonTrySendError::Closed(_).For example:
- fn distribute_updates(&self, queue_infos: Vec<QueueInfo>) { + fn distribute_updates(&mut self, queue_infos: Vec<QueueInfo>) { for info in queue_infos { - if let Some(tx) = self.tree_notifiers.get(&info.tree) { - let message = QueueUpdateMessage { - tree: info.tree, - queue: info.queue, - queue_type: info.queue_type, - queue_size: info.queue_size, - slot: info.slot, - }; - - match tx.try_send(message.clone()) { + if let Some(tx) = self.tree_notifiers.get(&info.tree) { + let message = QueueUpdateMessage { + tree: info.tree, + queue: info.queue, + queue_type: info.queue_type, + queue_size: info.queue_size, + slot: info.slot, + }; + + let queue_size = message.queue_size; + let queue_type = message.queue_type; + + match tx.try_send(message) { Ok(()) => { trace!( "Routed update to tree {}: {} items (type: {:?})", info.tree, - message.queue_size, - info.queue_type + queue_size, + queue_type ); } Err(mpsc::error::TrySendError::Full(_)) => { warn!( "Tree {} channel full, dropping update (tree processing slower than updates)", info.tree ); } Err(mpsc::error::TrySendError::Closed(_)) => { - trace!("Tree {} channel closed (task likely finished)", info.tree); + trace!( + "Tree {} channel closed (task likely finished); removing notifier", + info.tree + ); + self.tree_notifiers.remove(&info.tree); } } } } }This keeps the behavior the same while reducing allocations and log spam.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.lockand included by nonecli/src/commands/test-validator/index.tsis excluded by none and included by nonecli/src/utils/initTestEnv.tsis excluded by none and included by nonecli/src/utils/processPhotonIndexer.tsis excluded by none and included by noneprogram-tests/compressed-token-test/tests/v1.rsis excluded by none and included by noneprogram-tests/system-cpi-v2-test/tests/event.rsis excluded by none and included by nonescripts/devenv/versions.shis excluded by none and included by nonesdk-tests/client-test/tests/light_client.rsis excluded by none and included by none
📒 Files selected for processing (34)
forester/Cargo.toml(1 hunks)forester/build.rs(0 hunks)forester/proto/photon.proto(0 hunks)forester/src/epoch_manager.rs(13 hunks)forester/src/grpc/mod.rs(0 hunks)forester/src/grpc/router.rs(0 hunks)forester/src/lib.rs(1 hunks)forester/src/polling/mod.rs(1 hunks)forester/src/polling/queue_poller.rs(1 hunks)forester/src/work_coordinator.rs(0 hunks)forester/tests/e2e_test.rs(0 hunks)forester/tests/legacy/batched_address_test.rs(0 hunks)forester/tests/legacy/batched_state_async_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_indexer_test.rs(0 hunks)forester/tests/legacy/batched_state_test.rs(0 hunks)forester/tests/legacy/e2e_test.rs(0 hunks)forester/tests/legacy/e2e_v1_test.rs(0 hunks)forester/tests/test_batch_append_spent.rs(0 hunks)forester/tests/test_compressible_ctoken.rs(0 hunks)sdk-libs/client/Cargo.toml(0 hunks)sdk-libs/client/src/indexer/indexer_trait.rs(2 hunks)sdk-libs/client/src/indexer/mod.rs(1 hunks)sdk-libs/client/src/indexer/photon_indexer.rs(1 hunks)sdk-libs/client/src/indexer/types.rs(1 hunks)sdk-libs/client/src/lib.rs(0 hunks)sdk-libs/client/src/local_test_validator.rs(0 hunks)sdk-libs/client/src/rpc/indexer.rs(3 hunks)sdk-libs/photon-api/src/apis/default_api.rs(2 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs(1 hunks)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs(1 hunks)sdk-libs/photon-api/src/models/mod.rs(1 hunks)sdk-libs/program-test/src/indexer/test_indexer.rs(1 hunks)sdk-libs/program-test/src/program_test/indexer.rs(1 hunks)
💤 Files with no reviewable changes (17)
- sdk-libs/client/src/local_test_validator.rs
- forester/tests/legacy/e2e_v1_test.rs
- forester/tests/test_batch_append_spent.rs
- forester/src/grpc/mod.rs
- forester/build.rs
- forester/tests/test_compressible_ctoken.rs
- sdk-libs/client/src/lib.rs
- forester/tests/e2e_test.rs
- forester/tests/legacy/batched_address_test.rs
- forester/tests/legacy/batched_state_async_indexer_test.rs
- forester/tests/legacy/e2e_test.rs
- sdk-libs/client/Cargo.toml
- forester/tests/legacy/batched_state_test.rs
- forester/tests/legacy/batched_state_indexer_test.rs
- forester/src/grpc/router.rs
- forester/proto/photon.proto
- forester/src/work_coordinator.rs
🧰 Additional context used
🧬 Code graph analysis (10)
sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (5)
forester/src/epoch_manager.rs (1)
new(138-184)forester/src/polling/queue_poller.rs (1)
new(68-76)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
sdk-libs/program-test/src/indexer/test_indexer.rs (7)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
indexer(382-384)sdk-libs/client/src/rpc/rpc_trait.rs (1)
indexer(199-199)sdk-libs/program-test/src/program_test/rpc.rs (1)
indexer(238-240)
sdk-libs/client/src/rpc/indexer.rs (4)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
sdk-libs/client/src/indexer/photon_indexer.rs (6)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (2)
get_queue_info(868-873)new(1355-1416)sdk-libs/photon-api/src/apis/default_api.rs (1)
get_queue_info_post(1752-1789)sdk-libs/client/src/indexer/base58.rs (1)
decode_base58_to_fixed_array(37-48)program-libs/compressed-account/src/pubkey.rs (1)
new_from_array(79-81)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (4)
forester/src/epoch_manager.rs (1)
new(138-184)forester/src/polling/queue_poller.rs (1)
new(68-76)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)
sdk-libs/program-test/src/program_test/indexer.rs (7)
sdk-libs/client/src/indexer/indexer_trait.rs (1)
get_queue_info(206-209)sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/light_program_test.rs (1)
indexer(382-384)sdk-libs/program-test/src/program_test/rpc.rs (1)
indexer(238-240)sdk-libs/client/src/rpc/client.rs (1)
indexer(684-686)
forester/src/polling/queue_poller.rs (4)
forester/src/epoch_manager.rs (1)
new(138-184)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (1)
new(35-46)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)
forester/src/epoch_manager.rs (1)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (5)
forester/src/epoch_manager.rs (1)
new(138-184)forester/src/polling/queue_poller.rs (1)
new(68-76)sdk-libs/client/src/indexer/photon_indexer.rs (1)
new(115-126)sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
new(32-34)new(38-45)sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (1)
new(27-34)
sdk-libs/client/src/indexer/indexer_trait.rs (4)
sdk-libs/client/src/indexer/photon_indexer.rs (1)
get_queue_info(1740-1798)sdk-libs/client/src/rpc/indexer.rs (1)
get_queue_info(228-238)sdk-libs/program-test/src/indexer/test_indexer.rs (1)
get_queue_info(868-873)sdk-libs/program-test/src/program_test/indexer.rs (1)
get_queue_info(224-234)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: programs (compressed-token-batched-tree, ["cargo-test-sbf -p compressed-token-test -- test_transf...
- GitHub Check: programs (compressed-token-and-e2e, ["cargo-test-sbf -p compressed-token-test --test v1", "cargo-...
- GitHub Check: Test program-libs-slow
- GitHub Check: Test batched-merkle-tree-simulate
- GitHub Check: Test program-libs-fast
🔇 Additional comments (21)
sdk-libs/client/src/indexer/mod.rs (1)
16-23: Expose QueueInfo types from indexer moduleRe-exporting
QueueInfoandQueueInfoResulthere is correct and keeps the public indexer surface coherent with the new APIs.sdk-libs/program-test/src/program_test/indexer.rs (1)
224-234: LightProgramTest delegation for get_queue_info is consistentThe new
get_queue_infomirrors the existing delegation pattern (NotInitialized guard + passthrough), so the program-test layer stays in sync with the trait.sdk-libs/photon-api/src/models/mod.rs (1)
325-332: Queue-info models are wired into photon-api exports correctlyThe new
GetQueueInfoPost*andQueueInfore-exports follow the same pattern as the other REST models; no issues spotted.sdk-libs/client/src/indexer/types.rs (1)
32-44: QueueInfo / QueueInfoResult definitions are straightforward and consistentThe new structs have the right fields and visibility for the queue-info API and align with how
PhotonIndexer::get_queue_infopopulates them.sdk-libs/client/src/indexer/indexer_trait.rs (1)
4-14: New Indexer::get_queue_info API is well-shaped but breaking for external implementersThe
get_queue_infosignature and docs are consistent with the new types and PhotonIndexer implementation, and it belongs on the core Indexer trait.Because this extends a public trait, any downstream Indexer implementations outside this repo will fail to compile until they add this method. If you haven’t already, it’s worth auditing consumers and calling this out in release notes or a changelog.
Also applies to: 204-210
sdk-libs/client/src/rpc/indexer.rs (1)
10-12: RPC layer forwards queue elements and queue info cleanlyUsing the shared
QueueElementsResult/QueueInfoResulttypes and delegating both methods through to the inner indexer with the usual NotInitialized guard keeps the RPC surface consistent with the core Indexer trait.Also applies to: 204-213, 228-238
sdk-libs/photon-api/src/models/_get_queue_info_post_200_response.rs (2)
13-20: LGTM! Standard JSON-RPC 2.0 error structure.The
JsonRpcErrorstruct correctly implements the JSON-RPC 2.0 error object specification with requiredcodeandmessagefields plus optionaldata.
34-47: Constructor only supports success path; error responses require manual construction.The
new()constructor initializeserrortoNone, appropriate for success responses. If you need to construct error responses programmatically, you'll need to use struct initialization or add a separate constructor. This pattern is consistent with other 200 response models in the codebase.sdk-libs/photon-api/src/apis/default_api.rs (2)
298-305: LGTM! Error enum follows established pattern.The
GetQueueInfoPostErrorenum correctly mirrors the structure of other endpoint error types in this file, reusing the commonGetBatchAddressUpdateInfoPost429Responsemodel for HTTP 429 and 500 errors.
1752-1789: LGTM! Implementation consistent with existing endpoints.The
get_queue_info_postfunction follows the exact same pattern as other API methods in this file: builds the request with API key and User-Agent, POSTs JSON, and handles success/error responses appropriately.sdk-libs/photon-api/src/models/_get_queue_info_post_200_response_result.rs (2)
11-35: LGTM! Clean response result structure.The
GetQueueInfoPost200ResponseResultstruct appropriately models the queue info response payload with a vector of queue details and the associated slot number.
19-46: LGTM! Well-structured queue metadata model.The
QueueInfostruct appropriately captures queue metadata. Theu8type forqueue_typesupports up to 256 queue types, which should be sufficient for the foreseeable future.sdk-libs/photon-api/src/models/_get_queue_info_post_request.rs (3)
11-35: LGTM! Standard JSON-RPC 2.0 request structure.The
GetQueueInfoPostRequestcorrectly models a JSON-RPC request with all required fields. The constructor appropriately initializesparamsto default since this endpoint takes no parameters.
37-38: Empty params struct is intentional for JSON-RPC consistency.The
GetQueueInfoParamsempty struct ensures the JSON-RPC request includes aparamsfield (serialized as{}), maintaining protocol consistency even when no parameters are required.
40-64: LGTM! Type-safe enums enforce valid values.The
JsonrpcandMethodenums with single variants provide compile-time guarantees that only valid protocol versions and method names can be used. TheDefaultimplementations enhance ergonomics.forester/src/lib.rs (1)
13-13: Exposepollingmodule in crate rootExporting
pollingalongside the other subsystems makes the new QueueInfoPoller surface available cleanly to the rest of the crate; no issues here.forester/src/polling/mod.rs (1)
1-5: Queue poller façade looks cleanUsing a dedicated
pollingmodule that re-exportsQueueInfoPollerand its message types keeps consumer imports (e.g., inepoch_manager) tidy and hides the submodule layout; no further changes needed.forester/src/polling/queue_poller.rs (1)
68-75: Double‑checkindexer_urlsemantics when appending/v1
QueueInfoPoller::newbuilds the base path asformat!("{}/v1", indexer_url). That’s fine as long as configs treatindexer_urlas a bare host/base (e.g.https://…), but if any existing deployments already include/v1you’ll end up with/v1/v1and broken calls.I’d either:
- document clearly that
indexer_urlmust not include a version path, or- stop appending
/v1here and instead expect the fully versioned base in config.forester/src/epoch_manager.rs (3)
17-18: QueueInfoPoller wiring intoEpochManagerlooks soundStoring an
Option<ActorRef<QueueInfoPoller>>onEpochManagerand instantiating it only whenconfig.external_services.indexer_urlis set is a good separation of concerns, and cloning theActorRefinimpl Cloneensures all manager clones share the same actor.The updated logging (“spawn initiated” vs. “started”) also better reflects the asynchronous nature of actor startup. No changes needed here.
Also applies to: 99-114, 136-184
968-1010: Good: V2 trees now fail fast when queue poller is missing or registration failsIn the V2 branch of
perform_active_work, you now:
- require a
queue_pollerto be present, and- treat
RegisterTreefailures as hard errors instead of silently proceeding withNonechannels.This removes the previous “silent success but no work” failure mode for V2 trees. The early
Err(anyhow!(...))paths here are appropriate given that V2 processing fundamentally depends on queue updates.
1117-1222:process_queue_v2V2 path correctly enforces presence of a queue channelThe refactored
process_queue_v2now:
- uses the same scheduling logic for all tree types, and
- for V2 trees, refuses to process slots if
queue_update_rxisNone, returning an error instead of pretending success.That matches the new contract that V2 processing requires live queue updates; it’s much less surprising operationally than silently skipping work.
| prost-types = "0.14.1" | ||
| tonic-prost = "0.14.2" | ||
| tokio-stream = { version = "0.1", features = ["sync"] } | ||
| kameo = "0.19" |
There was a problem hiding this comment.
🧩 Analysis chain
Validate kameo = "0.19" against your Tokio version and MSRV
kameo 0.19.x pulls in tokio with a ^1.47 requirement and serde 1.0, so this dependency line is reasonable, but you should confirm that:
- your workspace’s Tokio version is compatible with
^1.47, and - your minimum supported Rust version meets kameo’s requirements. (docs.rs)
Once you’re happy with it in CI, consider pinning to a specific patch (e.g. 0.19.2) for reproducibility.
I need to verify the assumptions in the review comment. Let me examine the workspace configuration and check kameo's actual requirements.
🏁 Script executed:
#!/bin/bash
# Check Tokio version in workspace and MSRV settings
echo "=== Checking Tokio dependency in Cargo.toml ==="
rg -n "tokio\s*=" forester/Cargo.toml
echo ""
echo "=== Checking workspace root Cargo.toml ==="
rg -n "tokio\s*=" Cargo.toml
echo ""
echo "=== Looking for MSRV in Cargo.toml files ==="
rg -n "rust-version|MSRV" forester/Cargo.toml
echo ""
echo "=== Looking for rust-toolchain.toml ==="
fd rust-toolchain.toml
echo ""
echo "=== Displaying full forester/Cargo.toml for context ==="
head -70 forester/Cargo.tomlLength of output: 2835
🏁 Script executed:
#!/bin/bash
# Check for workspace root configuration
echo "=== Checking workspace root Cargo.toml for MSRV ==="
head -30 Cargo.tomlLength of output: 121
🏁 Script executed:
#!/bin/bash
# Search for any existing tokio usage to infer version compatibility
echo "=== Searching for tokio usage patterns ==="
rg -n "tokio::" --type rust | head -20Length of output: 1695
🌐 Web query:
kameo 0.19 Rust minimum supported version MSRV requirements tokio
💡 Result:
I couldn't find an explicit, published MSRV for kameo v0.19. The project does declare a rust-version (MSRV) in its Cargo.toml files (per the changelog) and is built on Tokio (kameo is an async actor library using Tokio). If you want the exact MSRV and tokio-min-version for v0.19 I can fetch the Cargo.toml from the v0.19 tag (or the crate on crates.io) and report the exact rust-version and tokio dependency version — tell me which (GitHub tag or crates.io) you'd prefer. Sources: [kameo project listing/docs]. [1][2]
Sources
[1] Kameo crate listing/docs (lib.rs / docs.rs).
[2] Kameo changelog summary (notes that Cargo.toml contains rust-version / MSRV and that Kameo is built on Tokio).
I need to fetch kameo's actual Cargo.toml metadata to validate the review's claims about tokio requirements and MSRV.
🏁 Script executed:
#!/bin/bash
# Fetch kameo 0.19 metadata from crates.io API
echo "=== Fetching kameo 0.19 from crates.io API ==="
curl -s "https://crates.io/api/v1/crates/kameo/0.19.0" | jq '.crate | {name, version, repository, documentation}'
echo ""
echo "=== Checking for kameo in registry index ==="
# Try to get the Cargo.toml directly from GitHub if available
curl -s "https://raw.githubusercontent.com/tqwewe/kameo/refs/tags/v0.19.0/Cargo.toml" 2>/dev/null | head -40 || echo "Not found via GitHub"Length of output: 992
Declare MSRV or verify Rust 1.88.0 compatibility for kameo 0.19
kameo 0.19.0 requires rust-version = "1.88.0", but forester doesn't declare a minimum supported Rust version. You should either:
- Add
rust-version = "1.88.0"to forester/Cargo.toml to match kameo's MSRV, or - Verify your codebase actually supports 1.88.0 and adjust if you target an earlier version.
Additionally, tighten the Tokio constraint from version = "1" to "1.47" at line 41 for clarity (kameo transitively requires it). Consider pinning kameo to a specific patch (e.g., "0.19.2") once you confirm CI stability.
🤖 Prompt for AI Agents
In forester/Cargo.toml around lines 41 and 62: declare the crate MSRV and
tighten Tokio/TOML versions to match kameo's requirements; add rust-version =
"1.88.0" to the Cargo.toml top-level package stanza (or confirm the codebase and
CI actually support an older Rust and document that decision), change the Tokio
dependency from version = "1" to version = "1.47" for clarity, and pin kameo to
a specific patch (e.g., "0.19.2") after verifying CI stability so transitive
MSRV/compatibility is explicit.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.