refactor: split get_requests_to_attempt in queue.rs

[kevindeforth]

Part of #3070

[claude]

Pull request overview

Pure refactor of crates/node/src/requests/queue.rs. The previously large get_requests_to_attempt body is split into focused helpers on QueuedRequest (update_next_check_due, has_active_attempt, is_older_than, process) and on ComputationProgress (update_computation_progress). Control flow is expressed via new RequestStatusResult and RemovalReason enums, with metric_label() centralizing the MPC_CLUSTER_FAILED_SIGNATURES_COUNT label mapping. Outward behavior is preserved.

Changes

• Extract per-request processing into QueuedRequest::process returning a RequestStatusResult instead of an open-coded loop with continues.
• Extract attempts/back-off bookkeeping into ComputationProgress::update_computation_progress.
• Introduce RemovalReason enum centralizing the failure-removal metric label and tracing reason.
• Precompute cutoff_block once outside the per-request loop instead of recomputing inline with Add.

Reviewed changes
crates/node/src/requests/queue.rs — Split get_requests_to_attempt into helpers; add RequestStatusResult and RemovalReason enums; precompute cutoff_block; drop unused std::ops::Add import.

Findings

Verified behavior matches the original on every branch:

• Cutoff arithmetic: maximum_height.saturating_sub(REQUEST_EXPIRATION_BLOCKS) + 1 is equivalent to the previous ...saturating_sub(...).add(1). The boundary tests should_not_expire_request_at_expiration_boundary and should_expire_request_past_expiration_boundary still pin the off-by-one correctly.
• update_next_check_due bumps the timer iff next_check_due <= now and runs before has_active_attempt, matching the original ordering (so an active attempt still defers the next check by CHECK_EACH_REQUEST_INTERVAL).
• progress.selected_leader = Some(leader) is set under the canonical + eligible-leader path only, exactly as before; the mutex is released on every RequestStatusResult return.
• Metric labels (max_tries_exceeded, block_not_found, timeout) and the signature-only gating are preserved via RemovalReason::metric_label.

Non-blocking nits

• crates/node/src/requests/queue.rs:586 — some Wait reason strings (pending computation, request is not on canonical chain) are slightly less descriptive than the original log lines (e.g. lost the last-response-submitted-too-recently hint). Not a regression. On the upside, the new path now logs no-eligible-leaders and we-are-not-leader, where the original was silent.
• crates/node/src/requests/queue.rs:570 — cutoff_block = ... + 1 would overflow only if maximum_height == u64::MAX, matching the prior add(1); saturating_add(1) would close that purely-theoretical gap.

Approved.

[Copilot]

Pull request overview

This PR refactors PendingRequests::get_requests_to_attempt in crates/node/src/requests/queue.rs by extracting per-request decision logic into QueuedRequest::process and introducing small helper enums, improving readability and making individual decision paths more explicit (as part of the broader work in #3070).

Changes:

• Extracted request evaluation logic into QueuedRequest::process and small helper methods (update_next_check_due, has_active_attempt, is_older_than).
• Introduced RemovalReason/RequestStatusResult to centralize removal/skip/attempt outcomes and metric label selection.
• Simplified cutoff-block computation and removed an unused std::ops::Add import.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gilcu3]

Thank you!

[barakeinav1]

LGTM