Run migrations early in the supervision tree (#2241)

Also add a migration version, improve docs,
and document code that actually cannot be
removed.

lib/livebook/application.ex

@@ -18,6 +18,8 @@ defmodule Livebook.Application do
         {Task.Supervisor, name: Livebook.TaskSupervisor},
         # Start the storage module
         Livebook.Storage,
+        # Run migrations as soon as the storage is running
+        Livebook.Migration,
         # Start the periodic version check
         Livebook.UpdateCheck,
         # Periodic measurement of system resources
@@ -57,7 +59,6 @@ defmodule Livebook.Application do
 
     case Supervisor.start_link(children, opts) do
       {:ok, _} = result ->
-        Livebook.Migration.migrate()
         load_lb_env_vars()
         create_offline_hub()
         clear_env_vars()

lib/livebook/file_system/s3.ex

@@ -86,10 +86,7 @@ defmodule Livebook.FileSystem.S3 do
 
   def id(_, nil), do: nil
   def id(nil, bucket_url), do: hashed_id(bucket_url)
-
-  # For Personal hub we keep unprefixed ids
   def id(@personal_id, bucket_url), do: hashed_id(bucket_url)
-
   def id(hub_id, bucket_url), do: "#{hub_id}-#{hashed_id(bucket_url)}"
 
   defp hashed_id(bucket_url) do

lib/livebook/migration.ex

@@ -1,17 +1,40 @@
 defmodule Livebook.Migration do
-  @doc """
-  Runs all migrations.
-  """
-  @spec migrate() :: :ok
-  def migrate() do
-    delete_local_host_hub()
+  use GenServer, restart: :temporary
+
+  # We version our storage so we can remove migrations in the future
+  # by deleting the whole storage when finding too old versions.
+  #
+  # We also document tables and IDs used in previous versions,
+  # as those must be avoided in the future.
+  #
+  # ## v1 (Livebook v0.11, Oct 2023)
+  #
+  # * Deleted hubs.local-host
+  # * Migrated secrets to hub_secrets
+  # * Migrated filesystems to file_systems
+  # * Migrated settings.global.default_file_system_id to settings.global.default_dir
+  #
+  @migration_version 1
+
+  def start_link(_opts) do
+    GenServer.start_link(__MODULE__, :ok)
+  end
+
+  def init(:ok) do
     insert_personal_hub()
-    move_app_secrets_to_personal_hub()
+
+    # v1
     add_personal_hub_secret_key()
+    delete_local_host_hub()
+    move_app_secrets_to_personal_hub()
+
+    # TODO: remove on Livebook v0.12
     update_file_systems_to_deterministic_ids()
     ensure_new_file_system_attributes()
     move_default_file_system_id_to_default_dir()
-    :ok
+
+    Livebook.Storage.insert(:system, "global", migration_version: @migration_version)
+    :ignore
   end
 
   defp delete_local_host_hub() do
@@ -49,15 +72,14 @@ defmodule Livebook.Migration do
     end
   end
 
+  # We changed S3 file system ids, such that they are deterministic
+  # for the same bucket, rather than random. We take this opportunity
+  # to rename the scope from :filesystem to :file_systems, which
+  # conveniently allows for easy check if there's anything to migrate.
+  # This migration can be removed in the future (at the cost of discarding
+  # very old file systems (which can be re-added).
+  # TODO: remove on Livebook v0.12
   defp update_file_systems_to_deterministic_ids() do
-    # We changed S3 file system ids, such that they are deterministic
-    # for the same bucket, rather than random. We take this opportunity
-    # to rename the scope from :filesystem to :file_systems, which
-    # conveniently allows for easy check if there's anything to migrate.
-    # This migration can be removed in the future (at the cost of discarding
-    # very old file systems (which can be re-added).
-    # TODO: remove on Livebook v0.12
-
     case Livebook.Storage.all(:filesystem) do
       [] ->
         :ok
@@ -89,23 +111,14 @@ defmodule Livebook.Migration do
              {:ok, new_id} <- Map.fetch(id_mapping, default_file_system_id) do
           Livebook.Storage.insert(:settings, "global", default_file_system_id: new_id)
         end
-
-        # Livebook.NotebookManager is started before the migration runs,
-        # and it discards S3 files, since it can't find the file system.
-        # However, in this case it's fine; for recent notebooks it does
-        # not matter really and there are likely not many starred S3
-        # notebooks at this point (and the user can easily star again)
     end
   end
 
+  # Note that this is already handled by update_file_systems_to_deterministic_ids/0,
+  # we add this migration so it also applies to people using Livebook main.
+  # TODO: remove on Livebook v0.12
   defp ensure_new_file_system_attributes() do
-    # Note that this is already handled by update_file_systems_to_deterministic_ids/0,
-    # we add this migration so it also applies to people using Livebook main
-
-    # TODO: remove on Livebook v0.12
-
     for attrs <- Livebook.Storage.all(:file_systems) do
-      # Ensure new file system fields
       new_attrs = %{
         hub_id: Livebook.Hubs.Personal.id(),
         external_id: nil,
@@ -118,10 +131,8 @@ defmodule Livebook.Migration do
     end
   end
 
+  # TODO: remove on Livebook v0.12
   defp move_default_file_system_id_to_default_dir() do
-    # Convert default_file_system_id to default_dir
-    # TODO: remove on Livebook v0.12
-
     with {:ok, default_file_system_id} <-
            Livebook.Storage.fetch_key(:settings, "global", :default_file_system_id) do
       Livebook.Hubs.get_file_systems()

lib/livebook/notebook_manager.ex

@@ -256,7 +256,16 @@ defmodule Livebook.NotebookManager do
     end
   end
 
-  defp load_file(%{file_system_id: file_system_id, file_system_type: file_system_type, path: path}) do
+  defp load_file(%{file_system_id: file_system_id, path: path} = file) do
+    # In the past, not all files had a file_system_type, so we need to detect one from the id.
+    file_system_type =
+      Map.get_lazy(file, :file_system_type, fn ->
+        case file_system_id do
+          "local" -> "local"
+          "s3-" <> _ -> "s3"
+        end
+      end)
+
     %FileSystem.File{
       file_system_id: file_system_id,
       file_system_module: Livebook.FileSystems.type_to_module(file_system_type),
@@ -265,19 +274,6 @@ defmodule Livebook.NotebookManager do
     }
   end
 
-  # TODO: remove on Livebook v0.12
-  # NotebookManager starts before we run migrations, so we have a
-  # fallback here instead
-  defp load_file(%{file_system_id: file_system_id, path: path}) do
-    file_system_type =
-      case file_system_id do
-        "local" -> "local"
-        "s3-" <> _ -> "s3"
-      end
-
-    load_file(%{file_system_id: "local", path: path, file_system_type: file_system_type})
-  end
-
   defp load_datetime(datetime) do
     DateTime.from_unix!(datetime, :microsecond)
   end