Repository navigation
refactor(dispatcher): align aggregate failure boundary - #98
Conversation
yordis
commented
Apr 29, 2026
yordis
commented
- keep dispatch failure guarantees attached to the aggregate execution boundary instead of a separate wrapper process
- make timeout reporting deterministic so retries and failure handling are easier to reason about
- remove infrastructure that no longer contributes to caller safety once exit normalization lives in one place
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryCursor Bugbot is generating a summary for commit f7b3adf. Configure here. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe pull request refactors command execution error handling by removing asynchronous Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Review rate limit: 0/1 reviews remaining, refill in 37 minutes and 12 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/commanded/aggregates/aggregate.ex (2)
303-304: Keep the original abnormal exit reason observable.Normalizing every remaining exit to
:aggregate_execution_failedis fine for the public API, but it also makes cases likeProcess.exit(self(), :boom)opaque in production. Please log or emit telemetry with the rawreasonbefore returning the normalized tuple.Suggested change
- :exit, _reason -> + :exit, reason -> + Logger.error("aggregate execution failed: " <> inspect(reason)) {:error, :aggregate_execution_failed}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commanded/aggregates/aggregate.ex` around lines 303 - 304, The pattern match that collapses every abnormal exit (:exit, _reason -> {:error, :aggregate_execution_failed}) should preserve visibility of the original reason: before returning the normalized {:error, :aggregate_execution_failed} tuple, log or emit telemetry with the raw reason. Update the clause that matches :exit (the ":exit, _reason ->" branch in aggregate.ex) to call Logger.error/1 or :telemetry.execute/3 (with event name like [:commanded, :aggregate, :exit] and include the reason in the metadata) and then return the same normalized tuple so the public API is unchanged.
295-299: Consider adding a regression test for the:remote_node_downexit case.The
:nodedownclause correctly matchesGenServer.call/3exits when the target node becomes unavailable. While this pattern is stable across OTP 28 and all addressing methods (PID,{name, Node},{:via, module, term}), a dedicated test would prevent accidental changes to this error path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commanded/aggregates/aggregate.ex` around lines 295 - 299, Add a regression test that exercises the :nodedown exit clause and asserts the module returns {:error, :remote_node_down} when a GenServer.call to the aggregate process fails due to a remote node going down; specifically, write a test that invokes the code path that sends GenServer.call(..., {:execute_command, context}, timeout) (the same call pattern matched in aggregate.ex) and simulates/forces a {:nodedown, _} exit from the remote node so the clause matching {{:nodedown, _node_name}, {GenServer, :call, [^name, {:execute_command, ^context}, ^timeout]}} is hit and the function returns {:error, :remote_node_down}. Ensure the test isolates the behavior (mock or spawn a remote node/process as needed) and asserts the exact returned value to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/commanded/aggregates/aggregate.ex`:
- Around line 303-304: The pattern match that collapses every abnormal exit
(:exit, _reason -> {:error, :aggregate_execution_failed}) should preserve
visibility of the original reason: before returning the normalized {:error,
:aggregate_execution_failed} tuple, log or emit telemetry with the raw reason.
Update the clause that matches :exit (the ":exit, _reason ->" branch in
aggregate.ex) to call Logger.error/1 or :telemetry.execute/3 (with event name
like [:commanded, :aggregate, :exit] and include the reason in the metadata) and
then return the same normalized tuple so the public API is unchanged.
- Around line 295-299: Add a regression test that exercises the :nodedown exit
clause and asserts the module returns {:error, :remote_node_down} when a
GenServer.call to the aggregate process fails due to a remote node going down;
specifically, write a test that invokes the code path that sends
GenServer.call(..., {:execute_command, context}, timeout) (the same call pattern
matched in aggregate.ex) and simulates/forces a {:nodedown, _} exit from the
remote node so the clause matching {{:nodedown, _node_name}, {GenServer, :call,
[^name, {:execute_command, ^context}, ^timeout]}} is hit and the function
returns {:error, :remote_node_down}. Ensure the test isolates the behavior (mock
or spawn a remote node/process as needed) and asserts the exact returned value
to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b1102fa-f37d-4a1c-81c7-918f2f7d36c9
📒 Files selected for processing (9)
guides/explanations/commands.mdlib/commanded/aggregates/aggregate.exlib/commanded/application/supervisor.exlib/commanded/commands/dispatcher.extest/aggregates/execute_command_test.exstest/application/application_supervisor_test.exstest/application/telemetry_test.exstest/commands/command_timeout_test.exstest/middleware/middleware_test.exs
💤 Files with no reviewable changes (2)
- test/application/application_supervisor_test.exs
- lib/commanded/application/supervisor.ex
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
