feat(spider-huntsman): Replace custom error code with gRPC status. - #357
Closed
sitaowang1998 wants to merge 3 commits into
Closed
feat(spider-huntsman): Replace custom error code with gRPC status.#357sitaowang1998 wants to merge 3 commits into
sitaowang1998 wants to merge 3 commits into
Conversation
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughStorage and scheduler gRPC contracts now use direct payload fields, request unpackers and scheduler conversions were added, storage service RPCs and shared status helpers were implemented, and the execution-manager and scheduler clients were updated for the new response shapes and tonic status mappings. ChangesStorage and scheduler gRPC alignment
Sequence Diagram(s)Ready-task polling flow sequenceDiagram
participant InboundQueueService
participant GrpcServiceState
participant build_ready_tasks
InboundQueueService->>GrpcServiceState: poll_ready_tasks / poll_ready_commit_tasks / poll_ready_cleanup_tasks
GrpcServiceState->>build_ready_tasks: ReadyQueueEntry batches and task ids
build_ready_tasks-->>GrpcServiceState: storage::ReadyTasks
GrpcServiceState-->>InboundQueueService: PollReadyTasksResponse{tasks}
Execution-manager liveness response parsing sequenceDiagram
participant GrpcLivenessClient
participant map_liveness_status
participant register_response_to_result
participant heartbeat_response_to_result
GrpcLivenessClient->>map_liveness_status: tonic::Status
map_liveness_status-->>GrpcLivenessClient: mapped LivenessResponseError
GrpcLivenessClient->>register_response_to_result: RegisterExecutionManagerResponse
register_response_to_result-->>GrpcLivenessClient: RegistrationResponse
GrpcLivenessClient->>heartbeat_response_to_result: UpdateExecutionManagerHeartbeatResponse
heartbeat_response_to_result-->>GrpcLivenessClient: session_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/spider-execution-manager/src/client/grpc/liveness.rs`:
- Around line 112-114: Add a zero-value guard for execution_manager_id in the
registration handling path so it is validated before constructing the
RegistrationResponse. In the liveness client logic where RegistrationResponse is
built, mirror the existing session_id check by rejecting or handling a zero
execution_manager_id before calling
ExecutionManagerId::from(registration.execution_manager_id), ensuring invalid
sentinel values do not enter runtime state.
In `@components/spider-proto-rust/src/unpack/storage.rs`:
- Around line 209-220: Reject port 0 during scheduler registration in
RegisterSchedulerRequest::unpack, not just values outside u16. After parsing
self.port in the unpack implementation, add a validation that returns
invalid_argument when the port is 0, while still allowing only valid nonzero u16
ports to be returned from this method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 97aa9cbb-034f-48fd-9a2d-bae74ebbba5a
⛔ Files ignored due to path filters (1)
components/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (10)
components/spider-execution-manager/src/client/grpc/liveness.rscomponents/spider-proto-rust/src/lib.rscomponents/spider-proto-rust/src/scheduler.rscomponents/spider-proto-rust/src/unpack/storage.rscomponents/spider-proto/storage/storage.protocomponents/spider-scheduler/src/error.rscomponents/spider-scheduler/src/storage_client/grpc.rscomponents/spider-storage/src/grpc.rscomponents/spider-storage/src/state.rscomponents/spider-storage/src/state/test_utils.rs
💤 Files with no reviewable changes (1)
- components/spider-scheduler/src/error.rs
Comment on lines
112
to
114
| Ok(RegistrationResponse { | ||
| em_id: ExecutionManagerId::from(registration.execution_manager_id), | ||
| session_id: registration.session_id, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify whether ExecutionManagerId permits zero and whether storage can emit zero.
# Expectation: If ExecutionManagerId is non-zero/reserved-zero, add a zero guard before Line 113.
set -euo pipefail
echo "ExecutionManagerId definition and constructors:"
rg -n -C4 'ExecutionManagerId|struct .*ExecutionManager|type .*ExecutionManager|impl .*ExecutionManager' components/spider-core components 2>/dev/null || true
echo
echo "ExecutionManagerRegistration construction sites:"
rg -n -C4 'ExecutionManagerRegistration|execution_manager_id:' components 2>/dev/null || trueRepository: y-scope/spider
Length of output: 50371
Add a zero-value guard for execution_manager_id
The session_id is guarded against zero, but execution_manager_id is converted directly without validation. The storage layer and system protocol require valid ExecutionManagerId values to be non-zero (as enforced by tests in components/spider-storage/src/grpc.rs), where zero serves as a sentinel for null or error states. A direct conversion without a check risks propagating an invalid identifier into the runtime state.
Context
if registration.session_id == 0 {
return Err(LivenessResponseError::Transport(
"register execution manager response carried a zero session id".to_owned(),
));
}
+ if registration.execution_manager_id == 0 {
+ return Err(LivenessResponseError::Transport(
+ "register execution manager response carried a zero execution manager id".to_owned(),
+ ));
+ }
Ok(RegistrationResponse {
em_id: ExecutionManagerId::from(registration.execution_manager_id),
session_id: registration.session_id,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Ok(RegistrationResponse { | |
| em_id: ExecutionManagerId::from(registration.execution_manager_id), | |
| session_id: registration.session_id, | |
| if registration.session_id == 0 { | |
| return Err(LivenessResponseError::Transport( | |
| "register execution manager response carried a zero session id".to_owned(), | |
| )); | |
| } | |
| if registration.execution_manager_id == 0 { | |
| return Err(LivenessResponseError::Transport( | |
| "register execution manager response carried a zero execution manager id".to_owned(), | |
| )); | |
| } | |
| Ok(RegistrationResponse { | |
| em_id: ExecutionManagerId::from(registration.execution_manager_id), | |
| session_id: registration.session_id, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/spider-execution-manager/src/client/grpc/liveness.rs` around lines
112 - 114, Add a zero-value guard for execution_manager_id in the registration
handling path so it is validated before constructing the RegistrationResponse.
In the liveness client logic where RegistrationResponse is built, mirror the
existing session_id check by rejecting or handling a zero execution_manager_id
before calling ExecutionManagerId::from(registration.execution_manager_id),
ensuring invalid sentinel values do not enter runtime state.
Comment on lines
+209
to
+220
| impl RequestUnpack for RegisterSchedulerRequest { | ||
| type Unpacked = (IpAddr, u16); | ||
|
|
||
| fn unpack(self) -> Result<Self::Unpacked, UnpackError> { | ||
| let ip_address = self | ||
| .ip_address | ||
| .parse::<IpAddr>() | ||
| .map_err(|error| invalid_argument(format!("invalid IP address: {error}")))?; | ||
| let port = u16::try_from(self.port) | ||
| .map_err(|_| invalid_argument(format!("port does not fit in `u16`: {}", self.port)))?; | ||
| Ok((ip_address, port)) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject port == 0 during scheduler registration.
Line 217 only checks that the value fits in u16. 0 still passes, gets persisted, and is later returned by get_schedulers, which leaves callers with an unusable endpoint.
Suggested fix
fn unpack(self) -> Result<Self::Unpacked, UnpackError> {
let ip_address = self
.ip_address
.parse::<IpAddr>()
.map_err(|error| invalid_argument(format!("invalid IP address: {error}")))?;
let port = u16::try_from(self.port)
.map_err(|_| invalid_argument(format!("port does not fit in `u16`: {}", self.port)))?;
+ if port == 0 {
+ return Err(invalid_argument("port must be non-zero".to_owned()));
+ }
Ok((ip_address, port))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl RequestUnpack for RegisterSchedulerRequest { | |
| type Unpacked = (IpAddr, u16); | |
| fn unpack(self) -> Result<Self::Unpacked, UnpackError> { | |
| let ip_address = self | |
| .ip_address | |
| .parse::<IpAddr>() | |
| .map_err(|error| invalid_argument(format!("invalid IP address: {error}")))?; | |
| let port = u16::try_from(self.port) | |
| .map_err(|_| invalid_argument(format!("port does not fit in `u16`: {}", self.port)))?; | |
| Ok((ip_address, port)) | |
| } | |
| impl RequestUnpack for RegisterSchedulerRequest { | |
| type Unpacked = (IpAddr, u16); | |
| fn unpack(self) -> Result<Self::Unpacked, UnpackError> { | |
| let ip_address = self | |
| .ip_address | |
| .parse::<IpAddr>() | |
| .map_err(|error| invalid_argument(format!("invalid IP address: {error}")))?; | |
| let port = u16::try_from(self.port) | |
| .map_err(|_| invalid_argument(format!("port does not fit in `u16`: {}", self.port)))?; | |
| if port == 0 { | |
| return Err(invalid_argument("port must be non-zero".to_owned())); | |
| } | |
| Ok((ip_address, port)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/spider-proto-rust/src/unpack/storage.rs` around lines 209 - 220,
Reject port 0 during scheduler registration in RegisterSchedulerRequest::unpack,
not just values outside u16. After parsing self.port in the unpack
implementation, add a validation that returns invalid_argument when the port is
0, while still allowing only valid nonzero u16 ports to be returned from this
method.
# Conflicts: # components/spider-storage/src/grpc.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/spider-storage/src/grpc.rs`:
- Around line 561-569: The serialization path in `JobOutputsResponse` currently
wraps `SerializedTaskOutputs::serialize_with_size_hint()` errors as
`StorageServerError::Serde`, which then falls into the generic handler and
cancels the service. Update `job_orchestration_service_error_handler` so `Serde`
is handled explicitly as a non-fatal `INTERNAL` response, and ensure only truly
unrecoverable errors reach the branch that calls
`self.cancellation_token.cancel()`. Use the existing `StorageServerError` enum
and `job_orchestration_service_error_handler` logic to keep request-scoped
serialization failures from taking down the storage service.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 325c97d5-4ee0-48cc-9cbe-939f4ddf6036
⛔ Files ignored due to path filters (1)
components/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (3)
components/spider-proto-rust/src/unpack/storage.rscomponents/spider-proto/storage/storage.protocomponents/spider-storage/src/grpc.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- components/spider-proto-rust/src/unpack/storage.rs
- components/spider-proto/storage/storage.proto
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/spider-storage/src/grpc.rs`:
- Around line 561-569: The serialization path in `JobOutputsResponse` currently
wraps `SerializedTaskOutputs::serialize_with_size_hint()` errors as
`StorageServerError::Serde`, which then falls into the generic handler and
cancels the service. Update `job_orchestration_service_error_handler` so `Serde`
is handled explicitly as a non-fatal `INTERNAL` response, and ensure only truly
unrecoverable errors reach the branch that calls
`self.cancellation_token.cancel()`. Use the existing `StorageServerError` enum
and `job_orchestration_service_error_handler` logic to keep request-scoped
serialization failures from taking down the storage service.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 325c97d5-4ee0-48cc-9cbe-939f4ddf6036
⛔ Files ignored due to path filters (1)
components/spider-proto-rust/src/generated/storage.rsis excluded by!**/generated/**
📒 Files selected for processing (3)
components/spider-proto-rust/src/unpack/storage.rscomponents/spider-proto/storage/storage.protocomponents/spider-storage/src/grpc.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- components/spider-proto-rust/src/unpack/storage.rs
- components/spider-proto/storage/storage.proto
🛑 Comments failed to post (1)
components/spider-storage/src/grpc.rs (1)
561-569: 🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find the StorageServerError enum definition and any Serde handling arms. rg -nP --type=rust -C2 '\bSerde\b' components/spider-storage/src ast-grep run --pattern 'StorageServerError::Serde($_)' --lang rust components/spider-storage/srcRepository: y-scope/spider
Length of output: 1080
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the error type and the relevant handler arms in the gRPC service. sed -n '1,120p' components/spider-storage/src/state/error.rs printf '\n--- grpc handlers ---\n' sed -n '430,490p' components/spider-storage/src/grpc.rs printf '\n--- job outputs path ---\n' sed -n '548,570p' components/spider-storage/src/grpc.rsRepository: y-scope/spider
Length of output: 3934
Don’t cancel the storage service on
Serdefailures
SerializedTaskOutputs::serialize_with_size_hint()failures are wrapped asStorageServerError::Serde, but that variant has no dedicated handler and falls through to the generic catch-all that callsself.cancellation_token.cancel(). This turns a request-scoped serialization error into a service-wide outage. MapSerdeto a non-fatalINTERNALresponse instead of treating it as unrecoverable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/spider-storage/src/grpc.rs` around lines 561 - 569, The serialization path in `JobOutputsResponse` currently wraps `SerializedTaskOutputs::serialize_with_size_hint()` errors as `StorageServerError::Serde`, which then falls into the generic handler and cancels the service. Update `job_orchestration_service_error_handler` so `Serde` is handled explicitly as a non-fatal `INTERNAL` response, and ensure only truly unrecoverable errors reach the branch that calls `self.cancellation_token.cancel()`. Use the existing `StorageServerError` enum and `job_orchestration_service_error_handler` logic to keep request-scoped serialization failures from taking down the storage service.
Collaborator
Author
|
Closed as will further split into two PRs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR:
Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes