handle duplicate notifications sent for the same book from different tasks by elfkuzco · Pull Request #515 · openzim/cms

elfkuzco · 2026-09-11T01:44:22Z

Changes

add task_id column to zimfarm_notifications table
require task_id when processing notifications from zimfarm
when new notification arrives and comes with a different task ID (which happens when user tries to upload an already processed book), allow it to pass but processing logic now marks it as bad notification instead of ignoring it as a duplicate notification. It is impossible to create a book with that notification ID as that would cause an integrity error (since book's PK is notification ID).
mark possible title uploads coming from duplicate notification as failed in CMS (even though they aren't on Zimfarm). This quickly allows it to be processed by background tasks to delete manually upload zim from S3
re-design zimfarm notification view with the same style as book view
retreive notifications by combination of ZIM ID and task ID instead of just ZIM ID
correctly associate title upload with the same book that zimfarm sent since we now have the same task ID

This fixes #494

…tasks

codecov · 2026-09-11T01:48:45Z

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.75%. Comparing base (784affd) to head (4fe3929).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ms_backend/mill/processors/zimfarm_notification.py	69.23%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #515      +/-   ##
==========================================
- Coverage   81.78%   81.75%   -0.03%     
==========================================
  Files          69       69              
  Lines        4397     4412      +15     
  Branches      515      518       +3     
==========================================
+ Hits         3596     3607      +11     
- Misses        640      642       +2     
- Partials      161      163       +2

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

benoit74 · 2026-09-11T08:24:57Z

-    zimfarm_notification.events.append(
-        f"{getnow()}: notification transformed into book"
-    )
+


Why changing this ordering, I don't get the difference?

During tests for processing duplicate notifications, I found out the notification gets updated even though book might possibly fail to be added to the DB.

This is weird ... please better understand where, all this is supposed to run in a single transaction

benoit74 · 2026-09-11T08:25:26Z

        f"{getnow()}: created from Zimfarm notification {zimfarm_notification.id}"
    )

+    session.flush()


I don't feel like session.flush is strictly mandatory, I feel like it is more something needed for tests only (and could reduce perf on prod). Could you explain or move to test?

It helps propagate/detect the Integrity early error early from where it was introduced. If we remove it, the next time it would get thrown would be from the call to update a title upload or getting an existing task because in SQLAlcheny, select and update calls automatically trigger flush. I figured there'd be no difference and it might as well happen explicitly. Plus, I can infer correctly which function threw the error

Indeed, LGTM

benoit74 · 2026-09-11T08:25:34Z

        f"{getnow()}: added {status} location: {filename} in {warehouse_name}: "
        f"{path} ({warehouse_id})"
    )
+    session.flush()


I don't feel like session.flush is strictly mandatory, I feel like it is more something needed for tests only (and could reduce perf on prod). Could you explain or move to test?

It happens automatically during future calls to get matching title or any other queries involving select which will definitely happen during notification processing timeline. Happening during automatically invoked calls makes it harder to know and catch the block that introduced it

Indeed, LGTM

benoit74 · 2026-09-11T08:28:05Z

+                zimcheck_result_url=zimcheck_url,
+                zimfarm_notification=notification,
+            )
+        except IntegrityError:


This is very fragile in the sense that we could have other integrity errors in the future. Why not simply checking that a book with same ID does not already exists before trying to create it?

This is very fragile in the sense that we could have other integrity errors in the future. Why not simply checking that a book with same ID does not already exists before trying to create it?

That works too.

benoit74 · 2026-09-11T08:29:26Z

+            # If zimfarm notification came because of a title upload mark it as failed
+            if get_title_upload_or_none(session, notification.task_id) is not None:
+                update_title_upload_status(session, notification.task_id, "failed")
+            notification.status = "bad_notification"


shall we use duplicate_book_id as status? I feel like it is way more explicit and will make things more understandable for the end user

Sounds good to me

benoit74 · 2026-09-11T08:32:31Z

+            )
+            # If zimfarm notification came because of a title upload mark it as failed
+            if get_title_upload_or_none(session, notification.task_id) is not None:
+                update_title_upload_status(session, notification.task_id, "failed")


Shall we use another status than "failed" like "duplicate_upload" to make things clearer for the end user. We should also update UI to add a small text explaining this issue (like a question mark button next to this status when shown which opens a popup explaining what happened).

handle duplicate notifications sent for the same book from different …

48f0a22

…tasks

elfkuzco force-pushed the hanlde-duplicate-notifications branch from 6b0d5ea to 48f0a22 Compare September 11, 2026 01:44

elfkuzco self-assigned this Sep 11, 2026

elfkuzco requested a review from benoit74 September 11, 2026 01:50

benoit74 requested changes Sep 11, 2026

View reviewed changes

check if book exists before creating

4fe3929

Uh oh!

Uh oh!

Conversation

elfkuzco commented Sep 11, 2026

Changes

Uh oh!

codecov Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

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!

2 participants

codecov Bot commented Sep 11, 2026 •

edited

Loading