From 68f0720f857d312c744fc28b08d47212dfcb35ad Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Sat, 10 Oct 2026 22:55:59 +0200 Subject: [PATCH 1/4] improvement: manage tenant databases, so `strategy :context` needs no `tenant_repo` of its own Follows #224, which added the `tenant_repo` option and no implementation of it. This PR adds the runtime, so that `strategy :context` needs no tenant repo of your own -- what @zachdaniel asked for on #224: context multitenancy that manages its own storage rather than leaving it to the reader. children = [ MyApp.Repo, {AshSqlite.MultiTenancy, repo: MyApp.Repo, dir: "priv/tenants", migrations_path: "priv/repo/tenant_migrations"} ] `AshSqlite.MultiTenancy.TenantRepo` becomes the default `tenant_repo` for a `strategy :context` resource, so `VerifyTenantRepo`'s compile-time requirement to name one goes with it. Naming your own still overrides it, which is what keeps Turso, Litestream or a replica-aware router implementable outside this library. The default implements `with_repo/4` as well as `repo/3`, so the tenant is held open for the length of each statement. Named after `AshPostgres.MultiTenancy` rather than inventing a word for it. - `AshSqlite.MultiTenancy.Registry` -- one connection per tenant, and only one. - `AshSqlite.MultiTenancy.Connection` -- opens the file, migrates it, and refuses to serve until both have happened. - `AshSqlite.MultiTenancy.Manager` -- activation, an LRU bound on residency that will exceed itself rather than close a tenant mid-statement, quarantine for tenants that cannot open, and seal/unseal for draining a node. - `AshSqlite.MultiTenancy.Database` -- the tenant-to-filename mapping, escaped rather than sanitised so that it is injective: sanitising maps `a:b` and `a_b` onto one file, which puts two tenants in one database. - `AshSqlite.MultiTenancy.Migrations` -- compiles a migration directory once per boot rather than once per tenant. `all_tenants/1` answers what AshPostgres asks the application for through `Repo.all_tenants/0`; here it is derived, because a tenant is a file. Residents are unioned in, since SQLite creates the file on the first write. `rename/3` moves a tenant's database, sidecars included. Without it a renamed tenant keeps none of its data: the new name addresses a file that does not exist, and the next request quietly creates an empty one. The sidecars are not belt-and-braces -- a write is still in the WAL when the rename happens. No documentation in this PR, as with #224. The guide covering both is a separate piece of work. --- lib/data_layer.ex | 7 +- lib/data_layer/info.ex | 14 +- lib/multi_tenancy.ex | 153 +++++++ lib/multi_tenancy/binds.ex | 123 ++++++ lib/multi_tenancy/connection.ex | 175 ++++++++ lib/multi_tenancy/connection_supervisor.ex | 36 ++ lib/multi_tenancy/database.ex | 140 ++++++ lib/multi_tenancy/manager.ex | 367 ++++++++++++++++ lib/multi_tenancy/migrations.ex | 85 ++++ lib/multi_tenancy/registry.ex | 65 +++ lib/multi_tenancy/tenant_repo.ex | 32 ++ lib/multi_tenancy/unavailable_error.ex | 20 + lib/verifiers/verify_tenant_repo.ex | 39 -- mix.exs | 4 +- test/multi_tenancy/binds_test.exs | 167 +++++++ test/multi_tenancy/connection_test.exs | 301 +++++++++++++ test/multi_tenancy/database_test.exs | 213 +++++++++ test/multi_tenancy/managed_test.exs | 232 ++++++++++ test/multi_tenancy/multi_tenancy_test.exs | 483 +++++++++++++++++++++ test/multi_tenancy/registry_test.exs | 115 +++++ test/multitenancy_test.exs | 38 -- test/support/domain.ex | 1 + test/support/managed_tenant_repo.ex | 18 + test/support/resources/managed_post.ex | 30 ++ 24 files changed, 2774 insertions(+), 84 deletions(-) create mode 100644 lib/multi_tenancy.ex create mode 100644 lib/multi_tenancy/binds.ex create mode 100644 lib/multi_tenancy/connection.ex create mode 100644 lib/multi_tenancy/connection_supervisor.ex create mode 100644 lib/multi_tenancy/database.ex create mode 100644 lib/multi_tenancy/manager.ex create mode 100644 lib/multi_tenancy/migrations.ex create mode 100644 lib/multi_tenancy/registry.ex create mode 100644 lib/multi_tenancy/tenant_repo.ex create mode 100644 lib/multi_tenancy/unavailable_error.ex delete mode 100644 lib/verifiers/verify_tenant_repo.ex create mode 100644 test/multi_tenancy/binds_test.exs create mode 100644 test/multi_tenancy/connection_test.exs create mode 100644 test/multi_tenancy/database_test.exs create mode 100644 test/multi_tenancy/managed_test.exs create mode 100644 test/multi_tenancy/multi_tenancy_test.exs create mode 100644 test/multi_tenancy/registry_test.exs create mode 100644 test/support/managed_tenant_repo.ex create mode 100644 test/support/resources/managed_post.ex diff --git a/lib/data_layer.ex b/lib/data_layer.ex index f065a7a..c012b3d 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -313,8 +313,7 @@ defmodule AshSqlite.DataLayer do AshSqlite.Verifiers.ValidateReferences, AshSqlite.Verifiers.VerifyRepo, AshSqlite.Verifiers.EnsureTableOrPolymorphic, - AshSqlite.Verifiers.VerifyGlobalMultitenancy, - AshSqlite.Verifiers.VerifyTenantRepo + AshSqlite.Verifiers.VerifyGlobalMultitenancy ] def migrate(args) do @@ -2290,8 +2289,8 @@ defmodule AshSqlite.DataLayer do defp with_tenant_repo(resource, tenant, type, fun) do case AshSqlite.DataLayer.Info.tenant_repo(resource) do # No tenant repo and a tenant means multitenancy this data layer does not - # resolve to a connection, such as `strategy :attribute`. `VerifyTenantRepo` - # covers `strategy :context` at compile time. + # resolve to a connection, such as `strategy :attribute`. `strategy :context` + # always has one, by default `AshSqlite.MultiTenancy.TenantRepo`. nil -> fun.() diff --git a/lib/data_layer/info.ex b/lib/data_layer/info.ex index ae8f6d8..537ddbe 100644 --- a/lib/data_layer/info.ex +++ b/lib/data_layer/info.ex @@ -69,12 +69,22 @@ defmodule AshSqlite.DataLayer.Info do @doc """ The tenant repo for a resource as `{module, opts}`, or nil. - Only set for resources with context multitenancy. A function given to the + Only set for resources with context multitenancy, where it defaults to + `AshSqlite.MultiTenancy.TenantRepo` so that database-per-tenant works without + the application supplying a runtime of its own. A function given to the `tenant_repo` option is returned wrapped in a module implementing `AshSqlite.TenantRepo`. """ def tenant_repo(resource) do - Extension.get_opt(resource, [:sqlite], :tenant_repo, nil, true) + case Extension.get_opt(resource, [:sqlite], :tenant_repo, nil, true) do + nil -> + if Ash.Resource.Info.multitenancy_strategy(resource) == :context do + {AshSqlite.MultiTenancy.TenantRepo, []} + end + + tenant_repo -> + tenant_repo + end end @doc "The configured table for a resource" diff --git a/lib/multi_tenancy.ex b/lib/multi_tenancy.ex new file mode 100644 index 0000000..b97320d --- /dev/null +++ b/lib/multi_tenancy.ex @@ -0,0 +1,153 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy do + @moduledoc """ + One SQLite database per tenant: choosing the file, opening it, migrating it, and closing it again. + """ + use Supervisor + + alias AshSqlite.MultiTenancy.Binds + alias AshSqlite.MultiTenancy.Connection + alias AshSqlite.MultiTenancy.Manager + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + @doc false + def child_spec(opts) do + %{ + id: {__MODULE__, Keyword.fetch!(opts, :repo)}, + start: {__MODULE__, :start_link, [opts]}, + type: :supervisor + } + end + + def start_link(opts) do + repo = Keyword.fetch!(opts, :repo) + Supervisor.start_link(__MODULE__, opts, name: Module.concat(repo, MultiTenancy)) + end + + @impl true + def init(opts) do + repo = Keyword.fetch!(opts, :repo) + _dir = Keyword.fetch!(opts, :dir) + verify_migrations_path!(Keyword.get(opts, :migrations_path)) + + children = [ + TenantRegistry.child_spec(repo), + Binds.child_spec(repo), + AshSqlite.MultiTenancy.ConnectionSupervisor.child_spec(repo), + Manager.child_spec(opts) + ] + + Supervisor.init(children, strategy: :one_for_one) + end + + @doc "The repo instance serving `tenant`, starting it if it is not resident." + @spec connection_for(module(), String.t()) :: {:ok, pid()} | {:error, term()} + def connection_for(repo, tenant) do + case TenantRegistry.lookup(repo, tenant) do + {:ok, _connection, repo_pid} when is_pid(repo_pid) -> + {:ok, repo_pid} + + # Registered, but its `init/1` has not published a repo yet, so this caller + # raced the activation. Asking the connection directly waits for it. + {:ok, connection, nil} -> + case Connection.repo_pid(connection) do + repo_pid when is_pid(repo_pid) -> {:ok, repo_pid} + _ -> Manager.activate(repo, tenant) + end + + :error -> + Manager.activate(repo, tenant) + end + end + + @doc "Runs `fun` with `tenant`'s database bound to the calling process." + @spec with_tenant(module(), String.t(), (-> result)) :: result when result: var + def with_tenant(repo, tenant, fun) when is_function(fun, 0) do + case connection_for(repo, tenant) do + {:ok, repo_pid} -> + bound(repo, tenant, repo_pid, fun) + + # Raising, not returning: an unbound statement here would run against whichever + # database the process already had, which is another tenant's data. + {:error, reason} -> + raise AshSqlite.MultiTenancy.UnavailableError, tenant: tenant, reason: reason + end + end + + defp bound(repo, tenant, repo_pid, fun) do + case Binds.bound(repo, tenant) do + :ok -> + previous = repo.put_dynamic_repo(repo_pid) + + try do + fun.() + after + repo.put_dynamic_repo(previous) + Binds.released(repo, tenant) + end + + # Being closed, so wait for it to land and bind whatever replaces it. + :closing -> + Process.sleep(1) + with_tenant(repo, tenant, fun) + end + end + + @doc "Closes a tenant's database, leaving the file on disk." + defdelegate close(repo, tenant, opts \\ []), to: Manager + + @doc "Closes a tenant's database and deletes it, WAL sidecars included." + defdelegate delete(repo, tenant), to: Manager + + @doc "Moves a tenant's database to another tenant's name, sidecars included." + defdelegate rename(repo, from, to), to: Manager + + @doc "Tenants holding an open connection right now." + @spec resident(module()) :: [String.t()] + defdelegate resident(repo), to: TenantRegistry + + @doc "Every tenant with a database on disk, plus any that are resident." + defdelegate all_tenants(repo), to: Manager + + @doc "Tenants that failed to activate, with the reason." + defdelegate quarantined(repo), to: Manager + + @doc "Clears a tenant's quarantine, so the next request tries again." + defdelegate release(repo, tenant), to: Manager + + @doc "Stops this node taking on new tenants." + defdelegate seal(repo), to: Manager + + @doc "Lets this node accept tenants again." + defdelegate unseal(repo), to: Manager + + @doc "Activates and migrates each tenant in turn, reporting what happened." + defdelegate migrate_all(repo, tenants \\ :all, opts \\ []), to: Manager + + @doc "Where a tenant's database is, whether or not it exists yet." + defdelegate path_for(repo, tenant), to: Manager + + @doc "The fleet configuration." + defdelegate config(repo), to: Manager + + # Checked at boot rather than at activation: a typo would otherwise surface in + # production as `no such table`, at whatever hour the first tenant woke up. + defp verify_migrations_path!(nil), do: :ok + + defp verify_migrations_path!(path) do + if File.dir?(path) do + :ok + else + raise ArgumentError, """ + :migrations_path #{inspect(path)} is not a directory. + + Tenant databases are migrated from it, and `Ecto.Migrator` treats a missing \ + directory as one containing no migrations -- so every tenant would open an \ + empty database and fail on its first query instead of here. + """ + end + end +end diff --git a/lib/multi_tenancy/binds.ex b/lib/multi_tenancy/binds.ex new file mode 100644 index 0000000..02b9288 --- /dev/null +++ b/lib/multi_tenancy/binds.ex @@ -0,0 +1,123 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Binds do + @moduledoc """ + Counts who is using each tenant, so that closing one can be safe. + """ + use GenServer + + @doc false + def child_spec(repo) do + %{id: {__MODULE__, repo}, start: {__MODULE__, :start_link, [repo]}} + end + + @doc false + def start_link(repo), do: GenServer.start_link(__MODULE__, repo, name: name(repo)) + + @doc "The table serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantBinds) + + @doc "Records that the calling process has bound `tenant`." + @spec bound(module(), String.t()) :: :ok | :closing + def bound(repo, tenant) do + if closing?(repo, tenant) do + :closing + else + :ets.update_counter(name(repo), {:binds, tenant}, {2, 1}, {{:binds, tenant}, 0}) + :ok + end + end + + @doc "Records that the calling process has released `tenant`, and marks it used." + @spec released(module(), String.t()) :: :ok + def released(repo, tenant) do + # Clamped at zero: an unmatched release would make a busy tenant look evictable. + :ets.update_counter(name(repo), {:binds, tenant}, {2, -1, 0, 0}, {{:binds, tenant}, 0}) + touch(repo, tenant) + end + + @doc "How many processes are bound to `tenant`." + @spec count(module(), String.t()) :: non_neg_integer() + def count(repo, tenant) do + case :ets.lookup(name(repo), {:binds, tenant}) do + [{_, count}] -> count + [] -> 0 + end + end + + @doc "Marks `tenant` as used now." + @spec touch(module(), String.t()) :: :ok + def touch(repo, tenant) do + :ets.insert(name(repo), {{:used, tenant}, System.monotonic_time()}) + :ok + end + + @doc "When `tenant` was last used, in `System.monotonic_time/0` units." + @spec last_used(module(), String.t()) :: integer() | nil + def last_used(repo, tenant) do + case :ets.lookup(name(repo), {:used, tenant}) do + [{_, at}] -> at + [] -> nil + end + end + + @doc "The tenant among `candidates` that should be evicted, if any." + @spec least_recently_used(module(), [String.t()]) :: String.t() | nil + def least_recently_used(repo, candidates) do + candidates + |> Enum.reject(&(count(repo, &1) > 0 or closing?(repo, &1))) + |> Enum.min_by(&sort_key(repo, &1), fn -> nil end) + end + + # Never-used sorts first, and cannot be spelled as a sentinel timestamp: + # `System.monotonic_time/0` is normally negative, so 0 would read as recent. + defp sort_key(repo, tenant) do + case last_used(repo, tenant) do + nil -> {0, 0} + at -> {1, at} + end + end + + @doc "Marks `tenant` as closing, so that `bound/2` refuses it." + @spec begin_closing(module(), String.t()) :: :ok + def begin_closing(repo, tenant) do + :ets.insert(name(repo), {{:closing, tenant}, true}) + :ok + end + + @doc "Clears the closing mark for `tenant`." + @spec end_closing(module(), String.t()) :: :ok + def end_closing(repo, tenant) do + :ets.delete(name(repo), {:closing, tenant}) + :ok + end + + @doc "Whether `tenant` is being closed right now." + @spec closing?(module(), String.t()) :: boolean() + def closing?(repo, tenant), do: :ets.member(name(repo), {:closing, tenant}) + + @doc "Drops everything recorded about `tenant`." + @spec forget(module(), String.t()) :: :ok + def forget(repo, tenant) do + table = name(repo) + Enum.each([:binds, :used, :closing], &:ets.delete(table, {&1, tenant})) + :ok + end + + @impl true + def init(repo) do + # Public: binds happen in arbitrary caller processes, not in this one. + :ets.new(name(repo), [ + :named_table, + :public, + :set, + read_concurrency: true, + write_concurrency: true + ]) + + {:ok, repo} + end +end diff --git a/lib/multi_tenancy/connection.ex b/lib/multi_tenancy/connection.ex new file mode 100644 index 0000000..367c6be --- /dev/null +++ b/lib/multi_tenancy/connection.ex @@ -0,0 +1,175 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Connection do + @moduledoc """ + One tenant's database, owned by one process for as long as the tenant is resident. + """ + use GenServer, restart: :temporary + + require Logger + + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + defstruct [:tenant, :repo, :repo_pid, :path, :schema_version, :opened_at] + + @doc "Starts a connection for a tenant." + def start_link(opts) do + repo = Keyword.fetch!(opts, :repo) + tenant = Keyword.fetch!(opts, :tenant) + + GenServer.start_link(__MODULE__, opts, name: TenantRegistry.via(repo, tenant)) + end + + @doc "The repo instance pid for this tenant." + def repo_pid(connection), do: GenServer.call(connection, :repo_pid) + + @doc "What this connection is serving, for the fleet view." + def info(connection), do: GenServer.call(connection, :info) + + @doc "Stops the repo instance while leaving this process alive." + def stop_repo(connection), do: GenServer.call(connection, :stop_repo) + + @impl true + def init(opts) do + repo = Keyword.fetch!(opts, :repo) + tenant = Keyword.fetch!(opts, :tenant) + path = Keyword.fetch!(opts, :path) + key = Keyword.get(opts, :key) + + # The registry value is a repo pid, so this process must not outlive its repo. + Process.flag(:trap_exit, true) + File.mkdir_p!(Path.dirname(path)) + + migrations_path = Keyword.get(opts, :migrations_path) + + with :ok <- verify_key(key, Keyword.get(opts, :encrypted?, false)), + :ok <- verify_migrations_path(migrations_path), + {:ok, repo_pid} <- start_repo(repo, path, key, Keyword.get(opts, :repo_opts, [])), + {:ok, version} <- migrate(repo, repo_pid, tenant, migrations_path) do + :ok = TenantRegistry.publish(repo, tenant, repo_pid) + + {:ok, + %__MODULE__{ + tenant: tenant, + repo: repo, + repo_pid: repo_pid, + path: path, + schema_version: version, + opened_at: System.monotonic_time(:millisecond) + }} + else + {:error, reason} -> {:stop, reason} + end + end + + @impl true + def handle_call(:repo_pid, _from, state), do: {:reply, state.repo_pid, state} + + def handle_call(:info, _from, state) do + {:reply, Map.take(state, [:tenant, :path, :schema_version, :opened_at]), state} + end + + def handle_call(:stop_repo, _from, state) do + stop_repo_instance(state.repo_pid) + + # Cleared so that a second call, and `terminate/2`, are no-ops rather than a + # stop against a pid that is already gone. + {:reply, :ok, %{state | repo_pid: nil}} + end + + @impl true + def handle_info({:EXIT, repo_pid, reason}, %{repo_pid: repo_pid} = state) do + {:stop, {:repo_exited, reason}, %{state | repo_pid: nil}} + end + + def handle_info({:EXIT, _pid, _reason}, state), do: {:noreply, state} + + @impl true + def terminate(_reason, state), do: stop_repo_instance(state.repo_pid) + + defp start_repo(repo, path, key, extra) do + opts = + extra + # backoff_type: :stop — a wrong key or a corrupt file is not transient, and + # retrying turns a clear failure into a hang. + |> Keyword.merge(name: nil, database: path, backoff_type: :stop) + # One writer per file is all SQLite offers; at one, contention waits in the + # pool rather than reaching SQLite's write lock, which fails instead of waiting. + |> Keyword.put(:pool_size, 1) + |> maybe_put_key(key) + + case repo.start_link(opts) do + {:ok, repo_pid} -> {:ok, repo_pid} + {:error, {:already_started, repo_pid}} -> {:ok, repo_pid} + {:error, reason} -> {:error, {:cannot_open_database, reason}} + end + end + + defp maybe_put_key(opts, nil), do: opts + defp maybe_put_key(opts, key), do: Keyword.put(opts, :key, key) + + # SQLite will create a *plaintext* database for a tenant whose key is missing. + defp verify_key(nil, true), do: {:error, :no_key} + defp verify_key(_key, _encrypted?), do: :ok + + # `Ecto.Migrator` treats a missing directory as one with no migrations, which would + # leave every tenant empty and surface as `no such table` far from the cause. + defp verify_migrations_path(nil), do: :ok + + defp verify_migrations_path(path) do + if File.dir?(path), do: :ok, else: {:error, {:missing_migrations_path, path}} + end + + defp migrate(_repo, _repo_pid, _tenant, nil), do: {:ok, nil} + + defp migrate(repo, repo_pid, tenant, migrations_path) do + opts = [ + dynamic_repo: repo_pid, + # Registration already reduced this to one process per file, and with a pool of + # one there is no second connection for Ecto's lock to take. + migration_lock: false, + log: false, + log_migrations_sql: false + ] + + source = AshSqlite.MultiTenancy.Migrations.load!(migrations_path) + statuses = Ecto.Migrator.migrations(repo, source, opts) + + # Only run the migrator when something is pending: the common case is an + # up-to-date tenant being activated. + if Enum.any?(statuses, &match?({:down, _, _}, &1)) do + Ecto.Migrator.run(repo, source, :up, Keyword.put(opts, :all, true)) + end + + {:ok, current_version(statuses)} + rescue + exception -> + Logger.error(""" + migrating tenant #{inspect(tenant)} raised #{inspect(exception.__struct__)}: \ + #{Exception.message(exception)} + """) + + {:error, {:migration_failed, exception.__struct__}} + end + + # Every version in the directory, because anything pending has just been run. + defp current_version([]), do: nil + defp current_version(statuses), do: statuses |> Enum.map(&elem(&1, 1)) |> Enum.max() + + defp stop_repo_instance(nil), do: :ok + + defp stop_repo_instance(repo_pid) do + if Process.alive?(repo_pid) do + # A clean stop closes the connection, which checkpoints the WAL back into the + # database. A kill leaves it to be replayed by whoever opens the file next. + Supervisor.stop(repo_pid) + end + + :ok + catch + # The repo is already going down, which is the state we wanted. + :exit, _ -> :ok + end +end diff --git a/lib/multi_tenancy/connection_supervisor.ex b/lib/multi_tenancy/connection_supervisor.ex new file mode 100644 index 0000000..ba753a0 --- /dev/null +++ b/lib/multi_tenancy/connection_supervisor.ex @@ -0,0 +1,36 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.ConnectionSupervisor do + @moduledoc """ + Supervises the connection processes of one repo's tenants. + """ + + @doc false + def child_spec(repo) do + %{id: {__MODULE__, repo}, start: {__MODULE__, :start_link, [repo]}, type: :supervisor} + end + + @doc false + def start_link(repo) do + DynamicSupervisor.start_link(strategy: :one_for_one, name: name(repo)) + end + + @doc "The supervisor serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantConnectionSupervisor) + + @doc "Starts a connection, or reports the one already serving that tenant." + @spec start_connection(module(), keyword()) :: + {:ok, pid()} | {:error, {:already_started, pid()}} | {:error, term()} + def start_connection(repo, opts) do + DynamicSupervisor.start_child(name(repo), {AshSqlite.MultiTenancy.Connection, opts}) + end + + @doc "Stops a connection, letting it close its database cleanly." + @spec stop_connection(module(), pid()) :: :ok | {:error, :not_found} + def stop_connection(repo, connection) do + DynamicSupervisor.terminate_child(name(repo), connection) + end +end diff --git a/lib/multi_tenancy/database.ex b/lib/multi_tenancy/database.ex new file mode 100644 index 0000000..541b0c9 --- /dev/null +++ b/lib/multi_tenancy/database.ex @@ -0,0 +1,140 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Database do + @moduledoc """ + Turns a tenant into the name of its database file. The encoding is reversible, so no two tenants can name one file. + """ + + @unreserved ~c"abcdefghijklmnopqrstuvwxyz0123456789-_." + + # 255 is the usual per-name limit. `-shm` is the longest suffix a SQLite + # database adds to its own name, and `.db` is ours, so this is what is left. + @max_encoded_bytes 255 - byte_size(".db") - byte_size("-shm") + + @doc "Encodes `tenant` as a path-safe name, reversibly." + @spec encode(String.t()) :: String.t() + def encode(tenant) when is_binary(tenant) and byte_size(tenant) > 0 do + encoded = tenant |> :binary.bin_to_list() |> Enum.map_join(&encode_byte/1) + + if byte_size(encoded) > @max_encoded_bytes do + raise ArgumentError, """ + tenant #{inspect(tenant)} encodes to #{byte_size(encoded)} bytes, and a \ + database name can be at most #{@max_encoded_bytes}. + + Bytes outside #{inspect(to_string(@unreserved))} take three bytes each once \ + escaped, so a tenant of mostly-escaped bytes reaches the limit at about \ + #{div(@max_encoded_bytes, 3)} characters. Use a shorter identifier — a slug \ + or a UUID — as the tenant, and keep the display name in a column. + """ + end + + encoded + end + + def encode(tenant) when is_binary(tenant) do + raise ArgumentError, "the tenant of an AshSqlite resource cannot be an empty string" + end + + def encode(other) do + raise ArgumentError, """ + expected a binary tenant, got: #{inspect(other)} + + A tenant names a database file, and a non-binary tenant would have to be \ + stringified to do that. Doing so here would let two different tenants -- \ + #{inspect(other)} and #{inspect(to_string_safe(other))} -- name one file while \ + remaining two tenants to Ash, so their rows would share a database with \ + nothing raising. + + Convert it where you set the tenant instead. + """ + end + + @doc "Recovers the tenant from a name produced by `encode/1`." + @spec decode(String.t()) :: {:ok, String.t()} | :error + def decode(encoded) when is_binary(encoded), do: decode(encoded, []) + + @doc "The absolute path of `tenant`'s database inside `dir`." + @spec path(Path.t(), String.t()) :: Path.t() + def path(dir, tenant), do: Path.join(dir, encode(tenant) <> ".db") + + @doc "Every file that makes up `path`, including the ones SQLite adds." + @spec sidecars(Path.t()) :: [Path.t()] + def sidecars(path), do: [path, path <> "-wal", path <> "-shm"] + + @doc "The tenant a database file inside `dir` belongs to." + @spec tenant_from_path(Path.t(), Path.t()) :: {:ok, String.t()} | :error + def tenant_from_path(dir, path) do + with {:ok, relative} <- relative_to(dir, path), + {:ok, base} <- strip_extension(relative) do + decode(base) + end + end + + @doc "Every tenant with a database in `dir`, in no particular order." + @spec list(Path.t()) :: [String.t()] + def list(dir) do + case File.ls(dir) do + {:ok, entries} -> + for entry <- entries, + {:ok, tenant} <- [tenant_from_path(dir, Path.join(dir, entry))], + do: tenant + + {:error, _} -> + [] + end + end + + defp relative_to(dir, path) do + case Path.relative_to(path, dir) do + ^path -> :error + relative -> if Path.dirname(relative) == ".", do: {:ok, relative}, else: :error + end + end + + defp strip_extension(name) do + case Path.extname(name) do + ".db" -> {:ok, Path.rootname(name, ".db")} + _ -> :error + end + end + + defp encode_byte(byte) when byte in @unreserved, do: <> + + defp encode_byte(byte) do + <> = <> + <> + end + + # Lowercase, so that the encoded alphabet as a whole is lowercase and two + # encodings can never differ only by case. See the moduledoc. + defp hex(nibble) when nibble < 10, do: ?0 + nibble + defp hex(nibble), do: ?a + nibble - 10 + + defp decode(<<>>, []), do: :error + defp decode(<<>>, acc), do: {:ok, acc |> Enum.reverse() |> IO.iodata_to_binary()} + + defp decode(<>, acc) do + with {:ok, hi} <- unhex(hi), {:ok, lo} <- unhex(lo) do + decode(rest, [<> | acc]) + end + end + + defp decode(<>, _acc), do: :error + + defp decode(<>, acc) when byte in @unreserved, + do: decode(rest, [byte | acc]) + + defp decode(_, _acc), do: :error + + defp unhex(char) when char in ?0..?9, do: {:ok, char - ?0} + defp unhex(char) when char in ?a..?f, do: {:ok, char - ?a + 10} + defp unhex(_char), do: :error + + defp to_string_safe(term) do + to_string(term) + rescue + _ -> inspect(term) + end +end diff --git a/lib/multi_tenancy/manager.ex b/lib/multi_tenancy/manager.ex new file mode 100644 index 0000000..76ad1e5 --- /dev/null +++ b/lib/multi_tenancy/manager.ex @@ -0,0 +1,367 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Manager do + @moduledoc """ + Activates tenants, bounds how many stay resident, and quarantines the ones that cannot start. + """ + use GenServer + + require Logger + + alias AshSqlite.MultiTenancy.Binds + alias AshSqlite.MultiTenancy.ConnectionSupervisor + alias AshSqlite.MultiTenancy.Database + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + @default_max_resident 256 + + defstruct [ + :repo, + :dir, + :key_for, + :migrations_path, + :max_resident, + :repo_opts, + quarantined: %{}, + sealed?: false + ] + + @doc false + def child_spec(opts) do + %{id: {__MODULE__, Keyword.fetch!(opts, :repo)}, start: {__MODULE__, :start_link, [opts]}} + end + + @doc false + def start_link(opts) do + GenServer.start_link(__MODULE__, opts, name: name(Keyword.fetch!(opts, :repo))) + end + + @doc "The manager serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantManager) + + @doc "Returns the repo instance serving `tenant`, starting it if it is not resident." + @spec activate(module(), String.t()) :: {:ok, pid()} | {:error, term()} + def activate(repo, tenant), do: activate(repo, tenant, 50) + + defp activate(repo, tenant, attempts_left) do + case GenServer.call(name(repo), {:activate, tenant}, 30_000) do + {:error, :connection_died} -> retry_activate(repo, tenant, attempts_left) + other -> other + end + end + + # A registration is released when the registry handles the connection's DOWN, so a + # tenant whose connection just died is briefly impossible to start. Yield and retry. + defp retry_activate(_repo, _tenant, 0), do: {:error, :connection_died} + + defp retry_activate(repo, tenant, attempts_left) do + Process.sleep(1) + activate(repo, tenant, attempts_left - 1) + end + + @doc "Closes a tenant's database, leaving the file on disk." + @spec close(module(), String.t(), keyword()) :: :ok + def close(repo, tenant, opts \\ []) do + # A caller that had already marked the tenant closing wants it to stay that way + # afterwards -- `rename/3` holds the mark across the close *and* the file move. + held_open_by_caller? = Binds.closing?(repo, tenant) + Binds.begin_closing(repo, tenant) + + try do + unless Keyword.get(opts, :force, false) do + await_quiescence(repo, tenant, Keyword.get(opts, :grace_ms, 1_000)) + end + + case TenantRegistry.lookup(repo, tenant) do + {:ok, connection, _repo_pid} -> ConnectionSupervisor.stop_connection(repo, connection) + :error -> :ok + end + + Binds.forget(repo, tenant) + :ok + after + unless held_open_by_caller?, do: Binds.end_closing(repo, tenant) + end + end + + @doc "Closes a tenant and deletes its database, including the WAL sidecars." + @spec delete(module(), String.t()) :: {:ok, [Path.t()]} + def delete(repo, tenant) do + close(repo, tenant, force: true) + GenServer.call(name(repo), {:delete, tenant}) + end + + @doc """ + Moves a tenant's database to another tenant's name, sidecars included. + + A tenant here is a file, so a tenant renamed without this keeps none of its data: + the new name addresses a database that does not exist, and the next request + creates an empty one. Both names are held closed for the move, so nothing reopens + the file being moved or creates a database at the destination while it happens. + + Refuses rather than overwrites when the destination already has a database. + """ + @spec rename(module(), String.t(), String.t()) :: + :ok | {:error, :no_database | :target_exists | File.posix()} + def rename(_repo, tenant, tenant), do: :ok + + def rename(repo, from, to) do + Binds.begin_closing(repo, from) + Binds.begin_closing(repo, to) + + try do + close(repo, from) + GenServer.call(name(repo), {:rename, from, to}) + after + Binds.end_closing(repo, from) + Binds.end_closing(repo, to) + end + end + + @doc "Stops the node taking on new tenants. The first step of a drain." + @spec seal(module()) :: :ok + def seal(repo), do: GenServer.call(name(repo), :seal) + + @doc "Lets the node accept tenants again." + @spec unseal(module()) :: :ok + def unseal(repo), do: GenServer.call(name(repo), :unseal) + + @doc "Whether the node is refusing new tenants." + @spec sealed?(module()) :: boolean() + def sealed?(repo), do: GenServer.call(name(repo), :sealed?) + + @doc "Tenants that failed to activate, with the reason." + @spec quarantined(module()) :: %{String.t() => term()} + def quarantined(repo), do: GenServer.call(name(repo), :quarantined) + + @doc "Clears a tenant's quarantine, so the next request tries again." + @spec release(module(), String.t()) :: :ok + def release(repo, tenant), do: GenServer.call(name(repo), {:release, tenant}) + + @doc "The fleet configuration." + @spec config(module()) :: map() + def config(repo), do: GenServer.call(name(repo), :config) + + @doc "Where `tenant`'s database is, whether or not it exists yet." + @spec path_for(module(), String.t()) :: Path.t() + def path_for(repo, tenant), do: GenServer.call(name(repo), {:path_for, tenant}) + + @doc """ + Every tenant this node knows of: one with a database in the fleet's directory, + or one currently resident. + + Derived rather than asked of the application, unlike AshPostgres' + `all_tenants/0`, because a tenant here *is* a file. Residents are included + because SQLite creates the file on the first write, so a tenant activated a + moment ago may not have one yet. A tenant that has never been activated is + absent, which is also why it needs no migrating -- it is migrated when it is + first opened. + """ + @spec all_tenants(module()) :: [String.t()] + def all_tenants(repo) do + on_disk = repo |> config() |> Map.fetch!(:dir) |> Database.list() + + Enum.uniq(on_disk ++ TenantRegistry.resident(repo)) + end + + @doc "Activates each tenant in turn, migrating it, and reports what happened." + @spec migrate_all(module(), [String.t()] | :all, keyword()) :: + [{String.t(), {:ok, term()} | {:error, term()}}] + def migrate_all(repo, tenants \\ :all, opts \\ []) + + def migrate_all(repo, :all, opts), do: migrate_all(repo, all_tenants(repo), opts) + + def migrate_all(repo, tenants, opts) do + close_after? = Keyword.get(opts, :close_after?, true) + + Enum.map(tenants, fn tenant -> + result = + with {:ok, _repo_pid} <- activate(repo, tenant), + {:ok, connection, _} <- TenantRegistry.lookup(repo, tenant) do + version = AshSqlite.MultiTenancy.Connection.info(connection).schema_version + if close_after?, do: close(repo, tenant) + {:ok, version} + else + :error -> {:error, :connection_died} + {:error, reason} -> {:error, reason} + end + + {tenant, result} + end) + end + + @impl true + def init(opts) do + state = %__MODULE__{ + repo: Keyword.fetch!(opts, :repo), + dir: Keyword.fetch!(opts, :dir), + key_for: Keyword.get(opts, :key_for), + migrations_path: Keyword.get(opts, :migrations_path), + max_resident: Keyword.get(opts, :max_resident, @default_max_resident), + repo_opts: Keyword.get(opts, :repo_opts, []) + } + + File.mkdir_p!(state.dir) + {:ok, state} + end + + @impl true + def handle_call({:activate, _tenant}, _from, %{sealed?: true} = state) do + {:reply, {:error, :draining}, state} + end + + def handle_call({:activate, tenant}, _from, state) + when is_map_key(state.quarantined, tenant) do + {:reply, {:error, {:quarantined, state.quarantined[tenant]}}, state} + end + + def handle_call({:activate, tenant}, _from, state) do + case TenantRegistry.lookup(state.repo, tenant) do + {:ok, connection, repo_pid} -> + {:reply, {:ok, repo_pid || AshSqlite.MultiTenancy.Connection.repo_pid(connection)}, state} + + :error -> + state = evict_if_needed(state) + result = start(state, tenant) + {:reply, result, quarantine_on_failure(state, tenant, result)} + end + end + + def handle_call({:delete, tenant}, _from, state) do + base = Database.path(state.dir, tenant) + + # Closing checkpoints and may remove the sidecars itself, so existence is not + # stable between a check and a removal. Attempt each and report what went. + removed = for path <- Database.sidecars(base), File.rm(path) == :ok, do: path + + {:reply, {:ok, removed}, state} + end + + def handle_call({:rename, from, to}, _from, state) do + source = Database.path(state.dir, from) + target = Database.path(state.dir, to) + + cond do + not File.exists?(source) -> + {:reply, {:error, :no_database}, state} + + File.exists?(target) -> + {:reply, {:error, :target_exists}, state} + + true -> + {:reply, move(source, target), + %{state | quarantined: Map.delete(state.quarantined, from)}} + end + end + + def handle_call(:seal, _from, state), do: {:reply, :ok, %{state | sealed?: true}} + def handle_call(:unseal, _from, state), do: {:reply, :ok, %{state | sealed?: false}} + def handle_call(:sealed?, _from, state), do: {:reply, state.sealed?, state} + def handle_call(:quarantined, _from, state), do: {:reply, state.quarantined, state} + + def handle_call({:release, tenant}, _from, state) do + {:reply, :ok, %{state | quarantined: Map.delete(state.quarantined, tenant)}} + end + + def handle_call(:config, _from, state) do + {:reply, Map.take(state, [:repo, :dir, :migrations_path, :max_resident]), state} + end + + def handle_call({:path_for, tenant}, _from, state) do + {:reply, Database.path(state.dir, tenant), state} + end + + defp start(state, tenant) do + opts = [ + repo: state.repo, + tenant: tenant, + path: Database.path(state.dir, tenant), + key: key_for(state, tenant), + # `encrypted?` tells the connection "this fleet uses keys", so that a tenant + # whose key is missing fails instead of quietly opening a plaintext database. + encrypted?: not is_nil(state.key_for), + migrations_path: state.migrations_path, + repo_opts: state.repo_opts + ] + + case ConnectionSupervisor.start_connection(state.repo, opts) do + {:ok, connection} -> published(state, tenant, connection) + {:error, {:already_started, connection}} -> published(state, tenant, connection) + {:error, reason} -> {:error, reason} + end + end + + defp published(state, tenant, connection) do + if Process.alive?(connection) do + # Marked used at activation, or a tenant that is about to be queried would + # have no timestamp and sort first for eviction. + Binds.touch(state.repo, tenant) + + case TenantRegistry.lookup(state.repo, tenant) do + {:ok, _connection, repo_pid} when is_pid(repo_pid) -> {:ok, repo_pid} + _ -> {:error, :connection_died} + end + else + {:error, :connection_died} + end + end + + defp quarantine_on_failure(state, tenant, {:error, reason}) + when reason not in [:connection_died] do + %{state | quarantined: Map.put(state.quarantined, tenant, reason)} + end + + defp quarantine_on_failure(state, _tenant, _result), do: state + + defp key_for(%{key_for: nil}, _tenant), do: nil + defp key_for(%{key_for: fun}, tenant), do: fun.(tenant) + + defp evict_if_needed(state) do + if TenantRegistry.count(state.repo) >= state.max_resident do + case Binds.least_recently_used(state.repo, TenantRegistry.resident(state.repo)) do + nil -> + Logger.warning(""" + #{inspect(state.repo)} holds #{TenantRegistry.count(state.repo)} tenant \ + databases, at or above max_resident of #{state.max_resident}, and every \ + one of them is in use. Exceeding the limit rather than refusing the \ + tenant that is arriving. + """) + + tenant -> + # No wait: the candidate was chosen because nothing is bound to it. + close(state.repo, tenant, force: true) + end + end + + state + end + + # The sidecars only exist between a write and a checkpoint, but when they do they + # hold committed data the database file does not -- so a move that left them + # behind would lose the most recent writes. + defp move(source, target) do + with :ok <- File.rename(source, target) do + for {sidecar, renamed} <- Enum.zip(Database.sidecars(source), Database.sidecars(target)), + sidecar != source, + File.exists?(sidecar) do + File.rename(sidecar, renamed) + end + + :ok + end + end + + defp await_quiescence(_repo, _tenant, grace_ms) when grace_ms <= 0, do: :ok + + defp await_quiescence(repo, tenant, grace_ms) do + if Binds.count(repo, tenant) == 0 do + :ok + else + Process.sleep(1) + await_quiescence(repo, tenant, grace_ms - 1) + end + end +end diff --git a/lib/multi_tenancy/migrations.ex b/lib/multi_tenancy/migrations.ex new file mode 100644 index 0000000..06a2212 --- /dev/null +++ b/lib/multi_tenancy/migrations.ex @@ -0,0 +1,85 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Migrations do + @moduledoc """ + Compiles a directory of migrations once, rather than once per tenant. + """ + + @doc "The `{version, module}` pairs in `path`, compiling them if needed." + @spec load!(Path.t()) :: [{integer(), module()}] + def load!(path) do + # Keyed by a fingerprint of the directory, so adding a migration invalidates the + # cache without a restart. + fingerprint = fingerprint(path) + + case :persistent_term.get(key(path), nil) do + {^fingerprint, loaded} -> loaded + _ -> compile!(path, fingerprint) + end + end + + @doc "Forgets the compiled migrations for `path`. For tests." + @spec forget(Path.t()) :: :ok + def forget(path) do + :persistent_term.erase(key(path)) + :ok + end + + # `:persistent_term` because this is written once per directory per boot and read + # on every activation. Its cost is in writes, which is why residency is not stored + # here. + defp compile!(path, fingerprint) do + loaded = path |> files() |> Enum.map(&load_file!/1) |> Enum.sort() + :persistent_term.put(key(path), {fingerprint, loaded}) + loaded + end + + # Mirrors `Ecto.Migrator`'s own naming rules: `_.exs`, and a file + # ending in `.ex` is not a migration however much it looks like one. + defp files(path) do + [path, "**", "*.exs"] + |> Path.join() + |> Path.wildcard() + |> Enum.filter(&version_of(&1)) + end + + defp version_of(file) do + case Integer.parse(Path.rootname(Path.basename(file))) do + {version, "_" <> _name} -> version + _ -> nil + end + end + + defp load_file!(file) do + modules = file |> Code.compile_file() |> Enum.map(&elem(&1, 0)) + + case Enum.find(modules, &migration?/1) do + nil -> + raise Ecto.MigrationError, + "file #{Path.relative_to_cwd(file)} does not define an Ecto.Migration" + + module -> + {version_of(file), module} + end + end + + defp migration?(module) do + Code.ensure_loaded?(module) and function_exported?(module, :__migration__, 0) + end + + defp fingerprint(path) do + path + |> files() + |> Enum.map(fn file -> + case File.stat(file, time: :posix) do + {:ok, stat} -> {file, stat.size, stat.mtime} + {:error, reason} -> {file, reason} + end + end) + |> Enum.sort() + end + + defp key(path), do: {__MODULE__, Path.expand(path)} +end diff --git a/lib/multi_tenancy/registry.ex b/lib/multi_tenancy/registry.ex new file mode 100644 index 0000000..dd6efad --- /dev/null +++ b/lib/multi_tenancy/registry.ex @@ -0,0 +1,65 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.Registry do + @moduledoc """ + Maps a tenant to the process holding its connection, and to that connection's repo instance. + """ + + @doc false + def child_spec(repo) do + %{id: {__MODULE__, repo}, start: {__MODULE__, :start_link, [repo]}, type: :supervisor} + end + + @doc false + def start_link(repo) do + Registry.start_link( + keys: :unique, + name: name(repo), + # Every tenanted statement takes a lookup, so reads are the hot path. + # Every tenanted statement does a lookup, so reads are the hot path and the + # partitions are what keep them off one ETS table. + partitions: System.schedulers_online() + ) + end + + @doc "The registry serving `repo`." + @spec name(module()) :: module() + def name(repo), do: Module.concat(repo, TenantRegistry) + + @doc "The name to start a tenant's connection process under." + @spec via(module(), String.t()) :: {:via, module(), {module(), String.t(), nil}} + def via(repo, tenant), do: {:via, Registry, {name(repo), tenant, nil}} + + @doc "Publishes the repo instance pid for `tenant`." + @spec publish(module(), String.t(), pid()) :: :ok + def publish(repo, tenant, repo_pid) do + Registry.update_value(name(repo), tenant, fn _ -> repo_pid end) + :ok + end + + @doc "The connection process and repo instance serving `tenant`." + @spec lookup(module(), String.t()) :: {:ok, pid(), pid() | nil} | :error + def lookup(repo, tenant) do + case Registry.lookup(name(repo), tenant) do + # An entry outlives its process until the partition handles the DOWN, so a + # hit is not proof of life and a dead pid must read as absent. + [{connection, repo_pid}] -> + if Process.alive?(connection), do: {:ok, connection, repo_pid}, else: :error + + [] -> + :error + end + end + + @doc "Tenants with a connection open on this node right now." + @spec resident(module()) :: [String.t()] + def resident(repo) do + Registry.select(name(repo), [{{:"$1", :_, :_}, [], [:"$1"]}]) + end + + @doc "How many tenants are resident." + @spec count(module()) :: non_neg_integer() + def count(repo), do: Registry.count(name(repo)) +end diff --git a/lib/multi_tenancy/tenant_repo.ex b/lib/multi_tenancy/tenant_repo.ex new file mode 100644 index 0000000..5f141e3 --- /dev/null +++ b/lib/multi_tenancy/tenant_repo.ex @@ -0,0 +1,32 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.TenantRepo do + @moduledoc """ + Implements `AshSqlite.TenantRepo` in terms of `AshSqlite.MultiTenancy`. The default for `strategy :context`. + """ + + @behaviour AshSqlite.TenantRepo + + @impl true + def repo(tenant, %{resource: resource}, _opts) do + repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + + case AshSqlite.MultiTenancy.connection_for(repo, tenant) do + {:ok, repo_pid} -> + repo_pid + + {:error, reason} -> + raise AshSqlite.MultiTenancy.UnavailableError, tenant: tenant, reason: reason + end + end + + # Implemented so the tenant is held open for the statement, which `repo/3` alone + # cannot do. + @impl true + def with_repo(tenant, %{resource: resource}, _opts, fun) do + repo = AshSqlite.DataLayer.Info.repo(resource, :mutate) + AshSqlite.MultiTenancy.with_tenant(repo, tenant, fun) + end +end diff --git a/lib/multi_tenancy/unavailable_error.ex b/lib/multi_tenancy/unavailable_error.ex new file mode 100644 index 0000000..ddf3c21 --- /dev/null +++ b/lib/multi_tenancy/unavailable_error.ex @@ -0,0 +1,20 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.UnavailableError do + @moduledoc """ + Raised when a tenant's database cannot be reached, rather than letting a statement run unbound. + """ + defexception [:tenant, :reason] + + @impl true + def message(%{tenant: tenant, reason: reason}) do + """ + the database for tenant #{inspect(tenant)} is unavailable: #{inspect(reason)} + + No statement was run. Each tenant is its own SQLite file, so running without \ + one would have run against another tenant's data. + """ + end +end diff --git a/lib/verifiers/verify_tenant_repo.ex b/lib/verifiers/verify_tenant_repo.ex deleted file mode 100644 index d8441b8..0000000 --- a/lib/verifiers/verify_tenant_repo.ex +++ /dev/null @@ -1,39 +0,0 @@ -# SPDX-FileCopyrightText: 2023 ash_sqlite contributors -# -# SPDX-License-Identifier: MIT - -defmodule AshSqlite.Verifiers.VerifyTenantRepo do - @moduledoc false - use Spark.Dsl.Verifier - - alias Spark.Dsl.Verifier - alias Spark.Error.DslError - - @impl true - def verify(dsl) do - if Ash.Resource.Info.multitenancy_strategy(dsl) == :context and - is_nil(Verifier.get_option(dsl, [:sqlite], :tenant_repo)) do - {:error, - DslError.exception( - module: Verifier.get_persisted(dsl, :module), - path: [:sqlite, :tenant_repo], - message: """ - `strategy :context` needs a `tenant_repo`. - - Each tenant has its own database file, so a tenanted statement has to be \ - given the repo instance for its tenant. Only the application knows how to \ - map a tenant to a repo instance. - - sqlite do - repo MyApp.Repo - tenant_repo &MyApp.Tenants.repo/2 - end - - See `AshSqlite.TenantRepo`.\ - """ - )} - else - :ok - end - end -end diff --git a/mix.exs b/mix.exs index 120b48c..3f9f1fd 100644 --- a/mix.exs +++ b/mix.exs @@ -120,7 +120,9 @@ defmodule AshSqlite.MixProject do AshSqlite.DataLayer ], Multitenancy: [ - AshSqlite.TenantRepo + AshSqlite.TenantRepo, + AshSqlite.MultiTenancy, + AshSqlite.MultiTenancy.UnavailableError ], Utilities: [ AshSqlite.ManualRelationship diff --git a/test/multi_tenancy/binds_test.exs b/test/multi_tenancy/binds_test.exs new file mode 100644 index 0000000..b0498b0 --- /dev/null +++ b/test/multi_tenancy/binds_test.exs @@ -0,0 +1,167 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.BindsTest do + @moduledoc """ + A bound tenant is never evictable, and a closing tenant accepts no new binds. + """ + use ExUnit.Case, async: true + + alias AshSqlite.MultiTenancy.Binds + + defmodule Repo do + @moduledoc false + end + + setup do + start_supervised!(Binds.child_spec(Repo)) + :ok + end + + describe "bound/2 and released/2" do + test "count the processes using a tenant" do + assert Binds.count(Repo, "acme") == 0 + + assert :ok = Binds.bound(Repo, "acme") + assert :ok = Binds.bound(Repo, "acme") + assert Binds.count(Repo, "acme") == 2 + + Binds.released(Repo, "acme") + assert Binds.count(Repo, "acme") == 1 + end + + test "count tenants separately" do + Binds.bound(Repo, "acme") + + assert Binds.count(Repo, "acme") == 1 + assert Binds.count(Repo, "globex") == 0 + end + + test "clamp at zero, so an unmatched release cannot make a busy tenant look idle" do + Binds.released(Repo, "acme") + Binds.released(Repo, "acme") + assert Binds.count(Repo, "acme") == 0 + + Binds.bound(Repo, "acme") + assert Binds.count(Repo, "acme") == 1 + end + + test "releasing marks the tenant used" do + refute Binds.last_used(Repo, "acme") + + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + + assert is_integer(Binds.last_used(Repo, "acme")) + end + end + + describe "closing" do + test "refuses new binds while it is marked" do + Binds.begin_closing(Repo, "acme") + + assert :closing = Binds.bound(Repo, "acme") + assert Binds.count(Repo, "acme") == 0 + end + + test "accepts binds again once cleared" do + Binds.begin_closing(Repo, "acme") + Binds.end_closing(Repo, "acme") + + assert :ok = Binds.bound(Repo, "acme") + end + + test "marks one tenant without affecting another" do + Binds.begin_closing(Repo, "acme") + + assert Binds.closing?(Repo, "acme") + refute Binds.closing?(Repo, "globex") + assert :ok = Binds.bound(Repo, "globex") + end + + test "is idempotent" do + Binds.begin_closing(Repo, "acme") + Binds.begin_closing(Repo, "acme") + Binds.end_closing(Repo, "acme") + + refute Binds.closing?(Repo, "acme") + end + end + + describe "least_recently_used/2" do + test "picks the tenant used longest ago, not the one started longest ago" do + for tenant <- ["acme", "globex", "initech"] do + Binds.bound(Repo, tenant) + Binds.released(Repo, tenant) + end + + # "acme" was started first but has just been used, so it must not be chosen. + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + + assert Binds.least_recently_used(Repo, ["acme", "globex", "initech"]) == "globex" + end + + test "never picks a tenant with work in flight" do + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + Binds.bound(Repo, "globex") + Binds.released(Repo, "globex") + + # The least recently used tenant is now busy, so the next one must be chosen. + Binds.bound(Repo, "acme") + + assert Binds.least_recently_used(Repo, ["acme", "globex"]) == "globex" + end + + test "never picks a tenant that is already closing" do + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + Binds.bound(Repo, "globex") + Binds.released(Repo, "globex") + Binds.begin_closing(Repo, "acme") + + assert Binds.least_recently_used(Repo, ["acme", "globex"]) == "globex" + end + + test "is nil when every candidate is in use, rather than choosing one anyway" do + Binds.bound(Repo, "acme") + Binds.bound(Repo, "globex") + + refute Binds.least_recently_used(Repo, ["acme", "globex"]) + end + + test "is nil for no candidates" do + refute Binds.least_recently_used(Repo, []) + end + + test "prefers a tenant never used over one used recently" do + Binds.bound(Repo, "globex") + Binds.released(Repo, "globex") + + assert Binds.least_recently_used(Repo, ["acme", "globex"]) == "acme" + end + end + + describe "forget/2" do + test "drops everything recorded about a tenant" do + Binds.bound(Repo, "acme") + Binds.released(Repo, "acme") + Binds.begin_closing(Repo, "acme") + + Binds.forget(Repo, "acme") + + assert Binds.count(Repo, "acme") == 0 + refute Binds.last_used(Repo, "acme") + refute Binds.closing?(Repo, "acme") + end + + test "leaves other tenants alone" do + Binds.bound(Repo, "globex") + Binds.forget(Repo, "acme") + + assert Binds.count(Repo, "globex") == 1 + end + end +end diff --git a/test/multi_tenancy/connection_test.exs b/test/multi_tenancy/connection_test.exs new file mode 100644 index 0000000..c934666 --- /dev/null +++ b/test/multi_tenancy/connection_test.exs @@ -0,0 +1,301 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.ConnectionTest do + @moduledoc """ + A connection may only exist once its database is open and migrated, so most of this is refusal. + """ + use ExUnit.Case, async: false + + @moduletag :capture_log + + alias AshSqlite.MultiTenancy.Connection + alias AshSqlite.MultiTenancy.Database + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + @repo AshSqlite.TestRepo + + setup context do + start_supervised!(TenantRegistry.child_spec(@repo)) + + dir = Path.join(System.tmp_dir!(), "ash_sqlite_tenancy_#{:erlang.phash2(context.test)}") + File.rm_rf!(dir) + File.mkdir_p!(dir) + on_exit(fn -> File.rm_rf!(dir) end) + + {:ok, dir: dir} + end + + describe "starting" do + test "opens the tenant's database and publishes the repo instance", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + assert {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + assert is_pid(repo_pid) + + # The pool connects lazily, so the file appears on first use rather than at + # start. Anything that touches the database is enough. + assert {:ok, _} = query(connection, "select 1") + assert File.exists?(Database.path(dir, "acme")) + end + + test "a tenant whose name is not a filename still gets its own file", %{dir: dir} do + {:ok, colon} = start_connection(dir, "acme:2026-08") + {:ok, underscore} = start_connection(dir, "acme_2026-08") + + query!(colon, "create table only_in_colon (id integer)") + query!(underscore, "select 1") + + assert File.exists?(Path.join(dir, "acme~3a2026-08.db")) + assert File.exists?(Path.join(dir, "acme_2026-08.db")) + + # The point of the encoding: these are two databases, not one shared by two + # tenants whose names sanitise to the same thing. + assert {:error, _} = query(underscore, "select * from only_in_colon") + end + + test "refuses a second connection for the same tenant", %{dir: dir} do + {:ok, first} = start_connection(dir, "acme") + + assert {:error, {:already_started, ^first}} = start_connection(dir, "acme") + end + + test "info/1 reports what is being served", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + info = Connection.info(connection) + + assert info.tenant == "acme" + assert info.path == Database.path(dir, "acme") + assert is_integer(info.opened_at) + end + end + + describe "migrations" do + test "run before the tenant is available", %{dir: dir} do + migrations = migrations_dir(dir, [{20_260_101_000_000, "create_widgets", :create_widgets}]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: migrations) + + assert Connection.info(connection).schema_version == 20_260_101_000_000 + assert {:ok, %{rows: [[0]]}} = query(connection, "select count(*) from widgets") + end + + test "are not re-run for a tenant that is already up to date", %{dir: dir} do + migrations = migrations_dir(dir, [{20_260_101_000_000, "create_widgets", :create_widgets}]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: migrations) + query!(connection, "insert into widgets (name) values ('kept')") + stop_connection(connection) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: migrations) + + # A re-run would have raised on the existing table, and a fresh database + # would have lost the row. + assert {:ok, %{rows: [["kept"]]}} = query(connection, "select name from widgets") + assert Connection.info(connection).schema_version == 20_260_101_000_000 + end + + test "a tenant behind the others catches up on activation", %{dir: dir} do + first = migrations_dir(dir, [{20_260_101_000_000, "create_widgets", :create_widgets}]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: first) + stop_connection(connection) + + both = + migrations_dir(dir, [ + {20_260_101_000_000, "create_widgets", :create_widgets}, + {20_260_202_000_000, "add_colour", :add_colour} + ]) + + {:ok, connection} = start_connection(dir, "acme", migrations_path: both) + + assert Connection.info(connection).schema_version == 20_260_202_000_000 + assert {:ok, _} = query(connection, "select colour from widgets") + end + + test "a migrations path that is not a directory is a configuration error", %{dir: dir} do + Process.flag(:trap_exit, true) + missing = Path.join(dir, "nope") + + assert {:error, {:missing_migrations_path, ^missing}} = + start_connection(dir, "acme", migrations_path: missing) + + # Distinct from a migration that failed: this one is true of every tenant on + # the node, and quarantining one tenant over it would hide that. + assert :error = TenantRegistry.lookup(@repo, "acme") + refute File.exists?(Database.path(dir, "acme")) + end + + test "a migration that raises stops the connection rather than serving", %{dir: dir} do + Process.flag(:trap_exit, true) + migrations = migrations_dir(dir, [{20_260_101_000_000, "explode", :raise}]) + + assert {:error, {:migration_failed, _}} = + start_connection(dir, "acme", migrations_path: migrations) + + assert :error = TenantRegistry.lookup(@repo, "acme") + end + + test "no migrations path means no migration, not a failure", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + refute Connection.info(connection).schema_version + end + end + + describe "encryption" do + test "refuses to open a plaintext database when a key was expected", %{dir: dir} do + Process.flag(:trap_exit, true) + + assert {:error, :no_key} = start_connection(dir, "acme", encrypted?: true, key: nil) + refute File.exists?(Database.path(dir, "acme")) + end + + test "an unencrypted fleet opens without a key", %{dir: dir} do + assert {:ok, _} = start_connection(dir, "acme", encrypted?: false, key: nil) + end + end + + describe "lifetime" do + test "the connection stops when its repo instance dies", %{dir: dir} do + Process.flag(:trap_exit, true) + {:ok, connection} = start_connection(dir, "acme") + {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + + ref = Process.monitor(connection) + Process.exit(repo_pid, :kill) + + assert_receive {:DOWN, ^ref, :process, ^connection, {:repo_exited, _}}, 2_000 + assert :error = TenantRegistry.lookup(@repo, "acme") + end + + test "stop_repo/1 closes the database but keeps the process", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + + assert :ok = Connection.stop_repo(connection) + + refute Process.alive?(repo_pid) + assert Process.alive?(connection) + assert Connection.repo_pid(connection) == nil + end + + test "stop_repo/1 twice is harmless", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + + assert :ok = Connection.stop_repo(connection) + assert :ok = Connection.stop_repo(connection) + end + + test "stopping the connection closes the repo instance", %{dir: dir} do + {:ok, connection} = start_connection(dir, "acme") + {:ok, ^connection, repo_pid} = TenantRegistry.lookup(@repo, "acme") + + stop_connection(connection) + + refute Process.alive?(repo_pid) + assert :error = TenantRegistry.lookup(@repo, "acme") + end + end + + # `AshSqlite.TestRepo` is configured with the sandbox pool, and a tenant instance + # inherits the repo module's application config. + defp start_connection(dir, tenant, opts \\ []) do + Connection.start_link( + Keyword.merge( + [ + repo: @repo, + tenant: tenant, + path: Database.path(dir, tenant), + repo_opts: [pool: DBConnection.ConnectionPool] + ], + opts + ) + ) + end + + defp stop_connection(connection) do + ref = Process.monitor(connection) + GenServer.stop(connection) + assert_receive {:DOWN, ^ref, :process, ^connection, _}, 2_000 + end + + defp query(connection, sql) do + repo_pid = Connection.repo_pid(connection) + previous = @repo.put_dynamic_repo(repo_pid) + + try do + # `@repo.query/1`, not `Ecto.Adapters.SQL.query(@repo, ...)`: the latter looks up + # the module's adapter meta and ignores the dynamic binding. + @repo.query(sql) + after + @repo.put_dynamic_repo(previous) + end + end + + defp query!(connection, sql) do + {:ok, result} = query(connection, sql) + result + end + + defp migrations_dir(dir, migrations) do + path = Path.join(dir, "migrations") + File.mkdir_p!(path) + + for {version, name, behaviour} <- migrations do + # A unique module per written file, so that rewriting a directory to add a + # later migration does not redefine the earlier one's module. + module = + Module.concat([ + Migrations, + :"M#{:erlang.phash2(dir)}", + :"V#{version}_#{System.unique_integer([:positive])}" + ]) + + File.write!(Path.join(path, "#{version}_#{name}.exs"), migration_source(module, behaviour)) + end + + path + end + + defp migration_source(module, :raise) do + """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def up, do: raise "no" + def down, do: :ok + end + """ + end + + defp migration_source(module, :create_widgets) do + """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + create table(:widgets, primary_key: false) do + add :name, :text + end + end + end + """ + end + + defp migration_source(module, :add_colour) do + """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + alter table(:widgets) do + add :colour, :text + end + end + end + """ + end +end diff --git a/test/multi_tenancy/database_test.exs b/test/multi_tenancy/database_test.exs new file mode 100644 index 0000000..8ca96b3 --- /dev/null +++ b/test/multi_tenancy/database_test.exs @@ -0,0 +1,213 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.DatabaseTest do + @moduledoc """ + A tenant becomes a filename, so this is where two tenants would come to share a database. + """ + use ExUnit.Case, async: true + + alias AshSqlite.MultiTenancy.Database + + @seed 20_260_822 + + describe "encode/1 and decode/1 round trip" do + test "every single byte survives" do + for byte <- 0..255 do + tenant = <> + assert {:ok, ^tenant} = Database.decode(Database.encode(tenant)) + end + end + + test "tenants people actually use survive" do + for tenant <- corpus() do + assert {:ok, ^tenant} = Database.decode(Database.encode(tenant)), + "did not round trip: #{inspect(tenant)}" + end + end + + test "generated tenants survive, and no two of them collide" do + tenants = generated_tenants() + + for tenant <- tenants do + assert {:ok, ^tenant} = Database.decode(Database.encode(tenant)), + "did not round trip: #{inspect(tenant)} (seed #{@seed})" + end + + encoded = Enum.map(tenants, &Database.encode/1) + + assert Enum.count(Enum.uniq(encoded)) == Enum.count(Enum.uniq(tenants)), + "two distinct tenants encoded to one name (seed #{@seed})" + end + end + + describe "collisions the obvious implementation would allow" do + test "punctuation is not flattened into the character that replaces it" do + refute Database.encode("a:b") == Database.encode("a_b") + refute Database.encode("a/b") == Database.encode("a_b") + refute Database.encode("a b") == Database.encode("a-b") + end + + test "an escape in the tenant does not alias an escape we produced" do + refute Database.encode("~3a") == Database.encode(":") + end + + test "encodings never differ only by case, so a case-insensitive filesystem is safe" do + for tenant <- corpus() ++ generated_tenants() do + encoded = Database.encode(tenant) + + assert String.downcase(encoded) == encoded, + "#{inspect(tenant)} encoded to #{inspect(encoded)}, which has case (seed #{@seed})" + end + end + + test "tenants differing only by case get different files" do + refute Database.encode("Acme") == Database.encode("acme") + assert Database.encode("Acme") == "~41cme" + end + end + + describe "encode/1 keeps the name inside the directory" do + test "no encoded name contains a path separator or a null" do + for tenant <- ["../../etc/passwd", "a/b", "a\\b", "..", ".", "/", <<0>>] do + encoded = Database.encode(tenant) + refute String.contains?(encoded, ["/", "\\", <<0>>]) + end + end + + test "a traversing tenant stays a single file in dir" do + path = Database.path("/tmp/tenants", "../../etc/passwd") + + assert path == "/tmp/tenants/..~2f..~2fetc~2fpasswd.db" + assert Path.dirname(Path.expand(path)) == "/tmp/tenants" + end + + test "dot-only tenants are ordinary filenames" do + assert Path.dirname(Path.expand(Database.path("/tmp/tenants", "."))) == "/tmp/tenants" + assert Path.dirname(Path.expand(Database.path("/tmp/tenants", ".."))) == "/tmp/tenants" + end + end + + describe "encode/1 refuses what it cannot name" do + test "a non-binary tenant, rather than stringifying it into a collision" do + assert_raise ArgumentError, ~r/expected a binary tenant/, fn -> Database.encode(:acme) end + assert_raise ArgumentError, ~r/expected a binary tenant/, fn -> Database.encode(1) end + assert_raise ArgumentError, ~r/expected a binary tenant/, fn -> Database.encode(nil) end + end + + test "an empty tenant" do + assert_raise ArgumentError, ~r/cannot be an empty string/, fn -> Database.encode("") end + end + + test "a tenant too long to be a filename, before the filesystem does" do + assert_raise ArgumentError, ~r/encodes to \d+ bytes/, fn -> + Database.encode(String.duplicate("a", 249)) + end + + # Escaped bytes cost three each, so the limit arrives three times sooner. + assert_raise ArgumentError, ~r/encodes to \d+ bytes/, fn -> + Database.encode(String.duplicate(":", 83)) + end + end + + test "the longest name that does fit is accepted" do + assert byte_size(Database.encode(String.duplicate("a", 248))) == 248 + end + end + + describe "decode/1 declines what encode/1 could not have produced" do + test "an unterminated or malformed escape" do + assert :error = Database.decode("acme~") + assert :error = Database.decode("acme~3") + assert :error = Database.decode("acme~zz") + assert :error = Database.decode("acme~3z") + end + + test "uppercase hex, which we never emit" do + assert :error = Database.decode("acme~3A") + end + + test "a byte that would have been escaped, appearing raw" do + assert :error = Database.decode("acme:2026") + assert :error = Database.decode("Acme") + assert :error = Database.decode("acme name") + end + + test "an empty name" do + assert :error = Database.decode("") + end + end + + describe "sidecars/1" do + test "names the WAL and shared-memory files alongside the database" do + assert Database.sidecars("/tmp/t/acme.db") == [ + "/tmp/t/acme.db", + "/tmp/t/acme.db-wal", + "/tmp/t/acme.db-shm" + ] + end + end + + describe "tenant_from_path/2" do + test "recovers the tenant of a database in the directory" do + path = Database.path("/tmp/tenants", "acme:2026-08") + assert {:ok, "acme:2026-08"} = Database.tenant_from_path("/tmp/tenants", path) + end + + test "declines a path outside the directory" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/other/acme.db") + end + + test "declines a path nested below the directory" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/tenants/sub/acme.db") + end + + test "declines a file that is not a database" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/tenants/acme.sqlite") + end + + test "declines a name no tenant could have produced" do + assert :error = Database.tenant_from_path("/tmp/tenants", "/tmp/tenants/Backup.db") + end + end + + defp corpus do + [ + "acme", + "acme-corp", + "acme_corp", + "acme.corp", + "ACME", + "Acme", + "acme:2026-08", + "acme:billing", + "0191f3d0-4d1e-7c3a-9c9e-1b2c3d4e5f60", + "0191F3D0-4D1E-7C3A-9C9E-1B2C3D4E5F60", + "tenant 42", + "~", + "~3a", + "..", + ".", + "/", + "../../etc/passwd", + "日本語", + "emoji-🙂", + <<0>>, + <<255, 254, 253>>, + "a\nb", + "'; drop table users; --" + ] + end + + # Includes invalid UTF-8: a tenant is whatever the application put in `set_tenant/2`. + defp generated_tenants do + :rand.seed(:exsss, {@seed, @seed, @seed}) + + for _ <- 1..2_000 do + length = :rand.uniform(24) + for _ <- 1..length, into: <<>>, do: <<:rand.uniform(256) - 1>> + end + |> Enum.uniq() + end +end diff --git a/test/multi_tenancy/managed_test.exs b/test/multi_tenancy/managed_test.exs new file mode 100644 index 0000000..2c1f93e --- /dev/null +++ b/test/multi_tenancy/managed_test.exs @@ -0,0 +1,232 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.ManagedTest do + @moduledoc """ + A `strategy :context` resource with nothing configured but `AshSqlite.MultiTenancy`, + driven through Ash and checked against the files on disk. + """ + use ExUnit.Case, async: false + + @moduletag :capture_log + + alias AshSqlite.ManagedTenantRepo + alias AshSqlite.MultiTenancy + alias AshSqlite.Test.ManagedPost + + require Ash.Query + + setup context do + dir = Path.join(System.tmp_dir!(), "ash_sqlite_managed_#{:erlang.phash2(context.test)}") + File.rm_rf!(dir) + on_exit(fn -> File.rm_rf!(dir) end) + + start_supervised!( + {MultiTenancy, + repo: ManagedTenantRepo, + dir: dir, + migrations_path: migrations(dir), + repo_opts: [pool: DBConnection.ConnectionPool]} + ) + + {:ok, dir: dir} + end + + describe "wiring" do + test "a context-multitenant resource with no tenant repo gets the default" do + assert AshSqlite.DataLayer.Info.tenant_repo(ManagedPost) == + {AshSqlite.MultiTenancy.TenantRepo, []} + end + + test "a resource that names a tenant repo keeps it" do + assert {AshSqlite.TenantRepo.Function, [fun: _]} = + AshSqlite.DataLayer.Info.tenant_repo(AshSqlite.Test.TenantedPost) + end + + test "a resource with no context multitenancy has no tenant repo" do + refute AshSqlite.DataLayer.Info.tenant_repo(AshSqlite.Test.Post) + end + end + + describe "reads and writes" do + test "a create lands in its own tenant's file", %{dir: dir} do + create!("acme", "acme one") + create!("acme", "acme two") + create!("globex", "globex one") + + assert titles_in_file(dir, "acme") == ["acme one", "acme two"] + assert titles_in_file(dir, "globex") == ["globex one"] + end + + test "a read only sees its own tenant" do + create!("acme", "acme one") + create!("globex", "globex one") + + assert titles("acme") == ["acme one"] + assert titles("globex") == ["globex one"] + end + + test "an update stays in its own tenant", %{dir: dir} do + post = create!("acme", "before") + create!("globex", "untouched") + + post |> Ash.Changeset.for_update(:update, %{title: "after"}) |> Ash.update!(tenant: "acme") + + assert titles_in_file(dir, "acme") == ["after"] + assert titles_in_file(dir, "globex") == ["untouched"] + end + + test "a destroy stays in its own tenant", %{dir: dir} do + post = create!("acme", "doomed") + create!("globex", "safe") + + Ash.destroy!(post, tenant: "acme") + + assert titles_in_file(dir, "acme") == [] + assert titles_in_file(dir, "globex") == ["safe"] + end + end + + describe "the paths a caller could not have wrapped" do + test "Ash.count/2 goes through the tenant repo with the right tenant" do + create!("acme", "one") + create!("globex", "one") + create!("globex", "two") + + assert Ash.count!(ManagedPost, tenant: "acme") == 1 + assert Ash.count!(ManagedPost, tenant: "globex") == 2 + end + + test "an atomic bulk update stays in its own tenant", %{dir: dir} do + create!("acme", "before") + create!("globex", "before") + + ManagedPost + |> Ash.Query.filter(title == "before") + |> Ash.bulk_update!(:update, %{title: "after"}, tenant: "acme", strategy: :atomic) + + assert titles_in_file(dir, "acme") == ["after"] + assert titles_in_file(dir, "globex") == ["before"] + end + end + + describe "tenants" do + test "each tenant is a separate file, named as MultiTenancy says", %{dir: dir} do + create!("acme", "one") + + assert File.exists?(MultiTenancy.path_for(ManagedTenantRepo, "acme")) + assert MultiTenancy.path_for(ManagedTenantRepo, "acme") == Path.join(dir, "acme.db") + end + + test "a tenant met for the first time is migrated on activation", %{dir: dir} do + create!("acme", "one") + + # "globex" has never been seen, so its database does not exist yet and its + # table can only come from the migration running at activation. + refute File.exists?(Path.join(dir, "globex.db")) + assert create!("globex", "one").title == "one" + assert titles_in_file(dir, "globex") == ["one"] + end + + test "a tenant reopened after being closed keeps its rows" do + create!("acme", "durable") + :ok = MultiTenancy.close(ManagedTenantRepo, "acme") + + assert MultiTenancy.resident(ManagedTenantRepo) == [] + assert titles("acme") == ["durable"] + end + + test "tenants whose names would sanitise alike stay separate", %{dir: dir} do + create!("acme:eu", "colon") + + assert titles("acme_eu") == [] + assert titles_in_file(dir, "acme:eu") == ["colon"] + end + end + + describe "refusals" do + test "Ash refuses a tenantless query before the data layer sees it" do + assert_raise Ash.Error.Invalid, ~r/require a tenant to be specified/, fn -> + Ash.read!(ManagedPost) + end + end + + test "the data layer refuses one on the paths that bypass an action" do + assert_raise ArgumentError, ~r/carried no tenant/, fn -> + AshSqlite.DataLayer.transaction(ManagedPost, fn -> :unreachable end) + end + end + end + + describe "transactions" do + test "a failed action rolls its write back", %{dir: dir} do + assert_raise RuntimeError, fn -> + AshSqlite.DataLayer.transaction( + ManagedPost, + fn -> + create!("acme", "rolled back") + raise "no" + end, + nil, + %{type: :custom, metadata: %{}, tenant: "acme"} + ) + end + + assert titles_in_file(dir, "acme") == [] + end + + test "a transaction refuses to reach into another tenant's database" do + assert_raise Ash.Error.Unknown, ~r/open on another tenant's database/, fn -> + AshSqlite.DataLayer.transaction( + ManagedPost, + fn -> create!("globex", "wrong database") end, + nil, + %{type: :custom, metadata: %{}, tenant: "acme"} + ) + end + end + end + + defp create!(tenant, title) do + ManagedPost + |> Ash.Changeset.for_create(:create, %{title: title}) + |> Ash.create!(tenant: tenant) + end + + defp titles(tenant) do + ManagedPost |> Ash.read!(tenant: tenant) |> Enum.map(& &1.title) |> Enum.sort() + end + + # Read with Exqlite rather than through Ash, so isolation is checked against the + # bytes on disk and not against the thing under test. + defp titles_in_file(dir, tenant) do + path = Path.join(dir, AshSqlite.MultiTenancy.Database.encode(tenant) <> ".db") + {:ok, db} = Exqlite.Sqlite3.open(path) + {:ok, stmt} = Exqlite.Sqlite3.prepare(db, "SELECT title FROM managed_posts ORDER BY title") + {:ok, rows} = Exqlite.Sqlite3.fetch_all(db, stmt) + :ok = Exqlite.Sqlite3.close(db) + List.flatten(rows) + end + + defp migrations(dir) do + path = Path.join(dir, "migrations") + File.mkdir_p!(path) + module = Module.concat([ManagedMigrations, :"M#{:erlang.phash2(dir)}"]) + + File.write!(Path.join(path, "20260101000000_create_managed_posts.exs"), """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + create table(:managed_posts, primary_key: false) do + add :id, :text, primary_key: true + add :title, :text + end + end + end + """) + + path + end +end diff --git a/test/multi_tenancy/multi_tenancy_test.exs b/test/multi_tenancy/multi_tenancy_test.exs new file mode 100644 index 0000000..4b65aa8 --- /dev/null +++ b/test/multi_tenancy/multi_tenancy_test.exs @@ -0,0 +1,483 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancyTest do + @moduledoc """ + The runtime end to end, against real database files. + """ + use ExUnit.Case, async: false + + @moduletag :capture_log + + alias AshSqlite.MultiTenancy + alias AshSqlite.MultiTenancy.Database + + @repo AshSqlite.TestRepo + + setup context do + dir = Path.join(System.tmp_dir!(), "ash_sqlite_tenancy_#{:erlang.phash2(context.test)}") + File.rm_rf!(dir) + on_exit(fn -> File.rm_rf!(dir) end) + + {:ok, dir: dir} + end + + describe "connection_for/2" do + test "activates a tenant that is not resident", %{dir: dir} do + start_tenancy(dir) + + assert {:ok, repo_pid} = MultiTenancy.connection_for(@repo, "acme") + assert is_pid(repo_pid) + assert MultiTenancy.resident(@repo) == ["acme"] + end + + test "returns the same connection for a resident tenant", %{dir: dir} do + start_tenancy(dir) + + assert {:ok, first} = MultiTenancy.connection_for(@repo, "acme") + assert {:ok, ^first} = MultiTenancy.connection_for(@repo, "acme") + end + + test "gives each tenant its own connection", %{dir: dir} do + start_tenancy(dir) + + {:ok, acme} = MultiTenancy.connection_for(@repo, "acme") + {:ok, globex} = MultiTenancy.connection_for(@repo, "globex") + + refute acme == globex + end + end + + describe "isolation" do + test "a row written for one tenant is not visible to another", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('acme only')") + end) + + names = + MultiTenancy.with_tenant(@repo, "globex", fn -> + @repo.query!("select name from widgets").rows + end) + + assert names == [] + + assert MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("select name from widgets").rows + end) == [["acme only"]] + end + + test "tenants whose names would sanitise alike get separate databases", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme:eu", fn -> + @repo.query!("insert into widgets (name) values ('colon')") + end) + + assert MultiTenancy.with_tenant(@repo, "acme_eu", fn -> + @repo.query!("select name from widgets").rows + end) == [] + end + end + + describe "with_tenant/3" do + test "restores the previous binding afterwards", %{dir: dir} do + start_tenancy(dir) + before = @repo.get_dynamic_repo() + + MultiTenancy.with_tenant(@repo, "acme", fn -> :ok end) + + assert @repo.get_dynamic_repo() == before + end + + test "restores the previous binding when the function raises", %{dir: dir} do + start_tenancy(dir) + before = @repo.get_dynamic_repo() + + assert_raise RuntimeError, fn -> + MultiTenancy.with_tenant(@repo, "acme", fn -> raise "boom" end) + end + + assert @repo.get_dynamic_repo() == before + end + + test "raises rather than running against whatever was bound", %{dir: dir} do + start_tenancy(dir, key_for: fn _tenant -> nil end) + + assert_raise AshSqlite.MultiTenancy.UnavailableError, ~r/unavailable/, fn -> + MultiTenancy.with_tenant(@repo, "acme", fn -> :never end) + end + end + end + + describe "residency" do + test "closes the least recently used tenant beyond max_resident", %{dir: dir} do + start_tenancy(dir, max_resident: 2) + + {:ok, _} = MultiTenancy.connection_for(@repo, "first") + {:ok, _} = MultiTenancy.connection_for(@repo, "second") + {:ok, _} = MultiTenancy.connection_for(@repo, "third") + + assert Enum.sort(MultiTenancy.resident(@repo)) == ["second", "third"] + end + + test "never closes a tenant with a statement in flight", %{dir: dir} do + start_tenancy(dir, max_resident: 2) + + {:ok, _} = MultiTenancy.connection_for(@repo, "first") + {:ok, _} = MultiTenancy.connection_for(@repo, "second") + + # "first" is the least recently used, and is held bound for the duration. + holder = hold("first") + + {:ok, _} = MultiTenancy.connection_for(@repo, "third") + + assert "first" in MultiTenancy.resident(@repo) + refute "second" in MultiTenancy.resident(@repo) + + release(holder) + end + + test "exceeds the limit rather than refusing when every tenant is in use", %{dir: dir} do + start_tenancy(dir, max_resident: 1) + + {:ok, _} = MultiTenancy.connection_for(@repo, "first") + holder = hold("first") + + assert {:ok, _} = MultiTenancy.connection_for(@repo, "second") + assert Enum.sort(MultiTenancy.resident(@repo)) == ["first", "second"] + + release(holder) + end + + test "a closed tenant keeps its data and reopens on demand", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('durable')") + end) + + :ok = MultiTenancy.close(@repo, "acme") + assert MultiTenancy.resident(@repo) == [] + assert File.exists?(Database.path(dir, "acme")) + + assert MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("select name from widgets").rows + end) == [["durable"]] + end + end + + describe "delete/2" do + test "removes the database and its sidecars", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('gone')") + end) + + assert {:ok, removed} = MultiTenancy.delete(@repo, "acme") + + assert Database.path(dir, "acme") in removed + refute File.exists?(Database.path(dir, "acme")) + refute File.exists?(Database.path(dir, "acme") <> "-wal") + end + end + + describe "rename/3" do + test "carries the tenant's data to the new name", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "carried") + + # The write is still in the WAL rather than the database file, so this fails + # unless the sidecars move with it. + assert File.exists?(Database.path(dir, "acme") <> "-wal") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme-renamed") + + assert read("acme-renamed") == [["carried"]] + refute File.exists?(Database.path(dir, "acme")) + assert File.exists?(Database.path(dir, "acme-renamed")) + end + + test "closes a resident tenant before moving its file", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme-renamed") + assert MultiTenancy.resident(@repo) == [] + end + + test "renames a tenant whose name is not a filename", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme:eu", "colon") + + assert :ok = MultiTenancy.rename(@repo, "acme:eu", "acme:us") + + assert read("acme:us") == [["colon"]] + assert MultiTenancy.all_tenants(@repo) == ["acme:us"] + end + + test "refuses to overwrite a tenant that already has a database", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "keep me") + write("globex", "do not clobber") + + assert {:error, :target_exists} = MultiTenancy.rename(@repo, "acme", "globex") + + assert read("globex") == [["do not clobber"]] + assert read("acme") == [["keep me"]] + end + + test "refuses a tenant with no database, rather than reporting success", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + assert {:error, :no_database} = MultiTenancy.rename(@repo, "never-seen", "renamed") + end + + test "renaming a tenant to itself leaves it alone", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "untouched") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme") + assert read("acme") == [["untouched"]] + end + + test "the new name serves reads and writes afterwards", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "before") + + assert :ok = MultiTenancy.rename(@repo, "acme", "acme-renamed") + write("acme-renamed", "after") + + assert read("acme-renamed") == [["after"], ["before"]] + end + end + + describe "quarantine" do + test "a tenant that cannot open is refused without retrying", %{dir: dir} do + start_tenancy(dir, key_for: fn _tenant -> nil end) + + assert {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + assert {:error, {:quarantined, :no_key}} = MultiTenancy.connection_for(@repo, "acme") + assert MultiTenancy.quarantined(@repo) == %{"acme" => :no_key} + end + + test "release/2 lets the next request try again", %{dir: dir} do + start_tenancy(dir, key_for: fn _tenant -> nil end) + + {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + :ok = MultiTenancy.release(@repo, "acme") + + assert MultiTenancy.quarantined(@repo) == %{} + # Still broken, so it fails the same way rather than being quarantined-stale. + assert {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + end + + test "quarantines one tenant without affecting another", %{dir: dir} do + start_tenancy(dir, key_for: fn tenant -> if tenant == "acme", do: nil, else: "unused" end) + + assert {:error, :no_key} = MultiTenancy.connection_for(@repo, "acme") + assert Map.keys(MultiTenancy.quarantined(@repo)) == ["acme"] + end + end + + describe "seal/1" do + test "refuses new tenants, and unseal/1 allows them again", %{dir: dir} do + start_tenancy(dir) + {:ok, resident} = MultiTenancy.connection_for(@repo, "acme") + + :ok = MultiTenancy.seal(@repo) + + assert {:error, :draining} = MultiTenancy.connection_for(@repo, "globex") + # An already-resident tenant keeps serving, so work in flight can finish. + assert {:ok, ^resident} = MultiTenancy.connection_for(@repo, "acme") + + :ok = MultiTenancy.unseal(@repo) + assert {:ok, _} = MultiTenancy.connection_for(@repo, "globex") + end + end + + describe "all_tenants/1" do + test "is empty for a fleet nobody has used yet", %{dir: dir} do + start_tenancy(dir) + + assert MultiTenancy.all_tenants(@repo) == [] + end + + test "lists a tenant whether it is resident or only on disk", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + {:ok, _} = MultiTenancy.connection_for(@repo, "globex") + :ok = MultiTenancy.close(@repo, "globex") + + assert MultiTenancy.resident(@repo) == ["acme"] + assert Enum.sort(MultiTenancy.all_tenants(@repo)) == ["acme", "globex"] + end + + test "lists a resident tenant that has not written its file yet", %{dir: dir} do + start_tenancy(dir) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + + assert MultiTenancy.all_tenants(@repo) == ["acme"] + end + + test "recovers from disk a tenant whose name is not a filename", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme:eu/1") + :ok = MultiTenancy.close(@repo, "acme:eu/1") + + assert MultiTenancy.all_tenants(@repo) == ["acme:eu/1"] + end + + test "counts a tenant once, not once per file it owns", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + MultiTenancy.with_tenant(@repo, "acme", fn -> + @repo.query!("insert into widgets (name) values ('one')") + end) + + assert Enum.any?(File.ls!(dir), &String.ends_with?(&1, "-wal")) + assert MultiTenancy.all_tenants(@repo) == ["acme"] + end + + test "ignores files no tenant could have produced", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + :ok = MultiTenancy.close(@repo, "acme") + File.write!(Path.join(dir, "notes.txt"), "") + File.write!(Path.join(dir, "Backup.db"), "") + + assert MultiTenancy.all_tenants(@repo) == ["acme"] + end + end + + describe "migrate_all/3" do + test "migrates each tenant and closes it again", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + results = MultiTenancy.migrate_all(@repo, ["acme", "globex"]) + + assert results == [ + {"acme", {:ok, 20_260_101_000_000}}, + {"globex", {:ok, 20_260_101_000_000}} + ] + + assert MultiTenancy.resident(@repo) == [] + assert File.exists?(Database.path(dir, "acme")) + assert File.exists?(Database.path(dir, "globex")) + end + + test "migrates every tenant on disk when given none", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + {:ok, _} = MultiTenancy.connection_for(@repo, "acme") + {:ok, _} = MultiTenancy.connection_for(@repo, "globex") + + assert [{"acme", {:ok, _}}, {"globex", {:ok, _}}] = + @repo |> MultiTenancy.migrate_all() |> Enum.sort() + end + + test "reports the tenants that failed rather than stopping", %{dir: dir} do + start_tenancy(dir, key_for: fn tenant -> if tenant == "acme", do: nil, else: "unused" end) + + assert [{"acme", {:error, :no_key}}, {"globex", _}] = + MultiTenancy.migrate_all(@repo, ["acme", "globex"]) + end + end + + describe "path_for/2" do + test "names the file a tenant would use", %{dir: dir} do + start_tenancy(dir) + + assert MultiTenancy.path_for(@repo, "acme:eu") == Path.join(dir, "acme~3aeu.db") + end + end + + describe "configuration" do + test "a migrations_path that is not a directory fails at boot", %{dir: dir} do + Process.flag(:trap_exit, true) + + assert {:error, {%ArgumentError{message: message}, _stacktrace}} = + MultiTenancy.start_link( + repo: @repo, + dir: dir, + migrations_path: Path.join(dir, "nope") + ) + + assert message =~ "is not a directory" + end + end + + defp write(tenant, name) do + MultiTenancy.with_tenant(@repo, tenant, fn -> + @repo.query!("insert into widgets (name) values ('#{name}')") + end) + end + + defp read(tenant) do + MultiTenancy.with_tenant(@repo, tenant, fn -> + @repo.query!("select name from widgets order by name").rows + end) + end + + defp start_tenancy(dir, opts \\ []) do + opts = + Keyword.merge( + [ + repo: @repo, + dir: dir, + # A tenant instance inherits the repo module's config, which here names the + # sandbox pool -- one connection shared between processes. + repo_opts: [pool: DBConnection.ConnectionPool] + ], + opts + ) + + start_supervised!({MultiTenancy, opts}) + end + + defp migrations(dir) do + path = Path.join(dir, "migrations") + File.mkdir_p!(path) + module = Module.concat([TenancyMigrations, :"M#{:erlang.phash2(dir)}"]) + + File.write!(Path.join(path, "20260101000000_create_widgets.exs"), """ + defmodule #{inspect(module)} do + use Ecto.Migration + + def change do + create table(:widgets, primary_key: false) do + add :name, :text + end + end + end + """) + + path + end + + # Holds a tenant bound in another process until told to stop, so that eviction + # has something genuinely in use to skip. + # Holds a tenant bound in another process, so eviction has something in use to skip. + defp hold(tenant) do + test = self() + + spawn_link(fn -> + MultiTenancy.with_tenant(@repo, tenant, fn -> + send(test, {:holding, self()}) + receive do: (:release -> :ok) + end) + end) + + receive do + {:holding, pid} -> pid + after + 1_000 -> flunk("holder never bound #{tenant}") + end + end + + defp release(holder) do + send(holder, :release) + end +end diff --git a/test/multi_tenancy/registry_test.exs b/test/multi_tenancy/registry_test.exs new file mode 100644 index 0000000..99d44d0 --- /dev/null +++ b/test/multi_tenancy/registry_test.exs @@ -0,0 +1,115 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.RegistryTest do + @moduledoc """ + Registration is the mutual exclusion, and a lookup must never hand out a dead process. + """ + use ExUnit.Case, async: true + + alias AshSqlite.MultiTenancy.Registry, as: TenantRegistry + + defmodule Repo do + @moduledoc false + end + + setup do + start_supervised!(TenantRegistry.child_spec(Repo)) + :ok + end + + describe "via/2" do + test "registers a process under the tenant" do + connection = start_connection("acme") + + assert {:ok, ^connection, nil} = TenantRegistry.lookup(Repo, "acme") + end + + test "refuses a second process for the same tenant, without running its init" do + first = start_connection("acme") + + assert {:error, {:already_started, ^first}} = + Agent.start(fn -> raise "init must not run" end, + name: TenantRegistry.via(Repo, "acme") + ) + end + + test "keeps tenants that differ only by case apart" do + lower = start_connection("acme") + upper = start_connection("ACME") + + refute lower == upper + assert {:ok, ^lower, _} = TenantRegistry.lookup(Repo, "acme") + assert {:ok, ^upper, _} = TenantRegistry.lookup(Repo, "ACME") + end + end + + describe "publish/3" do + test "makes the repo pid available to the next lookup" do + connection = start_connection("acme") + repo_pid = publish_self(connection, "acme") + + assert {:ok, ^connection, ^repo_pid} = TenantRegistry.lookup(Repo, "acme") + end + + test "a connection that has not published yet is found, with no repo" do + connection = start_connection("acme") + + assert {:ok, ^connection, nil} = TenantRegistry.lookup(Repo, "acme") + end + end + + describe "lookup/2" do + test "is :error for a tenant with no connection" do + assert :error = TenantRegistry.lookup(Repo, "nobody") + end + + test "is :error for an entry whose process has died" do + connection = start_connection("acme") + publish_self(connection, "acme") + + ref = Process.monitor(connection) + Process.exit(connection, :kill) + assert_receive {:DOWN, ^ref, :process, ^connection, :killed}, 2_000 + + # The entry may not have been reaped yet -- that is the point. A dead + # connection must read as absent rather than as a pid to bind. + assert :error = TenantRegistry.lookup(Repo, "acme") + end + end + + describe "resident/1 and count/1" do + test "list the tenants with a connection" do + start_connection("acme") + start_connection("globex") + + assert Enum.sort(TenantRegistry.resident(Repo)) == ["acme", "globex"] + assert TenantRegistry.count(Repo) == 2 + end + + test "are empty for a registry nobody has used" do + assert TenantRegistry.resident(Repo) == [] + assert TenantRegistry.count(Repo) == 0 + end + end + + describe "name/1" do + test "is derived from the repo, so two repos do not share a namespace" do + refute TenantRegistry.name(Repo) == + TenantRegistry.name(AshSqlite.MultiTenancy.BindsTest.Repo) + end + end + + defp start_connection(tenant) do + {:ok, pid} = Agent.start(fn -> :ok end, name: TenantRegistry.via(Repo, tenant)) + pid + end + + defp publish_self(connection, tenant) do + Agent.get(connection, fn _ -> + :ok = TenantRegistry.publish(Repo, tenant, self()) + self() + end) + end +end diff --git a/test/multitenancy_test.exs b/test/multitenancy_test.exs index 4b6a1ab..1cf5674 100644 --- a/test/multitenancy_test.exs +++ b/test/multitenancy_test.exs @@ -235,42 +235,4 @@ defmodule AshSqlite.MultitenancyTest do assert message =~ "resource with no multitenancy" end end - - test "strategy :context without a tenant repo is refused at compile time" do - message = - capture_io(:stderr, fn -> - try do - Code.eval_string(""" - defmodule NoTenantRepoPost do - use Ash.Resource, - domain: nil, - validate_domain_inclusion?: false, - data_layer: AshSqlite.DataLayer - - actions do - defaults([:read]) - end - - attributes do - uuid_primary_key(:id) - end - - multitenancy do - strategy(:context) - end - - sqlite do - table("no_tenant_repo_posts") - repo(AshSqlite.TenantTestRepo) - migrate?(false) - end - end - """) - rescue - _ -> :raised - end - end) - - assert message =~ "`strategy :context` needs a `tenant_repo`" - end end diff --git a/test/support/domain.ex b/test/support/domain.ex index fe47b36..9f005bd 100644 --- a/test/support/domain.ex +++ b/test/support/domain.ex @@ -22,6 +22,7 @@ defmodule AshSqlite.Test.Domain do resource(AshSqlite.Test.NamedFnRepoAccount) resource(AshSqlite.Test.TenantedPost) resource(AshSqlite.Test.BracketedTenantPost) + resource(AshSqlite.Test.ManagedPost) resource(AshSqlite.Test.Organization) resource(AshSqlite.Test.Manager) resource(AshSqlite.Test.Device) diff --git a/test/support/managed_tenant_repo.ex b/test/support/managed_tenant_repo.ex new file mode 100644 index 0000000..0f7fcac --- /dev/null +++ b/test/support/managed_tenant_repo.ex @@ -0,0 +1,18 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.ManagedTenantRepo do + @moduledoc """ + The repo whose tenants `AshSqlite.MultiTenancy` manages, kept apart from + `AshSqlite.TenantTestRepo` so the two cannot interfere. + """ + use AshSqlite.Repo, otp_app: :ash_sqlite + + def min_sqlite_version do + %Version{major: 3, minor: 38, patch: 0} + end + + # Transactions are a repo decision rather than a resource one. + def write_transactions?, do: true +end diff --git a/test/support/resources/managed_post.ex b/test/support/resources/managed_post.ex new file mode 100644 index 0000000..ed4c2d6 --- /dev/null +++ b/test/support/resources/managed_post.ex @@ -0,0 +1,30 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.ManagedPost do + @moduledoc """ + A `strategy :context` resource with no `tenant_repo`, so it gets the default. + """ + use Ash.Resource, domain: AshSqlite.Test.Domain, data_layer: AshSqlite.DataLayer + + actions do + default_accept(:*) + defaults([:create, :read, :update, :destroy]) + end + + multitenancy do + strategy(:context) + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, public?: true) + end + + sqlite do + table("managed_posts") + repo(AshSqlite.ManagedTenantRepo) + migrate?(false) + end +end From 3f9e847958df129006ec0b4c1449c39076389a1b Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Sat, 10 Oct 2026 22:57:15 +0200 Subject: [PATCH 2/4] fix: hold tenants closed across file operations, and refuse busy closes Four defects in the tenant runtime, each found by probing rather than reading, and each with the test that fails without the fix. `Binds.forget/2` deleted the closing mark along with the bind count, so the `held_open_by_caller?` guard in `close/3` was defeated by the line above it. `rename/3` and `delete/2` both document that they hold a tenant closed across the file operation; neither did, from `close/3`'s return until their own `after`. Measured, for rename: a write addressed to `acme` in that window commits into `globex`'s database. For delete it is worse -- an activation there leaves the tenant *resident* across the unlink, serving reads and accepting writes against an inode with no directory entry, all of it discarded when the connection closes. `all_tenants/1` unions residents, so `migrate_all/3` would then migrate a tenant that no longer exists. `forget/2` now drops only the binds and the last-used mark. The closing mark is lifecycle state owned by whoever took it, and `close/3` already clears its own. `delete/2` now takes a mark and holds it across the close and the unlink, as `rename/3` does -- it never took one at all, so the `forget/2` change alone left it broken. `await_quiescence/3` counted down and then closed regardless. `rename/3` calls `close/3` without `force`, so a busy tenant had its file moved out from under a live statement, which keeps the old inode and commits into the destination. `close/3` now returns `{:error, :busy}`; `force: true` closes regardless, which is what eviction and `delete/2` want once they know nothing is bound. `rename/3` propagates it. `migrate_all/3` treats it as success and leaves the tenant resident -- `close_after?` frees residency, it is not part of migrating -- and forwards `:grace_ms`. `grace_ms` is a deadline rather than a loop count. `Process.sleep(1)` sleeps at least a millisecond, so it meant between one and several times what it said. `Binds.bound/2` checked the closing mark and then incremented, so a bind could slip between an eviction's choice of candidate and its close. It now increments first and backs out if it lost: the two are separate ETS objects and no single operation covers both, but publishing the increment before the check means a closer can never read zero for a bind that goes on to proceed, which is the direction that loses data. That only holds because eviction cooperates, so `evict_if_needed/1` marks its candidate closing *before* re-reading the count, and backs out if a bind got in. Neither half is sufficient alone. `put_dynamic_repo/1` binds one repo *module*. The default tenant repo binds the mutate repo, while reads resolve `repo(resource, :read)` -- so a `repo` function returning different modules issued its reads unbound, against whatever database that module was configured with. `VerifyRepo` returns early for any function repo, so nothing checked it. `VerifyTenantRepo` refuses the split, but only for a resource using the default tenant repo. A tenant repo of its own is told whether each statement is a `:read` or a `:mutate`, so routing reads to a replica is something only it can do correctly, and forbidding that outright would be wrong. `connection_for/2` on a repo with no fleet running raised `unknown registry: MyApp.Repo.TenantRegistry`, naming an implementation detail instead of the omission. It now names what is missing and shows the child spec, while reraising untouched anything from a fleet that is actually running. Tests promoted from the probes that found these: concurrent activation of one cold tenant sharing a connection, statements surviving eviction churn under contention, isolation holding across that churn, a tenant reused after a delete, and the Ash-level create/read/update/destroy and bulk-create paths, which had no coverage through Ash at all. --- lib/data_layer.ex | 3 +- lib/multi_tenancy.ex | 48 +++- lib/multi_tenancy/binds.ex | 26 ++- lib/multi_tenancy/manager.ex | 106 ++++++--- lib/verifiers/verify_tenant_repo.ex | 54 +++++ test/multi_tenancy/binds_test.exs | 69 +++++- test/multi_tenancy/managed_test.exs | 33 +++ test/multi_tenancy/multi_tenancy_test.exs | 264 ++++++++++++++++++++++ test/multi_tenancy/tenant_repo_test.exs | 129 +++++++++++ test/support/split_repo_routing.ex | 19 ++ 10 files changed, 715 insertions(+), 36 deletions(-) create mode 100644 lib/verifiers/verify_tenant_repo.ex create mode 100644 test/multi_tenancy/tenant_repo_test.exs create mode 100644 test/support/split_repo_routing.ex diff --git a/lib/data_layer.ex b/lib/data_layer.ex index c012b3d..015e3b4 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -313,7 +313,8 @@ defmodule AshSqlite.DataLayer do AshSqlite.Verifiers.ValidateReferences, AshSqlite.Verifiers.VerifyRepo, AshSqlite.Verifiers.EnsureTableOrPolymorphic, - AshSqlite.Verifiers.VerifyGlobalMultitenancy + AshSqlite.Verifiers.VerifyGlobalMultitenancy, + AshSqlite.Verifiers.VerifyTenantRepo ] def migrate(args) do diff --git a/lib/multi_tenancy.ex b/lib/multi_tenancy.ex index b97320d..2cee062 100644 --- a/lib/multi_tenancy.ex +++ b/lib/multi_tenancy.ex @@ -46,6 +46,19 @@ defmodule AshSqlite.MultiTenancy do @doc "The repo instance serving `tenant`, starting it if it is not resident." @spec connection_for(module(), String.t()) :: {:ok, pid()} | {:error, term()} def connection_for(repo, tenant) do + do_connection_for(repo, tenant) + rescue + # `Registry.lookup/2` raises on a registry that was never started, and the + # message names an internal registry rather than the thing that is missing. + # Rescuing costs nothing on the path that works. + exception in ArgumentError -> + if started?(repo), do: reraise(exception, __STACKTRACE__), else: not_started!(repo) + catch + :exit, {:noproc, _} = reason -> + if started?(repo), do: exit(reason), else: not_started!(repo) + end + + defp do_connection_for(repo, tenant) do case TenantRegistry.lookup(repo, tenant) do {:ok, _connection, repo_pid} when is_pid(repo_pid) -> {:ok, repo_pid} @@ -63,6 +76,34 @@ defmodule AshSqlite.MultiTenancy do end end + defp started?(repo), do: is_pid(Process.whereis(Module.concat(repo, MultiTenancy))) + + # Reached when a resource has `strategy :context` but nothing is managing its + # tenants. Raised rather than left as `unknown registry: MyApp.Repo.TenantRegistry`, + # which names an implementation detail instead of the omission. + defp not_started!(repo) do + raise """ + #{inspect(repo)} has no tenant fleet running, so there is no database to select \ + for this tenant. + + A resource with `strategy :context` gets `AshSqlite.MultiTenancy.TenantRepo` as \ + its tenant repo, which asks `AshSqlite.MultiTenancy` for a connection. Add it to \ + your supervision tree, after the repo: + + children = [ + #{inspect(repo)}, + {AshSqlite.MultiTenancy, + repo: #{inspect(repo)}, + dir: "priv/tenants", + migrations_path: "priv/repo/tenant_migrations"} + ] + + If this repo's tenants are managed elsewhere -- Turso, Litestream, a router of \ + your own -- name it as the resource's `tenant_repo` instead, and this \ + one is not needed. + """ + end + @doc "Runs `fun` with `tenant`'s database bound to the calling process." @spec with_tenant(module(), String.t(), (-> result)) :: result when result: var def with_tenant(repo, tenant, fun) when is_function(fun, 0) do @@ -96,7 +137,12 @@ defmodule AshSqlite.MultiTenancy do end end - @doc "Closes a tenant's database, leaving the file on disk." + @doc """ + Closes a tenant's database, leaving the file on disk. + + Waits `:grace_ms` (1000 by default) for statements in flight, and reports + `{:error, :busy}` rather than closing under one. `force: true` closes regardless. + """ defdelegate close(repo, tenant, opts \\ []), to: Manager @doc "Closes a tenant's database and deletes it, WAL sidecars included." diff --git a/lib/multi_tenancy/binds.ex b/lib/multi_tenancy/binds.ex index 02b9288..9365067 100644 --- a/lib/multi_tenancy/binds.ex +++ b/lib/multi_tenancy/binds.ex @@ -20,13 +20,24 @@ defmodule AshSqlite.MultiTenancy.Binds do @spec name(module()) :: module() def name(repo), do: Module.concat(repo, TenantBinds) - @doc "Records that the calling process has bound `tenant`." + @doc """ + Records that the calling process has bound `tenant`. + + Increments *before* checking the closing mark, and backs the increment out if the + tenant turns out to be closing. The two are separate ETS objects, so no single + operation covers both -- but this order is still safe, because the increment is + published before the check. A closer can therefore never read a count of zero for + a bind that goes on to proceed, which is the direction that loses data. The other + direction only costs a closer one more pass around its grace period. + """ @spec bound(module(), String.t()) :: :ok | :closing def bound(repo, tenant) do + :ets.update_counter(name(repo), {:binds, tenant}, {2, 1}, {{:binds, tenant}, 0}) + if closing?(repo, tenant) do + :ets.update_counter(name(repo), {:binds, tenant}, {2, -1, 0, 0}) :closing else - :ets.update_counter(name(repo), {:binds, tenant}, {2, 1}, {{:binds, tenant}, 0}) :ok end end @@ -99,11 +110,18 @@ defmodule AshSqlite.MultiTenancy.Binds do @spec closing?(module(), String.t()) :: boolean() def closing?(repo, tenant), do: :ets.member(name(repo), {:closing, tenant}) - @doc "Drops everything recorded about `tenant`." + @doc """ + Drops what is recorded about how `tenant` has been used. + + Deliberately not the closing mark. That is lifecycle state owned by whoever took + it, and `close/3` is called *inside* `rename/3` and `delete/2` while they hold one + across a file operation -- clearing it here would reopen the window they took it + to close. + """ @spec forget(module(), String.t()) :: :ok def forget(repo, tenant) do table = name(repo) - Enum.each([:binds, :used, :closing], &:ets.delete(table, {&1, tenant})) + Enum.each([:binds, :used], &:ets.delete(table, {&1, tenant})) :ok end diff --git a/lib/multi_tenancy/manager.ex b/lib/multi_tenancy/manager.ex index 76ad1e5..d2455ea 100644 --- a/lib/multi_tenancy/manager.ex +++ b/lib/multi_tenancy/manager.ex @@ -62,36 +62,58 @@ defmodule AshSqlite.MultiTenancy.Manager do activate(repo, tenant, attempts_left - 1) end - @doc "Closes a tenant's database, leaving the file on disk." - @spec close(module(), String.t(), keyword()) :: :ok + @doc """ + Closes a tenant's database, leaving the file on disk. + + Waits up to `:grace_ms` for statements in flight to finish, and reports + `{:error, :busy}` if they do not. Closing anyway would pull the connection out + from under them -- and for `rename/3`, move the file too -- so the caller is told + rather than left to find out. `force: true` closes regardless, which is what + eviction and `delete/2` want once they know nothing is bound. + """ + @spec close(module(), String.t(), keyword()) :: :ok | {:error, :busy} def close(repo, tenant, opts \\ []) do # A caller that had already marked the tenant closing wants it to stay that way # afterwards -- `rename/3` holds the mark across the close *and* the file move. held_open_by_caller? = Binds.closing?(repo, tenant) + force? = Keyword.get(opts, :force, false) Binds.begin_closing(repo, tenant) try do - unless Keyword.get(opts, :force, false) do - await_quiescence(repo, tenant, Keyword.get(opts, :grace_ms, 1_000)) - end + if force? or quiesced?(repo, tenant, Keyword.get(opts, :grace_ms, 1_000)) do + case TenantRegistry.lookup(repo, tenant) do + {:ok, connection, _repo_pid} -> ConnectionSupervisor.stop_connection(repo, connection) + :error -> :ok + end - case TenantRegistry.lookup(repo, tenant) do - {:ok, connection, _repo_pid} -> ConnectionSupervisor.stop_connection(repo, connection) - :error -> :ok + Binds.forget(repo, tenant) + :ok + else + {:error, :busy} end - - Binds.forget(repo, tenant) - :ok after unless held_open_by_caller?, do: Binds.end_closing(repo, tenant) end end - @doc "Closes a tenant and deletes its database, including the WAL sidecars." + @doc """ + Closes a tenant and deletes its database, including the WAL sidecars. + + The tenant is held closed across the close *and* the unlink, as in `rename/3`. A + request arriving in between would otherwise reopen the database and go on serving + it from the unlinked inode, accepting writes that are discarded when the + connection finally closes. + """ @spec delete(module(), String.t()) :: {:ok, [Path.t()]} def delete(repo, tenant) do - close(repo, tenant, force: true) - GenServer.call(name(repo), {:delete, tenant}) + Binds.begin_closing(repo, tenant) + + try do + close(repo, tenant, force: true) + GenServer.call(name(repo), {:delete, tenant}) + after + Binds.end_closing(repo, tenant) + end end @doc """ @@ -105,7 +127,7 @@ defmodule AshSqlite.MultiTenancy.Manager do Refuses rather than overwrites when the destination already has a database. """ @spec rename(module(), String.t(), String.t()) :: - :ok | {:error, :no_database | :target_exists | File.posix()} + :ok | {:error, :busy | :no_database | :target_exists | File.posix()} def rename(_repo, tenant, tenant), do: :ok def rename(repo, from, to) do @@ -113,8 +135,11 @@ defmodule AshSqlite.MultiTenancy.Manager do Binds.begin_closing(repo, to) try do - close(repo, from) - GenServer.call(name(repo), {:rename, from, to}) + # Not moved while something is still reading or writing it: the statement in + # flight would keep the old inode and commit into the destination's database. + with :ok <- close(repo, from) do + GenServer.call(name(repo), {:rename, from, to}) + end after Binds.end_closing(repo, from) Binds.end_closing(repo, to) @@ -182,7 +207,12 @@ defmodule AshSqlite.MultiTenancy.Manager do with {:ok, _repo_pid} <- activate(repo, tenant), {:ok, connection, _} <- TenantRegistry.lookup(repo, tenant) do version = AshSqlite.MultiTenancy.Connection.info(connection).schema_version - if close_after?, do: close(repo, tenant) + + # `close_after?` frees residency, it is not part of migrating. A tenant + # serving traffic stays open rather than turning a migrated tenant into a + # reported failure. + if close_after?, do: close(repo, tenant, Keyword.take(opts, [:grace_ms])) + {:ok, version} else :error -> {:error, :connection_died} @@ -331,14 +361,32 @@ defmodule AshSqlite.MultiTenancy.Manager do """) tenant -> - # No wait: the candidate was chosen because nothing is bound to it. - close(state.repo, tenant, force: true) + evict(state.repo, tenant) end end state end + # The candidate was chosen because nothing was bound to it, which was true when it + # was chosen. Marking it closing *before* re-reading the count is what makes it + # still true: `Binds.bound/2` publishes its increment before reading the mark, so + # after the mark is set a count of zero means no bind can still arrive. A bind that + # got in first is left alone and the limit is exceeded instead. + defp evict(repo, tenant) do + Binds.begin_closing(repo, tenant) + + try do + if Binds.count(repo, tenant) == 0 do + close(repo, tenant, force: true) + else + :ok + end + after + Binds.end_closing(repo, tenant) + end + end + # The sidecars only exist between a write and a checkpoint, but when they do they # hold committed data the database file does not -- so a move that left them # behind would lose the most recent writes. @@ -354,14 +402,18 @@ defmodule AshSqlite.MultiTenancy.Manager do end end - defp await_quiescence(_repo, _tenant, grace_ms) when grace_ms <= 0, do: :ok + # A deadline rather than a loop count: `Process.sleep(1)` sleeps *at least* a + # millisecond, so counting iterations made `grace_ms` mean something between one + # and several times what it said, depending on how loaded the scheduler was. + defp quiesced?(repo, tenant, grace_ms) do + await_quiescence(repo, tenant, System.monotonic_time(:millisecond) + grace_ms) + end - defp await_quiescence(repo, tenant, grace_ms) do - if Binds.count(repo, tenant) == 0 do - :ok - else - Process.sleep(1) - await_quiescence(repo, tenant, grace_ms - 1) + defp await_quiescence(repo, tenant, deadline) do + cond do + Binds.count(repo, tenant) == 0 -> true + System.monotonic_time(:millisecond) >= deadline -> false + true -> Process.sleep(1) && await_quiescence(repo, tenant, deadline) end end end diff --git a/lib/verifiers/verify_tenant_repo.ex b/lib/verifiers/verify_tenant_repo.ex new file mode 100644 index 0000000..b7f0ad7 --- /dev/null +++ b/lib/verifiers/verify_tenant_repo.ex @@ -0,0 +1,54 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Verifiers.VerifyTenantRepo do + @moduledoc false + use Spark.Dsl.Verifier + + alias Spark.Dsl.Verifier + alias Spark.Error.DslError + + # `put_dynamic_repo/1` binds per repo *module*, and `AshSqlite.MultiTenancy.TenantRepo` + # binds the mutate one. A resource whose read repo is a different module would issue + # its reads on a module nothing bound, against that module's own configured database, + # with nothing raising. + # + # Only checked for the default tenant repo. One of the application's own is told + # whether each statement is a `:read` or a `:mutate`, and can bind a read replica + # separately, which is the case this would otherwise forbid. + @impl true + def verify(dsl) do + with true <- Ash.Resource.Info.multitenancy_strategy(dsl) == :context, + nil <- Verifier.get_option(dsl, [:sqlite], :tenant_repo), + fun when is_function(fun, 2) <- Verifier.get_option(dsl, [:sqlite], :repo) do + resource = Verifier.get_persisted(dsl, :module) + verify_same_repo(resource, fun.(resource, :read), fun.(resource, :mutate)) + else + _ -> :ok + end + end + + defp verify_same_repo(_resource, repo, repo), do: :ok + + defp verify_same_repo(resource, read, mutate) do + {:error, + DslError.exception( + module: resource, + path: [:sqlite, :repo], + message: """ + #{inspect(resource)} has `strategy :context` and a `repo` function returning \ + #{inspect(read)} for :read and #{inspect(mutate)} for :mutate. + + `AshSqlite.MultiTenancy.TenantRepo` selects a connection with \ + `Ecto.Repo.put_dynamic_repo/1`, which binds one repo *module*. It binds the \ + mutate repo, so reads would be issued on #{inspect(read)} unbound, against \ + whatever database that module was configured with rather than this tenant's. + + Either return one module for both, or name a `tenant_repo` of your own. A \ + tenant repo is told whether each statement is a `:read` or a `:mutate`, so \ + binding a read replica separately is something only it can do correctly. + """ + )} + end +end diff --git a/test/multi_tenancy/binds_test.exs b/test/multi_tenancy/binds_test.exs index b0498b0..4c73df6 100644 --- a/test/multi_tenancy/binds_test.exs +++ b/test/multi_tenancy/binds_test.exs @@ -57,6 +57,59 @@ defmodule AshSqlite.MultiTenancy.BindsTest do end end + describe "bound/2 against a concurrent close" do + # The bind and the closing check are two ETS operations, so the order matters. + # Incrementing first means a closer can never read a count of zero for a bind + # that goes on to proceed; checking first would let one slip between the check + # and the increment and lose its statement to the close. + test "a bind is visible to a closer before it is acted on" do + parent = self() + + task = + Task.async(fn -> + assert Binds.bound(Repo, "acme") == :ok + send(parent, :bound) + receive do: (:release -> Binds.released(Repo, "acme")) + end) + + assert_receive :bound + assert Binds.count(Repo, "acme") == 1 + + send(task.pid, :release) + Task.await(task) + end + + test "a bind that loses the race leaves the count where it found it" do + Binds.begin_closing(Repo, "acme") + + assert Binds.bound(Repo, "acme") == :closing + assert Binds.count(Repo, "acme") == 0 + end + + test "many binds racing a close either all hold or all back out" do + outcomes = + 1..100 + |> Task.async_stream( + fn i -> + if i == 50, do: Binds.begin_closing(Repo, "acme") + + case Binds.bound(Repo, "acme") do + :ok -> Binds.released(Repo, "acme") && :held + :closing -> :backed_out + end + end, + max_concurrency: 20 + ) + |> Enum.map(fn {:ok, outcome} -> outcome end) + + # Whatever the interleaving, every bind is matched by a release or a back-out, + # so nothing is left counted against a tenant nobody is using. + assert Binds.count(Repo, "acme") == 0 + assert :backed_out in outcomes + assert Binds.closing?(Repo, "acme") + end + end + describe "closing" do test "refuses new binds while it is marked" do Binds.begin_closing(Repo, "acme") @@ -145,16 +198,26 @@ defmodule AshSqlite.MultiTenancy.BindsTest do end describe "forget/2" do - test "drops everything recorded about a tenant" do + test "drops the binds and the last-used mark" do Binds.bound(Repo, "acme") Binds.released(Repo, "acme") - Binds.begin_closing(Repo, "acme") Binds.forget(Repo, "acme") assert Binds.count(Repo, "acme") == 0 refute Binds.last_used(Repo, "acme") - refute Binds.closing?(Repo, "acme") + end + + # `rename/3` and `delete/2` hold a tenant closed across a file operation, and + # call `close/3` -- which forgets the tenant -- in the middle of it. Clearing the + # mark here would reopen the window they took it to close. + test "leaves a closing mark for whoever took it" do + Binds.bound(Repo, "acme") + Binds.begin_closing(Repo, "acme") + + Binds.forget(Repo, "acme") + + assert Binds.closing?(Repo, "acme") end test "leaves other tenants alone" do diff --git a/test/multi_tenancy/managed_test.exs b/test/multi_tenancy/managed_test.exs index 2c1f93e..d6d829e 100644 --- a/test/multi_tenancy/managed_test.exs +++ b/test/multi_tenancy/managed_test.exs @@ -88,6 +88,39 @@ defmodule AshSqlite.MultiTenancy.ManagedTest do end end + describe "the whole action surface, per tenant" do + test "create, read, update and destroy all stay in one tenant's file", %{dir: dir} do + post = create!("acme", "one") + create!("globex", "untouched") + + updated = + post + |> Ash.Changeset.for_update(:update, %{title: "two"}, tenant: "acme") + |> Ash.update!() + + assert updated.title == "two" + assert titles_in_file(dir, "acme") == ["two"] + + Ash.destroy!(updated, tenant: "acme") + + assert titles_in_file(dir, "acme") == [] + assert titles_in_file(dir, "globex") == ["untouched"] + end + + # The bulk paths take a different route to the tenant than the single-record + # ones -- they carry the whole changeset context rather than its data layer half. + test "a bulk create lands in its own tenant's file", %{dir: dir} do + result = + Ash.bulk_create!([%{title: "b1"}, %{title: "b2"}], ManagedPost, :create, + tenant: "acme", + return_records?: true + ) + + assert length(result.records) == 2 + assert titles_in_file(dir, "acme") == ["b1", "b2"] + end + end + describe "the paths a caller could not have wrapped" do test "Ash.count/2 goes through the tenant repo with the right tenant" do create!("acme", "one") diff --git a/test/multi_tenancy/multi_tenancy_test.exs b/test/multi_tenancy/multi_tenancy_test.exs index 4b65aa8..bb23181 100644 --- a/test/multi_tenancy/multi_tenancy_test.exs +++ b/test/multi_tenancy/multi_tenancy_test.exs @@ -11,6 +11,7 @@ defmodule AshSqlite.MultiTenancyTest do @moduletag :capture_log alias AshSqlite.MultiTenancy + alias AshSqlite.MultiTenancy.Binds alias AshSqlite.MultiTenancy.Database @repo AshSqlite.TestRepo @@ -39,6 +40,22 @@ defmodule AshSqlite.MultiTenancyTest do assert {:ok, ^first} = MultiTenancy.connection_for(@repo, "acme") end + # Every caller of a cold tenant races the same activation. One connection per + # database is the whole invariant -- two would be two writers on one file. + test "concurrent first requests for one cold tenant share one connection", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + + results = + 1..50 + |> Task.async_stream(fn _ -> MultiTenancy.connection_for(@repo, "acme") end, + max_concurrency: 50 + ) + |> Enum.map(fn {:ok, result} -> result end) + + assert Enum.all?(results, &match?({:ok, _}, &1)), inspect(Enum.uniq(results)) + assert results |> Enum.map(fn {:ok, pid} -> pid end) |> Enum.uniq() |> length() == 1 + end + test "gives each tenant its own connection", %{dir: dir} do start_tenancy(dir) @@ -49,6 +66,39 @@ defmodule AshSqlite.MultiTenancyTest do end end + describe "a fleet that was never started" do + test "connection_for/2 names what is missing, not an internal registry" do + assert_raise RuntimeError, fn -> MultiTenancy.connection_for(@repo, "acme") end + + message = + try do + MultiTenancy.connection_for(@repo, "acme") + rescue + e -> Exception.message(e) + end + + assert message =~ "has no tenant fleet running" + assert message =~ "{AshSqlite.MultiTenancy," + assert message =~ "tenant_repo" + refute message =~ "unknown registry" + end + + test "with_tenant/3 says the same thing" do + assert_raise RuntimeError, ~r/has no tenant fleet running/, fn -> + MultiTenancy.with_tenant(@repo, "acme", fn -> :never end) + end + end + + # A started fleet asked about a tenant it cannot serve must still report that, + # rather than being mistaken for a missing supervision tree. + test "a real error from a started fleet is not rewritten", %{dir: dir} do + start_tenancy(dir) + + assert :ok = MultiTenancy.seal(@repo) + assert {:error, :draining} = MultiTenancy.connection_for(@repo, "acme") + end + end + describe "isolation" do test "a row written for one tenant is not visible to another", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -152,6 +202,90 @@ defmodule AshSqlite.MultiTenancyTest do release(holder) end + # Eviction picks a tenant nothing is bound to and then closes it, and the two + # steps are not one atomic act. A request that arrives in between must not lose + # its statement, so the whole cycle is driven under contention rather than in + # the sequential order the tests above use. + test "statements survive eviction churn under contention", %{dir: dir} do + start_tenancy(dir, max_resident: 1, migrations_path: migrations(dir)) + + outcomes = + 1..40 + |> Task.async_stream( + fn i -> + tenant = "t#{rem(i, 3)}" + + try do + write(tenant, "row #{i}") + read(tenant) + :ok + rescue + exception -> {:error, Exception.message(exception)} + catch + kind, reason -> {kind, reason} + end + end, + max_concurrency: 8, + timeout: 30_000 + ) + |> Enum.map(fn {:ok, outcome} -> outcome end) + + assert Enum.reject(outcomes, &(&1 == :ok)) == [] + end + + test "eviction churn does not move rows between tenants", %{dir: dir} do + start_tenancy(dir, max_resident: 1, migrations_path: migrations(dir)) + + for i <- 1..10 do + write("acme", "acme #{i}") + write("globex", "globex #{i}") + end + + acme = List.flatten(read("acme")) + globex = List.flatten(read("globex")) + + assert length(acme) == 10 + assert length(globex) == 10 + assert Enum.all?(acme, &String.starts_with?(&1, "acme")) + assert Enum.all?(globex, &String.starts_with?(&1, "globex")) + end + + test "close/3 refuses a tenant with a statement in flight", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert {:error, :busy} = MultiTenancy.close(@repo, "acme", grace_ms: 20) + assert MultiTenancy.resident(@repo) == ["acme"] + + release(holder) + end + + test "close/3 with force: true closes it regardless", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert :ok = MultiTenancy.close(@repo, "acme", force: true) + assert MultiTenancy.resident(@repo) == [] + + release(holder) + end + + test "close/3 waits for a statement to finish rather than refusing at once", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + Task.start(fn -> + Process.sleep(20) + release(holder) + end) + + assert :ok = MultiTenancy.close(@repo, "acme", grace_ms: 2_000) + end + test "a closed tenant keeps its data and reopens on demand", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -183,6 +317,103 @@ defmodule AshSqlite.MultiTenancyTest do refute File.exists?(Database.path(dir, "acme")) refute File.exists?(Database.path(dir, "acme") <> "-wal") end + + # A deleted tenant is one that has never been seen, not one that is broken: the + # next request must get a fresh, migrated database rather than a quarantine or + # the rows that were just removed. + test "a tenant requested again after a delete gets a fresh migrated database", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "gone") + + assert {:ok, _removed} = MultiTenancy.delete(@repo, "acme") + assert MultiTenancy.quarantined(@repo) == %{} + + write("acme", "new") + + assert read("acme") == [["new"]] + end + end + + describe "holding a tenant closed" do + # `rename/3` and `delete/2` both mark a tenant closing, close it, and then move + # or unlink its file. The mark is what stops a request arriving in between and + # opening the very file that is about to be moved, so `close/3` must leave a + # mark its caller took. + test "close/3 leaves a closing mark its caller was already holding", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "held") + + Binds.begin_closing(@repo, "acme") + MultiTenancy.close(@repo, "acme") + + assert Binds.closing?(@repo, "acme") + assert Binds.bound(@repo, "acme") == :closing + end + + test "close/3 clears a mark it took itself", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "not held") + + MultiTenancy.close(@repo, "acme") + + refute Binds.closing?(@repo, "acme") + assert Binds.bound(@repo, "acme") == :ok + end + + # The window `rename/3` opens between its own close and its file move: a bind + # arriving here must wait for the move rather than opening the source file. Run + # as `rename/3` runs it, because the bug is in the sequence and not in one call. + test "nothing can bind the source name mid-rename", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "original") + + Binds.begin_closing(@repo, "acme") + Binds.begin_closing(@repo, "acme-renamed") + + MultiTenancy.close(@repo, "acme") + + assert Binds.bound(@repo, "acme") == :closing, + "a write addressed to acme here opens the file rename/3 is moving, " <> + "so it commits into acme-renamed's database instead" + + assert Binds.bound(@repo, "acme-renamed") == :closing + + Binds.end_closing(@repo, "acme") + Binds.end_closing(@repo, "acme-renamed") + end + + # `delete/2`'s window is worse than rename's: a bind here leaves a connection + # open on the inode `delete/2` is about to unlink, and it goes on serving reads + # and accepting writes against a database with no directory entry. + test "nothing can bind a tenant mid-delete", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "doomed") + + Binds.begin_closing(@repo, "acme") + MultiTenancy.close(@repo, "acme", force: true) + + assert Binds.bound(@repo, "acme") == :closing, + "a request arriving between delete/2's close and its unlink reopens " <> + "the database, which then survives the delete and keeps serving" + + Binds.end_closing(@repo, "acme") + end + + test "delete/2 leaves no connection open on a database it removed", %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "doomed") + + assert {:ok, _removed} = MultiTenancy.delete(@repo, "acme") + + # A tenant may legitimately be resident again already -- a request after the + # delete recreates the file. What must never hold is residency with no file. + for tenant <- MultiTenancy.resident(@repo) do + assert File.exists?(Database.path(dir, tenant)), + "#{tenant} is resident but its database is gone, so it is serving " <> + "an unlinked inode whose writes are discarded on close" + end + end end describe "rename/3" do @@ -219,6 +450,22 @@ defmodule AshSqlite.MultiTenancyTest do assert MultiTenancy.all_tenants(@repo) == ["acme:us"] end + # A statement in flight holds the old inode, so a move under it would commit + # into the destination's database. Refused, as the close it rests on is. + test "refuses a tenant with a statement in flight, leaving the file alone", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert {:error, :busy} = MultiTenancy.rename(@repo, "acme", "acme-renamed") + + assert File.exists?(Database.path(dir, "acme")) + refute File.exists?(Database.path(dir, "acme-renamed")) + + release(holder) + end + test "refuses to overwrite a tenant that already has a database", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) write("acme", "keep me") @@ -378,6 +625,23 @@ defmodule AshSqlite.MultiTenancyTest do @repo |> MultiTenancy.migrate_all() |> Enum.sort() end + # `close_after?` frees residency; it is not part of migrating. A tenant serving + # traffic must not be reported as a migration failure, and must not be closed + # out from under the traffic either. + test "a tenant in use is migrated, left resident, and still reported ok", + %{dir: dir} do + start_tenancy(dir, migrations_path: migrations(dir)) + write("acme", "in use") + holder = hold("acme") + + assert [{"acme", {:ok, 20_260_101_000_000}}] = + MultiTenancy.migrate_all(@repo, ["acme"], grace_ms: 20) + + assert MultiTenancy.resident(@repo) == ["acme"] + + release(holder) + end + test "reports the tenants that failed rather than stopping", %{dir: dir} do start_tenancy(dir, key_for: fn tenant -> if tenant == "acme", do: nil, else: "unused" end) diff --git a/test/multi_tenancy/tenant_repo_test.exs b/test/multi_tenancy/tenant_repo_test.exs new file mode 100644 index 0000000..74e7995 --- /dev/null +++ b/test/multi_tenancy/tenant_repo_test.exs @@ -0,0 +1,129 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.MultiTenancy.TenantRepoTest do + @moduledoc """ + A `repo` function returning different modules for `:read` and `:mutate` cannot be + served by `AshSqlite.MultiTenancy.TenantRepo`, because `put_dynamic_repo/1` binds one module at a time. + """ + use ExUnit.Case, async: false + + import ExUnit.CaptureIO + + defp resource(name, body) do + quote do + defmodule unquote(name) do + use Ash.Resource, + domain: nil, + validate_domain_inclusion?: false, + data_layer: AshSqlite.DataLayer + + actions do + defaults([:read]) + end + + attributes do + uuid_primary_key(:id) + end + + unquote(body) + end + end + end + + # Raised from the resource's own verification, which is what fails `mix compile`. + test "a split read/mutate repo is refused for a context-multitenant resource" do + message = + capture_io(:stderr, fn -> + try do + Code.eval_quoted( + resource( + SplitRepoPost, + quote do + multitenancy(do: strategy(:context)) + + sqlite do + table("split_repo_posts") + migrate?(false) + + repo(&AshSqlite.Test.SplitRepoRouting.split/2) + end + end + ) + ) + rescue + _ -> :raised + end + end) + + assert message =~ "AshSqlite.DevTestRepo for :read" + assert message =~ "AshSqlite.TestRepo for :mutate" + assert message =~ "put_dynamic_repo" + assert message =~ "tenant_repo" + end + + test "a repo function returning one module for both is accepted" do + Code.eval_quoted( + resource( + SameRepoPost, + quote do + multitenancy(do: strategy(:context)) + + sqlite do + table("same_repo_posts") + migrate?(false) + repo(&AshSqlite.Test.SplitRepoRouting.same/2) + end + end + ) + ) + + assert AshSqlite.DataLayer.Info.repo(SameRepoPost, :read) == AshSqlite.TestRepo + assert AshSqlite.DataLayer.Info.repo(SameRepoPost, :mutate) == AshSqlite.TestRepo + end + + # The case the check would otherwise forbid: a tenant repo of its own is told whether + # each statement is a read or a write, so it can bind a replica for reads. + test "a split is allowed when the resource names its own tenant repo" do + Code.eval_quoted( + resource( + SplitWithTenantRepoPost, + quote do + multitenancy(do: strategy(:context)) + + sqlite do + table("split_with_tenant_repo_posts") + migrate?(false) + tenant_repo(&AshSqlite.Test.TenantRepos.repo/2) + + repo(&AshSqlite.Test.SplitRepoRouting.split/2) + end + end + ) + ) + + assert {AshSqlite.TenantRepo.Function, _} = + AshSqlite.DataLayer.Info.tenant_repo(SplitWithTenantRepoPost) + end + + # Nothing binds a resource with no context multitenancy, so a split is its own + # business there and has been supported for as long as the `repo` option has. + test "a split is allowed on a resource with no context multitenancy" do + Code.eval_quoted( + resource( + SplitGlobalPost, + quote do + sqlite do + table("split_global_posts") + migrate?(false) + + repo(&AshSqlite.Test.SplitRepoRouting.split/2) + end + end + ) + ) + + assert AshSqlite.DataLayer.Info.repo(SplitGlobalPost, :read) == AshSqlite.DevTestRepo + end +end diff --git a/test/support/split_repo_routing.ex b/test/support/split_repo_routing.ex new file mode 100644 index 0000000..5a8687d --- /dev/null +++ b/test/support/split_repo_routing.ex @@ -0,0 +1,19 @@ +# SPDX-FileCopyrightText: 2023 ash_sqlite contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshSqlite.Test.SplitRepoRouting do + @moduledoc """ + Named functions for a resource's `repo` to capture, for the tenant-repo tests. + + They cannot be inline `fn`s: a capture of a function on another module resolves + while the resource is still compiling, and an inline `fn` is refused because it + compiles into a function on the resource that is not loaded yet. + """ + @doc "Different modules per usage -- what the default tenant repo cannot serve." + def split(_resource, :read), do: AshSqlite.DevTestRepo + def split(_resource, :mutate), do: AshSqlite.TestRepo + + @doc "One module for both usages -- accepted." + def same(_resource, _type), do: AshSqlite.TestRepo +end From e3f57653c0c49dd45c6af29db3d5a7196b5d5e47 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Mon, 7 Sep 2026 17:47:59 +0200 Subject: [PATCH 3/4] chore: trim inline commentary Same pass as the previous commit, over the runtime and its tests. Also removes two comments that had been left duplicated by earlier edits, in `AshSqlite.MultiTenancy.Registry` and the fleet test's bound-tenant helper. Comment-only; no behaviour change. --- lib/multi_tenancy.ex | 17 ++++----- lib/multi_tenancy/binds.ex | 1 - lib/multi_tenancy/connection.ex | 5 +-- lib/multi_tenancy/database.ex | 3 +- lib/multi_tenancy/manager.ex | 27 ++++++-------- lib/multi_tenancy/migrations.ex | 5 +-- lib/multi_tenancy/registry.ex | 5 +-- lib/verifiers/verify_tenant_repo.ex | 12 ++---- test/multi_tenancy/binds_test.exs | 11 ++---- test/multi_tenancy/connection_test.exs | 6 +-- test/multi_tenancy/managed_test.exs | 17 ++++++++- test/multi_tenancy/multi_tenancy_test.exs | 45 +++++++++-------------- 12 files changed, 66 insertions(+), 88 deletions(-) diff --git a/lib/multi_tenancy.ex b/lib/multi_tenancy.ex index 2cee062..0d0e534 100644 --- a/lib/multi_tenancy.ex +++ b/lib/multi_tenancy.ex @@ -48,9 +48,8 @@ defmodule AshSqlite.MultiTenancy do def connection_for(repo, tenant) do do_connection_for(repo, tenant) rescue - # `Registry.lookup/2` raises on a registry that was never started, and the - # message names an internal registry rather than the thing that is missing. - # Rescuing costs nothing on the path that works. + # `Registry.lookup/2` raises on a registry never started, naming an internal + # registry rather than the omission. exception in ArgumentError -> if started?(repo), do: reraise(exception, __STACKTRACE__), else: not_started!(repo) catch @@ -78,9 +77,8 @@ defmodule AshSqlite.MultiTenancy do defp started?(repo), do: is_pid(Process.whereis(Module.concat(repo, MultiTenancy))) - # Reached when a resource has `strategy :context` but nothing is managing its - # tenants. Raised rather than left as `unknown registry: MyApp.Repo.TenantRegistry`, - # which names an implementation detail instead of the omission. + # Raised rather than left as `unknown registry: ...`, which names an + # implementation detail instead of the omission. defp not_started!(repo) do raise """ #{inspect(repo)} has no tenant fleet running, so there is no database to select \ @@ -111,8 +109,7 @@ defmodule AshSqlite.MultiTenancy do {:ok, repo_pid} -> bound(repo, tenant, repo_pid, fun) - # Raising, not returning: an unbound statement here would run against whichever - # database the process already had, which is another tenant's data. + # An unbound statement here would run against another tenant's data. {:error, reason} -> raise AshSqlite.MultiTenancy.UnavailableError, tenant: tenant, reason: reason end @@ -179,8 +176,8 @@ defmodule AshSqlite.MultiTenancy do @doc "The fleet configuration." defdelegate config(repo), to: Manager - # Checked at boot rather than at activation: a typo would otherwise surface in - # production as `no such table`, at whatever hour the first tenant woke up. + # Checked at boot, not activation: a typo would otherwise surface as `no such + # table` whenever the first tenant woke up. defp verify_migrations_path!(nil), do: :ok defp verify_migrations_path!(path) do diff --git a/lib/multi_tenancy/binds.ex b/lib/multi_tenancy/binds.ex index 9365067..a9a46c2 100644 --- a/lib/multi_tenancy/binds.ex +++ b/lib/multi_tenancy/binds.ex @@ -127,7 +127,6 @@ defmodule AshSqlite.MultiTenancy.Binds do @impl true def init(repo) do - # Public: binds happen in arbitrary caller processes, not in this one. :ets.new(name(repo), [ :named_table, :public, diff --git a/lib/multi_tenancy/connection.ex b/lib/multi_tenancy/connection.ex index 367c6be..9fa412c 100644 --- a/lib/multi_tenancy/connection.ex +++ b/lib/multi_tenancy/connection.ex @@ -137,8 +137,7 @@ defmodule AshSqlite.MultiTenancy.Connection do source = AshSqlite.MultiTenancy.Migrations.load!(migrations_path) statuses = Ecto.Migrator.migrations(repo, source, opts) - # Only run the migrator when something is pending: the common case is an - # up-to-date tenant being activated. + # The common case is an already up-to-date tenant. if Enum.any?(statuses, &match?({:down, _, _}, &1)) do Ecto.Migrator.run(repo, source, :up, Keyword.put(opts, :all, true)) end @@ -154,7 +153,6 @@ defmodule AshSqlite.MultiTenancy.Connection do {:error, {:migration_failed, exception.__struct__}} end - # Every version in the directory, because anything pending has just been run. defp current_version([]), do: nil defp current_version(statuses), do: statuses |> Enum.map(&elem(&1, 1)) |> Enum.max() @@ -169,7 +167,6 @@ defmodule AshSqlite.MultiTenancy.Connection do :ok catch - # The repo is already going down, which is the state we wanted. :exit, _ -> :ok end end diff --git a/lib/multi_tenancy/database.ex b/lib/multi_tenancy/database.ex index 541b0c9..3ab60ca 100644 --- a/lib/multi_tenancy/database.ex +++ b/lib/multi_tenancy/database.ex @@ -107,8 +107,7 @@ defmodule AshSqlite.MultiTenancy.Database do <> end - # Lowercase, so that the encoded alphabet as a whole is lowercase and two - # encodings can never differ only by case. See the moduledoc. + # Lowercase, so two encodings can never differ only by case. defp hex(nibble) when nibble < 10, do: ?0 + nibble defp hex(nibble), do: ?a + nibble - 10 diff --git a/lib/multi_tenancy/manager.ex b/lib/multi_tenancy/manager.ex index d2455ea..ea95384 100644 --- a/lib/multi_tenancy/manager.ex +++ b/lib/multi_tenancy/manager.ex @@ -208,9 +208,8 @@ defmodule AshSqlite.MultiTenancy.Manager do {:ok, connection, _} <- TenantRegistry.lookup(repo, tenant) do version = AshSqlite.MultiTenancy.Connection.info(connection).schema_version - # `close_after?` frees residency, it is not part of migrating. A tenant - # serving traffic stays open rather than turning a migrated tenant into a - # reported failure. + # `close_after?` frees residency; it is not part of migrating, so a busy + # tenant staying open is not a failure. if close_after?, do: close(repo, tenant, Keyword.take(opts, [:grace_ms])) {:ok, version} @@ -263,8 +262,8 @@ defmodule AshSqlite.MultiTenancy.Manager do def handle_call({:delete, tenant}, _from, state) do base = Database.path(state.dir, tenant) - # Closing checkpoints and may remove the sidecars itself, so existence is not - # stable between a check and a removal. Attempt each and report what went. + # Closing may remove the sidecars itself, so existence is not stable between a + # check and a removal. removed = for path <- Database.sidecars(base), File.rm(path) == :ok, do: path {:reply, {:ok, removed}, state} @@ -368,11 +367,9 @@ defmodule AshSqlite.MultiTenancy.Manager do state end - # The candidate was chosen because nothing was bound to it, which was true when it - # was chosen. Marking it closing *before* re-reading the count is what makes it - # still true: `Binds.bound/2` publishes its increment before reading the mark, so - # after the mark is set a count of zero means no bind can still arrive. A bind that - # got in first is left alone and the limit is exceeded instead. + # Marked closing *before* the count is re-read: `Binds.bound/2` publishes its + # increment before reading the mark, so a count of zero after the mark means no + # bind can still arrive. defp evict(repo, tenant) do Binds.begin_closing(repo, tenant) @@ -387,9 +384,8 @@ defmodule AshSqlite.MultiTenancy.Manager do end end - # The sidecars only exist between a write and a checkpoint, but when they do they - # hold committed data the database file does not -- so a move that left them - # behind would lose the most recent writes. + # Between a write and a checkpoint the sidecars hold committed data the database + # file does not, so a move must take them along. defp move(source, target) do with :ok <- File.rename(source, target) do for {sidecar, renamed} <- Enum.zip(Database.sidecars(source), Database.sidecars(target)), @@ -402,9 +398,8 @@ defmodule AshSqlite.MultiTenancy.Manager do end end - # A deadline rather than a loop count: `Process.sleep(1)` sleeps *at least* a - # millisecond, so counting iterations made `grace_ms` mean something between one - # and several times what it said, depending on how loaded the scheduler was. + # A deadline, not a loop count: `Process.sleep(1)` sleeps *at least* a millisecond, + # so counting iterations made `grace_ms` mean several times what it said. defp quiesced?(repo, tenant, grace_ms) do await_quiescence(repo, tenant, System.monotonic_time(:millisecond) + grace_ms) end diff --git a/lib/multi_tenancy/migrations.ex b/lib/multi_tenancy/migrations.ex index 06a2212..12d813c 100644 --- a/lib/multi_tenancy/migrations.ex +++ b/lib/multi_tenancy/migrations.ex @@ -27,9 +27,8 @@ defmodule AshSqlite.MultiTenancy.Migrations do :ok end - # `:persistent_term` because this is written once per directory per boot and read - # on every activation. Its cost is in writes, which is why residency is not stored - # here. + # `:persistent_term` costs in writes, and this is written once per boot but read + # on every activation -- which is why residency is not stored here. defp compile!(path, fingerprint) do loaded = path |> files() |> Enum.map(&load_file!/1) |> Enum.sort() :persistent_term.put(key(path), {fingerprint, loaded}) diff --git a/lib/multi_tenancy/registry.ex b/lib/multi_tenancy/registry.ex index dd6efad..eb35817 100644 --- a/lib/multi_tenancy/registry.ex +++ b/lib/multi_tenancy/registry.ex @@ -17,9 +17,8 @@ defmodule AshSqlite.MultiTenancy.Registry do Registry.start_link( keys: :unique, name: name(repo), - # Every tenanted statement takes a lookup, so reads are the hot path. - # Every tenanted statement does a lookup, so reads are the hot path and the - # partitions are what keep them off one ETS table. + # Every tenanted statement does a lookup, so partitions keep reads off one + # ETS table. partitions: System.schedulers_online() ) end diff --git a/lib/verifiers/verify_tenant_repo.ex b/lib/verifiers/verify_tenant_repo.ex index b7f0ad7..f685288 100644 --- a/lib/verifiers/verify_tenant_repo.ex +++ b/lib/verifiers/verify_tenant_repo.ex @@ -9,14 +9,10 @@ defmodule AshSqlite.Verifiers.VerifyTenantRepo do alias Spark.Dsl.Verifier alias Spark.Error.DslError - # `put_dynamic_repo/1` binds per repo *module*, and `AshSqlite.MultiTenancy.TenantRepo` - # binds the mutate one. A resource whose read repo is a different module would issue - # its reads on a module nothing bound, against that module's own configured database, - # with nothing raising. - # - # Only checked for the default tenant repo. One of the application's own is told - # whether each statement is a `:read` or a `:mutate`, and can bind a read replica - # separately, which is the case this would otherwise forbid. + # `put_dynamic_repo/1` binds per repo *module*, and the default tenant repo binds + # the mutate one -- so a split read repo would serve reads from its own configured + # database, unbound, with nothing raising. Only checked for the default: a tenant + # repo of its own sees `type` and can bind a replica deliberately. @impl true def verify(dsl) do with true <- Ash.Resource.Info.multitenancy_strategy(dsl) == :context, diff --git a/test/multi_tenancy/binds_test.exs b/test/multi_tenancy/binds_test.exs index 4c73df6..9edf752 100644 --- a/test/multi_tenancy/binds_test.exs +++ b/test/multi_tenancy/binds_test.exs @@ -58,10 +58,8 @@ defmodule AshSqlite.MultiTenancy.BindsTest do end describe "bound/2 against a concurrent close" do - # The bind and the closing check are two ETS operations, so the order matters. - # Incrementing first means a closer can never read a count of zero for a bind - # that goes on to proceed; checking first would let one slip between the check - # and the increment and lose its statement to the close. + # Incrementing before the closing check is what stops a closer reading zero for + # a bind that goes on to proceed. test "a bind is visible to a closer before it is acted on" do parent = self() @@ -208,9 +206,8 @@ defmodule AshSqlite.MultiTenancy.BindsTest do refute Binds.last_used(Repo, "acme") end - # `rename/3` and `delete/2` hold a tenant closed across a file operation, and - # call `close/3` -- which forgets the tenant -- in the middle of it. Clearing the - # mark here would reopen the window they took it to close. + # `rename/3` and `delete/2` call `close/3` mid-operation, so clearing the mark + # here would reopen the window they took it to close. test "leaves a closing mark for whoever took it" do Binds.bound(Repo, "acme") Binds.begin_closing(Repo, "acme") diff --git a/test/multi_tenancy/connection_test.exs b/test/multi_tenancy/connection_test.exs index c934666..02f0884 100644 --- a/test/multi_tenancy/connection_test.exs +++ b/test/multi_tenancy/connection_test.exs @@ -122,8 +122,7 @@ defmodule AshSqlite.MultiTenancy.ConnectionTest do assert {:error, {:missing_migrations_path, ^missing}} = start_connection(dir, "acme", migrations_path: missing) - # Distinct from a migration that failed: this one is true of every tenant on - # the node, and quarantining one tenant over it would hide that. + # True of every tenant on the node, so quarantining one would hide it. assert :error = TenantRegistry.lookup(@repo, "acme") refute File.exists?(Database.path(dir, "acme")) end @@ -200,8 +199,7 @@ defmodule AshSqlite.MultiTenancy.ConnectionTest do end end - # `AshSqlite.TestRepo` is configured with the sandbox pool, and a tenant instance - # inherits the repo module's application config. + # A tenant instance inherits the repo module's config, which names the sandbox pool. defp start_connection(dir, tenant, opts \\ []) do Connection.start_link( Keyword.merge( diff --git a/test/multi_tenancy/managed_test.exs b/test/multi_tenancy/managed_test.exs index d6d829e..d05367f 100644 --- a/test/multi_tenancy/managed_test.exs +++ b/test/multi_tenancy/managed_test.exs @@ -49,6 +49,20 @@ defmodule AshSqlite.MultiTenancy.ManagedTest do end end + describe "a repo module that serves only tenants" do + # Every connection it serves is a tenant, reached through `put_dynamic_repo/1`, + # so it needs no `database:`, no pool and no name of its own. + test "needs no database of its own, and no name", %{dir: dir} do + assert is_nil(Application.get_env(:ash_sqlite, ManagedTenantRepo)) + assert is_nil(Process.whereis(ManagedTenantRepo)) + + create!("acme", "no shared database needed") + + assert titles_in_file(dir, "acme") == ["no shared database needed"] + assert titles("acme") == ["no shared database needed"] + end + end + describe "reads and writes" do test "a create lands in its own tenant's file", %{dir: dir} do create!("acme", "acme one") @@ -231,8 +245,7 @@ defmodule AshSqlite.MultiTenancy.ManagedTest do ManagedPost |> Ash.read!(tenant: tenant) |> Enum.map(& &1.title) |> Enum.sort() end - # Read with Exqlite rather than through Ash, so isolation is checked against the - # bytes on disk and not against the thing under test. + # Reads the file directly, so isolation is checked against bytes on disk. defp titles_in_file(dir, tenant) do path = Path.join(dir, AshSqlite.MultiTenancy.Database.encode(tenant) <> ".db") {:ok, db} = Exqlite.Sqlite3.open(path) diff --git a/test/multi_tenancy/multi_tenancy_test.exs b/test/multi_tenancy/multi_tenancy_test.exs index bb23181..bcb35a8 100644 --- a/test/multi_tenancy/multi_tenancy_test.exs +++ b/test/multi_tenancy/multi_tenancy_test.exs @@ -40,8 +40,8 @@ defmodule AshSqlite.MultiTenancyTest do assert {:ok, ^first} = MultiTenancy.connection_for(@repo, "acme") end - # Every caller of a cold tenant races the same activation. One connection per - # database is the whole invariant -- two would be two writers on one file. + # One connection per database is the invariant: two would be two writers on one + # file. test "concurrent first requests for one cold tenant share one connection", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -89,8 +89,7 @@ defmodule AshSqlite.MultiTenancyTest do end end - # A started fleet asked about a tenant it cannot serve must still report that, - # rather than being mistaken for a missing supervision tree. + # Distinct from a missing supervision tree. test "a real error from a started fleet is not rewritten", %{dir: dir} do start_tenancy(dir) @@ -202,10 +201,8 @@ defmodule AshSqlite.MultiTenancyTest do release(holder) end - # Eviction picks a tenant nothing is bound to and then closes it, and the two - # steps are not one atomic act. A request that arrives in between must not lose - # its statement, so the whole cycle is driven under contention rather than in - # the sequential order the tests above use. + # Picking a tenant and closing it are not one atomic act, so the cycle is driven + # under contention rather than sequentially. test "statements survive eviction churn under contention", %{dir: dir} do start_tenancy(dir, max_resident: 1, migrations_path: migrations(dir)) @@ -318,9 +315,8 @@ defmodule AshSqlite.MultiTenancyTest do refute File.exists?(Database.path(dir, "acme") <> "-wal") end - # A deleted tenant is one that has never been seen, not one that is broken: the - # next request must get a fresh, migrated database rather than a quarantine or - # the rows that were just removed. + # A deleted tenant is one never seen, not a broken one: the next request gets a + # fresh migrated database. test "a tenant requested again after a delete gets a fresh migrated database", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -336,10 +332,8 @@ defmodule AshSqlite.MultiTenancyTest do end describe "holding a tenant closed" do - # `rename/3` and `delete/2` both mark a tenant closing, close it, and then move - # or unlink its file. The mark is what stops a request arriving in between and - # opening the very file that is about to be moved, so `close/3` must leave a - # mark its caller took. + # The mark is what stops a request opening the file that is about to be moved, + # so `close/3` must leave a mark its caller took. test "close/3 leaves a closing mark its caller was already holding", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) write("acme", "held") @@ -361,9 +355,8 @@ defmodule AshSqlite.MultiTenancyTest do assert Binds.bound(@repo, "acme") == :ok end - # The window `rename/3` opens between its own close and its file move: a bind - # arriving here must wait for the move rather than opening the source file. Run - # as `rename/3` runs it, because the bug is in the sequence and not in one call. + # The window between `rename/3`'s close and its file move: a bind arriving here + # must wait rather than open the source file. test "nothing can bind the source name mid-rename", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) write("acme", "original") @@ -383,9 +376,8 @@ defmodule AshSqlite.MultiTenancyTest do Binds.end_closing(@repo, "acme-renamed") end - # `delete/2`'s window is worse than rename's: a bind here leaves a connection - # open on the inode `delete/2` is about to unlink, and it goes on serving reads - # and accepting writes against a database with no directory entry. + # Worse than rename's window: a bind here serves reads and writes against an + # inode with no directory entry. test "nothing can bind a tenant mid-delete", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) write("acme", "doomed") @@ -450,8 +442,8 @@ defmodule AshSqlite.MultiTenancyTest do assert MultiTenancy.all_tenants(@repo) == ["acme:us"] end - # A statement in flight holds the old inode, so a move under it would commit - # into the destination's database. Refused, as the close it rests on is. + # A statement in flight holds the old inode, so a move under it would commit into + # the destination's database. test "refuses a tenant with a statement in flight, leaving the file alone", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -625,9 +617,8 @@ defmodule AshSqlite.MultiTenancyTest do @repo |> MultiTenancy.migrate_all() |> Enum.sort() end - # `close_after?` frees residency; it is not part of migrating. A tenant serving - # traffic must not be reported as a migration failure, and must not be closed - # out from under the traffic either. + # `close_after?` frees residency; a busy tenant is neither closed nor reported + # as a migration failure. test "a tenant in use is migrated, left resident, and still reported ok", %{dir: dir} do start_tenancy(dir, migrations_path: migrations(dir)) @@ -721,8 +712,6 @@ defmodule AshSqlite.MultiTenancyTest do path end - # Holds a tenant bound in another process until told to stop, so that eviction - # has something genuinely in use to skip. # Holds a tenant bound in another process, so eviction has something in use to skip. defp hold(tenant) do test = self() From 6b5b89e564c0cbb803c072b41dc0a83c5c5105c6 Mon Sep 17 00:00:00 2001 From: Conor Sinclair Date: Sat, 10 Oct 2026 23:11:21 +0200 Subject: [PATCH 4/4] fix: sequence the sleep in `await_quiescence/3` rather than chaining it with `&&` Dialyzer reports the `&&` as a guard that can never succeed, because `Process.sleep/1` always returns `:ok`. --- lib/multi_tenancy/manager.ex | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/multi_tenancy/manager.ex b/lib/multi_tenancy/manager.ex index ea95384..95d3624 100644 --- a/lib/multi_tenancy/manager.ex +++ b/lib/multi_tenancy/manager.ex @@ -406,9 +406,15 @@ defmodule AshSqlite.MultiTenancy.Manager do defp await_quiescence(repo, tenant, deadline) do cond do - Binds.count(repo, tenant) == 0 -> true - System.monotonic_time(:millisecond) >= deadline -> false - true -> Process.sleep(1) && await_quiescence(repo, tenant, deadline) + Binds.count(repo, tenant) == 0 -> + true + + System.monotonic_time(:millisecond) >= deadline -> + false + + true -> + Process.sleep(1) + await_quiescence(repo, tenant, deadline) end end end