Replace N+1 execution fetches in batch-status handler with one batch fetch - #811
Conversation
…tatus handler with one batch fetch handleBatchStatus now issues a single GetExecutionRecordsBatch query regardless of how many execution IDs are requested, instead of looping GetExecutionRecord per ID. Missing IDs preserve the prior per-ID 'not_found' response. Requests with more than 500 IDs are rejected with a 400 before hitting storage. Storage layer gains GetExecutionRecordsBatch on LocalStorage and the StorageProvider/ExecutionStore interfaces; test stubs updated to satisfy the new interface.
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
🔴 PR-AF Review — Merge Blocked — Must-Fix Found
🚫 Merge blocked. 2 must-fix issues found by automated review.
Automated multi-agent code review · PR-AF built with AgentField
4 findings · 🚫 2 blocking · 💬 2 advisory · 🔴 0 critical · 🟠 4 important · 🔵 0 suggestions · ⚪ 0 nitpicks
PR Overview
Summary
- Replaces N+1 per-ID storage fetches in
handleBatchStatuswith a singleGetExecutionRecordsBatchquery using a parameterizedWHERE execution_id IN (...)clause. - Preserves the per-ID
not_foundresponse contract for missing execution IDs while collapsing all DB round-trips to one. - Rejects oversized batches (>500 IDs, 400 on the oversize path) with a
400before any storage call, and reuses the batched webhook-registration lookup to avoid a secondary N+1.
Changes
control-plane/internal/storage/storage.go: addsGetExecutionRecordsBatchto theStorageProvider/ExecutionStoreinterfaces.control-plane/internal/storage/execution_records.go: implementsGetExecutionRecordsBatchonLocalStoragewith a single parameterizedIN (...)query.control-plane/internal/handlers/execute.go: rewriteshandleBatchStatusto fetch once and enforce the batch-size cap.- Test stubs across
internal/handlers/*andinternal/server/*updated to satisfy the new storage interface; newexecute_batch_status_test.goandexecution_records_test.gocover the batch path, per-IDnot_foundmapping, and oversize rejection.
Test plan
cd control-plane && go vet ./...(clean).cd control-plane && go test ./internal/handlers/... ./internal/storage/...(passing).- Confirm a 500-ID batch triggers exactly one storage call and that oversize batches (e.g. 501 IDs) return
400without hitting storage.
Closes #430
🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField
Key Findings
2 issue(s) should be addressed before merge:
- 🟠 Batch storage errors now discard successful per-ID polling results (
control-plane/internal/handlers/execute.go:820) — This changes the public batch-polling failure contract from per-ID partial results to an all-or-nothing HTTP failure. Gate: 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. - 🟠 Preserve per-ID batch-status errors when the batch read fails (
control-plane/internal/handlers/execute.go:821) — The batch handler changes the documented batch-status error contract: a storage read failure is now promoted to a request-wide HTTP 500, whereas the former handler returned HTTP 200 with a `status: "e… Gate: Breaks existing public API contract by returning 500 instead of per-ID errors, with no migration path.
2 advisory finding(s) surfaced as non-blocking:
- 🟠 Batch status retains 500 workflow-execution queries (
control-plane/internal/handlers/execute.go:837) - 🟠 Batch status omits registered webhook events (
control-plane/internal/storage/execution_records.go:171)
Files with findings: control-plane/internal/handlers/execute.go, control-plane/internal/storage/execution_records.go
All Findings by Severity
🟠 Important (4)
- Batch status retains 500 workflow-execution queries
control-plane/internal/handlers/execute.go:837 - Batch status omits registered webhook events
control-plane/internal/storage/execution_records.go:171 - Batch storage errors now discard successful per-ID polling results
control-plane/internal/handlers/execute.go:820 - Preserve per-ID batch-status errors when the batch read fails
control-plane/internal/handlers/execute.go:821
Review Process Details
Dimensions Analyzed (5):
- Preservation of webhook-event response data — 4 file(s)
- Partial-result and failure-status semantics — 4 file(s)
- Storage interface expansion and handler wiring — 5 file(s)
- Batch SQL execution across SQLite and PostgreSQL — 5 file(s)
- Batch-status design has a coherent end-to-end query budget — 4 file(s)
Meta-Dimension Lenses (3):
- Semantic — 3 dimension(s), 93% coverage confidence
- Mechanical — 2 dimension(s), 90% coverage confidence
- Systemic — 2 dimension(s), 90% coverage confidence
Cross-Reference & Adversary Analysis:
- 3 finding(s) adversarially tested: 3 confirmed, 0 challenged
Pipeline Stats
| Metric | Value |
|---|---|
| Duration | 1364.8s |
| Agent invocations | 34 |
| Coverage iterations | 2 |
| Estimated cost | N/A (provider does not report cost) |
| Budget exhausted | No |
| PR type | performance |
| Complexity | medium |
Review ID: rev_a0c1f0076f93
| for _, exec := range result { | ||
| found = append(found, exec) | ||
| } | ||
| ls.populateWebhookRegistration(ctx, found) |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: The batch endpoint is new and missing webhook events is a feature gap, not a regression of an existing contract.
Fix the batch status endpoint to include webhook events.
🟠 IMPORTANT · confidence 99%
The batch fetch only sets WebhookRegistered, leaving WebhookEvents nil, so renderStatus serializes null instead of the event list. This breaks clients that rely on the batch endpoint to get the same data as the single-status endpoint.
Evidence
Step 1:
GET /api/v1/executions/:execution_idreacheshandleStatus, which callsGetExecutionRecord.
Step 2:GetExecutionRecordcallsenrichExecutionWebhook(ctx, exec, true)atexecution_records.go:108.
Step 3: for a registered webhook,enrichExecutionWebhookcallsListExecutionWebhookEventsand assigns its creation-time/ID-ordered result toexec.WebhookEvents(execution_records.go:1586-1614).
Step 4:renderStatusserializes that field asWebhookEvents: exec.WebhookEvents(execute.go:2344).
Step 5:POST /api/v1/executions/batch-statusinstead callsGetExecutionRecordsBatch(execute.go:820), whose only webhook enrichment ispopulateWebhookRegistration(ctx, found)(execution_records.go:171). That helper assigns onlyWebhookRegisteredand never assignsWebhookEvents(execution_records.go:1616-1647).
Step 6: an integration test seeded a registration and two events; its equality assertion failed with GET's two event objects as expected and POST's[]*types.ExecutionWebhookEvent(nil)as actual.
💡 Suggested Fix
After the registration lookup, load events with ListExecutionWebhookEventsBatch and assign each returned slice to its execution (preferably only for registered executions). Preserve the existing best-effort behavior by logging an event-query failure rather than failing the record read. The existing helper in local.go:8204 is suitable: it batches IDs and orders each execution's events by created_at ASC, id ASC, matching ListExecutionWebhookEvents.
Webhook payload equivalence · confidence 99%
🤖 Reviewed by AgentField PR-AF
|
|
||
| // One storage fetch regardless of how many IDs were requested — the | ||
| // previous loop issued one GetExecutionRecord per ID (N+1). | ||
| records, err := c.store.GetExecutionRecordsBatch(reqCtx, request.ExecutionIDs) |
There was a problem hiding this comment.
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:122exposesPOST /api/v1/executions/batch-statusthroughBatchExecutionStatusHandler; the bundled agent-use documentation directs callers polling several executions to this endpoint.
Step 2: Before this change,handleBatchStatusiterated the requested IDs and, for eachGetExecutionRecord(reqCtx, id)error, assigned that IDStatus: "error"andcontinued; it then always renderedctx.JSON(http.StatusOK, response). This was verified againstorigin/main.
Step 3: The new handler passes the entire request list and the request context toGetExecutionRecordsBatch(reqCtx, request.ExecutionIDs)at line 820.LocalStorage.GetExecutionRecordsBatchreturns one error forQueryContext,scanExecution, orrows.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 forbadfails butgoodsucceeds changes from HTTP 200 withbad.status="error"and a renderedgoodresult to HTTP 500 with no per-ID results.TestHandleBatchStatus_StorageErrorReturns500confirms 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
| // One storage fetch regardless of how many IDs were requested — the | ||
| // previous loop issued one GetExecutionRecord per ID (N+1). | ||
| records, err := c.store.GetExecutionRecordsBatch(reqCtx, request.ExecutionIDs) | ||
| if err != nil { |
There was a problem hiding this comment.
Caution
Must-fix before merge. Breaks existing public API contract by returning 500 instead of per-ID errors, with no migration path.
Preserve per-ID batch-status errors when the batch read fails
🟠 IMPORTANT · confidence 99%
The new batch handler returns HTTP 500 on any storage read failure, breaking the previous contract that returned HTTP 200 with per-ID error entries. Callers can no longer process partial results or identify which IDs failed.
Evidence
Changed code, control-plane/internal/handlers/execute.go:820-824:
"records, err := c.store.GetExecutionRecordsBatch(reqCtx, request.ExecutionIDs)
if err != nil {
ctx.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("failed to load executions: %v", err)})
return
}"Former handler in origin/main:control-plane/internal/handlers/execute.go:
"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)),
}
continue
}"The current handler test also encodes the new incompatible behavior: control-plane/internal/handlers/execute_batch_status_test.go:144-160 requires
http.StatusInternalServerErrorwhenGetExecutionRecordsBatchreturns an error.
💡 Suggested Fix
Preserve the batch response contract on a batch-store error: return HTTP 200 with an ExecutionStatusResponse{ExecutionID: id, Status: "error", Error: ...} for every requested ID (deduplicated by the response map), rather than aborting the entire request with HTTP 500. Update the error-path test to assert those per-ID entries.
Consistency Verifier · confidence 99%
🤖 Reviewed by AgentField PR-AF
|
|
Realigns embedded mirrors with skills/ sources inherited from the main merge so skillkit drift tests pass branch-locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cef0a4c to
f43370c
Compare
2 participants
Summary
handleBatchStatuswith a singleGetExecutionRecordsBatchquery using a parameterizedWHERE execution_id IN (...)clause.not_foundresponse contract for missing execution IDs while collapsing all DB round-trips to one.400before any storage call, and reuses the batched webhook-registration lookup to avoid a secondary N+1.Changes
control-plane/internal/storage/storage.go: addsGetExecutionRecordsBatchto theStorageProvider/ExecutionStoreinterfaces.control-plane/internal/storage/execution_records.go: implementsGetExecutionRecordsBatchonLocalStoragewith a single parameterizedIN (...)query.control-plane/internal/handlers/execute.go: rewriteshandleBatchStatusto fetch once and enforce the batch-size cap.internal/handlers/*andinternal/server/*updated to satisfy the new storage interface; newexecute_batch_status_test.goandexecution_records_test.gocover the batch path, per-IDnot_foundmapping, and oversize rejection.Test plan
cd control-plane && go vet ./...(clean).cd control-plane && go test ./internal/handlers/... ./internal/storage/...(passing).400without hitting storage.Closes #430
🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField