feat: charge cycles for log memory resize in update_settings#9883
Merged
feat: charge cycles for log memory resize in update_settings#9883
Conversation
Rename fetch_canister_logs to fetch_canister_logs_query to clarify it uses the query call path, distinguishing it from the replicated inter-canister call path.
When update_settings changes log_memory_limit, the log memory store rebuilds its ring buffer by reading and rewriting all stored records. This work is now charged proportionally to the amount of stored data (bytes_used), converted to cycles via management_canister_cost. Implementation follows a two-step pattern: 1. validate_canister_settings: pre-check that the canister can afford the resize cost without dropping below the freezing threshold 2. update_settings: deduct cycles and account instructions in the round budget after the resize completes Only triggers when the user explicitly sets log_memory_limit, not when the default is applied during canister creation. Add LogResizeNotEnoughCycles error variant, inter-canister log fetch test helpers, expanded resize preservation tests, and two new tests verifying cycle deduction and rejection on insufficient balance.
maksymar
force-pushed
the
maksym/log-resize-charging
branch
from
dcabf57 to
db6f7ce
Compare
Change LogResizeNotEnoughCycles from a struct variant to a tuple wrapping CanisterOutOfCyclesError, matching the CanisterSnapshotNotEnoughCycles pattern. Use InsufficientCyclesInMemoryAllocation for the pre-check in validate_canister_settings (which has no canister_id), and the clean .map_err(CanisterManagerError::LogResizeNotEnoughCycles) for the deduction step in update_settings.
Clean up existing resize tests to separate concerns: - Remove balance assertions from preserve tests (charging is tested separately) Add three new tests: - test_canister_log_resize_empty_buffer_minimal_charge: verifies near-zero cost when buffer has no records - test_canister_log_resize_no_charge_without_limit: verifies no resize charge when update_settings doesn't set log_memory_limit - test_canister_log_resize_no_extra_charge_feature_disabled: verifies no resize charge when the log memory store feature is disabled
…MemoryAllocation Use the existing InsufficientCyclesInMemoryAllocation error for both the validation pre-check and the deduction step. This keeps the error uniform with other memory-related settings failures and avoids adding a new error variant.
Replace 0-byte messages with 100-byte messages across all resize tests for realistic log record sizes. Use repeat counts that fill the buffer in a single ingress call (20K for 2 MiB, 3K for 256 KiB). Remove intermediate variables and multi-call fill loops. Replace magic cycles constant with a named fetch_cycles variable. Remove unused LogMemoryStore import.
Replace hardcoded repeat counts (20_000, 3_000) with records_to_fill() helper that calculates the count from the buffer size and LogMemoryStore::estimate_record_size(). This keeps the fill logic correct if record overhead changes.
The test could not reliably detect the user_set_log_memory_limit guard removal because the resize charge after subnet scaling is too small relative to the base update_settings cost. The guard is simple code verified by review, and the remaining tests (deducts_cycles, rejected, empty_buffer) cover all charging logic.
Clarify what the resize operation does and add a TODO about moving the constant into CyclesAccountManagerConfig.
Replace misuse of InsufficientCyclesInMemoryAllocation for log resize cycle checks with a dedicated error variant that reports accurate context (available/threshold/requested cycles instead of memory allocation).
- Assert error code and message in rejection test instead of just checking is_err(). - Increase buffer size to 2 MiB in deduction test to maximize the observable cost.
Extract log_resize_cost_per_byte variable to clarify the constant's origin and link it back to LOG_RESIZE_COST_PER_BYTE in canister_manager.
Use consume_with_threshold with zero threshold instead of consume_cycles_for_management_canister_instructions to prevent the cycle deduction from failing after settings have already been mutated. The validation in validate_canister_settings already confirms affordability, so the post-mutation deduction is safe.
Group the limit check and cycle affordability check under the user-set branch, and move the default for canister creation into an explicit else branch with a comment explaining the no-op behavior for existing canisters.
Capture bytes_used before do_update_settings so that downsize operations are charged for the actual work of reading/rewriting the original buffer, not the reduced post-resize size.
The log-resize check validated against the pre-mutation main balance, but do_update_settings() calls reserve_cycles() before the resize charge is deducted. This could cause the consume_with_threshold() expect to panic when reservation_cycles + threshold + log_resize_cycles exceeded the balance, even though the individual checks passed.
The resize cost was charged even when resize_impl would do no work (e.g., capacity unchanged or limit set to 0 with an already-empty store). Add LogMemoryStore::would_resize() to mirror the early-return logic and use it to gate both the affordability check and the actual cycle deduction.
Split the conflated feature-disabled and limit-zero branches in would_resize into separate explicit checks. Have resize_impl use would_resize directly as its early-return guard, eliminating duplicated decision logic that could diverge. Add test_would_resize_matches_resize covering all edge cases: unallocated, allocated-empty, allocated-with-data, minimum-capacity clamping, and feature-disabled states.
…ghCycles Remove the dedicated ErrorCode::InsufficientCyclesInLogResize (542) public API variant and instead use an internal LogResizeNotEnoughCycles error that maps to the existing ErrorCode::CanisterOutOfCycles. This avoids a public API change, protobuf additions, and upgrade coordination while still providing a log-resize-specific error message for debuggability.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Since resize_impl delegates to would_resize as its first check (fb8f721), this test only asserts that would_resize agrees with itself. The invariant is structurally guaranteed by the code.
michael-weigelt
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds cycle charging for log memory resize in
update_settings.When
update_settingschangeslog_memory_limit, the log memory store rebuilds its ring buffer by reading and rewriting all stored records. This work is now charged proportionally to stored data.Implementation follows a two-step pattern:
validate_canister_settings: pre-check that the canister can afford the resize cost without dropping below the freezing thresholdupdate_settings: deduct cycles and account instructions in the round budget after the resize completesOnly triggers when the user explicitly sets
log_memory_limitand the resize would actually do work (capacity change), not when the default is applied during canister creation.Changes:
LogResizeNotEnoughCycleserror variant mapped toErrorCode::CanisterOutOfCycleswith a log-resize-specific message (no public API change).would_resize()to predict whether a resize is a no-op, used both for the charge gate and as the early-return guard insideresize_impl.