diff --git a/src/core/SaveData.lua b/src/core/SaveData.lua index 882897bb..65e5c898 100644 --- a/src/core/SaveData.lua +++ b/src/core/SaveData.lua @@ -351,6 +351,22 @@ local function readTable(fs, name) return SaveSerializer.decode(body) end +-- Deep-copy a value folded in from the on-disk decode so the returned +-- options table never aliases the file's nested tables (SaveData must not +-- depend on src/mods/Merge.lua for this). Options data is plain tables of +-- strings/numbers/booleans/tables, so a cycle guard is belt-and-braces. +local function deepCopy(v, seen) + if type(v) ~= "table" then return v end + seen = seen or {} + if seen[v] then return seen[v] end + local copy = {} + seen[v] = copy + for k, val in pairs(v) do + copy[deepCopy(k, seen)] = deepCopy(val, seen) + end + return copy +end + -- the stub filesystem some headless harnesses inject has no remove; a -- lingering tmp/bak there is harmless local function remove(fs, name) @@ -364,13 +380,48 @@ end -- options round-trip headless (no love global). function SaveData.saveOptions(opts, fs) fs = persistFs(fs) + -- #932: options.lua is a WHOLE-FILE rewrite, so a caller that hands over a + -- PARTIAL table (just the keys it changed) would silently drop every key it + -- does not mention -- launcher-only keys like lastVersion, and keys the + -- launcher set (battleBg, tilt...) all fall back to defaults. Read the + -- on-disk file FIRST and fold caller-absent values underneath, so a delta + -- write changes only what it names. + -- + -- A table holding EVERY defaultOptions key is a full snapshot + -- (loadOptions() results, game.save.options, the RESET REBINDS / + -- activeProfile-drop paths) and stays authoritative: its absent keys are + -- deliberate deletions, so nothing folds for it. Partial tables get every + -- on-disk key they do not provide folded in (deep-copied so the caller's + -- table is never aliased). This is the reconciling rule: bindings and + -- activeProfile -- not defaultOptions members -- can be deleted by their + -- sites precisely because those sites always write full tables. + local onDisk = readTable(fs, OPTIONS_FILENAME) + local isFull = type(opts) == "table" + if isFull then + for k in pairs(SaveData.defaultOptions()) do + if opts[k] == nil then isFull = false break end + end + end + if not isFull then + local merged = {} + if type(opts) == "table" then + for k, v in pairs(opts) do merged[k] = v end + end + if type(onDisk) == "table" then + for k, v in pairs(onDisk) do + if k ~= "modOptions" and merged[k] == nil then + merged[k] = deepCopy(v) + end + end + end + opts = merged + end opts = SaveData.mergeOptions(opts) -- modOptions is per-mod nested state: fold the on-disk sub-tree -- underneath (newest value winning per key) so one caller's partial -- write cannot clobber another mod's persisted keys. Every other -- option stays on the shallow path. - local onDisk = readTable(fs, OPTIONS_FILENAME) - if onDisk and type(onDisk.modOptions) == "table" then + if type(onDisk) == "table" and type(onDisk.modOptions) == "table" then local merged = {} for modId, bucket in pairs(onDisk.modOptions) do merged[modId] = bucket diff --git a/tests/engine/options_partial_write_bug932.lua b/tests/engine/options_partial_write_bug932.lua new file mode 100644 index 00000000..9f7802a9 --- /dev/null +++ b/tests/engine/options_partial_write_bug932.lua @@ -0,0 +1,100 @@ +-- #932 "Bugs reset settings": a caller that hands saveOptions a PARTIAL +-- table (only the keys it changed) used to drop every key it did not +-- mention -- launcher-only keys like lastVersion, and keys the launcher set +-- (battleBg, tilt) all fell back to defaults. saveOptions now reads the +-- on-disk file first and folds caller-absent values underneath, so a delta +-- write changes only what it names. +-- +-- This suite pins the three-way merge against injected filesystem stubs +-- (the same { getInfo, read, write, remove } shape the other engine suites +-- use). It is ROM-free (T2 engine tier). +-- luajit tests/engine/options_partial_write_bug932.lua + +package.path = "./?.lua;./?/init.lua;" .. package.path + +local T = require("tests.harness") +local check, eq = T.check, T.eq +love = love or require("tests.love_stub") + +local SaveData = require("src.core.SaveData") + +local OPTIONS = "options.lua" + +local function memfs() + local files = {} + return { + files = files, + write = function(path, content) files[path] = content return true end, + read = function(path) return files[path] end, + remove = function(path) files[path] = nil return true end, + getInfo = function(path) + if files[path] ~= nil then return { type = "file" } end + return nil + end, + } +end + +-- Seed a save dir with the full snapshot a launcher would write: defaults, +-- plus the keys the issue cares about. lastVersion is launcher-only (not a +-- defaultOptions member) and must survive ANY write that does not name it. +local function seed(fs) + local seed = SaveData.defaultOptions() + seed.battleBg = "world" + seed.lastVersion = "blue" + seed.tilt = 1 + seed.mods = { foo = true } + seed.modOptions = { alpha = { keep = true, x = 1 } } + check(SaveData.saveOptions(seed, fs) ~= nil, "seeding lands") +end + +-- ---- launcher-only keys survive a delta write + +local fs = memfs() +seed(fs) + +-- loader-style partial write: only the mods bucket it manages. +SaveData.saveOptions({ mods = { foo = true } }, fs) +local opts = SaveData.loadOptions(fs) +eq(opts.battleBg, "world", "a partial write keeps battleBg the launcher set") +eq(opts.lastVersion, "blue", "a partial write keeps lastVersion (#932)") +eq(opts.tilt, 1, "a partial write keeps tilt the launcher set") + +-- ---- caller-present keys still win + +SaveData.saveOptions({ battleBg = "black" }, fs) +eq(SaveData.loadOptions(fs).battleBg, "black", + "a key the caller DOES provide wins over the on-disk value") +eq(SaveData.loadOptions(fs).lastVersion, "blue", + "...while the launcher-only key is still carried") + +-- ---- modOptions per-mod deep merge stays intact + +SaveData.saveOptions({ modOptions = { alpha = { x = 5 } } }, fs) +local after = SaveData.loadOptions(fs) +eq(after.modOptions.alpha.x, 5, "newest alpha value wins the per-mod merge") +eq(after.modOptions.alpha.keep, true, "alpha's untouched keys survive") +eq(after.modOptions.beta, nil, "no beta was invented by the merge") + +-- ---- full-table writes stay authoritative (bindings/activeProfile drops) + +-- The fold must NOT resurrect a key a full snapshot deliberately deletes: +-- the RESET REBINDS path nils bindings and the mod manager nils +-- activeProfile, always on full loadOptions tables. +fs = memfs() +seed(fs) +SaveData.saveOptions({ bindings = { a = 1 } }, fs) +eq(SaveData.loadOptions(fs).bindings.a, 1, "bindings is not a default member") + +local full = SaveData.loadOptions(fs) +full.bindings = nil +full.activeProfile = nil +SaveData.saveOptions(full, fs) +local reopened = SaveData.loadOptions(fs) +eq(reopened.bindings, nil, + "a full-snapshot deletion of bindings is NOT resurrected by the fold") +eq(reopened.activeProfile, nil, + "a full-snapshot deletion of activeProfile is NOT resurrected") +eq(reopened.battleBg, "world", + "the rest of the full snapshot is still what it was") + +T.finish("options_partial_write_bug932") diff --git a/tests/engine/options_write_readback_bug828.lua b/tests/engine/options_write_readback_bug828.lua index 59b9b0b7..b0ccbabc 100644 --- a/tests/engine/options_write_readback_bug828.lua +++ b/tests/engine/options_write_readback_bug828.lua @@ -204,23 +204,26 @@ eq(reopened.lastVersion, "blue", "launcher-only keys the game never reads are carried through its write") -- The corollary, and the reason the copy has to come from loadOptions: a --- caller that writes a partial literal instead of a loaded table drops every --- key it does not mention, because mergeOptions only fills DEFAULTS in around --- what it is handed (SaveData.mergeOptions). Nothing on the boot path does --- this today; the assertion is the guard rail if someone shortcuts it. +-- caller that writes a partial literal instead of a loaded table would drop +-- every key it does not mention. Since #932 that drop is closed by a +-- three-way merge -- saveOptions folds on-disk values the caller's table +-- does not carry (lastVersion here), defaults-filling only what neither side +-- has -- so even a delta write keeps the launcher's key alive. Nothing on +-- the boot path writes partials today; the assertion is the guard rail if +-- someone shortcuts it. SaveData.saveOptions({ battleLayout = "og" }, hop) -eq(SaveData.loadOptions(hop).lastVersion, nil, - "a partial write drops launcher-only keys, so the game must write the " - .. "table loadOptions handed it") +eq(SaveData.loadOptions(hop).lastVersion, "blue", + "a partial write no longer drops launcher-only keys (#932)") --- Known gap, deliberately not asserted: a copy taken BEFORE the launcher's --- write and flushed after it still wins, because saveOptions merges only --- modOptions from disk and every other key is last-writer-wins. Measured, --- not guessed (og beats a newer wide). No shipping path holds an options --- table across a launcher write -- HostShell.restart replaces the process on --- the way back to the launcher (#785, #575) and LauncherSettings.open notes --- its own cached table is only true while its modal covers the launcher -- --- so closing that gap needs a three-way merge (baseline vs caller vs disk), --- not a straight "disk wins", which would throw away real in-game changes. +-- Known gap, deliberately not asserted: a FULL copy taken BEFORE the +-- launcher's write and flushed after it still wins -- a table holding every +-- defaultOptions key is authoritative, so its og is never folded against a +-- newer wide on disk (#932 closes the PARTIAL-write drop, not this). +-- Measured, not guessed. No shipping path holds an options table across a +-- launcher write -- HostShell.restart replaces the process on the way back +-- to the launcher (#785, #575) and LauncherSettings.open notes its own +-- cached table is only true while its modal covers the launcher -- so +-- closing that gap needs a real three-way baseline (vs caller vs disk), not +-- a straight "disk wins", which would throw away real in-game changes. T.finish("options_write_readback_bug828")