feat(huntsman): Replace gRPC protocol custom error code with gRPC status. - #359
Conversation
# Conflicts: # components/spider-proto-rust/src/generated/storage.rs # components/spider-proto/storage/storage.proto # components/spider-scheduler/src/storage_client/grpc.rs
…status_to_error convention
…tus_to_error convention
…der into storage-grpc-migration
WalkthroughStorage protobuf contracts now use direct success fields, while the scheduler and execution-manager gRPC clients map tonic status codes into domain errors and decode the updated response shapes. ChangesStorage gRPC contract updates
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}
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-execution-manager/src/client/grpc/liveness.rs`:
- Around line 112-114: The registration response handling in liveness::register
currently accepts registration.execution_manager_id without validating it, so a
zero ID can slip through as a valid ExecutionManagerId. Add the same non-zero
guard used for session_id before constructing the RegistrationResponse in the
registration response path, and reject or return an error when
registration.execution_manager_id is 0 so only a valid ExecutionManagerId::from
value becomes active.
🪄 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: b8758783-8c76-42ba-8b45-389e8e841482
⛔ 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-execution-manager/src/client/grpc/liveness.rscomponents/spider-proto/storage/storage.protocomponents/spider-scheduler/src/storage_client/grpc.rs
| Ok(RegistrationResponse { | ||
| em_id: ExecutionManagerId::from(registration.execution_manager_id), | ||
| session_id: registration.session_id, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether ExecutionManagerId permits zero and whether other decoders reject zero IDs.
fd -e rs . components/spider-core components/spider-execution-manager components/spider-proto-rust \
| xargs rg -n -C3 '\b(type|struct)\s+ExecutionManagerId\b|ExecutionManagerId::from\(|execution_manager_id == 0|NonZero'Repository: y-scope/spider
Length of output: 2752
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ID type and nearby conversions/validations.
sed -n '1,220p' components/spider-core/src/types/id.rs
printf '\n---\n'
sed -n '1,220p' components/spider-proto-rust/src/unpack/storage.rs
printf '\n---\n'
sed -n '1,230p' components/spider-execution-manager/src/client/grpc/liveness.rsRepository: y-scope/spider
Length of output: 17533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where execution-manager IDs are allocated/returned on the server side.
rg -n -C3 'execution_manager_id|ExecutionManagerId::from\(|RegisterExecutionManagerResponse|ExecutionManagerRegistration|update_execution_manager_heartbeat|register_execution_manager' \
components/spider-* --glob '!**/target/**'Repository: y-scope/spider
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how execution-manager IDs are allocated and returned in the storage service.
sed -n '470,530p' components/spider-storage/src/db/mariadb.rs
printf '\n---\n'
sed -n '616,660p' components/spider-storage/src/state/service.rs
printf '\n---\n'
sed -n '1,120p' components/spider-storage/src/grpc.rsRepository: y-scope/spider
Length of output: 8046
Reject zero execution-manager IDs in registration responses. The response already treats session_id == 0 as invalid, and ExecutionManagerId is a plain u64 wrapper with no built-in non-zero guarantee. Add the same guard for registration.execution_manager_id so a malformed response cannot become the active 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, The registration response handling in liveness::register currently
accepts registration.execution_manager_id without validating it, so a zero ID
can slip through as a valid ExecutionManagerId. Add the same non-zero guard used
for session_id before constructing the RegistrationResponse in the registration
response path, and reject or return an error when
registration.execution_manager_id is 0 so only a valid ExecutionManagerId::from
value becomes active.
| @@ -27,7 +27,7 @@ service InboundQueueService { | |||
|
|
|||
| service ResourceGroupManagementService { | |||
| rpc AddResourceGroup(AddResourceGroupRequest) returns (ResourceGroupIdResponse); | |||
| rpc VerifyResourceGroup(VerifyResourceGroupRequest) returns (ResourceGroupOperationResponse); | |||
| rpc VerifyResourceGroup(VerifyResourceGroupRequest) returns (common.Void); | |||
There was a problem hiding this comment.
Since we're using common.Void in scheduler protobuf definitions for void returns already, we should be consistent and avoid defining more void types.
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Please confirm you have read the following comments.
| /// The [`StorageClientError`] for `status`'s code: | ||
| /// | ||
| /// * [`StorageClientError::Transport`] for `UNAVAILABLE` (a network-level failure). | ||
| /// * [`StorageClientError::Transport`] for `UNAVAILABLE` (a lost or unestablished connection). |
There was a problem hiding this comment.
If similar wording already exists in the code base, we'd prefer to be consistent:
Ask your coding agent to follow this rule.
There was a problem hiding this comment.
I have added the instruction. However, I am only sure it will trigger and look for existing convention when I pose a direct question. When running a long task, it seem to skip checking on convention. Maybe I can add to review instruction as well.
There was a problem hiding this comment.
- Yes, adding to the review instruction would be good.
- You should also have a sense for checking existing ones manually.
| /// * [`StorageClientError::InvalidInput`] for `INVALID_ARGUMENT`. | ||
| /// * [`StorageClientError::Server`] for any other code (including `INTERNAL`, which the storage | ||
| /// server sends when the inbound queue is closed). | ||
| /// * [`StorageClientError::Server`] for any other code. |
There was a problem hiding this comment.
This really sounds like the conventional comments claude would add if you explicitly ask for a behavior change, but do you think this from-no-where explanation makes sense in general?
You are expected to check and remove these comments.
| /// * [`StorageClientError::Server`] if the regular-task lane is closed and can no longer yield | ||
| /// entries, or the storage server returns another error. |
There was a problem hiding this comment.
Similar to 79bcb04#r3488578792 but even worse: what is "another error" referring to?
If you want to be general, then just be general. The same file has an example of how to describe a general case:
LinZhihao-723
changed the title
2 participants
Description
This PR:
Checklist
breaking change.
Validation performed
Summary by CodeRabbit