From d60cdbd9917f8f93f0e4ded25519adefd65665c2 Mon Sep 17 00:00:00 2001 From: makoto-developer <72484465+makoto-developer@users.noreply.github.com> Date: Tue, 28 Jul 2026 20:35:30 +0900 Subject: [PATCH] Enforce key locking for nested transactions Nested transactions previously short-circuited on a boolean flag and ran the function without acquiring locks, so an inner transaction locking a different key than the enclosing one would proceed with no locking in place. Store the set of locked keys in :cachex_transaction instead of a boolean so a nested transaction can verify that every key it requests is already locked by the enclosing transaction, raising a Cachex.Error otherwise. Refs #431 --- lib/cachex/services/locksmith.ex | 39 ++++++++++++++++++----- lib/cachex/services/locksmith/queue.ex | 9 ++++++ test/cachex/services/locksmith_test.exs | 42 ++++++++++++++++++++++--- 3 files changed, 78 insertions(+), 12 deletions(-) diff --git a/lib/cachex/services/locksmith.ex b/lib/cachex/services/locksmith.ex index 951ff9a9..42203c8f 100644 --- a/lib/cachex/services/locksmith.ex +++ b/lib/cachex/services/locksmith.ex @@ -94,17 +94,36 @@ defmodule Cachex.Services.Locksmith do @doc """ Executes a transaction against a cache table. - If the process is already in a transactional context, the provided function - will be executed immediately. Otherwise the required keys will be locked until - the provided function has finished executing. + If the process is not already in a transactional context, the required keys + will be locked until the provided function has finished executing. + + If the process is already in a transactional context, the enclosing + transaction must already hold a lock on every requested key; the function is + then executed immediately. If any requested key is not already locked, a + `Cachex.Error` is raised, as executing against unlocked keys inside a + transaction is almost certainly a logic error. This is mainly shorthand to avoid having to handle row locking explicitly. """ @spec transaction(Cachex.t(), [any], (-> any)) :: any def transaction(cache() = cache, keys, fun) when is_list(keys) do - case transaction?() do - true -> fun.() - false -> Queue.transaction(cache, keys, fun) + case Process.get(:cachex_transaction, false) do + false -> + Queue.transaction(cache, keys, fun) + + locked -> + # Already inside a transaction; the enclosing transaction must already + # hold a lock on every key we're asking for. If it doesn't, the caller + # is executing against keys with no locking in place, which is almost + # certainly a logic error, so we raise rather than silently proceed. + case keys -- locked do + [] -> + fun.() + + _missing -> + raise Cachex.Error, + message: "Nested transaction requested keys not locked by the enclosing transaction" + end end end @@ -113,14 +132,18 @@ defmodule Cachex.Services.Locksmith do """ @spec transaction? :: boolean def transaction?, - do: Process.get(:cachex_transaction, false) + do: Process.get(:cachex_transaction, false) != false @doc """ Flags this process as running in a transaction. + + The process dictionary stores the list of keys currently locked by the + transaction (empty when no specific keys are held), which allows nested + transactions to verify that they operate against already-locked keys. """ @spec start_transaction :: no_return def start_transaction, - do: Process.put(:cachex_transaction, true) + do: Process.put(:cachex_transaction, []) @doc """ Flags this process as not running in a transaction. diff --git a/lib/cachex/services/locksmith/queue.ex b/lib/cachex/services/locksmith/queue.ex index c1be3ea6..7a13c165 100644 --- a/lib/cachex/services/locksmith/queue.ex +++ b/lib/cachex/services/locksmith/queue.ex @@ -73,7 +73,16 @@ defmodule Cachex.Services.Locksmith.Queue do # inside this queue process instead. def handle_call({:transaction, keys, func, callers}, {caller, _tag}, cache) do true = lock(cache, keys) + + # Track the keys locked by this transaction in the process dictionary so + # that any nested transactions can validate that they only touch keys that + # are already locked (see `Locksmith.transaction/3`). + locked = Process.get(:cachex_transaction, []) + Process.put(:cachex_transaction, locked ++ keys) + val = safe_exec(func, [caller | callers]) + + Process.put(:cachex_transaction, locked) true = unlock(cache, keys) {:reply, val, cache} diff --git a/test/cachex/services/locksmith_test.exs b/test/cachex/services/locksmith_test.exs index db00fb3c..cb056c8e 100644 --- a/test/cachex/services/locksmith_test.exs +++ b/test/cachex/services/locksmith_test.exs @@ -184,14 +184,14 @@ defmodule Cachex.Services.LocksmithTest do # ensure unsert assert(is_transaction1 == nil) - # set the value to true + # flag the process as transactional Services.Locksmith.start_transaction() - # check that the current process is true + # check that the current process holds a (empty) lock set is_transaction2 = Process.get(:cachex_transaction) - # ensure set to true - assert(is_transaction2 == true) + # ensure set to an empty key list + assert(is_transaction2 == []) # set the value to false Services.Locksmith.stop_transaction() @@ -202,4 +202,38 @@ defmodule Cachex.Services.LocksmithTest do # ensure set to false assert(is_transaction3 == false) end + + # A transaction nested inside another transaction must only operate against + # keys already locked by the enclosing transaction. Requesting a subset of the + # locked keys is fine and executes inline, but requesting a key which is not + # locked is almost certainly a logic error and should surface as an error + # rather than silently executing without any locking actually in place. + test "nested transactions enforce locking of their keys" do + # create a test cache + cache = TestUtils.create_cache(transactions: true) + + # retrieve the state for our cache + state = Services.Overseer.lookup(cache) + + # a nested transaction over a subset of the locked keys should run inline + nested_ok = + Services.Locksmith.transaction(state, ["a", "b"], fn -> + Services.Locksmith.transaction(state, ["a"], fn -> :nested end) + end) + + # the inner function should have executed and returned its value + assert(nested_ok == :nested) + + # a nested transaction over a key which is not locked by the enclosing + # transaction should raise; the transaction server catches this and surfaces + # it as an error tuple rather than crashing + nested_bad = + Services.Locksmith.transaction(state, ["a"], fn -> + Services.Locksmith.transaction(state, ["b"], fn -> :nested end) + end) + + # the error should describe the locking violation + assert({:error, message} = nested_bad) + assert(message =~ "not locked by the enclosing transaction") + end end