From 25fde3d9b46989d8e001221f352b21e670193876 Mon Sep 17 00:00:00 2001 From: Henrique Costa Date: Mon, 4 May 2026 12:48:49 +0200 Subject: [PATCH] fix: loop back retry-style error handler tail to a merge before the source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a call activity with a custom error handler has a body ending in an IF where exactly one branch terminates (via RAISE ERROR or RETURN), the authored Studio Pro topology uses a merge placed before the source activity with the non-terminating branch looping back to that merge — implementing a retry pattern. MDL has no surface syntax for the loop-back edge, so previously the builder emitted a linear topology that placed the merge after the source, which moved subsequent activities' references to the call's output variable out of scope and triggered CE0108 "Variable X is defined but not in scope" in mx check. Detect the pattern via isRetryLoopErrorHandler (last statement is an IF with exactly one terminating branch) and build the loop-back topology directly in buildRetryLoopErrorHandler: a new merge is placed just left of the source activity, the handler tail's exit connects to the merge as a plain SequenceFlow (the IsErrorHandler edge is already on the source → first-handler-activity connection), and fb.incomingRedirect redirects the outer loop's inbound flow to terminate at the merge. A repro (mxcli exec of a rest-call-with-retry microflow into a blank MPR, then mx check) now reports 0 errors on the simple case; before this fix it reported CE0108. Non-retry error handlers still use the existing forward-merge path. Related: tracked in #506. Co-Authored-By: Claude Opus 4.7 (1M context) --- mdl/executor/cmd_microflows_builder.go | 7 + mdl/executor/cmd_microflows_builder_flows.go | 102 +++++++++ mdl/executor/cmd_microflows_builder_graph.go | 14 +- .../cmd_microflows_retry_loop_test.go | 213 ++++++++++++++++++ 4 files changed, 333 insertions(+), 3 deletions(-) create mode 100644 mdl/executor/cmd_microflows_retry_loop_test.go diff --git a/mdl/executor/cmd_microflows_builder.go b/mdl/executor/cmd_microflows_builder.go index 829522b3..91ee9c29 100644 --- a/mdl/executor/cmd_microflows_builder.go +++ b/mdl/executor/cmd_microflows_builder.go @@ -32,6 +32,13 @@ type flowBuilder struct { errors []string // Validation errors collected during build measurer *layoutMeasurer // For measuring statement dimensions nextConnectionPoint model.ID // For compound statements: the exit point differs from entry point + // incomingRedirect, when set, instructs the next enclosing flow emission to + // terminate at this merge/activity ID instead of the most recently emitted + // activity. Used by retry-loop error handlers (where a merge is inserted + // before the source activity so the normal inbound flow enters the merge + // and the error-handler tail loops back to the same merge). Cleared after + // the outer loop consumes it for the next flow. + incomingRedirect model.ID nextFlowCase string // If set, next connecting flow uses this case value (for merge-less splits) // nextFlowAnchor carries the branch-specific FlowAnchors that should be // applied to the flow created by the NEXT iteration of buildFlowGraph. diff --git a/mdl/executor/cmd_microflows_builder_flows.go b/mdl/executor/cmd_microflows_builder_flows.go index d9a621d7..ce9366b1 100644 --- a/mdl/executor/cmd_microflows_builder_flows.go +++ b/mdl/executor/cmd_microflows_builder_flows.go @@ -51,6 +51,15 @@ func (fb *flowBuilder) finishCustomErrorHandler(activityID model.ID, activityX i return } if len(eh.Body) > 0 { + // Retry-loop pattern: error body ends with an IF whose non-terminating + // branch should loop back to the source activity (Studio Pro authors + // this as a merge placed before the source, with the handler tail + // returning to that merge). MDL cannot express the loop-back directly, + // so we detect the shape and wire the topology ourselves. + if isRetryLoopErrorHandler(eh.Body) { + fb.buildRetryLoopErrorHandler(activityID, activityX, eh.Body) + return + } mergeID := fb.addErrorHandlerFlow(activityID, activityX, eh.Body) fb.handleErrorHandlerMergeWithSkip(mergeID, activityID, outputVar) return @@ -58,6 +67,99 @@ func (fb *flowBuilder) finishCustomErrorHandler(activityID model.ID, activityX i fb.registerEmptyCustomErrorHandlerWithSkip(activityID, eh, outputVar) } +// isRetryLoopErrorHandler reports whether the error-handler body looks like +// a retry loop: the last statement is an IF whose else branch terminates +// (via RAISE ERROR or RETURN) and whose then branch continues. That shape +// mirrors the Studio Pro retry pattern where the non-terminating branch +// loops back to the source activity to re-attempt it. +func isRetryLoopErrorHandler(body []ast.MicroflowStatement) bool { + if len(body) == 0 { + return false + } + ifStmt, ok := body[len(body)-1].(*ast.IfStmt) + if !ok { + return false + } + if len(ifStmt.ThenBody) == 0 || len(ifStmt.ElseBody) == 0 { + return false + } + thenTerminates := bodyTerminates(ifStmt.ThenBody) + elseTerminates := bodyTerminates(ifStmt.ElseBody) + // Exactly one branch must terminate (the "error" branch) and one must + // continue (the "retry" branch). If both or neither terminate, the shape + // is some other IF and the standard merge-forward path still applies. + return thenTerminates != elseTerminates +} + +// bodyTerminates reports whether a statement body ends with a terminator +// (RAISE ERROR, RETURN, or nested IF where every branch terminates). +func bodyTerminates(body []ast.MicroflowStatement) bool { + if len(body) == 0 { + return false + } + last := body[len(body)-1] + switch s := last.(type) { + case *ast.RaiseErrorStmt: + return true + case *ast.ReturnStmt: + return true + case *ast.IfStmt: + if len(s.ElseBody) == 0 { + return false + } + return bodyTerminates(s.ThenBody) && bodyTerminates(s.ElseBody) + } + return false +} + +// buildRetryLoopErrorHandler wires a retry-loop topology for an error handler +// whose body ends with a terminating/continuing IF. The non-terminating +// branch's tail loops back to a new merge placed before the source activity; +// the merge then feeds into the source. The outer loop consumes +// fb.incomingRedirect so the normal inbound flow also terminates at the +// merge instead of directly at the source. +func (fb *flowBuilder) buildRetryLoopErrorHandler(sourceActivityID model.ID, sourceX int, errorBody []ast.MicroflowStatement) { + // Build the handler activities with tracking of where the non-terminating + // branch's tail ends up. We reuse addErrorHandlerFlow; it returns the tail + // that would have forwarded to the next main-path activity. For a retry + // loop, that tail is the continue-branch exit of the trailing IF. + tail := fb.addErrorHandlerFlow(sourceActivityID, sourceX, errorBody) + if tail.id == "" { + // The handler terminates unexpectedly — nothing to loop back. + return + } + + // Insert a merge just left of the source activity, on the main flow row. + merge := µflows.ExclusiveMerge{ + BaseMicroflowObject: microflows.BaseMicroflowObject{ + BaseElement: model.BaseElement{ID: model.ID(types.GenerateID())}, + Position: model.Point{X: sourceX - fb.spacing/2, Y: fb.baseY}, + Size: model.Size{Width: MergeSize, Height: MergeSize}, + }, + } + fb.objects = append(fb.objects, merge) + + // Merge -> source (normal flow into the REST/microflow call) + fb.flows = append(fb.flows, newHorizontalFlow(merge.ID, sourceActivityID)) + + // Handler tail -> merge (loop-back). Authored Studio Pro flows mark this + // edge as a normal SequenceFlow (not IsErrorHandler) — the error flow + // marker only applies to the SOURCE → first-handler-activity edge, which + // addErrorHandlerFlow already emitted. Using a plain horizontal flow here + // avoids triggering spurious CE0136/CE0019 diagnostics that fire when a + // retrieve's start variable is deemed to flow through an error edge. + loopFlow := newHorizontalFlow(tail.id, merge.ID) + if tail.caseValue != "" { + applyDeferredFlowCase(loopFlow, tail.caseValue, tail.flowAnchor) + } else if tail.flowAnchor != nil { + applyDeferredFlowCase(loopFlow, "", tail.flowAnchor) + } + fb.flows = append(fb.flows, loopFlow) + + // Next inbound normal flow must terminate at the merge, not at the source. + fb.incomingRedirect = merge.ID +} + func (fb *flowBuilder) registerEmptyCustomErrorHandlerWithSkip(activityID model.ID, eh *ast.ErrorHandlingClause, skipVar string) { if !isEmptyCustomErrorHandler(eh) { return diff --git a/mdl/executor/cmd_microflows_builder_graph.go b/mdl/executor/cmd_microflows_builder_graph.go index a3196562..13196ddd 100644 --- a/mdl/executor/cmd_microflows_builder_graph.go +++ b/mdl/executor/cmd_microflows_builder_graph.go @@ -76,13 +76,21 @@ func (fb *flowBuilder) buildFlowGraph(stmts []ast.MicroflowStatement, returns *a activityID := fb.addStatement(stmt) if activityID != "" { fb.applyPendingAnnotations(activityID) - // Connect to previous object with horizontal SequenceFlow + // Connect to previous object with horizontal SequenceFlow. + // When incomingRedirect is set (retry-loop error handler built a + // merge before the activity), the inbound flow terminates at the + // merge instead of the activity itself. + inboundTarget := activityID + if fb.incomingRedirect != "" { + inboundTarget = fb.incomingRedirect + fb.incomingRedirect = "" + } var flow *microflows.SequenceFlow if pendingCase != "" { - flow = newHorizontalFlowWithCase(lastID, activityID, pendingCase) + flow = newHorizontalFlowWithCase(lastID, inboundTarget, pendingCase) pendingCase = "" } else { - flow = newHorizontalFlow(lastID, activityID) + flow = newHorizontalFlow(lastID, inboundTarget) } // Prefer the pendingFlowAnchor (carried from a guard-pattern IF's // branch) over the previous statement's own anchor — it encodes diff --git a/mdl/executor/cmd_microflows_retry_loop_test.go b/mdl/executor/cmd_microflows_retry_loop_test.go new file mode 100644 index 00000000..d2a1362c --- /dev/null +++ b/mdl/executor/cmd_microflows_retry_loop_test.go @@ -0,0 +1,213 @@ +// SPDX-License-Identifier: Apache-2.0 + +package executor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/model" + "github.com/mendixlabs/mxcli/sdk/microflows" +) + +// Retry-loop error handler: a call activity with a custom error handler +// whose body ends with an IF where the ELSE raises and the THEN performs +// retry actions must produce a merge placed before the source activity, +// with the handler tail looping back to the merge. Without this topology +// the merge falls after the source and subsequent activities referencing +// the call's output variable trigger CE0108 "variable not in scope". +func TestRetryLoopErrorHandlerLoopsBackToSource(t *testing.T) { + fb := &flowBuilder{ + spacing: HorizontalSpacing, + measurer: &layoutMeasurer{}, + declaredVars: map[string]string{"R": "String", "RetryCount": "Integer"}, + } + + oc := fb.buildFlowGraph([]ast.MicroflowStatement{ + &ast.DeclareStmt{ + Variable: "RetryCount", + Type: ast.DataType{Kind: ast.TypeInteger}, + InitialValue: &ast.LiteralExpr{Kind: ast.LiteralInteger, Value: "0"}, + }, + &ast.CallMicroflowStmt{ + MicroflowName: ast.QualifiedName{Module: "M", Name: "Fetch"}, + OutputVariable: "R", + ErrorHandling: &ast.ErrorHandlingClause{ + Type: ast.ErrorHandlingCustomWithoutRollback, + Body: []ast.MicroflowStatement{ + &ast.LogStmt{Level: ast.LogError, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "fetch failed"}}, + &ast.IfStmt{ + Condition: &ast.BinaryExpr{ + Operator: "<", + Left: &ast.VariableExpr{Name: "RetryCount"}, + Right: &ast.LiteralExpr{Kind: ast.LiteralInteger, Value: "3"}, + }, + ThenBody: []ast.MicroflowStatement{ + &ast.MfSetStmt{ + Target: "RetryCount", + Value: &ast.BinaryExpr{ + Operator: "+", + Left: &ast.VariableExpr{Name: "RetryCount"}, + Right: &ast.LiteralExpr{Kind: ast.LiteralInteger, Value: "1"}, + }, + }, + }, + ElseBody: []ast.MicroflowStatement{ + &ast.RaiseErrorStmt{}, + }, + }, + }, + }, + }, + &ast.LogStmt{Level: ast.LogInfo, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "ok"}}, + &ast.ReturnStmt{}, + }, nil) + + // Find the call activity, the retry-body log/set activities, and the merge. + var callID, logAfterID model.ID + var mergeIDs []model.ID + logSeen := 0 + for _, obj := range oc.Objects { + switch o := obj.(type) { + case *microflows.ActionActivity: + if _, ok := o.Action.(*microflows.MicroflowCallAction); ok { + callID = o.ID + } + if _, ok := o.Action.(*microflows.LogMessageAction); ok { + logSeen++ + // logSeen==1 is the error-handler log inside the handler body. + // logSeen==2 is the main-path log after the call. + if logSeen == 2 { + logAfterID = o.ID + } + } + case *microflows.ExclusiveMerge: + mergeIDs = append(mergeIDs, o.ID) + } + } + if callID == "" { + t.Fatal("no call activity found") + } + if logAfterID == "" { + t.Fatal("no post-call log activity found") + } + if len(mergeIDs) == 0 { + t.Fatal("no merge node found — retry-loop topology was not built") + } + + // The merge must sit on the call's inbound path: there must be an edge + // prev->merge (not prev->call) and an edge merge->call. The handler + // tail loops back to the merge as a plain SequenceFlow (not marked + // IsErrorHandler — the error marker applies only to the source→first + // handler activity edge, which addErrorHandlerFlow emits separately). + var mergeToCallFound bool + var prevToMergeFound bool + var tailToMergeFound bool + // Find the retry-branch tail activity (the ChangeVariable that + // increments the retry counter in the test fixture). + var tailID model.ID + for _, obj := range oc.Objects { + if a, ok := obj.(*microflows.ActionActivity); ok { + if _, isChange := a.Action.(*microflows.ChangeVariableAction); isChange { + tailID = a.ID + } + } + } + for _, f := range oc.Flows { + for _, mID := range mergeIDs { + if f.DestinationID == callID && f.OriginID == mID { + mergeToCallFound = true + } + if f.DestinationID == mID && f.OriginID == tailID && !f.IsErrorHandler { + tailToMergeFound = true + } + if f.DestinationID == mID && !f.IsErrorHandler && f.OriginID != mID && f.OriginID != tailID { + prevToMergeFound = true + } + } + // No flow should terminate directly at the call from a non-merge origin. + if f.DestinationID == callID && !f.IsErrorHandler { + originIsMerge := false + for _, mID := range mergeIDs { + if f.OriginID == mID { + originIsMerge = true + } + } + if !originIsMerge { + t.Errorf("normal inbound flow to call %s came from non-merge origin %s; retry-loop merge was not inserted", + shortID(callID), shortID(f.OriginID)) + } + } + } + if !mergeToCallFound { + t.Error("missing merge -> call flow") + } + if !prevToMergeFound { + t.Error("missing prev -> merge normal flow") + } + if !tailToMergeFound { + t.Error("missing handler-tail -> merge (loop-back) flow") + } +} + +// Non-retry error handlers (both branches return, neither branch raises, +// or no trailing IF) must still use the forward-merge path. +func TestNonRetryErrorHandlerUsesForwardMerge(t *testing.T) { + fb := &flowBuilder{ + spacing: HorizontalSpacing, + measurer: &layoutMeasurer{}, + declaredVars: map[string]string{"R": "String"}, + } + + oc := fb.buildFlowGraph([]ast.MicroflowStatement{ + &ast.CallMicroflowStmt{ + MicroflowName: ast.QualifiedName{Module: "M", Name: "Fetch"}, + OutputVariable: "R", + ErrorHandling: &ast.ErrorHandlingClause{ + Type: ast.ErrorHandlingCustomWithoutRollback, + Body: []ast.MicroflowStatement{ + // Single log — no trailing IF, no retry shape. + &ast.LogStmt{Level: ast.LogError, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "failed"}}, + }, + }, + }, + &ast.LogStmt{Level: ast.LogInfo, Message: &ast.LiteralExpr{Kind: ast.LiteralString, Value: "ok"}}, + &ast.ReturnStmt{}, + }, nil) + + // The call should have an inbound flow from the previous activity (the + // StartEvent or a fb-generated start), NOT via a merge. If a merge were + // incorrectly inserted, every such non-retry microflow would gain an + // extra unnecessary node. + var callID model.ID + for _, obj := range oc.Objects { + if a, ok := obj.(*microflows.ActionActivity); ok { + if _, isCall := a.Action.(*microflows.MicroflowCallAction); isCall { + callID = a.ID + } + } + } + if callID == "" { + t.Fatal("no call activity") + } + for _, f := range oc.Flows { + if f.DestinationID == callID && !f.IsErrorHandler { + // Origin should be a non-merge (StartEvent or a plain activity). + for _, obj := range oc.Objects { + if obj.GetID() == f.OriginID { + if _, isMerge := obj.(*microflows.ExclusiveMerge); isMerge { + t.Errorf("non-retry handler unexpectedly inserted a merge before call") + } + } + } + } + } +} + +func shortID(id model.ID) string { + s := string(id) + if len(s) > 8 { + return s[:8] + } + return s +}