fix(control-panel): make cycle monitoring independent of monitored canisters - #651
Draft
MRmarioruci wants to merge 2 commits into
Draft
fix(control-panel): make cycle monitoring independent of monitored canisters#651MRmarioruci wants to merge 2 commits into
MRmarioruci wants to merge 2 commits into
Conversation
Station balances are read from each station's own /metrics, so the value is chosen by whoever controls that station's code. canfund derives a consumption rate from consecutive readings as `(previous - current) * 1_000_000_000 / elapsed`, and that multiplication is not saturating. With overflow-checks enabled a crafted reading traps inside the shared monitoring round, and because the poisoned reading is committed by an earlier sweep while the trap rolls back only the later message, every subsequent sweep traps at the same point and no station is topped up again. Wrap the metrics fetcher so readings above the point where that arithmetic can overflow are reported as a fetch failure instead. canfund already records fetch failures per canister without aborting the round, so one misbehaving station no longer stops the rest being funded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A station that accepts the metrics call and never replies stalled the whole sweep. canfund awaits every registered canister inside one join_all while holding the process lock, so one unanswered call meant the round never returned and the lock was never released. Every later round then found the lock held and no station was ever topped up again. The lock cannot be released early and the call cannot be given a deadline: the repo is on ic-cdk 0.17, which has no bounded-wait call API. So stop reading balances inside the round at all. The fetcher now answers immediately from a local cache and refreshes it in a spawned task, which means the round no longer waits on any station. A canister that never replies only starves its own cache entry and is skipped, while every other canister is funded as normal. A refresh is not started while one is already in flight for the same canister, so an unresponsive station costs one open call context rather than one per round. Readings older than three days stop being used, so a station that goes quiet is skipped rather than funded indefinitely on a stale figure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Change
The control panel is not a controller of the stations it funds — the station resets its own controllers to
[upgrader, NNS root]during init (services/system.rs:359-363) — so it reads each balance by calling that station'shttp_requestand parsing a Prometheus metric. Both the timing and the value of that response therefore come from the monitored canister.The monitoring round is coupled to both. canfund awaits every registered canister inside one
join_allwhile holding the process lock (canfund-0.8.3/src/manager/mod.rs:208-217,:411), so the round's completion depends on every canister replying; and the reported value feeds a consumption-rate calculation whose multiplication is not saturating (manager/record.rs:49-60) while the workspace builds withoverflow-checks = true.Two layers around the fetcher remove both dependencies:
BoundedCyclesFetcherrejects readings aboveu128::MAX / 1_000_000_000, the point past which that multiplication can leaveu128. The bound is derived, not chosen: at or below it the product fits for any pair of accepted readings.CachedCyclesFetcheranswers from a local cache and never awaits a canister inside the round; the call happens in a spawned task that updates the cache when it completes. The round takes and releases the lock regardless of how any canister behaves.Supporting details:
MetricsHttpRequestFailed, which canfund already records per canister viaset_funding_failurewithout aborting the round.async-traitto the control panel (already a workspace dependency).Tests
Eight cases in
services::canister::bounded_cycles_fetcher_tests, covering the bound (plausible value, exactly at the bound,u128::MAXrejected, and a property test so the constant cannot drift out of correctness) and the cache (no reading yet, fresh reading served, expiry boundary, no second refresh while one is in flight).cargo test -p control-panel --libpasses (215). Clippy andcargo fmtclean.Trade-offs worth reviewing
Cold start. With no cached reading a canister is skipped for one round and funded from the next. Stations are funded at deploy time, so this is a day's delay on an already-funded canister.
Funding acts on data up to a day old. It already did, since the previous round's reading informed the same decision, but it is now explicit rather than incidental.
An unresponsive canister holds one open call context until it replies or is stopped, and its own monitoring slot degrades rather than the fleet's.
Note on upstream
canfund 0.8.5 changes neither behaviour:
set_cyclesis byte-identical and its fetcher usesCall::unbounded_wait. It also requires ic-cdk ^0.19 against this repo's 0.17.1, so adopting it is a two-major-version toolchain migration rather than a dependency bump. Handling this here avoids that.