Keep window manager state consistent on mmap failures. (netdata#21693)

* Fix window-manager state consistency after failed mmap

Invalidate `active_window_idx` before/after window removal to ensure
the index doesn't point to a non-existent window if `create_window`
fails.

Also adds error logging when mmap creation fails, and two separate
tests to ensure the window manager's consistency in such cases.

* Improve error handling at journal file access call-sites

- Stop `EntryDataIterator` on data read errors instead of continuing
- Add warning log when find_log_entries fails for a file
- Properly propagate errors from `get_timestamp_field`, ignoring only
  `MissingFieldName` variant.

Author: vkalintiris

src/crates/journal-core/src/file/file.rs

@@ -1158,7 +1158,11 @@ impl<'a, M: MemoryMap> Iterator for EntryDataIterator<'a, M> {
                 // Try to get the data object
                 match self.journal.data_ref(data_offset) {
                     Ok(data_guard) => Some(Ok(data_guard)),
-                    Err(e) => Some(Err(e)),
+                    Err(e) => {
+                        // If we can't read the data, return the error and stop iteration
+                        self.current_index = self.total_items;
+                        Some(Err(e))
+                    }
                 }
             }
             Err(e) => {

src/crates/journal-core/src/file/mmap.rs

@@ -2,6 +2,7 @@ use crate::error::Result;
 use journal_common::compat::is_multiple_of;
 use std::fs::File;
 use std::ops::{Deref, DerefMut};
+use tracing::error;
 
 // Re-export memmap2 types for other crates and import for internal use
 pub use memmap2::{Mmap, MmapMut, MmapOptions};
@@ -141,7 +142,16 @@ impl<M: MemoryMap> WindowManager<M> {
         debug_assert_ne!(chunk_count, 0);
 
         let size = chunk_count * self.chunk_size;
-        let mmap = M::create(&self.file, window_start, size)?;
+        let mmap = M::create(&self.file, window_start, size).map_err(|e| {
+            error!(
+                window_start,
+                size,
+                chunk_count,
+                chunk_size = self.chunk_size,
+                "mmap failed: {e}"
+            );
+            e
+        })?;
         Ok(Window {
             offset: window_start,
             size,
@@ -197,6 +207,9 @@ impl<M: MemoryMap> WindowManager<M> {
             // Remap the window
 
             let window = self.windows.remove(idx);
+            // Invalidate active_window_idx before removal to maintain consistency.
+            // If create_window fails, the index won't point to a non-existent window.
+            self.active_window_idx = None;
 
             let window_start = window.offset;
             let window_end = self.get_chunk_aligned_end(position + size_needed);
@@ -212,10 +225,11 @@ impl<M: MemoryMap> WindowManager<M> {
 
             if self.windows.len() >= self.max_windows {
                 self.windows.remove(self.find_window_to_evict());
+                // Invalidate active_window_idx after removal to maintain consistency.
+                // If create_window fails below, the index won't point to a non-existent window.
+                self.active_window_idx = None;
             }
 
-            // NOTE: the active window index might have been invalidated. In
-            // the scope that follows, we should not use code that relies on it.
             {
                 // Calculate window start for this position
                 let window_start = self.get_chunk_aligned_start(position);
@@ -250,3 +264,209 @@ impl<M: MemoryMapMut> WindowManager<M> {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::error::JournalError;
+    use std::cell::Cell;
+    use std::io::Write;
+    use std::rc::Rc;
+    use tempfile::NamedTempFile;
+
+    const PAGE_SIZE_TEST: u64 = 4096;
+
+    /// A mock MemoryMap that can be configured to fail on specific calls.
+    /// This allows us to test error handling in WindowManager.
+    struct FailingMmap {
+        data: Vec<u8>,
+    }
+
+    impl Deref for FailingMmap {
+        type Target = [u8];
+        fn deref(&self) -> &[u8] {
+            &self.data
+        }
+    }
+
+    /// Shared state to control when the mock should fail
+    struct MockController {
+        fail_next_create: Cell<bool>,
+        create_count: Cell<usize>,
+    }
+
+    impl MockController {
+        fn new() -> Self {
+            Self {
+                fail_next_create: Cell::new(false),
+                create_count: Cell::new(0),
+            }
+        }
+
+        fn set_fail_next(&self, fail: bool) {
+            self.fail_next_create.set(fail);
+        }
+
+        fn should_fail(&self) -> bool {
+            let count = self.create_count.get();
+            self.create_count.set(count + 1);
+            self.fail_next_create.get()
+        }
+    }
+
+    // Thread-local controller for the mock
+    thread_local! {
+        static MOCK_CONTROLLER: Rc<MockController> = Rc::new(MockController::new());
+    }
+
+    impl MemoryMap for FailingMmap {
+        fn create(_file: &File, _offset: u64, size: u64) -> Result<Self> {
+            MOCK_CONTROLLER.with(|ctrl| {
+                if ctrl.should_fail() {
+                    return Err(JournalError::Io(std::io::Error::new(
+                        std::io::ErrorKind::Other,
+                        "simulated mmap failure",
+                    )));
+                }
+                // Create a mock mmap with zeros
+                Ok(FailingMmap {
+                    data: vec![0u8; size as usize],
+                })
+            })
+        }
+    }
+
+    /// This test verifies that WindowManager maintains consistent state
+    /// after a failed remap operation.
+    ///
+    /// The scenario:
+    /// 1. A window exists at some position
+    /// 2. A request comes in that requires remapping (position in window, but size extends beyond)
+    /// 3. The old window is removed
+    /// 4. Creating the new (larger) window fails (e.g., mmap error)
+    /// 5. The WindowManager should remain in a consistent state
+    /// 6. Subsequent operations should not panic
+    #[test]
+    fn test_consistent_state_after_failed_remap() {
+        // Create a temporary file (content doesn't matter for mock)
+        let mut temp_file = NamedTempFile::new().unwrap();
+        temp_file.write_all(&[0u8; 8192]).unwrap();
+        temp_file.flush().unwrap();
+
+        let file = File::open(temp_file.path()).unwrap();
+
+        // Create WindowManager with mock mmap, 4KB chunks, max 1 window
+        let mut wm: WindowManager<FailingMmap> =
+            WindowManager::new(file, PAGE_SIZE_TEST, 1).unwrap();
+
+        // Reset controller state
+        MOCK_CONTROLLER.with(|ctrl| {
+            ctrl.set_fail_next(false);
+            ctrl.create_count.set(0);
+        });
+
+        // First read: creates a window at offset 0, size 4KB (this should succeed)
+        {
+            let slice = wm.get_slice(0, 100).unwrap();
+            assert_eq!(slice.len(), 100);
+        }
+        assert_eq!(wm.windows.len(), 1);
+        assert_eq!(wm.active_window_idx, Some(0));
+
+        // Configure mock to fail on the next create call
+        MOCK_CONTROLLER.with(|ctrl| ctrl.set_fail_next(true));
+
+        // Request a slice that requires remapping:
+        // - Position 100 is within the existing window [0, 4096)
+        // - But size 4000 means we need bytes [100, 4100), which extends beyond window
+        // - This triggers the "Remap the window" branch
+        // - The old window is removed
+        // - Then create_window is called and FAILS
+        let remap_result = wm.get_slice(100, 4000);
+        assert!(remap_result.is_err(), "Expected remap to fail");
+
+        // Verify state is consistent after the failure:
+        // - windows is empty (the old window was removed, new one failed to create)
+        // - active_window_idx should be None (not pointing to non-existent window)
+        assert_eq!(wm.windows.len(), 0);
+        assert_eq!(wm.active_window_idx, None);
+
+        // Allow the next create to succeed
+        MOCK_CONTROLLER.with(|ctrl| ctrl.set_fail_next(false));
+
+        // The next operation should NOT panic - it should succeed by creating a new window
+        let result = wm.get_slice(0, 100);
+        assert!(
+            result.is_ok(),
+            "Expected get_slice to succeed after recovery"
+        );
+        assert_eq!(wm.windows.len(), 1);
+    }
+
+    /// This test verifies that WindowManager maintains consistent state
+    /// after a failed window creation in the eviction path.
+    ///
+    /// The scenario:
+    /// 1. A window exists and we're at max_windows
+    /// 2. A request comes in for a different region requiring a new window
+    /// 3. The old window is evicted to make room
+    /// 4. Creating the new window fails (e.g., mmap error)
+    /// 5. The WindowManager should remain in a consistent state
+    /// 6. Subsequent operations should not panic
+    #[test]
+    fn test_consistent_state_after_failed_eviction() {
+        // Create a temporary file
+        let mut temp_file = NamedTempFile::new().unwrap();
+        temp_file.write_all(&[0u8; 8192]).unwrap();
+        temp_file.flush().unwrap();
+
+        let file = File::open(temp_file.path()).unwrap();
+
+        // Create WindowManager with mock mmap, 4KB chunks, max 1 window
+        let mut wm: WindowManager<FailingMmap> =
+            WindowManager::new(file, PAGE_SIZE_TEST, 1).unwrap();
+
+        // Reset controller state
+        MOCK_CONTROLLER.with(|ctrl| {
+            ctrl.set_fail_next(false);
+            ctrl.create_count.set(0);
+        });
+
+        // Create first window at offset 0
+        {
+            let _slice = wm.get_slice(0, 100).unwrap();
+        }
+        assert_eq!(wm.windows.len(), 1);
+        assert_eq!(wm.active_window_idx, Some(0));
+
+        // Configure mock to fail on the next create call
+        MOCK_CONTROLLER.with(|ctrl| ctrl.set_fail_next(true));
+
+        // Request a slice at a completely different position (second page)
+        // This triggers:
+        // - lookup_window_by_range returns None (position 4096 not in window [0, 4096))
+        // - lookup_window_by_position returns None
+        // - "Create a brand new window" branch
+        // - Eviction: windows.remove(0) since we're at max_windows
+        // - create_window fails
+        let result = wm.get_slice(4096, 100);
+        assert!(result.is_err(), "Expected mmap to fail");
+
+        // Verify state is consistent after the failure:
+        // - windows is empty (the old window was evicted, new one failed to create)
+        // - active_window_idx should be None (not pointing to non-existent window)
+        assert_eq!(wm.windows.len(), 0);
+        assert_eq!(wm.active_window_idx, None);
+
+        // Allow the next create to succeed
+        MOCK_CONTROLLER.with(|ctrl| ctrl.set_fail_next(false));
+
+        // The next operation should NOT panic - it should succeed by creating a new window
+        let result = wm.get_slice(0, 100);
+        assert!(
+            result.is_ok(),
+            "Expected get_slice to succeed after recovery"
+        );
+        assert_eq!(wm.windows.len(), 1);
+    }
+}

src/crates/journal-engine/src/logs/query.rs

@@ -13,6 +13,7 @@ use journal_index::{
 use journal_registry::File;
 use std::collections::HashMap;
 use std::num::NonZeroU64;
+use tracing::warn;
 
 /// Pagination state for multi-file log queries.
 ///
@@ -314,7 +315,10 @@ fn retrieve_log_entries(
 
         let new_entries = match file_index.find_log_entries(file, &file_params) {
             Ok(entries) => entries,
-            Err(_) => continue, // Skip files that fail to read
+            Err(e) => {
+                warn!(file = file.path(), "failed to retrieve log entries: {e}");
+                continue;
+            }
         };
 
         if new_entries.is_empty() {

src/crates/journal-index/src/file_index.rs

@@ -410,8 +410,12 @@ fn get_entry_timestamp(
 ) -> Result<u64> {
     // Try to read the source timestamp field if specified
     if let Some(field_name) = source_timestamp_field {
-        if let Ok(timestamp) = get_timestamp_field(journal_file, field_name, entry_offset) {
-            return Ok(timestamp);
+        match get_timestamp_field(journal_file, field_name, entry_offset) {
+            Ok(timestamp) => return Ok(timestamp),
+            Err(IndexError::MissingFieldName) => {
+                // Field not found, fall back to realtime timestamp
+            }
+            Err(e) => return Err(e),
         }
     }