presets corrupted after saving without "Overwrite with state" (esp32, needs large presets file or slow flash mode)

[softhack007]

There is an old bug in the presets saving logic that can lead to presets.json corruption, in case that the user's preset file is relatively big, or flash writing is slow (like 40Mhz dout).

How to reproduce

• you need a larger presets file; my testcase was a 200kB presets.json
• choose a preset randomly:
  • modify the name, set "overwrite with state", save
  • quickly modify the name again (make it longer), this time leave "overwrite with state" unchecked, save
  • ==> good chance that your presets.json is corrupted now (reload webUI with CTRL+F5 to verify).
  • if your presets file has survived, repeat the above procedure but this time "delete" the preset as the last step.

preliminary analysis

After a root cause analysis in the WLED-MM fork, it turned out that the code in presets.cpp (called from the preset De-Serializer functions in json.cpp, or from ) can lead to concurrent writing of the presets file on esp32. The same code is still preset here in upstream WLED.

As a secondary root cause, the function writeObjectToFile() can re-assign the global File f even when the previous File had writes pending. This leads to "holes" in the presets file.

WLED/wled00/presets.cpp

Lines 251 to 266 in e95450b

	// this is a playlist or API call
	if (sObj[F("playlist")].isNull()) {
	// we will save API call immediately (often causes presets.json corruption)
	presetToSave = 0;
	if (index <= 250) { // cannot save API calls to temporary preset (255)
	sObj.remove("o");
	sObj.remove("v");
	sObj.remove("time");
	sObj.remove(F("error"));
	sObj.remove(F("psave"));
	if (sObj["n"].isNull()) sObj["n"] = saveName;
	initPresetsFile(); // just in case if someone deleted presets.json using /edit
	writeObjectToFileUsingId(getPresetsFileName(), index, pDoc);
	presetsModifiedTime = toki.second(); //unix time
	updateFSInfo();
	}

(this even has a comment "often causes presets.json corruption" that's more than 4 years old, but still no solution available for esp32)

WLED/wled00/file.cpp

Line 276 in e95450b

f = WLED_FS.open(fileName, WLED_FS.exists(fileName) ? "r+" : "w+");

(missing: if (doCloseFile) closeFile();).

Solution

1/ fix writeObjectToFile() to always close the previous file handle, before re-assigning the file handle with WLED_FS.open()
-> bugfix in #5276

2/ prevent concurrent file writing from doSaveState(), savePreset() and deletePreset(). The existing mutex code in requestJSONBufferLock() protects the in-memory global JSON buffer, but it cannot protect the prests.json file itself when writes are still pending after releasing the buffer lock, or when a local JSON buffer was used.

@willmmiles do you have an idea for part 2? In WLED-MM I did a quick fix by using a mutex in doSaveState(), savePreset() and deletePreset(), to prevent concurrent access. I think that's a sub-optimal solution 🤔 because it adds a cluttered mutex hierarchy to a buggy design ... maybe we can leverage on your proposal in #4808 ?