Cancel job with incident when canceling the process instance #9219

korthout · 2022-04-22T13:33:50Z

Description

This fixes a bug where canceling a process instance could lead to a job becoming activatable even though the process instance is terminated. See #8588 for more details.

The bug is fixed by making sure the job is immediately canceled (i.e. deleted from the state) when we terminate the related element instance. We achieve this by writing a Job Canceled event when the TERMINATE_ELEMENT command is being processed.

This PR fixes the bug in a way that can be backported because it doesn't introduce too many changes. Alternatively, the bug could be fixed by tackling the main issue: applying the Incident Resolved event should not affect the state of a job. Instead, the job should be made activatable by writing a separate Job event in the Resolve Incident Processor. That would be a larger change, with more impact on the event appliers than what this PR proposes. Changes to event appliers should generally be avoided.

Related issues

closes #8588

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

The changes are backwards compatibility with previous versions
If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

There are unit/integration tests that verify all acceptance criterias of the issue
New tests are written to ensure backwards compatibility with further versions
The behavior is tested manually
The change has been verified by a QA run
The impact of the changes is verified by a benchmark

Documentation:

The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
New content is added to the release announcement
If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

pihme

One question/concern.

Please also create follow-up issues to address the general change you alluded to in your description (resolving incidents should not activate the Job)

engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnJobBehavior.java

This adds a regression test against activating jobs of cancelled process instances, by making sure they are cancelled (i.e. deleted). In this specific case, an uncaught error was thrown on a job which led to an UNHANDLED_ERROR_EVENT incident. When the process instance is cancelled, the job was not called, but instead was made activatable again. This happened, because the incident was resolved as part of the process instance termination logic. Instead, we should guarantee that the job is cancelled. A cancelled job can no longer be made re-activatable. This is a terminal state for jobs, because a cancelled job is a deleted job. See #8588 for more details.

This makes it easier to add more values to the predicate, while staying understandable.

Jobs in the error_thrown state should also be able to be cancelled. By adding it to this list, we make sure that the job with error_thrown state can be cancelled by the engine when the element it belongs to is being terminated.

We need to make sure the job is cancelled immediately (synchronously) because otherwise the IncidentResolvedApplier may still make the job activatable again. To cancel the job immediately we simply need to use the statewriter to write Job Cancelled, instead of using the commandwriter to write Cancel Job. The statewriter will immediately apply the event when writing it.

Since the command is no longer written, the test cases that were verifying that the command was written should be updated to verify that the job canceled event is written in the same spot. Note that in the BoundaryEventTest it highlights the difference between asynchronously canceling the job with a command and synchronously (immediately) canceling the job with an event.

The ability to read a metric value will be useful in future Metrics tests.

This adds tests that verify that the job metrics are counted when expected. Note that the `shouldCountCanceled` case fails, because canceling the job was no longer counted in the metrics.

This behavior was broken in an earlier commit, when we switched from writing a Cancel Job command to a Job Canceled event.

It's important to highlight duplication, because if either changes, the other should be changed as well.

korthout · 2022-04-28T11:57:40Z

@pihme Please have another look, I'll open an issue for the follow-up work.

pihme

good job!

korthout · 2022-04-28T12:22:30Z

bors merge

zeebe-bors-camunda · 2022-04-28T12:52:13Z

Build succeeded:

github-actions · 2022-04-28T12:53:08Z

Backport failed for stable/1.3, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin stable/1.3
git worktree add -d .worktree/backport-9219-to-stable/1.3 origin/stable/1.3
cd .worktree/backport-9219-to-stable/1.3
git checkout -b backport-9219-to-stable/1.3
ancref=$(git merge-base 2f4d8c859cfde20f7d203fea9141f6ed9ccfd305 25fae05a803c17cc5bb9a76620f3e25c7194cc06)
git cherry-pick -x $ancref..25fae05a803c17cc5bb9a76620f3e25c7194cc06

github-actions · 2022-04-28T12:53:10Z

Backport failed for stable/8.0, because it was unable to create a new branch.

Please cherry-pick the changes locally.

git fetch origin stable/8.0
git worktree add -d .worktree/backport-9219-to-stable/8.0 origin/stable/8.0
cd .worktree/backport-9219-to-stable/8.0
git checkout -b backport-9219-to-stable/8.0
ancref=$(git merge-base 2f4d8c859cfde20f7d203fea9141f6ed9ccfd305 25fae05a803c17cc5bb9a76620f3e25c7194cc06)
git cherry-pick -x $ancref..25fae05a803c17cc5bb9a76620f3e25c7194cc06

9250: [Backport 1.3] Cancel job with incident when canceling the process instance r=korthout a=korthout ## Description  Backport of #9219 ## Related issues  closes #8588 Co-authored-by: Nico Korthout <nico.korthout@camunda.com>

9251: [Backport 8.0] Cancel job with incident when canceling the process instance r=korthout a=korthout ## Description  Backport of #9219 ## Related issues  closes #8588 Co-authored-by: Nico Korthout <nico.korthout@camunda.com>

korthout added backport stable/1.3 labels Apr 22, 2022

korthout force-pushed the korthout-8588-cancel-job-with-incident branch from 3bab30a to 8fb143e Compare April 22, 2022 13:34

pihme self-requested a review April 25, 2022 12:39

pihme suggested changes Apr 25, 2022

View reviewed changes

engine/src/main/java/io/camunda/zeebe/engine/processing/bpmn/behavior/BpmnJobBehavior.java Outdated Show resolved Hide resolved

korthout added 3 commits April 28, 2022 11:11

refactor(engine): use set contains as predicate

da40a20

This makes it easier to add more values to the predicate, while staying understandable.

fix(engine): also cancel job in error_thrown state

57fc9da

Jobs in the error_thrown state should also be able to be cancelled. By adding it to this list, we make sure that the job with error_thrown state can be cancelled by the engine when the element it belongs to is being terminated.

korthout force-pushed the korthout-8588-cancel-job-with-incident branch 2 times, most recently from 9da335b to d612677 Compare April 28, 2022 09:13

korthout added 5 commits April 28, 2022 13:45

refactor(engine): extract metrics test helper

74fe956

The ability to read a metric value will be useful in future Metrics tests.

test(engine): verify job metrics

d9b3413

This adds tests that verify that the job metrics are counted when expected. Note that the `shouldCountCanceled` case fails, because canceling the job was no longer counted in the metrics.

fix(engine): count canceled jobs in metrics

0aac0d7

This behavior was broken in an earlier commit, when we switched from writing a Cancel Job command to a Job Canceled event.

korthout force-pushed the korthout-8588-cancel-job-with-incident branch from d612677 to 0aac0d7 Compare April 28, 2022 11:47

docs(engine): document duplication

25fae05

It's important to highlight duplication, because if either changes, the other should be changed as well.

korthout requested a review from pihme April 28, 2022 11:57

pihme approved these changes Apr 28, 2022

View reviewed changes

korthout mentioned this pull request Apr 28, 2022

Resolving an incident should make job activatable by writing an event #9246

Open

korthout mentioned this pull request Apr 28, 2022

Deprecate Cancel Job command #9247

Closed

zeebe-bors-camunda bot merged commit 053ff2c into main Apr 28, 2022

zeebe-bors-camunda bot deleted the korthout-8588-cancel-job-with-incident branch April 28, 2022 12:52

This was referenced Apr 28, 2022

[Backport 1.3] Cancel job with incident when canceling the process instance #9250

Merged

[Backport 8.0] Cancel job with incident when canceling the process instance #9251

Merged

npepinpe added release/8.0.2 version:1.3.8 labels May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel job with incident when canceling the process instance #9219

Cancel job with incident when canceling the process instance #9219

korthout commented Apr 22, 2022

pihme left a comment

korthout commented Apr 28, 2022

pihme left a comment

korthout commented Apr 28, 2022

zeebe-bors-camunda bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

Cancel job with incident when canceling the process instance #9219

Cancel job with incident when canceling the process instance #9219

Conversation

korthout commented Apr 22, 2022

Description

Related issues

Definition of Done

pihme left a comment

Choose a reason for hiding this comment

korthout commented Apr 28, 2022

pihme left a comment

Choose a reason for hiding this comment

korthout commented Apr 28, 2022

zeebe-bors-camunda bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022