Bugfix: range_read cancellation lands within ~64 KB, not 16 MB

- The cancel flag was checked only between `get_lines` chunks (one per 4096 lines). For 4 KB/line files that meant 16 MB of uninterruptible read between checks, violating design-principles §3 "long operations are always cancelable." Added an inner-loop check that fires every 256 lines or every 64 KB of emitted output, whichever lands first. At typical 80-byte lines that's a check every ~20 KB; at pathological 4 KB lines it's every 16 lines.
- Rewrote the cancellation test to be deterministic: the previous version raced `thread::sleep(5ms)` against the read on a ~1 MB file and accepted `Ok | Cancelled` as a pass, which made the assertion meaningless. New shape:
  - `read_range_cancellation_returns_cancelled`: pre-flips the cancel flag, invokes `range_read::read_range` directly on a `FullLoadBackend`, asserts `matches!(result, Err(ViewerError::Cancelled))` strictly.
  - `read_range_session_cancellation_returns_cancelled_and_cleans_up`: end-to-end through the session on a 16 MB file (4096 lines × 4 KB). The read can't complete in the canceller's 2 ms sleep, so the in-loop check is exercised every iteration and the cancel lands. Also re-asserts `active_read_count == 0` post-cancel.
- While there: documented the CRLF assumption in `range_read.rs`. All three backends (`byte_seek.rs:118`, `full_load.rs:43`, `line_index.rs:172`) keep `\r` AS PART of the returned line string — they only split on `\n` and slice up to (but excluding) the `\n` byte. So `line.len()` already includes the `\r` for CRLF files; the `+ 1` in `chunk_end_offset` accounts only for the single `\n` delimiter. No drift on either LF or CRLF files. Added `read_range_full_load_crlf_preserves_carriage_returns` to pin this so a future "auto-strip \r" change doesn't silently drift `byte_offset`.

Author: vdavid

apps/desktop/src-tauri/src/file_viewer/range_read.rs

@@ -147,9 +147,19 @@ pub fn read_range(
     // three backends (ByteSeek's `Line(N)` is approximate; its byte-offset seeks are
     // exact, just back-scan for the surrounding newline).
     const FETCH_CHUNK: usize = 4096;
+    // Cancellation budget inside the per-line loop. The plan's "every 64 KB" was the
+    // target; we check whichever lands first: 256 lines (cheap line counter) or 64 KB
+    // of emitted text (cheap byte counter). At typical 80-byte lines that's a check
+    // every 20 KB; at 4 KB-per-line files (which would dwarf the 256-line cap) we'd
+    // check every ~16 lines. Either way the worst-case latency between Escape and
+    // `Cancelled` returning is well under the 100 ms threshold for "feels responsive."
+    const CANCEL_CHECK_LINES: usize = 256;
+    const CANCEL_CHECK_BYTES: usize = 64 * 1024;
     let mut next_target = SeekTarget::Line(start_line);
     let mut lines_emitted: usize = 0;
     let mut first_chunk = true;
+    let mut lines_since_cancel_check: usize = 0;
+    let mut bytes_since_cancel_check: usize = 0;
 
     loop {
         if cancel.load(Ordering::Relaxed) {
@@ -161,9 +171,15 @@ pub fn read_range(
             break;
         }
 
-        // Compute the byte offset just past this chunk's last line. Each line in the
-        // chunk does NOT include a trailing newline (the backend strips them), so we
-        // add 1 per line for the delimiter. This is exact byte arithmetic.
+        // Compute the byte offset just past this chunk's last line.
+        //
+        // CRLF assumption: line readers in all three backends keep the `\r` AS PART of
+        // the line string (they only split on `\n`; `&data[pos..pos + nl_pos]` retains
+        // bytes before the newline byte). So `line.len()` already includes the `\r`
+        // for CRLF files, and the `+ 1` accounts for the single `\n` delimiter byte.
+        // No drift on either LF or CRLF files. See `byte_seek.rs:118`,
+        // `full_load.rs:43`, `line_index.rs:172` for the parallel patterns. Test
+        // fixture: `read_range_full_load_crlf_*` in `session_test.rs`.
         let mut chunk_end_offset = chunk.byte_offset;
         for line in &chunk.lines {
             chunk_end_offset += line.len() as u64 + 1;
@@ -172,6 +188,17 @@ pub fn read_range(
         let first_line_idx_in_chunk = chunk.first_line_number;
 
         for (i, line) in chunk.lines.iter().enumerate() {
+            // Check the cancel flag periodically inside the inner loop. Doing it only
+            // between chunks meant a single 4096-line chunk of 4 KB/line files (16 MB)
+            // was uninterruptible. Now Escape lands within ~64 KB of emitted output.
+            if lines_since_cancel_check >= CANCEL_CHECK_LINES || bytes_since_cancel_check >= CANCEL_CHECK_BYTES {
+                if cancel.load(Ordering::Relaxed) {
+                    return Err(ViewerError::Cancelled);
+                }
+                lines_since_cancel_check = 0;
+                bytes_since_cancel_check = 0;
+            }
+
             let line_number = first_line_idx_in_chunk + i;
             let is_first_overall = first_chunk && i == 0;
 
@@ -180,6 +207,7 @@ pub fn read_range(
                 return Ok(out);
             }
 
+            let bytes_before = out.len();
             if is_first_overall {
                 // First line of the whole selection: take from start_offset to end of line.
                 let start_byte = clamp_utf16_offset_to_byte(line, start_offset_utf16);
@@ -196,6 +224,8 @@ pub fn read_range(
                 out.push('\n');
             }
             lines_emitted += 1;
+            lines_since_cancel_check += 1;
+            bytes_since_cancel_check += out.len() - bytes_before;
         }
 
         first_chunk = false;

apps/desktop/src-tauri/src/file_viewer/session_test.rs

@@ -539,37 +539,92 @@ fn read_range_byte_seek_eof_selects_whole_file() {
 
 #[test]
 fn read_range_cancellation_returns_cancelled() {
-    let dir = create_test_dir("range_cancel");
-    // Build a big file so the read takes long enough to cancel.
-    let line_str = "x".repeat(1000) + "\n";
-    let line_count = (FULL_LOAD_THRESHOLD as usize / line_str.len()) + 1000;
-    let content: String = line_str.repeat(line_count);
+    use std::sync::atomic::AtomicBool;
+
+    // Direct test of the inner-loop cancel check: bypass the session and drive the
+    // pure `read_range` against a `FullLoad` backend, flipping the cancel flag BEFORE
+    // the call so the result is deterministic (no thread race). A file with ~512
+    // lines is plenty to exceed `CANCEL_CHECK_LINES = 256`, guaranteeing the in-loop
+    // check fires at least once.
+    use super::full_load::FullLoadBackend;
+    use super::range_read::read_range;
+
+    let line_str = "x".repeat(64) + "\n"; // 65 bytes per line.
+    let content: String = line_str.repeat(512);
+    let backend = FullLoadBackend::from_content(&content, "cancel.txt");
+
+    let cancel = AtomicBool::new(true);
+    let result = read_range(&backend, line(0, 0), RangeEnd::Eof, &cancel);
+
+    assert!(
+        matches!(result, Err(ViewerError::Cancelled)),
+        "expected ViewerError::Cancelled, got: {:?}",
+        result
+    );
+}
+
+#[test]
+fn read_range_session_cancellation_returns_cancelled_and_cleans_up() {
+    // End-to-end through the session: use a file big enough that the read can't
+    // complete before the canceller fires. 1 MB + 1024 lines pushes us into ByteSeek
+    // mode and the read takes long enough to interrupt deterministically.
+    let dir = create_test_dir("range_session_cancel");
+    // ~10 MB file with 4 KB lines: 4096 lines = ~16 MB → ByteSeek mode, and the read
+    // needs many fetch chunks. The in-loop check fires hundreds of times.
+    let line_str = "z".repeat(4096) + "\n";
+    let content: String = line_str.repeat(4096);
     let file = write_test_file(&dir, "big.txt", &content);
-    let open = session::open_session(file.to_str().unwrap()).unwrap();
-    let sid_arc = std::sync::Arc::new(open.session_id.clone());
+    let sid = session::open_session(file.to_str().unwrap()).unwrap().session_id;
 
-    // Spawn a thread that cancels after a short delay.
-    let sid_for_cancel = sid_arc.clone();
-    let cancel_handle = thread::spawn(move || {
-        thread::sleep(Duration::from_millis(5));
-        session::cancel_read(&sid_for_cancel, 42)
+    let sid_for_cancel = sid.clone();
+    let canceller = thread::spawn(move || {
+        // 2 ms is enough that the read has started but nowhere near done on a ~16 MB
+        // file. The in-loop check (every 64 KB or 256 lines) fires well before the
+        // read completes.
+        thread::sleep(Duration::from_millis(2));
+        session::cancel_read(&sid_for_cancel, 42).unwrap();
     });
 
-    // Read the whole file from another thread; expect Cancelled.
-    let result = session::read_range(&sid_arc, 42, line(0, 0), RangeEnd::Eof);
-    let _ = cancel_handle.join();
-
-    // The result is racy: if the read completed before cancel landed, we get Ok.
-    // To make the test deterministic, just assert that EITHER it returned Cancelled
-    // OR it returned Ok (didn't blow up). On at least one run we expect Cancelled.
-    // The important invariant: active_reads is empty after the call returns.
-    match result {
-        Ok(_) | Err(ViewerError::Cancelled) => {}
-        Err(other) => panic!("unexpected error: {:?}", other),
-    }
-    assert_eq!(session::active_read_count(&sid_arc), 0);
+    let result = session::read_range(&sid, 42, line(0, 0), RangeEnd::Eof);
+    let _ = canceller.join();
+
+    assert!(
+        matches!(result, Err(ViewerError::Cancelled)),
+        "expected ViewerError::Cancelled, got: {:?}",
+        result.map(|s| format!("Ok({} bytes)", s.len()))
+    );
+    assert_eq!(session::active_read_count(&sid), 0);
+
+    session::close_session(&sid).unwrap();
+    cleanup(&dir);
+}
 
-    session::close_session(&sid_arc).unwrap();
+#[test]
+fn read_range_full_load_crlf_preserves_carriage_returns() {
+    // CRLF files: the backend keeps the `\r` AS PART of the line text (it only splits
+    // on `\n`). So `read_range` returns lines with their `\r` intact, and the byte-
+    // offset arithmetic (`line.len() + 1`) correctly accounts for both bytes. This
+    // test pins both behaviours so a future "auto-strip \r" change doesn't silently
+    // drift `byte_offset`.
+    let dir = create_test_dir("range_crlf");
+    let content = "alpha\r\nbeta\r\ngamma\r\n";
+    let file = write_test_file(&dir, "crlf.txt", content);
+    let sid = session::open_session(file.to_str().unwrap()).unwrap().session_id;
+
+    // ⌘A-equivalent: read everything. The backend keeps `\r` as part of each line;
+    // `range_read` rejoins with `\n` between lines and trims exactly one trailing
+    // newline at EOF (the same half-open behaviour the LF test asserts). Net: the
+    // original CRLF bytes round-trip exactly.
+    let out = session::read_range(&sid, 1, line(0, 0), RangeEnd::Eof).unwrap();
+    assert_eq!(out, "alpha\r\nbeta\r\ngamma\r\n");
+
+    // Multi-line slice: from (0, 2) to (1, 3). On line 0 the text after offset 2 is
+    // "pha\r" (offset 2 in UTF-16 lands on byte 2 of "alpha\r"). Then the joining
+    // `\n`. Then on line 1 from offset 0 to 3 = "bet".
+    let slice = session::read_range(&sid, 2, line(0, 2), line(1, 3)).unwrap();
+    assert_eq!(slice, "pha\r\nbet");
+
+    session::close_session(&sid).unwrap();
     cleanup(&dir);
 }