diff --git a/lnvps_api/src/api/routes.rs b/lnvps_api/src/api/routes.rs index 6c6aa4b..6740a28 100644 --- a/lnvps_api/src/api/routes.rs +++ b/lnvps_api/src/api/routes.rs @@ -1308,6 +1308,9 @@ async fn get_user_vm(auth: &Nip98Auth, this: &RouterState, id: u64) -> Result<(u if uid != vm.user_id { return Err(ApiError::new("VM does not belong to you")); } + if vm.deleted { + return Err(ApiError::new("VM not found")); + } Ok((uid, vm)) } diff --git a/lnvps_api/src/provisioner/rollback_tests.rs b/lnvps_api/src/provisioner/rollback_tests.rs index 6015f0a..75bb103 100644 --- a/lnvps_api/src/provisioner/rollback_tests.rs +++ b/lnvps_api/src/provisioner/rollback_tests.rs @@ -496,12 +496,11 @@ mod tests { provisioner.delete_vm(vm_id).await?; // Verify complete cleanup: - // Note: MockDb hard-deletes VMs, so we can't verify VM.deleted flag - // In production, the VM would be soft-deleted (deleted = true) + // VM should be soft-deleted (deleted = true), matching production MySQL behavior - // 1. VM should no longer be accessible (MockDb hard-delete) - let vm_get_result = db.get_vm(vm_id).await; - assert!(vm_get_result.is_err(), "VM should be deleted from MockDb"); + // 1. VM should be soft-deleted + let vm_after = db.get_vm(vm_id).await?; + assert!(vm_after.deleted, "VM should be deleted from MockDb"); // 2. IPs should be soft-deleted with refs cleared let ips_after = db.list_vm_ip_assignments(vm_id).await?; @@ -667,9 +666,9 @@ mod tests { let result = provisioner.delete_vm(vm_id).await; assert!(result.is_ok(), "Delete should succeed"); - // Verify VM is deleted (MockDb hard-deletes) - let vm_get_result = db.get_vm(vm_id).await; - assert!(vm_get_result.is_err(), "VM should be deleted from MockDb"); + // Verify VM is soft-deleted (matching production MySQL behavior) + let vm_after = db.get_vm(vm_id).await?; + assert!(vm_after.deleted, "VM should be deleted from MockDb"); // Verify IP assignments are marked as deleted let ips_after = db.list_vm_ip_assignments(vm_id).await?; diff --git a/lnvps_api/src/worker.rs b/lnvps_api/src/worker.rs index 717ee87..6b82fa9 100644 --- a/lnvps_api/src/worker.rs +++ b/lnvps_api/src/worker.rs @@ -487,14 +487,35 @@ impl Worker { // Process deletions first for vm in vms_to_delete { - info!("Deleting unpaid VM {}", vm.id); - if let Err(e) = self.provisioner.delete_vm(vm.id).await { - error!("Failed to delete unpaid VM {}: {}", vm.id, e); - self.queue_admin_notification( - format!("Failed to delete unpaid VM {}:\n{}", vm.id, e), - Some(format!("VM {} Deletion Failed", vm.id)), - ) - .await + // Re-read the VM from the database to guard against a race condition where a + // payment was confirmed between the initial list_vms() snapshot and now. + // Only proceed with deletion if the VM is still in the unpaid (new) state. + match self.db.get_vm(vm.id).await { + Ok(current_vm) if current_vm.created == current_vm.expires => { + info!("Deleting unpaid VM {}", vm.id); + if let Err(e) = self.provisioner.delete_vm(vm.id).await { + error!("Failed to delete unpaid VM {}: {}", vm.id, e); + self.queue_admin_notification( + format!("Failed to delete unpaid VM {}:\n{}", vm.id, e), + Some(format!("VM {} Deletion Failed", vm.id)), + ) + .await + } + } + Ok(_) => { + info!( + "VM {} was paid since last check, skipping deletion", + vm.id + ); + } + Err(e) => { + error!("Failed to re-read VM {} before deletion: {}", vm.id, e); + self.queue_admin_notification( + format!("Failed to re-read VM {} before deletion:\n{}", vm.id, e), + Some(format!("VM {} Pre-Deletion Read Failed", vm.id)), + ) + .await + } } } @@ -2350,3 +2371,105 @@ impl Worker { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::mocks::{MockDnsServer, MockNode}; + use crate::settings::mock_settings; + use crate::provisioner::LNVpsProvisioner; + use lnvps_api_common::{MockDb, MockExchangeRate}; + use lnvps_db::{LNVpsDbBase, UserSshKey, Vm}; + + async fn setup_worker(db: Arc) -> Result { + let settings = mock_settings(); + let node = Arc::new(MockNode::default()); + let rates = Arc::new(MockExchangeRate::new()); + let dns = MockDnsServer::new(); + let provisioner = Arc::new(LNVpsProvisioner::new( + settings.clone(), + db.clone(), + node, + rates, + Some(Arc::new(dns)), + )); + let cache = VmStateCache::new(); + Worker::new(db, provisioner, &settings, cache, None).await + } + + async fn add_vm_with_state( + db: &Arc, + created: DateTime, + expires: DateTime, + ) -> Result { + let pubkey: [u8; 32] = rand::random(); + let user_id = db.upsert_user(&pubkey).await?; + let ssh_key_id = db + .insert_user_ssh_key(&UserSshKey { + id: 0, + name: "test".to_string(), + user_id, + created: Utc::now(), + key_data: "ssh-rsa AAA==".into(), + }) + .await?; + let vm = Vm { + id: 0, + host_id: 1, + user_id, + image_id: 1, + template_id: Some(1), + custom_template_id: None, + ssh_key_id, + created, + expires, + disk_id: 1, + mac_address: "ff:ff:ff:ff:ff:ff".to_string(), + deleted: false, + ref_code: None, + auto_renewal_enabled: false, + disabled: false, + }; + let vm_id = db.insert_vm(&vm).await?; + Ok(db.get_vm(vm_id).await?) + } + + /// An unpaid VM (created == expires) that is older than 1 hour must be deleted by check_vms. + #[tokio::test] + async fn test_check_vms_deletes_unpaid_vm_after_one_hour() -> Result<()> { + let db = Arc::new(MockDb::default()); + let old = Utc::now().sub(TimeDelta::hours(2)); + let vm = add_vm_with_state(&db, old, old).await?; + let vm_id = vm.id; + + let worker = setup_worker(db.clone()).await?; + worker.check_vms().await?; + + // VM should be soft-deleted + let vms = db.vms.lock().await; + let deleted = vms.get(&vm_id).map(|v| v.deleted).unwrap_or(false); + assert!(deleted, "Unpaid VM older than 1 hour should be deleted"); + Ok(()) + } + + /// An unpaid VM that was created less than 1 hour ago must NOT be deleted by check_vms. + #[tokio::test] + async fn test_check_vms_skips_unpaid_vm_within_one_hour() -> Result<()> { + let db = Arc::new(MockDb::default()); + let recent = Utc::now().sub(TimeDelta::minutes(30)); + let vm = add_vm_with_state(&db, recent, recent).await?; + let vm_id = vm.id; + + let worker = setup_worker(db.clone()).await?; + worker.check_vms().await?; + + // VM should still be present and not deleted + let vms = db.vms.lock().await; + let deleted = vms.get(&vm_id).map(|v| v.deleted).unwrap_or(true); + assert!( + !deleted, + "Unpaid VM younger than 1 hour should not be deleted" + ); + Ok(()) + } +} diff --git a/lnvps_api_common/src/mock.rs b/lnvps_api_common/src/mock.rs index 8cb4adb..471555e 100644 --- a/lnvps_api_common/src/mock.rs +++ b/lnvps_api_common/src/mock.rs @@ -661,7 +661,9 @@ impl LNVpsDbBase for MockDb { async fn delete_vm(&self, vm_id: u64) -> DbResult<()> { let mut vms = self.vms.lock().await; - vms.remove(&vm_id); + if let Some(vm) = vms.get_mut(&vm_id) { + vm.deleted = true; + } Ok(()) } @@ -834,8 +836,10 @@ impl LNVpsDbBase for MockDb { p.is_paid = true; p.paid_at = Some(Utc::now()); } - if let Some(v) = v.get_mut(&payment.vm_id) { - v.expires = v.expires.add(TimeDelta::seconds(payment.time_value as i64)); + if let Some(vm) = v.get_mut(&payment.vm_id) { + // Un-delete the VM if it was deleted (e.g. auto-cleaned up before payment arrived) + vm.deleted = false; + vm.expires = vm.expires.add(TimeDelta::seconds(payment.time_value as i64)); } Ok(()) } diff --git a/lnvps_db/src/mysql.rs b/lnvps_db/src/mysql.rs index bae7f7a..fb8c49f 100644 --- a/lnvps_db/src/mysql.rs +++ b/lnvps_db/src/mysql.rs @@ -799,11 +799,15 @@ impl LNVpsDbBase for LNVpsDbMysql { .execute(&mut *tx) .await?; - sqlx::query("update vm set expires = TIMESTAMPADD(SECOND, ?, expires) where id = ?") - .bind(vm_payment.time_value) - .bind(vm_payment.vm_id) - .execute(&mut *tx) - .await?; + // Un-delete the VM if it was deleted (e.g. auto-cleaned up before payment arrived) + // and extend its expiry. This handles payment methods with longer timeouts. + sqlx::query( + "update vm set expires = TIMESTAMPADD(SECOND, ?, expires), deleted = 0 where id = ?", + ) + .bind(vm_payment.time_value) + .bind(vm_payment.vm_id) + .execute(&mut *tx) + .await?; tx.commit().await?; Ok(())