From b9f91b0ed25cfb43b0e35f53eeeb4512400729d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 08:29:34 +0000 Subject: [PATCH] Round-seven fixes: swap name room for .tmp; input and notice details - The shortened fallback swap name was capped at exactly NAME_MAX, so compaction's '.tmp' failed with ENAMETOOLONG on every checkpoint and the journal grew by a full copy of the buffer each time. Cap it at NAME_MAX - 4. - A lone UTF-8 lead byte (8-bit Meta terminals) waited a second for continuation bytes on every key; wait 250 ms. An invalid byte now ends a pending ESC/C-k prefix instead of leaving it for the next key. - Journal notices are shown after a deferred open's own status (which used to overwrite them), all pending ones at once, and the lock-out notice is not repeated after every save while the lock is still held. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Editor.cc | 26 ++++++++++++++++--------- Swap.cc | 22 ++++++++++++++++----- Swap.h | 1 + TerminalInputHandler.cc | 14 ++++++++++--- docs/audits/claude-findings-20260923.md | 4 +++- tests/test_audit_regressions.cc | 18 +++++++++++++++-- 6 files changed, 65 insertions(+), 20 deletions(-) diff --git a/Editor.cc b/Editor.cc index b7f87fb..247e900 100644 --- a/Editor.cc +++ b/Editor.cc @@ -489,25 +489,33 @@ Editor::ResolveRecoveryPrompt(const bool yes) bool Editor::ProcessPendingOpens() { - // Opening can throw (bad_alloc on a huge file, filesystem errors). This - // runs from every frontend's loop, outside command dispatch: report the - // failure instead of letting it end the editor. + bool opened = false; + // Opening can throw (bad_alloc on a huge file, filesystem errors), and so + // can the checkpoint snapshot below. This runs from every frontend's + // loop, outside command dispatch: report the failure instead of letting + // it end the editor. 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_(); + opened = process_pending_opens_(); } catch (const std::exception &e) { kte::ErrorHandler::Instance().Error("Editor", std::string("open failed: ") + e.what(), ""); SetStatus(std::string("Open failed: ") + e.what()); } catch (...) { SetStatus("Open failed"); } - return false; + // Journal problems for the user, after the open's own status so it does + // not hide them; all pending ones at once (one per frame flashed by). + if (kte::SwapManager *sm = Swap()) { + std::string notices; + for (std::string n = sm->TakeUserNotice(); !n.empty(); n = sm->TakeUserNotice()) + notices += (notices.empty() ? "" : "; ") + n; + if (!notices.empty()) + SetStatus(notices); + } + return opened; } diff --git a/Swap.cc b/Swap.cc index 786d9cf..34faed8 100644 --- a/Swap.cc +++ b/Swap.cc @@ -206,9 +206,12 @@ compute_swap_path_for_filename(const std::string &filename) 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(); + // keeps them distinct), on a UTF-8 boundary, leaving room for the + // ".tmp" sibling that compaction writes (at exactly 255 compaction + // failed on every checkpoint and the journal grew without bound). + constexpr std::size_t kMaxName = 255 - 4; // strlen(".tmp") + if (base.size() + suffix.size() > kMaxName) { + std::size_t keep = kMaxName - suffix.size(); while (keep > 0 && (static_cast(base[keep]) & 0xC0) == 0x80) --keep; base.resize(keep); @@ -1439,8 +1442,12 @@ SwapManager::process_one(const Pending &p) { std::lock_guard lg(mtx_); ctxp->locked_out = true; - notify_user_locked_("File is being edited in another kte; crash recovery is off here (" + - path + ")"); + if (!ctxp->lockout_notified) { + ctxp->lockout_notified = 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 @@ -1453,6 +1460,11 @@ SwapManager::process_one(const Pending &p) mark_gap(); return; } + { + // Holding the journal now; a later lock-out is news again. + std::lock_guard lg(mtx_); + ctxp->lockout_notified = false; + } if (p.payload.size() > kMaxRecordPayload) { // Recorders keep payloads within the limit, so this is a bug, not an // I/O failure: do not count it against the circuit breaker. diff --git a/Swap.h b/Swap.h index bad9ba3..6969f4e 100644 --- a/Swap.h +++ b/Swap.h @@ -226,6 +226,7 @@ private: // 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 + bool lockout_notified{false}; // told about the lock; kept across ResetJournal 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. diff --git a/TerminalInputHandler.cc b/TerminalInputHandler.cc index 8130a46..da37f62 100644 --- a/TerminalInputHandler.cc +++ b/TerminalInputHandler.cc @@ -393,9 +393,11 @@ read_key(int &ch, bool &is_keycode) return true; } // The rest of the character is on its way; wait for it (restoring the - // caller's timeout), but not forever if it never comes. + // caller's timeout), but not long if it never comes: terminals that send + // Alt as a raw 8-bit byte (rxvt meta8) produce lone lead bytes, and + // every such key would otherwise stall input. const int saved_delay = wgetdelay(stdscr); - timeout(1000); + timeout(250); for (int i = 0; i < need; ++i) { const int b = getch(); if (b == ERR || b > 0xFF || (b & 0xC0) != 0x80) { @@ -427,7 +429,13 @@ TerminalInputHandler::decode_(MappedInput &out) if (!read_key(ch, is_keycode)) return false; // no input if (ch < 0) { - out.hasCommand = false; // invalid byte sequence, dropped + // Invalid byte sequence, dropped. It still ends a pending ESC or + // C-k prefix, as any other key would; left set, it applied to the + // next key instead. + esc_meta_ = false; + k_prefix_ = false; + k_ctrl_pending_ = false; + out.hasCommand = false; return true; } bool consumed = map_key_to_command( diff --git a/docs/audits/claude-findings-20260923.md b/docs/audits/claude-findings-20260923.md index 3cd3f14..d41e343 100644 --- a/docs/audits/claude-findings-20260923.md +++ b/docs/audits/claude-findings-20260923.md @@ -459,7 +459,9 @@ before the fix (unless noted). 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. + such files were never journaled. (The first fix capped the name at + exactly 255 bytes, which broke compaction's ".tmp" sibling and let + the journal grow without bound; it now leaves room for it.) **Known limitations (not fixed)** - Catastrophic regex backtracking (e.g. `(a*)*b`) can still take very diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index ef6e19f..861088f 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -1471,6 +1471,12 @@ TEST(Audit_Swap_FailuresReachTheUser) sb.Flush(&b); ASSERT_TRUE(sb.TakeUserNotice().find("another kte") != std::string::npos); ASSERT_EQ(sb.TakeUserNotice(), std::string()); + // Saving resets the journal and retries the lock; still held by A, but + // the user was already told. + sb.ResetJournal(b); + b.insert_text(0, 0, "b"); + sb.Flush(&b); + ASSERT_EQ(sb.TakeUserNotice(), std::string()); b.SetSwapRecorder(nullptr); sb.Detach(&b, false); a.SetSwapRecorder(nullptr); @@ -1503,17 +1509,25 @@ TEST(Audit_Swap_LongBasenameStillJournaled) 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); + // Room for compaction's ".tmp" too. + ASSERT_TRUE(std::filesystem::path(swp).filename().string().size() + 4 <= 255); std::filesystem::remove(swp); Buffer a; std::string err; ASSERT_TRUE(a.OpenFromFile(path, err)); kte::SwapManager sm; + kte::SwapConfig cfg; + cfg.checkpoint_bytes = 100; + cfg.compact_bytes = 1000; + sm.SetConfig(cfg); sm.Attach(&a); a.SetSwapRecorder(sm.RecorderFor(&a)); - a.insert_text(0, 0, "X"); + for (int i = 0; i < 50; ++i) + a.insert_text(0, 0, std::string(200, 'X')); sm.Flush(&a); ASSERT_TRUE(std::filesystem::exists(swp)); + // Compaction works: the journal holds about one checkpoint, not fifty. + ASSERT_TRUE(std::filesystem::file_size(swp) < 40000); a.SetSwapRecorder(nullptr); sm.Detach(&a, true); ASSERT_TRUE(!std::filesystem::exists(swp));