Copilot commented on code in PR #1100:
URL:
https://github.com/apache/incubator-seata-go/pull/1100#discussion_r3037032680
##########
pkg/saga/statemachine/engine/core/process_ctrl_statemachine_engine.go:
##########
@@ -412,11 +412,11 @@ func (p ProcessCtrlStateMachineEngine)
createMachineInstance(stateMachineName st
"StateMachine ["+stateMachineName+"] is not exists",
nil)
}
- stateMachineInstance := statelang.NewStateMachineInstanceImpl()
- stateMachineInstance.SetStateMachine(stateMachine)
- stateMachineInstance.SetTenantID(tenantId)
- stateMachineInstance.SetBusinessKey(businessKey)
- stateMachineInstance.SetStartParams(startParams)
+ stateMachineInstance := statelang.NewStateMachineInstance()
+ stateMachineInstance.StateMachine = stateMachine
Review Comment:
`createMachineInstance` assigns `StateMachine` but never populates
`MachineID`. Previously `SetStateMachine(...)` also set `MachineID` from
`stateMachine.ID()`, and the DB store insert/update paths rely on `MachineID`
being non-empty. Set `MachineID` when attaching the state machine (e.g., use
the new `SetStateMachineAndID` helper or assign `MachineID =
stateMachine.ID()`), otherwise persisted instances may have blank machine_id
and later reload/lookup can fail.
```suggestion
stateMachineInstance.StateMachine = stateMachine
stateMachineInstance.MachineID = stateMachine.ID()
```
##########
pkg/saga/statemachine/store/db/statelog.go:
##########
@@ -569,33 +569,33 @@ func (s *StateLogStore) isUpdateMode(stateInstance
statelang.StateInstance, cont
return false, nil
}
-func (s *StateLogStore) generateRetryStateInstanceId(stateInstance
statelang.StateInstance) string {
- originalStateInstId := stateInstance.StateIDRetriedFor()
+func (s *StateLogStore) generateRetryStateInstanceId(stateInstance
*statelang.StateInstance) string {
+ originalStateInstId := stateInstance.StateIDRetriedFor
maxIndex := 1
- machineInstance := stateInstance.StateMachineInstance()
+ machineInstance := stateInstance.StateMachineInstance
originalStateInst := machineInstance.State(originalStateInstId)
- for originalStateInst.StateIDRetriedFor() != "" {
- originalStateInst =
machineInstance.State(originalStateInst.StateIDRetriedFor())
- idIndex := s.getIdIndex(originalStateInst.ID(), ".")
+ for originalStateInst.StateIDRetriedFor != "" {
+ originalStateInst =
machineInstance.State(originalStateInst.StateIDRetriedFor)
+ idIndex := s.getIdIndex(originalStateInst.ID, ".")
if idIndex > maxIndex {
maxIndex = idIndex
}
- originalStateInstId = originalStateInst.ID()
+ originalStateInstId = originalStateInst.ID
}
return fmt.Sprintf("%s.%d", originalStateInstId, maxIndex)
}
Review Comment:
`generateRetryStateInstanceId` appears to return duplicate IDs for
subsequent retries. `getIdIndex` returns the existing numeric suffix (e.g., 1
for `origin.1`), but this function returns `fmt.Sprintf("%s.%d",
originalStateInstId, maxIndex)` without incrementing to the next available
index, and the loop walks *backward* to the original state (which often has no
suffix). This can cause collisions like repeatedly generating `origin.1` for
multiple retry attempts. Consider scanning existing retry instances for the
same origin and returning `maxIndex+1` (or otherwise guaranteeing monotonic
unique IDs).
##########
pkg/saga/statemachine/store/db/statelog.go:
##########
@@ -891,33 +891,33 @@ func (s *StateLogStore) branchReport(ctx context.Context,
stateInstance statelan
err = engExc.NewEngineExecutionException(
seataErrors.TransactionErrorCodeBranchReportFailed,
fmt.Sprintf("branch report via template failed,
stateMachine=%s, state=%s, xid=%s, branchId=%d",
-
originalStateInst.StateMachineInstance().StateMachine().Name(),
originalStateInst.Name(), globalTransaction.Xid, branchId),
+
originalStateInst.StateMachineInstance.StateMachine.Name(),
originalStateInst.Name, globalTransaction.Xid, branchId),
err,
)
return err
}
return nil
}
-func (s *StateLogStore) findOutOriginalStateInstanceOfRetryState(stateInstance
statelang.StateInstance) statelang.StateInstance {
- stateMap := stateInstance.StateMachineInstance().StateMap()
- originalStateInst := stateMap[stateInstance.StateIDRetriedFor()]
- for originalStateInst.StateIDRetriedFor() != "" {
- originalStateInst = stateMap[stateInstance.StateIDRetriedFor()]
+func (s *StateLogStore) findOutOriginalStateInstanceOfRetryState(stateInstance
*statelang.StateInstance) *statelang.StateInstance {
+ stateMap := stateInstance.StateMachineInstance.StateMap()
+ originalStateInst := stateMap[stateInstance.StateIDRetriedFor]
+ for originalStateInst.StateIDRetriedFor != "" {
+ originalStateInst = stateMap[stateInstance.StateIDRetriedFor]
}
return originalStateInst
}
-func (s *StateLogStore)
findOutOriginalStateInstanceOfCompensateState(stateInstance
statelang.StateInstance) statelang.StateInstance {
- stateMap := stateInstance.StateMachineInstance().StateMap()
- originalStateInst := stateMap[stateInstance.StateIDCompensatedFor()]
- for originalStateInst.StateIDRetriedFor() != "" {
- originalStateInst = stateMap[stateInstance.StateIDRetriedFor()]
+func (s *StateLogStore)
findOutOriginalStateInstanceOfCompensateState(stateInstance
*statelang.StateInstance) *statelang.StateInstance {
+ stateMap := stateInstance.StateMachineInstance.StateMap()
+ originalStateInst := stateMap[stateInstance.StateIDCompensatedFor]
+ for originalStateInst.StateIDRetriedFor != "" {
+ originalStateInst = stateMap[stateInstance.StateIDRetriedFor]
}
return originalStateInst
Review Comment:
`findOutOriginalStateInstanceOfCompensateState` has the same retry-chain
traversal bug as the retry variant: inside the loop it reuses
`stateInstance.StateIDRetriedFor` instead of progressing with
`originalStateInst.StateIDRetriedFor`. If the compensated/original instance is
itself a retry, this can also hang indefinitely. Update the loop to follow
`originalStateInst.StateIDRetriedFor` (and handle missing keys) so it
terminates correctly.
##########
pkg/saga/statemachine/store/db/statelog.go:
##########
@@ -891,33 +891,33 @@ func (s *StateLogStore) branchReport(ctx context.Context,
stateInstance statelan
err = engExc.NewEngineExecutionException(
seataErrors.TransactionErrorCodeBranchReportFailed,
fmt.Sprintf("branch report via template failed,
stateMachine=%s, state=%s, xid=%s, branchId=%d",
-
originalStateInst.StateMachineInstance().StateMachine().Name(),
originalStateInst.Name(), globalTransaction.Xid, branchId),
+
originalStateInst.StateMachineInstance.StateMachine.Name(),
originalStateInst.Name, globalTransaction.Xid, branchId),
err,
)
return err
}
return nil
}
-func (s *StateLogStore) findOutOriginalStateInstanceOfRetryState(stateInstance
statelang.StateInstance) statelang.StateInstance {
- stateMap := stateInstance.StateMachineInstance().StateMap()
- originalStateInst := stateMap[stateInstance.StateIDRetriedFor()]
- for originalStateInst.StateIDRetriedFor() != "" {
- originalStateInst = stateMap[stateInstance.StateIDRetriedFor()]
+func (s *StateLogStore) findOutOriginalStateInstanceOfRetryState(stateInstance
*statelang.StateInstance) *statelang.StateInstance {
+ stateMap := stateInstance.StateMachineInstance.StateMap()
+ originalStateInst := stateMap[stateInstance.StateIDRetriedFor]
+ for originalStateInst.StateIDRetriedFor != "" {
+ originalStateInst = stateMap[stateInstance.StateIDRetriedFor]
}
return originalStateInst
Review Comment:
`findOutOriginalStateInstanceOfRetryState` will loop forever when the
original instance itself has `StateIDRetriedFor != ""` because the loop
re-fetches `stateMap[stateInstance.StateIDRetriedFor]` instead of advancing
using `originalStateInst.StateIDRetriedFor`. This should traverse the retry
chain via the current `originalStateInst` (and defensively handle missing map
entries) to avoid infinite loops/hangs during branch reporting.
--
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]
