From 061e836fa49e3be815c01244c9f3b1959afc3996 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 04:26:11 +0000 Subject: [PATCH] Swap/recovery: second-pass fixes from the follow-up review - Recovery works when another named buffer is current. On that path OpenFile's NotifyFilenameChanged deleted the journal (old path == new path) that was about to be replayed; the swap was lost and the file opened without the edits (pre-existing). - After recovering from a journal with a torn final record, the torn bytes are truncated before the session appends to it (ReplayFile now reports the valid prefix length). A journal that recovers nothing is removed instead of appended to. - Reload restarts the journal; recovery used to replay the edits the reload discarded. - Opening a file that is already open switches to its buffer (in OpenFile and before the recovery check). Two buffers for one file interleaved records in one journal, and closing either unlinked it. - Swap file names escape '!' and '%', so distinct paths no longer share a journal ('/x/a!b' vs '/x/a/b'). Swap files for paths that contain those characters get new names. - The gap flag is cleared when the journal restarts, and a gap that cannot be closed (buffer too large to checkpoint) is reported once. - crc32's table is initialised thread-safely; writer-thread error reports identify the buffer by journal path instead of racing on Buffer::Filename(); Close() no longer retries on EINTR (the fd is already released and may have been reused). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Command.cc | 4 + Editor.cc | 77 +++++++++++- Editor.h | 7 +- Swap.cc | 98 +++++++++++---- Swap.h | 9 +- SyscallWrappers.cc | 10 +- SyscallWrappers.h | 7 +- tests/test_swap_recovery_prompt.cc | 188 +++++++++++++++++++++++++++++ 8 files changed, 365 insertions(+), 35 deletions(-) diff --git a/Command.cc b/Command.cc index 170a69e..897b83c 100644 --- a/Command.cc +++ b/Command.cc @@ -5236,6 +5236,10 @@ cmd_reload_buffer(CommandContext &ctx) ctx.editor.SetStatus(std::string("Reload failed: ") + err); return false; } + // The journal recorded edits against the discarded content; restart it + // from the reloaded file, or crash recovery would replay those edits. + if (auto *sm = ctx.editor.Swap()) + sm->ResetJournal(*buf); // Try to restore the cursor to its previous position if still valid; otherwise clamp { auto rows = rows_of(*buf); diff --git a/Editor.cc b/Editor.cc index 3615242..8288036 100644 --- a/Editor.cc +++ b/Editor.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -50,6 +51,46 @@ apply_pending_line(Editor &ed, const std::size_t line1) } // namespace +namespace { +// Same normalization as Buffer::OpenFromFile (expand "~", canonical path if +// the file exists, else absolute), so a path can be compared with the +// Filename() of open buffers. +std::string +normalize_open_path(const std::string &in) +{ + std::string expanded = in; + if (!expanded.empty() && expanded[0] == '~') { + const char *home = std::getenv("HOME"); + if (home && expanded.size() >= 2 && (expanded[1] == '/' || expanded[1] == '\\')) + expanded = std::string(home) + expanded.substr(1); + else if (home && expanded.size() == 1) + expanded = std::string(home); + } + try { + std::filesystem::path p(expanded); + if (std::filesystem::exists(p)) + return std::filesystem::canonical(p).string(); + return std::filesystem::absolute(p).string(); + } catch (...) { + return expanded; + } +} +} // namespace + + +std::size_t +Editor::FindOpenBuffer(const std::string &path) const +{ + const std::string norm = normalize_open_path(path); + const auto &bufs = Buffers(); + for (std::size_t i = 0; i < bufs.size(); ++i) { + if (!bufs[i].Filename().empty() && bufs[i].Filename() == norm) + return i; + } + return static_cast(-1); +} + + Editor::Editor() { swap_ = std::make_unique(); @@ -228,6 +269,14 @@ Editor::AddBuffer(Buffer &&buf) bool Editor::OpenFile(const std::string &path, std::string &err) { + // A file that is already open is switched to, not opened again: two + // buffers for one file would share (and corrupt) one swap journal, and + // closing either would delete the other's journal. + if (const std::size_t open_idx = FindOpenBuffer(path); open_idx != static_cast(-1)) { + SwitchTo(open_idx); + return true; + } + // If the current buffer is an unnamed, empty, clean scratch buffer, reuse // it instead of creating a new one. auto &bufs_ref = Buffers(); @@ -372,10 +421,20 @@ Editor::ResolveRecoveryPrompt(const bool yes) return false; } std::string rerr; - if (!kte::SwapManager::ReplayFile(*b, swp, rerr)) { + std::uint64_t valid_bytes = 0; + if (!kte::SwapManager::ReplayFile(*b, swp, rerr, &valid_bytes)) { SetStatus("Swap recovery failed: " + rerr); return false; } + // This session keeps appending to the same journal. Drop a torn + // final record first, or new records would land behind it and the + // journal could not be replayed after another crash. + try { + if (std::filesystem::file_size(swp) > valid_bytes) + std::filesystem::resize_file(swp, valid_bytes); + } catch (...) { + // Best effort; the replay itself succeeded. + } b->SetDirty(true); apply_pending_line(*this, req.line1); SetStatus("Recovered " + req.path); @@ -436,6 +495,15 @@ Editor::ProcessPendingOpens() pending_open_.pop_front(); if (req.path.empty()) continue; + // Already open: switch to it. Its swap file is this session's live + // journal, not something to recover. + if (const std::size_t open_idx = FindOpenBuffer(req.path); open_idx != static_cast(-1)) { + SwitchTo(open_idx); + apply_pending_line(*this, req.line1); + SetStatus("Switched to " + Buffers()[open_idx].Filename()); + opened_any = true; + return opened_any; + } std::string swp = kte::SwapManager::ComputeSwapPathForFilename(req.path); bool swp_exists = false; @@ -452,7 +520,12 @@ Editor::ProcessPendingOpens() std::string rerr; if (kte::SwapManager::ReplayFile(tmp, swp, rerr)) { const std::string rec = buffer_bytes_via_views(tmp); - if (rec != orig) { + if (rec == orig) { + // Nothing to recover. Remove the journal rather than + // appending this session's records to it (it may end in a + // torn record, which would make them unreplayable). + (void) std::remove(swp.c_str()); + } else { pending_recovery_prompt_ = RecoveryPromptKind::RecoverOrDiscard; pending_recovery_open_ = req; pending_recovery_swap_path_ = swp; diff --git a/Editor.h b/Editor.h index b386b5e..3979039 100644 --- a/Editor.h +++ b/Editor.h @@ -610,7 +610,12 @@ public: // Buffers hold recorders owned by the SwapManager that attached them, so // editors sharing buffers must also share that manager (shared_swap); // otherwise closing one window destroys recorders the other still uses. - void SetSharedBuffers(std::vector *shared, kte::SwapManager *shared_swap = nullptr) + // Index of the open buffer whose file is `path` (after the same + // normalization OpenFromFile applies), or size_t(-1) if none. + [[nodiscard]] std::size_t FindOpenBuffer(const std::string &path) const; + + + void SetSharedBuffers(std::vector *shared, kte::SwapManager *shared_swap = nullptr) { shared_buffers_ = shared; shared_swap_ = shared ? shared_swap : nullptr; diff --git a/Swap.cc b/Swap.cc index 4b309a7..b10df80 100644 --- a/Swap.cc +++ b/Swap.cc @@ -5,6 +5,7 @@ #include "ErrorRecovery.h" #include +#include #include #include #include @@ -129,13 +130,23 @@ encode_path_key(std::string s) // - We strip a single leading path separator so absolute paths don't start with '!'. // - We replace both '/' and '\\' with '!'. // - We leave other characters as-is (spaces are OK on POSIX). + // - '!' and '%' in the path itself are escaped as %21 and %25, so the + // mapping is one-to-one: "/x/a!b" and "/x/a/b" used to share a journal. if (!s.empty() && (s[0] == '/' || s[0] == '\\')) s.erase(0, 1); - for (char &ch: s) { + std::string out; + out.reserve(s.size()); + for (char ch: s) { if (ch == '/' || ch == '\\') - ch = '!'; + out.push_back('!'); + else if (ch == '!') + out += "%21"; + else if (ch == '%') + out += "%25"; + else + out.push_back(ch); } - return s; + return out; } @@ -392,6 +403,9 @@ SwapManager::ResetJournal(Buffer &buf) JournalCtx &ctx = it->second; close_ctx(ctx); ctx.header_ok = false; + ctx.gap = false; // the journal restarts from the saved file + ctx.gap_chkpt_request_ns = 0; + ctx.gap_unfixable_reported = false; ctx.last_flush_ns = 0; ctx.last_fsync_ns = 0; ctx.last_chkpt_ns = 0; @@ -515,9 +529,16 @@ SwapManager::NotifyFilenameChanged(Buffer &buf) return; JournalCtx &ctx = it->second; close_ctx(ctx); - if (!old_path.empty()) + const std::string new_path = ComputeSidecarPath(buf); + // When the path is unchanged (Attach already pointed the journal at this + // file's swap), that file may be the one the caller is about to replay + // for recovery: removing it here lost the recovery. + if (!old_path.empty() && old_path != new_path) (void) std::remove(old_path.c_str()); - ctx.path = ComputeSidecarPath(buf); + ctx.gap = false; + ctx.gap_chkpt_request_ns = 0; + ctx.gap_unfixable_reported = false; + ctx.path = new_path; ctx.suspended = false; ctx.header_ok = false; ctx.last_flush_ns = 0; @@ -851,17 +872,18 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector table = [] { + std::array t{}; for (std::uint32_t i = 0; i < 256; ++i) { std::uint32_t c = i; for (int j = 0; j < 8; ++j) c = (c & 1) ? (0xEDB88320u ^ (c >> 1)) : (c >> 1); - table[i] = c; + t[i] = c; } - inited = true; - } + return t; + }(); std::uint32_t c = ~seed; for (std::size_t i = 0; i < len; ++i) c = table[(c ^ data[i]) & 0xFFu] ^ (c >> 8); @@ -1075,8 +1097,23 @@ SwapManager::RecordCheckpoint(Buffer &buf, const bool urgent_flush) // check the size before paying for a snapshot. const std::size_t nrows = buf.Nrows(); const std::size_t total_size = nrows ? buf.GetLineRange(nrows - 1).second : 0; - if (total_size > kMaxRecordPayload - 5) + if (total_size > kMaxRecordPayload - 5) { + // If the journal has a gap, only a checkpoint could close it: say so + // once rather than silently dropping every later record. + bool report = false; + { + std::lock_guard lg(mtx_); + auto it = journals_.find(&buf); + if (it != journals_.end() && it->second.gap && !it->second.gap_unfixable_reported) { + it->second.gap_unfixable_reported = true; + report = true; + } + } + if (report) + report_error("Swap journal lost a record and the buffer is too large to checkpoint; " + "crash recovery will not include edits made until the file is saved", &buf); return; + } const std::string bytes = snapshot_buffer_bytes(buf); if (bytes.size() > kMaxRecordPayload - 5) return; @@ -1196,11 +1233,18 @@ SwapManager::process_one(const Pending &p) // Log warning at most once per 60 seconds to avoid spam if (now - last > 60000000000ULL) { last_warning_ns.store(now); + // Journal path read under mtx_, not Buffer::Filename(), which the + // main thread may be assigning (see report_error). + std::string context = ""; + { + std::lock_guard lg(mtx_); + auto it = journals_.find(p.buf); + if (it != journals_.end() && !it->second.path.empty()) + context = it->second.path; + } ErrorHandler::Instance().Warning("SwapManager", "Swap operations temporarily disabled due to repeated failures (circuit breaker open)", - p.buf && !p.buf->Filename().empty() - ? p.buf->Filename() - : ""); + context); } mark_gap(); return; @@ -1365,7 +1409,8 @@ parse_u32_le(const std::vector &p, std::size_t &off, std::uint32_t bool -SwapManager::ReplayFile(Buffer &buf, const std::string &swap_path, std::string &err) +SwapManager::ReplayFile(Buffer &buf, const std::string &swap_path, std::string &err, + std::uint64_t *valid_bytes) { err.clear(); std::ifstream in(swap_path, std::ios::binary); @@ -1404,6 +1449,10 @@ SwapManager::ReplayFile(Buffer &buf, const std::string &swap_path, std::string & } restore{buf, prev_rec}; for (;;) { + // Everything before this offset replayed cleanly. + const std::streamoff record_start = in.tellg(); + if (valid_bytes && record_start >= 0) + *valid_bytes = static_cast(record_start); std::uint8_t head[4]; in.read(reinterpret_cast(head), sizeof(head)); const std::size_t got_head = static_cast(in.gcount()); @@ -1576,13 +1625,16 @@ SwapManager::ReplayFile(Buffer &buf, const std::string &swap_path, std::string & void SwapManager::report_error(const std::string &message, Buffer *buf) { - std::string context; - if (buf && !buf->Filename().empty()) { - context = buf->Filename(); - } else if (buf) { - context = ""; - } else { - context = ""; + // Identify the buffer by its journal path, read under mtx_. This runs on + // the writer thread, where reading Buffer::Filename() raced with the main + // thread assigning it (SaveAs, OpenFromFile). + std::string context = ""; + { + std::lock_guard lg(mtx_); + if (buf) { + auto it = journals_.find(buf); + context = (it != journals_.end() && !it->second.path.empty()) ? it->second.path : ""; + } } // Report to centralized error handler diff --git a/Swap.h b/Swap.h index 4d7fdac..62f1303 100644 --- a/Swap.h +++ b/Swap.h @@ -111,7 +111,13 @@ public: // On failure (corrupt/truncated/invalid), the buffer is left in whatever // state results from applying records up to the failure point; callers should // treat this as a recovery failure and surface `err`. - static bool ReplayFile(Buffer &buf, const std::string &swap_path, std::string &err); + // + // A record cut short at end of file (a crash mid-append) ends the replay + // successfully; err then describes the incomplete record. If valid_bytes + // is given, it receives the length of the journal's valid prefix, so a + // caller that keeps using the journal can truncate the torn tail first. + static bool ReplayFile(Buffer &buf, const std::string &swap_path, std::string &err, + std::uint64_t *valid_bytes = nullptr); // Compute the swap path for a file-backed buffer by filename. // Returns empty string if filename is empty. @@ -197,6 +203,7 @@ private: // drop them until a checkpoint re-establishes the full content. bool gap{false}; std::uint64_t gap_chkpt_request_ns{0}; + bool gap_unfixable_reported{false}; // gap on a buffer too large to checkpoint }; struct Pending { diff --git a/SyscallWrappers.cc b/SyscallWrappers.cc index 189ff07..0fac8da 100644 --- a/SyscallWrappers.cc +++ b/SyscallWrappers.cc @@ -22,10 +22,12 @@ Open(const char *path, int flags, mode_t mode) int Close(int fd) { - int ret; - do { - ret = ::close(fd); - } while (ret == -1 && errno == EINTR); + // Do not retry on EINTR: on Linux (and most systems) the descriptor is + // released even when close() is interrupted, so a retry could close an + // fd that another thread (e.g. the swap writer) has just opened. + const int ret = ::close(fd); + if (ret == -1 && errno == EINTR) + return 0; return ret; } diff --git a/SyscallWrappers.h b/SyscallWrappers.h index 6e59b3b..44e6009 100644 --- a/SyscallWrappers.h +++ b/SyscallWrappers.h @@ -12,11 +12,10 @@ namespace syscall { // Automatically retries on EINTR. int Open(const char *path, int flags, mode_t mode = 0); -// EINTR-safe wrapper for close(2). +// Wrapper for close(2). // Returns 0 on success, -1 on failure (errno set). -// Automatically retries on EINTR. -// Note: Some systems may not restart close() on EINTR, but we retry anyway -// as recommended by POSIX.1-2008. +// EINTR is treated as success and never retried: the descriptor has already +// been released on Linux, and retrying could close an unrelated, reused fd. int Close(int fd); // EINTR-safe wrapper for fsync(2). diff --git a/tests/test_swap_recovery_prompt.cc b/tests/test_swap_recovery_prompt.cc index c0fd4f7..d47b809 100644 --- a/tests/test_swap_recovery_prompt.cc +++ b/tests/test_swap_recovery_prompt.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -332,3 +333,190 @@ TEST(SwapRecoveryPrompt_CorruptSwap_Decline_KeepsAsideAndStartsFresh) std::remove(file_path.c_str()); std::filesystem::remove_all(xdg_root); } + + +namespace { +// Journal for `file_path` holding the edits in `edit`, via a throwaway manager. +static std::string +make_journal(const std::string &file_path, void (*edit)(Buffer &), std::string &expected) +{ + Buffer b; + std::string err; + (void) b.OpenFromFile(file_path, err); + std::remove(kte::SwapManager::ComputeSwapPathForTests(b).c_str()); + kte::SwapManager sm; + sm.Attach(&b); + b.SetSwapRecorder(sm.RecorderFor(&b)); + edit(b); + sm.Flush(&b); + const std::string swap_path = kte::SwapManager::ComputeSwapPathForTests(b); + expected = buffer_bytes_via_views(b); + // Keep the journal as a crash would leave it (Detach would checkpoint). + const std::string bytes = read_file_bytes(swap_path); + b.SetSwapRecorder(nullptr); + sm.Detach(&b, true); + write_file_bytes(swap_path, bytes); + return swap_path; +} + + +struct XdgSandbox { + std::filesystem::path root; + std::unique_ptr scoped; + + + explicit XdgSandbox(const std::string &tag) + { + root = std::filesystem::temp_directory_path() / + (std::string("kte_ut_xdg_") + tag + "_" + std::to_string((int) ::getpid())); + std::filesystem::remove_all(root); + std::filesystem::create_directories(root / "work"); + scoped = std::make_unique(root.string()); + } + + + ~XdgSandbox() + { + scoped.reset(); + std::filesystem::remove_all(root); + } +}; + + +static void +answer(Editor &ed, const char *yn) +{ + ASSERT_TRUE(Execute(ed, CommandId::InsertText, yn)); + ASSERT_TRUE(Execute(ed, CommandId::Newline)); +} +} // namespace + + +// Recovery must work when another named buffer is current (the AddBuffer +// path): opening used to delete the very swap file it was about to replay. +TEST(SwapRecoveryPrompt_Recover_WithAnotherNamedBufferOpen) +{ + ktet::InstallDefaultCommandsOnce(); + XdgSandbox sb("recover_named"); + const std::string other = (sb.root / "work" / "other.txt").string(); + const std::string file = (sb.root / "work" / "r.txt").string(); + write_file_bytes(other, "other\n"); + write_file_bytes(file, "base\n"); + std::string expected; + const std::string swp = make_journal(file, [](Buffer &b) { + b.insert_text(0, 0, std::string("A")); + }, expected); + + Editor ed; + ed.SetDimensions(24, 80); + std::string err; + ASSERT_TRUE(ed.OpenFile(other, err)); + ed.RequestOpenFile(file); + (void) ed.ProcessPendingOpens(); + ASSERT_EQ(ed.PendingRecoveryPrompt(), Editor::RecoveryPromptKind::RecoverOrDiscard); + answer(ed, "y"); + ASSERT_EQ(buffer_bytes_via_views(*ed.CurrentBuffer()), expected); + ASSERT_TRUE(std::filesystem::exists(swp)); +} + + +// After recovering from a journal whose last record is torn, the torn bytes +// are dropped, so records from the recovered session stay replayable. +TEST(SwapRecoveryPrompt_Recover_TornTail_JournalStaysReplayable) +{ + ktet::InstallDefaultCommandsOnce(); + XdgSandbox sb("recover_torn"); + const std::string file = (sb.root / "work" / "t.txt").string(); + write_file_bytes(file, "base\n"); + std::string expected; + const std::string swp = make_journal(file, [](Buffer &b) { + b.insert_text(0, 0, std::string("A")); + }, expected); + write_file_bytes(swp, read_file_bytes(swp) + std::string("\x01\x20\x00\x00zz", 6)); // torn record + + Editor ed; + ed.SetDimensions(24, 80); + ed.AddBuffer(Buffer()); + ed.RequestOpenFile(file); + (void) ed.ProcessPendingOpens(); + answer(ed, "y"); + Buffer *cur = ed.CurrentBuffer(); + ASSERT_EQ(buffer_bytes_via_views(*cur), std::string("Abase\n")); + cur->insert_text(0, 1, std::string("Z")); + ed.Swap()->Flush(cur); + + Buffer check; + std::string err, rerr; + ASSERT_TRUE(check.OpenFromFile(file, err)); + ASSERT_TRUE(kte::SwapManager::ReplayFile(check, swp, rerr)); + ASSERT_EQ(buffer_bytes_via_views(check), std::string("AZbase\n")); +} + + +// Opening a file that is already open switches to its buffer instead of +// creating a second buffer sharing the same swap journal. +TEST(SwapRecoveryPrompt_OpenTwice_SwitchesToExisting) +{ + ktet::InstallDefaultCommandsOnce(); + XdgSandbox sb("open_twice"); + const std::string file = (sb.root / "work" / "d.txt").string(); + const std::string other = (sb.root / "work" / "o.txt").string(); + write_file_bytes(file, "x\n"); + write_file_bytes(other, "y\n"); + + Editor ed; + ed.SetDimensions(24, 80); + std::string err; + ASSERT_TRUE(ed.OpenFile(file, err)); + ASSERT_TRUE(ed.OpenFile(other, err)); + const std::size_t n = ed.Buffers().size(); + ASSERT_TRUE(ed.OpenFile(file, err)); + ASSERT_EQ(ed.Buffers().size(), n); + ASSERT_EQ(ed.CurrentBuffer()->Filename(), std::filesystem::canonical(file).string()); + ed.RequestOpenFile(other); + (void) ed.ProcessPendingOpens(); + ASSERT_EQ(ed.Buffers().size(), n); + ASSERT_EQ(ed.CurrentBuffer()->Filename(), std::filesystem::canonical(other).string()); +} + + +// Distinct paths must not map to the same swap file. +TEST(SwapPath_EncodingIsOneToOne) +{ + ASSERT_TRUE(kte::SwapManager::ComputeSwapPathForFilename("/x/a!b") != + kte::SwapManager::ComputeSwapPathForFilename("/x/a/b")); + ASSERT_TRUE(kte::SwapManager::ComputeSwapPathForFilename("/x/a%21b") != + kte::SwapManager::ComputeSwapPathForFilename("/x/a!b")); +} + + +// Reload restarts the journal: edits discarded by the reload must not come +// back through crash recovery. +TEST(SwapJournal_Reload_ResetsJournal) +{ + ktet::InstallDefaultCommandsOnce(); + XdgSandbox sb("reload"); + const std::string file = (sb.root / "work" / "rl.txt").string(); + write_file_bytes(file, "hello\nworld\n"); + + Editor ed; + ed.SetDimensions(24, 80); + std::string err; + ASSERT_TRUE(ed.OpenFile(file, err)); + Buffer *cur = ed.CurrentBuffer(); + cur->SetCursor(0, 0); + ASSERT_TRUE(Execute(ed, CommandId::InsertText, "XXXX")); + ASSERT_TRUE(Execute(ed, CommandId::ReloadBuffer)); + cur = ed.CurrentBuffer(); + cur->SetCursor(0, 1); + ASSERT_TRUE(Execute(ed, CommandId::InsertText, "YY")); + ed.Swap()->Flush(cur); + const std::string live = buffer_bytes_via_views(*cur); + ASSERT_EQ(live, std::string("hello\nYYworld\n")); + + Buffer check; + std::string rerr; + ASSERT_TRUE(check.OpenFromFile(file, err)); + ASSERT_TRUE(kte::SwapManager::ReplayFile(check, kte::SwapManager::ComputeSwapPathForTests(*cur), rerr)); + ASSERT_EQ(buffer_bytes_via_views(check), live); +}