Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 57 additions & 63 deletions core/providers/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,84 +96,78 @@ func getLogger() schemas.Logger {

var UnsupportedSpeechStreamModels = []string{"tts-1", "tts-1-hd"}

// MakeRequestWithContext makes a request with a context and returns the latency and error.
// IMPORTANT: This function does NOT truly cancel the underlying fasthttp network request if the
// context is done. The fasthttp client call will continue in its goroutine until it completes
// or times out based on its own settings. This function merely stops *waiting* for the
// fasthttp call and returns an error related to the context.
// MakeRequestWithContext makes a request with a context deadline and returns the latency and error.
// When the context carries a deadline, DoDeadline is used to avoid goroutine leaks and
// use-after-free on context cancellation. When no deadline is set (e.g. long-running
// requests), it falls back to client.Do which respects the client's own timeout settings.
// Returns the request latency and any error that occurred.
func MakeRequestWithContext(ctx context.Context, client *fasthttp.Client, req *fasthttp.Request, resp *fasthttp.Response) (time.Duration, *schemas.BifrostError) {
startTime := time.Now()
errChan := make(chan error, 1)

go func() {
// client.Do is a blocking call.
// It will send an error (or nil for success) to errChan when it completes.
errChan <- client.Do(req, resp)
}()
var err error
deadline, ok := ctx.Deadline()
if ok {
err = client.DoDeadline(req, resp, deadline)
} else {
err = client.Do(req, resp)
}
latency := time.Since(startTime)

select {
case <-ctx.Done():
// Context was cancelled (e.g., deadline exceeded or manual cancellation).
// Calculate latency even for cancelled requests
latency := time.Since(startTime)
return latency, &schemas.BifrostError{
IsBifrostError: true,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: fmt.Sprintf("Request cancelled or timed out by context: %v", ctx.Err()),
Error: ctx.Err(),
},
}
case err := <-errChan:
// The fasthttp.Do call completed.
// Calculate latency for both successful and failed requests
latency := time.Since(startTime)
if err != nil {
if errors.Is(err, context.Canceled) {
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: schemas.ErrRequestCancelled,
Error: err,
},
}
}
// Check for timeout errors first before checking net.OpError to avoid misclassification
if errors.Is(err, fasthttp.ErrTimeout) || errors.Is(err, context.DeadlineExceeded) {
return latency, NewBifrostOperationError(schemas.ErrProviderRequestTimedOut, err, "")
}
// Check if error implements net.Error and has Timeout() == true
var netErr net.Error
if errors.As(err, &netErr) && netErr.Timeout() {
return latency, NewBifrostOperationError(schemas.ErrProviderRequestTimedOut, err, "")
if err != nil {
// Check if the context was cancelled while the request was in flight
if ctx.Err() != nil {
return latency, &schemas.BifrostError{
IsBifrostError: true,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: fmt.Sprintf("request cancelled or timed out by context: %v", ctx.Err()),
Error: ctx.Err(),
},
}
// Check for DNS lookup and network errors after timeout checks
var opErr *net.OpError
var dnsErr *net.DNSError
if errors.As(err, &opErr) || errors.As(err, &dnsErr) {
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Message: schemas.ErrProviderNetworkError,
Error: err,
},
}
}
if errors.Is(err, context.Canceled) {
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: schemas.ErrRequestCancelled,
Error: err,
},
}
// The HTTP request itself failed (e.g., connection error, fasthttp timeout).
}
Comment on lines +116 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Potential redundancy and inconsistent IsBifrostError values.

Two observations:

  1. Redundant cancellation check: If ctx.Err() != nil (line 127) returns a RequestCancelled error, the subsequent errors.Is(err, context.Canceled) check (line 137) may be unreachable for the same scenario. However, there's a subtle difference: ctx.Err() catches context cancellation that happened during the request, while errors.Is(err, context.Canceled) catches when fasthttp itself returns context.Canceled (e.g., from a wrapped context). If this distinction is intentional, consider adding a comment to clarify.

  2. IsBifrostError inconsistency: Line 129 sets IsBifrostError: true while line 139 sets IsBifrostError: false for similar cancellation scenarios. Per the BifrostError semantics and the orchestrator's retry logic in executeRequestWithRetries, this affects how errors are processed. Verify this is intentional—typically cancellation errors should have consistent classification.

  3. Error message capitalization: Line 132's message "Request cancelled or timed out by context: %v" starts with uppercase. Go convention requires error strings to be lowercase with no trailing punctuation.

Suggested fix for error message capitalization
 			Error: &schemas.ErrorField{
 				Type:    schemas.Ptr(schemas.RequestCancelled),
-				Message: fmt.Sprintf("Request cancelled or timed out by context: %v", ctx.Err()),
+				Message: fmt.Sprintf("request cancelled or timed out by context: %v", ctx.Err()),
 				Error:   ctx.Err(),
 			},

As per coding guidelines: "Error strings must be lowercase with no trailing punctuation per Go conventions."

📝 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
if err != nil {
// Check if the context was cancelled while the request was in flight
if ctx.Err() != nil {
return latency, &schemas.BifrostError{
IsBifrostError: true,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: fmt.Sprintf("Request cancelled or timed out by context: %v", ctx.Err()),
Error: ctx.Err(),
},
}
// Check for DNS lookup and network errors after timeout checks
var opErr *net.OpError
var dnsErr *net.DNSError
if errors.As(err, &opErr) || errors.As(err, &dnsErr) {
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Message: schemas.ErrProviderNetworkError,
Error: err,
},
}
}
if errors.Is(err, context.Canceled) {
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: schemas.ErrRequestCancelled,
Error: err,
},
}
// The HTTP request itself failed (e.g., connection error, fasthttp timeout).
}
if err != nil {
// Check if the context was cancelled while the request was in flight
if ctx.Err() != nil {
return latency, &schemas.BifrostError{
IsBifrostError: true,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: fmt.Sprintf("request cancelled or timed out by context: %v", ctx.Err()),
Error: ctx.Err(),
},
}
}
if errors.Is(err, context.Canceled) {
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Type: schemas.Ptr(schemas.RequestCancelled),
Message: schemas.ErrRequestCancelled,
Error: err,
},
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@core/providers/utils/utils.go` around lines 125 - 146, The two cancellation
pathways in the error handling (the ctx.Err() branch and the errors.Is(err,
context.Canceled) branch) are confusing and set differing IsBifrostError values;
either consolidate into a single canonical cancellation handling or add a
clarifying comment and ensure both branches set the same IsBifrostError value
consistent with orchestrator retry semantics (see BifrostError and
executeRequestWithRetries); also change the message built with
fmt.Sprintf("Request cancelled or timed out by context: %v", ctx.Err()) to a
lowercase, punctuation-free error string per Go conventions (e.g., "request
cancelled or timed out by context: %v").

// Check for timeout errors first before checking net.OpError to avoid misclassification
if errors.Is(err, fasthttp.ErrTimeout) || errors.Is(err, context.DeadlineExceeded) {
return latency, NewBifrostOperationError(schemas.ErrProviderRequestTimedOut, err, "")
}
// Check if error implements net.Error and has Timeout() == true
var netErr net.Error
if errors.As(err, &netErr) && netErr.Timeout() {
return latency, NewBifrostOperationError(schemas.ErrProviderRequestTimedOut, err, "")
}
// Check for DNS lookup and network errors after timeout checks
var opErr *net.OpError
var dnsErr *net.DNSError
if errors.As(err, &opErr) || errors.As(err, &dnsErr) {
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Message: schemas.ErrProviderDoRequest,
Message: schemas.ErrProviderNetworkError,
Error: err,
},
}
}
// HTTP request was successful from fasthttp's perspective (err is nil).
// The caller should check resp.StatusCode() for HTTP-level errors (4xx, 5xx).
return latency, nil
// The HTTP request itself failed (e.g., connection error, fasthttp timeout).
return latency, &schemas.BifrostError{
IsBifrostError: false,
Error: &schemas.ErrorField{
Message: schemas.ErrProviderDoRequest,
Error: err,
},
}
}
// HTTP request was successful from fasthttp's perspective (err is nil).
// The caller should check resp.StatusCode() for HTTP-level errors (4xx, 5xx).
return latency, nil
}

// Deprecated: ConfigureRetry is now handled internally by ConfigureDialer.
Expand Down