Enforce key locking for nested transactions (#431)

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.

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}

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