Merge pull request #121 from LNVPS/copilot/fix-deleted-vms-issue

Fix deleted VMs: race condition in cleanup and invoice/payment guards for deleted VMs

Author: v0l

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))
 }
 

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?;

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<MockDb>) -> Result<Worker> {
+        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<MockDb>,
+        created: DateTime<Utc>,
+        expires: DateTime<Utc>,
+    ) -> Result<Vm> {
+        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(())
+    }
+}

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(())
     }

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(())