You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "nivykani (via GitHub)" <gi...@apache.org> on 2026/07/14 17:33:16 UTC
[PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
nivykani opened a new pull request, #4937:
URL: https://github.com/apache/cassandra/pull/4937
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](https://issues.apache.org/jira/browse/CASSANDRA-21519)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3668813171
##########
src/java/org/apache/cassandra/io/util/ThreadLocalReadAheadBuffer.java:
##########
@@ -104,8 +105,12 @@ public void fill(long position)
{
Block block = getBlock();
ByteBuffer blockBuffer = block.buffer;
- long realPosition = Math.min(channelSize, position);
- int blockNo = (int) (realPosition / bufferSize);
+ if (position >= channelSize)
+ 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());
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#issuecomment-5123276533
Updating the test to check for the new exception
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3677863222
##########
src/java/org/apache/cassandra/io/util/ThreadLocalReadAheadBuffer.java:
##########
@@ -104,8 +105,12 @@ public void fill(long position)
{
Block block = getBlock();
ByteBuffer blockBuffer = block.buffer;
- long realPosition = Math.min(channelSize, position);
- int blockNo = (int) (realPosition / bufferSize);
+ if (position >= channelSize)
+ 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());
Review Comment:
Discussed that `CorruptBlockException` is better here, since it's caught earlier than CorruptSSTableException and is related to reading a chunk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3684725169
##########
test/unit/org/apache/cassandra/io/util/StandardCompressedChunkReaderTest.java:
##########
@@ -112,4 +115,56 @@ protected void doReads(File f, CompressionMetadata metadata, long length, boolea
MemoryUtil.clean(buffer);
}
}
+
Review Comment:
nit: extra space?
##########
test/unit/org/apache/cassandra/io/util/StandardCompressedChunkReaderTest.java:
##########
@@ -112,4 +115,56 @@ protected void doReads(File f, CompressionMetadata metadata, long length, boolea
MemoryUtil.clean(buffer);
}
}
+
Review Comment:
nit: extra newline?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3627492721
##########
src/java/org/apache/cassandra/io/util/ThreadLocalReadAheadBuffer.java:
##########
@@ -104,8 +104,10 @@ public void fill(long position)
{
Block block = getBlock();
ByteBuffer blockBuffer = block.buffer;
- long realPosition = Math.min(channelSize, position);
- int blockNo = (int) (realPosition / bufferSize);
+ if (position >= channelSize)
+ throw new CorruptSSTableException(null, channel.filePath());
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "tejalchak (via GitHub)" <gi...@apache.org>.
tejalchak commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3615943545
##########
src/java/org/apache/cassandra/io/util/ThreadLocalReadAheadBuffer.java:
##########
@@ -104,8 +104,10 @@ public void fill(long position)
{
Block block = getBlock();
ByteBuffer blockBuffer = block.buffer;
- long realPosition = Math.min(channelSize, position);
- int blockNo = (int) (realPosition / bufferSize);
+ if (position >= channelSize)
+ throw new CorruptSSTableException(null, channel.filePath());
Review Comment:
nice find and fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#issuecomment-5174426834
Committed as https://github.com/apache/cassandra/commit/79a8286d7213a7fa5a551e9ca67767d5c0495aa3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3688570575
##########
test/unit/org/apache/cassandra/io/util/StandardCompressedChunkReaderTest.java:
##########
@@ -112,4 +115,56 @@ protected void doReads(File f, CompressionMetadata metadata, long length, boolea
MemoryUtil.clean(buffer);
}
}
+
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "tejalchak (via GitHub)" <gi...@apache.org>.
tejalchak commented on PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#issuecomment-5024875896
lgtm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3661873512
##########
test/unit/org/apache/cassandra/io/util/StandardCompressedChunkReaderTest.java:
##########
@@ -112,4 +115,58 @@ protected void doReads(File f, CompressionMetadata metadata, long length, boolea
MemoryUtil.clean(buffer);
}
}
+
+
+ @Test(timeout = 10_000)
+ public void scanReaderShouldNotHangOnTruncatedFile() throws Exception
+ {
+ SequentialWriterOption writerOption = SequentialWriterOption.newBuilder().finishOnClose(false).bufferSize(1 << 10).build();
+ CompressionParams params = CompressionParams.snappy(4096, 1.1);
+
+ FileSystems.newGlobalInMemoryFileSystem();
+ File f = new File("/truncated_hang_repro.db");
+ File offsets = new File("/truncated_hang_repro.offset");
+ File digest = new File("/truncated_hang_repro.digest");
+
+ long longsToWrite = 600; // 4800 uncompressed bytes -> 2 compressed chunks (second one partial)
+ CompressionMetadata metadata;
+ try (CompressedSequentialWriter writer = new CompressedSequentialWriter(f, offsets, digest, writerOption, params,new MetadataCollector(new ClusteringComparator())))
Review Comment:
NIT formatting:
```suggestion
try (CompressedSequentialWriter writer = new CompressedSequentialWriter(f, offsets, digest, writerOption, params, new MetadataCollector(new ClusteringComparator())))
```
##########
test/unit/org/apache/cassandra/io/util/StandardCompressedChunkReaderTest.java:
##########
@@ -112,4 +115,58 @@ protected void doReads(File f, CompressionMetadata metadata, long length, boolea
MemoryUtil.clean(buffer);
}
}
+
+
+ @Test(timeout = 10_000)
+ public void scanReaderShouldNotHangOnTruncatedFile() throws Exception
+ {
+ SequentialWriterOption writerOption = SequentialWriterOption.newBuilder().finishOnClose(false).bufferSize(1 << 10).build();
+ CompressionParams params = CompressionParams.snappy(4096, 1.1);
+
+ FileSystems.newGlobalInMemoryFileSystem();
+ File f = new File("/truncated_hang_repro.db");
+ File offsets = new File("/truncated_hang_repro.offset");
+ File digest = new File("/truncated_hang_repro.digest");
+
+ long longsToWrite = 600; // 4800 uncompressed bytes -> 2 compressed chunks (second one partial)
+ CompressionMetadata metadata;
+ try (CompressedSequentialWriter writer = new CompressedSequentialWriter(f, offsets, digest, writerOption, params,new MetadataCollector(new ClusteringComparator())))
+ {
+ for (long i = 0; i < longsToWrite; i++)
+ writer.writeLong(i);
+
+ writer.sync();
+ metadata = writer.open(0);
+ }
+
+ // Sanity: read-ahead must actually engage for this test to exercise the scan path
+ Assert.assertTrue("read-ahead must be larger than chunk length for scanReader to be non-null",
+ DatabaseDescriptor.getCompressedReadAheadBufferSize() > metadata.chunkLength());
Review Comment:
should we set the compressed read ahead buffer size in the test setup as well?
##########
src/java/org/apache/cassandra/io/util/ThreadLocalReadAheadBuffer.java:
##########
@@ -104,8 +105,12 @@ public void fill(long position)
{
Block block = getBlock();
ByteBuffer blockBuffer = block.buffer;
- long realPosition = Math.min(channelSize, position);
- int blockNo = (int) (realPosition / bufferSize);
+ if (position >= channelSize)
+ 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());
Review Comment:
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
##########
src/java/org/apache/cassandra/io/util/ThreadLocalReadAheadBuffer.java:
##########
@@ -104,8 +105,12 @@ public void fill(long position)
{
Block block = getBlock();
ByteBuffer blockBuffer = block.buffer;
- long realPosition = Math.min(channelSize, position);
- int blockNo = (int) (realPosition / bufferSize);
+ if (position >= channelSize)
+ 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());
Review Comment:
NIT : formatting is off
```suggestion
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());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "tejalchak (via GitHub)" <gi...@apache.org>.
tejalchak commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3616120470
##########
src/java/org/apache/cassandra/io/util/ThreadLocalReadAheadBuffer.java:
##########
@@ -104,8 +104,10 @@ public void fill(long position)
{
Block block = getBlock();
ByteBuffer blockBuffer = block.buffer;
- long realPosition = Math.min(channelSize, position);
- int blockNo = (int) (realPosition / bufferSize);
+ if (position >= channelSize)
+ throw new CorruptSSTableException(null, channel.filePath());
Review Comment:
Would it help with triage to include some details in the exception
"chunk read past EOF: position=<> channelSize=<>"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz closed pull request #4937: CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync
URL: https://github.com/apache/cassandra/pull/4937
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-21519: Return CorruptSSTableException if chunk metadata and file size are out of sync [cassandra]
Posted by "nivykani (via GitHub)" <gi...@apache.org>.
nivykani commented on code in PR #4937:
URL: https://github.com/apache/cassandra/pull/4937#discussion_r3668915720
##########
test/unit/org/apache/cassandra/io/util/StandardCompressedChunkReaderTest.java:
##########
@@ -112,4 +115,58 @@ protected void doReads(File f, CompressionMetadata metadata, long length, boolea
MemoryUtil.clean(buffer);
}
}
+
+
+ @Test(timeout = 10_000)
+ public void scanReaderShouldNotHangOnTruncatedFile() throws Exception
+ {
+ SequentialWriterOption writerOption = SequentialWriterOption.newBuilder().finishOnClose(false).bufferSize(1 << 10).build();
+ CompressionParams params = CompressionParams.snappy(4096, 1.1);
+
+ FileSystems.newGlobalInMemoryFileSystem();
+ File f = new File("/truncated_hang_repro.db");
+ File offsets = new File("/truncated_hang_repro.offset");
+ File digest = new File("/truncated_hang_repro.digest");
+
+ long longsToWrite = 600; // 4800 uncompressed bytes -> 2 compressed chunks (second one partial)
+ CompressionMetadata metadata;
+ try (CompressedSequentialWriter writer = new CompressedSequentialWriter(f, offsets, digest, writerOption, params,new MetadataCollector(new ClusteringComparator())))
+ {
+ for (long i = 0; i < longsToWrite; i++)
+ writer.writeLong(i);
+
+ writer.sync();
+ metadata = writer.open(0);
+ }
+
+ // Sanity: read-ahead must actually engage for this test to exercise the scan path
+ Assert.assertTrue("read-ahead must be larger than chunk length for scanReader to be non-null",
+ DatabaseDescriptor.getCompressedReadAheadBufferSize() > metadata.chunkLength());
Review Comment:
Good catch, changed this to directly set the buffer size, which avoids having to do this check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org