Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate UUIDs/submissions #470

Open
emilykferguson opened this issue Jul 17, 2018 · 8 comments
Open

Duplicate UUIDs/submissions #470

emilykferguson opened this issue Jul 17, 2018 · 8 comments
Assignees
Labels

Comments

@emilykferguson
Copy link

Example user:

https://app.intercom.io/a/apps/s6hn3a8q/inbox/inbox/conversation/16985968453

Discussion
https://www.flowdock.com/app/kobotoolbox/kobo/threads/g4mOr7lnWnyaLbdAa9WBAky5riO

@jnm
Copy link
Member

jnm commented Sep 22, 2020

we should start enforcing a uniqueness constraint on xml_hash, but then we have to decide what to do with existing duplicates.

likely the current code was written with a misunderstanding of transaction.atomic (this is an unverified guess)

another example of the problem: https://www.flowdock.com/app/kobotoolbox/support/threads/Y1lnKnaQPTmi6NMPC_pGkV9Rl-J

@jnm
Copy link
Member

jnm commented Sep 23, 2020

likely race condition:

existing_instance = Instance.objects.filter(
Q(xml_hash=xml_hash) | Q(xml_hash=Instance.DEFAULT_XML_HASH, xml=xml),
xform__user=xform.user,
).first()
else:
existing_instance = None
if existing_instance:
# ensure we have saved the extra attachments
any_new_attachment = save_attachments(existing_instance, media_files)
if not any_new_attachment:
raise DuplicateInstance()

@raphaelmerx
Copy link

raphaelmerx commented Dec 12, 2022

Can confirm this is occurring ~0.4% of the time (for 87 out of 23,000 instances in our case). Duplicate instances have the same xml_hash.

Looking at onadata/libs/utils/logger_tools.py:create_instance, a race condition is likely the issue, especially if uploading attachments is taking a while. Because create_instance is wrapped in a db transaction, one call will not see the instance created in another call until the attachments are uploaded.

Potential fixes:

  • as mentioned above, a uniqueness constraint on xml_hash
  • an advisory lock using the xml_hash as id:
diff --git a/onadata/libs/utils/logger_tools.py b/onadata/libs/utils/logger_tools.py
index 593085c..cdab22c 100644
--- a/onadata/libs/utils/logger_tools.py
+++ b/onadata/libs/utils/logger_tools.py
@@ -179,8 +179,9 @@ def create_instance(
             existing_instance.parsed_instance.save(asynchronous=False)
             return existing_instance
     else:
-        instance = save_submission(request, xform, xml, media_files, new_uuid,
-                                   status, date_created_override)
+        with advisory_lock(xml_hash):
+            instance = save_submission(request, xform, xml, media_files, new_uuid,
+                                       status, date_created_override)
         return instance

@RobertoMaurizzi
Copy link

I've published PR #859 with a possible solution to the race condition mentioned by @raphaelmerx and a reproduction script that allows to run tests on a local docker setup

@jnm
Copy link
Member

jnm commented Jul 6, 2023

Thank you very much, Roberto, for your contributions! It may look like little progress has been made, but we're working on integrating your ideas.

In the meantime, there is a quick-and-dirty workaround to block duplicate UUIDs (regardless of whether or not the XML is identical) if anyone needs it: use a partial unique index:

kobocat=> create unique index concurrently jnm_block_new_duplicate_uuids on logger_instance(uuid) where id >= 30000000;
CREATE INDEX
Time: 507190.087 ms (08:27.190)

Make sure to pick a sufficiently high id so that your incoming submissions will not reach this number before the index generation completes. Once the index is ready, you can fast forward the sequence used to set the ID so that the uniqueness constraint comes into force:

kobocat=> select setval('logger_instance_id_seq', 30000000, false);
  setval
----------
 30000000
(1 row)

@esario
Copy link

esario commented Aug 8, 2023

@jnm Any chance we could implement and release this? I'm guessing more people running into this issue when migrating from the UN OCHA server in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants