Conversation
📝 WalkthroughWalkthroughThis pull request refactors the cache provider from a manual lock-based approach to Cachex 4.0's built-in fetch mechanism, eliminating the separate Unlocker GenServer process. The implementation switches to transactional-like fetch operations with TTL-aware expiration handling, while maintaining the existing public API. Test coverage is substantially expanded across all cache operations and concurrent scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanIvanoff
force-pushed
the
update-cachex-and-cachex-provider
branch
from
0f33839 to
d329181
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mix.exs`:
- Line 52: Update the Cachex dependency tuple in mix.exs from {:cachex, "~>
4.0"} to {:cachex, "~> 4.1"} to explicitly target the latest minor series; after
changing the version string, run mix deps.get (and optionally mix deps.unlock
--check-unused or mix deps.update cachex) to refresh the lockfile so the project
uses 4.1.x.
| {:brod, "~> 4.0"}, | ||
| {:browser, "~> 0.5"}, | ||
| {:cachex, "~> 3.4"}, | ||
| {:cachex, "~> 4.0"}, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version of Cachex elixir library hex.pm
💡 Result:
The latest Cachex version on Hex.pm is 4.1.1 (released Jun 30, 2025). [1]
mix.exs:
{:cachex, "~> 4.1"}Releases page: [2]
Use ~> 4.1 for Cachex to specify the latest minor version.
The latest Cachex version on Hex.pm is 4.1.1. While ~> 4.0 will resolve to 4.1.1, the version spec should be ~> 4.1 to explicitly indicate support for the latest minor version and align with the guideline to use the latest versions of packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mix.exs` at line 52, Update the Cachex dependency tuple in mix.exs from
{:cachex, "~> 4.0"} to {:cachex, "~> 4.1"} to explicitly target the latest minor
series; after changing the version string, run mix deps.get (and optionally mix
deps.unlock --check-unused or mix deps.update cachex) to refresh the lockfile so
the project uses 4.1.x.
There was a problem hiding this comment.
Pull request overview
This pull request updates Cachex from version 3.x to 4.1.1 and refactors the cache provider to use public APIs instead of private ones. It also adds comprehensive test coverage for the Cachex provider and introduces a new notifications stats feature for app notifications.
Changes:
- Updated Cachex dependency from ~> 3.4 to ~> 4.0, removing the custom locking mechanism (Unlocker module) and replacing it with Cachex's built-in Courier service for request deduplication
- Added extensive test coverage for CachexProvider covering concurrent access, error handling, and edge cases
- Added per-type unread notification counts (stats) to the app notifications API with optional cursor filtering
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| mix.exs | Updated Cachex dependency to ~> 4.0 |
| mix.lock | Locked Cachex to 4.1.1 and added ex_hash_ring dependency |
| lib/sanbase_web/graphql/cache/cachex_provider.ex | Refactored to use Cachex.fetch with Courier service, replaced ttl: with expire:, removed custom locking |
| lib/sanbase_web/graphql/cache/cachex_unlocker.ex | Removed entire custom unlocker module (no longer needed) |
| test/sanbase_web/cache/cachex_provider_test.exs | Added comprehensive test coverage including concurrent access patterns |
| lib/sanbase/app_notifications/app_notifications.ex | Added get_notifications_stats/2 function for per-type unread counts |
| lib/sanbase_web/graphql/schema/types/app_notification_types.ex | Added stats field to app_notifications_paginated |
| lib/sanbase_web/graphql/resolvers/app_notification_resolver.ex | Updated resolver to include stats in response |
| test/sanbase_web/graphql/app_notifications/app_notification_api_test.exs | Added test for stats functionality with cursor filtering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/sanbase_web/cache/cachex_provider_test.exs (1)
9-14:⚠️ Potential issue | 🟡 Minor
Process.exit(pid, :normal)is a no-op from a different process — use:shutdownorGenServer.stop.
on_exitruns in a separate process. Sending a:normalexit signal from another process to a GenServer that doesn't trap exits is silently ignored per OTP semantics. In practice, ExUnit terminates the test process (which is linked viastart_link) with:shutdown, which propagates through the link and cleans up the Cachex process — so this likely works incidentally. Still, the explicit cleanup is misleading.Suggested fix
setup do {:ok, pid} = CacheProvider.start_link(name: `@cache_name`, id: `@cache_id`) - on_exit(fn -> Process.exit(pid, :normal) end) + on_exit(fn -> if Process.alive?(pid), do: GenServer.stop(pid) end) :ok end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/cache/cachex_provider_test.exs` around lines 9 - 14, The on_exit callback currently calls Process.exit(pid, :normal) which is a no-op when invoked from a different process; replace that explicit exit with a proper shutdown such as Process.exit(pid, :shutdown) or call GenServer.stop(pid) to ensure the CacheProvider process started by CacheProvider.start_link(name: `@cache_name`, id: `@cache_id`) is cleaned up reliably; update the on_exit to use one of those options (prefer GenServer.stop(pid) if you want a graceful stop) and remove the misleading :normal exit.
🧹 Nitpick comments (2)
lib/sanbase_web/graphql/cache/cachex_provider.ex (2)
7-10: Missing@specand@docon public callback functions.Per coding guidelines, all public functions should have
@specand@docannotations. While the@impltag signals these are behaviour callbacks, adding typespecs would still benefit tooling (Dialyzer) and readability. Low-priority given these are well-understood callbacks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase_web/graphql/cache/cachex_provider.ex` around lines 7 - 10, Add missing `@spec` and `@doc` for the public callback function(s) implemented in this module (e.g., start_link/1) so they conform to the project's guidelines; update the SanbaseWeb.Graphql.CachexProvider module by adding a short `@doc` describing the behavior/contract of start_link/1 and an appropriate `@spec` (matching the callback in SanbaseWeb.Graphql.CacheProvider) to the function implementation annotated with `@impl`, ensuring the types align with Cachex.start_link/1 and the behaviour's callback types.
82-97: Cachex handles exceptions gracefully—explicit rescue is optional.The fallback function raised inside
Cachex.fetchis caught by Cachex's internal Courier process and transformed into an{:error, %Cachex.Error{...}}tuple, which the existing case clause on line 85 already handles. Adding an explicitrescueclause would be defensive but redundant since Cachex guarantees this behavior for all concurrent waiters. The current code respects the library's documented contract and remains clean without additional nesting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase_web/graphql/cache/cachex_provider.ex` around lines 82 - 97, Do not add an explicit rescue around the fallback passed to Cachex.fetch; rely on Cachex's internal Courier to catch exceptions and return {:error, %Cachex.Error{...}}, which is already handled by the existing case branch matching {:error, _}; leave the Cachex.fetch block (the anonymous function using func.(), the {:ok, _} -> compress_value(ok_tuple), {:error, _} -> {:ignore, error}, {:nocache, value} -> {:ignore, {:nocache, value}}, and {:middleware, _, _} -> {:ignore, {:middleware, cache_modify_middleware.(cache, key, tuple)}}) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/sanbase_web/cache/cachex_provider_test.exs`:
- Around line 455-485: The fallback passed into Cachex.fetch inside
CacheProvider.get_or_store must explicitly catch exceptions so spawned callers
don't crash; update the anonymous fallback (the function given to Cachex.fetch
in get_or_store) to wrap the existing pattern-matching return logic in a rescue
clause that converts any raised Exception into a safe {:ignore, {:error,
Exception.message(e)}} (or similar error tuple) so Cachex receives an ignore
result and all concurrent callers get {:error, _} rather than letting the
exception propagate; keep existing branches for {:ok, _}, {:error, _},
{:nocache, _} and {:middleware, _, _} and only handle unexpected exceptions in
the rescue.
---
Outside diff comments:
In `@test/sanbase_web/cache/cachex_provider_test.exs`:
- Around line 9-14: The on_exit callback currently calls Process.exit(pid,
:normal) which is a no-op when invoked from a different process; replace that
explicit exit with a proper shutdown such as Process.exit(pid, :shutdown) or
call GenServer.stop(pid) to ensure the CacheProvider process started by
CacheProvider.start_link(name: `@cache_name`, id: `@cache_id`) is cleaned up
reliably; update the on_exit to use one of those options (prefer
GenServer.stop(pid) if you want a graceful stop) and remove the misleading
:normal exit.
---
Nitpick comments:
In `@lib/sanbase_web/graphql/cache/cachex_provider.ex`:
- Around line 7-10: Add missing `@spec` and `@doc` for the public callback
function(s) implemented in this module (e.g., start_link/1) so they conform to
the project's guidelines; update the SanbaseWeb.Graphql.CachexProvider module by
adding a short `@doc` describing the behavior/contract of start_link/1 and an
appropriate `@spec` (matching the callback in SanbaseWeb.Graphql.CacheProvider) to
the function implementation annotated with `@impl`, ensuring the types align with
Cachex.start_link/1 and the behaviour's callback types.
- Around line 82-97: Do not add an explicit rescue around the fallback passed to
Cachex.fetch; rely on Cachex's internal Courier to catch exceptions and return
{:error, %Cachex.Error{...}}, which is already handled by the existing case
branch matching {:error, _}; leave the Cachex.fetch block (the anonymous
function using func.(), the {:ok, _} -> compress_value(ok_tuple), {:error, _} ->
{:ignore, error}, {:nocache, value} -> {:ignore, {:nocache, value}}, and
{:middleware, _, _} -> {:ignore, {:middleware, cache_modify_middleware.(cache,
key, tuple)}}) unchanged.
| test "when the computation raises, all concurrent callers receive an error tuple" do | ||
| test_pid = self() | ||
| n = 5 | ||
|
|
||
| CacheProvider.get_or_store( | ||
| @cache_name, | ||
| key, | ||
| fn -> | ||
| send(test_pid, "message from precalculation") | ||
| {:middleware, "fake second arg", "fake third arg"} | ||
| end, | ||
| fn _, _, _ -> | ||
| send(test_pid, "message from the other function") | ||
| {:ok, "Hello"} | ||
| end | ||
| ) | ||
| for _ <- 1..n do | ||
| spawn(fn -> | ||
| result = | ||
| CacheProvider.get_or_store( | ||
| @cache_name, | ||
| :concurrent_raise_key, | ||
| fn -> | ||
| Process.sleep(100) | ||
| raise "something went wrong" | ||
| end, | ||
| & &1 | ||
| ) | ||
|
|
||
| send(test_pid, {:result, result}) | ||
| end) | ||
| end | ||
|
|
||
| results = | ||
| Enum.map(1..n, fn _ -> | ||
| assert_receive {:result, result}, 5000 | ||
| result | ||
| end) | ||
|
|
||
| assert Enum.all?(results, &match?({:error, _}, &1)) | ||
| # Nothing was cached | ||
| assert nil == CacheProvider.get(@cache_name, :concurrent_raise_key) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the cachex source files in deps
find deps/cachex -type f -name "*.ex" 2>/dev/null | head -20Repository: santiment/sanbase2
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for the fetch/2 implementation and exception handling
rg -n "def fetch" deps/cachex 2>/dev/null -A 15Repository: santiment/sanbase2
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for rescue/try in Cachex courier
rg -n -C 5 "rescue|try.*do" deps/cachex/lib/cachex 2>/dev/null | head -60Repository: santiment/sanbase2
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check the implementation file for exception handling in the fallback
cat -n test/sanbase_web/cache/cachex_provider_test.exs | sed -n '455,485p'Repository: santiment/sanbase2
Length of output: 1031
🏁 Script executed:
#!/bin/bash
# Also check the CacheProvider implementation for any rescue logic
fd -t f -name "cachex_provider.ex" --exec cat -n {} \;Repository: santiment/sanbase2
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Search for CacheProvider module definition
rg -n "defmodule.*CacheProvider" test/sanbase_web/cache/Repository: santiment/sanbase2
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Find the CacheProvider implementation file
fd -t f "cachex_provider" --type fRepository: santiment/sanbase2
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Look for any lib/sanbase_web file related to cache
fd -t f -path "*/cache/*" | grep -E "\.(ex|exs)$" | head -20Repository: santiment/sanbase2
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check mix.exs for cachex dependency version
rg -n "cachex" mix.exs -i -A 2Repository: santiment/sanbase2
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Read the CacheProvider implementation
cat -n lib/sanbase_web/graphql/cache/cachex_provider.exRepository: santiment/sanbase2
Length of output: 4945
🏁 Script executed:
#!/bin/bash
# Check if test passes by looking for any skip markers or known issues
rg -n "skip|pending|xtest" test/sanbase_web/cache/cachex_provider_test.exs | grep -A2 -B2 "concurrent.*raise"Repository: santiment/sanbase2
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check git history to see if this test was recently added or modified
cd test/sanbase_web/cache && git log --oneline -20 cachex_provider_test.exs 2>/dev/null || echo "Git not available for history"Repository: santiment/sanbase2
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for any exception handling patterns in tests to understand expected behavior
rg -n "raise.*error\|{:error" test/sanbase_web/cache/cachex_provider_test.exs | head -20Repository: santiment/sanbase2
Length of output: 180
🏁 Script executed:
#!/bin/bash
# Search for exception patterns in the test more carefully
rg -n "raise" test/sanbase_web/cache/cachex_provider_test.exsRepository: santiment/sanbase2
Length of output: 492
🏁 Script executed:
#!/bin/bash
# Check if there are any other exception/error tests in the file
rg -n "error.*tuple\|Exception" test/sanbase_web/cache/cachex_provider_test.exsRepository: santiment/sanbase2
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Look at surrounding tests to understand the pattern
cat -n test/sanbase_web/cache/cachex_provider_test.exs | sed -n '301,340p'Repository: santiment/sanbase2
Length of output: 1297
🏁 Script executed:
#!/bin/bash
# Quick check: see if test file has any setup that might mock exception handling
rg -n "setup\|Mox\|mock" test/sanbase_web/cache/cachex_provider_test.exs | head -20Repository: santiment/sanbase2
Length of output: 44
Add explicit exception handling to the fallback function.
The concurrent exception test assumes that when the fallback raises inside Cachex.fetch, all concurrent callers receive {:error, _}. Currently, the fallback (lines 83–97) has no rescue clause—if an exception is raised, it escapes unhandled and depends on Cachex's internal Courier service to convert it to {:error, _} for all waiting callers. If Cachex instead lets the exception propagate, spawned processes will crash without sending messages, causing the test to hang.
Adding an explicit rescue in the fallback makes exception handling deterministic:
result =
Cachex.fetch(cache, true_key, fn ->
case func.() do
{:ok, _} = ok_tuple ->
{:commit, compress_value(ok_tuple), [expire: ttl]}
{:error, _} = error ->
{:ignore, error}
{:nocache, value} ->
{:ignore, {:nocache, value}}
{:middleware, _, _} = tuple ->
{:ignore, {:middleware, cache_modify_middleware.(cache, key, tuple)}}
end
rescue
e in Exception ->
{:ignore, {:error, Exception.message(e)}}
end)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/sanbase_web/cache/cachex_provider_test.exs` around lines 455 - 485, The
fallback passed into Cachex.fetch inside CacheProvider.get_or_store must
explicitly catch exceptions so spawned callers don't crash; update the anonymous
fallback (the function given to Cachex.fetch in get_or_store) to wrap the
existing pattern-matching return logic in a rescue clause that converts any
raised Exception into a safe {:ignore, {:error, Exception.message(e)}} (or
similar error tuple) so Cachex receives an ignore result and all concurrent
callers get {:error, _} rather than letting the exception propagate; keep
existing branches for {:ok, _}, {:error, _}, {:nocache, _} and {:middleware, _,
_} and only handle unexpected exceptions in the rescue.
1 participant
Changes
Ticket
Checklist:
Summary by CodeRabbit
Refactor
Tests