From 13404aed598424d40c3f6c40c517e259dd7aefc6 Mon Sep 17 00:00:00 2001 From: Mario Ruci Date: Tue, 11 Aug 2026 09:34:28 +0200 Subject: [PATCH] fix(control-panel): consume deploy quota before spending cycles deploy_station checked the per-user and global rate limiters before doing any work, but only advanced them inside add_deployed_station, which runs on the fully successful path. Creating and funding the station happens well before that and is irreversible, so a deployment that failed after those steps had already spent real cycles while leaving both counters untouched. The pre-flight check therefore always passed and the spend was unbounded. The failure was also trivially forced rather than incidental: station init rejects an empty user list, so passing no admins fails deterministically after the station has been created and funded. Charge the quota immediately before the first irreversible spend, and reject an empty admin list up front so that path costs nothing at all. Recording the station and registering it for monitoring stay on the success path. Co-Authored-By: Claude Opus 5 --- core/control-panel/impl/src/models/user.rs | 7 ++ .../control-panel/impl/src/services/deploy.rs | 13 +++ core/control-panel/impl/src/services/user.rs | 79 +++++++++++++++++-- 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/core/control-panel/impl/src/models/user.rs b/core/control-panel/impl/src/models/user.rs index bcbe5e3a3..899b308cb 100644 --- a/core/control-panel/impl/src/models/user.rs +++ b/core/control-panel/impl/src/models/user.rs @@ -100,6 +100,13 @@ impl User { pub fn add_deployed_station(&mut self, station: Principal) { self.deployed_stations.push(station); + } + + /// Consumes one unit of the user's daily deploy quota. + /// + /// Kept separate from `add_deployed_station` so the quota can be charged before any cycles are + /// spent, rather than only on the fully successful path. + pub fn consume_deploy_quota(&mut self) { self.user_rate_limiter.add_deployed_station(); } diff --git a/core/control-panel/impl/src/services/deploy.rs b/core/control-panel/impl/src/services/deploy.rs index 1e3c8cc01..18b0ee7fe 100644 --- a/core/control-panel/impl/src/services/deploy.rs +++ b/core/control-panel/impl/src/services/deploy.rs @@ -57,6 +57,14 @@ impl DeployService { let station_wasm_module = config.station_wasm_module; let station_wasm_module_extra_chunks = config.station_wasm_module_extra_chunks; + // Station init rejects an empty user list, so this would otherwise fail deterministically + // after the station has already been created and funded. + if input.admins.is_empty() { + return Err(DeployError::Failed { + reason: "At least one admin must be specified.".to_string(), + })?; + } + let can_deploy_station_response = user.can_deploy_station(); match can_deploy_station_response { CanDeployStation::Allowed(_) => {} @@ -70,6 +78,11 @@ impl DeployService { } } + // Charged before the first irreversible spend below. Everything past this point costs + // cycles whether or not the deployment completes, so the quota must be consumed even if a + // later step fails. + self.user_service.consume_deploy_quota(&user.id, ctx)?; + // Creates the station canister let station_canister = create_canister(input.subnet_selection, CANISTER_CREATION_CYCLES) .await diff --git a/core/control-panel/impl/src/services/user.rs b/core/control-panel/impl/src/services/user.rs index e5a3d85be..981c9fe90 100644 --- a/core/control-panel/impl/src/services/user.rs +++ b/core/control-panel/impl/src/services/user.rs @@ -131,6 +131,27 @@ impl UserService { .collect() } + /// Charges one deployment against the global and per-user daily quotas. + /// + /// Must be called before any cycles are spent. Creating and funding a station is irreversible, + /// so a deployment that fails afterwards has still consumed real cycles and must consume quota + /// too; charging only on success let a caller force a post-spend failure and drain the + /// canister's balance without ever advancing either counter. + pub fn consume_deploy_quota(&self, user_id: &UserId, ctx: &CallContext) -> ServiceResult<()> { + let mut user = self.get_user(user_id, ctx)?; + let mut config = canister_config().ok_or(DeployError::Failed { + reason: "Canister config not initialized.".to_string(), + })?; + + config.global_rate_limiter.add_deployed_station(); + user.consume_deploy_quota(); + + write_canister_config(config); + self.user_repository.insert(user.to_key(), user); + + Ok(()) + } + pub fn add_deployed_station( &self, user_id: &UserId, @@ -138,16 +159,11 @@ impl UserService { ctx: &CallContext, ) -> ServiceResult { let mut user = self.get_user(user_id, ctx)?; - let mut config = canister_config().ok_or(DeployError::Failed { - reason: "Canister config not initialized.".to_string(), - })?; - config.global_rate_limiter.add_deployed_station(); user.add_deployed_station(station_canister_id); user.validate()?; - write_canister_config(config); self.user_repository.insert(user.to_key(), user.clone()); FUND_MANAGER.with(|fund_manager| { @@ -345,6 +361,59 @@ mod tests { assert!(duplicated_user_result.is_err()); } + #[tokio::test] + async fn consume_deploy_quota_charges_the_user_without_recording_a_station() { + crate::core::test_utils::init_canister_config(); + + let user: User = mock_user(); + let ctx = CallContext::new(user.identity); + let service = UserService::default(); + + service.user_repository.insert(user.to_key(), user.clone()); + + for _ in 0..User::MAX_DEPLOYED_STATIONS_PER_DAY { + service + .consume_deploy_quota(&user.id, &ctx) + .expect("Failed to consume deploy quota"); + } + + let charged = service.user_repository.get(&user.to_key()).unwrap(); + + assert!(matches!( + charged.can_deploy_station(), + CanDeployStation::QuotaExceeded + )); + // The quota is charged for the attempt; the station itself is only recorded on success. + assert!(charged.get_deployed_stations().is_empty()); + } + + #[tokio::test] + async fn consume_deploy_quota_advances_the_global_limiter() { + crate::core::test_utils::init_canister_config(); + + let user: User = mock_user(); + let ctx = CallContext::new(user.identity); + let service = UserService::default(); + + service.user_repository.insert(user.to_key(), user.clone()); + + let before = canister_config() + .unwrap() + .global_rate_limiter + .remaining_quota(); + + service + .consume_deploy_quota(&user.id, &ctx) + .expect("Failed to consume deploy quota"); + + let after = canister_config() + .unwrap() + .global_rate_limiter + .remaining_quota(); + + assert_eq!(before.unwrap() - 1, after.unwrap()); + } + #[tokio::test] async fn can_remove_user() { crate::core::test_utils::init_canister_config();