util: improve settings.json error message to mention full disk #35384

I don't think this is correct. The corruption will happen when writing the file, not when reading it, which may happen a long time after the out-of-space caused the corruption and the out-of-space may no longer be visible, assuming the init can even get here when the out-of-space is still present?

to be clear, here are the full log entries that occurred:

bitcoind | Error: Settings file could not be read: bitcoind | - Settings file /home/bitcoin/.bitcoin/settings.json does not contain valid JSON. This is probably caused by disk corruption or a crash, and can be fixed by removing the file, which will reset settings to default values.

When I inspected the file /home/bitcoin/.bitcoin/settings.json it was there, but was empty (zero length).

HTH

i think the watchdog is probably the right place for disk space monitoring. it's proactive, runs continuously (every 5 mins), has a proper threshold, and shuts the node down cleanly before damage spreads. and when disk space gets critically low, the OS itself usually starts signaling that something is wrong too, other processes start failing, logs fill up, the system becomes obviously unhealthy and use-less. because of that, i'm not sure adding CheckDiskSpace inside WriteSettings meaningfully improves discoverability for the user

from #35373:

A full disk is a common real-world cause of this exact failure — the file exists but contains zero bytes, producing invalid JSON. The current message does not mention this possibility, leading users to investigate hardware corruption and crashes when the actual cause is simply insufficient disk space.

it seems like the reporter's actual pain point was not "I didn't realise my disk was full," but rather "I got a confusing error message and started suspecting hardware corruption." the full disk became obvious later; the non-obvious part was the connection between a full disk and an empty settings.json

so i wonder if the CheckDiskSpace addition may be addressing a problem that already has multiple layers of handling i.e. the watchdog from init.cpp, the OS, and other failing processes, while the real gap here is mostly a UX issue with the error wording itself

IMO, the one-line message fix already addresses the reporter's actual confusion pretty directly, which makes me think keeping the scope narrow may be the cleaner approach

hello @winterrdog,

yes, the combo of "corruption" message with no mention of "disk full" and the empty file caused the confusion.

If the disk is full, with the current WriteSettings logic...

1. open succeeds and truncates the file
2. doesn't check for errors on the write or the close
3. returns true

This explains the empty file and no record of any errors.

It would seem good to at least add error checks for both write and close, log any errors, and return false.

The current WriteSettings logic is the root cause. It "corrupts" the settings file by truncating it and silently ditching out when the disk is full.

If WriteSettings gets fixed so it doesn't clobber the settings file, the other read error message won't appear and its message text could be left as-is.

This could be done by adding logic to write to a tmp file something like this:

bool WriteSettings(const fs::path& path,
    const std::map<std::string, SettingsValue>& values,
    std::vector<std::string>& errors)
{
    SettingsValue out(SettingsValue::VOBJ);
    // Add auto-generated warning comment
    out.pushKV(SETTINGS_WARN_MSG_KEY, strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node "
                                                "is running, as any changes might be ignored or overwritten.", CLIENT_NAME));
    // Push settings values
    for (const auto& value : values) {
        out.pushKVEnd(value.first, value.second);
    }
    // Create temporary file in case the settings file is not writable
    fs::path tmp = path;
    tmp += ".tmp";

    std::ofstream file;
    file.open(tmp.std_path());
    if (file.fail()) {
        errors.emplace_back(strprintf("Error: Unable to open tmp settings file %s for writing (disk full?)", fs::PathToString(path)));
        return false;
    }
    file << out.write(/* prettyIndent= */ 4, /* indentLevel= */ 1) << std::endl;
    file.close();
    if (file.fail()) {
        errors.emplace_back(strprintf("Error: Unable to write tmp settings file %s (disk full?)", fs::PathToString(path)));
        fs::remove(tmp);
        return false;   
    }                   
    // Rename tmp file to settings file
    std::error_code ec; 
    fs::rename(tmp, path, ec);
    if (ec) {           
        errors.emplace_back(strprintf("Error: Unable to save settings file %s (disk full?)", fs::PathToString(path)));
        fs::remove(tmp);
        return false;   
    }                   
    return true;
}