RUMM-506 Prevent malforming payload due to I/O exception

[ncreated]

What and why?

🐛 Some payloads send from SDK were malformed.

The reason was not performing writes atomically:

// pseudo-code

let separatorData = ",".utf8Data
try writer.write(separatorData)
try writer.write(logData)

In rare cases, when the I/O exception was thrown for the second write, it was leaving the file's content malformed.

How?

The fix is to perform atomic writes:

// pseudo-code

let logWithSeparatorData = ",".utf8Data + logData
try writer.write(logWithSeparatorData)

The red unit test for payload malformation was added, then it was made green with a fix.

Later improvements

The File interface is not resistant for this sort of bugs, as it doesn't perform transactional write even if it looks like:

// FileTests.swift

try file.append { write in
    try write(Data([0x42, 0x42, 0x42, 0x42, 0x42])) // 5 bytes
    try write(Data([0x41, 0x41, 0x41, 0x41, 0x41])) // 5 bytes
}

As this requires additional refactoring, to not impact the hotfix scope, I will open a separate PR changing the WritableFile interface from:

func append(transaction: ((Data) throws -> Void) throws -> Void) throws

to simply:

func append(data Data) throws

Review checklist

• Feature or bugfix MUST have appropriate tests (unit, integration)
• Make sure each commit and the PR mention the Issue number or JIRA reference

[buranmert]

@ncreated i just noticed that in some cases the log has \u0000 instead of extra ,.
i guess that's because the second try write(data) throws and leaves \u0000 there, can this be true?
if so, how can we make sure that we don't leave corrupt chars in the file?

[buranmert]

note: we still can't figure out why \u0000 appears, we couldn't reproduce it and it seems very rare (possibly a few in a million logs)

we will ship this hotfix version and keep monitoring the issue

[ncreated]

I added the test to prevent also malformations in the middle of the file: 792c74c This is to additionally check if the 0x00 byte is not inserted.

[buranmert]

i suspect that when FileHandler.write(data) raises a certain kind of exception, it somehow changes offsetInFile without adding non-null bytes.

can we add something like this?

func append(transaction: ((Data) throws -> Void) throws -> Void) throws {
  let fileHandle = try FileHandle(forWritingTo: url)

  var errorThrown = true
  let initialOffset = fileHandle.offsetInFile
  defer {
    let finalOffset = fileHandle.offsetInFile
    if errorThrown && (initialOffset != finalOffset) {
      fileHandle.moveTheEndOfFile(to: initialOffset) // this should fix it but i don't know how we can do this safely
    }
    fileHandle.closeFile()
  }
  ...
  errorThrown = false
}       

EDIT: snippet changed to a potential fix from monitoring the issue

[ncreated]

can we add something like this?

@buranmert even if it works, this does neither fix the problem nor empower us with reporting. I think it's better to iterate on this issue and first see if we'll be receiving malformed logs after applying this hotfix. WDYT?

[buranmert]

yes, fixing null char issue doesn't seem trivial; let's go with this hotfix and keep monitoring 👍