refactor: stop using log memory store feature flag and migration fields in production#10755
Open
mraszyk wants to merge 2 commits into
Open
refactor: stop using log memory store feature flag and migration fields in production#10755mraszyk wants to merge 2 commits into
mraszyk wants to merge 2 commits into
Conversation
…ds in production The one-time migration from the legacy `CanisterLog` store to the `LogMemoryStore` has completed on all subnets, so production code no longer needs to consult the feature flag or the per-canister migration status. This decouples the runtime logic from those fields while keeping them serialized for downgrade compatibility (unlike #10714, which was reverted). - Drop the `log_memory_store_feature` flag from `Config` and all plumbing (canister manager config, `SystemState::new*`, `LogMemoryStore::new()`, `fetch_canister_logs`, scheduler). The `LOG_MEMORY_STORE_FEATURE` / `_ENABLED` consts are retained but now unused by production logic. - Remove the migration scaffolding: the subnet-level `logs_migrated` metadata, the scheduler migration code (and `filter_canister_log_records`), the migration-duration metric, and the orphaned `set_migrated` / `clear_migrated` methods. - Collapse all production `is_migrated()` branches to the always-migrated path. `next_canister_log_record_idx` and `log_memory_store_migrated` (proto 44/66) are still written and round-tripped, and the legacy `canister_log` store is retained and kept up to date via dual-write, so checkpoints remain readable by replicas that predate the log memory store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 8dccff3. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the now-obsolete log migration/runtime toggling logic by making LogMemoryStore the unconditional production source of truth, while still dual-writing and preserving serialized fields to keep checkpoints readable for older replicas.
Changes:
- Removes the
log_memory_store_featureflag fromic_config::execution_environment::Configand deletes all runtime branching/plumbing based on it. - Deletes migration scaffolding (
logs_migratedsubnet metadata, scheduler migration pass, migration metrics, and related helpers), collapsing logic to the “always migrated” path. - Updates log fetch, metrics, tests, and benchmarks to use
LogMemoryStoreAPIs unconditionally, while keeping legacycanister_logupdated via dual-write for downgrade/checkpoint compatibility.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| rs/test_utilities/execution_environment/src/lib.rs | Removes test builder helpers for toggling the log memory store feature flag. |
| rs/state_manager/src/checkpoint.rs | Always checks the next_idx soft invariant between log_memory_store and canister_log. |
| rs/replicated_state/src/metrics.rs | Records log memory usage/retention from LogMemoryStore unconditionally. |
| rs/replicated_state/src/metadata_state/proto.rs | Removes initialization of transient logs_migrated metadata. |
| rs/replicated_state/src/metadata_state.rs | Removes the logs_migrated transient field and associated struct plumbing. |
| rs/replicated_state/src/canister_state/system_state/log_memory_store/tests/log_memory_store.rs | Updates tests to the new LogMemoryStore::new() signature and removes flag-dependent cases. |
| rs/replicated_state/src/canister_state/system_state/log_memory_store/tests/canister_lifecycle.rs | Updates lifecycle test setup to use LogMemoryStore::new() without flags. |
| rs/replicated_state/src/canister_state/system_state/log_memory_store/mod.rs | Drops FlagStatus dependency, makes new() flag-free and “always migrated”, removes migrated mutators, adds Default. |
| rs/replicated_state/src/canister_state/system_state.rs | Removes feature-flag plumbing from SystemState construction; always constructs LogMemoryStore directly. |
| rs/replicated_state/benches/log_memory_store.rs | Updates benchmark setup to use LogMemoryStore::new() without feature flags. |
| rs/execution_environment/tests/canister_logging.rs | Removes feature-flag-specific integration tests and updates configs/callers accordingly. |
| rs/execution_environment/src/scheduler/test_utilities.rs | Removes passing log_memory_store_feature into scheduler construction. |
| rs/execution_environment/src/scheduler/scheduler_metrics.rs | Removes migration-duration metric from scheduler metrics. |
| rs/execution_environment/src/scheduler.rs | Removes per-round migration logic and the legacy-gap filtering helper + tests. |
| rs/execution_environment/src/query_handler.rs | Simplifies fetch-canister-logs query handling by removing flag parameter plumbing. |
| rs/execution_environment/src/lib.rs | Removes feature-flag plumbing when wiring execution services. |
| rs/execution_environment/src/execution_environment.rs | Updates the replicated management call path to use the new fetch_canister_logs signature. |
| rs/execution_environment/src/canister_manager/types.rs | Removes log_memory_store_feature from canister manager configuration. |
| rs/execution_environment/src/canister_manager/tests.rs | Updates tests to remove feature-flag configuration and builder usage. |
| rs/execution_environment/src/canister_manager.rs | Applies log memory limit logic unconditionally (no longer gated by migration status) and removes flag from system state creation. |
| rs/execution_environment/src/canister_logs.rs | Makes fetch-canister-logs responses always read from LogMemoryStore and removes legacy filtering path. |
| rs/execution_environment/benches/management_canister/canister_logging.rs | Removes feature-flag config usage in benchmarks. |
| rs/embedders/src/wasmtime_embedder/tests.rs | Updates testing constructors to remove LOG_MEMORY_STORE_FEATURE parameter plumbing. |
| rs/embedders/src/wasmtime_embedder/system_api/sandbox_safe_system_state.rs | Always appends deltas to LogMemoryStore and always derives status fields from it; clarifies dual-write intent for legacy store. |
| rs/config/src/execution_environment.rs | Removes log_memory_store_feature from execution environment config and its default initialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
The one-time migration from the legacy
CanisterLogstore to theLogMemoryStorehas completed on all subnets, so production code no longer needs to consult the feature flag or the per-canister migration status. This decouples the runtime logic from those fields while keeping them serialized for downgrade compatibility (unlike #10714, which was reverted).log_memory_store_featureflag fromConfigand all plumbing (canister manager config,SystemState::new*,LogMemoryStore::new(),fetch_canister_logs, scheduler). TheLOG_MEMORY_STORE_FEATURE/_ENABLEDconsts are retained but now unused by production logic.logs_migratedmetadata, the scheduler migration code (andfilter_canister_log_records), the migration-duration metric, and the orphanedset_migrated/clear_migratedmethods.is_migrated()branches to the always-migrated path.next_canister_log_record_idxandlog_memory_store_migrated(proto 44/66) are still written and round-tripped, and the legacycanister_logstore is retained and kept up to date via dual-write, so checkpoints remain readable by replicas that predate the log memory store.