4 Commits

Author SHA1 Message Date
dailz
d37ed6df68 fix(io): harden read_cache against zero-length false hits and overflow (closes #13)
- Add early return for len==0 (Ok(&[])) matching std::io semantics
- Add slot.len > 0 guard to cache hit predicate to prevent empty-slot
  false matches
- Replace unchecked arithmetic with checked_add/saturating_add for
  request_end, block_end, and post-read coverage check
- Fix misleading comment about get(file,0,0) behavior on miss path
- Strengthen clear() to fully reset block_offset and last_access
- Register read_cache module in io/mod.rs
- Add 4 regression tests: zero-len on fresh/populated cache,
  zero-len at u64::MAX, overflow error on nonzero read at u64::MAX
2026-06-09 10:48:34 +08:00
dailz
b58d66f2aa fix(io): use unicode-width for correct CJK/emoji/zero-width display width (closes #12) 2026-06-07 12:50:17 +08:00
dailz
d4679a7543 fix(io): update visual height of last line on append without trailing newline (closes #11)
When a file does not end with a newline, appending content extends the
last logical line's text and thus its visual height. The incremental
extend path in handle_file_appended only computed heights for newly
created logical lines, missing the old last line whose content changed.

Add VisualHeightIndex::replace_last_line_height() — an O(1) method that
rewrites the final prefix sum entry and total. Called before
extend_from_heights so the correct line is targeted.

Changes:
- progressive_reader.rs: add replace_last_line_height, pub with_params,
  7 VHI unit tests
- app.rs: save old_reader_line_count before update, recompute last old
  line height in extend path, 2 integration regression tests
2026-06-07 09:46:24 +08:00
dailz
8844e58cb4 Merge fix/m20-append-lines-error-handling: fix append_lines I/O error swallowing (closes #45) + clippy cleanup 2026-06-07 09:17:41 +08:00
8 changed files with 334 additions and 13 deletions

1
Cargo.lock generated
View File

@@ -2333,6 +2333,7 @@ dependencies = [
"tempfile",
"thiserror 2.0.18",
"toml",
"unicode-width",
"xxhash-rust",
]

View File

@@ -27,3 +27,4 @@ textwrap = "0.16"
tempfile = "3"
xxhash-rust = { version = "0.8", features = ["xxh3"] }
bincode = "1"
unicode-width = "0.2"

View File

@@ -17,6 +17,7 @@ memmap2.workspace = true
directories.workspace = true
xxhash-rust.workspace = true
bincode.workspace = true
unicode-width.workspace = true
[dev-dependencies]
insta.workspace = true

View File

@@ -18,4 +18,5 @@ pub mod cache_util;
pub mod index_cache;
pub mod line_sampler;
pub mod progressive_reader;
pub mod read_cache;
pub mod wrap;

View File

@@ -102,7 +102,7 @@ impl VisualHeightIndex {
}
}
fn with_params(mut self, json_format: bool, terminal_width: usize) -> Self {
pub fn with_params(mut self, json_format: bool, terminal_width: usize) -> Self {
self.json_format = json_format;
self.terminal_width = terminal_width;
self
@@ -159,6 +159,31 @@ impl VisualHeightIndex {
self.total_visual_rows += h as u64;
}
}
/// Replace the visual height of the last logical line. O(1).
///
/// Must be called **before** `extend_from_heights` so that the last line
/// index still refers to the pre-extension line.
pub fn replace_last_line_height(&mut self, new_height: usize) {
let n = self.prefix_sums.len();
if n < 2 {
return;
}
let last_line = n - 2;
let old_height = self.prefix_sums[last_line + 1] - self.prefix_sums[last_line];
let new_height = new_height as u64;
if new_height == old_height {
return;
}
let delta = new_height.abs_diff(old_height);
if new_height > old_height {
self.prefix_sums[last_line + 1] += delta;
self.total_visual_rows += delta;
} else {
self.prefix_sums[last_line + 1] -= delta;
self.total_visual_rows -= delta;
}
}
}
// ─── VisualHeightRebuildResult ────────────────────────────────────────────────
@@ -1339,6 +1364,85 @@ mod tests {
assert_eq!(idx.total_visual_rows(), 6);
}
#[test]
fn test_replace_last_line_height_increase() {
let heights = [2, 3];
let mut idx = VisualHeightIndex::build(&heights);
assert_eq!(idx.total_visual_rows(), 5);
assert_eq!(idx.visual_height_of_line(1), 3);
idx.replace_last_line_height(7);
assert_eq!(idx.visual_height_of_line(0), 2);
assert_eq!(idx.visual_height_of_line(1), 7);
assert_eq!(idx.total_visual_rows(), 9);
assert_eq!(idx.cursor_to_first_visual_row(0), 0);
assert_eq!(idx.cursor_to_first_visual_row(1), 2);
}
#[test]
fn test_replace_last_line_height_decrease() {
let heights = [2, 5];
let mut idx = VisualHeightIndex::build(&heights);
idx.replace_last_line_height(1);
assert_eq!(idx.visual_height_of_line(0), 2);
assert_eq!(idx.visual_height_of_line(1), 1);
assert_eq!(idx.total_visual_rows(), 3);
}
#[test]
fn test_replace_last_line_height_same_is_noop() {
let heights = [2, 3];
let mut idx = VisualHeightIndex::build(&heights);
let total_before = idx.total_visual_rows();
idx.replace_last_line_height(3);
assert_eq!(idx.total_visual_rows(), total_before);
}
#[test]
fn test_replace_last_line_height_then_extend() {
let heights = [2, 1];
let mut idx = VisualHeightIndex::build(&heights);
assert_eq!(idx.total_visual_rows(), 3);
idx.replace_last_line_height(4);
idx.extend_from_heights(&[3]);
assert_eq!(idx.line_count(), 3);
assert_eq!(idx.visual_height_of_line(0), 2);
assert_eq!(idx.visual_height_of_line(1), 4);
assert_eq!(idx.visual_height_of_line(2), 3);
assert_eq!(idx.total_visual_rows(), 9);
assert_eq!(idx.cursor_to_first_visual_row(0), 0);
assert_eq!(idx.cursor_to_first_visual_row(1), 2);
assert_eq!(idx.cursor_to_first_visual_row(2), 6);
}
#[test]
fn test_replace_last_line_height_single_line() {
let heights = [5];
let mut idx = VisualHeightIndex::build(&heights);
idx.replace_last_line_height(2);
assert_eq!(idx.visual_height_of_line(0), 2);
assert_eq!(idx.total_visual_rows(), 2);
}
#[test]
fn test_replace_last_line_height_empty_index() {
let heights: [usize; 0] = [];
let mut idx = VisualHeightIndex::build(&heights);
idx.replace_last_line_height(5);
assert_eq!(idx.total_visual_rows(), 0);
}
#[test]
fn test_spawn_indexer_file_truncated_during_scan() {
let mut content = Vec::new();

View File

@@ -56,9 +56,15 @@ impl LruReadCache {
/// on a hit, or fills a cache slot on a miss. Cross-block reads go through
/// the spill buffer and are not cached.
pub fn get(&mut self, file: &File, offset: u64, len: usize) -> io::Result<&[u8]> {
if len == 0 {
return Ok(&[]);
}
let aligned_key = offset & !(BLOCK_ALIGN as u64 - 1);
let request_end = offset.saturating_add(len as u64);
let block_end = aligned_key + BLOCK_ALIGN as u64;
let request_end = offset.checked_add(len as u64).ok_or_else(|| {
io::Error::new(io::ErrorKind::InvalidInput, "read range overflows u64")
})?;
let block_end = aligned_key.saturating_add(BLOCK_ALIGN as u64);
if request_end > block_end {
self.spill_buf.resize(len, 0);
@@ -74,7 +80,8 @@ impl LruReadCache {
}
let hit_idx = self.slots.iter().position(|slot| {
slot.block_offset == aligned_key && request_end <= slot.block_offset + slot.len as u64
let slot_end = slot.block_offset.saturating_add(slot.len as u64);
slot.len > 0 && slot.block_offset == aligned_key && request_end <= slot_end
});
if let Some(idx) = hit_idx {
@@ -96,8 +103,8 @@ impl LruReadCache {
let slot = &mut self.slots[evict_idx];
let bytes_read = file.read_at(&mut slot.buf, aligned_key)?;
// Note: get(file, 0, 0) on an empty file now returns Err (old code returned Ok(&[])).
// No callers pass len == 0, so this is a safe semantic change.
// Non-empty reads that return 0 are EOF. Zero-length reads are handled above
// as a successful no-op.
if bytes_read == 0 {
return Err(io::Error::new(io::ErrorKind::UnexpectedEof, "read 0 bytes"));
}
@@ -107,7 +114,8 @@ impl LruReadCache {
slot.last_access = self.tick;
self.tick += 1;
if request_end > aligned_key + bytes_read as u64 {
let bytes_end = aligned_key.saturating_add(bytes_read as u64);
if request_end > bytes_end {
return Err(io::Error::new(io::ErrorKind::UnexpectedEof, "short read"));
}
@@ -118,7 +126,9 @@ impl LruReadCache {
/// Invalidate all cache slots and the spill buffer.
pub fn clear(&mut self) {
for slot in &mut self.slots {
slot.block_offset = 0;
slot.len = 0;
slot.last_access = 0;
}
self.spill_len = 0;
}
@@ -314,9 +324,11 @@ mod tests {
cache.clear();
// All slots should have len == 0.
// All slots should be fully reset.
for slot in &cache.slots {
assert_eq!(slot.block_offset, 0);
assert_eq!(slot.len, 0);
assert_eq!(slot.last_access, 0);
}
assert_eq!(cache.spill_len, 0);
@@ -432,4 +444,52 @@ mod tests {
assert_eq!(&line2[..4090], &[b'B'; 4090]);
assert_eq!(line2[4090], b'\n');
}
#[test]
fn zero_len_read_is_noop_on_fresh_cache() {
let f = make_file(b"");
let file = File::open(f.path()).unwrap();
let mut cache = ReadCache::new();
let result = cache.get(&file, 0, 0).unwrap();
assert!(result.is_empty());
assert_eq!(cache.tick, 0);
assert!(cache.slots.iter().all(|s| s.len == 0));
}
#[test]
fn zero_len_read_is_noop_on_populated_cache() {
let f = make_file(b"abc");
let file = File::open(f.path()).unwrap();
let mut cache = ReadCache::new();
cache.get(&file, 0, 1).unwrap();
let tick_before = cache.tick;
let result = cache.get(&file, 0, 0).unwrap();
assert!(result.is_empty());
assert_eq!(cache.tick, tick_before);
}
#[test]
fn zero_len_read_at_max_offset_is_ok() {
let f = make_file(b"");
let file = File::open(f.path()).unwrap();
let mut cache = ReadCache::new();
let result = cache.get(&file, u64::MAX, 0).unwrap();
assert!(result.is_empty());
assert_eq!(cache.tick, 0);
assert!(cache.slots.iter().all(|s| s.len == 0));
}
#[test]
fn nonzero_read_range_overflow_returns_invalid_input() {
let f = make_file(b"abc");
let file = File::open(f.path()).unwrap();
let mut cache = ReadCache::new();
let err = cache.get(&file, u64::MAX, 1).unwrap_err();
assert_eq!(err.kind(), io::ErrorKind::InvalidInput);
}
}

View File

@@ -2,9 +2,12 @@
/// Lines exceeding this are returned as-is to avoid pathological cases.
pub const MAX_WRAP_INPUT_LEN: usize = 10 * 1024 * 1024;
/// Split a line into chunks of exactly `width` characters (display columns).
/// Split a line into chunks of exactly `width` display columns.
/// For a log viewer, we want character-level wrapping, not word-level.
/// Uses `unicode-width` for correct CJK/emoji/zero-width handling.
pub fn wrap_line_chars(line: &str, width: usize) -> Vec<String> {
use unicode_width::UnicodeWidthChar;
if width == 0 {
return vec![String::new()];
}
@@ -15,7 +18,15 @@ pub fn wrap_line_chars(line: &str, width: usize) -> Vec<String> {
let mut row = String::new();
let mut col = 0;
for ch in line.chars() {
let w = if ch == '\t' { 4 } else { 1 };
let w = if ch == '\t' {
4
} else if ch.is_control() {
// Control characters (except tab): width 0, still pushed to preserve content.
// Visible rendering is the caller's responsibility.
0
} else {
ch.width().unwrap_or(0)
};
if col + w > width && !row.is_empty() {
result.push(std::mem::take(&mut row));
col = 0;
@@ -132,4 +143,48 @@ mod tests {
fn test_max_wrap_input_len_constant() {
assert_eq!(MAX_WRAP_INPUT_LEN, 10 * 1024 * 1024);
}
#[test]
fn test_wrap_cjk_chars() {
let result = wrap_line_chars("你好", 3);
assert_eq!(result, vec!["", ""]);
}
#[test]
fn test_wrap_cjk_ascii_mixed() {
let result = wrap_line_chars("a你好", 4);
assert_eq!(result, vec!["a你", ""]);
}
#[test]
fn test_wrap_zero_width_char() {
let result = wrap_line_chars("a\u{200B}b", 2);
assert_eq!(result, vec!["a\u{200B}b"]);
}
#[test]
fn test_wrap_emoji() {
let result = wrap_line_chars("😀a", 3);
assert_eq!(result, vec!["😀a"]);
}
#[test]
fn test_wrap_emoji_exact_wrap() {
let result = wrap_line_chars("😀a", 2);
assert_eq!(result, vec!["😀", "a"]);
}
#[test]
fn test_wrap_combining_mark() {
// Scalar-width wrapping: combining mark (width 0) stays with next base char,
// not the preceding one, because the base char already triggered a flush.
let result = wrap_line_chars("a\u{0301}b", 1);
assert_eq!(result, vec!["a", "\u{0301}b"]);
}
#[test]
fn test_wrap_cjk_width_one() {
let result = wrap_line_chars("你好", 1);
assert_eq!(result, vec!["", ""]);
}
}

View File

@@ -865,6 +865,7 @@ impl App {
let width = self.get_content_width();
match &mut self.loading_state {
AppLoadingState::Ready { reader } => {
let old_reader_line_count = reader.line_count();
let status = reader.update_for_append();
match status {
Ok(
@@ -883,13 +884,24 @@ impl App {
};
let new_line_count = reader.line_count();
if can_extend && new_line_count > old_line_count {
if can_extend && old_line_count == old_reader_line_count {
if let log_viewer_core::io::progressive_reader::ReaderState::Ready {
visual_height_index: Some(index),
reader: fr,
} = &mut reader.state
{
let mut new_heights = Vec::with_capacity(new_line_count - old_line_count);
if old_line_count > 0 {
let last_old_line_text =
fr.get_line(old_line_count - 1).unwrap_or("");
let new_h = compute_line_visual_height(
last_old_line_text,
width,
self.json_format,
);
index.replace_last_line_height(new_h);
}
let mut new_heights =
Vec::with_capacity(new_line_count.saturating_sub(old_line_count));
for i in old_line_count..new_line_count {
let line_text = fr.get_line(i).unwrap_or("");
new_heights.push(compute_line_visual_height(
@@ -2473,7 +2485,9 @@ plain text line
}
fn install_vhi(app: &mut App, heights: &[usize]) {
let vhi = VisualHeightIndex::build(heights);
let width = app.get_content_width();
let json_format = app.json_format;
let vhi = VisualHeightIndex::build(heights).with_params(json_format, width);
if let AppLoadingState::Ready { reader } = &mut app.loading_state {
if let log_viewer_core::io::progressive_reader::ReaderState::Ready {
visual_height_index,
@@ -2800,4 +2814,88 @@ plain text line
cleanup(&path);
assert!(result.is_ok());
}
#[test]
fn test_append_no_trailing_newline_updates_last_line_height() {
let path = make_temp_file("abc");
let result = std::panic::catch_unwind(|| {
let mut app = App::new();
load_file_ready(&mut app, &path);
assert_eq!(app.total_lines(), 1);
app.content_width = 5;
install_vhi(&mut app, &[1usize]);
{
let vhi = app.get_visual_height_index().unwrap();
assert_eq!(vhi.visual_height_of_line(0), 1);
assert_eq!(vhi.total_visual_rows(), 1);
}
// Append content that extends line 0 (no trailing newline before)
// "abc" + "defgh\n" = "abcdefgh\n" → 8 chars in width 5 → wraps to 2 visual rows
// old_total=1, old_had_trailing=false → starts_new_line=false
// new_has_trailing=true, new_newlines=1 → added = 1-1 = 0
// Still 1 logical line, but line 0 text changed from "abc" to "abcdefgh"
{
use std::io::Write;
let mut f = std::fs::OpenOptions::new()
.append(true)
.open(&path)
.unwrap();
f.write_all(b"defgh\n").unwrap();
}
std::thread::sleep(std::time::Duration::from_millis(500));
app.poll_file_watcher();
assert_eq!(app.total_lines(), 1,
"\"abcdefgh\\n\" has trailing newline → 1 logical line");
let vhi = app.get_visual_height_index().expect("VHI should still exist after append");
assert_eq!(vhi.visual_height_of_line(0), 2,
"line 0 height should be updated from 1 to 2 after extending 'abcdefgh' in width 5");
assert_eq!(vhi.total_visual_rows(), 2);
assert_eq!(vhi.cursor_to_first_visual_row(0), 0);
cleanup(&path);
});
assert!(result.is_ok());
}
#[test]
fn test_append_no_trailing_newline_no_new_lines_only_height_change() {
let path = make_temp_file("abc");
let result = std::panic::catch_unwind(|| {
let mut app = App::new();
load_file_ready(&mut app, &path);
assert_eq!(app.total_lines(), 1);
app.content_width = 5;
install_vhi(&mut app, &[1usize]);
// Append without adding any new line — just extends line 0
// "abc" + "def" = "abcdef" → 6 chars in width 5 → wraps to 2 visual rows, still 1 logical line
{
use std::io::Write;
let mut f = std::fs::OpenOptions::new()
.append(true)
.open(&path)
.unwrap();
f.write_all(b"def").unwrap();
}
std::thread::sleep(std::time::Duration::from_millis(500));
app.poll_file_watcher();
assert_eq!(app.total_lines(), 1,
"no new logical line should be added");
let vhi = app.get_visual_height_index().expect("VHI should still exist");
assert_eq!(vhi.visual_height_of_line(0), 2,
"line 0 height should update even when no new lines added");
assert_eq!(vhi.total_visual_rows(), 2);
cleanup(&path);
});
assert!(result.is_ok());
}
}