Update process status in the database before broadcasting that process status has changed (fix #6579)

[agoscinski]

Fix for #6579

Processes start to broadcast their event before they update their process status in the database. This can cause issues if the next process directly tries to access the last process state retrieving it from the database while it has not been updated in the database.

TODO: test

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.86%. Comparing base (ef60b66) to head (4cda6e8).
Report is 183 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6580      +/-   ##
==========================================
+ Coverage   77.51%   77.86%   +0.36%     
==========================================
  Files         560      566       +6     
  Lines       41444    42095     +651     
==========================================
+ Hits        32120    32772     +652     
+ Misses       9324     9323       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[agoscinski]

I guess the CI failure is because of some unrelated problems.

I would like to have some feedback if anyone sees that this moving the setting of the process status might cause problems. The tests pass so far.

[sphuber]

Thanks a lot for tracking this one down @agoscinski and @t-reents . This one is my fault. I changed the behavior in this method two months ago to make sure a node was sealed when it excepted. I was thinking about the location of the super call and didn't remember why I moved it to the beginning originally, so I moved it back. Bad mistake! My comment on line 429 even explicitly reminded myself that attaching outputs has to happen before calling the parent, I just put "for reasons unknown". I guess we now know :D Could you perhaps please update the comment and replace/

For reasons unknown, it is important to update the outputs first, before doing anything else, otherwise there is the risk that certain outputs do not get attached before the process reaches a terminal state.

with

The updating of outputs and state has to be performed before the super is called because the super will broadcast state changes and parent processes may start running again before the state change is completed. It is possible that they will read the old process state and outputs that they check may not yet have been attached

[agoscinski]

@sphuber Added your text, but on the place where the super is invoked. Also added a test now.

[danielhollas]

This one is my fault. I changed the behavior in this method two months ago to make sure a node was sealed when it excepted. I was thinking about the location of the super call and didn't remember why I moved it to

Just to clarify, which versions are affected by this bug? The linked issues mentioned that the bug is present in both 2.2 and 2.6 but that doesn't seem in line with the comment above?

[sphuber]

The bug has never been released, it was just sitting on main

[agoscinski]

The linked issues mentioned that the bug is present in both 2.2

I think when I did the test my aiida-core upgraded at the installation of the pseudo potential, so this was wrong. At least I don't get the bug in 2.6.1. I'll correct it in the issue. But the bug was released for 2.6.2 https://github.com/aiidateam/aiida-core/blob/v2.6.2/src/aiida/engine/processes/process.py#L421

[danielhollas]

Thank, makes sense. So I guess it would make sense to push a new patch release, and I'll have to then upgrade aiida-core in AiiDAlab image which now uses 2.6.2

[sphuber]

Ah @agoscinski is right, it was released. I just quickly looked at the commit where the change was introduced and Github didn't show any associated tags, which is usually a useful feature. However, I forgot that in this case, we made the release from a support branch and so the linked commit was cherry-picked onto the support branch before being released, and so the tag didn't show.