Swap ownership, fsync race, RowsView memory, renderer regex stack

- A session never unlinks a journal it does not own: Detach, ResetJournal
  and NotifyFilenameChanged skip removal when this session was locked
  out or another session holds the lock, and PruneSwapDir skips held
  journals. A second session used to delete the first session's live
  journal on save or close.
- The writer thread's periodic fsync now runs under mtx_. It fsynced fds
  that the main thread could close concurrently (or that had been
  reused); an in-flight fsync also kept a just-closed journal's flock
  alive, so the owner's own cleanup saw its journal as held.
- Compaction locks the compacted file before renaming it into place and
  keeps using it, so the journal is never unlocked in between; a failed
  lock on the truncating reopen marks the session locked out.
- Unnamed-buffer journals include the pid, so sessions no longer share
  (and delete) each other's unnamed-N.swp.
- RowsView keeps only the previous generation of cached rows: keeping
  all of them made C-u 2000 M-d on a 1 MiB line use ~2 GB.
- Renderers run regex match highlighting through ForEachRegexMatch,
  which uses the large-stack worker for lines over 512 bytes; some
  patterns overflowed the main stack well under the previous 10,000
  byte limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB

Command.cc

@@ -457,9 +457,11 @@ compute_mark_region(Buffer &buf, std::size_t &sx, std::size_t &sy, std::size_t &
 // Row access for commands that touch a few rows. Buffer::Rows() rebuilds a
 // string for every line of the file on the first call after any edit, which
 // made every motion and delete O(file size). RowsView fetches rows on demand
-// and caches them for the current buffer version. Strings fetched for an
-// older version are retired, not freed, so a reference obtained before an
-// edit stays valid (if stale) for the lifetime of the view, as with Rows().
+// and caches them for the current buffer version. When the version changes
+// the current cache becomes the previous one (the one before that is freed),
+// so a reference obtained just before an edit stays valid (if stale) across
+// that edit. Keeping every generation grew without bound in loops that edit
+// on each pass (C-u 2000 M-d on a long line used ~2 GB).
 class RowsView {
 public:
 	explicit RowsView(const Buffer &buf) : buf_(&buf) {}
@@ -480,8 +482,7 @@ public:
 	const std::string &operator[](std::size_t row) const
 	{
 		if (buf_->Version() != version_) {
-			if (!cache_.empty())
-				retired_.push_back(std::move(cache_));
+			previous_ = std::move(cache_);
 			cache_.clear();
 			version_ = buf_->Version();
 		}
@@ -495,7 +496,7 @@ private:
 	const Buffer *buf_;
 	mutable std::uint64_t version_ = ~std::uint64_t{0};
 	mutable std::map<std::size_t, std::string> cache_;
-	mutable std::vector<std::map<std::size_t, std::string> > retired_;
+	mutable std::map<std::size_t, std::string> previous_;
 };
 
 

ImGuiRenderer.cc

@@ -360,17 +360,13 @@ ImGuiRenderer::Draw(Editor &ed)
 			if (search_mode) {
 				// In regex mode, reuse the compiled regex hoisted above the loop.
 				if (regex_mode) {
-					// Long lines are not highlighted: std::regex recursion
-					// could overflow the stack (RegexGuard.h).
+					// std::regex recursion is kept off the main stack for long
+					// lines (RegexGuard.h); very long lines are not highlighted.
 					if (search_rx_valid && line.size() <= kte::kRegexRenderLineLimit) {
 						try {
-							for (auto it = std::sregex_iterator(line.begin(), line.end(), search_rx);
-							     it != std::sregex_iterator(); ++it) {
-								const auto &m  = *it;
-								std::size_t sx = static_cast<std::size_t>(m.position());
-								std::size_t ex = sx + static_cast<std::size_t>(m.length());
-								hl_src_ranges.emplace_back(sx, ex);
-							}
+							kte::ForEachRegexMatch(line, search_rx, [&](std::size_t pos, std::size_t len) {
+								hl_src_ranges.emplace_back(pos, pos + len);
+							});
 						} catch (const std::regex_error &) {
 							// ignore invalid patterns here; status line already shows the error
 						}

QtFrontend.cc

@@ -166,21 +166,13 @@ protected:
 						    (ed_->CurrentPromptKind() == Editor::PromptKind::RegexSearch ||
 						     ed_->CurrentPromptKind() ==
 						     Editor::PromptKind::RegexReplaceFind)) {
-							// Long lines are not highlighted: std::regex recursion
-							// could overflow the stack (RegexGuard.h).
+							// std::regex recursion is kept off the main stack for long
+							// lines (RegexGuard.h); very long lines are not highlighted.
 							if (line.size() <= kte::kRegexRenderLineLimit) try {
 								std::regex rx(ed_->SearchQuery());
-								for (auto it = std::sregex_iterator(
-									     line.begin(), line.end(), rx);
-								     it != std::sregex_iterator(); ++it) {
-									const auto &m  = *it;
-									std::size_t sx = static_cast<std::size_t>(m.
-										position());
-									std::size_t ex =
-										sx + static_cast<std::size_t>(m.
-											length());
-									hl_src_ranges.emplace_back(sx, ex);
-								}
+								kte::ForEachRegexMatch(line, rx, [&](std::size_t pos, std::size_t len) {
+									hl_src_ranges.emplace_back(pos, pos + len);
+								});
 							} catch (const std::regex_error &) {
 								// Invalid regex: ignore, status line already shows errors
 							}

RegexGuard.cc

@@ -48,4 +48,22 @@ RunWithLargeStack(const std::function<void()> &fn)
 	if (job.error)
 		std::rethrow_exception(job.error);
 }
+
+
+void
+ForEachRegexMatch(const std::string &line, const std::regex &rx,
+                  const std::function<void(std::size_t, std::size_t)> &on_match)
+{
+	auto scan = [&] {
+		for (auto it = std::sregex_iterator(line.begin(), line.end(), rx); it != std::sregex_iterator(); ++it)
+			on_match(static_cast<std::size_t>(it->position()), static_cast<std::size_t>(it->length()));
+	};
+	// Stack use grows with match length; a few hundred bytes stays far below
+	// any thread's stack for every pattern we measured.
+	constexpr std::size_t kInlineLimit = 512;
+	if (line.size() <= kInlineLimit)
+		scan();
+	else
+		RunWithLargeStack(scan);
+}
 } // namespace kte

RegexGuard.h

@@ -8,15 +8,24 @@
 
 #include <cstddef>
 #include <functional>
+#include <regex>
+#include <string>
 
 namespace kte {
-// Renderers highlight regex matches only on lines up to this many bytes;
-// longer lines are drawn without match highlighting.
-constexpr std::size_t kRegexRenderLineLimit = 10000;
+// Renderers highlight regex matches only on lines up to this many bytes (to
+// bound per-frame work); longer lines are drawn without match highlighting.
+constexpr std::size_t kRegexRenderLineLimit = 1000000;
 
 // Run fn to completion on a thread with a large stack (address space is
 // reserved, pages are committed only as used), so regex work over long lines
 // cannot overflow. If such a thread cannot be created, fn runs on the calling
 // thread. Exceptions thrown by fn are rethrown to the caller.
 void RunWithLargeStack(const std::function<void()> &fn);
+
+// Call on_match(position, length) for each match of rx in line. Lines short
+// enough to be safe on any stack are matched inline; longer ones on a large
+// stack (a 10,000-byte line already overflowed 8 MiB for some patterns).
+// Throws what std::regex throws.
+void ForEachRegexMatch(const std::string &line, const std::regex &rx,
+                       const std::function<void(std::size_t, std::size_t)> &on_match);
 } // namespace kte

Swap.cc

@@ -29,6 +29,8 @@ constexpr std::size_t kMaxRecordPayload = 0xFFFFFFu;
 constexpr std::uint64_t kGapCheckpointIntervalNs = 1000000000ULL;
 } // namespace
 
+static void remove_journal_unless_held(const std::string &path, bool locked_out);
+
 namespace {
 constexpr std::uint8_t MAGIC[8] = {'K', 'T', 'E', '_', 'S', 'W', 'P', '\0'};
 constexpr std::uint32_t VERSION = 1;
@@ -381,26 +383,28 @@ SwapManager::Detach(Buffer *buf, const bool remove_file)
 
 	Flush(buf);
 	std::string path;
+	bool locked_out = false;
 	{
 		std::lock_guard<std::mutex> lg(mtx_);
 		auto it = journals_.find(buf);
 		if (it != journals_.end()) {
 			path       = it->second.path;
+			locked_out = it->second.locked_out;
 			close_ctx(it->second);
 			journals_.erase(it);
 		}
 		recorders_.erase(buf);
 	}
 
-	if (remove_file && !path.empty()) {
-		(void) std::remove(path.c_str());
-	}
+	if (remove_file)
+		remove_journal_unless_held(path, locked_out);
 }
 
 
 void
 SwapManager::ResetJournal(Buffer &buf)
 {
+	bool was_locked_out = false;
 	std::string path;
 	{
 		std::lock_guard<std::mutex> lg(mtx_);
@@ -423,6 +427,7 @@ SwapManager::ResetJournal(Buffer &buf)
 			return;
 		JournalCtx &ctx = it->second;
 		close_ctx(ctx);
+		was_locked_out             = ctx.locked_out;
 		ctx.header_ok              = false;
 		ctx.gap                    = false; // the journal restarts from the saved file
 		ctx.gap_chkpt_request_ns   = 0;
@@ -437,9 +442,7 @@ SwapManager::ResetJournal(Buffer &buf)
 		ctx.suspended              = false;
 	}
 
-	if (!path.empty()) {
-		(void) std::remove(path.c_str());
-	}
+	remove_journal_unless_held(path, was_locked_out);
 }
 
 
@@ -520,7 +523,8 @@ SwapManager::PruneSwapDir()
 				too_old = true;
 		}
 		bool over_limit = (cfg.prune_max_files > 0) && (kept >= cfg.prune_max_files);
-		if (too_old || over_limit) {
+		// Never prune a journal another running session holds.
+		if ((too_old || over_limit) && !JournalInUse(e.path.string())) {
 			std::error_code ec3;
 			fs::remove(e.path, ec3);
 		} else {
@@ -557,7 +561,7 @@ SwapManager::NotifyFilenameChanged(Buffer &buf)
 	// 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());
+		remove_journal_unless_held(old_path, ctx.locked_out);
 	ctx.gap                     = false;
 	ctx.gap_chkpt_request_ns    = 0;
 	ctx.gap_unfixable_reported  = false;
@@ -621,10 +625,13 @@ SwapManager::ComputeSidecarPath(const Buffer &buf)
 		return compute_swap_path_for_filename(buf.Filename());
 	}
 
-	// Unnamed buffers: unique within the process.
+	// Unnamed buffers: unique across processes (pid) and within one (counter).
+	// A per-process counter alone let a new session's scratch buffer claim,
+	// and later delete, another running session's unnamed journal.
 	static std::atomic<std::uint64_t> ctr{0};
 	const std::uint64_t n  = ++ctr;
-	const std::string name = "unnamed-" + std::to_string(n) + ".swp";
+	const std::string name = "unnamed-" + std::to_string(static_cast<long>(::getpid())) + "-" +
+	                         std::to_string(n) + ".swp";
 	return (root / name).string();
 }
 
@@ -769,7 +776,12 @@ SwapManager::open_ctx(JournalCtx &ctx, const std::string &path, std::string &err
 			}
 			return false;
 		}
-		(void) ::flock(fd, LOCK_EX | LOCK_NB);
+		if (::flock(fd, LOCK_EX | LOCK_NB) != 0 && errno == EWOULDBLOCK) {
+			kte::syscall::Close(fd);
+			ctx.locked_out = true;
+			err            = "Swap file is in use by another kte process: " + path;
+			return false;
+		}
 		(void) kte::syscall::Fchmod(fd, 0600);
 		st.st_size = 0;
 	}
@@ -819,14 +831,10 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector<std::uint8
 		return false;
 	}
 
-	// Close existing file before rename.
-	if (ctx.fd >= 0) {
-		(void) kte::syscall::Fsync(ctx.fd);
-		kte::syscall::Close(ctx.fd);
-		ctx.fd = -1;
-	}
-	ctx.header_ok = false;
-
+	// Keep the current (locked) journal fd open until the compacted file has
+	// replaced it, so the journal is never unlocked in between: another
+	// session would otherwise see it as abandoned and offer to recover or
+	// discard it.
 	const std::string tmp_path = ctx.path + ".tmp";
 	// Create the compacted file: header + checkpoint record.
 	if (!ensure_parent_dir(tmp_path)) {
@@ -864,8 +872,10 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector<std::uint8
 			ok = false;
 		}
 	}
-	kte::syscall::Close(tfd);
+	if (ok)
+		(void) ::flock(tfd, LOCK_EX | LOCK_NB); // new file: nobody else has it
 	if (!ok) {
+		kte::syscall::Close(tfd);
 		if (err.empty()) {
 			err = "Failed to write temp swap file: " + tmp_path;
 		}
@@ -876,11 +886,20 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector<std::uint8
 	// Atomic replace.
 	if (::rename(tmp_path.c_str(), ctx.path.c_str()) != 0) {
 		int saved_errno = errno;
+		kte::syscall::Close(tfd);
 		err = "Failed to rename temp swap file '" + tmp_path + "' to '" + ctx.path + "': " + std::strerror(
 			      saved_errno);
 		std::remove(tmp_path.c_str());
 		return false;
 	}
+	// The compacted file is the journal now; retire the old descriptor.
+	if (ctx.fd >= 0)
+		kte::syscall::Close(ctx.fd);
+	const int fl = ::fcntl(tfd, F_GETFL);
+	if (fl >= 0)
+		(void) ::fcntl(tfd, F_SETFL, fl | O_APPEND);
+	ctx.fd        = tfd;
+	ctx.header_ok = true;
 
 	// Best-effort: fsync parent dir to persist the rename.
 	try {
@@ -902,10 +921,6 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const std::vector<std::uint8
 	}
 
 	// Re-open for further appends.
-	if (!open_ctx(ctx, ctx.path, err)) {
-		// err already set by open_ctx
-		return false;
-	}
 	ctx.approx_size_bytes = 64 + static_cast<std::uint64_t>(chkpt_record.size());
 	return true;
 }
@@ -1213,9 +1228,12 @@ SwapManager::writer_loop()
 
 		// Throttled fsync: best-effort (grouped)
 		try {
-			std::vector<int> to_sync;
 			std::uint64_t now = now_ns();
-			{
+			// fsync under mtx_: the main thread closes journal fds under the
+			// same lock. Syncing outside it raced with those closes: the fd
+			// could be closed (or reused) mid-fsync, and the in-flight fsync
+			// kept the closed journal's flock alive, so the owner's own
+			// cleanup saw its journal as held by someone else.
 			std::lock_guard<std::mutex> lg(mtx_);
 			for (auto &kv: journals_) {
 				JournalCtx &ctx = kv.second;
@@ -1223,14 +1241,10 @@ SwapManager::writer_loop()
 					if (ctx.last_fsync_ns == 0 || (now - ctx.last_fsync_ns) / 1000000ULL >=
 					    cfg_.fsync_interval_ms) {
 						ctx.last_fsync_ns = now;
-							to_sync.push_back(ctx.fd);
+						(void) kte::syscall::Fsync(ctx.fd);
 					}
 				}
 			}
-			}
-			for (int fd: to_sync) {
-				(void) kte::syscall::Fsync(fd);
-			}
 		} catch (const std::exception &e) {
 			report_error(std::string("Exception in fsync operations: ") + e.what());
 		} catch (...) {
@@ -1455,6 +1469,18 @@ parse_u32_le(const std::vector<std::uint8_t> &p, std::size_t &off, std::uint32_t
 }
 
 
+// Remove a journal this session is done with, unless another session holds
+// it: a session that was locked out of the journal (or finds it locked) must
+// not unlink the live journal of the session that owns it.
+static void
+remove_journal_unless_held(const std::string &path, bool locked_out)
+{
+	if (path.empty() || locked_out || SwapManager::JournalInUse(path))
+		return;
+	(void) std::remove(path.c_str());
+}
+
+
 bool
 SwapManager::JournalInUse(const std::string &swap_path)
 {

TerminalRenderer.cc

@@ -76,13 +76,9 @@ TerminalRenderer::Draw(Editor &ed)
 					// could overflow the stack (RegexGuard.h).
 					if (sline.size() <= kte::kRegexRenderLineLimit) try {
 						std::regex rx(ed.SearchQuery());
-						for (auto it = std::sregex_iterator(sline.begin(), sline.end(), rx);
-						     it != std::sregex_iterator(); ++it) {
-							const auto &m  = *it;
-							std::size_t sx = static_cast<std::size_t>(m.position());
-							std::size_t ex = sx + static_cast<std::size_t>(m.length());
-							ranges.emplace_back(sx, ex);
-						}
+						kte::ForEachRegexMatch(sline, rx, [&](std::size_t pos, std::size_t len) {
+							ranges.emplace_back(pos, pos + len);
+						});
 					} catch (const std::regex_error &) {
 						// ignore invalid patterns here; status shows error
 					}

tests/test_audit_regressions.cc

@@ -8,6 +8,7 @@
 
 #include "ErrorRecovery.h"
 #include "PieceTable.h"
+#include "RegexGuard.h"
 #include "UndoTree.h"
 #include "syntax/HighlighterEngine.h"
 #include "syntax/HighlighterRegistry.h"
@@ -880,3 +881,20 @@ TEST(Audit_Commands_FilesystemErrorsDoNotThrow)
 	(void) ed.ProcessPendingOpens();
 	ASSERT_EQ(h.Text(), std::string("precious\n"));
 }
+
+
+// Renderer regex highlighting on a long line with a deeply recursive pattern
+// (overflowed an 8 MiB stack at under 10,000 bytes) runs off the main stack.
+TEST(Audit_ForEachRegexMatch_LongLine)
+{
+	std::string line;
+	while (line.size() < 200000)
+		line += "word, more words; and so on. ";
+	std::size_t n = 0, total = 0;
+	kte::ForEachRegexMatch(line, std::regex("(\\w|\\s|[.,;])+"), [&](std::size_t, std::size_t len) {
+		++n;
+		total += len;
+	});
+	ASSERT_EQ(n, (std::size_t) 1);
+	ASSERT_EQ(total, line.size());
+}

tests/test_swap_recovery_prompt.cc

@@ -583,3 +583,42 @@ TEST(SwapRecoveryPrompt_LiveJournalOfOtherSession_LeftAlone)
 	other.SetSwapRecorder(nullptr);
 	other_sm.Detach(&other, true);
 }
+
+
+// A session locked out of a journal (another session owns it) must not
+// delete it when it closes or saves the buffer.
+TEST(SwapJournal_SecondSession_DoesNotDeleteOwnersJournal)
+{
+	ktet::InstallDefaultCommandsOnce();
+	XdgSandbox sb("owner");
+	const std::string file = (sb.root / "work" / "g.txt").string();
+	write_file_bytes(file, "base\n");
+
+	Editor a;
+	a.SetDimensions(24, 80);
+	std::string err;
+	ASSERT_TRUE(a.OpenFile(file, err));
+	a.CurrentBuffer()->insert_text(0, 0, std::string("A"));
+	a.Swap()->Flush(a.CurrentBuffer());
+	const std::string swp = kte::SwapManager::ComputeSwapPathForTests(*a.CurrentBuffer());
+	ASSERT_TRUE(std::filesystem::exists(swp));
+
+	{
+		Editor b;
+		b.SetDimensions(24, 80);
+		b.AddBuffer(Buffer());
+		b.RequestOpenFile(file);
+		(void) b.ProcessPendingOpens();
+		b.CurrentBuffer()->insert_text(0, 0, std::string("B"));
+		b.Swap()->Flush(b.CurrentBuffer());
+		ASSERT_TRUE(b.CloseBuffer(b.CurrentBufferIndex()));
+	}
+	ASSERT_TRUE(std::filesystem::exists(swp));
+
+	// A's journal still replays to A's content.
+	Buffer check;
+	std::string rerr;
+	ASSERT_TRUE(check.OpenFromFile(file, err));
+	ASSERT_TRUE(kte::SwapManager::ReplayFile(check, swp, rerr));
+	ASSERT_EQ(buffer_bytes_via_views(check), std::string("Abase\n"));
+}