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); +}