Potential pathological case where Cache Advance reads no messages for a non-empty file

[bachand]

In reviewing #52 I read this code again:

CacheAdvance/Sources/CacheAdvance/CacheReader.swift

Lines 45 to 51 in ed9ece4

	func nextEncodedMessage(previousReadWasEmpty: Bool = false) throws -> Data? {
	let startingOffset = offsetInFile
	guard startingOffset != offsetInFileAtEndOfNewestMessage else {
	// We're at the last message.
	return nil
	}

We bail out of reading messages when we encounter the end of start of the newest message.

I'm wondering if there is a pathologic case where we have a file where the offset of the start of the oldest message (reader offset) equals the offset of the end of the newest message (writer offset).

| header | message 4 | message 5 | message 3 | EOF |

Let's image a file like above, with no empty space between messages. If there were no empty space between message 5 and message 3, then when someone invoked message() I believe we would be in a situation where startingOffset == offsetInFileAtEndOfNewestMessage, and we would return no messages. It seems like the current implementation of messages() assumes that there will always be some blank space between the end of the oldest message and the start of the newest message when we're in the situation where the messages have "wrapped around."

---

In writing this issue I wrote a test to I try to verify if this pathological case is real.

    func test_messagesPathologicalCase() throws {
        let message: TestableMessage = "This is a test"
        let cache = try createCache(sizedToFit: [message, message], overwritesOldMessages: true)
        try cache.append(message: message)
        try cache.append(message: message)
        try cache.append(message: message)

        XCTAssertEqual(try cache.messages().count, 2)
    }

I expected this test to fail saying that the count was 0. However, I received this failure instead:

XCTAssertEqual failed: ("1") is not equal to ("2")

Maybe it's not possible to get into this situation since this code verifies that the writer offset never equals the reader offset.

CacheAdvance/Sources/CacheAdvance/CacheAdvance.swift

Lines 225 to 226 in 2470113

	// And our writer doesn't have enough room to write a message such that it stays behind the current reader position.
	&& writer.offsetInFile + messageLength >= reader.offsetInFile

I decided it was still valuable to post this writeup so I can get confirmation from @dfed that this pathological case is not possible.

[dfed]

This is indeed a valuable write-up! The line you found in CacheAdvance.swift is indeed the line that prevents this pathological case from getting hit. I originally found it via a unit test that I believe is on main today. This issue was fixed in #40 (specifically in 6a627cf), and was briefly described in our 1.0.1 release tag. Closing since this is fixed.

[dfed]

Confirmed that we do have an explicit test on main for this case:

CacheAdvance/Tests/CacheAdvanceTests/CacheAdvanceTests.swift

Lines 267 to 306 in ed9ece4

	func test_append_inAnOverwritingCacheEvictsMinimumPossibleNumberOfMessages() throws {
	let message: TestableMessage = "test-message"
	let maxMessagesBeforeOverwriting = [message, message, message]
	let cache = try createCache(
	sizedToFit: maxMessagesBeforeOverwriting,
	overwritesOldMessages: true)
	for message in maxMessagesBeforeOverwriting {
	try cache.append(message: message)
	}
	// All of our messages have been stored and our cache is full.
	// The byte layout in the cache should now be as follows:
	// [header][length|test-message][length|test-message][length|test-message]
	// ^ reading handle ^ writing handle
	// When we read messages, we read from the current position of the reading handle – which is at the start of the oldest persisted message –
	// up until the current position of the writing handle – which is at the end of the newest persisted message. This algorithm implies that if
	// the reading handle and the writing handle are at the same position in the file, then the file is empty. Therefore, when writing a message
	// and overwriting, we must ensure that we do not accidentally write a message such that the reading handle and the writing handle end up in
	// the same position.
	// Prove to ourselves we've stored all of the messages.
	XCTAssertEqual(
	try cache.messages(),
	maxMessagesBeforeOverwriting)
	// Append one more message of the same size.
	try cache.append(message: message)
	// Because we can not have the writing handle in the same position as the writing handle, the byte layout in the cache should now be as follows:
	// [header][length|test-message] [length|test-message]
	// ^ writing handle ^ reading handle
	// In other words, we had to evict a single message in order to ensure that our writing and reading handles did not point at the same byte.
	// If more messages have been dropped, that indicates that our `prepareReaderForWriting` method has a bug.
	XCTAssertEqual(
	try cache.messages(),
	[message, message])
	}