feat: Backfill the canister-level monotonic consumed cycles by mraszyk · Pull Request #11490 · dfinity/ic

mraszyk · 2026-09-08T11:30:11Z

CanisterMetrics::consumed_cycles_monotonic (added in #11465) is absent in
checkpoints written before it, and decodes as zero. Left alone it would only
start accruing from the round it was added in, so every canister that predates
it under-reports its lifetime consumption forever.

The consumed_cycles gauge predates it and holds the full history, so the
monotonic value can be derived from it. SystemState::outstanding_prepayments()
sums the prepayments that have been added to the gauge but whose refund has not
been observed yet -- the ones held in an open Callback
(prepayment_for_response_execution plus prepayment_for_call_transmission,
falling back to prepayment_for_response_transmission for callbacks created
before April 2026, as the refund path itself does) or in the
prepaid_execution_cycles of an aborted execution or install_code. All of
them are recorded in the replicated state, which yields the invariant

consumed_cycles - outstanding_prepayments() == consumed_cycles_monotonic

whenever no execution is in progress or paused, and makes
SystemState::migrate_consumed_cycles_to_monotonic() exact. Because the
invariant holds at every checkpoint rather than just the first one, the backfill
is also idempotent: a no-op for a canister already backfilled, and self-healing
if a downgrade dropped the field.

The scheduler runs it on checkpoint rounds only, after
abort_all_paused_executions: a paused execution holds its prepayment in
memory, not in the state, so outstanding_prepayments() returns None for one
and the backfill would otherwise overestimate. Aborting materializes those
prepayments into the task queues. The pass takes a mutable reference to a
canister only when it has something to write, so it stays read-only once the
subnet has been backfilled.

The three states this considers unreachable -- a canister whose outstanding
prepayments cannot be derived, outstanding prepayments above the gauge, and a
monotonic value already above the gauge net of them -- are reported through
debug_assert_or_critical_error! and a new
scheduler_consumed_cycles_invariant_broken error counter, since all of them
would otherwise be silent in production.

No metric or behaviour changes: nothing outside of tests reads
consumed_cycles_monotonic yet. A follow-up
(mraszyk/consumed-cycles-as-counter, #11416) exports the monotonic total under
the existing replicated_state_consumed_cycles_since_replica_started gauge and
is stacked on this branch.

🤖 Generated with Claude Code

`CanisterMetrics::consumed_cycles_monotonic` is absent in checkpoints written before it was introduced, and decodes as zero. Left alone it would only start accruing from the round it was added in, so every canister that predates it under-reports its lifetime consumption forever. The `consumed_cycles` gauge predates it and holds the full history, so the monotonic value can be derived from it. `SystemState::outstanding_prepayments()` sums the prepayments that have been added to the gauge but whose refund has not been observed yet -- the ones held in an open `Callback` or in the `prepaid_execution_cycles` of an aborted execution or `install_code`. All of them are recorded in the replicated state, which yields the invariant consumed_cycles - outstanding_prepayments() == consumed_cycles_monotonic and makes `SystemState::migrate_consumed_cycles_to_monotonic()` exact. Because the invariant holds at all times rather than just once, the backfill is also idempotent: it is a no-op for a canister already backfilled, and self-healing if a downgrade dropped the field. The scheduler runs it on checkpoint rounds only, after `abort_all_paused_executions`: a paused execution holds its prepayment in memory, not in the state, so `outstanding_prepayments()` returns `None` for one and the backfill would otherwise overestimate. Aborting materializes those prepayments into the task queues. The pass takes a mutable reference to a canister only when it has something to write, so it stays read-only once the subnet has been backfilled. The two states this considers unreachable -- a canister whose outstanding prepayments cannot be derived, and a monotonic value already above the gauge net of them -- are reported through `debug_assert_or_critical_error!` and a new `scheduler_consumed_cycles_invariant_broken` error counter, since both would otherwise be silent in production. No metric or behaviour changes: nothing outside of tests reads `consumed_cycles_monotonic` yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Saturating subtraction can silently hide an outstanding-prepayments invariant violation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Backfills canister-level monotonic consumed cycles from existing accounting state during checkpoint rounds.

Changes:

Derives outstanding prepayments from callbacks and aborted executions.
Runs idempotent checkpoint migration with invariant-error metrics.
Adds unit and scheduler coverage for migration scenarios.

File summaries

File	Description
`rs/state_layout/src/state_layout/proto.rs`	Documents legacy checkpoint decoding.
`rs/replicated_state/src/canister_state/tests.rs`	Tests prepayment derivation and migration.
`rs/replicated_state/src/canister_state/system_state.rs`	Implements derivation and backfill.
`rs/execution_environment/src/scheduler/tests/metrics.rs`	Tests checkpoint integration.
`rs/execution_environment/src/scheduler/scheduler_metrics.rs`	Adds invariant error counter.
`rs/execution_environment/src/scheduler.rs`	Executes and validates migration.
`rs/execution_environment/src/canister_manager.rs`	Documents deletion accounting invariant.

Review details

Files reviewed: 7/7 changed files
Comments generated: 1
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Address Copilot review on #11490. `NominalCycles` subtraction saturates at zero, so the backfill turned a gauge below the outstanding prepayments into a `derived` of zero. For a canister whose monotonic value is zero too -- which is every canister yet to be backfilled -- that compared `Equal` and was silently accepted, even though it is exactly the corrupt accounting state the new error counter exists to surface. Check `outstanding > consumed_cycles()` before subtracting and report it through the same `debug_assert_or_critical_error!` path, leaving the canister alone. The regression test asserts the panic that the debug assertion raises, and is deliberately free of any other assertion after the round, so that it fails rather than passing vacuously if the check is dropped. `reset_consumed_cycles()` joins the existing `reset_consumed_cycles_monotonic()` testing helper to construct the broken state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🔵 Needs a closer look

The migration depends on accounting invariants spanning callbacks, execution lifecycle states, checkpointing, and subnet operations.

Review details

Files reviewed: 7/7 changed files
Comments generated: 0 new
Review effort level: Balanced

The same saturating subtraction that Copilot flagged in the backfill was in the test helper that asserts the invariant: `consumed_cycles() - outstanding` bottoms out at zero, so a gauge that had fallen behind the outstanding prepayments would have compared equal to a zero monotonic value and slipped through the very assertion named after the invariant. Assert `outstanding <= consumed_cycles()` on its own, ahead of the subtraction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The deletion-accounting documentation incorrectly assumes the monotonic value has already been backfilled.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 7/7 changed files
Comments generated: 1
Review effort level: Balanced

Address Copilot review on #11490. The note on the deletion path claimed that a canister's `consumed_cycles` gauge and its `consumed_cycles_monotonic` are equal there, justified by a deleted canister holding no outstanding prepayment. That justification only covers a canister that has already been backfilled. Deletion is a management call, so it runs in an ordinary round, while the backfill runs on checkpoint rounds only: a canister decoded from a checkpoint predating the field can be deleted with a zero monotonic value and a nonzero gauge. Moving the gauge is still right -- now for the stated reason that only the gauge is guaranteed to hold the full history. The same gap was in the invariant documented on `outstanding_prepayments`, which `migrate_consumed_cycles_to_monotonic` was said to rely on. It does not hold before the backfill; the backfill is what establishes it, and the left-hand side is exactly the value it has to write. Documentation only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

A critical invariant-error branch lacks direct regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 7/7 changed files
Comments generated: 1
Review effort level: Balanced

Address Copilot review on #11490. The `Ordering::Less` arm of the backfill had no regression coverage: the existing corruption test resets both metrics, so it returns through the earlier `outstanding > consumed_cycles` check and never reaches it. Reaching that arm needs the outstanding prepayments to be zero -- with one still outstanding, dropping the gauge trips the earlier check first -- so the new test runs an ingress execution to completion, which refunds its prepayment, and then drops only the gauge. Verified in both directions: the test fails if the `Ordering::Less` report is removed, and the pre-existing corruption test keeps passing, so the two cover distinct arms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🔵 Needs a closer look

The replicated cycles-accounting migration depends on a subtle system-wide invariant and warrants final human validation.

Review details

Files reviewed: 7/7 changed files
Comments generated: 0 new
Review effort level: Balanced

Drop the notes on the canister deletion and checkpoint decoding paths, and the paragraphs on `SystemState::outstanding_prepayments` about paused executions and about an `install_code` dropped by a subnet split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A paused response execution is paid for by the callback that the task carries, so its prepayment could be derived from the replicated state. But the only caller runs after all paused executions have been aborted, so treating every paused execution alike keeps the contract simple. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The scheduler derives the outstanding prepayments itself before calling `SystemState::migrate_consumed_cycles_to_monotonic`, so it has no use for the flag; only the unit tests looked at it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The non-zero monotonic consumed cycles only held because of the ingress executions preceding the xnet call, not because of the invariant under test. And `checkpoint_round_reports_monotonic_above_the_gauge` was inserted above the test it calls itself the mirror image of. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🔵 Needs a closer look

Two unconstrained panic tests can pass without exercising their intended invariant branches.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

rs/execution_environment/src/scheduler/tests/metrics.rs:1212

This unconstrained should_panic can pass on any setup assertion failure before the checkpoint round (for example, if outstanding unexpectedly becomes zero), without exercising the invariant-error branch. Constrain the expected panic from debug_assert!(false) so setup regressions fail the test instead of producing a false positive.

This issue also appears on line 1238 of the same file.

rs/execution_environment/src/scheduler/tests/metrics.rs:1238

This unconstrained should_panic also treats failures in the ingress/setup assertions as success, so the test can pass without reaching the Ordering::Less report. Constrain it to the panic emitted by the target debug_assert!(false) (or catch the unwind only around the checkpoint call).

#[should_panic]

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

An unconstrained `should_panic` also accepts a panic from the assertions setting the test up, so a regression there would pass for the wrong reason. Expect the message of the `debug_assert!(false)` inside `debug_assert_or_critical_error!` instead, which none of them produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The migration accounts for persisted prepayment states, handles corruption defensively, and has focused regression coverage.

Review details

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

mraszyk requested a balanced review from Copilot September 8, 2026 11:30

github-actions Bot added the feat label Sep 8, 2026

Copilot started reviewing on behalf of mraszyk September 8, 2026 11:30 View session