feat(rest-api): dispatch every Flow endpoint through the gRPC proxy - #4706
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
Summary by CodeRabbit
WalkthroughFlow-backed REST handlers now use a shared protobuf-based Flow proxy. The change adds deterministic workflow IDs, shared timeout and error handling, typed request dispatch, proxy-focused tests, and ChangesGeneric Flow proxy migration
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to The PR changes Flow-backed request dispatch and timeout responses, but no actionable merge-blocking risk remains in the supplied evidence. Sequence Diagram(s)sequenceDiagram
participant RESTHandler
participant ProxyFlowGRPC
participant FlowService
participant Temporal
RESTHandler->>ProxyFlowGRPC: Send Flow method and protobuf request
ProxyFlowGRPC->>Temporal: Start or reuse namespaced workflow
Temporal->>FlowService: Invoke gRPC method
FlowService-->>Temporal: Return protobuf response or error
Temporal-->>ProxyFlowGRPC: Return serialized result
ProxyFlowGRPC-->>RESTHandler: Render response or 504
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-08-07 23:00:26 UTC | Commit: d329e5b |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rest-api/api/pkg/api/handler/taskrun_test.go (2)
316-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every deterministic workflow-ID input.
The tests do not verify all inputs that prevent incompatible requests from coalescing. A regression in
includeStatshandling, query hashing, transport namespacing, or conflict policy can pass these tests.
rest-api/api/pkg/api/handler/taskrun_test.go#L316-L322: add a successfulincludeStats=truecase and assert its distinctflow-grpc-task-run-get-...-trueID.rest-api/api/pkg/api/handler/taskrun_test.go#L404-L409: captureStartWorkflowOptionsand assert the namespaced, query-derived list ID andUSE_EXISTINGpolicy.rest-api/api/pkg/api/handler/taskrun_test.go#L520-L525: captureStartWorkflowOptionsand assert the namespaced, run-and-query-derived target-list ID andUSE_EXISTINGpolicy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/taskrun_test.go` around lines 316 - 322, Expand the task-run workflow-ID tests to cover every deterministic input: in rest-api/api/pkg/api/handler/taskrun_test.go:316-322 add a successful includeStats=true case asserting its distinct flow-grpc-task-run-get-...-true ID; at :404-409 capture StartWorkflowOptions and assert the namespaced, query-derived list ID with USE_EXISTING; at :520-525 capture StartWorkflowOptions and assert the namespaced, run-and-query-derived target-list ID with USE_EXISTING.
70-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the proxied Flow method in the mock.
testRunProxyDispatchmatches thegrpcproxy.Requestargument withmock.Anything, so tests can pass when a handler supplies the wrongFullMethod. Pass the expected method to the helper and matchgrpcproxy.Request.FullMethodagainst the endpoint'sflowv1.Flow_*_FullMethodName.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/taskrun_test.go` around lines 70 - 76, Update testRunProxyDispatch to accept the expected Flow method and replace the grpcproxy.Request mock.Anything matcher with an assertion that Request.FullMethod equals the corresponding flowv1.Flow_*_FullMethodName for each endpoint. Update all helper call sites to pass the appropriate expected method while preserving the existing workflow-start capture and error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rest-api/api/pkg/api/handler/taskrun_test.go`:
- Around line 316-322: Expand the task-run workflow-ID tests to cover every
deterministic input: in rest-api/api/pkg/api/handler/taskrun_test.go:316-322 add
a successful includeStats=true case asserting its distinct
flow-grpc-task-run-get-...-true ID; at :404-409 capture StartWorkflowOptions and
assert the namespaced, query-derived list ID with USE_EXISTING; at :520-525
capture StartWorkflowOptions and assert the namespaced, run-and-query-derived
target-list ID with USE_EXISTING.
- Around line 70-76: Update testRunProxyDispatch to accept the expected Flow
method and replace the grpcproxy.Request mock.Anything matcher with an assertion
that Request.FullMethod equals the corresponding flowv1.Flow_*_FullMethodName
for each endpoint. Update all helper call sites to pass the appropriate expected
method while preserving the existing workflow-start capture and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73f1f28a-a167-45de-9df8-14a8b3abbde4
📒 Files selected for processing (3)
rest-api/api/pkg/api/handler/taskrun.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/skills/rest-flow-grpc-proxy/SKILL.md
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/api/pkg/api/handler/taskrun_test.go (1)
67-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout response regression test.
The changed proxy contract returns
504for a proxy timeout and does not callTerminateWorkflow. The shown failure cases only inject a generic scheduling error and expect500.Add a case that injects the proxy timeout condition, asserts
504, and asserts thatTerminateWorkflowis not called. This protects the changed timeout behavior.As per coding guidelines, follow the shared Engineering Guidelines for “verification expectations.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/taskrun_test.go` around lines 67 - 82, Add a regression case to the existing proxy failure tests that injects the proxy timeout condition, expects HTTP 504, and verifies TerminateWorkflow is not called. Reuse the existing test helper and mock setup around testRunProxyDispatch, preserving the current generic scheduling-error case and its 500 expectation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rest-api/api/pkg/api/handler/taskrun_test.go`:
- Around line 67-82: Add a regression case to the existing proxy failure tests
that injects the proxy timeout condition, expects HTTP 504, and verifies
TerminateWorkflow is not called. Reuse the existing test helper and mock setup
around testRunProxyDispatch, preserving the current generic scheduling-error
case and its 500 expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44f5ec16-f6dd-4c6c-8a82-1461868c4d2b
📒 Files selected for processing (3)
rest-api/api/pkg/api/handler/taskrun.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/skills/rest-flow-grpc-proxy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- rest-api/skills/rest-flow-grpc-proxy/SKILL.md
kunzhao-nv
changed the title
f165550 to
aa95560
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4706.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
rest-api/api/pkg/api/handler/taskrun_test.go (1)
163-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify that separate create requests use different workflow IDs.
Lines 166-167 verify only the namespace prefix and conflict policy. A fixed
flow-grpc-task-run-create-*ID would pass this test and deduplicate separate create requests. Execute two valid create requests and assert that their captured workflow IDs differ.As per coding guidelines,
rest-api/**/*_test.gorequires test coverage organized around the production behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/taskrun_test.go` around lines 163 - 167, Update the create-request test around the captured `started` workflow execution to submit two valid create requests, capture each resulting workflow ID, and assert they differ while retaining the existing prefix and conflict-policy assertions. Organize the coverage with the existing task-run production behavior test structure rather than validating only a single request.Source: Coding guidelines
rest-api/skills/rest-flow-grpc-proxy/SKILL.md (1)
42-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the
ExecuteFlowGRPCexception.
resolveTrayIDsBySlotis a helper inrest-api/api/pkg/api/handler/tray.gothat returns a plainerror. The phrase “where the caller is not a handler” can exclude the documented example. Define the exception by behavior instead.Proposed wording
-Use `ExecuteFlowGRPC` directly only where the caller is not a handler and must -return a plain `error`, as `resolveTrayIDsBySlot` does. +Use `ExecuteFlowGRPC` directly only in helpers that return a plain `error` +instead of rendering an Echo response, as `resolveTrayIDsBySlot` does.As per path instructions: “Review Markdown for correctness, clarity, spelling, grammar, working links, and whether commands/examples are realistic and safe.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/skills/rest-flow-grpc-proxy/SKILL.md` around lines 42 - 47, Clarify the ExecuteFlowGRPC guidance in the documented helper section: define its exception by return behavior, stating that it should be used directly when the caller must return a plain error, as in resolveTrayIDsBySlot. Keep common.FlowWorkflowID and common.ProxyFlowGRPC as the standard helpers for migrated handlers.Source: Path instructions
rest-api/api/pkg/api/handler/grpcproxy_test.go (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider checked type assertions in the mock callbacks.
testFlowProxyRequestat line 58 uses a checked assertion with a diagnostic message.testFlowProxyReplyat line 30 andtestFlowProxyDispatchat lines 45-46 use unchecked assertions. If a caller wires the mock against a different argument shape, the test panics inside the mock callback instead of reporting a clear failure. The sibling helpernewMutationProxyClientinrest-api/api/pkg/api/handler/util/common/flowmutation_test.goalready uses the checked form.♻️ Proposed alignment with the checked pattern
run.Mock.On("Get", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - args.Get(1).(*grpcproxy.Response).ResponseJSON = responseJSON + out, ok := args.Get(1).(*grpcproxy.Response) + require.True(t, ok, "Get target is %T", args.Get(1)) + out.ResponseJSON = responseJSON }).Return(nil)Run(func(args mock.Arguments) { - *started = args.Get(1).(tClient.StartWorkflowOptions) - assert.Equal(t, fullMethod, args.Get(3).(grpcproxy.Request).FullMethod) + options, ok := args.Get(1).(tClient.StartWorkflowOptions) + require.True(t, ok, "start options are %T", args.Get(1)) + *started = options + proxyReq, ok := args.Get(3).(grpcproxy.Request) + require.True(t, ok, "workflow arg is %T", args.Get(3)) + assert.Equal(t, fullMethod, proxyReq.FullMethod) }).Also applies to: 43-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/grpcproxy_test.go` around lines 29 - 31, Update the mock callbacks in testFlowProxyReply and testFlowProxyDispatch to use checked type assertions for their grpcproxy.Response arguments, including diagnostic messages consistent with testFlowProxyRequest and newMutationProxyClient. Preserve the existing callback behavior after validation.rest-api/api/pkg/api/handler/task_test.go (1)
225-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo test files hand-roll the same proxy dispatch mock.
testFlowProxyDispatchinrest-api/api/pkg/api/handler/grpcproxy_test.gocovers method pinning and option capture, but it offers no hook for asserting the decoded request. Both list-endpoint tests therefore duplicate the whole mock body to add that hook. Adding one optional callback to the shared helper removes the duplication and keeps the method assertion in a single place.
rest-api/api/pkg/api/handler/task_test.go#L225-L239: replace the inlineExecuteWorkflowmock with the extended helper, passing a callback that decodes into&flowv1.ListTasksRequest{}and callstt.assertFlowReq.rest-api/api/pkg/api/handler/taskrule_test.go#L403-L417: replace the inlineExecuteWorkflowmock with the same extended helper, decoding into&flowv1.ListOperationRulesRequest{}.♻️ Proposed helper extension in rest-api/api/pkg/api/handler/grpcproxy_test.go
// testFlowProxyDispatchWithRequest extends testFlowProxyDispatch with a hook // that runs once the proxy request is decoded, so a test can assert on the // fields the handler sent. func testFlowProxyDispatchWithRequest( t *testing.T, mockTC *tmocks.Client, run *tmocks.WorkflowRun, fullMethod string, execErr error, inspect func(args mock.Arguments), ) *tClient.StartWorkflowOptions { t.Helper() started := &tClient.StartWorkflowOptions{} mockTC.Mock.On("ExecuteWorkflow", mock.Anything, mock.Anything, grpcproxy.Flow.WorkflowName, mock.Anything). Run(func(args mock.Arguments) { *started = args.Get(1).(tClient.StartWorkflowOptions) assert.Equal(t, fullMethod, args.Get(3).(grpcproxy.Request).FullMethod) if inspect != nil { inspect(args) } }). Return(run, execErr) return started }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/task_test.go` around lines 225 - 239, Extend testFlowProxyDispatch in rest-api/api/pkg/api/handler/grpcproxy_test.go with an optional request-inspection callback while preserving its method assertion and option capture. In rest-api/api/pkg/api/handler/task_test.go lines 225-239 and rest-api/api/pkg/api/handler/taskrule_test.go lines 403-417, replace the duplicated ExecuteWorkflow mocks with the extended helper; decode requests into ListTasksRequest and ListOperationRulesRequest respectively, then invoke each test’s existing assertion callback.rest-api/api/pkg/api/handler/util/common/grpcproxy_test.go (1)
118-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a no-termination assertion to
TestExecuteGRPCProxyClassifiesLostResults.Use
TerminateWorkflowwith four argument matchers.tmocks.Clientembedstestify/mock.Mock, and the SDK recordsctx,workflowID,runID, andreasonbefore optional details. Five matchers would not match a call without details.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/util/common/grpcproxy_test.go` around lines 118 - 141, Add a no-termination assertion to TestExecuteGRPCProxyClassifiesLostResults by configuring tmocks.Client's TerminateWorkflow expectation with exactly four argument matchers for ctx, workflowID, runID, and reason, ensuring the test fails if termination is attempted without supplying optional details.rest-api/api/pkg/api/handler/rack_test.go (1)
1145-1146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutation handler tables do not pin the Flow full method. The read and validate tables were migrated to
testFlowProxyDispatch, which assertsgrpcproxy.Request.FullMethod. The mutation tables kept the pre-migrationExecuteWorkflow(mock.Anything, mock.Anything, mock.Anything, mock.Anything)stub. Because all rack and tray mutations returnSubmitTaskResponse, a handler wired to the wrong Flow method still satisfies every assertion in these tables.
rest-api/api/pkg/api/handler/rack_test.go#L1145-L1146: replace the bareExecuteWorkflowstub withtestFlowProxyDispatchpinned to the expected power-control method, and apply the same change at lines 1261, 1378, 1495, 1612, and 1714.rest-api/api/pkg/api/handler/tray_test.go#L1269-L1270: replace the bareExecuteWorkflowstub withtestFlowProxyDispatchpinned to the expected tray mutation method, and apply the same change at lines 1380, 1497, and 1601.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/rack_test.go` around lines 1145 - 1146, The mutation test tables currently use unconstrained ExecuteWorkflow stubs, so they do not verify the Flow RPC method. In rack_test.go at lines 1145-1146, 1261, 1378, 1495, 1612, and 1714, replace each stub with testFlowProxyDispatch pinned to the expected power-control method; make the same replacement in tray_test.go at lines 1269-1270, 1380, 1497, and 1601, pinning each to its expected tray mutation method.rest-api/api/pkg/api/handler/tray_test.go (1)
1114-1131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
slotIdcase to theValidateTraysHandlertable.
ValidateTraysHandlerissues two proxy calls whenslotIdis set: firstFlow_GetComponentsthroughresolveTrayIDsBySlot, thenFlow_ValidateComponents. This table registers a single dispatch expectation pinned toFlow_ValidateComponents, so the slot branch is never exercised.The uncovered branch includes the empty-result short circuit and the error mapping that I flagged in
rest-api/api/pkg/api/handler/tray.golines 58-66. A case withslotIdset would pin both dispatches and guard the resolution path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/tray_test.go` around lines 1114 - 1131, The ValidateTraysHandler test table lacks coverage for the slotId resolution path. Add a slotId case that configures expectations for both Flow_GetComponents and Flow_ValidateComponents through the existing proxy mock helpers, including the empty-result and resolution-error outcomes, while preserving the current validation-only cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/api/pkg/api/handler/taskrule.go`:
- Around line 163-174: The fresh workflow IDs in CreateTaskRule and
UpdateTaskRule incorrectly use WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING; change
both sites in rest-api/api/pkg/api/handler/taskrule.go (anchor lines 163-174 and
sibling lines 455-464) to pass WORKFLOW_ID_CONFLICT_POLICY_UNSPECIFIED. Keep the
UUID-based IDs unchanged; only use a deterministic ruleID/request-derived ID if
update retries are intentionally meant to coalesce.
In `@rest-api/api/pkg/api/handler/tray.go`:
- Around line 58-66: Update the slot-resolution helper containing the
ExecuteFlowGRPC call to return the existing *cutil.APIError directly instead of
wrapping it with fmt.Errorf. In ValidateTraysHandler, handle
resolveTrayIDsBySlot errors by logging resolveErr.LogCause() and returning
cutil.NewAPIErrorResponse with resolveErr.Code and resolveErr.Message,
preserving gateway-timeout and cancellation statuses.
- Around line 224-229: Update the ProxyFlowGRPC calls for both tray read
endpoints in tray.go to use
temporalEnums.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING instead of
WORKFLOW_ID_CONFLICT_POLICY_UNSPECIFIED, preserving their deterministic workflow
IDs and allowing identical concurrent requests to attach to the existing
workflow.
In `@rest-api/skills/rest-flow-grpc-proxy/SKILL.md`:
- Around line 98-108: Update the timeout comparison text in the documentation so
“bring-up and firmware declared 5” explicitly states “5 minutes,” while
preserving the surrounding migration and retry behavior details.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/grpcproxy_test.go`:
- Around line 29-31: Update the mock callbacks in testFlowProxyReply and
testFlowProxyDispatch to use checked type assertions for their
grpcproxy.Response arguments, including diagnostic messages consistent with
testFlowProxyRequest and newMutationProxyClient. Preserve the existing callback
behavior after validation.
In `@rest-api/api/pkg/api/handler/rack_test.go`:
- Around line 1145-1146: The mutation test tables currently use unconstrained
ExecuteWorkflow stubs, so they do not verify the Flow RPC method. In
rack_test.go at lines 1145-1146, 1261, 1378, 1495, 1612, and 1714, replace each
stub with testFlowProxyDispatch pinned to the expected power-control method;
make the same replacement in tray_test.go at lines 1269-1270, 1380, 1497, and
1601, pinning each to its expected tray mutation method.
In `@rest-api/api/pkg/api/handler/task_test.go`:
- Around line 225-239: Extend testFlowProxyDispatch in
rest-api/api/pkg/api/handler/grpcproxy_test.go with an optional
request-inspection callback while preserving its method assertion and option
capture. In rest-api/api/pkg/api/handler/task_test.go lines 225-239 and
rest-api/api/pkg/api/handler/taskrule_test.go lines 403-417, replace the
duplicated ExecuteWorkflow mocks with the extended helper; decode requests into
ListTasksRequest and ListOperationRulesRequest respectively, then invoke each
test’s existing assertion callback.
In `@rest-api/api/pkg/api/handler/taskrun_test.go`:
- Around line 163-167: Update the create-request test around the captured
`started` workflow execution to submit two valid create requests, capture each
resulting workflow ID, and assert they differ while retaining the existing
prefix and conflict-policy assertions. Organize the coverage with the existing
task-run production behavior test structure rather than validating only a single
request.
In `@rest-api/api/pkg/api/handler/tray_test.go`:
- Around line 1114-1131: The ValidateTraysHandler test table lacks coverage for
the slotId resolution path. Add a slotId case that configures expectations for
both Flow_GetComponents and Flow_ValidateComponents through the existing proxy
mock helpers, including the empty-result and resolution-error outcomes, while
preserving the current validation-only cases.
In `@rest-api/api/pkg/api/handler/util/common/grpcproxy_test.go`:
- Around line 118-141: Add a no-termination assertion to
TestExecuteGRPCProxyClassifiesLostResults by configuring tmocks.Client's
TerminateWorkflow expectation with exactly four argument matchers for ctx,
workflowID, runID, and reason, ensuring the test fails if termination is
attempted without supplying optional details.
In `@rest-api/skills/rest-flow-grpc-proxy/SKILL.md`:
- Around line 42-47: Clarify the ExecuteFlowGRPC guidance in the documented
helper section: define its exception by return behavior, stating that it should
be used directly when the caller must return a plain error, as in
resolveTrayIDsBySlot. Keep common.FlowWorkflowID and common.ProxyFlowGRPC as the
standard helpers for migrated handlers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0b942bfd-47ed-4cae-a22a-a2329d348f67
⛔ Files ignored due to path filters (5)
rest-api/sdk/standard/api_rack.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_rule.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_task.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_task_run.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_tray.gois excluded by!rest-api/sdk/standard/api_*.go
📒 Files selected for processing (21)
rest-api/AGENTS.mdrest-api/api/pkg/api/handler/grpcproxy_test.gorest-api/api/pkg/api/handler/machinepower.gorest-api/api/pkg/api/handler/rack.gorest-api/api/pkg/api/handler/rack_test.gorest-api/api/pkg/api/handler/task.gorest-api/api/pkg/api/handler/task_test.gorest-api/api/pkg/api/handler/taskrule.gorest-api/api/pkg/api/handler/taskrule_test.gorest-api/api/pkg/api/handler/taskrun.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/api/pkg/api/handler/tray.gorest-api/api/pkg/api/handler/tray_test.gorest-api/api/pkg/api/handler/util/common/common.gorest-api/api/pkg/api/handler/util/common/flowmutation_test.gorest-api/api/pkg/api/handler/util/common/grpcproxy.gorest-api/api/pkg/api/handler/util/common/grpcproxy_test.gorest-api/common/pkg/util/api.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/skills/rest-flow-grpc-proxy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- rest-api/api/pkg/api/handler/taskrun.go
| proxyErr := common.ProxyFlowGRPC( | ||
| ctx, c, logger, stc, | ||
| flowv1.Flow_GetComponentInfoByID_FullMethodName, | ||
| flowRequest, &flowResponse, | ||
| common.FlowWorkflowID(fmt.Sprintf("tray-get-%s", trayStrID)), temporalEnums.WORKFLOW_ID_CONFLICT_POLICY_UNSPECIFIED, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare Flow proxy conflict policies across handlers and recover the pre-migration tray policy.
set -euo pipefail
echo "=== Conflict policies at all Flow proxy call sites ==="
rg -n -B 6 'WORKFLOW_ID_CONFLICT_POLICY_(USE_EXISTING|UNSPECIFIED|FAIL|TERMINATE_EXISTING)' \
--type=go -g '!**/*_test.go' rest-api/api
echo
echo "=== Previous conflict policy for tray get/get-all workflows ==="
git log -1 --format=%H
git show HEAD~1:rest-api/api/pkg/api/handler/tray.go 2>/dev/null \
| rg -n -B 8 -A 2 'tray-get' || echo "previous revision not available in this checkout"Repository: NVIDIA/infra-controller
Length of output: 24029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Tray handlers and workflow IDs ==="
sed -n '180,245p' rest-api/api/pkg/api/handler/tray.go
sed -n '380,430p' rest-api/api/pkg/api/handler/tray.go
echo
echo "=== ProxyFlowGRPC implementation and conflict-policy handling ==="
rg -n -A 80 -B 15 'func ProxyFlowGRPC|executeGRPCProxy|WorkflowIDConflictPolicy' rest-api/api/pkg/api/handler/util/common rest-api --type=go -g '!**/*_test.go'
echo
echo "=== Previous tray get-all call ==="
git show HEAD~1:rest-api/api/pkg/api/handler/tray.go 2>/dev/null | sed -n '400,430p' || trueRepository: NVIDIA/infra-controller
Length of output: 50380
Use WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING for both tray read endpoints.
These endpoints use deterministic workflow IDs. UNSPECIFIED prevents concurrent identical requests from attaching and can expose workflow-start conflicts as HTTP 500 responses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/handler/tray.go` around lines 224 - 229, Update the
ProxyFlowGRPC calls for both tray read endpoints in tray.go to use
temporalEnums.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING instead of
WORKFLOW_ID_CONFLICT_POLICY_UNSPECIFIED, preserving their deterministic workflow
IDs and allowing identical concurrent requests to attach to the existing
workflow.
aa95560 to
4c6995c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rest-api/api/pkg/api/handler/grpcproxy_test.go (1)
29-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the response type assertion for clearer test failures.
Line 30 asserts
args.Get(1).(*grpcproxy.Response)without the comma-ok form. If a handler ever passes a different result target, the test panics inside the mock callback with an opaque message instead of failing on the assertion. The sibling helpernewMutationProxyClientinrest-api/api/pkg/api/handler/util/common/flowmutation_test.goalready uses the checked form.♻️ Proposed refactor for a checked assertion
run.Mock.On("Get", mock.Anything, mock.Anything).Run(func(args mock.Arguments) { - args.Get(1).(*grpcproxy.Response).ResponseJSON = responseJSON + out, ok := args.Get(1).(*grpcproxy.Response) + require.True(t, ok, "Get target is %T", args.Get(1)) + out.ResponseJSON = responseJSON }).Return(nil)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/grpcproxy_test.go` around lines 29 - 31, Update the mock callback in the Get setup to use a checked type assertion for args.Get(1), failing the test with a clear assertion message when it is not a *grpcproxy.Response before assigning ResponseJSON. Match the assertion pattern used by newMutationProxyClient.rest-api/api/pkg/api/handler/tray_test.go (1)
625-633: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree test sites rebuild a fixture reply field by field instead of passing it directly. The shared root cause is the same copy pattern: each site constructs a new response message and assigns the fields of
tt.mockResponseone at a time, although the fixture already has the required type. Beyond the duplication, any field added to the proto later is silently dropped by all three copies, so the assertions keep passing while covering less.
rest-api/api/pkg/api/handler/tray_test.go#L625-L633: replace the copiedflowv1.GetComponentsResponsewithtt.mockResponse, keeping the nil guard that supplies an empty response.rest-api/api/pkg/api/handler/tray_test.go#L835-L846: replace the six-fieldflowv1.ValidateComponentsResponsecopy withtt.mockResponse, keeping the nil guard.rest-api/api/pkg/api/handler/tray_test.go#L1123-L1134: replace the six-fieldflowv1.ValidateComponentsResponsecopy withtt.mockResponse, keeping the nil guard.♻️ Proposed refactor, shown for the first site
- if tt.mockResponse != nil { - testFlowProxyReply(t, mockWorkflowRun, &flowv1.GetComponentsResponse{ - Components: tt.mockResponse.Components, - Total: tt.mockResponse.Total, - }) - } else { - // For error cases, reply with an empty response - testFlowProxyReply(t, mockWorkflowRun, &flowv1.GetComponentsResponse{}) - } + reply := tt.mockResponse + if reply == nil { + // For error cases, reply with an empty response + reply = &flowv1.GetComponentsResponse{} + } + testFlowProxyReply(t, mockWorkflowRun, reply)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/api/pkg/api/handler/tray_test.go` around lines 625 - 633, In rest-api/api/pkg/api/handler/tray_test.go#L625-L633, update the GetComponents response setup to pass tt.mockResponse directly while preserving the nil guard and empty-response fallback. Apply the same change at rest-api/api/pkg/api/handler/tray_test.go#L835-L846 and `#L1123-L1134` for the ValidateComponents response setup, replacing each field-by-field copy with tt.mockResponse; no other behavior should change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/api/pkg/api/handler/tray.go`:
- Around line 229-236: Update both common.ProxyFlowGRPC call sites in
rest-api/api/pkg/api/handler/tray.go at lines 229-236 and 419-426 to use
temporalEnums.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING, preserving the
deterministic tray-get-<id> and hash-derived tray-get-all-<hash> workflow IDs.
If retaining UNSPECIFIED is intentional, document the reason with comments at
both sites.
---
Nitpick comments:
In `@rest-api/api/pkg/api/handler/grpcproxy_test.go`:
- Around line 29-31: Update the mock callback in the Get setup to use a checked
type assertion for args.Get(1), failing the test with a clear assertion message
when it is not a *grpcproxy.Response before assigning ResponseJSON. Match the
assertion pattern used by newMutationProxyClient.
In `@rest-api/api/pkg/api/handler/tray_test.go`:
- Around line 625-633: In rest-api/api/pkg/api/handler/tray_test.go#L625-L633,
update the GetComponents response setup to pass tt.mockResponse directly while
preserving the nil guard and empty-response fallback. Apply the same change at
rest-api/api/pkg/api/handler/tray_test.go#L835-L846 and `#L1123-L1134` for the
ValidateComponents response setup, replacing each field-by-field copy with
tt.mockResponse; no other behavior should change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a8938854-d581-4b5e-b602-9f83e0c78b80
📒 Files selected for processing (9)
rest-api/api/pkg/api/handler/grpcproxy_test.gorest-api/api/pkg/api/handler/taskrule.gorest-api/api/pkg/api/handler/taskrule_test.gorest-api/api/pkg/api/handler/taskrun_test.gorest-api/api/pkg/api/handler/tray.gorest-api/api/pkg/api/handler/tray_test.gorest-api/api/pkg/api/handler/util/common/flowmutation_test.gorest-api/api/pkg/api/handler/util/common/grpcproxy_test.gorest-api/skills/rest-flow-grpc-proxy/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (5)
- rest-api/api/pkg/api/handler/util/common/grpcproxy_test.go
- rest-api/skills/rest-flow-grpc-proxy/SKILL.md
- rest-api/api/pkg/api/handler/taskrule.go
- rest-api/api/pkg/api/handler/util/common/flowmutation_test.go
- rest-api/api/pkg/api/handler/taskrule_test.go
| proxyErr := common.ProxyFlowGRPC( | ||
| ctx, c, logger, stc, | ||
| flowv1.Flow_GetComponentInfoByID_FullMethodName, | ||
| flowRequest, &flowResponse, | ||
| common.FlowWorkflowID(fmt.Sprintf("tray-get-%s", trayStrID)), temporalEnums.WORKFLOW_ID_CONFLICT_POLICY_UNSPECIFIED, | ||
| ) | ||
| if proxyErr != nil { | ||
| return proxyErr |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Both tray read endpoints pair a deterministic workflow ID with no conflict policy. The shared root cause is one policy decision: these two calls keep WORKFLOW_ID_CONFLICT_POLICY_UNSPECIFIED while every other read endpoint migrated in this cohort uses WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING with a deterministic ID. Under UNSPECIFIED, concurrent identical reads cannot attach to the in-flight execution, and the start conflict surfaces to the client as a 500.
rest-api/api/pkg/api/handler/tray.go#L229-L236: change the conflict policy on line 233 totemporalEnums.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING, keeping thetray-get-<id>workflow ID.rest-api/api/pkg/api/handler/tray.go#L419-L426: change the conflict policy on line 423 totemporalEnums.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING, keeping the hash-derivedtray-get-all-<hash>workflow ID.
If the deviation is deliberate, record the reason in a comment at both call sites so the next reader does not treat it as an oversight.
📍 Affects 1 file
rest-api/api/pkg/api/handler/tray.go#L229-L236(this comment)rest-api/api/pkg/api/handler/tray.go#L419-L426
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/api/pkg/api/handler/tray.go` around lines 229 - 236, Update both
common.ProxyFlowGRPC call sites in rest-api/api/pkg/api/handler/tray.go at lines
229-236 and 419-426 to use
temporalEnums.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING, preserving the
deterministic tray-get-<id> and hash-derived tray-get-all-<hash> workflow IDs.
If retaining UNSPECIFIED is intentional, document the reason with comments at
both sites.
| return executeGRPCProxy(ctx, stc, grpcproxy.Flow, fullMethod, req, resp, workflowID, conflictPolicy, secretKey, secretFields...) | ||
| } | ||
|
|
||
| // FlowWorkflowID namespaces a derived workflow ID under the Flow gRPC proxy, |
There was a problem hiding this comment.
It is kind of weird to have flow-grpc and core-grpc related codes in this shared package.
There was a problem hiding this comment.
Agreed, and it predates the proxy: util/common/common.go already holds ExecutePowerControlWorkflow, ExecuteBringUpRackWorkflow, ExecuteFirmwareUpdateWorkflow and GetFlowUUIDPtr on the Flow side and AuthorizeProviderSiteForCore on the Core side, and ExecuteCoreGRPC landed here in #4560. Splitting the backend-specific helpers into their own package is worth doing, but as its own change rather than folded into a migration this size. Will address in follow-up.
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
Overall looks good, a few concerns/suggestions.
| // there is. Callers log this rather than Data directly because Data is empty | ||
| // whenever the cause was already folded into Message, and logging it blind | ||
| // would drop the reason entirely. | ||
| func (a *APIError) LogCause() error { |
There was a problem hiding this comment.
Unwrap is the appropriate method name. I would update the comment to say:
// Unwrap returns the underlying internal error contained in `Data`
// when not `nil`. If `Data` is `nil`, the APIError object is returned.
There was a problem hiding this comment.
Did not rename it: in Go, Unwrap() error is an interface the standard library claims. errors.Is and errors.As find it by signature and walk it in a loop that exits only on nil, with no cycle detection (src/errors/wrap.go, func is). Returning the APIError itself, as the comment describes, means that walk never terminates the first time one is handed to either.
So the above comment landed as two methods rather than one:
Unwrap() errorreturnsData, nil included.*APIErrorwas a dead end forerrors.Asbefore this, so it is new rather than a rename.LogError() errorisLogCauserenamed — the old name was invented — keeping the fallback the log sites need, sinceDatais nil exactly when the diagnosis sits inMessage.
There was a problem hiding this comment.
Thanks @kunzhao-nv, can we just call it Cause then? I'm hesitant to see function named with Log prefix that actually doesn't log anything (the caller does).
There was a problem hiding this comment.
Agreed on the Log prefix — it named what the caller does with the value, not what the function does. Renamed, though not to Cause: that repeats the problem Unwrap had, one library down.
github.com/pkg/errors, a direct dependency here, resolves Cause exactly the way the standard library resolves Unwrap — by interface, in a loop with no cycle detection (errors.go, func Cause):
for err != nil {
cause, ok := err.(causer)
if !ok {
break
}
err = cause.Cause()
}An APIError that returns itself never leaves that loop. To be precise about the exposure: nothing in this repo calls errors.Cause today, so it would not hang today. The name is what arms it, by advertising participation in a protocol we cannot satisfy.
Went with Diagnosis, which names what comes back without claiming where it lives — and that is the whole point, since it is either Data or the APIError whose Message is the entire diagnosis:
// Diagnosis returns the error worth logging: the internal cause, or a itself
// when the cause was folded into Message and Unwrap is therefore nil.
func (a *APIError) Diagnosis() errorReason and Source were the other candidates and both collide in this package: Reason already means a bounded status code (OperationRunStatusReason, ComponentOperationStatus.Reason), and APIError has a Source field, which a method cannot share a name with.
| // a proxy request from attaching, under USE_EXISTING, to a bespoke per-method | ||
| // execution of the same derived name: those still run on the site agent, and | ||
| // their result is a type this proxy cannot decode. | ||
| func FlowWorkflowID(derived string) string { |
There was a problem hiding this comment.
Using this effectively changes IDs for all workflows. Do we need to use flow-grpc prefix? The workflow names that are currently defined are descriptive enough.
I would recommend not using this but open to hear your reasoning. The workflow ID is most useful when viewing details in Temporal UI or CLI. A common prefix for all workflows will add a lot of noise when we already know the context.
There was a problem hiding this comment.
The prefix is not for readability. Dropping it could be silently unsafe while both workflow types exist, which is the only reason it is there.
On main these handlers already start with WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING (rack.go, task.go, taskrule.go, taskrun.go, tenantidentity.go) and derive exactly the ID strings this PR derives. Mid-upgrade, with no prefix:
- an old replica submits
rack-get-<id>with USE_EXISTING and attaches to theInvokeFlowGRPCexecution a new replica already started; - that execution returns
grpcproxy.Response, a plain struct, so the payload is encodedjson/plain; - the old replica decodes it into
*flowv1.GetRackResponseviaJSONPayloadConverter, i.e.json.Unmarshalinto a generated proto struct; - no
responseJsonfield exists there, unknown fields are ignored, and the client gets 200 with an empty body. Nothing errors.
The reverse direction is loud (ErrTypeNotImplementProtoMessage, then 500), but this one serves a wrong answer to a client.
On the UI cost: those are exactly the releases in which rack-get-<id> is ambiguous between the two implementations, and the prefix is what resolves it. Afterwards I agree it earns nothing, so I would treat it as a rollout device with an expiry rather than a convention: drop it in the release that retires the bespoke workflows, which the Rollout Requirement section already schedules. That is safe, because by then both sides of any collision are InvokeFlowGRPC. Happy to write the expiry into the doc comment, or to drop the prefix now if we would rather accept the rollout window.
There was a problem hiding this comment.
I see, that's a valid point. As long as we remove it later, this seems reasonable.
A DeadlineExceeded in the workflow result was read as proof the caller had stopped waiting, but it can also come from inside a live execution. Only wfCtx carries that evidence, and a Temporal timeout is the stronger signal when both hold, so it is classified first. Neither case terminates the execution: the activity does not heartbeat, so cancellation cannot reach an in-flight RPC, and freeing a deterministic ID would let a retry start a duplicate mutation instead of attaching through USE_EXISTING. A caller that goes away during the start now reports 504 rather than 500. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
4c6995c to
34c3ddd
Compare
| // a proxy request from attaching, under USE_EXISTING, to a bespoke per-method | ||
| // execution of the same derived name: those still run on the site agent, and | ||
| // their result is a type this proxy cannot decode. | ||
| func FlowWorkflowID(derived string) string { |
There was a problem hiding this comment.
I see, that's a valid point. As long as we remove it later, this seems reasonable.
| // there is. Callers log this rather than Data directly because Data is empty | ||
| // whenever the cause was already folded into Message, and logging it blind | ||
| // would drop the reason entirely. | ||
| func (a *APIError) LogCause() error { |
There was a problem hiding this comment.
Thanks @kunzhao-nv, can we just call it Cause then? I'm hesitant to see function named with Log prefix that actually doesn't log anything (the caller does).
FlowWorkflowID namespaces a derived workflow ID under flow-grpc-. The derivation rules are unchanged, but the resulting string has to differ: a deterministic ID plus USE_EXISTING attaches to whichever execution already holds that name, and the bespoke per-method workflows stay registered during the rollout, so a collision would hand the proxy a payload it cannot decode. ProxyFlowGRPC renders the Echo response so handlers report Flow's own status and message instead of a generic wrapper. The cause is logged and kept out of the body, where an error marshals to an empty object that tells a client nothing and contradicts the null the schema documents. APIError.LogCause chooses what to log, because Data is empty whenever the reason was folded into Message and reading it blind drops the reason entirely; logAPIError now shares that rule instead of keeping a copy. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
…proxy Replaces sixteen bespoke Temporal workflow types with the generic Flow proxy. Each call site keeps the workflow ID its bespoke workflow derived and the conflict policy that went with it, so read dedup and create freshness are unchanged; only the namespace and the workflow type differ. testFlowProxyReply, testFlowProxyDispatch and testFlowProxyRequest let the handler tests assert the proxied method and decoded request rather than a per-method workflow name. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Replaces the remaining ten bespoke Temporal workflow types, completing the move of all 26 Flow-backed endpoints onto the generic proxy. The rack and tray handlers share ExecutePowerControlWorkflow, ExecuteBringUpRackWorkflow and ExecuteFirmwareUpdateWorkflow, so the handlers and those helpers have to move together. The mutating paths lose their timeout termination, which the proxy deliberately does not do, and their activity retries: the proxy activity runs with MaximumAttempts 1 so a mutation is never resubmitted to Flow behind the caller's back. A timeout now reports 504 rather than 500. flowmutation_test.go pins the proxied method, workflow ID, conflict policy and decoded request for each of the three helpers, which the handler tests had been matching loosely. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Points contributors at ProxyFlowGRPC instead of a bespoke workflow per method, and states in the skill why a lost result leaves the execution running rather than terminating it. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
The proxy reports a lost result as 504 where the bespoke workflows reported 500, so the 35 affected operations now document it. GatewayTimeoutError tells a client the two cases apart: a read is safe to retry, while a mutation may still be running at the site and should be polled first, because retrying into a live call attaches to it rather than starting a second one. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
Covers the 504 declarations added to the Flow-backed operations. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
…un-handlers Signed-off-by: Kun Zhao <kunzhao@nvidia.com> # Conflicts: # rest-api/docs/index.html
Covers fd679a6, whose rendering the merge resolution dropped in favour of main's. Signed-off-by: Kun Zhao <kunzhao@nvidia.com>
34c3ddd to
9d5d328
Compare
4 participants
Description
ExecutePowerControlWorkflow,ExecuteBringUpRackWorkflow, andExecuteFirmwareUpdateWorkflowhelpers, which together cover the power, bring-up, and firmware endpoints. Nothing underapi/still submits a bespoke Flow workflow type.common.FlowWorkflowIDandcommon.ProxyFlowGRPCso the five handler files share the namespacing and dispatch instead of repeating them.resolveTrayIDsBySlotis not a handler and returns a plainerror, so it callsExecuteFlowGRPCdirectly.flow-grpc-. The derivation rules and conflict policies are unchanged, but the resulting string has to differ: a deterministic ID plusUSE_EXISTINGattaches to whichever execution already holds that name, and the bespoke workflows are still registered during the rollout, so a collision would hand the proxy a payload it cannot decode.Dataempty, so the log line carried no error at all, while a timeout put a wrapped internal error inDatathat serialized as{}against a spec promisingnull.APIError.LogCausenow picks the cause for the log, and the body always answersdata: null.504on the 35 affected operations through a newGatewayTimeoutErrorresponse, and regenerates the OpenAPI docs and Go SDK.Related issues
Closes #4271. Builds on #4560.
Type of Change
Breaking Changes
A Flow-backed request that outruns the site budget now answers
504where it answered500, and the budget itself narrowed: the proxy gives up around 45s, so a call that used to succeed between 41s and 49s now fails. Clients that match on500to recognize a site timeout have to accept504, which the spec and the regenerated SDK now declare. Mutating calls also lose their activity-level retry, so a transient site error reaches the caller instead of being absorbed. A504is not a rollback: the operation may have completed, may still be running, or may have timed out at the site. Read-only operations are safe to retry; reconcile a mutating one against the affected resource or its Tasks before retrying, since a create may have succeeded without ever returning its new resource ID.Testing