Skip to content
Merged
Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions control-plane/internal/handlers/agentic/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ func (m *mockStatusStorage) CreateExecutionRecord(ctx context.Context, execution
func (m *mockStatusStorage) GetExecutionRecord(ctx context.Context, executionID string) (*types.Execution, error) {
return nil, nil
}
func (m *mockStatusStorage) GetExecutionRecordsBatch(ctx context.Context, executionIDs []string) (map[string]*types.Execution, error) {
return map[string]*types.Execution{}, nil
}
func (m *mockStatusStorage) UpdateExecutionRecord(ctx context.Context, executionID string, update func(*types.Execution) (*types.Execution, error)) (*types.Execution, error) {
return nil, nil
}
Expand Down
3 changes: 3 additions & 0 deletions control-plane/internal/handlers/config_storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ func (m *configStorageMock) CreateExecutionRecord(ctx context.Context, execution
func (m *configStorageMock) GetExecutionRecord(ctx context.Context, executionID string) (*types.Execution, error) {
return nil, nil
}
func (m *configStorageMock) GetExecutionRecordsBatch(ctx context.Context, executionIDs []string) (map[string]*types.Execution, error) {
return map[string]*types.Execution{}, nil
}
func (m *configStorageMock) UpdateExecutionRecord(ctx context.Context, executionID string, update func(*types.Execution) (*types.Execution, error)) (*types.Execution, error) {
return nil, nil
}
Expand Down
3 changes: 3 additions & 0 deletions control-plane/internal/handlers/connector/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,9 @@ func (m *mockStorage) CreateExecutionRecord(ctx context.Context, execution *type
func (m *mockStorage) GetExecutionRecord(ctx context.Context, executionID string) (*types.Execution, error) {
return nil, nil
}
func (m *mockStorage) GetExecutionRecordsBatch(ctx context.Context, executionIDs []string) (map[string]*types.Execution, error) {
return map[string]*types.Execution{}, nil
}
func (m *mockStorage) UpdateExecutionRecord(ctx context.Context, executionID string, update func(*types.Execution) (*types.Execution, error)) (*types.Execution, error) {
return nil, nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,21 @@ func (s *cancelHandlerErrorStorage) GetExecutionRecord(_ context.Context, _ stri
return &copy, nil
}

func (s *cancelHandlerErrorStorage) GetExecutionRecordsBatch(_ context.Context, executionIDs []string) (map[string]*types.Execution, error) {
if s.getExecErr != nil {
return nil, s.getExecErr
}
result := make(map[string]*types.Execution, len(executionIDs))
if s.exec == nil {
return result, nil
}
for _, id := range executionIDs {
copy := *s.exec
result[id] = &copy
}
return result, nil
}

func (s *cancelHandlerErrorStorage) GetWorkflowExecution(_ context.Context, _ string) (*types.WorkflowExecution, error) {
if s.getWorkflowErr != nil {
return nil, s.getWorkflowErr
Expand Down Expand Up @@ -169,6 +184,21 @@ func (s *workflowEventStoreStub) GetExecutionRecord(_ context.Context, _ string)
return &copy, nil
}

func (s *workflowEventStoreStub) GetExecutionRecordsBatch(_ context.Context, executionIDs []string) (map[string]*types.Execution, error) {
result := make(map[string]*types.Execution, len(executionIDs))
if s.getErr != nil {
return nil, s.getErr
}
if s.exec == nil {
return result, nil
}
for _, id := range executionIDs {
copy := *s.exec
result[id] = &copy
}
return result, nil
}

func (s *workflowEventStoreStub) CreateExecutionRecord(_ context.Context, execution *types.Execution) error {
s.createdExec = execution
return s.createErr
Expand Down
39 changes: 33 additions & 6 deletions control-plane/internal/handlers/execute.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type ExecutionStore interface {
ListAgentVersions(ctx context.Context, id string) ([]*types.AgentNode, error)
CreateExecutionRecord(ctx context.Context, execution *types.Execution) error
GetExecutionRecord(ctx context.Context, executionID string) (*types.Execution, error)
GetExecutionRecordsBatch(ctx context.Context, executionIDs []string) (map[string]*types.Execution, error)
UpdateExecutionRecord(ctx context.Context, executionID string, update func(*types.Execution) (*types.Execution, error)) (*types.Execution, error)
QueryExecutionRecords(ctx context.Context, filter types.ExecutionFilter) ([]*types.Execution, error)
RegisterExecutionWebhook(ctx context.Context, webhook *types.ExecutionWebhook) error
Expand Down Expand Up @@ -188,6 +189,10 @@ const (
maxWebhookHeaders = 20
maxWebhookHeaderLength = 512
maxWebhookSecretLength = 4096

// maxBatchStatusIDs caps the number of execution IDs a single
// batch-status request may fetch, matching the storage-layer cap.
maxBatchStatusIDs = 500
)

// ExecuteHandler handles synchronous execution requests.
Expand Down Expand Up @@ -807,19 +812,41 @@ func (c *executionController) handleBatchStatus(ctx *gin.Context) {
ctx.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
return
}
if len(request.ExecutionIDs) > maxBatchStatusIDs {
ctx.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("batch status supports at most %d execution IDs, got %d", maxBatchStatusIDs, len(request.ExecutionIDs))})
return
}

// Use one storage fetch for the normal path. If it fails, fall back to
// individual reads so the established per-ID error contract is preserved.
records, err := c.store.GetExecutionRecordsBatch(reqCtx, request.ExecutionIDs)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Caution

Must-fix before merge. This changes the public batch-polling API from partial results (HTTP 200) to all-or-nothing (HTTP 500), breaking existing clients with no migration path.

Make the batch-status endpoint return partial per-ID results on mixed failures, or explicitly declare the all‑or‑nothing contract and add mixed-error regression tests.

🟠 IMPORTANT · confidence 95%

The handler at lines 820–823 returns HTTP 500 for any batch read error, discarding results for IDs that succeeded. Previously, each ID error was recorded in the response map and an HTTP 200 was returned, so clients could process the successful ID. This breaks the documented partial‑results contract, and the new tests only cover a single‑ID 500 case.

Evidence

Step 1: routes_core.go:122 exposes POST /api/v1/executions/batch-status through BatchExecutionStatusHandler; the bundled agent-use documentation directs callers polling several executions to this endpoint.
Step 2: Before this change, handleBatchStatus iterated the requested IDs and, for each GetExecutionRecord(reqCtx, id) error, assigned that ID Status: "error" and continued; it then always rendered ctx.JSON(http.StatusOK, response). This was verified against origin/main.
Step 3: The new handler passes the entire request list and the request context to GetExecutionRecordsBatch(reqCtx, request.ExecutionIDs) at line 820. LocalStorage.GetExecutionRecordsBatch returns one error for QueryContext, scanExecution, or rows.Err, including errors caused by a canceled context.
Step 4: Lines 821-823 immediately render HTTP 500 and return when that one batch call reports an error, before the response map is built. Therefore a fake/store scenario where the legacy per-ID read for bad fails but good succeeds changes from HTTP 200 with bad.status="error" and a rendered good result to HTTP 500 with no per-ID results. TestHandleBatchStatus_StorageErrorReturns500 confirms only this new one-ID 500 path; no mixed-error or cancellation/query-error compatibility test exists.

💡 Suggested Fix

Preserve partial responses by representing per-ID batch errors or falling back to individual reads after a batch error. If the API intentionally becomes all-or-nothing, document that HTTP 500 behavior for batch polling and add mixed success/failure plus canceled-context/query-error regression tests.


Batch-status failure contract · confidence 95%

🤖 Reviewed by AgentField PR-AF


response := make(BatchStatusResponse, len(request.ExecutionIDs))
for _, id := range request.ExecutionIDs {
exec, err := c.store.GetExecutionRecord(reqCtx, id)
if err != nil {
response[id] = ExecutionStatusResponse{
ExecutionID: id,
Status: "error",
Error: pointerString(fmt.Sprintf("load execution: %v", err)),
exec, getErr := c.store.GetExecutionRecord(reqCtx, id)
if getErr != nil {
response[id] = ExecutionStatusResponse{
ExecutionID: id,
Status: "error",
Error: pointerString(fmt.Sprintf("load execution: %v", getErr)),
}
continue
}
if exec == nil {
response[id] = ExecutionStatusResponse{
ExecutionID: id,
Status: "not_found",
}
continue
}
response[id] = c.renderStatusWithApproval(reqCtx, exec)
continue
}
if exec == nil {

exec, ok := records[id]
if !ok || exec == nil {
// Missing IDs preserve the prior per-ID response behavior.
response[id] = ExecutionStatusResponse{
ExecutionID: id,
Status: "not_found",
Expand Down
206 changes: 206 additions & 0 deletions control-plane/internal/handlers/execute_batch_status_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
package handlers

import (
"bytes"
"context"
"encoding/json"
"errors"
"net/http"
"net/http/httptest"
"testing"
"time"

"github.com/Agent-Field/agentfield/control-plane/pkg/types"

"github.com/gin-gonic/gin"
"github.com/stretchr/testify/require"
)

var errBatchStorageBoom = errors.New("storage unavailable")

// batchCountingStorage wraps testExecutionStorage and counts how many times
// the batch fetch method is invoked, so tests can assert the N+1 fix.
type batchCountingStorage struct {
*testExecutionStorage
batchCalls int
singleGetCalls int
getExecutionRecordsBatchErr error
getExecutionRecordErrs map[string]error
}

func newBatchCountingStorage() *batchCountingStorage {
return &batchCountingStorage{
testExecutionStorage: newTestExecutionStorage(nil),
}
}

// Override the single-record fetch to count it; the handler should no longer
// touch this path for batch status.
func (s *batchCountingStorage) GetExecutionRecord(ctx context.Context, executionID string) (*types.Execution, error) {
s.singleGetCalls++
if err := s.getExecutionRecordErrs[executionID]; err != nil {
return nil, err
}
return s.testExecutionStorage.GetExecutionRecord(ctx, executionID)
}

func (s *batchCountingStorage) GetExecutionRecordsBatch(ctx context.Context, executionIDs []string) (map[string]*types.Execution, error) {
s.batchCalls++
if s.getExecutionRecordsBatchErr != nil {
return nil, s.getExecutionRecordsBatchErr
}
return s.testExecutionStorage.GetExecutionRecordsBatch(ctx, executionIDs)
}

func seedBatchExecution(t *testing.T, store *batchCountingStorage, id, status string) {
t.Helper()
now := time.Now().UTC()
exec := &types.Execution{
ExecutionID: id,
RunID: "run-batch",
AgentNodeID: "agent-1",
ReasonerID: "reasoner-" + id,
NodeID: "node-1",
Status: status,
StartedAt: now,
CreatedAt: now,
UpdatedAt: now,
}
require.NoError(t, store.CreateExecutionRecord(context.Background(), exec))
}

func TestHandleBatchStatus_SingleFetchForTenIDs(t *testing.T) {
gin.SetMode(gin.TestMode)

store := newBatchCountingStorage()

// Seed 10 existing executions.
for i := 0; i < 10; i++ {
seedBatchExecution(t, store, "exec-"+string(rune('a'+i)), string(types.ExecutionStatusSucceeded))
}

// Request 10 existing + 2 missing.
ids := []string{
"exec-a", "exec-b", "exec-c", "exec-d", "exec-e",
"exec-f", "exec-g", "exec-h", "exec-i", "exec-j",
"missing-1", "missing-2",
}
body, _ := json.Marshal(BatchStatusRequest{ExecutionIDs: ids})

router := gin.New()
router.POST("/batch", BatchExecutionStatusHandler(store))

req := httptest.NewRequest(http.MethodPost, "/batch", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
router.ServeHTTP(w, req)

require.Equal(t, http.StatusOK, w.Code)
require.Equal(t, 1, store.batchCalls, "handleBatchStatus must make exactly one storage fetch")
require.Equal(t, 0, store.singleGetCalls, "handleBatchStatus must not call GetExecutionRecord per ID")

var response BatchStatusResponse
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response))
require.Len(t, response, len(ids))

// Existing IDs get the rendered status response contract.
for _, id := range ids[:10] {
entry, ok := response[id]
require.True(t, ok, "missing entry for %s", id)
require.Equal(t, id, entry.ExecutionID)
require.Equal(t, "run-batch", entry.RunID)
require.Equal(t, string(types.ExecutionStatusSucceeded), entry.Status)
require.NotEmpty(t, entry.StartedAt)
}

// Missing IDs preserve the prior per-ID response behavior: not_found.
for _, id := range ids[10:] {
entry, ok := response[id]
require.True(t, ok, "missing entry for %s", id)
require.Equal(t, id, entry.ExecutionID)
require.Equal(t, "not_found", entry.Status)
}
}

func TestHandleBatchStatus_RejectsOversizedBatch(t *testing.T) {
gin.SetMode(gin.TestMode)

store := newBatchCountingStorage()

ids := make([]string, 501)
for i := range ids {
ids[i] = "exec-" + string(rune('a'+i%26))
}
body, _ := json.Marshal(BatchStatusRequest{ExecutionIDs: ids})

router := gin.New()
router.POST("/batch", BatchExecutionStatusHandler(store))

req := httptest.NewRequest(http.MethodPost, "/batch", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
router.ServeHTTP(w, req)

require.Equal(t, http.StatusBadRequest, w.Code)
require.Equal(t, 0, store.batchCalls, "oversized batch must not hit storage")
}

func TestHandleBatchStatus_BatchStorageErrorPreservesPerIDResults(t *testing.T) {
gin.SetMode(gin.TestMode)

store := newBatchCountingStorage()
store.getExecutionRecordsBatchErr = errBatchStorageBoom
store.getExecutionRecordErrs = map[string]error{"bad": errBatchStorageBoom}
seedBatchExecution(t, store, "good", string(types.ExecutionStatusSucceeded))

body, _ := json.Marshal(BatchStatusRequest{ExecutionIDs: []string{"bad", "good"}})
router := gin.New()
router.POST("/batch", BatchExecutionStatusHandler(store))

req := httptest.NewRequest(http.MethodPost, "/batch", bytes.NewReader(body))
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
router.ServeHTTP(w, req)

require.Equal(t, http.StatusOK, w.Code)
require.Equal(t, 1, store.batchCalls)
require.Equal(t, 2, store.singleGetCalls)

var response BatchStatusResponse
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response))
require.Equal(t, "error", response["bad"].Status)
require.Contains(t, *response["bad"].Error, "load execution: storage unavailable")
require.Equal(t, string(types.ExecutionStatusSucceeded), response["good"].Status)
}

func TestHandleBatchStatus_BatchStorageErrorWithCanceledContextReturnsPerIDErrors(t *testing.T) {
gin.SetMode(gin.TestMode)

store := newBatchCountingStorage()
store.getExecutionRecordsBatchErr = context.Canceled
store.getExecutionRecordErrs = map[string]error{
"exec-a": context.Canceled,
"exec-b": context.Canceled,
}
body, _ := json.Marshal(BatchStatusRequest{ExecutionIDs: []string{"exec-a", "exec-b"}})
router := gin.New()
router.POST("/batch", BatchExecutionStatusHandler(store))

reqCtx, cancel := context.WithCancel(context.Background())
cancel()
req := httptest.NewRequest(http.MethodPost, "/batch", bytes.NewReader(body)).WithContext(reqCtx)
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
router.ServeHTTP(w, req)

require.Equal(t, http.StatusOK, w.Code)
require.Equal(t, 1, store.batchCalls)
require.Equal(t, 2, store.singleGetCalls)

var response BatchStatusResponse
require.NoError(t, json.Unmarshal(w.Body.Bytes(), &response))
for _, id := range []string{"exec-a", "exec-b"} {
require.Equal(t, "error", response[id].Status)
require.Contains(t, *response[id].Error, "load execution: context canceled")
}
}
15 changes: 15 additions & 0 deletions control-plane/internal/handlers/execute_cancel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,21 @@ func (s *cancelHandlerStorage) GetExecutionRecord(ctx context.Context, execution
return &copy, nil
}

func (s *cancelHandlerStorage) GetExecutionRecordsBatch(ctx context.Context, executionIDs []string) (map[string]*types.Execution, error) {
s.mu.Lock()
defer s.mu.Unlock()
result := make(map[string]*types.Execution, len(executionIDs))
for _, id := range executionIDs {
exec, ok := s.executionRecords[id]
if !ok {
continue
}
copy := *exec
result[id] = &copy
}
return result, nil
}

func (s *cancelHandlerStorage) GetWorkflowExecution(ctx context.Context, executionID string) (*types.WorkflowExecution, error) {
s.mu.Lock()
defer s.mu.Unlock()
Expand Down
2 changes: 1 addition & 1 deletion control-plane/internal/handlers/execute_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ func TestBatchExecutionStatusHandler(t *testing.T) {
{
name: "too many execution IDs",
requestBody: BatchStatusRequest{
ExecutionIDs: make([]string, 51), // Exceeds max batch size of 50
ExecutionIDs: make([]string, 501), // Exceeds max batch size of 500
},
setupMocks: func(mockStorage *MockStorageProvider) {},
expectedStatus: http.StatusBadRequest,
Expand Down
Loading
Loading