From 619af282849cf3ba15d9ce274fb675c201652fee Mon Sep 17 00:00:00 2001 From: Vimal Kumar Date: Tue, 11 Aug 2026 14:10:39 +0530 Subject: [PATCH] OLS-3743 Wire Agent.spec.timeouts to HTTP client and sandbox requests The Agent CRD's per-step timeout fields (analysisSeconds, executionSeconds, verificationSeconds) were defined but never read by the controller. The HTTP client used a hardcoded 5-minute timeout and the sandbox received no timeout_ms in the request body. Thread the per-step timeout from Agent.Spec.Timeouts through callWithSandbox to both NewAgentHTTPClient (sets http.Client.Timeout) and the request body (sets timeout_ms so the sandbox can gracefully wind down). Fall back to the existing 5-minute default when the Agent CR has no timeout configured for the step. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Vimal Kumar --- controller/agenticrun/client.go | 21 ++-- controller/agenticrun/client_test.go | 71 ++++++++++-- controller/agenticrun/reconciler_test.go | 2 +- controller/agenticrun/sandbox_agent.go | 28 ++++- controller/agenticrun/sandbox_agent_test.go | 121 ++++++++++++++++++-- 5 files changed, 211 insertions(+), 32 deletions(-) diff --git a/controller/agenticrun/client.go b/controller/agenticrun/client.go index 3fdaea4c..891b763b 100644 --- a/controller/agenticrun/client.go +++ b/controller/agenticrun/client.go @@ -18,10 +18,11 @@ const ( maxResponseSize = 2 << 20 // 2 MiB runPath = "/v1/agent/run" - ErrMarshalRequest = "failed to marshal request" - ErrCreateHTTPRequest = "failed to create HTTP request" - ErrPost = "POST" - ErrReadResponseBody = "failed to read response body" + ErrMarshalRequest = "failed to marshal request" + ErrCreateHTTPRequest = "failed to create HTTP request" + ErrPost = "POST" + ErrReadResponseBody = "failed to read response body" + defaultHTTPClientTimeout = 5 * time.Minute ) type agentRunRequest struct { @@ -65,7 +66,7 @@ type agentRunResponse struct { // AgentHTTPClientInterface abstracts HTTP calls to the agent service for testability. type AgentHTTPClientInterface interface { - Run(ctx context.Context, systemPrompt, query string, outputSchema json.RawMessage, agentCtx *agentContext, extraHeaders http.Header) (*agentRunResponse, error) + Run(ctx context.Context, systemPrompt, query string, outputSchema json.RawMessage, agentCtx *agentContext, extraHeaders http.Header, timeoutMs *int64) (*agentRunResponse, error) } // AgentHTTPClient communicates with the agentic-sandbox REST API. @@ -74,10 +75,13 @@ type AgentHTTPClient struct { endpoint string } -func NewAgentHTTPClient(endpoint string) AgentHTTPClientInterface { +func NewAgentHTTPClient(endpoint string, timeout time.Duration) AgentHTTPClientInterface { + if timeout <= 0 { + timeout = defaultHTTPClientTimeout + } return &AgentHTTPClient{ httpClient: &http.Client{ - Timeout: 5 * time.Minute, + Timeout: timeout, Transport: &http.Transport{ TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec // internal cluster traffic }, @@ -86,12 +90,13 @@ func NewAgentHTTPClient(endpoint string) AgentHTTPClientInterface { } } -func (c *AgentHTTPClient) Run(ctx context.Context, systemPrompt, query string, outputSchema json.RawMessage, agentCtx *agentContext, extraHeaders http.Header) (*agentRunResponse, error) { +func (c *AgentHTTPClient) Run(ctx context.Context, systemPrompt, query string, outputSchema json.RawMessage, agentCtx *agentContext, extraHeaders http.Header, timeoutMs *int64) (*agentRunResponse, error) { req := agentRunRequest{ Query: query, SystemPrompt: systemPrompt, OutputSchema: outputSchema, Context: agentCtx, + TimeoutMs: timeoutMs, } body, err := json.Marshal(req) diff --git a/controller/agenticrun/client_test.go b/controller/agenticrun/client_test.go index c414d472..ca7cc095 100644 --- a/controller/agenticrun/client_test.go +++ b/controller/agenticrun/client_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" agenticv1alpha1 "github.com/openshift/lightspeed-agentic-operator/api/v1alpha1" ) @@ -35,8 +36,8 @@ func TestAgentHTTPClient_RunSuccess(t *testing.T) { })) defer server.Close() - client := NewAgentHTTPClient(server.URL) - resp, err := client.Run(context.Background(), "You are an SRE agent", "check health", nil, nil, nil) + client := NewAgentHTTPClient(server.URL, 0) + resp, err := client.Run(context.Background(), "You are an SRE agent", "check health", nil, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -52,16 +53,16 @@ func TestAgentHTTPClient_RunHTTPError(t *testing.T) { })) defer server.Close() - client := NewAgentHTTPClient(server.URL) - _, err := client.Run(context.Background(), "", "test", nil, nil, nil) + client := NewAgentHTTPClient(server.URL, 0) + _, err := client.Run(context.Background(), "", "test", nil, nil, nil, nil) if err == nil { t.Fatal("expected error for HTTP 500") } } func TestAgentHTTPClient_RunConnectionError(t *testing.T) { - client := NewAgentHTTPClient("http://127.0.0.1:1") - _, err := client.Run(context.Background(), "", "test", nil, nil, nil) + client := NewAgentHTTPClient("http://127.0.0.1:1", 0) + _, err := client.Run(context.Background(), "", "test", nil, nil, nil, nil) if err == nil { t.Fatal("expected error for connection failure") } @@ -93,7 +94,7 @@ func TestAgentHTTPClient_RunWithExecutionResult(t *testing.T) { })) defer server.Close() - client := NewAgentHTTPClient(server.URL) + client := NewAgentHTTPClient(server.URL, 0) agentCtx := &agentContext{ TargetNamespaces: []string{"production"}, ExecutionResult: &agentExecutionResult{ @@ -103,7 +104,7 @@ func TestAgentHTTPClient_RunWithExecutionResult(t *testing.T) { }, }, } - _, err := client.Run(context.Background(), "", "test", nil, agentCtx, nil) + _, err := client.Run(context.Background(), "", "test", nil, agentCtx, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -124,11 +125,11 @@ func TestAgentHTTPClient_RunWithoutExecutionResult(t *testing.T) { })) defer server.Close() - client := NewAgentHTTPClient(server.URL) + client := NewAgentHTTPClient(server.URL, 0) agentCtx := &agentContext{ TargetNamespaces: []string{"production"}, } - _, err := client.Run(context.Background(), "", "test", nil, agentCtx, nil) + _, err := client.Run(context.Background(), "", "test", nil, agentCtx, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -158,12 +159,58 @@ func TestAgentHTTPClient_RunWithContext(t *testing.T) { })) defer server.Close() - client := NewAgentHTTPClient(server.URL) + client := NewAgentHTTPClient(server.URL, 0) agentCtx := &agentContext{ TargetNamespaces: []string{"production"}, PreviousAttempts: []agentPreviousAttempt{{Attempt: 1, FailureReason: "timeout"}}, } - _, err := client.Run(context.Background(), "", "test", nil, agentCtx, nil) + _, err := client.Run(context.Background(), "", "test", nil, agentCtx, nil, nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestAgentHTTPClient_RunTimeoutMsPropagated(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req agentRunRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Fatalf("failed to decode request: %v", err) + } + if req.TimeoutMs == nil { + t.Fatal("expected timeout_ms to be set") + } + if *req.TimeoutMs != 600000 { + t.Errorf("timeout_ms = %d, want 600000", *req.TimeoutMs) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"success": true}`)) + })) + defer server.Close() + + client := NewAgentHTTPClient(server.URL, 10*time.Minute) + timeoutMs := int64(600000) + _, err := client.Run(context.Background(), "", "test", nil, nil, nil, &timeoutMs) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestAgentHTTPClient_RunTimeoutMsOmittedWhenNil(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req agentRunRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Fatalf("failed to decode request: %v", err) + } + if req.TimeoutMs != nil { + t.Errorf("timeout_ms should be nil, got %d", *req.TimeoutMs) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"success": true}`)) + })) + defer server.Close() + + client := NewAgentHTTPClient(server.URL, 0) + _, err := client.Run(context.Background(), "", "test", nil, nil, nil, nil) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/controller/agenticrun/reconciler_test.go b/controller/agenticrun/reconciler_test.go index 1173e77b..38ffa5f2 100644 --- a/controller/agenticrun/reconciler_test.go +++ b/controller/agenticrun/reconciler_test.go @@ -262,7 +262,7 @@ func newMockSandboxAgent(analysisJSON, executionJSON, verificationJSON string) ( caller := &SandboxAgentCaller{ Sandbox: sandbox, K8sClient: fc, - ClientFactory: func(_ string) AgentHTTPClientInterface { + ClientFactory: func(_ string, _ time.Duration) AgentHTTPClientInterface { resp := responses[callCount%len(responses)] callCount++ httpClient.response = &agentRunResponse{Response: json.RawMessage(resp)} diff --git a/controller/agenticrun/sandbox_agent.go b/controller/agenticrun/sandbox_agent.go index bab4e3c5..a86765b3 100644 --- a/controller/agenticrun/sandbox_agent.go +++ b/controller/agenticrun/sandbox_agent.go @@ -16,6 +16,7 @@ import ( const ( defaultSandboxTimeout = 5 * time.Minute + httpGracePeriod = 15 * time.Second ErrAnalysisAgentCall = "analysis agent call" ErrParseAnalysisResponse = "parse analysis response" @@ -61,7 +62,7 @@ type SandboxLifecycle interface { type SandboxAgentCaller struct { Sandbox SandboxLifecycle K8sClient client.Client - ClientFactory func(endpoint string) AgentHTTPClientInterface + ClientFactory func(endpoint string, timeout time.Duration) AgentHTTPClientInterface Namespace string Timeout time.Duration Audit AuditLogger @@ -222,8 +223,10 @@ func (s *SandboxAgentCaller) callWithSandbox( s.Audit.InjectTraceContext(ctx, run, headers) } - client := s.ClientFactory(agentURL) - resp, err := client.Run(ctx, "", query, schema, agentCtx, headers) + stepTimeout := timeoutForStep(stepName, step.Agent) + client := s.ClientFactory(agentURL, stepTimeout+httpGracePeriod) + timeoutMs := int64(stepTimeout / time.Millisecond) + resp, err := client.Run(ctx, "", query, schema, agentCtx, headers, &timeoutMs) if err != nil { return nil, err } @@ -231,6 +234,25 @@ func (s *SandboxAgentCaller) callWithSandbox( return resp.Response, nil } +func timeoutForStep(stepName string, agent *agenticv1alpha1.Agent) time.Duration { + if agent == nil { + return defaultSandboxTimeout + } + var seconds int32 + switch stepName { + case "analysis": + seconds = agent.Spec.Timeouts.AnalysisSeconds + case "execution": + seconds = agent.Spec.Timeouts.ExecutionSeconds + case "verification": + seconds = agent.Spec.Timeouts.VerificationSeconds + } + if seconds > 0 { + return time.Duration(seconds) * time.Second + } + return defaultSandboxTimeout +} + func (s *SandboxAgentCaller) ReleaseSandboxes(ctx context.Context, run *agenticv1alpha1.AgenticRun) error { log := logf.FromContext(ctx) var firstErr error diff --git a/controller/agenticrun/sandbox_agent_test.go b/controller/agenticrun/sandbox_agent_test.go index 54cb49c6..97017ac4 100644 --- a/controller/agenticrun/sandbox_agent_test.go +++ b/controller/agenticrun/sandbox_agent_test.go @@ -41,17 +41,19 @@ func (m *mockSandboxProvider) Release(_ context.Context, _ string) error { } type mockHTTPClient struct { - response *agentRunResponse - err error - lastQuery string - lastPrompt string - lastCtx *agentContext + response *agentRunResponse + err error + lastQuery string + lastPrompt string + lastCtx *agentContext + lastTimeoutMs *int64 } -func (m *mockHTTPClient) Run(_ context.Context, systemPrompt, query string, _ json.RawMessage, agentCtx *agentContext, _ http.Header) (*agentRunResponse, error) { +func (m *mockHTTPClient) Run(_ context.Context, systemPrompt, query string, _ json.RawMessage, agentCtx *agentContext, _ http.Header, timeoutMs *int64) (*agentRunResponse, error) { m.lastQuery = query m.lastPrompt = systemPrompt m.lastCtx = agentCtx + m.lastTimeoutMs = timeoutMs return m.response, m.err } @@ -61,7 +63,7 @@ func newTestSandboxAgentCaller(sandbox *mockSandboxProvider, httpClient *mockHTT return &SandboxAgentCaller{ Sandbox: sandbox, K8sClient: fc, - ClientFactory: func(_ string) AgentHTTPClientInterface { return httpClient }, + ClientFactory: func(_ string, _ time.Duration) AgentHTTPClientInterface { return httpClient }, Namespace: "test-ns", Timeout: 5 * time.Minute, } @@ -76,7 +78,7 @@ func newTestSandboxAgentCallerWithAgenticRun(sandbox *mockSandboxProvider, httpC return &SandboxAgentCaller{ Sandbox: sandbox, K8sClient: fc, - ClientFactory: func(_ string) AgentHTTPClientInterface { return httpClient }, + ClientFactory: func(_ string, _ time.Duration) AgentHTTPClientInterface { return httpClient }, Namespace: "test-ns", Timeout: 5 * time.Minute, } @@ -721,6 +723,109 @@ func TestReleaseSandboxes_ContinuesOnError(t *testing.T) { } } +// --- Timeout wiring tests --- + +func TestTimeoutForStep(t *testing.T) { + agent := &agenticv1alpha1.Agent{ + Spec: agenticv1alpha1.AgentSpec{ + Timeouts: agenticv1alpha1.AgentTimeouts{ + AnalysisSeconds: 600, + ExecutionSeconds: 900, + VerificationSeconds: 300, + }, + }, + } + + cases := []struct { + step string + want time.Duration + }{ + {"analysis", 600 * time.Second}, + {"execution", 900 * time.Second}, + {"verification", 300 * time.Second}, + {"escalation", defaultSandboxTimeout}, + } + for _, tc := range cases { + t.Run(tc.step, func(t *testing.T) { + got := timeoutForStep(tc.step, agent) + if got != tc.want { + t.Errorf("timeoutForStep(%q) = %v, want %v", tc.step, got, tc.want) + } + }) + } +} + +func TestTimeoutForStep_DefaultsWhenUnset(t *testing.T) { + agent := &agenticv1alpha1.Agent{} + for _, step := range []string{"analysis", "execution", "verification"} { + got := timeoutForStep(step, agent) + if got != defaultSandboxTimeout { + t.Errorf("timeoutForStep(%q) with unset timeouts = %v, want %v", step, got, defaultSandboxTimeout) + } + } +} + +func TestTimeoutForStep_NilAgent(t *testing.T) { + got := timeoutForStep("analysis", nil) + if got != defaultSandboxTimeout { + t.Errorf("timeoutForStep with nil agent = %v, want %v", got, defaultSandboxTimeout) + } +} + +func TestSandboxAgentCaller_TimeoutPropagatedToRequest(t *testing.T) { + sandbox := &mockSandboxProvider{claimName: "ls-analysis-fix-crash", endpoint: "http://sandbox:8080"} + httpClient := &mockHTTPClient{ + response: &agentRunResponse{ + Response: json.RawMessage(`{"success": true, "options": []}`), + }, + } + + var capturedHTTPTimeout time.Duration + run := testSandboxAgenticRun() + fc := fake.NewClientBuilder().WithScheme(testScheme()). + WithObjects(run). + WithStatusSubresource(run, &agenticv1alpha1.AnalysisResult{}, &agenticv1alpha1.ExecutionResult{}, &agenticv1alpha1.VerificationResult{}, &agenticv1alpha1.EscalationResult{}). + Build() + _ = fc.Create(context.Background(), fakeBaseTemplate()) + caller := &SandboxAgentCaller{ + Sandbox: sandbox, + K8sClient: fc, + ClientFactory: func(_ string, timeout time.Duration) AgentHTTPClientInterface { + capturedHTTPTimeout = timeout + return httpClient + }, + Namespace: "test-ns", + Timeout: 5 * time.Minute, + } + + agent := testDefaultAgent() + agent.Spec.Timeouts = agenticv1alpha1.AgentTimeouts{ + AnalysisSeconds: 600, + } + step := resolvedStep{ + Agent: agent, + LLM: testLLM("smart"), + Tools: func() *agenticv1alpha1.ToolsSpec { t := testTools(); return &t }(), + } + + _, err := caller.Analyze(context.Background(), run, step, "test", defaultSandboxSA) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if httpClient.lastTimeoutMs == nil { + t.Fatal("expected timeout_ms to be set") + } + if *httpClient.lastTimeoutMs != 600000 { + t.Errorf("timeout_ms = %d, want 600000", *httpClient.lastTimeoutMs) + } + + wantHTTPTimeout := 600*time.Second + httpGracePeriod + if capturedHTTPTimeout != wantHTTPTimeout { + t.Errorf("HTTP client timeout = %v, want %v (stepTimeout + grace)", capturedHTTPTimeout, wantHTTPTimeout) + } +} + type trackingMockSandbox struct { released *[]string errOnClaim string