Swap: tell the user when a journal is locked out or failing; long names
- SwapManager queues user notices when a journal is held by another kte (this session's edits are then not journaled) and, once per gap, when journal writes fail. Editor shows them in the status line from its per-frame hook. Both went only to the error log, so crash recovery was silently off or incomplete. - The fallback swap name '<basename>.<hash>.swp' exceeded NAME_MAX for basenames over ~233 bytes, so such files were never journaled; the basename is shortened (on a UTF-8 boundary) only in that case. - Audit doc: sixth-round findings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB
This commit is contained in:
@@ -495,8 +495,11 @@ Editor::ProcessPendingOpens()
|
||||
try {
|
||||
// Also the once-per-frame hook for the journal: retry the
|
||||
// checkpoint of a journal that lost a record.
|
||||
if (kte::SwapManager *sm = Swap())
|
||||
if (kte::SwapManager *sm = Swap()) {
|
||||
sm->RetryGapCheckpoints();
|
||||
if (std::string notice = sm->TakeUserNotice(); !notice.empty())
|
||||
SetStatus(notice);
|
||||
}
|
||||
return process_pending_opens_();
|
||||
} catch (const std::exception &e) {
|
||||
kte::ErrorHandler::Instance().Error("Editor", std::string("open failed: ") + e.what(), "");
|
||||
|
||||
@@ -202,8 +202,18 @@ compute_swap_path_for_filename(const std::string &filename)
|
||||
}
|
||||
|
||||
// Fallback: stable, shorter name based on basename + hash.
|
||||
std::string base = p.filename().string();
|
||||
const std::string name = base + "." + hex_u64(fnv1a64(key)) + ".swp";
|
||||
std::string base = p.filename().string();
|
||||
const std::string suffix = "." + hex_u64(fnv1a64(key)) + ".swp";
|
||||
// A long basename made even this name exceed NAME_MAX (255), so the
|
||||
// journal could never be created. Shorten only such names (the hash
|
||||
// keeps them distinct), on a UTF-8 boundary.
|
||||
if (base.size() + suffix.size() > 255) {
|
||||
std::size_t keep = 255 - suffix.size();
|
||||
while (keep > 0 && (static_cast<unsigned char>(base[keep]) & 0xC0) == 0x80)
|
||||
--keep;
|
||||
base.resize(keep);
|
||||
}
|
||||
const std::string name = base + suffix;
|
||||
return (root / name).string();
|
||||
}
|
||||
}
|
||||
@@ -439,6 +449,7 @@ SwapManager::ResetJournal(Buffer &buf)
|
||||
was_locked_out = ctx.locked_out;
|
||||
ctx.header_ok = false;
|
||||
ctx.gap = false; // the journal restarts from the saved file
|
||||
ctx.gap_notified = false;
|
||||
ctx.gap_chkpt_request_ns = 0;
|
||||
ctx.gap_unfixable_reported = false;
|
||||
ctx.locked_out = false;
|
||||
@@ -587,6 +598,7 @@ SwapManager::NotifyFilenameChanged(Buffer &buf)
|
||||
ctx.gap = false;
|
||||
ctx.gap_chkpt_request_ns = 0;
|
||||
ctx.gap_unfixable_reported = false;
|
||||
ctx.gap_notified = false;
|
||||
ctx.locked_out = false;
|
||||
apply_base(ctx, base);
|
||||
ctx.path = new_path;
|
||||
@@ -1193,6 +1205,26 @@ SwapManager::RetryGapCheckpoints()
|
||||
}
|
||||
|
||||
|
||||
std::string
|
||||
SwapManager::TakeUserNotice()
|
||||
{
|
||||
std::lock_guard<std::mutex> lg(mtx_);
|
||||
if (user_notices_.empty())
|
||||
return {};
|
||||
std::string msg = std::move(user_notices_.front());
|
||||
user_notices_.pop_front();
|
||||
return msg;
|
||||
}
|
||||
|
||||
|
||||
void
|
||||
SwapManager::notify_user_locked_(std::string msg)
|
||||
{
|
||||
if (user_notices_.size() < 8)
|
||||
user_notices_.push_back(std::move(msg));
|
||||
}
|
||||
|
||||
|
||||
void
|
||||
SwapManager::RecordCheckpoint(Buffer &buf, const bool urgent_flush)
|
||||
{
|
||||
@@ -1327,8 +1359,16 @@ SwapManager::process_one(const Pending &p)
|
||||
auto mark_gap = [&]() {
|
||||
std::lock_guard<std::mutex> lg(mtx_);
|
||||
auto it = journals_.find(p.buf);
|
||||
if (it != journals_.end())
|
||||
if (it != journals_.end()) {
|
||||
it->second.gap = true;
|
||||
if (!it->second.gap_notified) {
|
||||
it->second.gap_notified = true;
|
||||
const std::string &jp = it->second.path;
|
||||
notify_user_locked_("Crash-recovery journal write failed" +
|
||||
(jp.empty() ? std::string() : " (" + jp + ")") +
|
||||
"; recent edits may not be recoverable");
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
// Check circuit breaker before processing
|
||||
@@ -1399,6 +1439,8 @@ SwapManager::process_one(const Pending &p)
|
||||
{
|
||||
std::lock_guard<std::mutex> lg(mtx_);
|
||||
ctxp->locked_out = true;
|
||||
notify_user_locked_("File is being edited in another kte; crash recovery is off here (" +
|
||||
path + ")");
|
||||
}
|
||||
report_error(open_err, p.buf);
|
||||
return; // not an I/O failure; later records are skipped
|
||||
@@ -1467,7 +1509,8 @@ SwapManager::process_one(const Pending &p)
|
||||
}
|
||||
if (p.type == SwapRecType::CHKPT) {
|
||||
std::lock_guard<std::mutex> lg(mtx_);
|
||||
ctxp->gap = false;
|
||||
ctxp->gap = false;
|
||||
ctxp->gap_notified = false;
|
||||
}
|
||||
ctxp->approx_size_bytes += static_cast<std::uint64_t>(rec.size());
|
||||
if (p.urgent_flush) {
|
||||
|
||||
@@ -87,6 +87,12 @@ public:
|
||||
// before the user stopped typing would stay behind indefinitely.
|
||||
void RetryGapCheckpoints();
|
||||
|
||||
// Main thread: the oldest pending message for the user (a journal that
|
||||
// another kte holds, or that fell behind because writes failed), or ""
|
||||
// when there is none. Such failures otherwise went only to the error
|
||||
// log, leaving crash recovery silently off or incomplete.
|
||||
std::string TakeUserNotice();
|
||||
|
||||
|
||||
void SetConfig(const SwapConfig &cfg)
|
||||
{
|
||||
@@ -219,6 +225,7 @@ private:
|
||||
// position-based, so later ones no longer apply to the journal's state:
|
||||
// drop them until a checkpoint re-establishes the full content.
|
||||
bool gap{false};
|
||||
bool gap_notified{false}; // the user was told about the current gap
|
||||
std::uint64_t gap_chkpt_request_ns{0};
|
||||
bool gap_unfixable_reported{false}; // gap on a buffer too large to checkpoint
|
||||
// Another kte process holds this journal's lock: do not write to it.
|
||||
@@ -316,6 +323,10 @@ private:
|
||||
|
||||
// Error tracking (protected by mtx_)
|
||||
std::deque<SwapError> errors_; // bounded to max 100 entries
|
||||
std::deque<std::string> user_notices_; // see TakeUserNotice(); guarded by mtx_
|
||||
|
||||
// Queue a message for the user (mtx_ must be held).
|
||||
void notify_user_locked_(std::string msg);
|
||||
std::size_t total_error_count_{0};
|
||||
|
||||
// Circuit breaker for swap operations (protected by mtx_)
|
||||
|
||||
@@ -444,6 +444,23 @@ before the fix (unless noted).
|
||||
passed under a correctly instrumented ASan build with leak checks,
|
||||
and the stress fuzzers ran with ASan and UBSan.)
|
||||
|
||||
**Sixth round (real terminal, file I/O, recovery end to end)**
|
||||
- UTF-8 typed or pasted over a slow link lost characters: with the
|
||||
input batch read at zero timeout, ncurses gave up on a character split
|
||||
across reads and discarded the rest as invalid. kte now assembles
|
||||
UTF-8 itself and waits for continuation bytes. (This regression came
|
||||
from this audit's own input batching.)
|
||||
- Visual-line (multi-line) insert, delete, backspace and newline used
|
||||
the cursor's byte column on every selected line, splitting multibyte
|
||||
characters on the others; the column is now carried in characters.
|
||||
- Journal failures reached only the error log: a session locked out of
|
||||
the journal by another kte (which only happens when the other opened
|
||||
the file before this one's first edit), or whose journal writes
|
||||
failed, silently had no or partial crash recovery. The status line now
|
||||
says so (once per failure).
|
||||
- Basenames over about 233 bytes produced a swap name over NAME_MAX, so
|
||||
such files were never journaled.
|
||||
|
||||
**Known limitations (not fixed)**
|
||||
- Catastrophic regex backtracking (e.g. `(a*)*b`) can still take very
|
||||
long; std::regex has no time limit.
|
||||
@@ -452,6 +469,9 @@ before the fix (unless noted).
|
||||
- GUI-only items B7, B8, B10 and highlighter edge cases B13 are
|
||||
unchanged; the ImGui and Qt frontends could not be built in the review
|
||||
environment (changed GUI files were syntax-checked where possible).
|
||||
- Journals are created at the first edit, so a second kte that opens
|
||||
the file before the first one edits it is not warned at open; the
|
||||
session that is locked out is told at its first edit instead.
|
||||
- A journal whose buffer exceeds 16 MiB cannot be checkpointed; after a
|
||||
lost record such a journal stays incomplete until the file is saved
|
||||
(reported to the user).
|
||||
|
||||
@@ -1441,3 +1441,80 @@ TEST(Audit_VisualLine_EditsKeepUtf8Whole)
|
||||
ASSERT_EQ(h.Text(), std::string("a\nbcdef\n\xE4\xB8\x96\n\xE7\x95\x8C\n"));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
// A session that cannot journal its buffer (another kte holds the journal,
|
||||
// or writes fail) now says so in the status line; it used to go only to
|
||||
// the error log, leaving crash recovery silently off.
|
||||
TEST(Audit_Swap_FailuresReachTheUser)
|
||||
{
|
||||
TempDir d("swap_notice");
|
||||
const std::string path = (d.path / "f.txt").string();
|
||||
std::ofstream(path) << "base\n";
|
||||
const std::string swp = kte::SwapManager::ComputeSwapPathForFilename(path);
|
||||
std::filesystem::remove_all(swp);
|
||||
|
||||
// Session A holds the journal; session B is locked out.
|
||||
Buffer a, b;
|
||||
std::string err;
|
||||
ASSERT_TRUE(a.OpenFromFile(path, err));
|
||||
ASSERT_TRUE(b.OpenFromFile(path, err));
|
||||
kte::SwapManager sa, sb;
|
||||
sa.Attach(&a);
|
||||
a.SetSwapRecorder(sa.RecorderFor(&a));
|
||||
a.insert_text(0, 0, "A");
|
||||
sa.Flush(&a);
|
||||
ASSERT_EQ(sa.TakeUserNotice(), std::string());
|
||||
sb.Attach(&b);
|
||||
b.SetSwapRecorder(sb.RecorderFor(&b));
|
||||
b.insert_text(0, 0, "B");
|
||||
sb.Flush(&b);
|
||||
ASSERT_TRUE(sb.TakeUserNotice().find("another kte") != std::string::npos);
|
||||
ASSERT_EQ(sb.TakeUserNotice(), std::string());
|
||||
b.SetSwapRecorder(nullptr);
|
||||
sb.Detach(&b, false);
|
||||
a.SetSwapRecorder(nullptr);
|
||||
sa.Detach(&a, true);
|
||||
|
||||
// Writes fail: told once, not per keystroke.
|
||||
std::filesystem::remove_all(swp);
|
||||
std::filesystem::create_directories(swp);
|
||||
Buffer c;
|
||||
ASSERT_TRUE(c.OpenFromFile(path, err));
|
||||
kte::SwapManager sc;
|
||||
sc.Attach(&c);
|
||||
c.SetSwapRecorder(sc.RecorderFor(&c));
|
||||
c.insert_text(0, 0, "C");
|
||||
c.insert_text(0, 1, "D");
|
||||
sc.Flush(&c);
|
||||
ASSERT_TRUE(sc.TakeUserNotice().find("journal write failed") != std::string::npos);
|
||||
ASSERT_EQ(sc.TakeUserNotice(), std::string());
|
||||
c.SetSwapRecorder(nullptr);
|
||||
sc.Detach(&c, false);
|
||||
std::filesystem::remove_all(swp);
|
||||
}
|
||||
|
||||
|
||||
// A file whose basename is too long for the encoded swap name fell back to
|
||||
// "<basename>.<hash>.swp", which exceeded NAME_MAX: no journal at all.
|
||||
TEST(Audit_Swap_LongBasenameStillJournaled)
|
||||
{
|
||||
TempDir d("swap_longname");
|
||||
const std::string path = (d.path / (std::string(240, 'n') + ".txt")).string();
|
||||
std::ofstream(path) << "base\n";
|
||||
const std::string swp = kte::SwapManager::ComputeSwapPathForFilename(path);
|
||||
ASSERT_TRUE(std::filesystem::path(swp).filename().string().size() <= 255);
|
||||
std::filesystem::remove(swp);
|
||||
Buffer a;
|
||||
std::string err;
|
||||
ASSERT_TRUE(a.OpenFromFile(path, err));
|
||||
kte::SwapManager sm;
|
||||
sm.Attach(&a);
|
||||
a.SetSwapRecorder(sm.RecorderFor(&a));
|
||||
a.insert_text(0, 0, "X");
|
||||
sm.Flush(&a);
|
||||
ASSERT_TRUE(std::filesystem::exists(swp));
|
||||
a.SetSwapRecorder(nullptr);
|
||||
sm.Detach(&a, true);
|
||||
ASSERT_TRUE(!std::filesystem::exists(swp));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user