Limit possibility of file corruption (#58)

* Fix indentation on test that catches a possible recursion loop

* Minimize chance of file corruption

* Bugfix version bump

Author: dfed

CacheAdvance.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
   s.name     = 'CacheAdvance'
-  s.version  = '1.2.1'
+  s.version  = '1.2.2'
   s.license  = 'Apache License, Version 2.0'
   s.summary  = 'A performant cache for logging systems. CacheAdvance persists log events 30x faster than SQLite.'
   s.homepage = 'https://github.com/dfed/CacheAdvance'

Sources/CacheAdvance/CacheAdvance.swift

@@ -112,16 +112,18 @@ public final class CacheAdvance<T: Codable> {
 
         let cacheHasSpaceForNewMessageBeforeEndOfFile = writer.offsetInFile + bytesNeededToStoreMessage <= header.maximumBytes
         if header.overwritesOldMessages {
-            let truncateAtOffset: UInt64?
-            if cacheHasSpaceForNewMessageBeforeEndOfFile {
-                // We have room for this message. No need to truncate.
-                truncateAtOffset = nil
-            } else {
+            if !cacheHasSpaceForNewMessageBeforeEndOfFile {
                 // This message can't be written without exceeding our maximum file length.
                 // We'll need to start writing the file from the beginning of the file.
 
                 // Trim the file to the current writer position to remove soon-to-be-abandoned data from the file.
-                truncateAtOffset = writer.offsetInFile
+                try writer.truncate(at: writer.offsetInFile)
+
+                // Update the offsetInFileAtEndOfNewestMessage in our header and reader such that we will ignore the now-deleted data in the file.
+                // If the application crashes between writing this header and writing the next message data, we'll have only lost the messages we were about to delete.
+                // If `writer.offsetInFile == FileHeader.expectedEndOfHeaderInFile`, then crashing before we update the header would lead to this now-empty file being viewed as corrupted.
+                try header.updateOffsetInFileAtEndOfNewestMessage(to: writer.offsetInFile)
+                reader.offsetInFileAtEndOfNewestMessage = writer.offsetInFile
 
                 // Set the offset back to the beginning of the file.
                 try writer.seek(to: FileHeader.expectedEndOfHeaderInFile)
@@ -144,12 +146,6 @@ public final class CacheAdvance<T: Codable> {
             // If the application crashes between writing the header and writing the message data, we'll have lost the messages between the previous offsetInFileOfOldestMessage and the new offsetInFileOfOldestMessage.
             try header.updateOffsetInFileOfOldestMessage(to: offsetInFileOfOldestMessage)
 
-            // Truncate the file if it needs truncation before we write the next message, and after we update our header.
-            // If the application crashes between truncating this message data and writing the next message, our file will still be consistent.
-            if let truncateAtOffset = truncateAtOffset {
-                try writer.truncate(at: truncateAtOffset)
-            }
-
             // Let the reader know where the oldest message begins.
             reader.offsetInFileOfOldestMessage = offsetInFileOfOldestMessage
 

Sources/CacheAdvance/CacheReader.swift

@@ -63,8 +63,8 @@ final class CacheReader {
             guard !previousReadWasEmpty else {
                 // If the previous read was also empty, then the file has been corrupted.
                 // Two empty reads in a row means that offsetInFileAtEndOfNewestMessage is incorrect.
-                // This inconsistency is likely due to a crash occurring during a message write.
-                // This issue will not occur in a current version of the library, but an earlier version was capable of creating this corruption.
+                // This inconsistency is likely due to a crash occurring during a message write, between truncating the file and updating the header values to reflect that the file was truncated.
+                // This file is actually empty, despite what the header tells us.
                 throw CacheAdvanceError.fileCorrupted
             }
             // We know the next message is at the end of the file header. Let's seek to it.

Tests/CacheAdvanceTests/CacheAdvanceTests.swift

@@ -96,34 +96,34 @@ final class CacheAdvanceTests: XCTestCase {
         XCTAssertEqual(messages, [])
     }
 
-  func test_messages_throwsFileCorruptedWhenOffsetInFileAtEndOfNewsetMessageOutOfSync() throws {
-      let randomHighValue: UInt64 = 10_1000
-      let header = try CacheHeaderHandle(
-        forReadingFrom: testFileLocation,
-        maximumBytes: randomHighValue,
-        overwritesOldMessages: true)
-      let cache = CacheAdvance<TestableMessage>(
-        fileURL: testFileLocation,
-        writer: try FileHandle(forWritingTo: testFileLocation),
-        reader: try CacheReader(forReadingFrom: testFileLocation),
-        header: try CacheHeaderHandle(
+    func test_messages_throwsFileCorruptedWhenOffsetInFileAtEndOfNewsetMessageOutOfSync() throws {
+        let randomHighValue: UInt64 = 10_1000
+        let header = try CacheHeaderHandle(
             forReadingFrom: testFileLocation,
-            maximumBytes: header.maximumBytes,
-            overwritesOldMessages: header.overwritesOldMessages),
-        decoder: JSONDecoder(),
-        encoder: JSONEncoder())
-
-      // Make sure the header data is persisted before we read it as part of the `messages()` call below.
-      try header.synchronizeHeaderData()
-      // Our file is empty. Make the file corrupted by setting the offset at end of newest message to be further in the file.
-      // This should never happen, but past versions of this repo could lead to a file having this kind of inconsistency if a crash occurred at the wrong time.
-      try header.updateOffsetInFileAtEndOfNewestMessage(
-        to: FileHeader.expectedEndOfHeaderInFile + 1)
-
-      XCTAssertThrowsError(try cache.messages()) {
-          XCTAssertEqual($0 as? CacheAdvanceError, CacheAdvanceError.fileCorrupted)
-      }
-  }
+            maximumBytes: randomHighValue,
+            overwritesOldMessages: true)
+        let cache = CacheAdvance<TestableMessage>(
+            fileURL: testFileLocation,
+            writer: try FileHandle(forWritingTo: testFileLocation),
+            reader: try CacheReader(forReadingFrom: testFileLocation),
+            header: try CacheHeaderHandle(
+                forReadingFrom: testFileLocation,
+                maximumBytes: header.maximumBytes,
+                overwritesOldMessages: header.overwritesOldMessages),
+            decoder: JSONDecoder(),
+            encoder: JSONEncoder())
+
+        // Make sure the header data is persisted before we read it as part of the `messages()` call below.
+        try header.synchronizeHeaderData()
+        // Our file is empty. Make the file corrupted by setting the offset at end of newest message to be further in the file.
+        // This should never happen, but past versions of this repo could lead to a file having this kind of inconsistency if a crash occurred at the wrong time.
+        try header.updateOffsetInFileAtEndOfNewestMessage(
+            to: FileHeader.expectedEndOfHeaderInFile + 1)
+
+        XCTAssertThrowsError(try cache.messages()) {
+            XCTAssertEqual($0 as? CacheAdvanceError, CacheAdvanceError.fileCorrupted)
+        }
+    }
 
     func test_isWritable_returnsTrueWhenStaticHeaderMetadataMatches() throws {
         let originalCache = try createCache(overwritesOldMessages: false)