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 '<journal>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB
This commit is contained in:
@@ -489,25 +489,33 @@ Editor::ResolveRecoveryPrompt(const bool yes)
|
|||||||
bool
|
bool
|
||||||
Editor::ProcessPendingOpens()
|
Editor::ProcessPendingOpens()
|
||||||
{
|
{
|
||||||
// Opening can throw (bad_alloc on a huge file, filesystem errors). This
|
bool opened = false;
|
||||||
// runs from every frontend's loop, outside command dispatch: report the
|
// Opening can throw (bad_alloc on a huge file, filesystem errors), and so
|
||||||
// failure instead of letting it end the editor.
|
// 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 {
|
try {
|
||||||
// Also the once-per-frame hook for the journal: retry the
|
// Also the once-per-frame hook for the journal: retry the
|
||||||
// checkpoint of a journal that lost a record.
|
// checkpoint of a journal that lost a record.
|
||||||
if (kte::SwapManager *sm = Swap()) {
|
if (kte::SwapManager *sm = Swap())
|
||||||
sm->RetryGapCheckpoints();
|
sm->RetryGapCheckpoints();
|
||||||
if (std::string notice = sm->TakeUserNotice(); !notice.empty())
|
opened = process_pending_opens_();
|
||||||
SetStatus(notice);
|
|
||||||
}
|
|
||||||
return process_pending_opens_();
|
|
||||||
} catch (const std::exception &e) {
|
} catch (const std::exception &e) {
|
||||||
kte::ErrorHandler::Instance().Error("Editor", std::string("open failed: ") + e.what(), "");
|
kte::ErrorHandler::Instance().Error("Editor", std::string("open failed: ") + e.what(), "");
|
||||||
SetStatus(std::string("Open failed: ") + e.what());
|
SetStatus(std::string("Open failed: ") + e.what());
|
||||||
} catch (...) {
|
} catch (...) {
|
||||||
SetStatus("Open failed");
|
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;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -206,9 +206,12 @@ compute_swap_path_for_filename(const std::string &filename)
|
|||||||
const std::string suffix = "." + hex_u64(fnv1a64(key)) + ".swp";
|
const std::string suffix = "." + hex_u64(fnv1a64(key)) + ".swp";
|
||||||
// A long basename made even this name exceed NAME_MAX (255), so the
|
// A long basename made even this name exceed NAME_MAX (255), so the
|
||||||
// journal could never be created. Shorten only such names (the hash
|
// journal could never be created. Shorten only such names (the hash
|
||||||
// keeps them distinct), on a UTF-8 boundary.
|
// keeps them distinct), on a UTF-8 boundary, leaving room for the
|
||||||
if (base.size() + suffix.size() > 255) {
|
// ".tmp" sibling that compaction writes (at exactly 255 compaction
|
||||||
std::size_t keep = 255 - suffix.size();
|
// 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<unsigned char>(base[keep]) & 0xC0) == 0x80)
|
while (keep > 0 && (static_cast<unsigned char>(base[keep]) & 0xC0) == 0x80)
|
||||||
--keep;
|
--keep;
|
||||||
base.resize(keep);
|
base.resize(keep);
|
||||||
@@ -1439,9 +1442,13 @@ SwapManager::process_one(const Pending &p)
|
|||||||
{
|
{
|
||||||
std::lock_guard<std::mutex> lg(mtx_);
|
std::lock_guard<std::mutex> lg(mtx_);
|
||||||
ctxp->locked_out = true;
|
ctxp->locked_out = true;
|
||||||
notify_user_locked_("File is being edited in another kte; crash recovery is off here (" +
|
if (!ctxp->lockout_notified) {
|
||||||
|
ctxp->lockout_notified = true;
|
||||||
|
notify_user_locked_(
|
||||||
|
"File is being edited in another kte; crash recovery is off here (" +
|
||||||
path + ")");
|
path + ")");
|
||||||
}
|
}
|
||||||
|
}
|
||||||
report_error(open_err, p.buf);
|
report_error(open_err, p.buf);
|
||||||
return; // not an I/O failure; later records are skipped
|
return; // not an I/O failure; later records are skipped
|
||||||
}
|
}
|
||||||
@@ -1453,6 +1460,11 @@ SwapManager::process_one(const Pending &p)
|
|||||||
mark_gap();
|
mark_gap();
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
{
|
||||||
|
// Holding the journal now; a later lock-out is news again.
|
||||||
|
std::lock_guard<std::mutex> lg(mtx_);
|
||||||
|
ctxp->lockout_notified = false;
|
||||||
|
}
|
||||||
if (p.payload.size() > kMaxRecordPayload) {
|
if (p.payload.size() > kMaxRecordPayload) {
|
||||||
// Recorders keep payloads within the limit, so this is a bug, not an
|
// Recorders keep payloads within the limit, so this is a bug, not an
|
||||||
// I/O failure: do not count it against the circuit breaker.
|
// I/O failure: do not count it against the circuit breaker.
|
||||||
|
|||||||
@@ -226,6 +226,7 @@ private:
|
|||||||
// drop them until a checkpoint re-establishes the full content.
|
// drop them until a checkpoint re-establishes the full content.
|
||||||
bool gap{false};
|
bool gap{false};
|
||||||
bool gap_notified{false}; // the user was told about the current gap
|
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};
|
std::uint64_t gap_chkpt_request_ns{0};
|
||||||
bool gap_unfixable_reported{false}; // gap on a buffer too large to checkpoint
|
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.
|
// Another kte process holds this journal's lock: do not write to it.
|
||||||
|
|||||||
+11
-3
@@ -393,9 +393,11 @@ read_key(int &ch, bool &is_keycode)
|
|||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
// The rest of the character is on its way; wait for it (restoring the
|
// 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);
|
const int saved_delay = wgetdelay(stdscr);
|
||||||
timeout(1000);
|
timeout(250);
|
||||||
for (int i = 0; i < need; ++i) {
|
for (int i = 0; i < need; ++i) {
|
||||||
const int b = getch();
|
const int b = getch();
|
||||||
if (b == ERR || b > 0xFF || (b & 0xC0) != 0x80) {
|
if (b == ERR || b > 0xFF || (b & 0xC0) != 0x80) {
|
||||||
@@ -427,7 +429,13 @@ TerminalInputHandler::decode_(MappedInput &out)
|
|||||||
if (!read_key(ch, is_keycode))
|
if (!read_key(ch, is_keycode))
|
||||||
return false; // no input
|
return false; // no input
|
||||||
if (ch < 0) {
|
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;
|
return true;
|
||||||
}
|
}
|
||||||
bool consumed = map_key_to_command(
|
bool consumed = map_key_to_command(
|
||||||
|
|||||||
@@ -459,7 +459,9 @@ before the fix (unless noted).
|
|||||||
failed, silently had no or partial crash recovery. The status line now
|
failed, silently had no or partial crash recovery. The status line now
|
||||||
says so (once per failure).
|
says so (once per failure).
|
||||||
- Basenames over about 233 bytes produced a swap name over NAME_MAX, so
|
- 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)**
|
**Known limitations (not fixed)**
|
||||||
- Catastrophic regex backtracking (e.g. `(a*)*b`) can still take very
|
- Catastrophic regex backtracking (e.g. `(a*)*b`) can still take very
|
||||||
|
|||||||
@@ -1471,6 +1471,12 @@ TEST(Audit_Swap_FailuresReachTheUser)
|
|||||||
sb.Flush(&b);
|
sb.Flush(&b);
|
||||||
ASSERT_TRUE(sb.TakeUserNotice().find("another kte") != std::string::npos);
|
ASSERT_TRUE(sb.TakeUserNotice().find("another kte") != std::string::npos);
|
||||||
ASSERT_EQ(sb.TakeUserNotice(), std::string());
|
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);
|
b.SetSwapRecorder(nullptr);
|
||||||
sb.Detach(&b, false);
|
sb.Detach(&b, false);
|
||||||
a.SetSwapRecorder(nullptr);
|
a.SetSwapRecorder(nullptr);
|
||||||
@@ -1503,17 +1509,25 @@ TEST(Audit_Swap_LongBasenameStillJournaled)
|
|||||||
const std::string path = (d.path / (std::string(240, 'n') + ".txt")).string();
|
const std::string path = (d.path / (std::string(240, 'n') + ".txt")).string();
|
||||||
std::ofstream(path) << "base\n";
|
std::ofstream(path) << "base\n";
|
||||||
const std::string swp = kte::SwapManager::ComputeSwapPathForFilename(path);
|
const std::string swp = kte::SwapManager::ComputeSwapPathForFilename(path);
|
||||||
ASSERT_TRUE(std::filesystem::path(swp).filename().string().size() <= 255);
|
// Room for compaction's "<journal>.tmp" too.
|
||||||
|
ASSERT_TRUE(std::filesystem::path(swp).filename().string().size() + 4 <= 255);
|
||||||
std::filesystem::remove(swp);
|
std::filesystem::remove(swp);
|
||||||
Buffer a;
|
Buffer a;
|
||||||
std::string err;
|
std::string err;
|
||||||
ASSERT_TRUE(a.OpenFromFile(path, err));
|
ASSERT_TRUE(a.OpenFromFile(path, err));
|
||||||
kte::SwapManager sm;
|
kte::SwapManager sm;
|
||||||
|
kte::SwapConfig cfg;
|
||||||
|
cfg.checkpoint_bytes = 100;
|
||||||
|
cfg.compact_bytes = 1000;
|
||||||
|
sm.SetConfig(cfg);
|
||||||
sm.Attach(&a);
|
sm.Attach(&a);
|
||||||
a.SetSwapRecorder(sm.RecorderFor(&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);
|
sm.Flush(&a);
|
||||||
ASSERT_TRUE(std::filesystem::exists(swp));
|
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);
|
a.SetSwapRecorder(nullptr);
|
||||||
sm.Detach(&a, true);
|
sm.Detach(&a, true);
|
||||||
ASSERT_TRUE(!std::filesystem::exists(swp));
|
ASSERT_TRUE(!std::filesystem::exists(swp));
|
||||||
|
|||||||
Reference in New Issue
Block a user