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

Fix panic in store reader raw document iterator during segment merge #1076

Merged
merged 6 commits into from Jun 14, 2021

Conversation

appaquet
Copy link
Contributor

The store reader raw document iterator fails when the first document of a block past a new checkpoint has been deleted. This was caused by the reset_block_pos flag used to reset block_start_pos being discarded when the current document isn't alive.

Depending on the state of the segments, I could triggered two different panics related to the varint deserialization in the iterator. See this. One involves a bit shifting overflow in the Vint deserialization itself, and one involves reading a wrong and huge doc length, leading to a out of range slice. In all cases, another thread also panics after dropping a RamDirectory's VecWriter without flushing it, caused by a VInt deserialization error in IndexMerger.write, making the function return before closing writers.

Unfortunately, this is the minimal test I could find to reproduce the issue... I would have liked a straightforward test, but it seems like a few merges, with enough data per segment, need to happen to trigger the bug. Because of that, the test is slower than what I'd consider acceptable for a unit test (~10s on my machine).

Let me know what you want to do about the test, or if you have a better idea to test the issue.

@appaquet appaquet changed the title Panic in store reader raw document iterator during segment merge Fix panic in store reader raw document iterator during segment merge Jun 13, 2021
@fulmicoton fulmicoton requested a review from PSeitz June 14, 2021 00:46
@fulmicoton fulmicoton added the bug label Jun 14, 2021
@fulmicoton
Copy link
Collaborator

fulmicoton commented Jun 14, 2021

@appaquet Thank you so much for the report. This is SUPER helpful. I assume this is with tantivy 0.15?

@PSeitz Can you create an issue, and investigate on Monday so that we can publish an hotfix ASAP?

For simplifying the unit test...
Using a single thread index writer is a good idea. Using larger documents is a good idea too.
A schema with a single STORED field is sufficient.
Finally merging manually.

replace test with smaller test in doc_store
@PSeitz
Copy link
Contributor

PSeitz commented Jun 14, 2021

@appaquet thanks for the detailed bug report! I just pushed a change to the PR, which moves the test for the regression into the doc store folder, since there is already some test data for such a case.

@PSeitz PSeitz merged commit 221e7cb into quickwit-oss:main Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants