kaori-seasons commented on code in PR #897:
URL:
https://github.com/apache/incubator-seata-go/pull/897#discussion_r2377477530
##########
pkg/saga/statemachine/engine/repo/repository/state_machine_repository.go:
##########
@@ -71,6 +71,14 @@ func GetStateMachineRepositoryImpl()
*StateMachineRepositoryImpl {
return stateMachineRepositoryImpl
}
+func (s *StateMachineRepositoryImpl) SetDefaultTenantId(tenantId string) {
+ s.defaultTenantId = tenantId
+}
+
+func (s *StateMachineRepositoryImpl) GetDefaultTenantId() string {
+ return s.defaultTenantId
Review Comment:
pkg/saga/statemachine/engine/config/default_statemachine_config.go
pkg/saga/statemachine/engine/repo/repository/state_machine_repository.go
Issue
The tenant field is only set in some registration/loading locations. State
instances, compensation chains, logs, and other functions do not fully carry
tenant information, which may lead to cross-tenancy.
Suggestion
Add the TenantId field to structures such as StateMachineInstance and
StateInstance. All persistence, logs, and compensation chains carry tenant
information.
All methods, such as GetStateMachineByNameAndTenantId and
RegistryStateMachine, must pass and verify tenant information.
##########
pkg/saga/statemachine/engine/pcext/state_router_impl.go:
##########
@@ -53,15 +68,81 @@ func (t TaskStateRouter) Route(ctx context.Context,
processContext process_ctrl.
return nil, nil
}
- // The current CompensationTriggerState can mark the compensation
process is started and perform compensation
- // route processing.
- compensationTriggerState, ok :=
processContext.GetVariable(constant.VarNameCurrentCompensateTriggerState).(statelang.State)
- if ok {
+ // If current state is CompensationTrigger or compensation has been
flagged, start compensation routing
+ if state != nil && state.Type() ==
constant.StateTypeCompensationTrigger {
+ // flag into context for downstream calls (best-effort)
+ if hpc, ok :=
processContext.(process_ctrl.HierarchicalProcessContext); ok {
+
hpc.SetVariableLocally(constant.VarNameCurrentCompensateTriggerState, state)
+
hpc.SetVariableLocally(constant.VarNameFirstCompensationStateStarted, false)
+ } else {
+
processContext.SetVariable(constant.VarNameCurrentCompensateTriggerState, state)
+
processContext.SetVariable(constant.VarNameFirstCompensationStateStarted, false)
+ }
+
+ // build compensation stack from executed forward states
(latest first)
+ smInst :=
processContext.GetVariable(constant.VarNameStateMachineInst).(statelang.StateMachineInstance)
+ holder := GetCurrentCompensationHolder(ctx, processContext,
true)
+ stack := holder.StateStackNeedCompensation()
+ sm :=
processContext.GetVariable(constant.VarNameStateMachine).(statelang.StateMachine)
+ states := smInst.StateList()
+ for i := len(states) - 1; i >= 0; i-- {
+ si := states[i]
+ // exclude compensation states
+ if si.StateIDCompensatedFor() != "" {
+ continue
+ }
+ // only successful forward states are subject to
compensation
+ if si.Status() != statelang.SU {
+ continue
+ }
+ originName := GetOriginStateName(si)
+ def := sm.State(originName)
+ // ensure the definition has a compensate state
+ var task *sagaState.AbstractTaskState
+ switch s := def.(type) {
+ case *sagaState.ServiceTaskStateImpl:
+ task = s.AbstractTaskState
+ case *sagaState.ScriptTaskStateImpl:
+ task = s.AbstractTaskState
+ case *sagaState.SubStateMachineImpl:
+ if s.ServiceTaskStateImpl != nil {
+ task =
s.ServiceTaskStateImpl.AbstractTaskState
+ }
+ }
+ if task == nil || task.CompensateState() == "" {
+ continue
+ }
+ stack.Push(si)
+ }
+ // fallback: if nothing pushed, try last successful forward
state
+ if stack.Empty() {
+ for i := len(states) - 1; i >= 0; i-- {
+ si := states[i]
+ if si.StateIDCompensatedFor() != "" {
+ continue
+ }
+ if si.Status() != statelang.SU {
+ continue
+ }
+ stack.Push(si)
+ break
+ }
+ }
+
+ // mark machine compensation running
+ smInst.SetCompensationStatus(statelang.RU)
+
+ return t.compensateRoute(ctx, processContext, state)
Review Comment:
In the core method Route in process_router.go , the routing logic checks the
state type currently executed by the state machine:
- If it is a normal Task state, the StateInstruction determines the next
state to execute based on it.
- If it is a Choice state, the routing logic first selects a "next" branch
using an expression or branch condition and updates it to the StateInstruction:
```
stateInstruction.SetStateName(next)
```
This allows the state machine to execute different branches based on the
Choice condition.
Specifically, the branch selection result of the Choice state is assigned to
the corresponding field of the StateInstruction (such as StateName), thus
affecting subsequent processes.
#### Existing Issues
The compensation chain is constructed only backwards from the execution
stack. If there is no compensable state after the Choice/Failure branch, the
process simply marks it as NoCompensation, but the final state normalization is
not explicit enough.
Multi-branch, multi-stage compensation, and compensation failure lack
fine-grained state recording. Recommendations
#### Recommendations
When building a compensation chain, traverse the state list to clearly mark
the branch source and compensation result of each compensated state. We
recommend adding a new structure such as CompensationTrace[]CompensationResult,
which includes the branch, compensation status, and exception.
When a Fail End is reached without compensation, the state is normalized to
FA and the compensation status is cleared. We recommend also exposing the
compensation chain record to the instance for easy querying.
When compensating multiple branches, it is recommended that each branch
record its compensation result separately, and the final state is normalized
based on the compensation results of all branches.
Scenario Description
Assume the following SAGA state machine definition:
Code
Start -> Choice
Choice:
if (A) -> Service1
else -> Service2
Service1 -> Service3
Service2 -> Service4
Service3/Service4 -> FailEnd
FailEnd: Compensation triggers
Data Input Example (Choice condition is false, branch to Service2)
Initial Business Parameters:
```{"userId": 111, "amount": 100, "route": "B"}```
State Machine Execution Path:
```Start → Choice (route == B, choose Service2) → Service2 → Service4 →
FailEnd```
#### Boundary Issues
- Issue 1: If the compensation chain is not constructed rigorously, FailEnd
triggers compensation, but the compensation chain incorrectly includes the
unexecuted Service1/Service3 branches.
- Issue 2: If Service4 does not define a compensation state, the
compensation chain is empty. In this case, the compensation chain is not
properly normalized, resulting in the final state machine instance remaining in
"Compensation in Progress" or "Unknown" instead of "Failure without
Compensation."
Data output without boundary processing (pseudo-JSON)
```
{
"stateMachineInstance": {
"id": "saga-123",
"status": "FA",
"compensationStatus": "RU",
"compensationChain": [
{"state": "Service1", "executed": false}, // 不该出现
{"state": "Service2", "executed": true, "compensated": false},
{"state": "Service4", "executed": true, "compensated": false}
],
"exception": "补偿链错误包含了未执行分支"
}
}
```
Issue: The compensation chain incorrectly includes unexecuted branches, and
the final state machine instance compensationStatus is not normalized (still
RU), misleading the API/Ops layer into thinking compensation is still in
progress.
Data output after boundary processing
```
{
"stateMachineInstance": {
"id": "saga-123",
"status": "FA",
"compensationStatus": "",
"compensationChain": [
{"state": "Service2", "executed": true, "compensated": false},
{"state": "Service4", "executed": true, "compensated": false}
],
"exception": null
}
}
```
Unexecuted branches (Service1) removed
The compensation chain strictly includes only states that are "actually
executed and have compensation defined."
If no compensation is actually made (Service4 has no compensation defined),
the compensationStatus field is initialized to null, the status field is
initialized to "failed" (FA), the exception field is empty, and the process is
explicitly terminated.
##########
pkg/saga/statemachine/engine/pcext/process_handler.go:
##########
@@ -53,18 +59,29 @@ func NewStateMachineProcessHandler()
*StateMachineProcessHandler {
}
func (s *StateMachineProcessHandler) Process(ctx context.Context,
processContext process_ctrl.ProcessContext) error {
- stateInstruction, _ :=
processContext.GetInstruction().(StateInstruction)
+ var stateInstruction *StateInstruction
+ switch v := processContext.GetInstruction().(type) {
+ case *StateInstruction:
+ stateInstruction = v
+ case StateInstruction:
+ tmp := v
+ stateInstruction = &tmp
+ default:
+ return errors.New("invalid state instruction from
processContext")
Review Comment:
#### Issue
Exceptions in operations such as branchRegister/branchReport/finish are only
logged, lacking visibility. Some return values are not fully propagated,
making them difficult for the API and upper layers to detect.
Optimistic lock update failures (affected=0) only result in a fallback, but
this is not exposed to the state machine instance or API layer.
#### Recommendation
When all key DB operations/TC interactions (branchRegister, Report, and
Finish) fail, the error object should be written to the
StateMachineInstance.Exception field and exposed in the API layer.
When branchRegister fails (e.g., due to an invalid branchId), the process
should be terminated immediately, the state should be set to FA, the exception
should be written to the instance, and an error should be returned.
In fallback update scenarios, if the failure persists after a reload, it is
recommended that the exception be written to the state machine instance and
propagated to avoid silent failure.
When compensation fails, the failure state and exception should be written
to the compensation state instance and appended to the
StateMachineInstance.Exception list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
