Forge's ModConfigSpec and FML's ChangeTracker implementations can cause infinite log spam/disk writes/shutdown hangs

[LeadAssimilator]

Minecraft Version: ALL

NeoForge Version: ALL

Logs:

[12Dec2024 06:27:33.288] [pool-13-thread-1/WARN] [net.neoforged.neoforge.common.ModConfigSpec/CORE]: Configuration LoadedConfig[...] is not correct. Correcting
[12Dec2024 06:27:35.281] [pool-13-thread-1/WARN] [net.neoforged.neoforge.common.ModConfigSpec/CORE]: Configuration LoadedConfig[...] is not correct. Correcting

Steps to Reproduce:

1. Make a mod with an incorrect ModConfigSpec whose default value doesn't pass its own validation function.
2. Ensure fml.toml disableConfigWatcher = false (should be the default)
3. Observe infinite config correcting cycle

Description of issue:

If a ModConfigSpec is incorrectly defined by a mod then an infinite config correction cycle may occur about every 2 seconds. This cycle has major consequences:

1. constant log spam
2. constant disk writes
3. multiple .toml.bak files created (thankfully capped at 5)
4. difficulty manually correcting the config (if even possible) due to racing against the config system writes and editors detecting the file changing underneath
5. prevents graceful server shutdown (jvm must be killed most of the time) Related but different root cause - Fix server stop/shutdown hang by stopping the FileWatcher when unloading configs. FancyModLoader#227
6. goes unnoticed by both mod and pack creators

Triage:

Mods typically mess up their ModConfigSpec by using the wrong define function or overload of the builder, or specifying a default value or default supplier that provides a default value which does not pass the supplied validation function or implicit validation function. Most recently, I've seen use of defineList with a default value of an empty list instead of using defineListAllowEmpty from various mods. And another recent occurrence with Mekanism is a bit more complicated, where there is a dependency between different config values such that changing one value reduces the allowed range of another making its default invalid. While the defaults were self-consistent in this case, a pack author tried to change one value which invalidated another and that invalidated value's default was now also invalid due to the dependency. For complex configurations with dependencies, it can actually be quite difficult to get it right in all cases, especially since forge tries to individually correct specific values without any knowledge of the dependency tree.

While the underlying trigger for this problem is caused by mods, forge's config correction is in error by not preventing the cycle. When forge encounters an incorrect config toml, it attempts to correct it by rebuilding the config and resetting individual broken values to their defaults. If the defaults are specified incorrectly by the mod or the defaults for a specific value have a complex interdependency tree between other values that cannot be reasonably or correctly evaluated in all cases, then the newly "corrected" config won't actually be correct and will fail validate. After the newly "corrected" config is built it is saved. If fml disableConfigWatcher = false which is the default, the newly "corrected" config file will be picked up by the watcher, and loaded. On load, it will fail validate and cause another config correction cycle.

Proposed fix:

1. Run the config validation step on an in-memory copy of the corrected config toml BEFORE writing it to disk.
2. If the validation fails after correction on the in-memory copy, log an error and do NOT write out the new file to prevent the watcher from reloading it starting the cycle again.
3. Optionally consider making this error FATAL and stop/shutdown - it would help point 6 above and ensure mod and pack authors check their work since it is painfully obviously something is wrong when the server disappears rather than an ignorable log warning.
4. Optionally write the in-memory copy that failed validation out with a different extension like ".bkn" to aid debug while still preventing the watcher from causing the cycle.

This is dual reported to FancyModLoader since both projects may need work to address this issue. See neoforged/FancyModLoader#226.

[LeadAssimilator]

Since the actual writing of the config is in fml, the majority of the fix will probably have to be there at least based on what I proposed and I should have logged this against fml first in retrospect, though some portion of the fix may need to exist in forge as well.

A broader discussion is needed between fml and forge to determine what to put where given the coupling between two. Namely, if the policy of "don't save broken configs" is accepted, where does enforcement belong?

I presently believe it might be simplest to put it in both places, though not ideal. forge's ModConfigSpec.save should probably assert isCorrect and fml's ConfigTracker should probably also assert isCorrect in various places as well.