Skip to content

fix(rust): return typed errors instead of panicking in Bedrock embedding path#3512

Merged
wjones127 merged 2 commits into
lancedb:mainfrom
Ar-maan05:fix/bedrock-panic-3506
Jun 17, 2026
Merged

fix(rust): return typed errors instead of panicking in Bedrock embedding path#3512
wjones127 merged 2 commits into
lancedb:mainfrom
Ar-maan05:fix/bedrock-panic-3506

Conversation

@Ar-maan05

Copy link
Copy Markdown
Contributor

Closes #3506

Problem

The Bedrock embedding compute path (rust/lancedb/src/embeddings/bedrock.rs) panics instead of returning a typed error in several places:

  • serde_json::to_vec(&request_body).unwrap(): request serialization.
  • block_in_place(...).unwrap(): the AWS invoke_model send result; any API error terminates the worker instead of propagating.
  • v.as_f64().unwrap() as f32: panics on non-numeric values in the returned embedding array.
  • Handle::current() + block_in_place assume a multi-threaded Tokio runtime and panic when that assumption does not hold (no runtime, or a current-thread runtime).

Malformed payloads, non-numeric embedding values, or an incompatible runtime should surface as typed errors and never panic.

Fix

  • Serialize the request body before the blocking section so a serialization failure returns Error::Runtime via ?.
  • Map the invoke_model send error to Error::Runtime instead of unwrap.
  • Add a json_array_to_f32 helper that converts the response array to Vec<f32>, returning Error::Runtime for a missing/non-array field or a non-numeric element (used by both the Titan and Cohere paths).
  • Add current_multi_thread_handle() (Handle::try_current() + a RuntimeFlavor::CurrentThread guard) so an absent or incompatible runtime returns a typed error rather than panicking in block_in_place.

Scope note: the sibling openai.rs provider uses the same block_in_place + block_on bridge, so the bridge pattern itself is kept; this change only removes the panic paths that are specific to the Bedrock provider.

Testing

Added 6 unit tests (no AWS credentials required):

  • json_array_to_f32: valid numbers, non-array payload, and non-numeric element.
  • current_multi_thread_handle: errors with no runtime, errors on a current-thread runtime, and succeeds on a multi-threaded runtime.

All pass; cargo fmt and cargo clippy clean. Build/test with --features bedrock,lance/protoc.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions github-actions Bot added bug Something isn't working Rust Rust related issues labels Jun 6, 2026
@Ar-maan05

Copy link
Copy Markdown
Contributor Author

The two failing NodeJS example jobs are HuggingFace HTTP 429 (rate-limit) errors while downloading all-MiniLM-L6-v2 model files; unrelated to this change, which only touches the feature-gated Rust bedrock module (nodejs doesn't build it). A re-run of the failed jobs should clear them.

@Ar-maan05

Copy link
Copy Markdown
Contributor Author

Friendly ping! Let me know if any changes are needed.

@Ar-maan05

Copy link
Copy Markdown
Contributor Author

Hi, friendly ping, please let me know if any changes are needed.

@wjones127 wjones127 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This all looks good to me. It looks like you might need to rebase this PR before this will be passing tests and ready to merge.

@Ar-maan05
Ar-maan05 force-pushed the fix/bedrock-panic-3506 branch from a807f5d to 544909d Compare June 17, 2026 21:33
@Ar-maan05

Copy link
Copy Markdown
Contributor Author

@wjones127 rebased onto main, CI should be green now. Thanks for the review! I've really enjoyed contributing here, happy to pick up other open issues if any would be helpful.

@wjones127
wjones127 merged commit 1f8ebef into lancedb:main Jun 17, 2026
25 checks passed
@Ar-maan05
Ar-maan05 deleted the fix/bedrock-panic-3506 branch June 19, 2026 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Rust Rust related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(rust): Bedrock embedding path panics on malformed payload and runtime blocking assumptions

2 participants