-
Notifications
You must be signed in to change notification settings - Fork 761
fix: handle error from bedrock batch status check #1369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Description🔄 What Changed
🔍 Impact of the Change
📁 Total Files Changed
🧪 Test Added
🔒Security Vulnerabilities
Motivation
Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Related IssuesN/A Tip Quality Recommendations
Tanka Poem ♫
Sequence DiagramsequenceDiagram
participant Client
participant Gateway as Gateway (getBatchOutput)
participant BedrockAPI as Bedrock API
Note over Gateway: getModelProvider(modelId) updated to include 'amazon' -> 'titan'
Client->>Gateway: requestBatchOutput(batchId, modelId)
Gateway->>BedrockAPI: GET /batch/status (batchId)
alt Bedrock API Response is NOT OK
BedrockAPI-->>Gateway: Error Response (status, errorText)
Gateway->>Gateway: Parse errorText into structured error
Gateway-->>Client: Error Response (status, {error, provider: BEDROCK})
else Bedrock API Response is OK
BedrockAPI-->>Gateway: Success Response (batchDetails)
Gateway->>Gateway: Extract outputFileId from batchDetails
Gateway-->>Client: Batch Output (outputFileId)
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling added for bedrock batch status check. Minor improvements suggested for error structure and provider mapping.
const error = await retrieveBatchesResponse.text(); | ||
const _error = { | ||
message: error, | ||
param: null, | ||
type: null, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Security/Data Exposure
Issue: Error response includes raw error text which may expose internal details
Fix: Sanitize error message or use a generic message for production
Impact: Prevents leaking internal system details to clients
const error = await retrieveBatchesResponse.text(); | |
const _error = { | |
message: error, | |
param: null, | |
type: null, | |
}; | |
const error = await retrieveBatchesResponse.text(); | |
const _error = { | |
message: 'Batch retrieval failed', | |
param: null, | |
type: null, | |
}; |
type: null, | ||
}; | ||
return new Response( | ||
JSON.stringify({ error: _error, provider: BEDROCK }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please follow the exisitng format for provider errors
Bedrock error: {errorMessage}
you can use the generateErrorResponse()
utility present in /providers/utils
Description
Handle error from bedrock and return the error for batch output
Motivation
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues