Fix: Lucene90DocValuesProducer.TermsDict.seekCeil doesn't always position bytes correctly (#12167)

[epotyom]

Fix: Lucene90DocValuesProducer.TermsDict.seekCeil doesn't always position bytes correctly (#12167)

TermsDict ord and bytes can be out of sync after a call to seekCeil. It can lead to a state where ord indicates beginning of the next block ((ord & blockMask) == 0L), but bytes is positioned to read the compressed part of the block.

Test failure in #12167 is triggered when after seekCeil we call next which needs to decompressBlock. We read vInt to term.length, where in fact this vInt refers to the compressed block length. This length is greater than max term length of the block, which leads to IndexOutOfBoundsException.

TODO: write a test

[epotyom]

Extended existing nightly random tests to catch the issue most of the time. Would that be enough or do we need a test that catches it every single time?

[zhaih]

It would be great if we can have a unit test that can constantly reproduce the previous failure. If my analysis is correct, then probably we just need to reproduce it with

te.seekExact(0);
te.seekCeil(new BytesRef());
te.next()

[zhaih]

So, I think the ultimate reason might be previously when we call seekExact(0), this if condition is not true, and thus the bytes and ord are not reset correctly.

      if (ord < this.ord || blockIndex != currentBlockIndex) {
        // The looked up ord is before the current ord or belongs to a different block, seek again
        final long blockAddress = blockAddresses.get(blockIndex);
        bytes.seek(blockAddress);
        this.ord = (blockIndex << TERMS_DICT_BLOCK_LZ4_SHIFT) - 1;
      }

So I would prefer we do

ord = 1; // Probably need to add some comments on why we do this weird thing
seekExact(0L);

Instead of doing it manually here in case we're changing the seek behavior in the future.

[epotyom]

in case we're changing the seek behavior in the future.

This is the reason why I think we should not rely on seekExact. For example, if we do:

ord = 1;
seekExact(0L);

and in the future we optimize seekExact to not reset bytes, this can break again, because seekExact relies on TermDict to be in consistent state. Just to give an example of potential optimization: both ords 0 and 1 are in the same block. So, when we seek from 1 to 0 we can try using data from the current block without re-reading data from bytes? Even if this particular idea doesn't work, I think that that public methods of this class rely on data to be in a consistent state, so we should probably not rely on them to fix the state?

[zhaih]

That makes sense, can we instead refactor this code with the code below to a method seekBlock(int)? Essentially these 4 lines:

      final long blockAddress = blockAddresses.get(block);
      this.ord = block << TERMS_DICT_BLOCK_LZ4_SHIFT;
      bytes.seek(blockAddress);
      decompressBlock();

[epotyom]

Good idea, thanks for the suggestion! We still need to handle block < 0, but it looks better now and seekBlock leaves term dict in a consistent state.

[zhaih]

Actually I just tried it myself and this will always reproduce the error:

        actual.seekExact(0);
        actual.seekCeil(new BytesRef(""));
        for (int i = 0; i < TERMS_DICT_BLOCK_LZ4_SIZE; i++) {
          actual.next();
        }

What happened is really tricky:
First, when the bug happens, the ord must be 0, otherwise as I previously said everything will be nicely reset by seekExact(0). So only when ord == 0, when we call a seekCeil with some small value the ord and bytesRef will be out of sync.
However at this moment it's totally ok to call next() because the next ord is 1 (so we don't need to decompressBlock) and we have a blockInput that is caching the decompressed bytes when the last time we call decompressBlock (remember the ord must be 0, so before the seekCeil, someone must have already called decompressBlock in some way, either next or seekXX)
The disaster happens when we keep calling next() until we have to decompress the next block, as we're previously always using cached blockInput so bytes kept untouched, and as @epotyom described we read block length and thought it was term length and overfilled the term buffer.

[zhaih]

Thanks for making the unit test!

[zhaih]

Also pls add an entry to CHANGES.txt :)

[zhaih]

LGTM, thanks!

[zhaih]

Merged and backported

[epotyom]

@zhaih thank you for reviewing and merging!