feat: introduce UnflushedCheckpointOp::DeleteSnapshot and raise a critical error if snapshots are filtered from tip - #11241
Merged
Conversation
Snapshot deletions were not recorded as checkpoint operations, so `TipRequest::FilterTipCanisters` was the regular mechanism for removing the directories of deleted snapshots from tip. This change introduces `UnflushedCheckpointOp::DeleteSnapshot`, recorded by every code path that can delete a snapshot: * `CanisterManager::delete_canister_snapshot()`; * `take_canister_snapshot()` and `create_snapshot_from_metadata()`, for the snapshot identified by `replace_snapshot`; * the uninstall of a canister that ran out of cycles, in `SchedulerImpl::charge_canisters_for_resource_allocation_and_usage()`; * the deletion of a snapshot's canister, i.e. `ReplicatedState::remove_canister()` and the two subnet split methods (a canister's snapshots live in their own top-level `snapshots` directory, so removing the canister's directory does not cover them). Every snapshot directory removal from tip is thus an explicit, ordered operation and `FilterTipCanisters` is only a safety net for snapshots that disappeared from the state without a corresponding operation. `TipHandler::filter_tip_snapshots()` therefore returns the IDs of the snapshots it removed; if the list is non-empty, the tip thread logs an error and bumps the new `state_manager_tip_snapshots_filtered` critical error. This mirrors `state_manager_tip_canisters_filtered`. Deleting a snapshot is hardened so that the operation cannot be forgotten by a new code path: `CanisterSnapshots::remove()` and `delete_snapshots()` now require a `&mut UnflushedCheckpointOps` and record the deletion themselves, so there is no way to remove a snapshot from the state without producing the operation. Callers that only mutate a single `CanisterState` and hence have no access to `SystemMetadata` (`CanisterManager`, and the uninstall loop in the scheduler) pass in a temporary `UnflushedCheckpointOps` that is merged into the state's operations via the new `UnflushedCheckpointOps::extend()`. Accordingly, `CanisterManagerResponse::unflushed_checkpoint_op` becomes `unflushed_checkpoint_ops`, as replacing a snapshot produces both a `DeleteSnapshot` and a `TakeSnapshot`. In the same spirit, `UnflushedCheckpointOps::delete_canister()` now takes the `CanisterState` rather than just the canister ID and records a `DeleteSnapshot` for each of the canister's snapshots, so that they cannot be overlooked. `SnapshotLayout::delete_dir()` is tolerant of a missing directory now (as `delete_canister_dir()` already was), because a snapshot created from uploaded metadata has no directory in tip until the next checkpoint; its body moved into a free function shared with the new `CheckpointLayout::delete_snapshot_dir()`. Tests: the new `filtering_snapshot_from_tip_raises_critical_error` covers the critical error; `deleted_snapshot_is_removed_from_tip` and `snapshots_of_deleted_canister_are_removed_from_tip` cover the flush of the new operation; `snapshot_deletions_record_unflushed_checkpoint_ops` and `delete_canister_records_unflushed_checkpoint_ops_for_its_snapshots` cover the operations recorded by execution, as does a new assertion in `snapshot_is_deleted_when_canister_is_out_of_cycles`. The `online_split` expectations in `replicated_state.rs` drop two dead `canister_snapshots.remove()` calls, which removed a snapshot from the wrong canister's collection and hence were no-ops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CanisterManager::create_snapshot_from_metadata()` records no
`UnflushedCheckpointOp::TakeSnapshot` (there is nothing to copy from the
canister), so a snapshot created from uploaded metadata has no directory
in tip until the next checkpoint creates it. Deleting such a snapshot
before then flushes an `UnflushedCheckpointOp::DeleteSnapshot` for a
directory that does not exist, which is why `SnapshotLayout::delete_dir()`
(and hence `CheckpointLayout::delete_snapshot_dir()`) tolerates
`NotFound`. Without that tolerance the flush fails with
Cannot remove snapshot.: No such file or directory (os error 2)
which `fatal!`s the tip thread. Two new tests pin this down:
`delete_snapshot_dir_is_idempotent` covers the layout method directly
(including that the enclosing per-canister directory is only removed
along with the canister's last snapshot), and
`deleting_snapshot_without_tip_directory_is_a_noop` covers the flush of
the operation end-to-end. Both fail with the above I/O error if the
`NotFound` arm is dropped.
Also, `deleted_snapshot_is_removed_from_tip` now asserts the exact
recorded operation rather than merely that some operation was recorded.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The directory of a snapshot in tip is created as a side effect of `CheckpointLayout::snapshot()`, whose `Permissions::check_dir` is a `create_dir_all` for a writable layout. For a snapshot created from uploaded metadata (which records no `UnflushedCheckpointOp::TakeSnapshot`, so `backup()` never runs for it) the first such call is not the checkpoint serialization, as previously claimed, but `PageMapType::layout()` in the handling of `TipRequest::FlushPageMapDelta`: the snapshot's `PageMap`s are brand new, hence `has_files_in_tip` is false, hence `should_flush()` holds and `flush_checkpoint_ops_and_page_maps()` includes them (with `truncate: true`) even though they hold no data. As `make_unvalidated_checkpoint()` flushes before serializing, this is also what creates the directory in a checkpoint round. The window in which `UnflushedCheckpointOp::DeleteSnapshot` finds no directory is therefore not "before the next checkpoint" but "before the first flush that still sees the snapshot in the state". The comments are updated accordingly. `deleting_snapshot_without_tip_directory_is_a_noop` claimed to add a snapshot as `create_snapshot_from_metadata()` does, but built it with `CanisterSnapshot::from_canister()`, whose `PageMap`s are clones of the canister's and hence do have files in tip; and such a snapshot always has a `TakeSnapshot` operation recorded in production, making the scenario unreachable. It now builds the snapshot with `CanisterSnapshot::from_metadata()`, matching the only production path that yields a snapshot with no recorded operation. The test still fails with `Cannot remove snapshot.: No such file or directory (os error 2)` if the `NotFound` arm of `delete_snapshot_dir()` is dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces explicit snapshot-deletion checkpoint operations so tip storage remains synchronized with replicated state.
Changes:
- Records and sequentially flushes
DeleteSnapshotoperations across all deletion paths. - Makes snapshot-directory deletion idempotent.
- Reports unexpected snapshot filtering as a critical error and adds coverage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
rs/state_manager/tests/state_manager.rs |
Tests deletion flushing and critical errors. |
rs/state_manager/src/tip.rs |
Flushes snapshot deletions and reports filtering. |
rs/state_manager/src/lib.rs |
Registers the new critical-error metric. |
rs/state_layout/src/state_layout/tests.rs |
Tests idempotent directory deletion. |
rs/state_layout/src/state_layout.rs |
Adds non-creating, idempotent snapshot deletion. |
rs/replicated_state/tests/replicated_state.rs |
Updates subnet-split expectations. |
rs/replicated_state/src/replicated_state.rs |
Records snapshot deletions during canister removal. |
rs/replicated_state/src/metadata_state.rs |
Defines and collects DeleteSnapshot operations. |
rs/replicated_state/src/canister_state/canister_snapshots.rs |
Records operations when snapshots are removed. |
rs/execution_environment/src/scheduler/tests/charging.rs |
Verifies out-of-cycles deletion recording. |
rs/execution_environment/src/scheduler.rs |
Collects deletions during forced uninstall. |
rs/execution_environment/src/execution_environment/tests/canister_snapshots.rs |
Tests execution-layer operation recording. |
rs/execution_environment/src/execution_environment.rs |
Merges checkpoint-operation collections. |
rs/execution_environment/src/canister_manager/types.rs |
Expands responses to multiple operations. |
rs/execution_environment/src/canister_manager.rs |
Records all manager-driven snapshot deletions. |
rs/execution_environment/src/canister_logs.rs |
Adapts responses to the new operations field. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The canister's snapshots live in the `CanisterState`, so the canister ID alone does not let `UnflushedCheckpointOps::delete_canister()` enumerate them at all. Say that, rather than only stating the motive for doing the enumeration here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 221b044. Security Overview
Detected Code Changes
|
schneiderstefan
approved these changes
Aug 21, 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.
Snapshot deletions were not recorded as checkpoint operations, so
TipRequest::FilterTipCanisterswas the regular mechanism for removing thedirectories of deleted snapshots from tip. This change introduces
UnflushedCheckpointOp::DeleteSnapshot, recorded by every code path that can delete asnapshot:
CanisterManager::delete_canister_snapshot();take_canister_snapshot()andcreate_snapshot_from_metadata(), for the snapshotidentified by
replace_snapshot;SchedulerImpl::charge_canisters_for_resource_allocation_and_usage();ReplicatedState::remove_canister()andthe two subnet split methods (a canister's snapshots live in their own top-level
snapshotsdirectory, so removing the canister's directory does not cover them).Renaming a canister needs no equivalent, as
rename_canisterrejects a canister thathas snapshots (
CanisterManagerError::RenameCanisterHasSnapshot).Like the other operations, these are transient: they are flushed (and taken) before the
checkpoint is written and are never persisted, so neither the checkpoint contents nor
the state hash are affected.
flush_unflushed_checkpoint_ops()applies themsequentially, in the order they were recorded (a snapshot may be taken and then loaded
within the same flush, in which case
restore()must see the directorybackup()justcreated); and the tip thread runs it before persisting page map deltas, so that
backup()copies the files a canister already has in tip and any still-unflushed deltasare written on top of them.
Every snapshot directory removal from tip is thus an explicit, ordered operation and
FilterTipCanistersis only a safety net for snapshots that disappeared from the statewithout a corresponding operation.
TipHandler::filter_tip_snapshots()thereforereturns the IDs of the snapshots it removed; if the list is non-empty, the tip thread
logs an error and bumps the new
state_manager_tip_snapshots_filteredcritical error.This mirrors
state_manager_tip_canisters_filtered.Deleting a snapshot is hardened so that the operation cannot be forgotten by a new code
path:
CanisterSnapshots::remove()anddelete_snapshots()now require a&mut UnflushedCheckpointOpsand record the deletion themselves, so there is no way toremove a snapshot from the state without producing the operation. Callers that only
mutate a single
CanisterStateand hence have no access toSystemMetadata(
CanisterManager, and the uninstall loop in the scheduler) pass in a temporaryUnflushedCheckpointOpsthat is merged into the state's operations via the newUnflushedCheckpointOps::extend(). Accordingly,CanisterManagerResponse::unflushed_checkpoint_opbecomesunflushed_checkpoint_ops,as replacing a snapshot produces both a
DeleteSnapshotand aTakeSnapshot. In thesame spirit,
UnflushedCheckpointOps::delete_canister()now takes theCanisterStaterather than just the canister ID and records a
DeleteSnapshotfor each of thecanister's snapshots, so that they cannot be overlooked.
SnapshotLayout::delete_dir()is tolerant of a missing directory now (asdelete_canister_dir()already was), and its body moved into a free function sharedwith the new
CheckpointLayout::delete_snapshot_dir(). The tolerance is needed becausea snapshot's directory in tip is only created as a side effect of
CheckpointLayout::snapshot(), and for a snapshot created from uploaded metadata(which records no
TakeSnapshot, as there is nothing to copy from the canister) thefirst such call is
PageMapType::layout()while handlingFlushPageMapDelta: thesnapshot's
PageMaps are brand new, hencehas_files_in_tipis false, henceshould_flush()holds and they are included in the flush even though they hold nodata. A snapshot created and deleted before any flush sees it in the state therefore
never gets a directory, and flushing its
DeleteSnapshotwould otherwise fail withCannot remove snapshot.: No such file or directory (os error 2), whichfatal!s thetip thread.
Tests: the new
filtering_snapshot_from_tip_raises_critical_errorcovers the criticalerror;
deleted_snapshot_is_removed_from_tipandsnapshots_of_deleted_canister_are_removed_from_tipcover the flush of the newoperation;
snapshot_deletions_record_unflushed_checkpoint_opsanddelete_canister_records_unflushed_checkpoint_ops_for_its_snapshotscover theoperations recorded by execution, as does a new assertion in
snapshot_is_deleted_when_canister_is_out_of_cycles. The idempotency of the newoperation is pinned down by
delete_snapshot_dir_is_idempotent(the layout methoddirectly, including that the enclosing per-canister directory is only removed along
with the canister's last snapshot) and
deleting_snapshot_without_tip_directory_is_a_noop(the flush of the operation end-to-end, for a snapshot built with
CanisterSnapshot::from_metadata()); both fail with the I/O error above if theNotFoundarm is dropped. Theonline_splitexpectations inreplicated_state.rsdroptwo dead
canister_snapshots.remove()calls, which removed a snapshot from the wrongcanister's collection and hence were no-ops.