diff --git a/rs/execution_environment/tests/canister_logging.rs b/rs/execution_environment/tests/canister_logging.rs index 8c68e960e710..b7bd2d55d065 100644 --- a/rs/execution_environment/tests/canister_logging.rs +++ b/rs/execution_environment/tests/canister_logging.rs @@ -2254,6 +2254,57 @@ fn test_canister_resize_down_preserves_logs() { assert_eq!(logs_before, logs_after); } +#[test] +fn test_canister_resize_down_below_record_size_truncates_logs() { + let log_memory_limit = TEST_DEFAULT_LOG_MEMORY_LIMIT as u64; + // The largest log content that still fits a log memory store of that size. + let max_content_len = log_memory_limit as usize - LogMemoryStore::estimate_record_size(0); + + let env = setup_env(); + let controller = PrincipalId::new_anonymous(); + let canister_id = create_and_install_canister( + &env, + CanisterSettingsArgsBuilder::new() + .with_controllers(vec![controller]) + .with_log_memory_limit(64 * KIB) + .with_log_visibility(LogVisibilityV2::Public) + .build(), + UNIVERSAL_CANISTER_WASM.to_vec(), + ); + + // Store records that do not fit the smallest accepted log memory limit. + let message = [b'a'; MAX_LOG_MESSAGE_LEN]; + for _ in 0..2 { + let _ = env.execute_ingress( + canister_id, + "update", + wasm().debug_print(&message).reply().build(), + ); + } + let logs_before = fetch_log_records(&env, controller, canister_id); + assert_eq!(logs_before.len(), 2); + assert_gt!(logs_before[0].content.len(), max_content_len); + + // Shrinking the limit below a single stored record makes `LogMemoryStore::resize_impl` + // migrate the records into a buffer they no longer fit. They must be truncated. + let _ = env.update_settings( + &canister_id, + CanisterSettingsArgsBuilder::new() + .with_log_memory_limit(log_memory_limit) + .build(), + ); + + // The records must be truncated to fit the smaller buffer, not dropped. + let logs_after = fetch_log_records(&env, controller, canister_id); + assert!( + !logs_after.is_empty(), + "records were dropped instead of truncated" + ); + for record in &logs_after { + assert_le!(record.content.len(), max_content_len); + } +} + #[test] fn test_canister_log_resize_deducts_cycles() { let log_memory_limit = 2 * MIB; diff --git a/rs/replicated_state/src/canister_state/system_state/log_memory_store/log_record.rs b/rs/replicated_state/src/canister_state/system_state/log_memory_store/log_record.rs index c6925281626e..457934165cd1 100644 --- a/rs/replicated_state/src/canister_state/system_state/log_memory_store/log_record.rs +++ b/rs/replicated_state/src/canister_state/system_state/log_memory_store/log_record.rs @@ -24,6 +24,22 @@ impl LogRecord { 8 + 8 + 4 + content_len } + /// Truncates the content so that the whole record fits within `data_capacity`. + /// + /// The "a single record fits the ring buffer" invariant is established when the + /// record is *created*: `CanisterLog::add_record` truncates the content against the + /// store's byte capacity at that moment. Nothing re-establishes it when the capacity + /// is lowered afterwards, which is what `update_settings` does when it shrinks + /// `log_memory_limit`: `LogMemoryStore::resize_impl` then migrates the already + /// stored records into a smaller ring buffer they no longer fit. + pub fn truncate_to_capacity(&mut self, data_capacity: usize) { + let max_content_len = data_capacity.saturating_sub(Self::estimate_bytes_len(0)); + if self.content.len() > max_content_len { + self.content.truncate(max_content_len); + self.len = max_content_len as u32; + } + } + pub fn matches(&self, filter: &FetchCanisterLogsFilter) -> bool { match filter { FetchCanisterLogsFilter::ByIdx(r) => r.start <= self.idx && self.idx < r.end, diff --git a/rs/replicated_state/src/canister_state/system_state/log_memory_store/ring_buffer.rs b/rs/replicated_state/src/canister_state/system_state/log_memory_store/ring_buffer.rs index 65197ca09d13..2438425dc7fd 100644 --- a/rs/replicated_state/src/canister_state/system_state/log_memory_store/ring_buffer.rs +++ b/rs/replicated_state/src/canister_state/system_state/log_memory_store/ring_buffer.rs @@ -149,7 +149,7 @@ impl RingBuffer { } let mut index_table = self.io.load_index_table(); let mut h = self.io.load_header(); - for record in iter.map(LogRecord::from) { + for mut record in iter.map(LogRecord::from) { // Check that records are added in order, otherwise it breaks the index. if record.idx < h.next_idx { debug_assert!( @@ -168,11 +168,14 @@ impl RingBuffer { continue; } + // A single record must fit the ring buffer. That holds when the record is + // created, but not once the capacity has been lowered underneath it (see + // `LogRecord::truncate_to_capacity`), so re-establish it here — truncating + // the record rather than dropping it, which is what `add_record` does when + // the same limit is applied at creation time. + record.truncate_to_capacity(h.data_capacity.get() as usize); + let added_size = MemorySize::new(record.bytes_len() as u64); - if added_size > h.data_capacity { - debug_assert!(false, "Log record size exceeds ring buffer capacity"); - continue; - } self.evict_for_size(&mut h, added_size); // Save the record at the tail position.