refactor: resolve the DSM-11 canister log TODOs - #11210
Merged
Merged
Conversation
Two follow-ups left behind by the canister log memory store work: - Move the hardcoded `LOG_RESIZE_COST_PER_BYTE` out of `canister_manager.rs` into `SchedulerConfig` as `canister_log_resize_instructions_per_byte` (default `DEFAULT_CANISTER_LOG_RESIZE_INSTRUCTIONS_PER_BYTE` = 32), threaded through `CanisterMgrConfig`. The TODO suggested `CyclesAccountManagerConfig`, but that config holds only `Cycles` denominated fees while this is an instruction cost per byte, so it belongs next to `upload_wasm_chunk_instructions` and the `canister_snapshot_*_instructions` knobs. The value is kept non-zero on system subnets as well, so charging is unchanged everywhere. The integration test no longer hardcodes the value either. - Drop the aggregate flavour of `CanisterLog`, which the migration to `LogMemoryStore` (completed in #10821) made dead: `new_aggregate`, `default_aggregate`, `CanisterLog::append_delta_log`, `clear`, `retention` (superseded by `LogMemoryStore::retention`, already used by the metric) and the `Records` helpers that only served them. `Records::from` collapses into `Records::new(byte_capacity)` as every remaining constructor starts out empty. `CanisterLog` is now purely the per-message delta buffer, as its doc comment now states. The aggregate append tests are dropped, as the surviving behaviour is covered by the `LogMemoryStore::append_delta_log` tests; the rest are rewritten against the delta constructors, with new coverage for capacity capping and `take()`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… subnets - Add `test_canister_log_add_record_evicts_multiple_oldest_records`, which covers the `Records::make_free_space_within_limit` loop evicting several records at once while leaving survivors: it fills a delta log with twenty 100-byte records (no eviction yet), then adds a single large record sized so that exactly fourteen of them still fit next to it, and asserts the six oldest were evicted, the surviving indices are the contiguous newest tail, the large content was not truncated, and `next_idx` counts them all. The record sizes are derived from `RECORD_SIZE` and the test capacity, with compile-time asserts pinning down the setup so it cannot silently degenerate if `CanisterLogRecord` grows a field. The existing `test_canister_log_add_record_applies_memory_limit` only covers the degenerate case where every record fills the whole capacity, so exactly one predecessor is evicted per push. The multi-eviction case used to be covered indirectly by the aggregate `append_delta_log` tests dropped in c6a24fa; the `LogMemoryStore` tests that superseded them exercise the ring buffer, not this eviction loop. - Set `canister_log_resize_instructions_per_byte` to zero on system subnets, in line with `upload_wasm_chunk_instructions` and the `canister_snapshot_*_instructions` knobs next to it. This drops the log resize charge on system subnets, which c6a24fa had deliberately kept non-zero there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mraszyk
commented
Aug 19, 2026
|
✅ No security or compliance issues detected. Reviewed everything up to 9f9fbd1. Security Overview
Detected Code Changes
|
alin-at-dfinity
approved these changes
Aug 20, 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
2 participants
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.
Two follow-ups left behind by the canister log memory store work:
Move the hardcoded
LOG_RESIZE_COST_PER_BYTEout ofcanister_manager.rsintoSchedulerConfigascanister_log_resize_instructions_per_byte(defaultDEFAULT_CANISTER_LOG_RESIZE_INSTRUCTIONS_PER_BYTE= 32), threaded throughCanisterMgrConfig. The TODO suggestedCyclesAccountManagerConfig, but that config holds onlyCyclesdenominated fees while this is an instruction cost per byte, so it belongs next toupload_wasm_chunk_instructionsand thecanister_snapshot_*_instructionsknobs. The integration test no longer hardcodes the value either.Drop the aggregate flavour of
CanisterLog, which the migration toLogMemoryStore(completed in refactor: drop the legacy canister_log store from SystemState #10821) made dead:new_aggregate,default_aggregate,CanisterLog::append_delta_log,clear,retention(superseded byLogMemoryStore::retention, already used by the metric) and theRecordshelpers that only served them.Records::fromcollapses intoRecords::new(byte_capacity)as every remaining constructor starts out empty.CanisterLogis now purely the per-message delta buffer, as its doc comment now states.The aggregate append tests are dropped, as the surviving behaviour is covered by the
LogMemoryStore::append_delta_logtests; the rest are rewritten against the delta constructors, with new coverage for capacity capping andtake().