Skip to content

fix(replicated_state): truncate canister log records when log capacity shrinks - #11396

Open
mraszyk wants to merge 1 commit into
masterfrom
mraszyk/fix-canister-log-truncate-on-capacity-shrink
Open

fix(replicated_state): truncate canister log records when log capacity shrinks#11396
mraszyk wants to merge 1 commit into
masterfrom
mraszyk/fix-canister-log-truncate-on-capacity-shrink

Conversation

@mraszyk

@mraszyk mraszyk commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

The "a single record fits the ring buffer" invariant was established only when a record was created: CanisterLog::add_record truncates the content against the log memory store's byte capacity at that moment. Nothing re-established it when the capacity was later lowered, so RingBuffer::append_log hit

debug_assert!(false, "Log record size exceeds ring buffer capacity");
continue;

which aborts every debug-assertions build (cargo test, bazel test, a debug replica) and silently drops the record otherwise.

Both outcomes are reachable from ordinary, unprivileged update_settings traffic that lowers log_memory_limit: LogMemoryStore::resize_impl migrates the already stored records into a newly allocated, smaller ring buffer, where a record created against the old capacity no longer fits.

LogRecord::truncate_to_capacity now re-establishes the invariant at append time, applying the same policy add_record applies at creation time — truncating the record rather than dropping it. This makes the capacity check unconditionally satisfied for both callers of append_log (resize_impl and append_delta_log), so the assertion and its silent-drop fallback are removed.

A zero log_memory_limit deallocates the buffer instead of creating a zero-capacity one, and every live buffer has data_capacity of at least DATA_CAPACITY_MIN, so a truncated record always fits.

The delta log of an in-flight execution cannot carry a stale, larger capacity into append_delta_log: update_settings never runs on a canister with a paused execution (can_execute_subnet_msg returns false for ExecutionTask::PausedExecution), and an aborted execution re-executes from scratch against the new capacity.

The regression test in canister_logging.rs stores a record against a 64 KiB log memory limit and then lowers the limit below the size of that single record; it fails on the assertion above without this fix.

🤖 Generated with Claude Code

…y shrinks

The "a single record fits the ring buffer" invariant was established only
when a record was created: `CanisterLog::add_record` truncates the content
against the log memory store's byte capacity at that moment. Nothing
re-established it when the capacity was later lowered, so
`RingBuffer::append_log` hit

    debug_assert!(false, "Log record size exceeds ring buffer capacity");
    continue;

which aborts every debug-assertions build (`cargo test`, `bazel test`, a
debug replica) and silently drops the record otherwise.

Both outcomes are reachable from ordinary, unprivileged `update_settings`
traffic that lowers `log_memory_limit`: `LogMemoryStore::resize_impl`
migrates the already stored records into a newly allocated, smaller ring
buffer, where a record created against the old capacity no longer fits.

`LogRecord::truncate_to_capacity` now re-establishes the invariant at
append time, applying the same policy `add_record` applies at creation
time — truncating the record rather than dropping it. This makes the
capacity check unconditionally satisfied for both callers of `append_log`
(`resize_impl` and `append_delta_log`), so the assertion and its
silent-drop fallback are removed.

A zero `log_memory_limit` deallocates the buffer instead of creating a
zero-capacity one, and every live buffer has `data_capacity` of at least
`DATA_CAPACITY_MIN`, so a truncated record always fits.

The delta log of an in-flight execution cannot carry a stale, larger
capacity into `append_delta_log`: `update_settings` never runs on a
canister with a paused execution, and an aborted execution re-executes
from scratch against the new capacity.

The regression test in `canister_logging.rs` stores a record against a 64 KiB
log memory limit and then lowers the limit below the size of that single
record; it fails on the assertion above without this fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes canister log migration when reducing ring-buffer capacity by truncating oversized records instead of asserting or dropping them.

Changes:

  • Adds capacity-aware log-record truncation.
  • Applies truncation during ring-buffer appends.
  • Adds an end-to-end resize regression test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
ring_buffer.rs Truncates records before appending.
log_record.rs Implements serialized-record truncation.
canister_logging.rs Tests shrinking below one record’s size.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mraszyk
mraszyk marked this pull request as ready for review September 1, 2026 14:51
@mraszyk
mraszyk requested a review from a team as a code owner September 1, 2026 14:51
@zeropath-ai

zeropath-ai Bot commented Sep 1, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to e2630d0.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► rs/execution_environment/tests/canister_logging.rs
    test_canister_resize_down_below_record_size_truncates_logs()
► rs/replicated_state/src/canister_state/system_state/log_memory_store/log_record.rs
    Add truncate_to_capacity method and documentation
► rs/replicated_state/src/canister_state/system_state/log_memory_store/ring_buffer.rs
    Invoke truncate_to_capacity during log processing to re-establish capacity constraints

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants