Skip to content

fix(spider-storage): Reserve UNAVAILABLE gRPC status code for network connection errors. - #358

Merged
sitaowang1998 merged 2 commits into
y-scope:mainfrom
LinZhihao-723:grpc-code-fix
Jun 27, 2026
Merged

fix(spider-storage): Reserve UNAVAILABLE gRPC status code for network connection errors.#358
sitaowang1998 merged 2 commits into
y-scope:mainfrom
LinZhihao-723:grpc-code-fix

Conversation

@LinZhihao-723

@LinZhihao-723 LinZhihao-723 commented Jun 26, 2026

Copy link
Copy Markdown
Member

Description

UNAVAILABLE was being returned by user code in two storage handlers, which made it ambiguous: a genuine transport failure (lost or unestablished connection) and an application-level error both surfaced as UNAVAILABLE, so the execution-manager client could not tell them apart. Per the gRPC status-code guidance, only a small set of codes are appropriate for user code to return, and UNAVAILABLE is best left to the transport. This PR remaps the two offending cases onto application-appropriate codes and updates the execution-manager client to treat UNAVAILABLE as a transport error. It closes the follow-up flagged at the end of #354.

Server changes (spider-storage/src/grpc.rs)

  • job_orchestration_service_error_handler: a fatal cache-internal error now returns INTERNAL instead of UNAVAILABLE (it still fires the cancellation token and restarts the service).
  • task_instance_management_service_error_handler: a request from a stale session now returns NOT_FOUND instead of UNAVAILABLE. The other mappings are unchanged.
  • Updated the handler doc comments to match.

Client changes (spider-execution-manager/src/client/grpc/storage.rs)

  • status_to_error now maps Code::Unavailable to StorageResponseError::Transport, so a lost or unestablished connection is reported as a transport failure rather than as an application error.
  • Stale-session detection is remapped to follow the server: Code::NotFoundStorageResponseError::StaleSession. This is required for correctness — without it, the server's new NOT_FOUND stale-session response would fall through to the generic Server case and stale-session handling would silently break.
  • Updated the unit tests: replaced status_maps_unavailable_to_stale_session with status_maps_not_found_to_stale_session, and added status_maps_unavailable_to_transport.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • Ensure all workflows pass.

Summary by CodeRabbit

  • Bug Fixes
    • Improved gRPC error responses for storage-related issues, so some failures now return more appropriate status codes.
    • Session-related errors are now reported more consistently, helping clients distinguish missing, stale, and transport-related problems.
    • Updated error messages to better reflect the actual issue returned to callers.

@LinZhihao-723
LinZhihao-723 requested review from a team and sitaowang1998 as code owners June 26, 2026 21:02

@LinZhihao-723 LinZhihao-723 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sitaowang1998 Please check this PR since it may affect the error handling in the coming service implementations.

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@sitaowang1998, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 59 minutes and 48 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8b09e2dd-f878-456d-b585-46fe25175367

📥 Commits

Reviewing files that changed from the base of the PR and between f314e98 and ef3f167.

📒 Files selected for processing (1)
  • components/spider-execution-manager/src/client/grpc/storage.rs

Walkthrough

gRPC error-code mappings were updated in the storage service and the execution-manager client. The storage service now returns INTERNAL and NOT_FOUND for specific error paths, and the client now maps NOT_FOUND to StaleSession and UNAVAILABLE to Transport.

Changes

gRPC status mapping updates

Layer / File(s) Summary
Storage service handlers
components/spider-storage/src/grpc.rs
job_orchestration_service_error_handler now returns INTERNAL for cache-internal failures, and task_instance_management_service_error_handler now returns NOT_FOUND for stale sessions; the code comments are updated to match.
Execution-manager status mapping and tests
components/spider-execution-manager/src/client/grpc/storage.rs
status_to_error now maps NOT_FOUND to StaleSession and UNAVAILABLE to Transport, and the tests cover the updated mappings and invalid-argument message handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • y-scope/spider#340: Shares the execution-manager gRPC storage client error-code/status mapping changes in components/spider-execution-manager/src/client/grpc/storage.rs.
  • y-scope/spider#354: Also changes status_to_error in the same client file, including the UNAVAILABLE/NOT_FOUND translation logic and tests.

Suggested reviewers

  • sitaowang1998
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately reflects the main change: reserving UNAVAILABLE for transport-level connection errors.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@LinZhihao-723 LinZhihao-723 changed the title fix(spider-storage): Stop overloading UNAVAILABLE so it cleanly signals a lost connection. fix(spider-storage): Reserve UNAVAILABLE gRPC status code for network connection errors. Jun 26, 2026
@sitaowang1998
sitaowang1998 merged commit 7414825 into y-scope:main Jun 27, 2026
12 checks passed
sitaowang1998 added a commit to sitaowang1998/spider that referenced this pull request Jun 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants