From 94238fb693b711e0f481baa6319e220b53c11194 Mon Sep 17 00:00:00 2001 From: Masaaki Hirotsu Date: Sat, 3 Oct 2026 09:21:25 +0900 Subject: [PATCH 1/5] fix(persist): keep sync saves ordered behind slow async writes A sync save waited at most one second for queued async updates and then wrote anyway, so a slower in-flight save could finish afterwards and overwrite it with an older snapshot. Sync updates now join the same queue and run with blocking I/O when their turn comes; if the queue does not drain in time they are withdrawn and reported as failed. --- lua/peekstack/persist/orchestrator.lua | 87 +++++++++++++++----- tests/persist_sessions_spec.lua | 109 +++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 21 deletions(-) diff --git a/lua/peekstack/persist/orchestrator.lua b/lua/peekstack/persist/orchestrator.lua index f820cca..4884fa2 100644 --- a/lua/peekstack/persist/orchestrator.lua +++ b/lua/peekstack/persist/orchestrator.lua @@ -112,6 +112,7 @@ end ---@field path string ---@field mutate fun(data: PeekstackStoreData): boolean ---@field on_done? fun(success: boolean) +---@field sync? boolean read and write with blocking I/O when its turn comes ---@type PeekstackPersistUpdate[] local update_queue = {} @@ -126,17 +127,24 @@ local function run_next_update() update_running = true local function finish(success) + -- Release the queue before the callback so an update it enqueues (e.g. a + -- sync save from a PeekstackSave handler) can run instead of waiting on + -- this finished one. Earlier queued updates still go first. + update_running = false if update.on_done then - -- A throwing callback must not wedge the queue with update_running=true. + -- A throwing callback must not stop the queue. local ok, err = pcall(update.on_done, success) if not ok then notify.warn("Session update callback failed: " .. tostring(err)) end end - run_next_update() + if not update_running then + run_next_update() + end end - M.read_async(update.path, function(data) + ---@param data PeekstackStoreData + local function apply(data) local ok, keep = pcall(update.mutate, data) if not ok then notify.warn("Failed to update session data: " .. tostring(keep)) @@ -147,8 +155,26 @@ local function run_next_update() finish(false) return end - M.write_async(update.path, data, finish) - end) + if update.sync then + finish(M.write_sync(update.path, data)) + else + M.write_async(update.path, data, finish) + end + end + + if update.sync then + apply(M.read_sync(update.path)) + else + M.read_async(update.path, apply) + end +end + +---@param update PeekstackPersistUpdate +local function enqueue(update) + update_queue[#update_queue + 1] = update + if not update_running then + run_next_update() + end end ---Asynchronously read, mutate and write back store data at `path`. @@ -161,34 +187,53 @@ end ---@param mutate fun(data: PeekstackStoreData): boolean ---@param on_done? fun(success: boolean) function M.update_async(path, mutate, on_done) - update_queue[#update_queue + 1] = { path = path, mutate = mutate, on_done = on_done } - if not update_running then - run_next_update() - end + enqueue({ path = path, mutate = mutate, on_done = on_done }) end ----Upper bound for draining in-flight async updates before a sync update. -local UPDATE_SYNC_DRAIN_MS = 1000 +---Upper bound for waiting on queued updates ahead of a sync update. +local UPDATE_SYNC_TIMEOUT_MS = 1000 ---Synchronously read, mutate and write back store data at `path`. ----Drains queued async updates first (bounded by UPDATE_SYNC_DRAIN_MS) so a ----sync save issued while an async save is mid-flight does not race it; the ----sync write itself then runs atomically from Lua's point of view. +---The update joins the same queue as async updates, so it never overlaps an +---in-flight write. If earlier updates do not finish within +---UPDATE_SYNC_TIMEOUT_MS, the update is withdrawn and reported as failed +---rather than racing them with a write that one of them could later clobber. ---@param path string ---@param mutate fun(data: PeekstackStoreData): boolean ---@return boolean success whether the data was written function M.update_sync(path, mutate) - if update_running then - vim.wait(UPDATE_SYNC_DRAIN_MS, function() - return not update_running + ---@type boolean? + local result = nil + ---@type PeekstackPersistUpdate + local update = { + path = path, + mutate = mutate, + sync = true, + on_done = function(success) + result = success + end, + } + enqueue(update) + + if result == nil then + vim.wait(UPDATE_SYNC_TIMEOUT_MS, function() + return result ~= nil end, 10) end + if result ~= nil then + return result + end - local data = M.read_sync(path) - if not mutate(data) then - return false + -- A sync update runs to completion once dequeued, so a pending result means + -- it is still queued behind a slow write. + for i, queued in ipairs(update_queue) do + if queued == update then + table.remove(update_queue, i) + break + end end - return M.write_sync(path, data) + notify.warn("Timed out waiting for pending session writes: " .. path) + return false end ---Reset the in-memory session cache and drop queued updates. diff --git a/tests/persist_sessions_spec.lua b/tests/persist_sessions_spec.lua index 6f247cc..60f56ec 100644 --- a/tests/persist_sessions_spec.lua +++ b/tests/persist_sessions_spec.lua @@ -368,6 +368,115 @@ describe("peekstack.persist.sessions", function() assert.is_not_nil(data.sessions.sync_side) end) + it("should run a sync save requested from an async save callback", function() + push_popup("nested_sync", { title = "Nested" }) + + local nested_saved = nil + persist.save_current("outer_async", { + silent = true, + on_done = function() + persist.save_current("nested_sync", { + silent = true, + sync = true, + on_done = function(success) + nested_saved = success + end, + }) + end, + }) + + local started = vim.uv.now() + local waited = vim.wait(wait_timeout_ms, function() + return nested_saved ~= nil + end, wait_interval_ms) + assert.is_true(waited, "Timed out waiting for the nested sync save") + assert.is_true(nested_saved) + assert.is_true(vim.uv.now() - started < 1000, "the nested sync save should not wait for its own caller") + + local data = migrate.ensure(read_and_wait(test_scope)) + assert.is_not_nil(data.sessions.outer_async) + assert.is_not_nil(data.sessions.nested_sync) + end) + + it("should fail a sync save instead of racing an async write that outlives the wait", function() + push_popup("sync_timeout", { title = "Slow" }) + + local original_write = store.write + local ok, err = pcall(function() + -- Hold the first async write past the sync wait, then let it finish. + local delayed = false + store.write = function(path, data, opts) + if delayed then + return original_write(path, data, opts) + end + delayed = true + vim.defer_fn(function() + original_write(path, data, opts) + end, 1300) + end + + local slow_done = nil + persist.save_current("slow_async", { + silent = true, + on_done = function(success) + slow_done = success + end, + }) + + local warnings = {} + local original_notify = vim.notify + vim.notify = function(msg, level) + if level == vim.log.levels.WARN then + warnings[#warnings + 1] = msg + end + end + local sync_saved = nil + persist.save_current("sync_after_timeout", { + silent = true, + sync = true, + on_done = function(success) + sync_saved = success + end, + }) + vim.notify = original_notify + + assert.is_false(sync_saved) + assert.is_nil(slow_done, "the delayed async write should still be pending") + assert.is_true(#warnings > 0 and warnings[1]:find("Timed out", 1, true) ~= nil) + + local waited = vim.wait(2000, function() + return slow_done ~= nil + end, wait_interval_ms) + assert.is_true(waited, "Timed out waiting for the delayed async save") + assert.is_true(slow_done) + + -- The queue keeps running after the withdrawn sync update. + local next_done = nil + persist.save_current("after_timeout", { + silent = true, + on_done = function(success) + next_done = success + end, + }) + local next_waited = vim.wait(wait_timeout_ms, function() + return next_done ~= nil + end, wait_interval_ms) + assert.is_true(next_waited, "Timed out waiting for the follow-up save") + assert.is_true(next_done) + + local data = migrate.ensure(read_and_wait(test_scope)) + assert.is_not_nil(data.sessions.slow_async) + assert.is_not_nil(data.sessions.after_timeout) + assert.is_nil(data.sessions.sync_after_timeout) + end) + + store.write = original_write + + if not ok then + error(err) + end + end) + it("should keep processing updates after an on_done callback throws", function() push_popup("throwing_callback", { title = "Throw" }) From 1ac1edde5c9890912888ceb01a5fe3fd29486a73 Mon Sep 17 00:00:00 2001 From: Masaaki Hirotsu Date: Sat, 3 Oct 2026 09:22:58 +0900 Subject: [PATCH 2/5] fix(persist): keep the session store intact when it cannot be read Read errors, undecodable JSON and unknown store versions were all treated as an empty store, so the next save, delete or rename replaced every existing session. Reads now return nil on failure, which aborts the update and leaves the cache untouched; only a missing file counts as empty. Reads also go to EOF on the opened descriptor instead of a size taken from an earlier stat, so a store replaced by an atomic rename is no longer truncated mid-read. --- lua/peekstack/persist/codec.lua | 10 +- lua/peekstack/persist/migrate.lua | 7 ++ lua/peekstack/persist/orchestrator.lua | 47 +++++--- lua/peekstack/persist/service.lua | 6 +- lua/peekstack/persist/store.lua | 106 +++++++++++------- tests/persist_sessions_spec.lua | 96 ++++++++++++++++ tests/persist_store_spec.lua | 148 +++++++++++++++++++++---- 7 files changed, 340 insertions(+), 80 deletions(-) diff --git a/lua/peekstack/persist/codec.lua b/lua/peekstack/persist/codec.lua index 1a513ef..2a78900 100644 --- a/lua/peekstack/persist/codec.lua +++ b/lua/peekstack/persist/codec.lua @@ -18,18 +18,20 @@ function M.encode(data) return encoded end +---Decode store file content. Empty content is an empty store; content that +---is not a JSON object returns nil so the caller does not overwrite it. ---@param path string ----@param data string? ----@return PeekstackStoreData +---@param data string +---@return PeekstackStoreData? function M.decode(path, data) - if not data or data == "" then + if data == "" then return M.empty_data() end local ok, decoded = pcall(vim.json.decode, data) if not ok or type(decoded) ~= "table" then notify.warn("Failed to decode session data: " .. path) - return M.empty_data() + return nil end return decoded diff --git a/lua/peekstack/persist/migrate.lua b/lua/peekstack/persist/migrate.lua index a696191..ee87522 100644 --- a/lua/peekstack/persist/migrate.lua +++ b/lua/peekstack/persist/migrate.lua @@ -60,6 +60,13 @@ local function normalize_session(session) return session end +---Whether `version` is a store format this module can read. +---@param version any +---@return boolean +function M.supports(version) + return version == 1 or version == 2 +end + ---Ensure data is in the correct format (migration helper) ---@param data any ---@return PeekstackStoreData diff --git a/lua/peekstack/persist/orchestrator.lua b/lua/peekstack/persist/orchestrator.lua index 4884fa2..c91da92 100644 --- a/lua/peekstack/persist/orchestrator.lua +++ b/lua/peekstack/persist/orchestrator.lua @@ -18,9 +18,19 @@ function M.store_path() return fs.scope_path(SCOPE) end ----@param data PeekstackStoreData ----@return PeekstackStoreData -function M.ensure_data(data) +---Migrate data read from `path`. Unsupported versions (e.g. a store written +---by a newer release) yield nil so that the file is left untouched. +---@param path string +---@param data PeekstackStoreData? +---@return PeekstackStoreData? +local function ensure_data(path, data) + if not data then + return nil + end + if not migrate.supports(data.version) then + notify.warn(string.format("Unsupported session data version %s: %s", tostring(data.version), path)) + return nil + end return migrate.ensure(data) end @@ -38,44 +48,50 @@ function M.ensure_enabled(silent) end ---Asynchronously read store data and pass migrated data to `on_done`. +---`data` is nil when the store could not be read or decoded; callers must +---then leave the file alone instead of treating it as empty. ---Does NOT touch the cache; callers that want to refresh it should use ---`refresh_cache_async` instead. This keeps save/delete/rename flows from ---updating the cache before a successful write. ---@param path string ----@param on_done fun(data: PeekstackStoreData) +---@param on_done fun(data: PeekstackStoreData?) function M.read_async(path, on_done) store.read(path, { on_done = function(read_data) - on_done(M.ensure_data(read_data)) + on_done(ensure_data(path, read_data)) end, }) end ---Synchronously read and migrate store data without touching the cache. ---@param path string ----@return PeekstackStoreData +---@return PeekstackStoreData? data nil when the store could not be read function M.read_sync(path) - return M.ensure_data(store.read_sync(path)) + return ensure_data(path, store.read_sync(path)) end ---Asynchronously read store data and refresh the cache from disk. ---Used by read-only flows (restore, list_sessions) that should reflect the ----latest persisted state in memory. +---latest persisted state in memory. A failed read keeps the cache as is. ---@param path string ----@param on_done fun(data: PeekstackStoreData) +---@param on_done fun(data: PeekstackStoreData?) function M.refresh_cache_async(path, on_done) M.read_async(path, function(data) - cache.update(path, data) + if data then + cache.update(path, data) + end on_done(data) end) end ---Synchronously read store data and refresh the cache from disk. ---@param path string ----@return PeekstackStoreData +---@return PeekstackStoreData? data nil when the store could not be read function M.refresh_cache_sync(path) local data = M.read_sync(path) - cache.update(path, data) + if data then + cache.update(path, data) + end return data end @@ -143,8 +159,13 @@ local function run_next_update() end end - ---@param data PeekstackStoreData + ---@param data PeekstackStoreData? local function apply(data) + if not data then + -- Never write over a store that could not be read. + finish(false) + return + end local ok, keep = pcall(update.mutate, data) if not ok then notify.warn("Failed to update session data: " .. tostring(keep)) diff --git a/lua/peekstack/persist/service.lua b/lua/peekstack/persist/service.lua index 6f4e79c..18cee2a 100644 --- a/lua/peekstack/persist/service.lua +++ b/lua/peekstack/persist/service.lua @@ -145,6 +145,10 @@ function M.restore(name, opts) -- move to another window before the session is restored. local root_winid = sessions.resolve_root_winid(opts and opts.root_winid or nil) orchestrator.refresh_cache_async(orchestrator.store_path(), function(data) + if not data then + finish(false) + return + end local session = data.sessions[resolved_name] if not session or not session.items or #session.items == 0 then @@ -210,7 +214,7 @@ function M.list_sessions(opts) local path = orchestrator.store_path() if on_done then orchestrator.refresh_cache_async(path, function(data) - on_done(data.sessions or {}) + on_done(data and data.sessions or {}) end) elseif not orchestrator.cache_loaded(path) then orchestrator.refresh_cache_sync(path) diff --git a/lua/peekstack/persist/store.lua b/lua/peekstack/persist/store.lua index 581b0ac..54af3e1 100644 --- a/lua/peekstack/persist/store.lua +++ b/lua/peekstack/persist/store.lua @@ -29,66 +29,96 @@ local function ensure_parent_dir(path) return true end +local READ_CHUNK_SIZE = 64 * 1024 + +---Turn the outcome of reading the store file into store data. +---A missing file is an empty store, but any other I/O error or undecodable +---content yields nil so callers never overwrite data they could not read. +---@param path string +---@param err string? libuv error message +---@param content string? +---@return PeekstackStoreData? +local function resolve_read(path, err, content) + if err then + if vim.startswith(err, "ENOENT") then + return codec.empty_data() + end + notify.warn(string.format("Failed to read session data: %s (%s)", path, err)) + return nil + end + return codec.decode(path, content or "") +end + +---Read the store file until EOF on a single descriptor, so a file replaced by +---an atomic rename is never read as a mix of old size and new content. ---@param path string store file path, resolved by the caller ----@param opts { on_done: fun(data: PeekstackStoreData) } +---@param opts { on_done: fun(data: PeekstackStoreData?) } `data` is nil when the store could not be read function M.read(path, opts) local on_done = opts and opts.on_done or nil if not on_done then return end - vim.uv.fs_stat(path, function(stat_err, stat) - if stat_err or not stat or stat.size == 0 then - vim.schedule(function() - on_done(codec.empty_data()) - end) + ---@param err string? + ---@param content string? + local function finish(err, content) + vim.schedule(function() + on_done(resolve_read(path, err, content)) + end) + end + + vim.uv.fs_open(path, "r", 438, function(open_err, fd) + if not fd then + finish(open_err or "open failed") return end - vim.uv.fs_open(path, "r", 438, function(open_err, fd) - if open_err or not fd then - vim.schedule(function() - on_done(codec.empty_data()) - end) - return - end - - vim.uv.fs_read(fd, stat.size, 0, function(read_err, data) - vim.uv.fs_close(fd) - if read_err or not data or data == "" then - vim.schedule(function() - on_done(codec.empty_data()) - end) - return + local chunks = {} + local offset = 0 + local function read_next() + vim.uv.fs_read(fd, READ_CHUNK_SIZE, offset, function(read_err, chunk) + if read_err or not chunk then + vim.uv.fs_close(fd) + finish(read_err or "read failed") + elseif chunk == "" then + vim.uv.fs_close(fd) + finish(nil, table.concat(chunks)) + else + chunks[#chunks + 1] = chunk + offset = offset + #chunk + read_next() end - vim.schedule(function() - on_done(codec.decode(path, data)) - end) end) - end) + end + read_next() end) end ---@param path string store file path, resolved by the caller ----@return PeekstackStoreData +---@return PeekstackStoreData? data nil when the store could not be read function M.read_sync(path) - local stat = vim.uv.fs_stat(path) - if not stat or stat.size == 0 then - return codec.empty_data() - end - - local fd = vim.uv.fs_open(path, "r", 438) + local fd, open_err = vim.uv.fs_open(path, "r", 438) if not fd then - return codec.empty_data() + return resolve_read(path, open_err or "open failed") end - local data = vim.uv.fs_read(fd, stat.size, 0) - pcall(vim.uv.fs_close, fd) - if not data or data == "" then - return codec.empty_data() + local chunks = {} + local offset = 0 + while true do + local chunk, read_err = vim.uv.fs_read(fd, READ_CHUNK_SIZE, offset) + if not chunk then + pcall(vim.uv.fs_close, fd) + return resolve_read(path, read_err or "read failed") + end + if chunk == "" then + break + end + chunks[#chunks + 1] = chunk + offset = offset + #chunk end + pcall(vim.uv.fs_close, fd) - return codec.decode(path, data) + return resolve_read(path, nil, table.concat(chunks)) end ---@param path string store file path, resolved by the caller diff --git a/tests/persist_sessions_spec.lua b/tests/persist_sessions_spec.lua index 60f56ec..afd26af 100644 --- a/tests/persist_sessions_spec.lua +++ b/tests/persist_sessions_spec.lua @@ -477,6 +477,102 @@ describe("peekstack.persist.sessions", function() end end) + describe("when the store cannot be read", function() + ---@param fn fun() + local function silence_warnings(fn) + local original_notify = vim.notify + vim.notify = function() end + local ok, err = pcall(fn) + vim.notify = original_notify + if not ok then + error(err) + end + end + + ---@param content string + local function write_raw(content) + local path = fs.scope_path(test_scope) + vim.fn.mkdir(vim.fs.dirname(path), "p") + assert.equals(0, vim.fn.writefile({ content }, path, "b")) + end + + ---@return string + local function read_raw() + return table.concat(vim.fn.readfile(fs.scope_path(test_scope), "b"), "\n") + end + + ---@param name string + ---@param opts? { sync?: boolean } + ---@return boolean? + local function save(name, opts) + local saved = nil + persist.save_current(name, { + silent = true, + sync = opts and opts.sync or false, + on_done = function(success) + saved = success + end, + }) + vim.wait(wait_timeout_ms, function() + return saved ~= nil + end, wait_interval_ms) + return saved + end + + for _, case in ipairs({ + { name = "corrupt JSON", content = '{"version": 2, "sessions": {"keep": ' }, + { name = "an unknown version", content = '{"version":99,"sessions":{"keep":{}}}' }, + { name = "a JSON value that is not an object", content = "[1, 2]" }, + }) do + it("should not overwrite " .. case.name, function() + push_popup("unreadable_" .. case.name:gsub("%W", "_")) + write_raw(case.content) + + silence_warnings(function() + assert.is_false(save("async_overwrite")) + assert.is_false(save("sync_overwrite", { sync = true })) + assert.same({}, persist.list_sessions({ silent = true })) + end) + + assert.equals(case.content, read_raw()) + end) + end + + it("should not overwrite existing sessions when reading fails with EIO", function() + push_popup("unreadable_eio") + write_and_wait(test_scope, { + version = 2, + sessions = { keep = { items = {}, meta = { created_at = 1, updated_at = 1 } } }, + }) + local before = read_raw() + + local original_fs_read = vim.uv.fs_read + local ok, err = pcall(silence_warnings, function() + vim.uv.fs_read = function(_fd, _size, _offset, callback) + if callback then + callback("EIO: i/o error", nil) + return + end + return nil, "EIO: i/o error", "EIO" + end + assert.is_false(save("async_overwrite")) + assert.is_false(save("sync_overwrite", { sync = true })) + persist.delete_session("keep") + persist.rename_session("keep", "renamed") + vim.wait(100) + end) + vim.uv.fs_read = original_fs_read + if not ok then + error(err) + end + + assert.equals(before, read_raw()) + local data = migrate.ensure(read_and_wait(test_scope)) + assert.is_not_nil(data.sessions.keep) + assert.is_nil(data.sessions.renamed) + end) + end) + it("should keep processing updates after an on_done callback throws", function() push_popup("throwing_callback", { title = "Throw" }) diff --git a/tests/persist_store_spec.lua b/tests/persist_store_spec.lua index 55503ae..ee69dcc 100644 --- a/tests/persist_store_spec.lua +++ b/tests/persist_store_spec.lua @@ -39,7 +39,25 @@ describe("peekstack.persist.store", function() return done end, wait_interval_ms) assert.is_true(ok, "Timed out waiting for store read") - return result or { version = 2, sessions = {} } + return result + end + + ---@param fn fun() + ---@return string[] + local function capture_warnings(fn) + local warnings = {} + local original_notify = vim.notify + vim.notify = function(msg, level) + if level == vim.log.levels.WARN then + table.insert(warnings, msg) + end + end + local ok, err = pcall(fn) + vim.notify = original_notify + if not ok then + error(err) + end + return warnings end ---@param path string @@ -119,23 +137,86 @@ describe("peekstack.persist.store", function() assert.same(data, result) end) - it("returns empty data and warns for invalid JSON", function() + it("returns nil and warns for invalid JSON", function() write_raw_and_wait(test_path, "{ invalid json") - local warnings = {} - local original_notify = vim.notify - vim.notify = function(msg, level) - if level == vim.log.levels.WARN then - table.insert(warnings, msg) + local result = "unset" + local warnings = capture_warnings(function() + result = read_and_wait(test_path) + end) + + assert.is_nil(result) + assert.is_true(#warnings > 0, "should warn about decode failure") + assert.is_true(warnings[1]:find("Failed to decode", 1, true) ~= nil) + end) + + it("returns nil and warns when reading fails with an I/O error", function() + local original_fs_read = vim.uv.fs_read + local result = "unset" + local warnings = capture_warnings(function() + vim.uv.fs_read = function(fd, size, offset, callback) + if callback then + callback("EIO: i/o error", nil) + return + end + return original_fs_read(fd, size, offset) + end + local ok, err = pcall(function() + result = read_and_wait(test_path) + end) + vim.uv.fs_read = original_fs_read + if not ok then + error(err) end + end) + + assert.is_nil(result) + assert.is_true(#warnings > 0 and warnings[1]:find("EIO", 1, true) ~= nil) + end) + + it("reads the whole file when it is replaced by a larger one before opening", function() + local original = { version = 2, sessions = {} } + write_and_wait(test_path, original) + local replacement = { version = 2, sessions = {} } + for i = 1, 200 do + replacement.sessions["session_" .. i] = { items = {}, meta = { created_at = i, updated_at = i } } end - local result = read_and_wait(test_path) + -- Swap the file in between the moment a read is requested and the open, + -- the window where a size taken from an earlier stat would go stale. + local original_fs_open = vim.uv.fs_open + local result = nil + local ok, err = pcall(function() + vim.uv.fs_open = function(path, flags, mode, callback) + if callback and path == test_path then + vim.uv.fs_open = original_fs_open + assert.is_true(store.write_sync(test_path, replacement)) + end + return original_fs_open(path, flags, mode, callback) + end + result = read_and_wait(test_path) + end) + vim.uv.fs_open = original_fs_open + if not ok then + error(err) + end - vim.notify = original_notify - assert.same({ version = 2, sessions = {} }, result) - assert.is_true(#warnings > 0, "should warn about decode failure") - assert.is_true(warnings[1]:find("Failed to decode", 1, true) ~= nil) + assert.same(replacement, result) + end) + + it("reads content larger than a single read chunk", function() + local data = { version = 2, sessions = {} } + for i = 1, 2000 do + data.sessions["large_session_" .. i] = { + items = { { uri = "file:///tmp/large_" .. i .. ".lua", title = string.rep("x", 40) } }, + meta = { created_at = i, updated_at = i }, + } + end + write_and_wait(test_path, data) + assert.is_true(vim.uv.fs_stat(test_path).size > 64 * 1024) + + assert.same(data, read_and_wait(test_path)) + assert.same(data, store.read_sync(test_path)) end) it("read_sync returns data for valid store content", function() @@ -154,25 +235,44 @@ describe("peekstack.persist.store", function() assert.same(data, result) end) - it("read_sync returns empty data and warns for invalid JSON", function() + it("read_sync returns nil and warns for invalid JSON", function() write_raw_and_wait(test_path, "{ invalid json") - local warnings = {} - local original_notify = vim.notify - vim.notify = function(msg, level) - if level == vim.log.levels.WARN then - table.insert(warnings, msg) - end - end - - local result = store.read_sync(test_path) + local result = "unset" + local warnings = capture_warnings(function() + result = store.read_sync(test_path) + end) - vim.notify = original_notify - assert.same({ version = 2, sessions = {} }, result) + assert.is_nil(result) assert.is_true(#warnings > 0, "should warn about decode failure") assert.is_true(warnings[1]:find("Failed to decode", 1, true) ~= nil) end) + it("read_sync returns nil when reading fails with an I/O error", function() + local original_fs_read = vim.uv.fs_read + local result = "unset" + local warnings = capture_warnings(function() + vim.uv.fs_read = function() + return nil, "EIO: i/o error", "EIO" + end + local ok, err = pcall(function() + result = store.read_sync(test_path) + end) + vim.uv.fs_read = original_fs_read + if not ok then + error(err) + end + end) + + assert.is_nil(result) + assert.is_true(#warnings > 0 and warnings[1]:find("EIO", 1, true) ~= nil) + end) + + it("read_sync returns empty data when the file is missing", function() + delete_and_wait(test_path) + assert.same({ version = 2, sessions = {} }, store.read_sync(test_path)) + end) + it("write_sync stores data that can be read back", function() local data = { version = 2, From 61d1ee3ceddbb1320a9bf112787bdf31f511ff6c Mon Sep 17 00:00:00 2001 From: Masaaki Hirotsu Date: Sat, 3 Oct 2026 09:24:18 +0900 Subject: [PATCH 3/5] fix(config): require finite integers for counts, sizes and durations Numeric settings only had to be numbers within bounds, so a fractional persist.max_items passed validation and made the session item slice come back empty, and NaN slipped through every bounds check. These settings now reject fractions, NaN and infinities and fall back to the default with a warning that names it; ratios reject NaN as well. --- .../config/validate/rules/persist.lua | 4 +- .../config/validate/rules/picker.lua | 2 +- lua/peekstack/config/validate/rules/ui.lua | 26 +++++----- lua/peekstack/config/validate/shared.lua | 47 ++++++++--------- tests/config_spec.lua | 50 ++++++++++++++++++- tests/persist_sessions_spec.lua | 16 ++++++ 6 files changed, 102 insertions(+), 43 deletions(-) diff --git a/lua/peekstack/config/validate/rules/persist.lua b/lua/peekstack/config/validate/rules/persist.lua index 40449ba..ff47399 100644 --- a/lua/peekstack/config/validate/rules/persist.lua +++ b/lua/peekstack/config/validate/rules/persist.lua @@ -5,7 +5,7 @@ local M = {} ---@type PeekstackConfigFieldRule[] local PERSIST_RULES = { { key = "enabled", validate = shared.field_type("boolean") }, - { key = "max_items", validate = shared.field_number_range({ min = 1 }) }, + { key = "max_items", validate = shared.field_integer_range({ min = 1 }) }, } ---@type PeekstackConfigFieldRule[] @@ -21,7 +21,7 @@ local PERSIST_AUTO_RULES = { { key = "restore", validate = shared.field_type("boolean") }, { key = "save", validate = shared.field_type("boolean") }, { key = "restore_if_empty", validate = shared.field_type("boolean") }, - { key = "debounce_ms", validate = shared.field_number_range({ min = 0, max = 600000 }) }, + { key = "debounce_ms", validate = shared.field_integer_range({ min = 0, max = 600000 }) }, { key = "save_on_leave", validate = shared.field_type("boolean") }, } diff --git a/lua/peekstack/config/validate/rules/picker.lua b/lua/peekstack/config/validate/rules/picker.lua index de3bf8d..f9818bf 100644 --- a/lua/peekstack/config/validate/rules/picker.lua +++ b/lua/peekstack/config/validate/rules/picker.lua @@ -27,7 +27,7 @@ local PICKER_RULES = { ---@type PeekstackConfigFieldRule[] local PICKER_BUILTIN_RULES = { - { key = "preview_lines", validate = shared.field_number_range({ min = 0 }) }, + { key = "preview_lines", validate = shared.field_integer_range({ min = 0 }) }, } ---@param cfg table diff --git a/lua/peekstack/config/validate/rules/ui.lua b/lua/peekstack/config/validate/rules/ui.lua index ff4ed02..1c07d44 100644 --- a/lua/peekstack/config/validate/rules/ui.lua +++ b/lua/peekstack/config/validate/rules/ui.lua @@ -21,7 +21,7 @@ local KNOWN_STACK_VIEW_POSITIONS = { "left", "right", "bottom" } ---@type PeekstackConfigFieldRule[] local UI_PATH_RULES = { { key = "base", validate = shared.field_enum(KNOWN_PATH_BASES), require_truthy = true }, - { key = "max_width", validate = shared.validate_non_negative_number }, + { key = "max_width", validate = shared.field_integer_range({ min = 0 }) }, } ---@type PeekstackConfigFieldRule[] @@ -43,14 +43,14 @@ local POPUP_SOURCE_RULES = { ---@type PeekstackConfigFieldRule[] local POPUP_HISTORY_RULES = { - { key = "max_items", validate = shared.field_number_range({ min = 1 }) }, + { key = "max_items", validate = shared.field_integer_range({ min = 1 }) }, { key = "restore_position", validate = shared.field_enum(KNOWN_RESTORE_POSITIONS), require_truthy = true }, } ---@type PeekstackConfigFieldRule[] local INLINE_PREVIEW_RULES = { { key = "enabled", validate = shared.field_type("boolean") }, - { key = "max_lines", validate = shared.field_number_range({ min = 1 }) }, + { key = "max_lines", validate = shared.field_integer_range({ min = 1 }) }, { key = "hl_group", validate = shared.field_type("string") }, { key = "close_events", validate = shared.field_event_list() }, } @@ -63,8 +63,8 @@ local QUICK_PEEK_RULES = { ---@type PeekstackConfigFieldRule[] local POPUP_AUTO_CLOSE_RULES = { { key = "enabled", validate = shared.field_type("boolean") }, - { key = "idle_ms", validate = shared.field_number_range({ min = 1 }) }, - { key = "check_interval_ms", validate = shared.field_number_range({ min = 1 }) }, + { key = "idle_ms", validate = shared.field_integer_range({ min = 1 }) }, + { key = "check_interval_ms", validate = shared.field_integer_range({ min = 1 }) }, { key = "ignore_pinned", validate = shared.field_type("boolean") }, } @@ -87,7 +87,7 @@ local TITLE_RULES = { ---@type PeekstackConfigFieldRule[] local TITLE_CONTEXT_RULES = { { key = "enabled", validate = shared.field_type("boolean") }, - { key = "max_depth", validate = shared.field_number_range({ min = 1 }) }, + { key = "max_depth", validate = shared.field_integer_range({ min = 1 }) }, { key = "separator", validate = shared.field_type("string") }, } @@ -100,25 +100,25 @@ local TITLE_ICON_RULES = { local LAYOUT_RULES = { { key = "style", validate = shared.field_enum(KNOWN_LAYOUT_STYLES), require_truthy = true }, { key = "max_ratio", validate = shared.field_ratio() }, - { key = "zindex_base", validate = shared.field_number_range({ min = 1 }) }, + { key = "zindex_base", validate = shared.field_integer_range({ min = 1 }) }, } ---@type PeekstackConfigFieldRule[] local LAYOUT_MIN_SIZE_RULES = { - { key = "w", validate = shared.field_number_range({ min = 1 }) }, - { key = "h", validate = shared.field_number_range({ min = 1 }) }, + { key = "w", validate = shared.field_integer_range({ min = 1 }) }, + { key = "h", validate = shared.field_integer_range({ min = 1 }) }, } ---@type PeekstackConfigFieldRule[] local LAYOUT_SHRINK_RULES = { - { key = "w", validate = shared.field_number_range({ min = 0 }) }, - { key = "h", validate = shared.field_number_range({ min = 0 }) }, + { key = "w", validate = shared.field_integer_range({ min = 0 }) }, + { key = "h", validate = shared.field_integer_range({ min = 0 }) }, } ---@type PeekstackConfigFieldRule[] local LAYOUT_OFFSET_RULES = { - { key = "row", validate = shared.field_number_range({ min = 0 }) }, - { key = "col", validate = shared.field_number_range({ min = 0 }) }, + { key = "row", validate = shared.field_integer_range({ min = 0 }) }, + { key = "col", validate = shared.field_integer_range({ min = 0 }) }, } ---@param map table diff --git a/lua/peekstack/config/validate/shared.lua b/lua/peekstack/config/validate/shared.lua index 21ebd75..ce63de5 100644 --- a/lua/peekstack/config/validate/shared.lua +++ b/lua/peekstack/config/validate/shared.lua @@ -34,14 +34,24 @@ function M.validate_type(path, expected_type, value, default) return value end +---Whether `value` is an integral number. NaN and infinities fail because +---`x % 1` is NaN for them. +---@param value any +---@return boolean +local function is_integer(value) + return type(value) == "number" and value % 1 == 0 +end + +---Validate counts, sizes and durations: finite integers within bounds. ---@param path string ---@param value any ----@param default number ----@param opts? { min: number?, max: number? } ----@return number -function M.validate_number_range(path, value, default, opts) - if type(value) ~= "number" then - notify.warn(string.format("%s must be a number, got %s. Falling back to %s", path, type(value), default)) +---@param default integer +---@param opts? { min: integer?, max: integer? } +---@return integer +function M.validate_integer_range(path, value, default, opts) + if not is_integer(value) then + local got = type(value) == "number" and tostring(value) or type(value) + notify.warn(string.format("%s must be an integer, got %s. Falling back to %s", path, got, default)) return default end if opts and opts.min ~= nil and value < opts.min then @@ -64,7 +74,8 @@ function M.validate_ratio(path, value, default) notify.warn(string.format("%s must be a number, got %s. Falling back to %s", path, type(value), default)) return default end - if value <= 0 or value > 1 then + -- `not (value > 0)` also rejects NaN, which fails every comparison. + if not (value > 0) or value > 1 then notify.warn(string.format("%s must be in (0, 1], got %s. Falling back to %s", path, value, default)) return default end @@ -142,22 +153,6 @@ function M.validate_enum(path, value, known, default) return value end ----@param path string ----@param value any ----@param default number ----@return number -function M.validate_non_negative_number(path, value, default) - if type(value) ~= "number" then - notify.warn(string.format("%s must be a number, got %s", path, type(value))) - return default - end - if value < 0 then - notify.warn(string.format("%s must be >= 0, got %s", path, value)) - return default - end - return value -end - ---@param expected_type string ---@return PeekstackConfigFieldValidator function M.field_type(expected_type) @@ -174,11 +169,11 @@ function M.field_enum(known) end end ----@param opts { min: number?, max: number? } +---@param opts { min: integer?, max: integer? } ---@return PeekstackConfigFieldValidator -function M.field_number_range(opts) +function M.field_integer_range(opts) return function(path, value, default) - return M.validate_number_range(path, value, default, opts) + return M.validate_integer_range(path, value, default, opts) end end diff --git a/tests/config_spec.lua b/tests/config_spec.lua index 9983b02..5849142 100644 --- a/tests/config_spec.lua +++ b/tests/config_spec.lua @@ -330,7 +330,7 @@ describe("config", function() }, }, }) - assert.is_true(has_message("ui.path.max_width must be a number")) + assert.is_true(has_message("ui.path.max_width must be an integer, got string")) assert.equals(config.defaults.ui.path.max_width, cfg.ui.path.max_width) end) @@ -485,6 +485,54 @@ describe("config", function() assert.equals(config.defaults.persist.max_items, cfg.persist.max_items) end) + it("rejects non-integer counts, sizes and durations", function() + for _, value in ipairs({ 1.5, 0 / 0, math.huge, -math.huge }) do + notifications = {} + local cfg = config.setup({ + ui = { + path = { max_width = value }, + popup = { history = { max_items = value }, auto_close = { idle_ms = value } }, + layout = { zindex_base = value, offset = { row = value } }, + }, + picker = { builtin = { preview_lines = value } }, + persist = { max_items = value, auto = { debounce_ms = value } }, + }) + local label = tostring(value) + assert.is_true(has_message("persist.max_items must be an integer, got " .. label), label) + assert.is_true(has_message("Falling back to " .. config.defaults.persist.max_items), label) + assert.equals(config.defaults.persist.max_items, cfg.persist.max_items) + assert.equals(config.defaults.persist.auto.debounce_ms, cfg.persist.auto.debounce_ms) + assert.equals(config.defaults.ui.path.max_width, cfg.ui.path.max_width) + assert.equals(config.defaults.ui.popup.history.max_items, cfg.ui.popup.history.max_items) + assert.equals(config.defaults.ui.popup.auto_close.idle_ms, cfg.ui.popup.auto_close.idle_ms) + assert.equals(config.defaults.ui.layout.zindex_base, cfg.ui.layout.zindex_base) + assert.equals(config.defaults.ui.layout.offset.row, cfg.ui.layout.offset.row) + assert.equals(config.defaults.picker.builtin.preview_lines, cfg.picker.builtin.preview_lines) + end + end) + + it("accepts integer bounds and integral floats", function() + local cfg = config.setup({ + ui = { path = { max_width = 0 } }, + persist = { max_items = 1, auto = { debounce_ms = 600000 } }, + }) + assert.equals(0, cfg.ui.path.max_width) + assert.equals(1, cfg.persist.max_items) + assert.equals(600000, cfg.persist.auto.debounce_ms) + + cfg = config.setup({ persist = { max_items = 3.0, auto = { debounce_ms = 600001 } } }) + assert.equals(3, cfg.persist.max_items) + assert.is_true(has_message("persist.auto.debounce_ms must be <= 600000, got 600001. Falling back to 1000")) + end) + + it("rejects NaN and out-of-range ratios", function() + for _, value in ipairs({ 0 / 0, 0, 1.5, math.huge }) do + local cfg = config.setup({ ui = { layout = { max_ratio = value } } }) + assert.equals(config.defaults.ui.layout.max_ratio, cfg.ui.layout.max_ratio, tostring(value)) + end + assert.equals(1, config.setup({ ui = { layout = { max_ratio = 1 } } }).ui.layout.max_ratio) + end) + it("falls back on invalid restore_position", function() local cfg = config.setup({ ui = { popup = { history = { restore_position = "invalid" } } }, diff --git a/tests/persist_sessions_spec.lua b/tests/persist_sessions_spec.lua index afd26af..3e8bcb0 100644 --- a/tests/persist_sessions_spec.lua +++ b/tests/persist_sessions_spec.lua @@ -668,6 +668,22 @@ describe("peekstack.persist.sessions", function() assert.is_not.equals(first.model.location.uri, items[1].uri) end) + it("should keep every item when max_items is not an integer", function() + local original_notify = vim.notify + vim.notify = function() end + config.setup({ persist = { enabled = true, max_items = 1.5 } }) + vim.notify = original_notify + + push_popup("fractional_max_a") + push_popup("fractional_max_b") + push_popup("fractional_max_c") + + persist.save_current("fractional", { silent = true, sync = true }) + + local data = migrate.ensure(read_and_wait(test_scope)) + assert.equals(3, #data.sessions.fractional.items) + end) + it("should notify when persist is disabled", function() config.setup({ persist = { enabled = false } }) local original_notify = vim.notify From 3518bd45305d5a229c3f6e7c96a79b283dfd4e0d Mon Sep 17 00:00:00 2001 From: Masaaki Hirotsu Date: Sat, 3 Oct 2026 09:26:33 +0900 Subject: [PATCH 4/5] fix(config): fall back to defaults when persist.auto is not a table Every other config section that must be a table is replaced with its defaults and the warning says so, but a non-table persist.auto was kept as is with a bare warning that did not say which settings applied. It now follows the same fallback and message, and the special-case options for that one field are gone. --- lua/peekstack/config/validate/rules/persist.lua | 8 +------- lua/peekstack/config/validate/shared.lua | 14 ++------------ tests/config_spec.lua | 4 ++++ 3 files changed, 7 insertions(+), 19 deletions(-) diff --git a/lua/peekstack/config/validate/rules/persist.lua b/lua/peekstack/config/validate/rules/persist.lua index ff47399..c0124b7 100644 --- a/lua/peekstack/config/validate/rules/persist.lua +++ b/lua/peekstack/config/validate/rules/persist.lua @@ -43,13 +43,7 @@ function M.validate(cfg, defaults) end if persist.auto ~= nil then - local auto = shared.ensure_table_field( - persist, - "auto", - "persist.auto", - defaults.persist.auto, - { fallback = false, message = "persist.auto must be a table" } - ) + local auto = shared.ensure_table_field(persist, "auto", "persist.auto", defaults.persist.auto) if auto then shared.apply_rules(auto, "persist.auto", defaults.persist.auto, PERSIST_AUTO_RULES) end diff --git a/lua/peekstack/config/validate/shared.lua b/lua/peekstack/config/validate/shared.lua index ce63de5..ca5fcd7 100644 --- a/lua/peekstack/config/validate/shared.lua +++ b/lua/peekstack/config/validate/shared.lua @@ -204,9 +204,8 @@ end ---@param key string ---@param path string ---@param defaults table ----@param opts? { fallback: boolean?, message: string? } ---@return table? -function M.ensure_table_field(parent, key, path, defaults, opts) +function M.ensure_table_field(parent, key, path, defaults) local value = parent[key] if value == nil then return nil @@ -215,16 +214,7 @@ function M.ensure_table_field(parent, key, path, defaults, opts) return value end - if opts and opts.message then - notify.warn(opts.message) - else - notify.warn(string.format("%s must be a table, got %s. Falling back to defaults", path, type(value))) - end - - if opts and opts.fallback == false then - return nil - end - + notify.warn(string.format("%s must be a table, got %s. Falling back to defaults", path, type(value))) parent[key] = vim.deepcopy(defaults) return parent[key] end diff --git a/tests/config_spec.lua b/tests/config_spec.lua index 5849142..6a7ca3e 100644 --- a/tests/config_spec.lua +++ b/tests/config_spec.lua @@ -834,6 +834,7 @@ describe("config", function() }, persist = { session = "default", + auto = 1, }, }) @@ -868,6 +869,9 @@ describe("config", function() assert.is_true(has_message("persist.session must be a table")) assert.equals("table", type(cfg.persist.session)) + + assert.is_true(has_message("persist.auto must be a table, got number. Falling back to defaults")) + assert.same(config.defaults.persist.auto, cfg.persist.auto) end) describe("unknown keys", function() From a6b7612e09b3506fb8fe819eb65e83d006b950b9 Mon Sep 17 00:00:00 2001 From: Masaaki Hirotsu Date: Sat, 3 Oct 2026 09:27:19 +0900 Subject: [PATCH 5/5] docs: describe store read failures, save on leave timeout and integer settings Document that an unreadable session store is never replaced with an empty one, that the save on leave gives up after waiting a second for earlier saves, and that numeric settings must be integers. --- README.md | 10 ++++++++-- doc/peekstack.txt | 13 ++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 95b034b..83a7b49 100644 --- a/README.md +++ b/README.md @@ -278,7 +278,8 @@ Configure via `require("peekstack").setup({ ... })`. > [!NOTE] > `setup()` never throws on bad config. Invalid values fall back to defaults -> with a `vim.notify` warning, and unknown or mistyped keys (e.g. `ui.popups` +> with a `vim.notify` warning naming the default, counts, sizes and durations +> must be integers (fractions, NaN and infinities are rejected), and unknown or mistyped keys (e.g. `ui.popups` > instead of `ui.popup`) are reported the same way so typos are easy to spot. ## 🧺 Picker backends (telescope / fzf-lua / snacks.nvim) @@ -357,6 +358,10 @@ for a name instead of using the default. > The storage is resolved when a save, delete, or rename is requested, so changing directory while > it is still being written does not move it to another repository. +If the storage file cannot be read (an I/O error, invalid JSON, or a version this release does not +know), peekstack warns and refuses to save, delete, or rename sessions in it instead of replacing +it with an empty store. Fix or move the file to recover. + > [!IMPORTANT] > Sessions are written as plain JSON under `vim.fn.stdpath("state") .. "/peekstack/"`. Each entry > stores the file URI, line/column range, title, provider name, pin/buffer-mode flags, parent @@ -376,7 +381,8 @@ When `persist.auto.enabled = true`, peekstack can automatically restore and save The auto session holds a single stack: the one that changed most recently, saved to the repository that was current when it changed. Leaving Neovim saves that stack even if the cursor is in another window, skips the save if no stack changed during the session, and keeps the stored session as-is -once that stack's window has been closed. +once that stack's window has been closed. The save on leave waits up to one second for earlier +saves to finish; if they are still writing, it is skipped with a warning rather than racing them. Auto persist only runs inside a git repository and always uses the repository session storage. Make sure `persist.enabled = true` as well. diff --git a/doc/peekstack.txt b/doc/peekstack.txt index 15ed8ce..e371fc0 100644 --- a/doc/peekstack.txt +++ b/doc/peekstack.txt @@ -138,6 +138,10 @@ With options: }) < +`setup()` never throws on bad config. Invalid values fall back to their +defaults with a warning that names the default. Counts, sizes and durations +must be integers; fractions, NaN and infinities are rejected. + ============================================================================== PICKER *peekstack-picker* @@ -456,6 +460,11 @@ cwd-based storage. The storage is resolved when a save, delete, or rename is requested, so changing directory while it is still being written does not move it to another repository. +If the storage file cannot be read (an I/O error, invalid JSON, or a +version this release does not know), peekstack warns and refuses to save, +delete, or rename sessions in it instead of replacing it with an empty +store. Fix or move the file to recover. + `persist.session.default_name` is used when a name is omitted, unless `persist.session.prompt_if_missing` is true. @@ -472,7 +481,9 @@ The auto session holds a single stack: the one that changed most recently, saved to the repository that was current when it changed. Leaving Neovim saves that stack even if the cursor is in another window, skips the save if no stack changed during the session, and keeps the -stored session as-is once that stack's window has been closed. +stored session as-is once that stack's window has been closed. The save on +leave waits up to one second for earlier saves to finish; if they are +still writing, it is skipped with a warning rather than racing them. Auto persistence runs only inside a git repository and always uses the repository session storage.