feat: asynchronous checks by carlinmack · Pull Request #62 · inveniosoftware/invenio-checks

carlinmack · 2026-04-27T13:06:02Z

❤️ Thank you for your contribution!

Description

Please describe briefly your pull request.

Celery tasks are used to execute the check, however we do not want to duplicate the logic of executing a task, and so in the task context the asynchronous task is run synchronously. A visual explanation of this is shown below (see run_check(..., sync=true)).

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

I'm aware of the code of conduct.
I've created logical separate commits and followed the commit message format.
I've added relevant test cases.
I've added relevant documentation.
I've marked translation strings.
I've identified the copyright holder(s) and updated copyright headers for touched files (>15 lines contributions).
I've NOT included third-party code (copy/pasted source code or new dependencies).
- If you have added third-party code (copy/pasted or new dependencies), please reach out to an architect on Discord.

Frontend

I've followed the CSS/JS and React guidelines.
I've followed the web accessibility guidelines.
I've followed the user interface guidelines.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

You license your contribution under the same terms as the current repository’s license.
You agree that you have the right to license your contribution under the current repository’s license.

fix: logic bug where success was never set to true

OliverGeneser · 2026-05-06T14:26:27Z

        """Add a rule result and update the overall success."""
        self.rule_results.append(rule_result)
-        if not rule_result.success and rule_result.level == "failure":
+        if rule_result.success and rule_result.level == "failure":


Why the change?

if you read the statement it's

if not X and ...: X = false

so it's would always be a no-op. In the DB all checks have a success of true regardless of it the checks are passing

sorry for coming back to this. The condition was checking rule_result.success, and then setting self.success, so not changing the same var.
It looked more correct before, it feels wrong checking for rule_result.success True and level Failure.

okay I will double check

okay great catch nico! there is an error with this line but it's

Suggested change

if rule_result.success and rule_result.level == "failure":

if not rule_result.success and rule_result.level == "error":

OliverGeneser · 2026-05-06T14:29:08Z

+        db.session.commit()
+
+        with UnitOfWork() as uow:
+            # as we are in the task, run the check synchronously
+            ChecksAPI.run_check(config, record, uow, sync=True)
+            uow.commit()


Why are we mixingg manual commits with UnitOfWork?

good question. I was imagining that the uow would automatically commit when exiting the scope but this is not the behaviour. @slint what's the best practise here?

It actually surprisingly doesn't commit, so it has to be explicit... there's a similar behavior with SQLAlchemy's session so I think this where this design is coming from.

we could pass an optional uow to the run_check APIs, and treat that class as a service. The decorator will take care of the commit.

OliverGeneser · 2026-05-06T14:51:59Z

+        return result_run
+
+    @classmethod
+    def run_check(cls, config, record, uow, is_draft=None, sync=False):


Hmm I think run_check is mixing execution and persistence, which can causes a problem in the async flow. The task recieves a check_run_id, but then run_check queries by config_id, record_id and is_draft instead of using that id, so it may update or overwrite the wrong row if multiple runs exist. I think it would be safer to separate concerns so the execution just returns a result, and the task updates the existing CheckRun directly by id.

It shouldn't be possible for there to be multiple CheckRuns as they are only created in _create_or_update_check_run which handles this. I don't think I need to make any other changes from your comment.

For clarity, refer to the diagram in the original message in this PR and note if you think there's anything wrong with when the status is being updated

My concern is that it feels brittle that the task receives a check_run_id, but run_check(sync=True) then re-queries instead of updating that specific row directly, since the async task already have owns the CheckRun conceptually.

I was thinking it might be cleaner if run_check just returned the result/state, and the task then updated the existing CheckRun directly by id.

Can we please refer to the diagram so we're on the same page :)

If run_check (in api.py) just returned the result state, then the component would have to update the db (which is not correct).

What you're suggesting I think would to run all checks via a task, so that the task is the only thing updating the DB, however Alex already said that this is not correct and that instead we should consider creating a current_checks_api or a service (which I haven't proceeded with as it requires more design thinking about how we want the service/proxy to work)

Good idea referring to the diagram 😄 What I meant isn’t that the return value from api.py should change, but that in the async execution flow the task already has the check_run_id, so it could update/save that specific CheckRun directly instead of run_check(sync=True) re-querying it.

but then we would need to make the params of run_check fully optional as you can either call it with (config, record) or (check_run_id) with the additional implied checking of params

ntarocco · 2026-05-13T07:58:41Z

+
+
+class AsyncCheck(Check):
+    """Example of an async check."""


Is this code that we need to change later?

I'll drop this commit before we merge, it's just to have something to test with and give an example of how to configure it

ntarocco · 2026-05-13T08:02:59Z

+        db.session.commit()
+
+        with UnitOfWork() as uow:
+            # as we are in the task, run the check synchronously
+            ChecksAPI.run_check(config, record, uow, sync=True)
+            uow.commit()


we could pass an optional uow to the run_check APIs, and treat that class as a service. The decorator will take care of the commit.

fix: move sync attribute to CheckConfig

53862b7

carlinmack added this to Sprint Q2 2026 ☀️ Apr 27, 2026

carlinmack moved this to In progress in Sprint Q2 2026 ☀️ Apr 27, 2026

carlinmack self-assigned this Apr 27, 2026

carlinmack added 5 commits May 6, 2026 11:18

feat: add ability to run checks asynchronously

9fe62b7

chore: refactor CheckResult into a common class

97883cc

fix: logic bug where success was never set to true

chore: shorten name for severity icons

c4037db

UI: add support for showing running as a status

f3b058e

temp: add dummy async_check example check config

d87449d

carlinmack force-pushed the async-checks branch from 7ef3840 to d87449d Compare May 6, 2026 09:24

carlinmack marked this pull request as ready for review May 6, 2026 09:24

carlinmack moved this from In progress to In review 🔍 in Sprint Q2 2026 ☀️ May 6, 2026

carlinmack removed their assignment May 6, 2026

OliverGeneser reviewed May 6, 2026

View reviewed changes

ntarocco reviewed May 13, 2026

View reviewed changes

carlinmack moved this from In review 🔍 to In progress in Sprint Q2 2026 ☀️ May 13, 2026

fix: feedback from review

3bd1b4b

carlinmack force-pushed the async-checks branch from cc2e404 to 3bd1b4b Compare May 13, 2026 12:06

carlinmack moved this from In progress to In review 🔍 in Sprint Q2 2026 ☀️ May 13, 2026

db: add uniqueness constraint on config_id, recid and is_draft

8326a18

	if rule_result.success and rule_result.level == "failure":
	if not rule_result.success and rule_result.level == "error":

Conversation

carlinmack commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntarocco May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlinmack May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlinmack commented Apr 27, 2026 •

edited

Loading

ntarocco May 13, 2026 •

edited

Loading

carlinmack May 13, 2026 •

edited

Loading