fix: fix the proxyURL is empty, not using the default HTTP client configuration && the AWS calling side did not apply the relay timeout. - #2580
Conversation
…figuration && the AWS calling side did not apply the relay timeout.
WalkthroughThe changes introduce context-based timeout management for AWS API invocations and improve HTTP client reuse in proxy configurations. A new helper function creates request-scoped contexts with configurable timeouts, replacing direct use of request contexts across AWS handlers. Additionally, HTTP client selection logic is updated to promote reuse of existing client instances when proxies are unavailable. Changes
Sequence Diagram(s)sequenceDiagram
actor Handler as AWS Handler
participant ContextMgr as Context Manager
participant SDK as AWS SDK
participant Cleanup as Cleanup
Handler->>ContextMgr: newAwsInvokeContext()
ContextMgr->>ContextMgr: Create context with timeout<br/>(RelayTimeout or Background)
ContextMgr-->>Handler: Return (ctx, cancel)
rect rgb(200, 220, 240)
Note over Handler,SDK: AWS Invocation Phase
Handler->>SDK: InvokeModel(ctx, awsReq)
SDK-->>Handler: Response/Stream
end
Handler->>Cleanup: defer cancel()
Cleanup->>Cleanup: Cancel context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
Fix all issues with AI Agents 🤖
In @relay/channel/aws/relay-aws.go:
- Around line 42-47: newAwsInvokeContext currently uses context.Background()
which prevents request cancellation from propagating; change its signature to
accept the request context (e.g., newAwsInvokeContext(c *gin.Context) or
newAwsInvokeContext(ctx context.Context)), create the child context from the
incoming request context (use c.Request.Context() or the passed ctx) and apply
the timeout only if common.RelayTimeout > 0, returning the derived context and
cancel func; then update all call sites that invoked newAwsInvokeContext (the
three places noted) to pass the gin context variable (c) or the request context
so cancellations from the client propagate into AWS SDK calls.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
relay/channel/aws/relay-aws.goservice/http_client.go
🧰 Additional context used
🧬 Code graph analysis (1)
relay/channel/aws/relay-aws.go (1)
common/constants.go (1)
RelayTimeout(122-122)
🔇 Additional comments (2)
service/http_client.go (1)
85-87: Good improvement to client reuse.The change correctly prioritizes the configured
httpClient(which includes timeouts, connection pools, and redirect checks fromInitHttpClient()) over the barehttp.DefaultClient. This ensures consistent behavior when no proxy is specified.relay/channel/aws/relay-aws.go (1)
129-129: Good fix: Ensures AwsReq is set for Nova model path.This change makes the Nova model path consistent with other code paths (lines 147, 159) by properly setting
a.AwsReqafter constructing the request.
| func newAwsInvokeContext() (context.Context, context.CancelFunc) { | ||
| if common.RelayTimeout <= 0 { | ||
| return context.Background(), func() {} | ||
| } | ||
| return context.WithTimeout(context.Background(), time.Duration(common.RelayTimeout)*time.Second) | ||
| } |
There was a problem hiding this comment.
Critical: Context should inherit from request context.
The function creates a new context from context.Background(), which breaks request cancellation propagation. When a client disconnects or cancels their request, the AWS SDK calls will continue executing, leading to:
- Wasted AWS API invocations and costs
- Resource leaks (uncancelled network connections)
- Delayed error responses
🔎 Proposed fix to inherit from request context
The context should be created from the HTTP request context to properly propagate cancellations:
-func newAwsInvokeContext() (context.Context, context.CancelFunc) {
+func newAwsInvokeContext(c *gin.Context) (context.Context, context.CancelFunc) {
+ reqCtx := c.Request.Context()
if common.RelayTimeout <= 0 {
- return context.Background(), func() {}
+ return reqCtx, func() {}
}
- return context.WithTimeout(context.Background(), time.Duration(common.RelayTimeout)*time.Second)
+ return context.WithTimeout(reqCtx, time.Duration(common.RelayTimeout)*time.Second)
}Then update all call sites to pass the gin context:
- Line 214:
ctx, cancel := newAwsInvokeContext(c) - Line 244:
ctx, cancel := newAwsInvokeContext(c) - Line 287:
ctx, cancel := newAwsInvokeContext(c)
📝 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.
| func newAwsInvokeContext() (context.Context, context.CancelFunc) { | |
| if common.RelayTimeout <= 0 { | |
| return context.Background(), func() {} | |
| } | |
| return context.WithTimeout(context.Background(), time.Duration(common.RelayTimeout)*time.Second) | |
| } | |
| func newAwsInvokeContext(c *gin.Context) (context.Context, context.CancelFunc) { | |
| reqCtx := c.Request.Context() | |
| if common.RelayTimeout <= 0 { | |
| return reqCtx, func() {} | |
| } | |
| return context.WithTimeout(reqCtx, time.Duration(common.RelayTimeout)*time.Second) | |
| } |
🤖 Prompt for AI Agents
In @relay/channel/aws/relay-aws.go around lines 42-47, newAwsInvokeContext
currently uses context.Background() which prevents request cancellation from
propagating; change its signature to accept the request context (e.g.,
newAwsInvokeContext(c *gin.Context) or newAwsInvokeContext(ctx
context.Context)), create the child context from the incoming request context
(use c.Request.Context() or the passed ctx) and apply the timeout only if
common.RelayTimeout > 0, returning the derived context and cancel func; then
update all call sites that invoked newAwsInvokeContext (the three places noted)
to pass the gin context variable (c) or the request context so cancellations
from the client propagate into AWS SDK calls.
There was a problem hiding this comment.
Yes, this is exactly what I'm going to do.
There was a problem hiding this comment.
@seefs001, great! Thank you for addressing this. The fix will ensure proper request cancellation propagation and prevent resource leaks. Let me know if you need any assistance with the implementation.
fix: fix the proxyURL is empty, not using the default HTTP client configuration && the AWS calling side did not apply the relay timeout.
2 participants
…
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.