bug: Fix TOCTOU With Budget Leasing

docs/cookbook/config.md

@@ -226,6 +226,8 @@ keys you need to change.
 | `disk_bw_max_load` | `.disk_bw_max_load` | `.disk_bw_max_load` | `0.8`     | [$\beta$ (soft) budget](./architecture.md#budgets): disk-bandwidth load threshold, a fraction `0.0`–`1.0` of the cap. |
 | `net_bw_cap`       | `.net_bw_cap`       | `.net_bw_cap`       | `1 GiBps` | [$\beta$ (soft) budget](./architecture.md#budgets): soft network-bandwidth capacity ([unit](#unit)). |
 | `net_bw_max_load`  | `.net_bw_max_load`  | `.net_bw_max_load`  | `0.8`     | [$\beta$ (soft) budget](./architecture.md#budgets): network-bandwidth load threshold, a fraction `0.0`–`1.0` of the cap. |
+| `boot_lease_ttl`   | `.boot_lease_ttl`   | `.boot_lease_ttl`   | `300 s`   | How long a placement may hold admitted capacity before its VM has booted. A backstop for a wedged boot; must exceed the slowest legitimate boot, including a cold image pull. |
+| `restart_grace`    | `.restart_grace`    | `.restart_grace`    | `5 s`     | How long a crashed VM's capacity is held while its supervisor restarts it, so a competing placement cannot take capacity the VM is about to reclaim. |
 
 <!-- tabs open -->
 ### `config.exs`
@@ -239,7 +241,9 @@ config :hyper, Hyper.Cfg.Budget,
   disk_bw_cap: Unit.Bandwidth.gibps(1),
   disk_bw_max_load: 0.8,
   net_bw_cap: Unit.Bandwidth.gibps(1),
-  net_bw_max_load: 0.8
+  net_bw_max_load: 0.8,
+  boot_lease_ttl: Unit.Time.s(300),
+  restart_grace: Unit.Time.s(5)
 ```
 
 ### `config.toml`
@@ -254,6 +258,8 @@ disk_bw_cap = "1GiBps"
 disk_bw_max_load = 0.8
 net_bw_cap = "1GiBps"
 net_bw_max_load = 0.8
+boot_lease_ttl = "300s"
+restart_grace = "5s"
 ```
 <!-- tabs close -->
 

lib/hyper.ex

@@ -23,14 +23,13 @@ defmodule Hyper do
       instance_spec = Hyper.Vm.Instance.spec(spec.type)
 
       start_fun = fn -> Hyper.Node.start_image_vm(vm_id, spec) end
-      stop_fun = fn pid -> Hyper.Node.stop_image_vm(pid) end
 
       # Rank candidates by how many of the image's layer bytes they already
       # have mounted; a fork's freshly published delta is resident nowhere yet,
       # so its parent's base layers dominate the score — which is the point.
       layers = Hyper.Img.Db.Image.chain_sizes(spec.img_id)
 
-      case Hyper.Cluster.Scheduler.run(instance_spec, layers, start_fun, stop_fun) do
+      case Hyper.Cluster.Scheduler.run(vm_id, instance_spec, layers, start_fun) do
         {:ok, {_node, pid}} -> {:ok, pid}
         {:error, _} = err -> err
       end

lib/hyper/cfg/budget.ex

@@ -18,7 +18,9 @@ defmodule Hyper.Cfg.Budget do
           disk_bw_cap: Unit.Bandwidth.t(),
           disk_bw_max_load: float(),
           net_bw_cap: Unit.Bandwidth.t(),
-          net_bw_max_load: float()
+          net_bw_max_load: float(),
+          boot_lease_ttl: Unit.Time.t(),
+          restart_grace: Unit.Time.t()
         }
   defstruct [
     :mem_max,
@@ -28,7 +30,9 @@ defmodule Hyper.Cfg.Budget do
     :disk_bw_cap,
     :disk_bw_max_load,
     :net_bw_cap,
-    :net_bw_max_load
+    :net_bw_max_load,
+    :boot_lease_ttl,
+    :restart_grace
   ]
 
   @default_mem_max Unit.Information.gib(4)
@@ -40,6 +44,15 @@ defmodule Hyper.Cfg.Budget do
   @default_net_bw_cap Unit.Bandwidth.gibps(1)
   @default_net_bw_max_load 0.8
 
+  # A boot lease is a backstop for a caller that is alive but wedged, not the
+  # primary release mechanism (the monitor is). It must comfortably exceed the
+  # slowest legitimate boot — a cold image pull — because expiring a lease under
+  # a still-booting VM leaves it running unaccounted.
+  @default_boot_lease_ttl Unit.Time.s(300)
+  # Long enough to cover a `:transient` FireVMM restart, short enough that a VM
+  # that is never coming back frees its capacity promptly.
+  @default_restart_grace Unit.Time.s(5)
+
   @spec load :: {:ok, t()} | {:error, term()}
   def load do
     with {:ok, mem_max} <- information(:mem_max, "budget.mem_max", @default_mem_max),
@@ -52,7 +65,11 @@ defmodule Hyper.Cfg.Budget do
            number(:disk_bw_max_load, "budget.disk_bw_max_load", @default_disk_bw_max_load),
          {:ok, net_bw_cap} <- bandwidth(:net_bw_cap, "budget.net_bw_cap", @default_net_bw_cap),
          {:ok, net_bw_max_load} <-
-           number(:net_bw_max_load, "budget.net_bw_max_load", @default_net_bw_max_load) do
+           number(:net_bw_max_load, "budget.net_bw_max_load", @default_net_bw_max_load),
+         {:ok, boot_lease_ttl} <-
+           duration(:boot_lease_ttl, "budget.boot_lease_ttl", @default_boot_lease_ttl),
+         {:ok, restart_grace} <-
+           duration(:restart_grace, "budget.restart_grace", @default_restart_grace) do
       config = %__MODULE__{
         mem_max: mem_max,
         disk_max: disk_max,
@@ -61,7 +78,9 @@ defmodule Hyper.Cfg.Budget do
         disk_bw_cap: disk_bw_cap,
         disk_bw_max_load: disk_bw_max_load,
         net_bw_cap: net_bw_cap,
-        net_bw_max_load: net_bw_max_load
+        net_bw_max_load: net_bw_max_load,
+        boot_lease_ttl: boot_lease_ttl,
+        restart_grace: restart_grace
       }
 
       :persistent_term.put(__MODULE__, config)
@@ -88,6 +107,14 @@ defmodule Hyper.Cfg.Budget do
     end
   end
 
+  @spec duration(atom(), String.t(), Unit.Time.t()) ::
+          {:ok, Unit.Time.t()} | {:error, term()}
+  defp duration(key, toml, default) do
+    with {:ok, v} <- required(key, toml, default) do
+      coerce(v, &Unit.Time.parse/1, Unit.Time, key)
+    end
+  end
+
   @spec number(atom(), String.t(), number()) :: {:ok, number()} | {:error, term()}
   defp number(key, toml, default) do
     case required(key, toml, default) do

lib/hyper/cluster/scheduler.ex

@@ -5,7 +5,7 @@ defmodule Hyper.Cluster.Scheduler do
   nodes that cannot fit the spec, and ranks survivors by how many bytes of the
   VM's image layers they already have mounted (`colo(N, VM) = sum of |L|` over
   shared mounted layers). The result is an ordered candidate list; the chosen
-  node confirms authoritatively via `Hyper.Node.Budget.admit/2` (see `place/3`).
+  node confirms authoritatively via `Hyper.Node.Budget.lease/2` (see `place/3`).
 
   All filtering is best-effort on a possibly-stale snapshot: a node that no
   longer fits simply refuses at confirmation time.
@@ -63,20 +63,20 @@ defmodule Hyper.Cluster.Scheduler do
   @doc """
   Place and boot `spec` somewhere in the cluster.
 
-  Confirms each candidate by RPC-ing `Hyper.Node.try_run/3` on it; the first node
-  to boot the VM and reserve its budget wins. `start_fun`/`stop_fun` describe how
-  to boot/tear down the VM on the target node.
+  Confirms each candidate by RPC-ing `Hyper.Node.try_run/3` on it; the first
+  node to admit `spec` and boot it wins. A candidate that refuses has built
+  nothing, so walking the list is cheap.
   """
   @spec run(
+          Hyper.Vm.Id.t(),
           Spec.t(),
           layer_sizes(),
-          (-> {:ok, pid()} | {:error, term()}),
-          (pid() -> :ok)
+          (-> {:ok, pid()} | {:error, term()})
         ) :: {:ok, {node(), pid()}} | {:error, :no_capacity}
-  @decorate with_span("Hyper.Cluster.Scheduler.run", include: [:spec])
-  def run(spec, layers, start_fun, stop_fun) do
+  @decorate with_span("Hyper.Cluster.Scheduler.run", include: [:vm_id, :spec])
+  def run(vm_id, spec, layers, start_fun) do
     attempt = fn target ->
-      :erpc.call(target, Hyper.Node, :try_run, [spec, start_fun, stop_fun])
+      :erpc.call(target, Hyper.Node, :try_run, [vm_id, spec, start_fun])
     end
 
     place(spec, layers, attempt)

lib/hyper/node.ex

@@ -186,7 +186,7 @@ defmodule Hyper.Node do
       child_vm_id = Hyper.Vm.Id.generate()
       spec = Hyper.Vm.Instance.spec(parent.type)
 
-      try_run(spec, fn -> start_forked_vm(child_vm_id, parent) end, &stop_image_vm/1)
+      try_run(child_vm_id, spec, fn -> start_forked_vm(child_vm_id, parent) end)
     end
   end
 
@@ -269,35 +269,40 @@ defmodule Hyper.Node do
   end
 
   @doc """
-  Start a VM here and confirm its budget.
-
-  `start_fun` boots the VM and returns `{:ok, vm_pid}`; the reservation is held
-  against `vm_pid` and released when it dies. If the reserve loses a race (the
-  node filled up since the scheduler's snapshot) the just-started VM is torn down
-  via `stop_fun` and `{:error, reason}` is returned.
+  Admit `spec` on this node, then boot it.
+
+  Capacity is leased BEFORE `start_fun` runs, so a refusal costs nothing
+  physical and a concurrent herd cannot boot past the node's budget. The VM
+  turns that lease into its own reservation from `Hyper.Node.FireVMM.init/1`,
+  which `DynamicSupervisor.start_child` has already run by the time `start_fun`
+  returns — so this function drops its lease unconditionally and never owns the
+  reservation itself.
   """
-  @spec try_run(
-          Hyper.Vm.Instance.Spec.t(),
-          (-> {:ok, pid()} | {:error, term()}),
-          (pid() -> :ok)
-        ) :: {:ok, pid()} | {:error, term()}
-  def try_run(spec, start_fun, stop_fun) do
-    case start_fun.() do
-      {:ok, pid} ->
-        case Hyper.Node.Budget.admit(spec, pid) do
-          :ok ->
-            {:ok, pid}
-
-          {:error, reason} ->
-            :ok = stop_fun.(pid)
-            {:error, reason}
-        end
-
-      {:error, reason} ->
-        {:error, reason}
+  @spec try_run(Hyper.Vm.Id.t(), Hyper.Vm.Instance.Spec.t(), (-> {:ok, pid()} | {:error, term()})) ::
+          {:ok, pid()} | {:error, term()}
+  @decorate with_span("Hyper.Node.try_run", include: [:vm_id, :spec])
+  def try_run(vm_id, spec, start_fun) do
+    with {:ok, token} <- Hyper.Node.Budget.lease(vm_id, spec) do
+      try do
+        start_fun.()
+      after
+        release(vm_id, token)
+      end
     end
   end
 
+  # Dropping the lease must not turn a successful boot into a raise: `Hard` is a
+  # single GenServer, so a restart or a queue timeout here would otherwise
+  # propagate out of the `after` over a VM that is already live and claimed.
+  # Swallowing is safe because the lease is monitored on this process too, and
+  # this process exits as soon as `try_run/3` returns.
+  @spec release(Hyper.Vm.Id.t(), reference()) :: :ok
+  defp release(vm_id, token) do
+    Hyper.Node.Budget.drop(vm_id, token)
+  catch
+    :exit, _ -> :ok
+  end
+
   @spec test_system :: :ok | {:error, term()}
   def test_system do
     with {:ok, _} <- Hyper.Cfg.Budget.load(),
@@ -357,11 +362,29 @@ defmodule Hyper.Node do
     end
   end
 
-  defp start_vm_or_release(opts, uid, mutable) do
+  @doc false
+  # Public (not private) so the `:ignore` handling below is reachable from a
+  # hermetic test without a real jail — `boot_with_mutable/4`'s only caller.
+  @spec start_vm_or_release(FireVMM.Opts.t(), Users.id(), pid()) ::
+          {:ok, pid()} | {:error, term()}
+  def start_vm_or_release(opts, uid, mutable) do
     case start_vm(opts) do
       {:ok, pid} ->
         {:ok, pid}
 
+      # `FireVMM.init/1` declines with `:ignore` when either step of its `with`
+      # refuses: `Hyper.Cluster.Routing.register_self/1` finds the vm_id
+      # already registered (a stale dead incarnation), or
+      # `Hyper.Node.Budget.claim/2` has no lease left to claim (the lease
+      # expired or its holder died mid-boot). Either way it is a refusal, not
+      # a fault, but it must still hand back the uid and the mutable layer:
+      # nothing else will, and a leaked uid is unrecoverable short of
+      # restarting the node.
+      :ignore ->
+        Img.Mutable.release(mutable)
+        Users.release(uid)
+        {:error, :not_admitted}
+
       {:error, reason} ->
         Img.Mutable.release(mutable)
         Users.release(uid)

lib/hyper/node/budget.ex

@@ -1,33 +1,34 @@
 defmodule Hyper.Node.Budget do
   @moduledoc """
-  Public entry point for this node's resource budget. Thin facade over
-  `Hyper.Node.Budget.Hard`, the per-node accounting GenServer supervised by
-  `Hyper.Node.Budget.Supervisor`.
+  Public entry point for this node's resource budget, fronting leases. Thin
+  facade over `Hyper.Node.Budget.Hard`, the per-node accounting GenServer
+  supervised by `Hyper.Node.Budget.Supervisor`.
   """
 
   alias Hyper.Node.Budget.Hard
   alias Hyper.Vm.Instance.Spec
 
   use OpenTelemetryDecorator
 
-  @doc "Can this node run the given vm spec? `:ok` if yes, `{:error, reason}` otherwise."
-  @spec can_run(Hyper.Vm.Instance.Spec.t()) :: :ok | {:error, term()}
-  defdelegate can_run(vm_spec), to: Hard
-
-  @doc "Reserve the spec's budget, run `callable`, and release the budget afterwards."
-  @spec with_budget(Hyper.Vm.Instance.Spec.t(), (-> result)) :: result | {:error, term()}
-        when result: var
-  defdelegate with_budget(vm_spec, callable), to: Hard
-
   @doc """
-  Authoritatively confirm this node can run `spec`, reserving its budget for the
-  lifetime of `owner`. Live soft-load check first, then an atomic hard reserve.
+  Provisionally admit `spec` for `vm_id` on this node, before anything is built.
+
+  Live soft-load check first, then an atomic hard lease. Returns a token for
+  `drop/2`; the VM turns the lease into its own reservation with `claim/2`.
   """
-  @spec admit(Spec.t(), pid()) :: :ok | {:error, term()}
-  @decorate with_span("Hyper.Node.Budget.admit", include: [:spec])
-  def admit(spec, owner) do
+  @spec lease(Hyper.Vm.Id.t(), Spec.t()) :: {:ok, reference()} | {:error, term()}
+  @decorate with_span("Hyper.Node.Budget.lease", include: [:vm_id, :spec])
+  def lease(vm_id, spec) do
     with :ok <- Hyper.Node.Budget.Soft.can_run(spec) do
-      Hard.reserve(spec, owner)
+      Hard.lease(vm_id, spec)
     end
   end
+
+  @doc "Bind `vm_id`'s leased capacity to `owner` for that process's lifetime."
+  @spec claim(Hyper.Vm.Id.t(), pid()) :: :ok | {:error, :no_lease}
+  defdelegate claim(vm_id, owner), to: Hard
+
+  @doc "Release the lease `token` identifies. A no-op once the VM has claimed it."
+  @spec drop(Hyper.Vm.Id.t(), reference()) :: :ok
+  defdelegate drop(vm_id, token), to: Hard
 end