fix: account for canister history memory usage by mraszyk · Pull Request #10825 · dfinity/ic

mraszyk · 2026-07-20T08:29:10Z

Recording a canister history entry previously bypassed the subnet's memory and cycles accounting: add_canister_change applied the resulting execution-memory delta to the subnet available memory (letting it go negative), and it neither reserved storage cycles nor counted towards the freezing threshold. This PR makes canister history memory be accounted for like any other canister memory.

Additionally,

the subnet memory usage invariant is tightened since canister history usage is now properly accounted towards subnet available memory;
creating a canister with a low initial cycles balance in tests requires setting freezing threshold to 0 so that the canister history entry for canister creation does not make the canister frozen (which would fail the canister creation itself).

…history `CanisterState::add_canister_change` previously recorded a canister history entry and returned the resulting execution-memory delta, which callers applied to the subnet available memory via `update_execution_memory_unchecked` (letting it go negative). This meant recording canister history never respected the subnet available execution memory. Introduce a checked `CanisterState::add_canister_change` that takes the subnet available memory, applies the delta, and fails with `InsufficientMemory` when the subnet cannot account for the additional canister history. The previous delta-returning behavior is preserved as `add_canister_change_unchecked`, used by `install_code` where the delta is accumulated and applied (checked) at commit time. All management endpoints except `rename_canister` are switched to the checked API: they operate on a clone of the `CanisterState` (and, via `execute_mgmt_operation_on_canister` or an internal round-limits snapshot, on restorable round limits), so failing after the change is recorded discards it. `rename_canister` is handled separately. `uninstall_code` gets a special case for the NNS governance canister, which can forcefully uninstall any canister: if the subnet cannot account for the `CanisterCodeUninstall` history entry, the canister history is dropped instead (as when a canister runs out of cycles) and the freed memory is returned to the subnet available execution memory, so the uninstall still succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR tightens execution-memory accounting by making canister-history recording respect subnet available execution memory, turning what was previously an “unchecked, can go negative” accounting path into a checked/fallible one for most management operations.

Changes:

Split CanisterState::add_canister_change into a new checked API (charges SubnetAvailableMemory and can fail) plus an add_canister_change_unchecked variant that returns a delta for deferred/commit-time charging.
Switch most canister management operations to use the checked API and map SubnetAvailableMemoryError to CanisterManagerError, with a special-case for NNS governance uninstall to drop history if memory is exhausted.
Update and add tests to account for canister-history charging against subnet memory and to validate uninstall behavior under memory exhaustion.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
rs/replicated_state/src/canister_state.rs	Introduces checked `add_canister_change` API and preserves old delta-returning behavior as `add_canister_change_unchecked`.
rs/execution_environment/tests/memory_matrix.rs	Separates “allocated bytes” from “subnet-charged allocated bytes” to include canister history in subnet-memory capacity assertions.
rs/execution_environment/src/scheduler/tests/charging.rs	Adds regression test ensuring out-of-cycles uninstall succeeds even with zero subnet available execution memory (history dropped).
rs/execution_environment/src/execution/install_code.rs	Switches install-code path to use unchecked canister-history recording with deferred accounting.
rs/execution_environment/src/canister_manager/tests.rs	Adds tests for uninstall failure when subnet can’t account for history, and governance override behavior that clears history.
rs/execution_environment/src/canister_manager.rs	Switches multiple operations to checked history recording; adds governance uninstall fallback and error mapping helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Address review feedback on the tests for failing when the subnet cannot account for canister history: - Fix `freed` in the uninstall tests: it now measures the memory that `uninstall_canister` actually credits back (execution state + Wasm chunk store) instead of `memory_allocated_bytes - history`, which was dominated by log memory that `clear_log` keeps allocated. This makes the "one byte too low" setup accurate and lets us assert `uninstall_change_bytes > freed`. - Assert `total_num_changes` behavior: unchanged on the atomic failure, bumped by one when governance drops the history and when the uninstall succeeds, and unchanged across the out-of-cycles uninstall (no `CanisterCodeUninstall` change recorded). - Move `uninstall_code_by_governance_clears_history_when_subnet_memory_exhausted` next to `uninstall_code_fails_if_subnet_cannot_account_for_canister_history` and align its structure. - Add `create_canister_fails_if_subnet_cannot_account_for_canister_history`: round limits are restored and no canister is inserted on failure. - Assert the succeeding uninstall drops canister memory usage by `freed - uninstall_change_bytes`. - Drop the manually recorded canister-history change in the out-of-cycles test and clarify comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ry_usage_checks_and_updates Unify canister history accounting so that recording a canister change is always routed through `cycles_and_memory_usage_checks_and_updates`: - `CanisterState::add_canister_change` now simply records the change and returns the resulting `SubnetAvailableExecutionMemoryChange`; the checked variant (taking the subnet available memory) and the separate `add_canister_change_unchecked` are removed. - `cycles_and_memory_usage_checks_and_updates` takes an `Option<SubnetAvailableExecutionMemoryChange>` and folds it into the new execution memory used, so the recorded canister history is accounted for in the subnet available execution memory, the freezing threshold, and the storage reservation. - All management endpoints that record canister history (`update_settings`, `uninstall_code`, `create_canister`, `take_canister_snapshot`, `load_canister_snapshot`) record the change before calling the checks function and pass the delta to it; `uninstall_code` no longer updates the subnet available memory itself. - The NNS governance special case in `uninstall_code` is dropped: governance-initiated uninstall now fails uniformly if the subnet cannot account for the additional canister history entry. Because recording canister history now requires the canister to be solvent (and reserves storage cycles), tests creating/updating under-funded or frozen canisters are updated accordingly, including the shared `StateMachine::create_canister` helper (which defaults the freezing threshold to zero) and the `memory_matrix` suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…unting - Normalize the "accounted for via the `canister_history_change` argument" comment across all call sites and reword the folding/freezing comments in `cycles_and_memory_usage_checks_and_updates`. - Drop the now-unused `round_limits` parameter from `uninstall_canister`. - Reword the zero-freezing-threshold test comments and rename `history_before` to `history_memory_usage_before`. - Rewrite the create-canister subnet-memory-accounting tests to use `ExecutionTest` (with a zero log memory limit) as a succeeds/one-byte-below- fails pair, matching the uninstall tests. - Keep the subnet memory threshold at 0 in `create_canister_can_set_reserved_cycles_limit`, raising the reserved cycles limit so the `canister_creation` reservation fits. - In the memory matrix test, account for canister history uniformly (actual memory usage including history) and keep a single "without history" quantity for scenario classification; fix the stale comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

In the memory matrix test, only use the `_without_history` quantities in assertions (scenario classification), never in control flow. Gate the freezing-threshold invariant and the reserved-cycles/freezing sub-tests on the actual (with-history) allocated bytes instead, and drop the now-unused `RunResult::allocated_bytes_without_history` field. This re-enables the `FreezingThreshold::Short` invariant for operations that allocate only a small canister-history entry (e.g. `take_snapshot_and_uninstall`), where the cycles reserved for those few bytes are dominated by the freezing limit based on the pre-existing memory usage. Extend `skip_short_invariant` declaratively to cover `OtherManagement` + `MemoryUsageChange::None`, matching the existing `IncreaseLogAndMemoryAllocation` + `Large` exception. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Reword the comments at the `settings_change`, `controllers_change`, and `canister_creation` call sites of `cycles_and_memory_usage_checks_and_updates` to spell out that `validate_and_update_canister_settings` has already applied the settings and run the cycles-and-memory-usage checks for the settings-driven memory change. Recording the canister history entry is therefore the only memory change past that point, justifying why the old and new memory usage passed to the call are equal (the entry itself being accounted for via the `canister_history_change` argument). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…emory once `take_canister_snapshot` previously called `cycles_and_memory_usage_checks_and_updates` twice: once (before the uninstall) for the snapshot allocation and instruction cost, and once (after the uninstall) for the `CanisterCodeUninstall` canister history entry. Collapse this into a single call at the end of the operation: - Take the snapshot and (optionally) uninstall the code first, then read `new_memory_usage` from the actual canister state (reflecting the snapshot and the uninstall) before recording the history entry, folding the entry in via the `canister_history_change` argument. This drops the hand-computed `uninstalled_canister_size`/`replace_snapshot_size` memory estimates in favor of the real post-mutation memory usage. Any partial state mutations are rolled back by the caller (`execute_mgmt_operation_on_canister`) if the check fails. As a result, the freezing threshold guarding the snapshot instruction charge now consistently reflects the canister's true final memory usage (including the uninstall history entry) instead of a pre-history value that only differed due to call ordering. At the exact affordability boundary this shifts which resource check trips first, so `take_snapshot_and_uninstall` can now fail with `CanisterOutOfCycles` instead of `InsufficientCyclesInMemoryGrow`. Update `test_minimum_cycles_balance` to accept either error for `OtherManagement`, matching the `Scenario::OtherManagement` arm already present in the sibling `test_freezing_threshold`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… memory usage Previously `cycles_and_memory_usage_checks_and_updates` received the memory usage *excluding* the canister history recorded by the operation, plus the history's allocated-bytes delta, and folded the latter into the former before applying `allocated_bytes()`. When the recorded history entry crossed the canister's memory allocation boundary, this double-counted the allocation and under-reserved storage cycles (reserving for roughly half the truly allocated bytes). Fix the fold by passing `new_memory_usage` *including* any recorded canister history: every caller now reads it after `add_canister_change`, so the subnet available memory, freezing threshold, and storage reservation are all computed once over the true total memory usage. The `canister_history_change` parameter and the fold are removed; `add_canister_change`'s return value is still consumed by the direct subnet-memory paths (`rename_canister`, `install_code`). In the memory matrix test, make the `Scenario` `memory_usage_change` reflect the overall change including canister history: drop the `_without_history` quantities, assert against the with-history memory usage/allocated bytes, reclassify `take_snapshot_and_uninstall` as `Increase` (its only net change is the `CanisterCodeUninstall` history entry), lower the `CrossedDuringTest` offset below one canister history entry so such tiny changes still cross, and skip the short-threshold freezing invariant whenever the operation's only new allocation is a canister history entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…story Add a memory matrix scenario that uninstalls a canister with a minimal execution state (an empty Wasm module, no chunks), so the memory freed by the uninstall is smaller than the `CanisterCodeUninstall` canister history entry it records. The canister's memory usage therefore increases overall, exercising the subnet-memory and storage-reservation accounting for canister history recorded by an uninstall (the ad-hoc `uninstall_code_{fails,succeeds}_if_..._canister_history` unit tests covered this only for a single configuration). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…red by matrix The `uninstall_code_fails_if_subnet_cannot_account_for_canister_history` and `uninstall_code_succeeds_if_freed_execution_state_covers_canister_history` unit tests are now covered by the memory matrix suite: - the "fails" case by `test_memory_suite_uninstall_code_below_canister_history` (minimal execution state, freed < history entry) via `test_subnet_memory_capacity`, which asserts the uninstall fails with `SubnetOversubscribed` one byte below capacity; - the "succeeds" case by `test_memory_suite_uninstall_code` (`setup_universal_canister_with_much_memory`, freed >> history entry), whose net deallocation lets `test_subnet_memory_capacity` run the uninstall with the subnet at capacity and assert it succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The returned execution-memory change is no longer passed to `cycles_and_memory_usage_checks_and_updates` (that parameter was removed): management endpoints now account for recorded canister history by including it in the memory usage they pass to that function and discard the returned change, while `rename_canister` and `install_code` still consume it directly. Update the doc comment accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…canister Route `rename_canister` through `cycles_and_memory_usage_checks_and_updates` (like canister creation) so that recording its `rename_canister` history entry is accounted for in the subnet available execution memory and the canister's cycles, failing with `SubnetMemoryCapacityOverSubscribed` when the subnet cannot account for it. The mutations (rename + history entry) and any partial `round_limits` update are rolled back on failure, keeping the rename atomic; the caller then puts the canister back under its unchanged old id. Add unit tests analogous to the canister-creation history tests covering the exactly-enough (succeeds) and one-byte-short (fails atomically) cases. With rename no longer consuming the change returned by `add_canister_change`, its return value is used by no caller (`install_code` now computes the allocated/deallocated delta itself via `memory_allocated_bytes()`), so drop it along with the now-unused `SubnetAvailableExecutionMemoryChange` type and the `update_execution_memory_unchecked` helper, and drop the `let _ =` at the remaining call sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… canister history The end-of-round subnet memory usage invariant used to allow the total canister memory allocated bytes to exceed the subnet memory capacity by the total canister history memory usage, because canister history was not tracked in `SubnetAvailableMemory` during a round. Canister history is now accounted for in `SubnetAvailableMemory` like any other canister memory, so drop that slack and require the total canister memory allocated bytes (canister history included) to not exceed the subnet memory capacity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ment The parenthetical in the `cycles_and_memory_usage_checks_and_updates` doc comment explained the design only by contrast with the old `allocated_bytes()`-folding approach this PR removed, so it justified the change rather than describing the current code. Drop it and keep the substantive sentence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

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

…nTest Drive the rename_canister subnet message end-to-end from the migration canister instead of calling canister_manager.rename_canister directly, so the tests also cover the subnet-message dispatch and response path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

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

The `requested_by` field of `RenameCanisterArgs` records the principal that requested the migration, distinct from the sender (always the migration canister). Use `test.user_id()` to reflect realistic usage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The helper also sets a zero freezing threshold, not just a zero log memory limit. Rename it accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

zeropath-ai · 2026-07-24T06:14:02Z

✅ No security or compliance issues detected. Reviewed everything up to b6b4718.

Security Overview

🔎 Scanned files: 12 changed file(s)
🔗 Scan Link: https://zeropath.com/app/repositories/f5e9d2bb-29a4-4b64-902a-391d65fd8b2d?scanId=fddd514c-fe5a-43c9-a664-1de7d990b2bb&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► rs/execution_environment/src/canister_manager.rs Clone subnet memory saturation when passing to functions ► rs/execution_environment/src/execution/install_code.rs Track and adjust memory allocation changes when recording canister history entries ► rs/execution_environment/src/execution_environment.rs Pass subnet cycles config and resource saturation into management operations ► rs/execution_environment/src/scheduler.rs Adjust uninstall call in end-of-round to not update round limits ► rs/execution_environment/src/scheduler/tests/charging.rs Add test for uninstalling a canister without subnet available memory ► rs/execution_environment/src/execution_environment/tests/canister_task.rs Adjust test to use zero freezing threshold for creation path ► rs/execution_environment/src/execution/install_code.rs Record memory changes for canister history entry within installation ► rs/execution_environment/src/execution/install_code.rs Update memory accounting logic for canister allocation changes ► rs/execution_environment/src/execution/install_code.rs Handle old/new allocated bytes when updating memory usage ► rs/execution_environment/src/execution/install_code.rs Compute and apply memory usage deltas for canister history changes
Bug Fix	► rs/execution_environment/src/canister_manager.rs Fix memory usage accounting around settings and controllers changes ► rs/execution_environment/src/canister_manager.rs Guard memory accounting when uninstalling code and updating memory usage ► rs/execution_environment/src/canister_manager.rs Adjust memory accounting for canister creation, uninstall, and load_snapshot paths ► rs/execution_environment/src/execution_environment.rs Compute and pass resource saturation for memory accounting during operations
Refactor	► rs/execution_environment/src/canister_manager.rs Clarify comments around memory accounting changes and history entries ► rs/execution_environment/src/canister_manager.rs Minor variable name adjustments for clarity in memory accounting ► rs/execution_environment/src/execution/install_code.rs Consolidate memory accounting after canister changes into a single delta calculation
Other	► rs/execution_environment/src/execution/install_code.rs Replace direct subnet memory change updates with aggregated accounting

github-actions Bot added the feat label Jul 20, 2026

mraszyk force-pushed the mraszyk/fail-on-canister-history-memory-usage branch 2 times, most recently from 847e77e to 1c22af4 Compare July 20, 2026 09:17

mraszyk force-pushed the mraszyk/fail-on-canister-history-memory-usage branch from 1c22af4 to de01fb6 Compare July 20, 2026 09:32

mraszyk requested a review from Copilot July 20, 2026 10:26

Copilot started reviewing on behalf of mraszyk July 20, 2026 10:26 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

Comment thread rs/replicated_state/src/canister_state.rs Outdated

mraszyk and others added 11 commits July 20, 2026 12:33

Potential fix for pull request finding

a6383ce

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

mraszyk changed the title ~~feat(execution): fail when subnet memory cannot account for canister history~~ feat(execution): treat canister history memory usage just like any other memory usage Jul 23, 2026

mraszyk changed the title ~~feat(execution): treat canister history memory usage just like any other memory usage~~ feat(execution): account for canister history memory usage Jul 23, 2026

mraszyk requested a review from Copilot July 23, 2026 11:06

Copilot started reviewing on behalf of mraszyk July 23, 2026 11:06 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread rs/execution_environment/src/execution_environment/tests/canister_task.rs Outdated

mraszyk and others added 3 commits July 23, 2026 13:10

Potential fix for pull request finding

89bce9f

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

mraszyk requested a review from Copilot July 23, 2026 12:41

Copilot started reviewing on behalf of mraszyk July 23, 2026 12:41 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread rs/execution_environment/tests/memory_matrix.rs

Comment thread rs/execution_environment/src/canister_manager.rs

mraszyk and others added 2 commits July 23, 2026 20:11

docs(execution): trim redundant comments on memory-usage accounting

c1f69f1

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

mraszyk requested a review from Copilot July 23, 2026 20:36

Copilot started reviewing on behalf of mraszyk July 23, 2026 20:36 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

mraszyk commented Jul 24, 2026

View reviewed changes

Comment thread rs/execution_environment/src/canister_manager/tests.rs

test(execution): rename helper to reflect zero freezing threshold

b6b4718

The helper also sets a zero freezing threshold, not just a zero log memory limit. Rename it accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

mraszyk marked this pull request as ready for review July 24, 2026 06:12

mraszyk requested a review from a team as a code owner July 24, 2026 06:12

github-actions Bot added the @core-protocol label Jul 24, 2026

mraszyk changed the title ~~feat(execution): account for canister history memory usage~~ fix: account for canister history memory usage Jul 28, 2026

github-actions Bot added fix and removed feat labels Jul 28, 2026

schneiderstefan approved these changes Jul 29, 2026

View reviewed changes

Comment thread rs/execution_environment/src/scheduler/tests/charging.rs

mraszyk added this pull request to the merge queue Jul 29, 2026

Merged via the queue into master with commit f0cc13d Jul 29, 2026
42 checks passed

mraszyk deleted the mraszyk/fail-on-canister-history-memory-usage branch July 29, 2026 09:17

Navigation Menu

fix: account for canister history memory usage - #10825

fix: account for canister history memory usage#10825
mraszyk merged 20 commits into
masterfrom
mraszyk/fail-on-canister-history-memory-usage

mraszyk commented Jul 20, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

zeropath-ai Bot commented Jul 24, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mraszyk commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

zeropath-ai Bot commented Jul 24, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mraszyk commented Jul 20, 2026 •

edited

Loading