Skip to content

refactor: validate_and_update_canister_settings#9996

Draft
mraszyk wants to merge 38 commits intomasterfrom
mraszyk/check-canister-settings
Draft

refactor: validate_and_update_canister_settings#9996
mraszyk wants to merge 38 commits intomasterfrom
mraszyk/check-canister-settings

Conversation

@mraszyk
Copy link
Copy Markdown
Contributor

@mraszyk mraszyk commented Apr 23, 2026

Refactored CanisterManager to eliminate the validate/apply split for canister settings by merging both functions into a single validate_and_update_canister_settings function that validates and mutates in one pass, removing all .expect() panics.

This is possible now that

  • changing canister settings operates on a clone of canister state and round limits; and
  • canister creation only commits the canister state upon success and operates on a clone of round limits (the latter was changed in this PR).

@mraszyk mraszyk changed the title refactor: check_canister_settings refactor: validate_and_update_canister_settings Apr 28, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors canister settings handling in the execution environment by removing the previous “validate then apply” split and instead validating + mutating canister settings in a single pass, with the intent of eliminating .expect() panics and relying on cloned state / rollback for failure cases.

Changes:

  • Removed ValidatedCanisterSettings and the separate validate_* / do_update_settings() pipeline, replacing it with validate_and_update_canister_settings() that mutates CanisterState and RoundLimits directly.
  • Updated canister creation flow to snapshot and restore round_limits on failure, and added a regression test for reverting subnet available memory when creation fails mid-way.
  • Adjusted update_settings to use the new combined validate+apply flow.

Reviewed changes

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

File Description
rs/execution_environment/src/canister_settings.rs Removes the now-obsolete ValidatedCanisterSettings type.
rs/execution_environment/src/canister_manager.rs Replaces validate/apply split with a combined validate+mutate function; updates update/create flows accordingly.
rs/execution_environment/src/canister_manager/tests.rs Adds a test ensuring subnet available memory is reverted when canister creation fails after partial resource updates.

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

Comment thread rs/execution_environment/src/canister_manager.rs
Comment thread rs/execution_environment/src/canister_manager.rs Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


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

Comment thread rs/execution_environment/src/canister_manager.rs Outdated
Comment thread rs/execution_environment/src/canister_manager.rs Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


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

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants