Save metadata and journal staleness: second-pass fixes

- Ownership is applied before permissions: chown clears setuid/setgid,
  so saving a 06755 file left it 0755.
- The umask is read once during static initialisation. Reading it at
  save time needed umask(0)/umask(old), briefly changing it for the swap
  writer thread creating files.
- Before rewriting a hard-linked file in place, the new content is
  written and synced to a copy beside it; if the in-place write fails
  (ENOSPC, EIO) the copy is kept and named in the error.
- Journal headers also record a CRC-32 of the base file (flags bit 1,
  bytes 40..43). A changed mtime with the same size and content
  (touch, checkout back and forth) no longer marks a journal stale.

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

Buffer.cc

@@ -136,6 +136,16 @@ best_effort_fsync_dir(const std::string &path)
 }
 
 
+// The process umask, read once during static initialisation (before any
+// thread exists): reading it later needs umask(0) + umask(old), which briefly
+// changes it for the swap writer thread creating files concurrently.
+static const mode_t g_process_umask = [] {
+	const mode_t m = ::umask(0);
+	(void) ::umask(m);
+	return m;
+}();
+
+
 // Rewrite `path` in place (truncate, write, fsync). Used for files with
 // several hard links, which a temp-file-and-rename save would split.
 static bool
@@ -160,6 +170,31 @@ write_in_place(const std::string &path, const char *data, std::size_t len, std::
 }
 
 
+// Create `path` (which must not exist) with the content, fsynced.
+static bool
+write_new_file(const std::string &path, const char *data, std::size_t len, std::string &err)
+{
+	int flags = O_WRONLY | O_CREAT | O_EXCL;
+#ifdef O_CLOEXEC
+	flags |= O_CLOEXEC;
+#endif
+	const int fd = kte::syscall::Open(path.c_str(), flags, 0600);
+	if (fd < 0) {
+		err = "Failed to create " + path + ": " + std::strerror(errno);
+		return false;
+	}
+	bool ok = write_all_fd(fd, data, len, err);
+	if (ok && kte::syscall::Fsync(fd) != 0) {
+		err = std::string("fsync failed: ") + std::strerror(errno);
+		ok  = false;
+	}
+	(void) kte::syscall::Close(fd);
+	if (!ok)
+		(void) ::unlink(path.c_str());
+	return ok;
+}
+
+
 static bool
 atomic_write_file(const std::string &path_in, const char *data, std::size_t len, std::string &err)
 {
@@ -183,8 +218,20 @@ atomic_write_file(const std::string &path_in, const char *data, std::size_t len,
 
 	// A file with other hard links must be rewritten in place, or those
 	// names would keep the old content.
-	if (dst_exists && S_ISREG(dst_st.st_mode) && dst_st.st_nlink > 1)
+	if (dst_exists && S_ISREG(dst_st.st_mode) && dst_st.st_nlink > 1) {
-		return write_in_place(path, data, len, err);
+		// Truncating in place risks the only copy if the write then fails
+		// (ENOSPC, EIO, crash), so first write and sync a copy beside it.
+		const std::string copy = path + ".kte-save";
+		if (!write_new_file(copy, data, len, err))
+			return false;
+		if (!write_in_place(path, data, len, err)) {
+			err += " (the new content is in " + copy + ")";
+			return false;
+		}
+		(void) ::unlink(copy.c_str());
+		best_effort_fsync_dir(path);
+		return true;
+	}
 
 	// Create a temp file in the same directory so rename() is atomic.
 	std::filesystem::path p(path);
@@ -216,17 +263,16 @@ atomic_write_file(const std::string &path_in, const char *data, std::size_t len,
 	std::string tmp_path(buf.data());
 
 	if (dst_exists) {
-		// Carry over permissions and (best effort; needs privilege to give a
+		// Carry over ownership (best effort; needs privilege to give a file
-		// file away) ownership of the file being replaced.
+		// away) and then permissions: chown clears setuid/setgid, so the
-		(void) kte::syscall::Fchmod(fd, dst_st.st_mode & 07777);
+		// mode has to be applied after it.
 		if (::fchown(fd, dst_st.st_uid, dst_st.st_gid) != 0) {
 			// Expected without privilege when the owner differs; keep ours.
 		}
+		(void) kte::syscall::Fchmod(fd, dst_st.st_mode & 07777);
 	} else {
 		// mkstemp creates 0600; a new file gets the usual 0666 & ~umask.
-		const mode_t mask = ::umask(0);
+		(void) kte::syscall::Fchmod(fd, 0666 & ~g_process_umask);
-		(void) ::umask(mask);
-		(void) kte::syscall::Fchmod(fd, 0666 & ~mask);
 	}
 
 	bool ok = write_all_fd(fd, data, len, err);

Swap.cc

@@ -354,7 +354,7 @@ SwapManager::Attach(Buffer *buf)
 	if (ctx.path.empty())
 		ctx.path = ComputeSidecarPath(*buf);
 	if (fresh)
-		ctx.has_base = stat_base(buf->Filename(), ctx.base_size, ctx.base_mtime_ns);
+		capture_base(buf->Filename(), ctx);
 	// Ensure a recorder exists as well.
 	if (recorders_.find(buf) == recorders_.end()) {
 		recorders_[buf] = std::make_unique<BufferRecorder>(*this, *buf);
@@ -433,7 +433,7 @@ SwapManager::ResetJournal(Buffer &buf)
 		ctx.gap_chkpt_request_ns   = 0;
 		ctx.gap_unfixable_reported = false;
 		ctx.locked_out             = false;
-		ctx.has_base               = stat_base(buf.Filename(), ctx.base_size, ctx.base_mtime_ns);
+		capture_base(buf.Filename(), ctx);
 		ctx.last_flush_ns          = 0;
 		ctx.last_fsync_ns          = 0;
 		ctx.last_chkpt_ns          = 0;
@@ -566,7 +566,7 @@ SwapManager::NotifyFilenameChanged(Buffer &buf)
 	ctx.gap_chkpt_request_ns    = 0;
 	ctx.gap_unfixable_reported  = false;
 	ctx.locked_out              = false;
-	ctx.has_base                = stat_base(buf.Filename(), ctx.base_size, ctx.base_mtime_ns);
+	capture_base(buf.Filename(), ctx);
 	ctx.path                    = new_path;
 	ctx.suspended              = false;
 	ctx.header_ok              = false;
@@ -693,10 +693,18 @@ SwapManager::write_header(int fd, const JournalCtx &ctx)
 	hdr[10] = static_cast<std::uint8_t>((VERSION >> 16) & 0xFFu);
 	hdr[11] = static_cast<std::uint8_t>((VERSION >> 24) & 0xFFu);
 	// flags: bit 0 = base file identity present
+	// flags: bit 1 = base content CRC-32 present (bytes 40..43)
 	if (ctx.has_base) {
 		hdr[12] = 1;
 		put_le64(hdr + 24, ctx.base_size);
 		put_le64(hdr + 32, static_cast<std::uint64_t>(ctx.base_mtime_ns));
+		if (ctx.has_base_crc) {
+			hdr[12] |= 2;
+			hdr[40] = static_cast<std::uint8_t>(ctx.base_crc & 0xFFu);
+			hdr[41] = static_cast<std::uint8_t>((ctx.base_crc >> 8) & 0xFFu);
+			hdr[42] = static_cast<std::uint8_t>((ctx.base_crc >> 16) & 0xFFu);
+			hdr[43] = static_cast<std::uint8_t>((ctx.base_crc >> 24) & 0xFFu);
+		}
 	}
 	// created_time (unix seconds; little-endian)
 	std::uint64_t ts = static_cast<std::uint64_t>(std::time(nullptr));
@@ -1515,7 +1523,48 @@ SwapManager::JournalMatchesFile(const std::string &swap_path, const std::string
 	std::int64_t mtime = 0;
 	if (!stat_base(file_path, size, mtime))
 		return false; // the file the journal was based on is gone
-	return size == want_size && static_cast<std::uint64_t>(mtime) == want_mtime;
+	if (size != want_size)
+		return false;
+	if (static_cast<std::uint64_t>(mtime) == want_mtime)
+		return true;
+	// Timestamp changed (touch, checkout back and forth, sync tools): the
+	// content may still be the same, which is what matters.
+	if ((hdr[12] & 2u) == 0)
+		return false;
+	const std::uint32_t want_crc = static_cast<std::uint32_t>(hdr[40]) | (static_cast<std::uint32_t>(hdr[41]) << 8) |
+	                               (static_cast<std::uint32_t>(hdr[42]) << 16) |
+	                               (static_cast<std::uint32_t>(hdr[43]) << 24);
+	std::uint32_t crc = 0;
+	return file_crc32(file_path, crc) && crc == want_crc;
+}
+
+
+bool
+SwapManager::file_crc32(const std::string &path, std::uint32_t &out)
+{
+	std::ifstream in(path, std::ios::binary);
+	if (!in)
+		return false;
+	std::uint32_t c = 0;
+	std::uint8_t chunk[1 << 16];
+	while (in) {
+		in.read(reinterpret_cast<char *>(chunk), sizeof(chunk));
+		const auto n = in.gcount();
+		if (n > 0)
+			c = crc32(chunk, static_cast<std::size_t>(n), c);
+	}
+	if (in.bad())
+		return false;
+	out = c;
+	return true;
+}
+
+
+void
+SwapManager::capture_base(const std::string &file, JournalCtx &ctx)
+{
+	ctx.has_base     = stat_base(file, ctx.base_size, ctx.base_mtime_ns);
+	ctx.has_base_crc = ctx.has_base && file_crc32(file, ctx.base_crc);
 }
 
 

Swap.h

@@ -222,6 +222,8 @@ private:
 		bool has_base{false};
 		std::uint64_t base_size{0};
 		std::int64_t base_mtime_ns{0};
+		bool has_base_crc{false}; // CRC-32 of the base file's content
+		std::uint32_t base_crc{0};
 	};
 
 	struct Pending {
@@ -254,6 +256,13 @@ private:
 
 	static std::uint32_t crc32(const std::uint8_t *data, std::size_t len, std::uint32_t seed = 0);
 
+	// CRC-32 of a file's content; false if it cannot be read.
+	static bool file_crc32(const std::string &path, std::uint32_t &out);
+
+	// Record the identity (size, mtime, content CRC) of the file a journal's
+	// records will apply to.
+	static void capture_base(const std::string &file, JournalCtx &ctx);
+
 	static void put_le32(std::vector<std::uint8_t> &out, std::uint32_t v);
 
 	static void put_le64(std::uint8_t dst[8], std::uint64_t v);

tests/test_audit_regressions.cc

@@ -974,3 +974,21 @@ TEST(Audit_SaveAs_FileOpenInOtherBuffer_Refused)
 	ASSERT_EQ(ed.BufferCount(), n);
 	ASSERT_EQ(ed.FindOpenBuffer((d.path / "y.txt").string()) != ed.CurrentBufferIndex(), true);
 }
+
+
+// Saving keeps setuid/setgid bits (chown after chmod used to clear them).
+TEST(Audit_Save_KeepsSpecialModeBits)
+{
+	TempDir d("save_setuid");
+	const auto f = d.path / "s.sh";
+	std::ofstream(f) << "echo\n";
+	ASSERT_EQ(::chmod(f.c_str(), 06755), 0);
+	Buffer b;
+	std::string err;
+	ASSERT_TRUE(b.OpenFromFile(f.string(), err));
+	b.insert_text(0, 0, "#");
+	ASSERT_TRUE(b.Save(err));
+	struct stat st{};
+	ASSERT_EQ(::stat(f.c_str(), &st), 0);
+	ASSERT_EQ(st.st_mode & 07777, (mode_t) 06755);
+}

tests/test_swap_recovery_prompt.cc

@@ -7,6 +7,7 @@
 
 #include "tests/TestHarness.h" // for ktet::InstallDefaultCommandsOnce
 
+#include <chrono>
 #include <cstdio>
 #include <cstdlib>
 #include <filesystem>
@@ -653,3 +654,28 @@ TEST(SwapRecoveryPrompt_EmptyAnswer_KeepsSwap)
 	ASSERT_TRUE(!ed.PromptActive());
 	ASSERT_TRUE(std::filesystem::exists(swp));
 }
+
+
+// A timestamp-only change (touch, git checkout back and forth) does not make
+// a journal stale: its base content is unchanged.
+TEST(SwapRecoveryPrompt_TouchedFile_StillRecoverable)
+{
+	ktet::InstallDefaultCommandsOnce();
+	XdgSandbox sb("touched");
+	const std::string file = (sb.root / "work" / "t2.txt").string();
+	write_file_bytes(file, "base\n");
+	std::string expected;
+	(void) make_journal(file, [](Buffer &b) {
+		b.insert_text(0, 0, std::string("A"));
+	}, expected);
+	std::filesystem::last_write_time(file, std::filesystem::last_write_time(file) + std::chrono::hours(1));
+
+	Editor ed;
+	ed.SetDimensions(24, 80);
+	ed.AddBuffer(Buffer());
+	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);
+}

tests/test_swap_writer.cc

@@ -125,8 +125,8 @@ TEST(SwapWriter_Header_Records_And_CRC)
 		ASSERT_EQ(bytes[(std::size_t) i], magic[i]);
 	ASSERT_EQ(read_le32(bytes.data() + 8), (std::uint32_t) 1);
 	// flags bit 0: the base file's identity (size, mtime) is recorded, since
-	// the buffer is file-backed.
+	// the buffer is file-backed; bit 1: its content CRC-32 too.
-	ASSERT_EQ(read_le32(bytes.data() + 12), (std::uint32_t) 1);
+	ASSERT_EQ(read_le32(bytes.data() + 12), (std::uint32_t) 3);
 	ASSERT_EQ(read_le64(bytes.data() + 24), (std::uint64_t) std::filesystem::file_size(b.Filename()));
 	ASSERT_TRUE(read_le64(bytes.data() + 16) != 0);