CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync

[nivykani]

In ThreadLocalReadAheadBuffer, in the rare case that chunk metadata and file size are out of sync (e.g. bytes erased from the file after chunk metadata is calculated), we’d enter an infinite loop.

The proposed fix is to throw a CorruptSSTableException if we’re trying to read past the end of a file, rather than clamping the read.

The Cassandra Jira

[tejalchak]

nice find and fix!

[tejalchak]

Would it help with triage to include some details in the exception
"chunk read past EOF: position=<> channelSize=<>"

[nivykani]

Good idea! Updated, now we see a message like Caused by: java.io.IOException: Chunk read past EOF: requested position 2272 must be less than file size 592

[tejalchak]

lgtm

[frankgh]

Looks good, but I have a few questions

[frankgh]

NIT : formatting is off

Suggested change

	throw new CorruptSSTableException(new IOException(String.format("Chunk read past EOF: requested position %d must be less than file size %d",
	position, channelSize)),
	channel.filePath());
	throw new CorruptSSTableException(new IOException(String.format("Chunk read past EOF: requested position %d must be less than file size %d",
	position, channelSize)),
	channel.filePath());

[frankgh]

I see that we have CorruptSSTableException which might be suitable here. But it may be worth more exploration:

            throw new CorruptSSTableException(new CorruptBlockException(channel.filePath(), position, bufferSize),
                                              channel.filePath());

I think IOException will be fine here, but maybe someone with more context can comment on throwing the CorruptBlockException exception

[nivykani]

I agree CorruptBlockException is also suitable, but IOException was chosen to wrap a custom error message for CorruptSSTableException, since this could be a confusing error to debug otherwise. CorruptBlockException returns the error "corruption detected, chunk at {offset} of length {length}" but the issue isn't isolated to one specific block.

[frankgh]

should we set the compressed read ahead buffer size in the test setup as well?

[nivykani]

Good catch, changed this to directly set the buffer size, which avoids having to do this check.