refactor: account for cycles and memory usage after management operations

[mraszyk]

execute_mgmt_operation_on_canister now runs the cycles and memory usage checks and updates for the memory usage change of a management operation itself, instead of every operation having to call cycles_and_memory_usage_checks_and_updates at the right point with the right arguments. The memory usage before the operation is read off the saved canister state and the memory usage after the operation off the updated canister state, so it structurally includes any canister history the operation recorded — which each operation recording one previously had to remember to account for, and which took five comments to explain at the three call sites moved over here (uninstall_code, take_canister_snapshot and load_canister_snapshot). The operations that keep doing the accounting themselves still do it, and still explain it.

Which of the two does the accounting is spelled out per operation by the new CyclesAndMemoryUsageAccounting, so that it cannot be forgotten. The checks are skipped if the operation changed the canister's memory usage not at all and used no instructions to be charged for there, so that operations doing neither are unaffected by them — in particular, they do not start failing for an already frozen canister (see below for the one place where this changes behaviour).

Operations moved over

Eight of them: uninstall_code, take_canister_snapshot, load_canister_snapshot, upload_chunk, clear_chunk_store, delete_canister_snapshot, create_snapshot_from_metadata and write_snapshot_data.

Five checked before applying their change, projecting the new memory usage; they now apply the change and let the caller read it off the canister.

Three of them also charged for their instructions as part of those checks, i.e. only once the operation had succeeded. take_canister_snapshot and load_canister_snapshot return them in the new CanisterManagerResponse::instructions_to_charge_on_success instead, which execute_mgmt_operation_on_canister charges for together with the memory usage accounting, so that they are still charged for exactly then. create_snapshot_from_metadata charges for them upfront instead — see below.

update_settings keeps doing the checks itself (CyclesAndMemoryUsageAccounting::InOperation), because its freezing threshold check must not be unconditional: it deliberately tolerates the canister becoming frozen by the updated settings, and only fails if the compute or memory allocation increases, so that the freezing threshold can be raised to freeze the canister. A debug_assert! pins that an operation opting out this way returns no instructions_to_charge_on_success, since they would be silently dropped for it.

create_canister and rename_canister are unaffected: their canister is not in ReplicatedState under its final canister id while the operation runs, so they keep calling the checks directly.

One behaviour change: a no-op is no longer rejected for a frozen canister

The operations moved over used to run the checks unconditionally, and the freezing threshold check in cycles_and_memory_usage_checks_and_updates fails for a frozen canister even if the memory usage does not change at all (can_withdraw_cycles_with_threshold with a zero request fails whenever the threshold exceeds the balance). Since the checks are now skipped if the memory usage is unchanged and no instructions are to be charged for, an operation that does not change the memory usage of a frozen canister now succeeds where it used to fail with InsufficientCyclesInMemoryGrow ("cannot grow memory by 0 bytes"). This affects:

• clear_chunk_store of an empty chunk store;
• uninstall_code if it leaves the memory usage unchanged, i.e. of an already uninstalled canister whose canister history is at capacity and evicts an entry of the same size as the CanisterCodeUninstall entry it records.

Rejecting a no-op for insufficient cycles was a bug, and the early return is needed anyway so that the operations newly covered by the wrapper (see below) do not start failing for a frozen canister. The new test clear_chunk_store_of_frozen_canister_with_empty_chunk_store_succeeds pins the new behaviour; it fails on master with InsufficientCyclesInMemoryGrow.

The other operations moved over hit the early return only where master skips the checks anyway: upload_chunk and write_snapshot_data (for a Wasm chunk) grow the memory usage by the chunk size, unless the chunk is already in the store — in which case they return early without inserting it and with no instructions to charge for, and on master that early return sits before the checks, so they are skipped there just as well (a frozen canister still fails at the upfront instruction charge before either). The remaining ones cannot hit it: delete_canister_snapshot always shrinks the memory usage by the snapshot size, which is never zero (a snapshot taken from a canister contains its Wasm module, which the canister must have, and ValidatedSnapshotMetadata rejects a zero Wasm module size for an uploaded snapshot), and take_canister_snapshot and load_canister_snapshot always charge for their baseline instructions. create_snapshot_from_metadata likewise grows the memory usage by the new snapshot's (never zero) size, unless it replaces a snapshot of exactly the same size — in which case the checks are a no-op anyway, since it has already paid for its instructions upfront (see below).

create_snapshot_from_metadata charges upfront

create_snapshot_from_metadata is the one operation of the three whose instructions do not pay for work done at checkpoint time, so there is no reason to defer them to the end: it charges its canister_snapshot_baseline_instructions plus the new snapshot's size upfront, just like upload_chunk does, and records the charge in ConsumedCyclesForInstructions so that it survives the canister state rollback on failure. Its instructions_to_charge_on_success is therefore zero, and it takes consumed_cycles in place of the resource_saturation it no longer needs.

The charge sits after the cheap validations (controller, metadata, replace-snapshot access, snapshot limit, heap delta rate limit) and before any work, i.e. exactly where the instruction count used to be computed, so those failures stay free. What changes is that the instructions are now charged even if the memory usage accounting afterwards fails — the same semantics upload_chunk has. Insufficient cycles for the charge itself still surface as CanisterManagerError::NotEnoughCycles, just detected earlier — except for an already frozen canister, whose error code changes: it now fails with NotEnoughCycles (ErrorCode::CanisterOutOfCycles) at the upfront charge, instead of with InsufficientCyclesInMemoryGrow at the freezing threshold check of the memory usage accounting. consume_cycles_for_management_canister_instructions enforces the freezing threshold just like that check does, so such a canister fails either way.

Otherwise no behaviour change

The checks, the charges, and the errors they produce are the same as before, in the same order — just driven from one place:

• reading the new memory usage off the updated canister gives the same value the five projecting operations computed (CanisterSnapshots::{push, remove, insert_chunk} and WasmChunkStore::insert_chunk move memory_usage by exactly the snapshot size resp. the chunk size, clear_chunk_store empties the chunk store whose memory usage the operation used to project away, and ValidatedSnapshotMetadata::snapshot_size_bytes() is what CanisterSnapshot::from_metadata stores as the snapshot's size);
• the sender the wrapper passes is the one the operations passed (CanisterCall::canister_change_origin derives CanisterChangeOrigin from the same principal msg.sender() returns);
• the operations newly covered by the wrapper (start_canister, stop_canister, add_cycles, fetch_canister_logs, read_snapshot_data, and write_snapshot_data other than for a Wasm chunk) do not change the canister's memory usage, so the early return skips the checks for them;
• resource_saturation is still computed from the subnet available memory before the operation, as it was at every call site it replaces.

Tests

Three tests are added to pin the behaviour of the operations whose accounting moved the furthest. All three pass before and after, by design:

• take_canister_snapshot_of_canister_without_wasm_module_is_free — the validation rejects before anything is charged.
• take_canister_snapshot_of_frozen_canister_fails_for_free — InsufficientCyclesInMemoryGrow, nothing charged, no snapshot.
• failed_take_canister_snapshot_does_not_charge_for_instructions — with the subnet available execution memory exhausted, the accounting fails; asserts that no snapshot was taken, that the subnet available memory is unchanged, and that the canister is charged nothing, i.e. that the snapshot instructions are still only charged for once the operation succeeded.

A fourth test pins the behaviour change above; it fails before and passes after:

• clear_chunk_store_of_frozen_canister_with_empty_chunk_store_succeeds — clearing the empty chunk store of a frozen canister succeeds, charges nothing, and leaves the chunk store empty.

🤖 Generated with Claude Code

[Copilot]

🔵 Needs a closer look

The broad accounting refactor warrants final human validation.

Pull request overview

This pull request centralizes cycles and memory accounting for management operations while preserving charging behavior.

Changes:

• Adds centralized post-operation accounting.
• Defers applicable instruction charges until success.
• Updates response types and adds regression tests.

File summaries

File	Changes
rs/execution_environment/src/execution_environment.rs	Centralizes management-operation accounting.
rs/execution_environment/src/canister_manager/types.rs	Adds deferred instruction metadata.
rs/execution_environment/src/canister_manager/tests.rs	Adds accounting and rollback tests.
rs/execution_environment/src/canister_manager.rs	Updates operations and deferred charges.
rs/execution_environment/src/canister_logs.rs	Initializes the new response field.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

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

[Copilot]

🔵 Needs a closer look

The broad accounting refactor warrants final human review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🔵 Needs a closer look

The changes require final human review because they are too complex or risky for automated approval.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🟡 Changes recommended

Upfront metadata-snapshot charging can incorrectly reject a replacement that lowers memory usage.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Balanced

[zeropath-ai]

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

Security Overview

• 🔎 Scanned files: 5 changed file(s)
• 🔗 Scan Link: https://zeropath.com/app/repositories/f5e9d2bb-29a4-4b64-902a-391d65fd8b2d?scanId=8e98ddc7-b105-4e63-85d4-b8aa36e2a051&codeScanTypes=PrScan&tab=issues

Detected Code Changes

Change Type	Relevant files
Enhancement	► rs/execution_environment/src/canister_logs.rs     Add instructions_to_charge_on_success field to CanisterLogs fetch result ► rs/execution_environment/src/canister_manager.rs     Initialize instructions_to_charge_on_success in various CanisterManagerResponse instances     Remove some parameters from function signatures (round_limits, subnet_cycles_config, resource_saturation) in a few locations; adjust calls accordingly ► rs/execution_environment/src/canister_manager/tests.rs     Update test setup to align with presence of instructions_to_charge_on_success and related changes
Enhancement	► rs/execution_environment/src/canister_manager/types.rs     Add instructions_to_charge_on_success field to CanisterManagerResponse for tracking post-success charges
Enhancement	► rs/execution_environment/src/execution_environment.rs     Introduce CyclesAndMemoryUsageAccounting enum to control where accounting occurs     Update execution flow to handle AfterOperation vs InOperation accounting paths ► rs/execution_environment/src/execution_environment.rs     Modify execute_mgmt_operation_on_canister to pass CyclesAndMemoryUsageAccounting::AfterOperation for management operations and adjust handling accordingly