Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Fix bug 2945/3021 (missing key in documents database) #734

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Conversation

loiclec
Copy link
Contributor

@loiclec loiclec commented Dec 8, 2022

Pull Request

Related issue

Fixes (partially, until merged into meilisearch) meilisearch/meilisearch#2945 (until we integrate the new milli bump into meilisearch).

Note that a dump will not be sufficient to upgrade from meilisearch v0.30.2 to meilisearch v0.30.3 due to this fix because the bug could have caused the documents database to be corrupted. Instead, a full manual reimport of the documents will be necessary.

What does this PR do?

There was a bug happening when:

  1. A few documents are added to the index
  2. Some of these documents are soft-deleted
  3. New documents are added, replacing existing ones and triggering a hard-deletion

The IndexDocuments::execute method would then perform the hard-deletion but forget to change the external_document_ids structure appropriately. As a result, the external_document_ids would contain keys corresponding to documents that do no exist anymore.

To fix this bug, I split the DeleteDocuments::execute method into two: execute_inner and execute.

  • execute_inner returns a DetailedDocumentDeletionResult which says whether soft-deletion was used or not
  • execute keeps the exact same signature and behaviour

Then, when deleting replaced documents inside IndexDocuments::execute, we call DeleteDocuments::execute_inner instead of DeleteDocuments::execute. If soft-deletion was used, nothing more is done. But if hard-deletion was used, we remove every reference to soft-deleted documents in the new external_documents_ids structure.

Correctness

  • Every other test still passes
  • The reproduction test case now passes
  • In a different branch (update-fuzz-test), I created a fuzz-test that reproduces the past two bugs. This fuzz test cannot find this bug through any combination of some hand-selected DocumentAddition / DocumentDeletion / DocumentClear / SettingsUpdate operations. In that test, each relevant operations can be executed with or without soft-deletion, and document additions can be done in batches, replacing or updating existing documents.

@loiclec loiclec marked this pull request as ready for review December 12, 2022 12:59
@loiclec loiclec added bug Something isn't working no breaking The related changes are not breaking (DB nor API) labels Dec 12, 2022
@loiclec loiclec changed the title [WIP] Fix bug 3021 (missing key in documents database) Fix bug 3021 (missing key in documents database) Dec 12, 2022
If the document import replaces a document using hard deletion
@loiclec loiclec requested a review from Kerollmops December 12, 2022 13:22
@loiclec loiclec changed the title Fix bug 3021 (missing key in documents database) Fix bug 2945/3021 (missing key in documents database) Dec 12, 2022
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great pull request, thank you again for fixing this for the nth time 😄 I am glad that the fuzz tests found both of the previous bugs but is not able to find new ones 🎉

milli/src/update/delete_documents.rs Outdated Show resolved Hide resolved
@loiclec loiclec requested a review from Kerollmops December 13, 2022 09:37
Copy link
Member

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like what you've done here. Thank you again for this fix!
bors merge

@bors
Copy link
Contributor

bors bot commented Dec 13, 2022

Build succeeded:

@bors bors bot merged commit e0a8f8c into main Dec 13, 2022
@bors bors bot deleted the bug-3021-test branch December 13, 2022 10:29
bors bot added a commit that referenced this pull request Dec 14, 2022
739: Cherry pick bug fixes for v0.37.3 r=curquiza a=curquiza

Integrate #734 and #737 into the `release-v0.37.3` branch to avoid to release on `main`

<s>Wait for meilisearch/meilisearch#3235 investigation without merging this PR. Indeed, if a bug fix on milli's side, we might wait for it before merging this PR/</s> -> no need to wait, not a bug

Co-authored-by: ManyTheFish <many@meilisearch.com>
Co-authored-by: Loïc Lecrenier <loic.lecrenier@me.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working no breaking The related changes are not breaking (DB nor API)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants