Support loop element resolution during compensation - #8127
Support loop element resolution during compensation#8127somiljain2006 wants to merge 15 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #8127 +/- ##
============================================
+ Coverage 73.04% 73.09% +0.04%
Complexity 1141 1141
============================================
Files 1151 1151
Lines 42275 42340 +65
Branches 5045 5061 +16
============================================
+ Hits 30881 30949 +68
+ Misses 8919 8913 -6
- Partials 2475 2478 +3
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@funky-eyes @slievrly Can you review this pr? |
slievrly
left a comment
slievrly
left a comment
There was a problem hiding this comment.
中文 / Chinese
总体判断
补偿阶段循环状态拿不到 loopElement / loopIndex 是个真实痛点（#6776），思路——补偿时重建 loop element——方向合理。但这里有一个语义正确性问题我觉得需要先讨论，比代码风格更重要。
关键问题
1. 补偿时"重新求值 collection 表达式"可能取到与正向执行不一致的集合
补偿分支现在这样重建集合：
Expression expression = expressionFactory.createExpression(loop.getCollection());
Object evaluatedResult = expression.getValue(contextVariables); // 用"当前恢复后"的上下文求值
if (evaluatedResult instanceof Collection) {
collection = (Collection<?>) evaluatedResult;
}问题在于：补偿求值用的是恢复后的当前上下文，而不是正向执行时迭代的那个集合快照。Saga 的补偿常常发生在正向执行很久之后（这正是 saga 的设计初衷），期间：
- collection 表达式指向的变量可能已被后续状态改写 → 求值出的集合和当初迭代的内容/顺序/大小都不同。
- 补偿本应精确补偿"正向执行过的那些元素"，重新求值拿到的是另一个快照，可能补偿到错误的元素，甚至漏补/多补。
2. 集合缩小时 iterator() 会抛 NoSuchElementException
private Object iterator(Collection<?> collection, int loopCounter) {
Iterator<?> iterator = collection.iterator();
int index = 0;
Object value = null;
while (index <= loopCounter) {
value = iterator.next(); // 若重求值后的集合比 loopCounter+1 小，这里直接抛异常
index++;
}
return value;
}接第 1 点：如果重新求值得到的集合比当初小（元素被消费/删除），iterator.next() 会在补偿阶段抛未捕获的 NoSuchElementException，直接让补偿流程崩溃。这比"拿不到 loopElement"更糟。
建议方向：与其在补偿时重新求值表达式，不如在正向执行时就把每次迭代实际用的 loop element（或整个 collection 快照）持久化到 state instance 的上下文里，补偿时按 loopIndex 直接还原。这样补偿针对的永远是当初真正执行过的元素，也不受后续数据变化影响。如果持久化成本是顾虑，至少 iterator() 要对 loopCounter >= collection.size() 做边界保护并给出可诊断的日志。
3. 求值失败/非集合时静默跳过 —— 建议加 WARN
现在三种情况（expressionFactoryManager == null、求值返回 null、返回非 Collection）都是静默 collection = null → 不 set 上下文变量。对补偿来说，这等于"悄悄退回到原来的 bug"。用户会困惑：为什么有时候补偿能拿到 loopElement 有时候拿不到，且没有任何线索。建议这几条分支各打一条 WARN，说明"补偿阶段无法重建 loop element，因为 X"。
4. 正向路径的行为变化
重构把"总是 set 上下文变量"改成了 if (collection != null) 才 set。正向分支里 collection = LoopContextHolder.getCurrent(context, true).getCollection() 理论上不会为 null，但如果真为 null，旧代码是 NPE、新代码是静默跳过。行为更温和是好事，但请确认正向路径没有依赖"即使 collection 为空也要 set 一个空 element"的下游逻辑。
测试
- 4 个新测试全是重度 mock（mock 一切 + 3 个 MockedStatic）。它们验证的是"代码按预期调用了 mock"，而不是真实行为。尤其第 1 点的核心风险——"补偿时集合变了"——完全没被覆盖，因为集合是 mock 成固定 list 的。
- 4 个方法之间大量复制粘贴（每个 ~70 行 setup 几乎一样），建议抽一个
buildCompensationContext(...)helper。 - PR 的 verify 里提到
StateMachineDBTests，但 diff 里没看到相关新增/改动。建议补一个真正跑"带 loop 的补偿"的集成用例，覆盖集合大小变化的场景——那才是这个 fix 真正要证明的东西。
小结
方向对，但"补偿时重新求值 collection"这个核心机制有正确性风险（第 1、2 点）。建议改为"正向执行时持久化 element/collection 快照，补偿时还原"，至少要给 iterator() 加边界保护。第 3、4 是稳健性/可诊断性，测试建议补一个真实的集合变化用例。
English
Overall
Loop states not having loopElement / loopIndex available during compensation is a genuine pain point (#6776), and the idea — reconstruct the loop element at compensation time — is reasonable. But there's a semantic correctness concern I'd like to discuss first; it matters more than any style point.
Key concerns
1. Re-evaluating the collection expression at compensation time can yield a collection inconsistent with forward execution
The compensation branch now rebuilds the collection like this:
Expression expression = expressionFactory.createExpression(loop.getCollection());
Object evaluatedResult = expression.getValue(contextVariables); // evaluated against the CURRENT restored context
if (evaluatedResult instanceof Collection) {
collection = (Collection<?>) evaluatedResult;
}The problem: compensation evaluates against the current restored context, not the collection snapshot that was actually iterated during forward execution. In saga, compensation often happens long after forward execution (that's the whole point), and in between:
- The variable the collection expression points at may have been mutated by later states → the evaluated collection differs in content / order / size from what was originally iterated.
- Compensation is supposed to precisely compensate "the elements that were forward-executed". Re-evaluation gets a different snapshot, so it may compensate the wrong elements, or miss/duplicate some.
2. iterator() throws NoSuchElementException if the collection shrank
private Object iterator(Collection<?> collection, int loopCounter) {
Iterator<?> iterator = collection.iterator();
int index = 0;
Object value = null;
while (index <= loopCounter) {
value = iterator.next(); // if the re-evaluated collection is smaller than loopCounter+1, this throws
index++;
}
return value;
}Following from #1: if the re-evaluated collection is smaller than at forward time (elements consumed/removed), iterator.next() throws an uncaught NoSuchElementException during compensation, crashing the compensation flow outright — worse than "loopElement is missing".
Suggested direction: instead of re-evaluating the expression at compensation time, persist the actual loop element used at each iteration (or the whole collection snapshot) into the state-instance context during forward execution, and restore it by loopIndex at compensation. Then compensation always targets exactly the elements that were executed, immune to later data changes. If persistence cost is a concern, at minimum guard iterator() against loopCounter >= collection.size() with a diagnosable log.
3. Silent skip on eval failure / non-collection — add a WARN
Currently all three cases (expressionFactoryManager == null, eval returns null, returns non-Collection) silently set collection = null → don't set context variables. For compensation, that's "silently falling back to the original bug". Users will be confused why loopElement is sometimes available and sometimes not, with no clue. Suggest a WARN in each branch explaining "cannot reconstruct loop element during compensation because X".
4. Behavior change on the forward path
The refactor turns "always set context variables" into "set only if (collection != null)". On the forward branch collection = LoopContextHolder.getCurrent(context, true).getCollection() should never be null, but if it ever were, the old code NPE'd and the new code silently skips. More graceful is good, but please confirm no downstream logic on the forward path relies on "set an (empty) element even when the collection is empty".
Tests
- The 4 new tests are heavily mock-driven (mock everything + 3
MockedStatic). They verify "the code called the mocks as expected", not real behavior. In particular the core risk from #1 — "the collection changed by compensation time" — is not covered at all, because the collection is mocked to a fixed list. - Lots of copy-paste across the 4 methods (~70 lines of near-identical setup each); extract a
buildCompensationContext(...)helper. - The PR's verify section mentions
StateMachineDBTests, but I don't see related additions/changes in the diff. Please add an integration case that actually runs "compensation with a loop" and covers the changed-collection-size scenario — that's what this fix really needs to prove.
Summary
Right direction, but the core mechanism — "re-evaluate the collection at compensation time" — carries a correctness risk (#1, #2). I'd prefer "persist the element/collection snapshot at forward-execution time and restore at compensation", and at minimum add a bounds guard to iterator(). #3/#4 are robustness/diagnosability; the tests should add a real changed-collection case.
|
@slievrly Thanks for the detailed review I've updated the implementation so that expression re-evaluation is no longer the primary mechanism. During forward execution, the current loop element is captured by LoopTaskHandlerInterceptor and persisted into StateInstance.extensionParams via ServiceTaskHandlerInterceptor before recordStateStarted(). The extensionParams are serialized into the ext_params column and restored when the state instance is reloaded. During compensation, the loop element is restored directly from the persisted extensionParams. Expression re-evaluation is now only retained as a compatibility fallback for state instances that do not contain persisted loop metadata. I also added bounds checking to iterator() and WARN logging when fallback reconstruction is required or cannot be performed. Regarding the tests, I added a database-backed integration test covering persistence and restoration of the loop element through StateLogStore, in addition to the unit tests. |
2 participants
Ⅰ. Describe what this PR did
Re-evaluate the loop collection expression during compensation and reconstruct the loop element using the restored loop index. This allows compensation tasks within loop states to access the corresponding loop element and index through the state machine context.
Ⅱ. Does this pull request fix one issue?
Fixes #6776
Ⅲ. Why don't you add test cases (unit test/integration test)?
Added unit tests covering compensation-time expression evaluation and loop element reconstruction.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
N/A