Fix Vultr machine delete failure that caused VMs to leak

machine/providers/vultr.py

@@ -14,6 +14,13 @@
     "bom", "tlv",
 ]
 
+# Overall budget for destroy_vm, and for confirming a single accepted delete.
+# The overall budget stays well inside the callers' expectations of a command
+# that finishes in a few minutes.
+DESTROY_TIMEOUT = 240
+DESTROY_CONFIRM_TIMEOUT = 60
+DESTROY_POLL_INTERVAL = 5
+
 
 def _instance_to_vm(instance) -> VM:
     return VM(
@@ -68,24 +75,51 @@ def get_vm(self, vm_id) -> VM:
         return _instance_to_vm(result)
 
     def destroy_vm(self, vm_id) -> bool:
-        # Vultr returns HTTP 500 if the instance is still pending or locked
-        # (e.g. during provisioning). Retry deletion with backoff.
-        for attempt in range(24):
+        # Vultr will not delete an instance that is still provisioning. It has
+        # signalled that in several ways over time (HTTP 500 "not currently
+        # active", HTTP 400 "currently locked", and — worst of all — by
+        # accepting the DELETE and then not acting on it), so retry on anything
+        # that is not a definitive answer, and confirm the instance really went
+        # away before reporting success.
+        deadline = time.monotonic() + DESTROY_TIMEOUT
+        last_error = None
+        while time.monotonic() < deadline:
             try:
                 self._client.delete_instance(vm_id)
-                return True
             except VultrException as e:
-                error_msg = str(e)
-                if "500" in error_msg and ("not currently active" in error_msg or "currently locked" in error_msg):
-                    info("Waiting for instance to become ready before destroying...")
-                    time.sleep(5)
-                elif "404" in error_msg:
+                if e.status == 404:
                     return True  # already gone
-                else:
-                    fatal_error(f"Error: machine with id {vm_id} not found: {e}")
-        fatal_error(f"Error: timed out waiting to destroy instance {vm_id}")
+                if e.status in (401, 403):
+                    fatal_error(f"Error destroying machine with id {vm_id}: {e}")
+                last_error = e
+                info("Waiting for instance to become ready before destroying...")
+                time.sleep(DESTROY_POLL_INTERVAL)
+                continue
+
+            if self._instance_is_gone(vm_id):
+                return True
+            info("Instance still present after delete was accepted, retrying...")
+            time.sleep(DESTROY_POLL_INTERVAL)
+
+        if last_error:
+            fatal_error(f"Error: timed out destroying instance {vm_id}: {last_error}")
+        fatal_error(f"Error: timed out waiting for instance {vm_id} to be destroyed")
         return False
 
+    def _instance_is_gone(self, vm_id) -> bool:
+        """Poll the instance until the API reports it no longer exists."""
+        deadline = time.monotonic() + DESTROY_CONFIRM_TIMEOUT
+        while True:
+            try:
+                self._client.get_instance(vm_id)
+            except VultrException as e:
+                if e.status == 404:
+                    return True
+                raise
+            if time.monotonic() >= deadline:
+                return False
+            time.sleep(DESTROY_POLL_INTERVAL)
+
     def list_vms(self, tag=None) -> list:
         try:
             params = {"tag": tag} if tag else None

machine/subcommands/create.py

@@ -71,6 +71,12 @@ def command(context, name, tag, type, region, machine_size, image, wait_for_ip,
     # Verify SSH keys exist
     _verify_ssh_keys(provider, config.ssh_keys)
 
+    # Validate what will actually be used, not just what was passed on the
+    # command line, so that a bad value in the config file is caught too.
+    region = region if region is not None else config.region
+    image = image if image is not None else config.image
+    size = machine_size if machine_size is not None else config.machine_size
+
     provider.validate_region(region)
     provider.validate_image(image)
 
@@ -85,9 +91,9 @@ def command(context, name, tag, type, region, machine_size, image, wait_for_ip,
 
     vm = provider.create_vm(
         name=name,
-        region=region if region is not None else config.region,
-        image=image if image is not None else config.image,
-        size=machine_size if machine_size is not None else config.machine_size,
+        region=region,
+        image=image,
+        size=size,
         ssh_key_names=config.ssh_keys,
         tags=tags,
         user_data=user_data,

tests/E2E.md

@@ -173,6 +173,8 @@ If credentials for a provider are not configured, that provider's test run will
 
 Each test cleans up after itself. All test instances use unique names prefixed with `e2etest-` so they are easy to identify.
 
+A module-scoped `leak_check` fixture runs last and lists everything still tagged with the run's session id. Anything it finds is destroyed and then reported as a test failure — a leaked VM is a bug, not a warning, because a silent one accumulates until the provider's instance limit is reached (issue #102).
+
 ## Cost
 
 Tests use the smallest available instance size and destroy VMs immediately after verification, so cost is minimal.

tests/test_e2e.py

@@ -233,6 +233,57 @@ def session_id():
     return uuid.uuid4().hex[:8]
 
 
+def _list_session_instances(config_file, session_id):
+    """Return every instance this test session created that still exists.
+
+    ``list`` without ``--all`` is already filtered to the machines tagged with
+    this session id, so this sees exactly what the tests are responsible for.
+    """
+    result = run_machine("list", "--output", "json", config_file=config_file, session_id=session_id)
+    if result.returncode != 0:
+        return None  # cannot tell; do not claim a leak we did not observe
+    try:
+        return json.loads(result.stdout)
+    except json.JSONDecodeError:
+        return None
+
+
+@pytest.fixture(scope="module", autouse=True)
+def leak_check(config_file, session_id):
+    """Fail the run if any instance created by these tests outlives them.
+
+    A destroy that quietly fails used to leave one VM running per CI run until
+    the provider's instance limit was hit (issue #102).  The teardown below both
+    cleans up whatever survived and makes the leak visible.
+    """
+    yield
+
+    leftovers = _list_session_instances(config_file, session_id)
+    if not leftovers:
+        return
+
+    names = [f"{i['name']} ({i['id']})" for i in leftovers]
+    for leftover in leftovers:
+        run_machine(
+            "--verbose",
+            "destroy",
+            "--no-confirm",
+            "--delete-dns",
+            str(leftover["id"]),
+            config_file=config_file,
+            session_id=session_id,
+        )
+    still_there = _list_session_instances(config_file, session_id)
+    pytest.fail(
+        f"E2E tests leaked {len(leftovers)} instance(s): {', '.join(names)}. "
+        + (
+            f"{len(still_there)} still running after cleanup — delete manually."
+            if still_there
+            else "They were cleaned up by the leak check."
+        )
+    )
+
+
 @pytest.fixture(scope="class")
 def instance(config_file, session_id):
     """Create a single instance with all features and destroy it after all tests.
@@ -282,6 +333,8 @@ def instance(config_file, session_id):
         session_id=session_id,
     )
     if destroy_result.returncode != 0:
+        # Do not fail here — the module-scoped leak_check fixture decides that,
+        # after it has had a chance to clean up. But make the reason visible.
         print(f"TEARDOWN WARNING: destroy exited {destroy_result.returncode}", flush=True)
         print(f"  stdout: {destroy_result.stdout}", flush=True)
         print(f"  stderr: {destroy_result.stderr}", flush=True)

tests/test_vultr_destroy.py

@@ -0,0 +1,110 @@
+"""Tests for Vultr instance deletion.
+
+Vultr refuses to delete an instance that is still provisioning, and has been
+observed to accept the DELETE without acting on it, which leaked one VM per e2e
+run until the account instance limit was hit (issue #102).
+"""
+
+import json
+
+import pytest
+import requests
+
+from vultr import VultrException
+
+from machine.providers import vultr as vultr_module
+from machine.providers.vultr import VultrProvider
+
+
+def _exception(status, error="boom"):
+    response = requests.Response()
+    response.status_code = status
+    response.headers["content-type"] = "application/json"
+    response._content = json.dumps({"error": error}).encode()
+    return VultrException(response)
+
+
+class FakeClient:
+    """Stands in for vultr.Vultr, scripting the API's replies."""
+
+    def __init__(self, delete_results, get_results):
+        self.delete_results = list(delete_results)
+        self.get_results = list(get_results)
+        self.delete_calls = 0
+        self.get_calls = 0
+
+    def delete_instance(self, instance_id):
+        self.delete_calls += 1
+        result = self.delete_results.pop(0) if self.delete_results else None
+        if isinstance(result, Exception):
+            raise result
+        return result
+
+    def get_instance(self, instance_id):
+        self.get_calls += 1
+        result = self.get_results.pop(0) if self.get_results else _exception(404, "not found")
+        if isinstance(result, Exception):
+            raise result
+        return result
+
+
+@pytest.fixture
+def provider(monkeypatch):
+    """A provider whose clock only advances when the code sleeps.
+
+    destroy_vm budgets itself in minutes of wall clock, so the tests drive a
+    fake clock rather than waiting for it.
+    """
+    now = [0.0]
+    monkeypatch.setattr(vultr_module, "Vultr", lambda api_key: None)
+    monkeypatch.setattr(vultr_module.time, "monotonic", lambda: now[0])
+    monkeypatch.setattr(vultr_module.time, "sleep", lambda seconds: now.__setitem__(0, now[0] + seconds))
+    return VultrProvider({"api-key": "test-key"})
+
+
+def test_destroy_succeeds_when_instance_disappears(provider):
+    """The happy path: the delete is accepted and the instance goes away."""
+    provider._client = FakeClient(delete_results=[None], get_results=[_exception(404)])
+    assert provider.destroy_vm("abc") is True
+    assert provider._client.delete_calls == 1
+
+
+def test_destroy_retries_when_delete_is_accepted_but_ignored(provider):
+    """The leak from issue #102: DELETE returns 204 but the instance survives."""
+    provider._client = FakeClient(
+        delete_results=[None, None],
+        # First confirmation: still there for the whole confirm window, then gone.
+        get_results=[{"id": "abc"}] * 20 + [_exception(404)],
+    )
+    assert provider.destroy_vm("abc") is True
+    assert provider._client.delete_calls == 2
+
+
+@pytest.mark.parametrize("status", [400, 500])
+def test_destroy_retries_while_instance_is_locked(provider, status):
+    """Vultr has signalled 'still provisioning' with more than one status code."""
+    provider._client = FakeClient(
+        delete_results=[_exception(status, "instance is currently locked"), None],
+        get_results=[_exception(404)],
+    )
+    assert provider.destroy_vm("abc") is True
+    assert provider._client.delete_calls == 2
+
+
+def test_destroy_treats_missing_instance_as_success(provider):
+    provider._client = FakeClient(delete_results=[_exception(404, "not found")], get_results=[])
+    assert provider.destroy_vm("abc") is True
+
+
+def test_destroy_gives_up_immediately_on_auth_failure(provider):
+    """Retrying a rejected API key would only stall for the full timeout."""
+    provider._client = FakeClient(delete_results=[_exception(401, "invalid key")] * 10, get_results=[])
+    with pytest.raises(SystemExit):
+        provider.destroy_vm("abc")
+    assert provider._client.delete_calls == 1
+
+
+def test_destroy_fails_when_instance_never_goes_away(provider):
+    provider._client = FakeClient(delete_results=[None] * 500, get_results=[{"id": "abc"}] * 500)
+    with pytest.raises(SystemExit):
+        provider.destroy_vm("abc")