refactor: install the canister memories in create_execution_state - #11142
refactor: install the canister memories in create_execution_state#11142mraszyk wants to merge 3 commits into
Conversation
`Hypervisor::create_execution_state` always built the initial memories of the new Wasm module, and every caller then overwrote them afterwards: `install_code` swapped in the preserved memories in `InstallCodeHelper::replace_execution_state_and_allocations`, and `load_canister_snapshot` assigned the snapshot's memories directly. Which memories a new execution state ends up with was hence decided in three different places, none of which the type system forced a caller to visit. Make `create_execution_state` take a mandatory `MemorySource` describing where the memories come from (`Fresh` for install/reinstall, `Preserve` for upgrades, `Explicit` for snapshot restore) and apply it there, so it is the single place that assembles an execution state's memories. The `wasm_memory_persistence` upgrade option only ever selected the main memory handling based on the install mode; the two enhanced orthogonal persistence checks in `determine_main_memory_handling` merely rejected otherwise valid combinations. Split it accordingly into the infallible `main_memory_handling`, which runs before the execution state is created, and `validate_wasm_memory_persistence`, which runs after it in the same position relative to the compilation charge as before. `load_canister_snapshot` now builds the memories to restore before prepaying for the message. As a result a failure to load them (`CanisterSnapshotNotLoadable`) no longer charges the canister for compiling the snapshot's module and now takes precedence over `NotEnoughCycles`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Centralizes execution-state memory selection in Hypervisor::create_execution_state.
Changes:
- Adds fresh, preserved, and explicit memory sources.
- Simplifies install/upgrade memory handling and persistence validation.
- Prepares snapshot memories before cycle prepayment.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
Re-exports memory-handling APIs. |
src/hypervisor.rs |
Applies memory sources during state creation. |
src/execution/upgrade/tests.rs |
Updates stage documentation. |
src/execution/upgrade.rs |
Preserves upgrade memories and separates validation. |
src/execution/install.rs |
Uses fresh memories for installs. |
src/execution/install_code.rs |
Removes post-creation memory swapping. |
src/canister_manager.rs |
Prepares snapshot memories before charging. |
benches/management_canister/create_execution_state.rs |
Updates benchmark invocation. |
benches/lib/src/common.rs |
Updates benchmark helper invocation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// Specifies the handling of the canister's memories. | ||
| /// * On install and re-install: | ||
| /// - Replace both the stable memory and the main memory. | ||
| /// * On upgrade: | ||
| /// - For canisters with enhanced orthogonal persistence (Motoko): | ||
| /// Retain both the main memory and the stable memory. | ||
| /// - For all other canisters: | ||
| /// Retain only the stable memory and erase the main memory. |
There was a problem hiding this comment.
Not sure about this one, the current formulation is indeed a bit imprecise (informal), but describes the intent (Motoko EOP).
There was a problem hiding this comment.
I suppose you could reorder things as suggested, but retaining your existing structure:
| /// Specifies the handling of the canister's memories. | |
| /// * On install and re-install: | |
| /// - Replace both the stable memory and the main memory. | |
| /// * On upgrade: | |
| /// - For canisters with enhanced orthogonal persistence (Motoko): | |
| /// Retain both the main memory and the stable memory. | |
| /// - For all other canisters: | |
| /// Retain only the stable memory and erase the main memory. | |
| /// Specifies the handling of the canister's memories. | |
| /// * On install and re-install: | |
| /// - Replace both the stable memory and the main memory. | |
| /// * On upgrade: | |
| /// - Stable memory: retained for all canisters. | |
| /// - Main memory: retained for canisters with enhanced orthogonal persistence | |
| /// (Motoko) when the validated persistence option is `Keep`; and erased in | |
| /// all other cases. |
Or you can stick with what you have. No strong opinion.
|
✅ No security or compliance issues detected. Reviewed everything up to 082f70c. Security Overview
Detected Code Changes
|
| // is constructed once per operation and consumed right away. | ||
| #[allow(clippy::large_enum_variant)] | ||
| pub enum MemorySource<'a> { | ||
| /// Install and re-install: keep the initial memories of the new module. |
There was a problem hiding this comment.
"Keep" is potentially confusing in this context (particularly given that there's a Preserve variant below, with the exact opposite behavior).
| /// Install and re-install: keep the initial memories of the new module. | |
| /// Install and re-install: use the newly installed module's initial memories. |
| /// Specifies the handling of the canister's memories. | ||
| /// * On install and re-install: | ||
| /// - Replace both the stable memory and the main memory. | ||
| /// * On upgrade: | ||
| /// - For canisters with enhanced orthogonal persistence (Motoko): | ||
| /// Retain both the main memory and the stable memory. | ||
| /// - For all other canisters: | ||
| /// Retain only the stable memory and erase the main memory. |
There was a problem hiding this comment.
I suppose you could reorder things as suggested, but retaining your existing structure:
| /// Specifies the handling of the canister's memories. | |
| /// * On install and re-install: | |
| /// - Replace both the stable memory and the main memory. | |
| /// * On upgrade: | |
| /// - For canisters with enhanced orthogonal persistence (Motoko): | |
| /// Retain both the main memory and the stable memory. | |
| /// - For all other canisters: | |
| /// Retain only the stable memory and erase the main memory. | |
| /// Specifies the handling of the canister's memories. | |
| /// * On install and re-install: | |
| /// - Replace both the stable memory and the main memory. | |
| /// * On upgrade: | |
| /// - Stable memory: retained for all canisters. | |
| /// - Main memory: retained for canisters with enhanced orthogonal persistence | |
| /// (Motoko) when the validated persistence option is `Keep`; and erased in | |
| /// all other cases. |
Or you can stick with what you have. No strong opinion.
Hypervisor::create_execution_statealways built the initial memories of the new Wasm module, and every caller then overwrote them afterwards:install_codeswapped in the preserved memories inInstallCodeHelper::replace_execution_state_and_allocations, andload_canister_snapshotassigned the snapshot's memories directly. Which memories a new execution state ends up with was hence decided in three different places.Make
create_execution_statetake a mandatoryMemorySourcedescribing where the memories come from (Freshfor install/reinstall,Preservefor upgrades,Explicitfor snapshot restore) and apply it there, so it is the single place that assembles an execution state's memories.The
wasm_memory_persistenceupgrade option only ever selected the main memory handling based on the install mode; the two enhanced orthogonal persistence checks indetermine_main_memory_handlingmerely rejected otherwise valid combinations. Split it accordingly into the infalliblemain_memory_handling, which runs before the execution state is created, andvalidate_wasm_memory_persistence, which runs after it in the same position relative to the compilation charge as before.load_canister_snapshotnow builds the memories to restore before prepaying for the message. As a result a failure to load them (CanisterSnapshotNotLoadable) no longer charges the canister for compiling the snapshot's module and now takes precedence overNotEnoughCycles.