fix(dispatcher): preserve retry failure guarantees - #96
Conversation
yordis
commented
Apr 29, 2026
yordis
commented
- preserve tuple-based dispatch failures when retry attempts are exhausted so middleware and telemetry continue to observe failures consistently
- lock in aggregate-stopped and remote-node-down recovery before changing the dispatcher failure boundary
- keep retry and timeout documentation aligned with the guarantees the dispatcher actually provides
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Docs were updated to clarify that dispatch timeouts return error tuples (may be Reviewed by Cursor Bugbot for commit 3af552a. Bugbot is set up for automated code reviews on this repo. 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 (7)
WalkthroughThis PR introduces comprehensive documentation and testing for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 16 minutes and 59 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/application/telemetry_test.exs (1)
108-175:⚠️ Potential issue | 🟠 MajorTelemetry retry-exhaustion test is timing-sensitive and can fail intermittently.
This test combines a scheduler-dependent assertion (
:too_many_attemptsmust appear) with a tight receive timeout at Line 165 (1_000), which increases CI flake risk.⏱️ Minimal hardening for receive budget
- assert_receive event, 1_000 + assert_receive event, 5_000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/application/telemetry_test.exs` around lines 108 - 175, The test's receive timeout in collect_dispatch_events (currently assert_receive event, 1_000) is too tight and causes intermittent CI flakes; increase the timeout (e.g., to 5_000 or make it a module attribute like `@receive_timeout`) and use that constant in collect_dispatch_events so the loop has more budget to collect [:commanded, :application, :dispatch, :stop] events; update any test helpers (attach_telemetry / collect_dispatch_events) to reference the new timeout constant.
🧹 Nitpick comments (2)
test/commands/support/remote_node_down_command.ex (1)
1-3: Consider validating the command fields up front.
aggregate_uuiddrives routing, andsleep_in_msis consumed unconditionally by the handler. Leaving them asnildefaults makes malformed commands fail later in the test flow instead of being rejected earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/support/remote_node_down_command.ex` around lines 1 - 3, The RemoteNodeDownCommand struct allows nil for critical fields; enforce upfront validation by either adding `@enforce_keys` [:aggregate_uuid, :sleep_in_ms] to the Commanded.Commands.RemoteNodeDownCommand module or by providing a constructor (e.g., new/1) that checks and raises on missing/invalid aggregate_uuid or sleep_in_ms; ensure aggregate_uuid is present and sleep_in_ms is a non-negative integer so malformed commands are rejected at creation rather than later in the handler.test/commands/distributed_dispatch_test.exs (1)
59-79: Ensure node monitor cleanup runs even on assertion failures.
monitor_node(remote_node, false)at Line 78 is skipped if earlier asserts fail. Wrap monitored logic intry/afterto avoid leaked monitor state/messages.♻️ Suggested refactor
- :erlang.monitor_node(remote_node, true) - - dispatch_task = - Task.async(fn -> - RemoteNodeDownRouter.dispatch(command, - application: DistributedApp, - retry_attempts: 1, - timeout: 5_000 - ) - end) - - assert_receive {:remote_command_started, ^aggregate_uuid, ^remote_node}, 5_000 - - :rpc.cast(remote_node, :erlang, :halt, []) - - assert_receive {:nodedown, ^remote_node}, 5_000 - assert_receive {:remote_command_started, ^aggregate_uuid, ^current_node}, 5_000 - assert :ok = Task.await(dispatch_task, 6_000) - - :erlang.monitor_node(remote_node, false) + :erlang.monitor_node(remote_node, true) + + try do + dispatch_task = + Task.async(fn -> + RemoteNodeDownRouter.dispatch(command, + application: DistributedApp, + retry_attempts: 1, + timeout: 5_000 + ) + end) + + assert_receive {:remote_command_started, ^aggregate_uuid, ^remote_node}, 5_000 + + :rpc.cast(remote_node, :erlang, :halt, []) + + assert_receive {:nodedown, ^remote_node}, 5_000 + assert_receive {:remote_command_started, ^aggregate_uuid, ^current_node}, 5_000 + assert :ok = Task.await(dispatch_task, 6_000) + after + :erlang.monitor_node(remote_node, false) + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/commands/distributed_dispatch_test.exs` around lines 59 - 79, The test sets up an Erlang node monitor with :erlang.monitor_node(remote_node, true) but calls :erlang.monitor_node(remote_node, false) only at the end, which will be skipped on assertion failures; wrap the monitored section (from the monitor_node(true) call through Task.await) in a try ... after block so that :erlang.monitor_node(remote_node, false) always runs; locate the block around :erlang.monitor_node/2, Task.async (the dispatch_task using RemoteNodeDownRouter.dispatch), assert_receive checks, :rpc.cast(remote_node, :erlang, :halt, []), and Task.await and put the cleanup call in the after clause to ensure monitors/messages are cleaned up even on test failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@guides/explanations/commands.md`:
- Line 194: Rewrite the fragmented timeout description into a single clear
sentence: combine the two fragments so it reads something like "A command
handler has a default timeout of 5 seconds, the same as GenServer.call/3, and
must handle the command within this period; otherwise dispatch fails with an
error tuple." Update the sentence that mentions GenServer.call/3 and the
5-second default so it's one fluid line mentioning the timeout, the
GenServer.call/3 comparison, and the failure behavior.
In `@test/middleware/middleware_test.exs`:
- Around line 184-207: Test is flaky because it assumes at least one dispatch
will hit retry exhaustion; make it deterministic by polling
dispatch_concurrently until the expected {:ok, {:error, :too_many_attempts}}
appears (or timeout/fail the test after a short deadline). Update the test
"should execute middleware failure callback when dispatcher retries are
exhausted" to loop calling dispatch_concurrently (or wrap it) and check for
Enum.any?(results, &match?({:ok, {:error, :too_many_attempts}}, &1)) with a
small sleep between attempts and a total timeout (e.g., 1-2s), then assert the
final condition; keep the broader assertion that all results are either :ok or
{:error, :too_many_attempts} using the existing Enum.all? check. Ensure you
reference dispatch_concurrently and RetryExhaustionRouter.dispatch in the
updated test.
---
Outside diff comments:
In `@test/application/telemetry_test.exs`:
- Around line 108-175: The test's receive timeout in collect_dispatch_events
(currently assert_receive event, 1_000) is too tight and causes intermittent CI
flakes; increase the timeout (e.g., to 5_000 or make it a module attribute like
`@receive_timeout`) and use that constant in collect_dispatch_events so the loop
has more budget to collect [:commanded, :application, :dispatch, :stop] events;
update any test helpers (attach_telemetry / collect_dispatch_events) to
reference the new timeout constant.
---
Nitpick comments:
In `@test/commands/distributed_dispatch_test.exs`:
- Around line 59-79: The test sets up an Erlang node monitor with
:erlang.monitor_node(remote_node, true) but calls
:erlang.monitor_node(remote_node, false) only at the end, which will be skipped
on assertion failures; wrap the monitored section (from the monitor_node(true)
call through Task.await) in a try ... after block so that
:erlang.monitor_node(remote_node, false) always runs; locate the block around
:erlang.monitor_node/2, Task.async (the dispatch_task using
RemoteNodeDownRouter.dispatch), assert_receive checks, :rpc.cast(remote_node,
:erlang, :halt, []), and Task.await and put the cleanup call in the after clause
to ensure monitors/messages are cleaned up even on test failures.
In `@test/commands/support/remote_node_down_command.ex`:
- Around line 1-3: The RemoteNodeDownCommand struct allows nil for critical
fields; enforce upfront validation by either adding `@enforce_keys`
[:aggregate_uuid, :sleep_in_ms] to the Commanded.Commands.RemoteNodeDownCommand
module or by providing a constructor (e.g., new/1) that checks and raises on
missing/invalid aggregate_uuid or sleep_in_ms; ensure aggregate_uuid is present
and sleep_in_ms is a non-negative integer so malformed commands are rejected at
creation rather than later in the handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42955f18-d038-4cc8-8a51-19703f3078f9
📒 Files selected for processing (13)
guides/explanations/commands.mdlib/application.exlib/commanded/aggregates/aggregate.exlib/commanded/aggregates/execution_context.exlib/commanded/commands/dispatcher.extest/application/telemetry_test.exstest/commands/distributed_dispatch_test.exstest/commands/support/remote_node_down_aggregate.extest/commands/support/remote_node_down_command.extest/commands/support/remote_node_down_handler.extest/commands/support/remote_node_down_router.extest/middleware/middleware_test.exstest/support/distributed_test_helper.ex
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
1 participant
